[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 at 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

>   		}
>   	}
>   
>