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

...

          }
  }