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@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.
^^ 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
