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

[Daniel Vetter <daniel@ffwll.ch>]

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