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