[PATCH] drm: Check actual format for legacy pageflip.

[PATCH] drm: Check actual format for legacy pageflip.

[Bas Nieuwenhuizen]

With modifiers one can actually have different format_info structs
for the same format, which now matters for AMDGPU since we convert
implicit modifiers to explicit modifiers with multiple planes.

I checked other drivers and it doesn't look like they end up triggering
this case so I think this is safe to relax.

Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
---
 drivers/gpu/drm/drm_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index e6231947f987..f5085990cfac 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
-	if (old_fb->format != fb->format) {
+	if (old_fb->format->format != fb->format->format) {
 		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
 		ret = -EINVAL;
 		goto out;
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Check actual format for legacy pageflip.

[Bas Nieuwenhuizen]

With modifiers one can actually have different format_info structs
for the same format, which now matters for AMDGPU since we convert
implicit modifiers to explicit modifiers with multiple planes.

I checked other drivers and it doesn't look like they end up triggering
this case so I think this is safe to relax.

Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
---
 drivers/gpu/drm/drm_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index e6231947f987..f5085990cfac 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
-	if (old_fb->format != fb->format) {
+	if (old_fb->format->format != fb->format->format) {
 		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
 		ret = -EINVAL;
 		goto out;
-- 
2.29.2

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
<bas@basnieuwenhuizen.nl> wrote:
>
> With modifiers one can actually have different format_info structs
> for the same format, which now matters for AMDGPU since we convert
> implicit modifiers to explicit modifiers with multiple planes.
>
> I checked other drivers and it doesn't look like they end up triggering
> this case so I think this is safe to relax.
>
> Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e6231947f987..f5085990cfac 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>         if (ret)
>                 goto out;
>
> -       if (old_fb->format != fb->format) {
> +       if (old_fb->format->format != fb->format->format) {

This was btw. the original way before Ville made it more strict about
4 years ago, to catch issues related to tiling, and more complex
layouts, like the dcc tiling/retiling introduced by your modifier
patches. That's why I hope my alternative patch is a good solution for
atomic drivers while keeping the strictness for potential legacy
drivers.

-mario

>                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>                 ret = -EINVAL;
>                 goto out;
> --
> 2.29.2
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
<bas@basnieuwenhuizen.nl> wrote:
>
> With modifiers one can actually have different format_info structs
> for the same format, which now matters for AMDGPU since we convert
> implicit modifiers to explicit modifiers with multiple planes.
>
> I checked other drivers and it doesn't look like they end up triggering
> this case so I think this is safe to relax.
>
> Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e6231947f987..f5085990cfac 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>         if (ret)
>                 goto out;
>
> -       if (old_fb->format != fb->format) {
> +       if (old_fb->format->format != fb->format->format) {

This was btw. the original way before Ville made it more strict about
4 years ago, to catch issues related to tiling, and more complex
layouts, like the dcc tiling/retiling introduced by your modifier
patches. That's why I hope my alternative patch is a good solution for
atomic drivers while keeping the strictness for potential legacy
drivers.

-mario

>                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>                 ret = -EINVAL;
>                 goto out;
> --
> 2.29.2
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Pierre-Eric Pelloux-Prayer]

Hi Bas,

On 02/01/2021 15:02, Bas Nieuwenhuizen wrote:
> With modifiers one can actually have different format_info structs
> for the same format, which now matters for AMDGPU since we convert
> implicit modifiers to explicit modifiers with multiple planes.
> 
> I checked other drivers and it doesn't look like they end up triggering
> this case so I think this is safe to relax.

This patch fixes https://gitlab.freedesktop.org/drm/amd/-/issues/1379:

   Tested-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Thanks!
P-E

> 
> Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e6231947f987..f5085990cfac 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Pierre-Eric Pelloux-Prayer]

Hi Bas,

On 02/01/2021 15:02, Bas Nieuwenhuizen wrote:
> With modifiers one can actually have different format_info structs
> for the same format, which now matters for AMDGPU since we convert
> implicit modifiers to explicit modifiers with multiple planes.
> 
> I checked other drivers and it doesn't look like they end up triggering
> this case so I think this is safe to relax.

This patch fixes https://gitlab.freedesktop.org/drm/amd/-/issues/1379:

   Tested-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Thanks!
P-E

> 
> Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e6231947f987..f5085990cfac 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Christian König]

Am 02.01.21 um 15:02 schrieb Bas Nieuwenhuizen:
> With modifiers one can actually have different format_info structs
> for the same format, which now matters for AMDGPU since we convert
> implicit modifiers to explicit modifiers with multiple planes.
>
> I checked other drivers and it doesn't look like they end up triggering
> this case so I think this is safe to relax.
>
> Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")

I don't know the details of the code at all, but from a high level point 
of view that makes much more sense than the original patch.

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/drm_plane.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e6231947f987..f5085990cfac 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>   	if (ret)
>   		goto out;
>   
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>   		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>   		ret = -EINVAL;
>   		goto out;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Christian König]

Am 02.01.21 um 15:02 schrieb Bas Nieuwenhuizen:
> With modifiers one can actually have different format_info structs
> for the same format, which now matters for AMDGPU since we convert
> implicit modifiers to explicit modifiers with multiple planes.
>
> I checked other drivers and it doesn't look like they end up triggering
> this case so I think this is safe to relax.
>
> Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")

I don't know the details of the code at all, but from a high level point 
of view that makes much more sense than the original patch.

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/drm_plane.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e6231947f987..f5085990cfac 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>   	if (ret)
>   		goto out;
>   
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>   		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>   		ret = -EINVAL;
>   		goto out;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Alex Deucher]

On Sat, Jan 2, 2021 at 9:02 AM Bas Nieuwenhuizen
<bas@basnieuwenhuizen.nl> wrote:
>
> With modifiers one can actually have different format_info structs
> for the same format, which now matters for AMDGPU since we convert
> implicit modifiers to explicit modifiers with multiple planes.
>
> I checked other drivers and it doesn't look like they end up triggering
> this case so I think this is safe to relax.
>
> Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e6231947f987..f5085990cfac 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>         if (ret)
>                 goto out;
>
> -       if (old_fb->format != fb->format) {
> +       if (old_fb->format->format != fb->format->format) {
>                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>                 ret = -EINVAL;
>                 goto out;
> --
> 2.29.2
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Alex Deucher]

On Sat, Jan 2, 2021 at 9:02 AM Bas Nieuwenhuizen
<bas@basnieuwenhuizen.nl> wrote:
>
> With modifiers one can actually have different format_info structs
> for the same format, which now matters for AMDGPU since we convert
> implicit modifiers to explicit modifiers with multiple planes.
>
> I checked other drivers and it doesn't look like they end up triggering
> this case so I think this is safe to relax.
>
> Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index e6231947f987..f5085990cfac 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>         if (ret)
>                 goto out;
>
> -       if (old_fb->format != fb->format) {
> +       if (old_fb->format->format != fb->format->format) {
>                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>                 ret = -EINVAL;
>                 goto out;
> --
> 2.29.2
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Bas Nieuwenhuizen]

+ Ville

On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>
> On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> <bas@basnieuwenhuizen.nl> wrote:
> >
> > With modifiers one can actually have different format_info structs
> > for the same format, which now matters for AMDGPU since we convert
> > implicit modifiers to explicit modifiers with multiple planes.
> >
> > I checked other drivers and it doesn't look like they end up triggering
> > this case so I think this is safe to relax.
> >
> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index e6231947f987..f5085990cfac 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >         if (ret)
> >                 goto out;
> >
> > -       if (old_fb->format != fb->format) {
> > +       if (old_fb->format->format != fb->format->format) {
>
> This was btw. the original way before Ville made it more strict about
> 4 years ago, to catch issues related to tiling, and more complex
> layouts, like the dcc tiling/retiling introduced by your modifier
> patches. That's why I hope my alternative patch is a good solution for
> atomic drivers while keeping the strictness for potential legacy
> drivers.
>
> -mario
>
> >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >                 ret = -EINVAL;
> >                 goto out;
> > --
> > 2.29.2
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Bas Nieuwenhuizen]

+ Ville

On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>
> On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> <bas@basnieuwenhuizen.nl> wrote:
> >
> > With modifiers one can actually have different format_info structs
> > for the same format, which now matters for AMDGPU since we convert
> > implicit modifiers to explicit modifiers with multiple planes.
> >
> > I checked other drivers and it doesn't look like they end up triggering
> > this case so I think this is safe to relax.
> >
> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index e6231947f987..f5085990cfac 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >         if (ret)
> >                 goto out;
> >
> > -       if (old_fb->format != fb->format) {
> > +       if (old_fb->format->format != fb->format->format) {
>
> This was btw. the original way before Ville made it more strict about
> 4 years ago, to catch issues related to tiling, and more complex
> layouts, like the dcc tiling/retiling introduced by your modifier
> patches. That's why I hope my alternative patch is a good solution for
> atomic drivers while keeping the strictness for potential legacy
> drivers.
>
> -mario
>
> >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >                 ret = -EINVAL;
> >                 goto out;
> > --
> > 2.29.2
> >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Liu, Zhan]

[AMD Official Use Only - Internal Distribution Only]

+ Ville

On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>
> On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> <bas@basnieuwenhuizen.nl> wrote:
> >
> > With modifiers one can actually have different format_info structs
> > for the same format, which now matters for AMDGPU since we convert
> > implicit modifiers to explicit modifiers with multiple planes.
> >
> > I checked other drivers and it doesn't look like they end up triggering
> > this case so I think this is safe to relax.
> >
> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index e6231947f987..f5085990cfac 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >         if (ret)
> >                 goto out;
> >
> > -       if (old_fb->format != fb->format) {
> > +       if (old_fb->format->format != fb->format->format) {
>

I agree with this patch, though considering the original way was made by Ville, I will wait for Ville's input first. Adding my "Acked-by" here.

This patch is:
Acked-by: Zhan Liu <zhan.liu@amd.com>

> This was btw. the original way before Ville made it more strict about
> 4 years ago, to catch issues related to tiling, and more complex
> layouts, like the dcc tiling/retiling introduced by your modifier
> patches. That's why I hope my alternative patch is a good solution for
> atomic drivers while keeping the strictness for potential legacy
> drivers.
>
> -mario
>
> >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >                 ret = -EINVAL;
> >                 goto out;
> > --
> > 2.29.2
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Liu, Zhan]

[AMD Official Use Only - Internal Distribution Only]

+ Ville

On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>
> On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> <bas@basnieuwenhuizen.nl> wrote:
> >
> > With modifiers one can actually have different format_info structs
> > for the same format, which now matters for AMDGPU since we convert
> > implicit modifiers to explicit modifiers with multiple planes.
> >
> > I checked other drivers and it doesn't look like they end up triggering
> > this case so I think this is safe to relax.
> >
> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index e6231947f987..f5085990cfac 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >         if (ret)
> >                 goto out;
> >
> > -       if (old_fb->format != fb->format) {
> > +       if (old_fb->format->format != fb->format->format) {
>

I agree with this patch, though considering the original way was made by Ville, I will wait for Ville's input first. Adding my "Acked-by" here.

This patch is:
Acked-by: Zhan Liu <zhan.liu@amd.com>

> This was btw. the original way before Ville made it more strict about
> 4 years ago, to catch issues related to tiling, and more complex
> layouts, like the dcc tiling/retiling introduced by your modifier
> patches. That's why I hope my alternative patch is a good solution for
> atomic drivers while keeping the strictness for potential legacy
> drivers.
>
> -mario
>
> >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >                 ret = -EINVAL;
> >                 goto out;
> > --
> > 2.29.2
> >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm: Check actual format for legacy pageflip.

[Liu, Zhan]

[AMD Official Use Only - Internal Distribution Only]

> -----Original Message-----
> From: Liu, Zhan <Zhan.Liu@amd.com>
> Sent: 2021/January/04, Monday 3:46 PM
> To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> <mario.kleiner.de@gmail.com>
> Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> gfx@lists.freedesktop.org>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Subject: Re: [PATCH] drm: Check actual format for legacy pageflip.
>
>
>
> + Ville
>
> On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner <mario.kleiner.de@gmail.com>
> wrote:
> >
> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > <bas@basnieuwenhuizen.nl> wrote:
> > >
> > > With modifiers one can actually have different format_info structs
> > > for the same format, which now matters for AMDGPU since we convert
> > > implicit modifiers to explicit modifiers with multiple planes.
> > >
> > > I checked other drivers and it doesn't look like they end up
> > > triggering this case so I think this is safe to relax.
> > >
> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> > >converted metadata.")
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > b/drivers/gpu/drm/drm_plane.c index e6231947f987..f5085990cfac
> > > 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> drm_device
> > >*dev,
> > >         if (ret)
> > >                 goto out;
> > >
> > > -       if (old_fb->format != fb->format) {
> > > +       if (old_fb->format->format != fb->format->format) {
> >
>
> I agree with this patch, though considering the original way was made by
> Ville, I will wait for Ville's input first. Adding my "Acked-by" here.
>
> This patch is:
> Acked-by: Zhan Liu <zhan.liu@amd.com>

Ping...

>
> > This was btw. the original way before Ville made it more strict about
> > 4 years ago, to catch issues related to tiling, and more complex
> > layouts, like the dcc tiling/retiling introduced by your modifier
> > patches. That's why I hope my alternative patch is a good solution for
> > atomic drivers while keeping the strictness for potential legacy
> > drivers.
> >
> > -mario
> >
> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> > >frame buffer format.\n");
> > >                 ret = -EINVAL;
> > >                 goto out;
> > > --
> > > 2.29.2
> > >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm: Check actual format for legacy pageflip.

[Liu, Zhan]

[AMD Official Use Only - Internal Distribution Only]

> -----Original Message-----
> From: Liu, Zhan <Zhan.Liu@amd.com>
> Sent: 2021/January/04, Monday 3:46 PM
> To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> <mario.kleiner.de@gmail.com>
> Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> gfx@lists.freedesktop.org>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Subject: Re: [PATCH] drm: Check actual format for legacy pageflip.
>
>
>
> + Ville
>
> On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner <mario.kleiner.de@gmail.com>
> wrote:
> >
> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > <bas@basnieuwenhuizen.nl> wrote:
> > >
> > > With modifiers one can actually have different format_info structs
> > > for the same format, which now matters for AMDGPU since we convert
> > > implicit modifiers to explicit modifiers with multiple planes.
> > >
> > > I checked other drivers and it doesn't look like they end up
> > > triggering this case so I think this is safe to relax.
> > >
> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> > >converted metadata.")
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > b/drivers/gpu/drm/drm_plane.c index e6231947f987..f5085990cfac
> > > 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> drm_device
> > >*dev,
> > >         if (ret)
> > >                 goto out;
> > >
> > > -       if (old_fb->format != fb->format) {
> > > +       if (old_fb->format->format != fb->format->format) {
> >
>
> I agree with this patch, though considering the original way was made by
> Ville, I will wait for Ville's input first. Adding my "Acked-by" here.
>
> This patch is:
> Acked-by: Zhan Liu <zhan.liu@amd.com>

Ping...

>
> > This was btw. the original way before Ville made it more strict about
> > 4 years ago, to catch issues related to tiling, and more complex
> > layouts, like the dcc tiling/retiling introduced by your modifier
> > patches. That's why I hope my alternative patch is a good solution for
> > atomic drivers while keeping the strictness for potential legacy
> > drivers.
> >
> > -mario
> >
> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> > >frame buffer format.\n");
> > >                 ret = -EINVAL;
> > >                 goto out;
> > > --
> > > 2.29.2
> > >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm: Check actual format for legacy pageflip.

[Liu, Zhan]

[AMD Official Use Only - Internal Distribution Only]

> -----Original Message-----
> From: Liu, Zhan
> Sent: 2021/January/06, Wednesday 10:04 AM
> To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> <mario.kleiner.de@gmail.com>
> Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> gfx@lists.freedesktop.org>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Subject: RE: [PATCH] drm: Check actual format for legacy pageflip.
>
>
> > -----Original Message-----
> > From: Liu, Zhan <Zhan.Liu@amd.com>
> > Sent: 2021/January/04, Monday 3:46 PM
> > To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> > <mario.kleiner.de@gmail.com>
> > Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> > gfx@lists.freedesktop.org>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> > Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> > <ville.syrjala@linux.intel.com>
> > Subject: Re: [PATCH] drm: Check actual format for legacy pageflip.
> >
> >
> >
> > + Ville
> >
> > On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner
> > <mario.kleiner.de@gmail.com>
> > wrote:
> > >
> > > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > > <bas@basnieuwenhuizen.nl> wrote:
> > > >
> > > > With modifiers one can actually have different format_info structs
> > > > for the same format, which now matters for AMDGPU since we convert
> > > > implicit modifiers to explicit modifiers with multiple planes.
> > > >
> > > > I checked other drivers and it doesn't look like they end up
> > > > triggering this case so I think this is safe to relax.
> > > >
> > > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> > > >converted metadata.")
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > b/drivers/gpu/drm/drm_plane.c index e6231947f987..f5085990cfac
> > > > 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> > drm_device
> > > >*dev,
> > > >         if (ret)
> > > >                 goto out;
> > > >
> > > > -       if (old_fb->format != fb->format) {
> > > > +       if (old_fb->format->format != fb->format->format) {
> > >
> >
> > I agree with this patch, though considering the original way was made
> > by Ville, I will wait for Ville's input first. Adding my "Acked-by" here.
> >
> > This patch is:
> > Acked-by: Zhan Liu <zhan.liu@amd.com>

Since there is no objection from the community on this patch over the past few days, and this patch totally makes sense to me, this patch is:

Reviewed-by: Zhan Liu <zhan.liu@amd.com>

>
> Ping...
>
> >
> > > This was btw. the original way before Ville made it more strict
> > > about
> > > 4 years ago, to catch issues related to tiling, and more complex
> > > layouts, like the dcc tiling/retiling introduced by your modifier
> > > patches. That's why I hope my alternative patch is a good solution
> > > for atomic drivers while keeping the strictness for potential legacy
> > > drivers.
> > >
> > > -mario
> > >
> > > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> > > >frame buffer format.\n");
> > > >                 ret = -EINVAL;
> > > >                 goto out;
> > > > --
> > > > 2.29.2
> > > >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm: Check actual format for legacy pageflip.

[Liu, Zhan]

[AMD Official Use Only - Internal Distribution Only]

> -----Original Message-----
> From: Liu, Zhan
> Sent: 2021/January/06, Wednesday 10:04 AM
> To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> <mario.kleiner.de@gmail.com>
> Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> gfx@lists.freedesktop.org>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Subject: RE: [PATCH] drm: Check actual format for legacy pageflip.
>
>
> > -----Original Message-----
> > From: Liu, Zhan <Zhan.Liu@amd.com>
> > Sent: 2021/January/04, Monday 3:46 PM
> > To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> > <mario.kleiner.de@gmail.com>
> > Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> > gfx@lists.freedesktop.org>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> > Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> > <ville.syrjala@linux.intel.com>
> > Subject: Re: [PATCH] drm: Check actual format for legacy pageflip.
> >
> >
> >
> > + Ville
> >
> > On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner
> > <mario.kleiner.de@gmail.com>
> > wrote:
> > >
> > > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > > <bas@basnieuwenhuizen.nl> wrote:
> > > >
> > > > With modifiers one can actually have different format_info structs
> > > > for the same format, which now matters for AMDGPU since we convert
> > > > implicit modifiers to explicit modifiers with multiple planes.
> > > >
> > > > I checked other drivers and it doesn't look like they end up
> > > > triggering this case so I think this is safe to relax.
> > > >
> > > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> > > >converted metadata.")
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > b/drivers/gpu/drm/drm_plane.c index e6231947f987..f5085990cfac
> > > > 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> > drm_device
> > > >*dev,
> > > >         if (ret)
> > > >                 goto out;
> > > >
> > > > -       if (old_fb->format != fb->format) {
> > > > +       if (old_fb->format->format != fb->format->format) {
> > >
> >
> > I agree with this patch, though considering the original way was made
> > by Ville, I will wait for Ville's input first. Adding my "Acked-by" here.
> >
> > This patch is:
> > Acked-by: Zhan Liu <zhan.liu@amd.com>

Since there is no objection from the community on this patch over the past few days, and this patch totally makes sense to me, this patch is:

Reviewed-by: Zhan Liu <zhan.liu@amd.com>

>
> Ping...
>
> >
> > > This was btw. the original way before Ville made it more strict
> > > about
> > > 4 years ago, to catch issues related to tiling, and more complex
> > > layouts, like the dcc tiling/retiling introduced by your modifier
> > > patches. That's why I hope my alternative patch is a good solution
> > > for atomic drivers while keeping the strictness for potential legacy
> > > drivers.
> > >
> > > -mario
> > >
> > > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> > > >frame buffer format.\n");
> > > >                 ret = -EINVAL;
> > > >                 goto out;
> > > > --
> > > > 2.29.2
> > > >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Daniel Vetter]

On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
> On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> <bas@basnieuwenhuizen.nl> wrote:
> >
> > With modifiers one can actually have different format_info structs
> > for the same format, which now matters for AMDGPU since we convert
> > implicit modifiers to explicit modifiers with multiple planes.
> >
> > I checked other drivers and it doesn't look like they end up triggering
> > this case so I think this is safe to relax.
> >
> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index e6231947f987..f5085990cfac 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >         if (ret)
> >                 goto out;
> >
> > -       if (old_fb->format != fb->format) {
> > +       if (old_fb->format->format != fb->format->format) {
> 
> This was btw. the original way before Ville made it more strict about
> 4 years ago, to catch issues related to tiling, and more complex
> layouts, like the dcc tiling/retiling introduced by your modifier
> patches. That's why I hope my alternative patch is a good solution for
> atomic drivers while keeping the strictness for potential legacy
> drivers.

Yeah this doesn't work in full generality, because hw might need to do a
full modeset to do a full modeset to reallocate resources (like scanout
fifo space) if the format changes.

But for atomic drivers that should be caught in ->atomic_check, which
should result in -EINVAL, so should do the right thing. So it should be
all good, but imo needs a comment to explain what's going on:

	/*
	 * Only check the FOURCC format code, excluding modifiers. This is
	 * enough for all legacy drivers. Atomic drivers have their own
	 * checks in their ->atomic_check implementation, which will
	 * return -EINVAL if any hw or driver constraint is violated due
	 * to modifier changes.
	 */

Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?

With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> -mario
> 
> >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >                 ret = -EINVAL;
> >                 goto out;
> > --
> > 2.29.2
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Daniel Vetter]

On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
> On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> <bas@basnieuwenhuizen.nl> wrote:
> >
> > With modifiers one can actually have different format_info structs
> > for the same format, which now matters for AMDGPU since we convert
> > implicit modifiers to explicit modifiers with multiple planes.
> >
> > I checked other drivers and it doesn't look like they end up triggering
> > this case so I think this is safe to relax.
> >
> > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index e6231947f987..f5085990cfac 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >         if (ret)
> >                 goto out;
> >
> > -       if (old_fb->format != fb->format) {
> > +       if (old_fb->format->format != fb->format->format) {
> 
> This was btw. the original way before Ville made it more strict about
> 4 years ago, to catch issues related to tiling, and more complex
> layouts, like the dcc tiling/retiling introduced by your modifier
> patches. That's why I hope my alternative patch is a good solution for
> atomic drivers while keeping the strictness for potential legacy
> drivers.

Yeah this doesn't work in full generality, because hw might need to do a
full modeset to do a full modeset to reallocate resources (like scanout
fifo space) if the format changes.

But for atomic drivers that should be caught in ->atomic_check, which
should result in -EINVAL, so should do the right thing. So it should be
all good, but imo needs a comment to explain what's going on:

	/*
	 * Only check the FOURCC format code, excluding modifiers. This is
	 * enough for all legacy drivers. Atomic drivers have their own
	 * checks in their ->atomic_check implementation, which will
	 * return -EINVAL if any hw or driver constraint is violated due
	 * to modifier changes.
	 */

Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?

With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> -mario
> 
> >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >                 ret = -EINVAL;
> >                 goto out;
> > --
> > 2.29.2
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

[-- Attachment #1.1: Type: text/plain, Size: 4947 bytes --]

On Thu, Jan 7, 2021 at 6:21 PM Liu, Zhan <Zhan.Liu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> > -----Original Message-----
> > From: Liu, Zhan
> > Sent: 2021/January/06, Wednesday 10:04 AM
> > To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> > <mario.kleiner.de@gmail.com>
> > Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> > gfx@lists.freedesktop.org>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> > Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> > <ville.syrjala@linux.intel.com>
> > Subject: RE: [PATCH] drm: Check actual format for legacy pageflip.
> >
> >
> > > -----Original Message-----
> > > From: Liu, Zhan <Zhan.Liu@amd.com>
> > > Sent: 2021/January/04, Monday 3:46 PM
> > > To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> > > <mario.kleiner.de@gmail.com>
> > > Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> > > gfx@lists.freedesktop.org>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> > > Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> > > <ville.syrjala@linux.intel.com>
> > > Subject: Re: [PATCH] drm: Check actual format for legacy pageflip.
> > >
> > >
> > >
> > > + Ville
> > >
> > > On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner
> > > <mario.kleiner.de@gmail.com>
> > > wrote:
> > > >
> > > > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > > > <bas@basnieuwenhuizen.nl> wrote:
> > > > >
> > > > > With modifiers one can actually have different format_info structs
> > > > > for the same format, which now matters for AMDGPU since we convert
> > > > > implicit modifiers to explicit modifiers with multiple planes.
> > > > >
> > > > > I checked other drivers and it doesn't look like they end up
> > > > > triggering this case so I think this is safe to relax.
> > > > >
> > > > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> > > > >converted metadata.")
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > > b/drivers/gpu/drm/drm_plane.c index e6231947f987..f5085990cfac
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> > > drm_device
> > > > >*dev,
> > > > >         if (ret)
> > > > >                 goto out;
> > > > >
> > > > > -       if (old_fb->format != fb->format) {
> > > > > +       if (old_fb->format->format != fb->format->format) {
> > > >
> > >
> > > I agree with this patch, though considering the original way was made
> > > by Ville, I will wait for Ville's input first. Adding my "Acked-by"
here.
> > >
> > > This patch is:
> > > Acked-by: Zhan Liu <zhan.liu@amd.com>
>
> Since there is no objection from the community on this patch over the
past few days, and this patch totally makes sense to me, this patch is:
>
> Reviewed-by: Zhan Liu <zhan.liu@amd.com>
>

Well, there is my alternative one-line patch, which is equally simple and
solves the problem in a similar way that doesn't undo Ville's stricter
checks, but it doesn't seem to get any attention:

https://lists.freedesktop.org/archives/dri-devel/2021-January/292763.html

Mine keeps the check as strict as before for non-atomic drivers, but
removes the check for atomic drivers, given the assumption that they should
be able to do without it.

In the end both patches solve the problem in the short term, also
satisfying my (users) needs, and the future is unknown. But it would be
nice to get an opinion from an atomic expert which one is the more future
proof / elegant / final solution to stick to in the face of potential
future atomic kms drivers

With that said, i will add to Bas patch a

Reported-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Acked-by: Mario Kleiner <mario.kleiner.de@gmail.com>

thanks,
-mario

> >
> > Ping...
> >
> > >
> > > > This was btw. the original way before Ville made it more strict
> > > > about
> > > > 4 years ago, to catch issues related to tiling, and more complex
> > > > layouts, like the dcc tiling/retiling introduced by your modifier
> > > > patches. That's why I hope my alternative patch is a good solution
> > > > for atomic drivers while keeping the strictness for potential legacy
> > > > drivers.
> > > >
> > > > -mario
> > > >
> > > > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> > > > >frame buffer format.\n");
> > > > >                 ret = -EINVAL;
> > > > >                 goto out;
> > > > > --
> > > > > 2.29.2
> > > > >

[-- Attachment #1.2: Type: text/html, Size: 7830 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

[-- Attachment #1.1: Type: text/plain, Size: 4947 bytes --]

On Thu, Jan 7, 2021 at 6:21 PM Liu, Zhan <Zhan.Liu@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> > -----Original Message-----
> > From: Liu, Zhan
> > Sent: 2021/January/06, Wednesday 10:04 AM
> > To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> > <mario.kleiner.de@gmail.com>
> > Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> > gfx@lists.freedesktop.org>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> > Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> > <ville.syrjala@linux.intel.com>
> > Subject: RE: [PATCH] drm: Check actual format for legacy pageflip.
> >
> >
> > > -----Original Message-----
> > > From: Liu, Zhan <Zhan.Liu@amd.com>
> > > Sent: 2021/January/04, Monday 3:46 PM
> > > To: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>; Mario Kleiner
> > > <mario.kleiner.de@gmail.com>
> > > Cc: dri-devel <dri-devel@lists.freedesktop.org>; amd-gfx list <amd-
> > > gfx@lists.freedesktop.org>; Deucher, Alexander
> > > <Alexander.Deucher@amd.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> > > Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Ville Syrjälä
> > > <ville.syrjala@linux.intel.com>
> > > Subject: Re: [PATCH] drm: Check actual format for legacy pageflip.
> > >
> > >
> > >
> > > + Ville
> > >
> > > On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner
> > > <mario.kleiner.de@gmail.com>
> > > wrote:
> > > >
> > > > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > > > <bas@basnieuwenhuizen.nl> wrote:
> > > > >
> > > > > With modifiers one can actually have different format_info structs
> > > > > for the same format, which now matters for AMDGPU since we convert
> > > > > implicit modifiers to explicit modifiers with multiple planes.
> > > > >
> > > > > I checked other drivers and it doesn't look like they end up
> > > > > triggering this case so I think this is safe to relax.
> > > > >
> > > > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> > > > >converted metadata.")
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > > b/drivers/gpu/drm/drm_plane.c index e6231947f987..f5085990cfac
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> > > drm_device
> > > > >*dev,
> > > > >         if (ret)
> > > > >                 goto out;
> > > > >
> > > > > -       if (old_fb->format != fb->format) {
> > > > > +       if (old_fb->format->format != fb->format->format) {
> > > >
> > >
> > > I agree with this patch, though considering the original way was made
> > > by Ville, I will wait for Ville's input first. Adding my "Acked-by"
here.
> > >
> > > This patch is:
> > > Acked-by: Zhan Liu <zhan.liu@amd.com>
>
> Since there is no objection from the community on this patch over the
past few days, and this patch totally makes sense to me, this patch is:
>
> Reviewed-by: Zhan Liu <zhan.liu@amd.com>
>

Well, there is my alternative one-line patch, which is equally simple and
solves the problem in a similar way that doesn't undo Ville's stricter
checks, but it doesn't seem to get any attention:

https://lists.freedesktop.org/archives/dri-devel/2021-January/292763.html

Mine keeps the check as strict as before for non-atomic drivers, but
removes the check for atomic drivers, given the assumption that they should
be able to do without it.

In the end both patches solve the problem in the short term, also
satisfying my (users) needs, and the future is unknown. But it would be
nice to get an opinion from an atomic expert which one is the more future
proof / elegant / final solution to stick to in the face of potential
future atomic kms drivers

With that said, i will add to Bas patch a

Reported-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Acked-by: Mario Kleiner <mario.kleiner.de@gmail.com>

thanks,
-mario

> >
> > Ping...
> >
> > >
> > > > This was btw. the original way before Ville made it more strict
> > > > about
> > > > 4 years ago, to catch issues related to tiling, and more complex
> > > > layouts, like the dcc tiling/retiling introduced by your modifier
> > > > patches. That's why I hope my alternative patch is a good solution
> > > > for atomic drivers while keeping the strictness for potential legacy
> > > > drivers.
> > > >
> > > > -mario
> > > >
> > > > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> > > > >frame buffer format.\n");
> > > > >                 ret = -EINVAL;
> > > > >                 goto out;
> > > > > --
> > > > > 2.29.2
> > > > >

[-- Attachment #1.2: Type: text/html, Size: 7830 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

[-- Attachment #1.1: Type: text/plain, Size: 3051 bytes --]

On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > <bas@basnieuwenhuizen.nl> wrote:
> > >
> > > With modifiers one can actually have different format_info structs
> > > for the same format, which now matters for AMDGPU since we convert
> > > implicit modifiers to explicit modifiers with multiple planes.
> > >
> > > I checked other drivers and it doesn't look like they end up triggering
> > > this case so I think this is safe to relax.
> > >
> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> converted metadata.")
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index e6231947f987..f5085990cfac 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device
> *dev,
> > >         if (ret)
> > >                 goto out;
> > >
> > > -       if (old_fb->format != fb->format) {
> > > +       if (old_fb->format->format != fb->format->format) {
> >
> > This was btw. the original way before Ville made it more strict about
> > 4 years ago, to catch issues related to tiling, and more complex
> > layouts, like the dcc tiling/retiling introduced by your modifier
> > patches. That's why I hope my alternative patch is a good solution for
> > atomic drivers while keeping the strictness for potential legacy
> > drivers.
>
> Yeah this doesn't work in full generality, because hw might need to do a
> full modeset to do a full modeset to reallocate resources (like scanout
> fifo space) if the format changes.
>
> But for atomic drivers that should be caught in ->atomic_check, which
> should result in -EINVAL, so should do the right thing. So it should be
> all good, but imo needs a comment to explain what's going on:
>
>         /*
>          * Only check the FOURCC format code, excluding modifiers. This is
>          * enough for all legacy drivers. Atomic drivers have their own
>          * checks in their ->atomic_check implementation, which will
>          * return -EINVAL if any hw or driver constraint is violated due
>          * to modifier changes.
>          */
>
> Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?
>
> With that:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>
Ah, my "atomic expert", posting simultaneously with myself :). Happy new
year. Opinions on my variant, just replied a minute ago?

thanks,
-mario

>
> > -mario
> >
> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> frame buffer format.\n");
> > >                 ret = -EINVAL;
> > >                 goto out;
> > > --
> > > 2.29.2
> > >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #1.2: Type: text/html, Size: 4372 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

[-- Attachment #1.1: Type: text/plain, Size: 3051 bytes --]

On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > <bas@basnieuwenhuizen.nl> wrote:
> > >
> > > With modifiers one can actually have different format_info structs
> > > for the same format, which now matters for AMDGPU since we convert
> > > implicit modifiers to explicit modifiers with multiple planes.
> > >
> > > I checked other drivers and it doesn't look like they end up triggering
> > > this case so I think this is safe to relax.
> > >
> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> converted metadata.")
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index e6231947f987..f5085990cfac 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device
> *dev,
> > >         if (ret)
> > >                 goto out;
> > >
> > > -       if (old_fb->format != fb->format) {
> > > +       if (old_fb->format->format != fb->format->format) {
> >
> > This was btw. the original way before Ville made it more strict about
> > 4 years ago, to catch issues related to tiling, and more complex
> > layouts, like the dcc tiling/retiling introduced by your modifier
> > patches. That's why I hope my alternative patch is a good solution for
> > atomic drivers while keeping the strictness for potential legacy
> > drivers.
>
> Yeah this doesn't work in full generality, because hw might need to do a
> full modeset to do a full modeset to reallocate resources (like scanout
> fifo space) if the format changes.
>
> But for atomic drivers that should be caught in ->atomic_check, which
> should result in -EINVAL, so should do the right thing. So it should be
> all good, but imo needs a comment to explain what's going on:
>
>         /*
>          * Only check the FOURCC format code, excluding modifiers. This is
>          * enough for all legacy drivers. Atomic drivers have their own
>          * checks in their ->atomic_check implementation, which will
>          * return -EINVAL if any hw or driver constraint is violated due
>          * to modifier changes.
>          */
>
> Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?
>
> With that:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>
Ah, my "atomic expert", posting simultaneously with myself :). Happy new
year. Opinions on my variant, just replied a minute ago?

thanks,
-mario

>
> > -mario
> >
> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> frame buffer format.\n");
> > >                 ret = -EINVAL;
> > >                 goto out;
> > > --
> > > 2.29.2
> > >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #1.2: Type: text/html, Size: 4372 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Daniel Vetter]

On Thu, Jan 7, 2021 at 7:00 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>
> On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
>> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
>> > <bas@basnieuwenhuizen.nl> wrote:
>> > >
>> > > With modifiers one can actually have different format_info structs
>> > > for the same format, which now matters for AMDGPU since we convert
>> > > implicit modifiers to explicit modifiers with multiple planes.
>> > >
>> > > I checked other drivers and it doesn't look like they end up triggering
>> > > this case so I think this is safe to relax.
>> > >
>> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
>> > > ---
>> > >  drivers/gpu/drm/drm_plane.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> > > index e6231947f987..f5085990cfac 100644
>> > > --- a/drivers/gpu/drm/drm_plane.c
>> > > +++ b/drivers/gpu/drm/drm_plane.c
>> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>> > >         if (ret)
>> > >                 goto out;
>> > >
>> > > -       if (old_fb->format != fb->format) {
>> > > +       if (old_fb->format->format != fb->format->format) {
>> >
>> > This was btw. the original way before Ville made it more strict about
>> > 4 years ago, to catch issues related to tiling, and more complex
>> > layouts, like the dcc tiling/retiling introduced by your modifier
>> > patches. That's why I hope my alternative patch is a good solution for
>> > atomic drivers while keeping the strictness for potential legacy
>> > drivers.
>>
>> Yeah this doesn't work in full generality, because hw might need to do a
>> full modeset to do a full modeset to reallocate resources (like scanout
>> fifo space) if the format changes.
>>
>> But for atomic drivers that should be caught in ->atomic_check, which
>> should result in -EINVAL, so should do the right thing. So it should be
>> all good, but imo needs a comment to explain what's going on:
>>
>>         /*
>>          * Only check the FOURCC format code, excluding modifiers. This is
>>          * enough for all legacy drivers. Atomic drivers have their own
>>          * checks in their ->atomic_check implementation, which will
>>          * return -EINVAL if any hw or driver constraint is violated due
>>          * to modifier changes.
>>          */
>>
>> Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?
>>
>> With that:
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>
> Ah, my "atomic expert", posting simultaneously with myself :). Happy new year. Opinions on my variant, just replied a minute ago?

Full disclosure, Ville wanted to do something similar since forever
I'm not a huge fan of removing limitations of legacy ioctls. Worst
case we break something, best case no gain in features since why don't
you just use atomic. Since this (amdgpu modifiers) broke something we
have to fix it, hence I'd go with the more minimal version from Bas
here.

But in general your patch should be correct too.
-Daniel

>
> thanks,
> -mario
>
>> >
>> > -mario
>> >
>> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>> > >                 ret = -EINVAL;
>> > >                 goto out;
>> > > --
>> > > 2.29.2
>> > >
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Daniel Vetter]

On Thu, Jan 7, 2021 at 7:00 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>
> On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
>> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
>> > <bas@basnieuwenhuizen.nl> wrote:
>> > >
>> > > With modifiers one can actually have different format_info structs
>> > > for the same format, which now matters for AMDGPU since we convert
>> > > implicit modifiers to explicit modifiers with multiple planes.
>> > >
>> > > I checked other drivers and it doesn't look like they end up triggering
>> > > this case so I think this is safe to relax.
>> > >
>> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
>> > > ---
>> > >  drivers/gpu/drm/drm_plane.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> > > index e6231947f987..f5085990cfac 100644
>> > > --- a/drivers/gpu/drm/drm_plane.c
>> > > +++ b/drivers/gpu/drm/drm_plane.c
>> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>> > >         if (ret)
>> > >                 goto out;
>> > >
>> > > -       if (old_fb->format != fb->format) {
>> > > +       if (old_fb->format->format != fb->format->format) {
>> >
>> > This was btw. the original way before Ville made it more strict about
>> > 4 years ago, to catch issues related to tiling, and more complex
>> > layouts, like the dcc tiling/retiling introduced by your modifier
>> > patches. That's why I hope my alternative patch is a good solution for
>> > atomic drivers while keeping the strictness for potential legacy
>> > drivers.
>>
>> Yeah this doesn't work in full generality, because hw might need to do a
>> full modeset to do a full modeset to reallocate resources (like scanout
>> fifo space) if the format changes.
>>
>> But for atomic drivers that should be caught in ->atomic_check, which
>> should result in -EINVAL, so should do the right thing. So it should be
>> all good, but imo needs a comment to explain what's going on:
>>
>>         /*
>>          * Only check the FOURCC format code, excluding modifiers. This is
>>          * enough for all legacy drivers. Atomic drivers have their own
>>          * checks in their ->atomic_check implementation, which will
>>          * return -EINVAL if any hw or driver constraint is violated due
>>          * to modifier changes.
>>          */
>>
>> Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?
>>
>> With that:
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>
> Ah, my "atomic expert", posting simultaneously with myself :). Happy new year. Opinions on my variant, just replied a minute ago?

Full disclosure, Ville wanted to do something similar since forever
I'm not a huge fan of removing limitations of legacy ioctls. Worst
case we break something, best case no gain in features since why don't
you just use atomic. Since this (amdgpu modifiers) broke something we
have to fix it, hence I'd go with the more minimal version from Bas
here.

But in general your patch should be correct too.
-Daniel

>
> thanks,
> -mario
>
>> >
>> > -mario
>> >
>> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>> > >                 ret = -EINVAL;
>> > >                 goto out;
>> > > --
>> > > 2.29.2
>> > >
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

[-- Attachment #1.1: Type: text/plain, Size: 4710 bytes --]

On Thu, Jan 7, 2021 at 7:04 PM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jan 7, 2021 at 7:00 PM Mario Kleiner <mario.kleiner.de@gmail.com>
> wrote:
> >
> > On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
> >> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> >> > <bas@basnieuwenhuizen.nl> wrote:
> >> > >
> >> > > With modifiers one can actually have different format_info structs
> >> > > for the same format, which now matters for AMDGPU since we convert
> >> > > implicit modifiers to explicit modifiers with multiple planes.
> >> > >
> >> > > I checked other drivers and it doesn't look like they end up
> triggering
> >> > > this case so I think this is safe to relax.
> >> > >
> >> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> >> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> converted metadata.")
> >> > > ---
> >> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_plane.c
> b/drivers/gpu/drm/drm_plane.c
> >> > > index e6231947f987..f5085990cfac 100644
> >> > > --- a/drivers/gpu/drm/drm_plane.c
> >> > > +++ b/drivers/gpu/drm/drm_plane.c
> >> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> drm_device *dev,
> >> > >         if (ret)
> >> > >                 goto out;
> >> > >
> >> > > -       if (old_fb->format != fb->format) {
> >> > > +       if (old_fb->format->format != fb->format->format) {
> >> >
> >> > This was btw. the original way before Ville made it more strict about
> >> > 4 years ago, to catch issues related to tiling, and more complex
> >> > layouts, like the dcc tiling/retiling introduced by your modifier
> >> > patches. That's why I hope my alternative patch is a good solution for
> >> > atomic drivers while keeping the strictness for potential legacy
> >> > drivers.
> >>
> >> Yeah this doesn't work in full generality, because hw might need to do a
> >> full modeset to do a full modeset to reallocate resources (like scanout
> >> fifo space) if the format changes.
> >>
> >> But for atomic drivers that should be caught in ->atomic_check, which
> >> should result in -EINVAL, so should do the right thing. So it should be
> >> all good, but imo needs a comment to explain what's going on:
> >>
> >>         /*
> >>          * Only check the FOURCC format code, excluding modifiers. This
> is
> >>          * enough for all legacy drivers. Atomic drivers have their own
> >>          * checks in their ->atomic_check implementation, which will
> >>          * return -EINVAL if any hw or driver constraint is violated due
> >>          * to modifier changes.
> >>          */
> >>
> >> Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?
> >>
> >> With that:
> >>
> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >
> > Ah, my "atomic expert", posting simultaneously with myself :). Happy new
> year. Opinions on my variant, just replied a minute ago?
>
> Full disclosure, Ville wanted to do something similar since forever
> I'm not a huge fan of removing limitations of legacy ioctls. Worst
> case we break something, best case no gain in features since why don't
> you just use atomic. Since this (amdgpu modifiers) broke something we
> have to fix it, hence I'd go with the more minimal version from Bas
> here.
>
>
Fair point. Means though that somebody will have to convert many user-space
clients, e.g., all OSS Vulkan drivers. And XOrg could not do that, as the
kernel uabi even blocks use of atomic drmSetClientCap(...ATOMIC...) for any
process whose taskname starts with 'X', as a workaround for a
modesetting-ddx with broken atomic implementation. So at least for (pun
ahead) "X" applications, atomic modesetting is not an option.

For my use cases, X11/XOrg native is still the only display server capable
enough to fulfill the needs, although I'm mixing in a bit of
Vulkan/WSI/DirectDisplay for direct DRM/KMS access to work around some
limitations, e.g., to get HDR or fp16 support.

But in general your patch should be correct too.
> -Daniel
>
>
Thanks for the feedback. I rest my case.
-mario


> >
> > thanks,
> > -mario
> >
> >> >
> >> > -mario
> >> >
> >> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> frame buffer format.\n");
> >> > >                 ret = -EINVAL;
> >> > >                 goto out;
> >> > > --
> >> > > 2.29.2
> >> > >
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #1.2: Type: text/html, Size: 6960 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

[-- Attachment #1.1: Type: text/plain, Size: 4710 bytes --]

On Thu, Jan 7, 2021 at 7:04 PM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jan 7, 2021 at 7:00 PM Mario Kleiner <mario.kleiner.de@gmail.com>
> wrote:
> >
> > On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
> >> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> >> > <bas@basnieuwenhuizen.nl> wrote:
> >> > >
> >> > > With modifiers one can actually have different format_info structs
> >> > > for the same format, which now matters for AMDGPU since we convert
> >> > > implicit modifiers to explicit modifiers with multiple planes.
> >> > >
> >> > > I checked other drivers and it doesn't look like they end up
> triggering
> >> > > this case so I think this is safe to relax.
> >> > >
> >> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> >> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> converted metadata.")
> >> > > ---
> >> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_plane.c
> b/drivers/gpu/drm/drm_plane.c
> >> > > index e6231947f987..f5085990cfac 100644
> >> > > --- a/drivers/gpu/drm/drm_plane.c
> >> > > +++ b/drivers/gpu/drm/drm_plane.c
> >> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> drm_device *dev,
> >> > >         if (ret)
> >> > >                 goto out;
> >> > >
> >> > > -       if (old_fb->format != fb->format) {
> >> > > +       if (old_fb->format->format != fb->format->format) {
> >> >
> >> > This was btw. the original way before Ville made it more strict about
> >> > 4 years ago, to catch issues related to tiling, and more complex
> >> > layouts, like the dcc tiling/retiling introduced by your modifier
> >> > patches. That's why I hope my alternative patch is a good solution for
> >> > atomic drivers while keeping the strictness for potential legacy
> >> > drivers.
> >>
> >> Yeah this doesn't work in full generality, because hw might need to do a
> >> full modeset to do a full modeset to reallocate resources (like scanout
> >> fifo space) if the format changes.
> >>
> >> But for atomic drivers that should be caught in ->atomic_check, which
> >> should result in -EINVAL, so should do the right thing. So it should be
> >> all good, but imo needs a comment to explain what's going on:
> >>
> >>         /*
> >>          * Only check the FOURCC format code, excluding modifiers. This
> is
> >>          * enough for all legacy drivers. Atomic drivers have their own
> >>          * checks in their ->atomic_check implementation, which will
> >>          * return -EINVAL if any hw or driver constraint is violated due
> >>          * to modifier changes.
> >>          */
> >>
> >> Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?
> >>
> >> With that:
> >>
> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >
> > Ah, my "atomic expert", posting simultaneously with myself :). Happy new
> year. Opinions on my variant, just replied a minute ago?
>
> Full disclosure, Ville wanted to do something similar since forever
> I'm not a huge fan of removing limitations of legacy ioctls. Worst
> case we break something, best case no gain in features since why don't
> you just use atomic. Since this (amdgpu modifiers) broke something we
> have to fix it, hence I'd go with the more minimal version from Bas
> here.
>
>
Fair point. Means though that somebody will have to convert many user-space
clients, e.g., all OSS Vulkan drivers. And XOrg could not do that, as the
kernel uabi even blocks use of atomic drmSetClientCap(...ATOMIC...) for any
process whose taskname starts with 'X', as a workaround for a
modesetting-ddx with broken atomic implementation. So at least for (pun
ahead) "X" applications, atomic modesetting is not an option.

For my use cases, X11/XOrg native is still the only display server capable
enough to fulfill the needs, although I'm mixing in a bit of
Vulkan/WSI/DirectDisplay for direct DRM/KMS access to work around some
limitations, e.g., to get HDR or fp16 support.

But in general your patch should be correct too.
> -Daniel
>
>
Thanks for the feedback. I rest my case.
-mario


> >
> > thanks,
> > -mario
> >
> >> >
> >> > -mario
> >> >
> >> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> frame buffer format.\n");
> >> > >                 ret = -EINVAL;
> >> > >                 goto out;
> >> > > --
> >> > > 2.29.2
> >> > >
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #1.2: Type: text/html, Size: 6960 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Daniel Vetter]

On Thu, Jan 7, 2021 at 7:16 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>
> On Thu, Jan 7, 2021 at 7:04 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, Jan 7, 2021 at 7:00 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>> >
>> > On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>> >>
>> >> On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
>> >> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
>> >> > <bas@basnieuwenhuizen.nl> wrote:
>> >> > >
>> >> > > With modifiers one can actually have different format_info structs
>> >> > > for the same format, which now matters for AMDGPU since we convert
>> >> > > implicit modifiers to explicit modifiers with multiple planes.
>> >> > >
>> >> > > I checked other drivers and it doesn't look like they end up triggering
>> >> > > this case so I think this is safe to relax.
>> >> > >
>> >> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>> >> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
>> >> > > ---
>> >> > >  drivers/gpu/drm/drm_plane.c | 2 +-
>> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> > >
>> >> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> >> > > index e6231947f987..f5085990cfac 100644
>> >> > > --- a/drivers/gpu/drm/drm_plane.c
>> >> > > +++ b/drivers/gpu/drm/drm_plane.c
>> >> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>> >> > >         if (ret)
>> >> > >                 goto out;
>> >> > >
>> >> > > -       if (old_fb->format != fb->format) {
>> >> > > +       if (old_fb->format->format != fb->format->format) {
>> >> >
>> >> > This was btw. the original way before Ville made it more strict about
>> >> > 4 years ago, to catch issues related to tiling, and more complex
>> >> > layouts, like the dcc tiling/retiling introduced by your modifier
>> >> > patches. That's why I hope my alternative patch is a good solution for
>> >> > atomic drivers while keeping the strictness for potential legacy
>> >> > drivers.
>> >>
>> >> Yeah this doesn't work in full generality, because hw might need to do a
>> >> full modeset to do a full modeset to reallocate resources (like scanout
>> >> fifo space) if the format changes.
>> >>
>> >> But for atomic drivers that should be caught in ->atomic_check, which
>> >> should result in -EINVAL, so should do the right thing. So it should be
>> >> all good, but imo needs a comment to explain what's going on:
>> >>
>> >>         /*
>> >>          * Only check the FOURCC format code, excluding modifiers. This is
>> >>          * enough for all legacy drivers. Atomic drivers have their own
>> >>          * checks in their ->atomic_check implementation, which will
>> >>          * return -EINVAL if any hw or driver constraint is violated due
>> >>          * to modifier changes.
>> >>          */
>> >>
>> >> Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?
>> >>
>> >> With that:
>> >>
>> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>
>> >
>> > Ah, my "atomic expert", posting simultaneously with myself :). Happy new year. Opinions on my variant, just replied a minute ago?
>>
>> Full disclosure, Ville wanted to do something similar since forever
>> I'm not a huge fan of removing limitations of legacy ioctls. Worst
>> case we break something, best case no gain in features since why don't
>> you just use atomic. Since this (amdgpu modifiers) broke something we
>> have to fix it, hence I'd go with the more minimal version from Bas
>> here.
>>
>
> Fair point. Means though that somebody will have to convert many user-space clients, e.g., all OSS Vulkan drivers. And XOrg could not do that, as the kernel uabi even blocks use of atomic drmSetClientCap(...ATOMIC...) for any process whose taskname starts with 'X', as a workaround for a modesetting-ddx with broken atomic implementation. So at least for (pun ahead) "X" applications, atomic modesetting is not an option.

If you ask for atomic v2 in the setclientcap ioctl you'll get atomic
even if you're X. The issue is no one's caring enough to fix it up
Xorg atomic support to make that happen.

And yes the vulkan drivers should attempt at least some atomic, the
reason for that was that amdgpu didn't have atomic back when the
original code was typed. Maybe it'll happen with more modifier use,
now that both amdgpu and i915 support them.
-Daniel

> For my use cases, X11/XOrg native is still the only display server capable enough to fulfill the needs, although I'm mixing in a bit of Vulkan/WSI/DirectDisplay for direct DRM/KMS access to work around some limitations, e.g., to get HDR or fp16 support.
>
>> But in general your patch should be correct too.
>> -Daniel
>>
>
> Thanks for the feedback. I rest my case.
> -mario
>
>>
>> >
>> > thanks,
>> > -mario
>> >
>> >> >
>> >> > -mario
>> >> >
>> >> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>> >> > >                 ret = -EINVAL;
>> >> > >                 goto out;
>> >> > > --
>> >> > > 2.29.2
>> >> > >
>> >>
>> >> --
>> >> Daniel Vetter
>> >> Software Engineer, Intel Corporation
>> >> http://blog.ffwll.ch
>>
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Check actual format for legacy pageflip.

[Daniel Vetter]

On Thu, Jan 7, 2021 at 7:16 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>
> On Thu, Jan 7, 2021 at 7:04 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, Jan 7, 2021 at 7:00 PM Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
>> >
>> > On Thu, Jan 7, 2021 at 6:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>> >>
>> >> On Sat, Jan 02, 2021 at 04:31:36PM +0100, Mario Kleiner wrote:
>> >> > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
>> >> > <bas@basnieuwenhuizen.nl> wrote:
>> >> > >
>> >> > > With modifiers one can actually have different format_info structs
>> >> > > for the same format, which now matters for AMDGPU since we convert
>> >> > > implicit modifiers to explicit modifiers with multiple planes.
>> >> > >
>> >> > > I checked other drivers and it doesn't look like they end up triggering
>> >> > > this case so I think this is safe to relax.
>> >> > >
>> >> > > Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>> >> > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
>> >> > > ---
>> >> > >  drivers/gpu/drm/drm_plane.c | 2 +-
>> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> > >
>> >> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> >> > > index e6231947f987..f5085990cfac 100644
>> >> > > --- a/drivers/gpu/drm/drm_plane.c
>> >> > > +++ b/drivers/gpu/drm/drm_plane.c
>> >> > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>> >> > >         if (ret)
>> >> > >                 goto out;
>> >> > >
>> >> > > -       if (old_fb->format != fb->format) {
>> >> > > +       if (old_fb->format->format != fb->format->format) {
>> >> >
>> >> > This was btw. the original way before Ville made it more strict about
>> >> > 4 years ago, to catch issues related to tiling, and more complex
>> >> > layouts, like the dcc tiling/retiling introduced by your modifier
>> >> > patches. That's why I hope my alternative patch is a good solution for
>> >> > atomic drivers while keeping the strictness for potential legacy
>> >> > drivers.
>> >>
>> >> Yeah this doesn't work in full generality, because hw might need to do a
>> >> full modeset to do a full modeset to reallocate resources (like scanout
>> >> fifo space) if the format changes.
>> >>
>> >> But for atomic drivers that should be caught in ->atomic_check, which
>> >> should result in -EINVAL, so should do the right thing. So it should be
>> >> all good, but imo needs a comment to explain what's going on:
>> >>
>> >>         /*
>> >>          * Only check the FOURCC format code, excluding modifiers. This is
>> >>          * enough for all legacy drivers. Atomic drivers have their own
>> >>          * checks in their ->atomic_check implementation, which will
>> >>          * return -EINVAL if any hw or driver constraint is violated due
>> >>          * to modifier changes.
>> >>          */
>> >>
>> >> Also can you pls cc: intel-gfx to get this vetted by the intel-gfx ci?
>> >>
>> >> With that:
>> >>
>> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>
>> >
>> > Ah, my "atomic expert", posting simultaneously with myself :). Happy new year. Opinions on my variant, just replied a minute ago?
>>
>> Full disclosure, Ville wanted to do something similar since forever
>> I'm not a huge fan of removing limitations of legacy ioctls. Worst
>> case we break something, best case no gain in features since why don't
>> you just use atomic. Since this (amdgpu modifiers) broke something we
>> have to fix it, hence I'd go with the more minimal version from Bas
>> here.
>>
>
> Fair point. Means though that somebody will have to convert many user-space clients, e.g., all OSS Vulkan drivers. And XOrg could not do that, as the kernel uabi even blocks use of atomic drmSetClientCap(...ATOMIC...) for any process whose taskname starts with 'X', as a workaround for a modesetting-ddx with broken atomic implementation. So at least for (pun ahead) "X" applications, atomic modesetting is not an option.

If you ask for atomic v2 in the setclientcap ioctl you'll get atomic
even if you're X. The issue is no one's caring enough to fix it up
Xorg atomic support to make that happen.

And yes the vulkan drivers should attempt at least some atomic, the
reason for that was that amdgpu didn't have atomic back when the
original code was typed. Maybe it'll happen with more modifier use,
now that both amdgpu and i915 support them.
-Daniel

> For my use cases, X11/XOrg native is still the only display server capable enough to fulfill the needs, although I'm mixing in a bit of Vulkan/WSI/DirectDisplay for direct DRM/KMS access to work around some limitations, e.g., to get HDR or fp16 support.
>
>> But in general your patch should be correct too.
>> -Daniel
>>
>
> Thanks for the feedback. I rest my case.
> -mario
>
>>
>> >
>> > thanks,
>> > -mario
>> >
>> >> >
>> >> > -mario
>> >> >
>> >> > >                 DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>> >> > >                 ret = -EINVAL;
>> >> > >                 goto out;
>> >> > > --
>> >> > > 2.29.2
>> >> > >
>> >>
>> >> --
>> >> Daniel Vetter
>> >> Software Engineer, Intel Corporation
>> >> http://blog.ffwll.ch
>>
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx