[PATCH] drm: Check actual format for legacy pageflip.

[Mario Kleiner]

On Thu, Jan 7, 2021 at 6:21 PM Liu, Zhan <Zhan.Liu at amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> > -----Original Message-----
> > From: Liu, Zhan
> > Sent: 2021/January/06, Wednesday 10:04 AM
> > To: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>; Mario Kleiner
> > <mario.kleiner.de at gmail.com>
> > Cc: dri-devel <dri-devel at lists.freedesktop.org>; amd-gfx list <amd-
> > gfx at lists.freedesktop.org>; Deucher, Alexander
> > <Alexander.Deucher at amd.com>; Daniel Vetter <daniel.vetter at ffwll.ch>;
> > Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Ville Syrjälä
> > <ville.syrjala at linux.intel.com>
> > Subject: RE: [PATCH] drm: Check actual format for legacy pageflip.
> >
> >
> > > -----Original Message-----
> > > From: Liu, Zhan <Zhan.Liu at amd.com>
> > > Sent: 2021/January/04, Monday 3:46 PM
> > > To: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>; Mario Kleiner
> > > <mario.kleiner.de at gmail.com>
> > > Cc: dri-devel <dri-devel at lists.freedesktop.org>; amd-gfx list <amd-
> > > gfx at lists.freedesktop.org>; Deucher, Alexander
> > > <Alexander.Deucher at amd.com>; Daniel Vetter <daniel.vetter at ffwll.ch>;
> > > Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Ville Syrjälä
> > > <ville.syrjala at linux.intel.com>
> > > Subject: Re: [PATCH] drm: Check actual format for legacy pageflip.
> > >
> > >
> > >
> > > + Ville
> > >
> > > On Sat, Jan 2, 2021 at 4:31 PM Mario Kleiner
> > > <mario.kleiner.de at gmail.com>
> > > wrote:
> > > >
> > > > On Sat, Jan 2, 2021 at 3:02 PM Bas Nieuwenhuizen
> > > > <bas at basnieuwenhuizen.nl> wrote:
> > > > >
> > > > > With modifiers one can actually have different format_info structs
> > > > > for the same format, which now matters for AMDGPU since we convert
> > > > > implicit modifiers to explicit modifiers with multiple planes.
> > > > >
> > > > > I checked other drivers and it doesn't look like they end up
> > > > > triggering this case so I think this is safe to relax.
> > > > >
> > > > > Signed-off-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
> > > > > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for
> > > > >converted metadata.")
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > > b/drivers/gpu/drm/drm_plane.c index e6231947f987..f5085990cfac
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1163,7 +1163,7 @@ int drm_mode_page_flip_ioctl(struct
> > > drm_device
> > > > >*dev,
> > > > >         if (ret)
> > > > >                 goto out;
> > > > >
> > > > > -       if (old_fb->format != fb->format) {
> > > > > +       if (old_fb->format->format != fb->format->format) {
> > > >
> > >
> > > I agree with this patch, though considering the original way was made
> > > by Ville, I will wait for Ville's input first. Adding my "Acked-by"
here.
> > >
> > > This patch is:
> > > Acked-by: Zhan Liu <zhan.liu at amd.com>
>
> Since there is no objection from the community on this patch over the
past few days, and this patch totally makes sense to me, this patch is:
>
> Reviewed-by: Zhan Liu <zhan.liu at amd.com>
>

Well, there is my alternative one-line patch, which is equally simple and
solves the problem in a similar way that doesn't undo Ville's stricter
checks, but it doesn't seem to get any attention:

https://lists.freedesktop.org/archives/dri-devel/2021-January/292763.html

Mine keeps the check as strict as before for non-atomic drivers, but
removes the check for atomic drivers, given the assumption that they should
be able to do without it.

In the end both patches solve the problem in the short term, also
satisfying my (users) needs, and the future is unknown. But it would be
nice to get an opinion from an atomic expert which one is the more future
proof / elegant / final solution to stick to in the face of potential
future atomic kms drivers

With that said, i will add to Bas patch a

Reported-by: Mario Kleiner <mario.kleiner.de at gmail.com>
Acked-by: Mario Kleiner <mario.kleiner.de at gmail.com>

thanks,
-mario

> >
> > Ping...
> >
> > >
> > > > This was btw. the original way before Ville made it more strict
> > > > about
> > > > 4 years ago, to catch issues related to tiling, and more complex
> > > > layouts, like the dcc tiling/retiling introduced by your modifier
> > > > patches. That's why I hope my alternative patch is a good solution
> > > > for atomic drivers while keeping the strictness for potential legacy
> > > > drivers.
> > > >
> > > > -mario
> > > >
> > > > >                 DRM_DEBUG_KMS("Page flip is not allowed to change
> > > > >frame buffer format.\n");
> > > > >                 ret = -EINVAL;
> > > > >                 goto out;
> > > > > --
> > > > > 2.29.2
> > > > >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20210107/f1590f41/attachment-0001.htm>