[PATCH] drm/amdgpu: Do not change amdgpu framebuffer format during page flip

[PATCH] drm/amdgpu: Do not change amdgpu framebuffer format during page flip

[Zhan Liu]

[Why]
Driver cannot change amdgpu framebuffer (afb) format while doing
page flip. Force system doing so will cause ioctl error, and result in
breaking several functionalities including FreeSync.

If afb format is forced to change during page flip, following message
will appear in dmesg.log:

"[drm:drm_mode_page_flip_ioctl [drm]]
Page flip is not allowed to change frame buffer format."

[How]
Do not change afb format while doing page flip. It is okay to check
whether afb format is valid here, however, forcing afb format change
shouldn't happen here.

Signed-off-by: Zhan Liu <zhan.liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index a638709e9c92..0efebd592b65 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -831,8 +831,6 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
 								modifier);
 			if (!format_info)
 				return -EINVAL;
-
-			afb->base.format = format_info;
 		}
 	}
 
-- 
2.25.1

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

Re: [PATCH] drm/amdgpu: Do not change amdgpu framebuffer format during page flip

[Kazlauskas, Nicholas]

On 2020-12-21 10:18 p.m., Zhan Liu wrote:
> [Why]
> Driver cannot change amdgpu framebuffer (afb) format while doing
> page flip. Force system doing so will cause ioctl error, and result in
> breaking several functionalities including FreeSync.
> 
> If afb format is forced to change during page flip, following message
> will appear in dmesg.log:
> 
> "[drm:drm_mode_page_flip_ioctl [drm]]
> Page flip is not allowed to change frame buffer format."
> 
> [How]
> Do not change afb format while doing page flip. It is okay to check
> whether afb format is valid here, however, forcing afb format change
> shouldn't happen here.
> 
> Signed-off-by: Zhan Liu <zhan.liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index a638709e9c92..0efebd592b65 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -831,8 +831,6 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
>   								modifier);
>   			if (!format_info)
>   				return -EINVAL;
> -
> -			afb->base.format = format_info;

Adding Bas for comment since he added the modifiers conversion, but I'll 
leave my own thoughts below.

This patch is a NAK from me - the framebuffer is still expected to be in 
a specific format/tiling configuration and ignoring the incoming format 
doesn't resolve the problem.

The problem is that the legacy page IOCTL has this check in the first 
place expecting that no driver is capable of performing this action.

This is not the case for amdgpu (be it DC enabled or not), so I think 
it's best to have a driver cap here or some new driver hook to validate 
that the flip is valid.

This is legacy code, and in the shared path, so I don't know how others 
in the list feel about modifying this but I think we do expect that 
legacy userspace can do this from the X side of things.

I recall seeing this happen going from DCC disabled to non DCC enabled 
buffers and some of this functionality being behind a version check in 
xf86-video-amdgpu.

Regards,
Nicholas Kazlauskas

>   		}
>   	}
>   
> 

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

Re: [PATCH] drm/amdgpu: Do not change amdgpu framebuffer format during page flip

[Kazlauskas, Nicholas]

On 2020-12-21 10:18 p.m., Zhan Liu wrote:
> [Why]
> Driver cannot change amdgpu framebuffer (afb) format while doing
> page flip. Force system doing so will cause ioctl error, and result in
> breaking several functionalities including FreeSync.
> 
> If afb format is forced to change during page flip, following message
> will appear in dmesg.log:
> 
> "[drm:drm_mode_page_flip_ioctl [drm]]
> Page flip is not allowed to change frame buffer format."
> 
> [How]
> Do not change afb format while doing page flip. It is okay to check
> whether afb format is valid here, however, forcing afb format change
> shouldn't happen here.
> 
> Signed-off-by: Zhan Liu <zhan.liu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index a638709e9c92..0efebd592b65 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -831,8 +831,6 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
>   								modifier);
>   			if (!format_info)
>   				return -EINVAL;
> -
> -			afb->base.format = format_info;

Adding Bas for comment since he added the modifiers conversion, but I'll 
leave my own thoughts below.

This patch is a NAK from me - the framebuffer is still expected to be in 
a specific format/tiling configuration and ignoring the incoming format 
doesn't resolve the problem.

The problem is that the legacy page IOCTL has this check in the first 
place expecting that no driver is capable of performing this action.

This is not the case for amdgpu (be it DC enabled or not), so I think 
it's best to have a driver cap here or some new driver hook to validate 
that the flip is valid.

This is legacy code, and in the shared path, so I don't know how others 
in the list feel about modifying this but I think we do expect that 
legacy userspace can do this from the X side of things.

I recall seeing this happen going from DCC disabled to non DCC enabled 
buffers and some of this functionality being behind a version check in 
xf86-video-amdgpu.

Regards,
Nicholas Kazlauskas

>   		}
>   	}
>   
> 

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

Re: [PATCH] drm/amdgpu: Do not change amdgpu framebuffer format during page flip

[Bas Nieuwenhuizen]

On Tue, Dec 22, 2020 at 9:55 PM Kazlauskas, Nicholas
<nicholas.kazlauskas@amd.com> wrote:
>
> On 2020-12-21 10:18 p.m., Zhan Liu wrote:
> > [Why]
> > Driver cannot change amdgpu framebuffer (afb) format while doing
> > page flip. Force system doing so will cause ioctl error, and result in
> > breaking several functionalities including FreeSync.
> >
> > If afb format is forced to change during page flip, following message
> > will appear in dmesg.log:
> >
> > "[drm:drm_mode_page_flip_ioctl [drm]]
> > Page flip is not allowed to change frame buffer format."
> >
> > [How]
> > Do not change afb format while doing page flip. It is okay to check
> > whether afb format is valid here, however, forcing afb format change
> > shouldn't happen here.
> >
> > Signed-off-by: Zhan Liu <zhan.liu@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 --
> >   1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index a638709e9c92..0efebd592b65 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -831,8 +831,6 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
> >                                                               modifier);
> >                       if (!format_info)
> >                               return -EINVAL;
> > -
> > -                     afb->base.format = format_info;
>
> Adding Bas for comment since he added the modifiers conversion, but I'll
> leave my own thoughts below.
>
> This patch is a NAK from me - the framebuffer is still expected to be in
> a specific format/tiling configuration and ignoring the incoming format
> doesn't resolve the problem.
>
> The problem is that the legacy page IOCTL has this check in the first
> place expecting that no driver is capable of performing this action.
>
> This is not the case for amdgpu (be it DC enabled or not), so I think
> it's best to have a driver cap here or some new driver hook to validate
> that the flip is valid.
>
> This is legacy code, and in the shared path, so I don't know how others
> in the list feel about modifying this but I think we do expect that
> legacy userspace can do this from the X side of things.

I think the "new" thing is that we can have different format_info
structs for the same drm fourcc (as we need a different number of
planes depending on modifier). It would probably make sense to relax
this check to check the actual drm fourcc (i.e. fb->format->format)
instead of the format_info pointer. This case would likely only be hit
in the AMDGPU driver anyway (with intel being the other possibility).


>
> I recall seeing this happen going from DCC disabled to non DCC enabled
> buffers and some of this functionality being behind a version check in
> xf86-video-amdgpu.
>
> Regards,
> Nicholas Kazlauskas
>
> >               }
> >       }
> >
> >
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdgpu: Do not change amdgpu framebuffer format during page flip

[Bas Nieuwenhuizen]

On Tue, Dec 22, 2020 at 9:55 PM Kazlauskas, Nicholas
<nicholas.kazlauskas@amd.com> wrote:
>
> On 2020-12-21 10:18 p.m., Zhan Liu wrote:
> > [Why]
> > Driver cannot change amdgpu framebuffer (afb) format while doing
> > page flip. Force system doing so will cause ioctl error, and result in
> > breaking several functionalities including FreeSync.
> >
> > If afb format is forced to change during page flip, following message
> > will appear in dmesg.log:
> >
> > "[drm:drm_mode_page_flip_ioctl [drm]]
> > Page flip is not allowed to change frame buffer format."
> >
> > [How]
> > Do not change afb format while doing page flip. It is okay to check
> > whether afb format is valid here, however, forcing afb format change
> > shouldn't happen here.
> >
> > Signed-off-by: Zhan Liu <zhan.liu@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 --
> >   1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index a638709e9c92..0efebd592b65 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -831,8 +831,6 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
> >                                                               modifier);
> >                       if (!format_info)
> >                               return -EINVAL;
> > -
> > -                     afb->base.format = format_info;
>
> Adding Bas for comment since he added the modifiers conversion, but I'll
> leave my own thoughts below.
>
> This patch is a NAK from me - the framebuffer is still expected to be in
> a specific format/tiling configuration and ignoring the incoming format
> doesn't resolve the problem.
>
> The problem is that the legacy page IOCTL has this check in the first
> place expecting that no driver is capable of performing this action.
>
> This is not the case for amdgpu (be it DC enabled or not), so I think
> it's best to have a driver cap here or some new driver hook to validate
> that the flip is valid.
>
> This is legacy code, and in the shared path, so I don't know how others
> in the list feel about modifying this but I think we do expect that
> legacy userspace can do this from the X side of things.

I think the "new" thing is that we can have different format_info
structs for the same drm fourcc (as we need a different number of
planes depending on modifier). It would probably make sense to relax
this check to check the actual drm fourcc (i.e. fb->format->format)
instead of the format_info pointer. This case would likely only be hit
in the AMDGPU driver anyway (with intel being the other possibility).


>
> I recall seeing this happen going from DCC disabled to non DCC enabled
> buffers and some of this functionality being behind a version check in
> xf86-video-amdgpu.
>
> Regards,
> Nicholas Kazlauskas
>
> >               }
> >       }
> >
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Do not change amdgpu framebuffer format during page flip

[Bas Nieuwenhuizen]

https://lists.freedesktop.org/archives/dri-devel/2021-January/292761.html
is my alternative patch.

On Tue, Dec 22, 2020 at 4:18 AM Zhan Liu <zhan.liu@amd.com> wrote:
>
> [Why]
> Driver cannot change amdgpu framebuffer (afb) format while doing
> page flip. Force system doing so will cause ioctl error, and result in
> breaking several functionalities including FreeSync.
>
> If afb format is forced to change during page flip, following message
> will appear in dmesg.log:
>
> "[drm:drm_mode_page_flip_ioctl [drm]]
> Page flip is not allowed to change frame buffer format."
>
> [How]
> Do not change afb format while doing page flip. It is okay to check
> whether afb format is valid here, however, forcing afb format change
> shouldn't happen here.
>
> Signed-off-by: Zhan Liu <zhan.liu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index a638709e9c92..0efebd592b65 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -831,8 +831,6 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
>                                                                 modifier);
>                         if (!format_info)
>                                 return -EINVAL;
> -
> -                       afb->base.format = format_info;
>                 }
>         }
>
> --
> 2.25.1
>
> _______________________________________________
> 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