On Wed, Jan 11, 2023 at 11:32:43AM -0300, Maíra Canal wrote:
> On 1/11/23 10:28, Ville Syrjälä wrote:
> > On Mon, Jan 02, 2023 at 03:20:06PM +0100, 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 <mca...@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.
> >
> > That was the approach I tried originally but there were a bunch of
> > problems with various drivers it at the time. Dunno if all of those
> > got sorted out or not. IIRC the idea floating around for ancient
> > drivers was to skip the check based on plane->format_default. Looks
> > like we're already using that approach in the setcrtc ioctl.
> >
>
> I ended up following Thomas's idea to check drm_drv_uses_atomic_modesetting()
> in order to check the modifier only for atomic drivers. If you have any
> feedback
> on this idea, I would be glad to hear it. The current version of this patch
> is [1].
>
> [1]
> https://lore.kernel.org/dri-devel/20230109105807.18172-1-mca...@igalia.com/T/
Yeah for atomic drivers the format list better be accurate, so we should
be able to enforce this.
For legacy driver it's a bit a mess, but I'm toying with some ideas how we
could add at least some validation there too. It's just that the audit is
a total pain :-/
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
