On Tue, Dec 22, 2020 at 9:55 PM Kazlauskas, Nicholas <nicholas.kazlauskas@xxxxxxx> 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@xxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
