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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 02, 2023 at 04:21:55PM +0100, 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@igalia.com>
> > > > ---
> > > >   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.

^^  this sounds like a good idea. Also note that for these checks the
right check is actually drm_drv_uses_atomic_modesetting, since it's about
the driver interface. DRIVER_ATOMIC is more about whether the driver
interface actually upholds the atomic/tearfree guarantees userspace
expects.
-Daniel

> 
> 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.)
> 
> 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




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch