Re: [PATCH] drm/vc4: Check for valid formats

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 02.01.23 um 16:39 schrieb Maíra Canal:
> On 1/2/23 12:21, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 02.01.23 um 15:29 schrieb Maíra Canal:
> > > Hi,
> > >
> > > On 1/2/23 11:20, Thomas Zimmermann wrote:
> > > > Hi
> > > >
> > > > Am 02.01.23 um 14:57 schrieb Maíra Canal:
> > > > > Currently, vc4 is not checking valid formats before creating a
> > > > > framebuffer, which is triggering the IGT test
> > > > > igt@kms_addfb_basic@addfb25-bad-modifier to fail, as vc4 accepts
> > > > > to create a framebuffer with an invalid modifier. Therefore, check
> > > > > for valid formats before creating framebuffers on vc4 and vc5 in
> > > > > order to avoid creating framebuffers with invalid formats.
> > > > >
> > > > > Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
> > > > > ---
> > > > >   drivers/gpu/drm/vc4/vc4_kms.c | 23 ++++++++++++++++++++++-
> > > > >   1 file changed, 22 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c 
> > > > > b/drivers/gpu/drm/vc4/vc4_kms.c
> > > > > index 53d9f30460cf..5d1afd66fcc1 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > > > > @@ -500,6 +500,27 @@ static struct drm_framebuffer 
> > > > > *vc4_fb_create(struct drm_device *dev,
> > > > >         mode_cmd = &mode_cmd_local;
> > > > >     }
> > > > >
> > > > > +    if (!drm_any_plane_has_format(dev, mode_cmd->pixel_format,
> > > > > +                      mode_cmd->modifier[0])) {
> > > > > +        drm_dbg_kms(dev, "Unsupported pixel format %p4cc / 
> > > > > modifier 0x%llx\n",
> > > > > +                &mode_cmd->pixel_format, mode_cmd->modifier[0]);
> > > > > +        return ERR_PTR(-EINVAL);
> > > > > +    }
> > > >
> > > > This might be a stupid question, but why doesn't 
> > > > drm_fbgem_fb_create() do this test already? It seems like a 
> > > > no-brainer and *not* testing for the plane formats should be the 
> > > > exception.
> > >
> > > I thought the same initially, but then I found this mention on the 
> > > TODO list [1]. So, it
> > > is not possible to test it on drm_gem_fb_create() because, for 
> > > non-atomic, checking
> > > drm_any_plane_has_format() is not possible since like the format list 
> > > for the primary plane
> > > is fake and we'd therefor reject valid formats.
> >
> > Thanks for the pointer to the docs.
> >
> > I see two options:
> >
> > 1) Testing for atomic modesetting can be done via 
> > drm_core_check_feature(dev, DRIVER_ATOMIC).  If true, 
> > drm_gem_fb_create() can further test for the plane formats. For 
> > non-atomic drivers, just skip the format test.
> >
> > 2) As an alternative, we could invert the IGT test and explicitly 
> > allow any format to be allocated. Almost no drivers currently bother 
> > with the format test anyway. And DRM will already fail if userspace 
> > attaches a framebuffer to a plane with incompatible formats. [1]  (I'd 
> > personally prefer this option, but I'm not sure if there's any 
> > consensus on that.)
>
> I guess the second option will probably break Intel's CI, which I'm not 
> sure if it is a good
> idea. Maybe the first option is a bit less intrusive and will help the 
> DRM helper to have the
> same behavior as drivers like i915 and AMDGPU.

That makes some sense. I'd like to hear the opinion of the affected 
developers. If no one shows up, go for option 1 then. It has the 
potential to break someone's userspace code, but it's easily revert-able.

Best regards
Thomas

> Thanks for the suggestions!
>
> Best Regards,
> - Maíra Canal
>
> > With either implemented, the TODO item could be remvoed AFAICT.
> >
> > Best regards
> > Thomas
> >
> > [1] 
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_plane.c#L789
> >
> > > I'm not sure if anything changed since this was written, but that was 
> > > the reason that I
> > > decided to introduce the check in the driver instead of the API.
> > >
> > > [1] 
> > > https://cgit.freedesktop.org/drm/drm/tree/Documentation/gpu/todo.rst#n279
> > >
> > > Best Regards,
> > > - Maíra Canal
> > >
> > > > Best regards
> > > > Thomas
> > > >
> > > > > +
> > > > > +    return drm_gem_fb_create(dev, file_priv, mode_cmd);
> > > > > +}
> > > > > +
> > > > > +static struct drm_framebuffer *vc5_fb_create(struct drm_device *dev,
> > > > > +                         struct drm_file *file_priv,
> > > > > +                         const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > > +{
> > > > > +    if (!drm_any_plane_has_format(dev, mode_cmd->pixel_format,
> > > > > +                      mode_cmd->modifier[0])) {
> > > > > +        drm_dbg_kms(dev, "Unsupported pixel format %p4cc / 
> > > > > modifier 0x%llx\n",
> > > > > +                &mode_cmd->pixel_format, mode_cmd->modifier[0]);
> > > > > +        return ERR_PTR(-EINVAL);
> > > > > +    }
> > > > > +
> > > > >     return drm_gem_fb_create(dev, file_priv, mode_cmd);
> > > > >   }
> > > > >
> > > > > @@ -1033,7 +1054,7 @@ static const struct drm_mode_config_funcs 
> > > > > vc4_mode_funcs = {
> > > > >   static const struct drm_mode_config_funcs vc5_mode_funcs = {
> > > > >     .atomic_check = vc4_atomic_check,
> > > > >     .atomic_commit = drm_atomic_helper_commit,
> > > > > -    .fb_create = drm_gem_fb_create,
> > > > > +    .fb_create = vc5_fb_create,
> > > > >   };
> > > > >
> > > > >   int vc4_kms_load(struct drm_device *dev)
> > > > > --
> > > > > 2.38.1

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature