Re: [PATCH RESEND] drm/mediatek: Add valid modifier check

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 03, 2023 at 08:48:56AM -0400, Justin Green wrote:
> > See c91acda3a380 ("drm/gem: Check for valid formats") and the related gem
> fb helper functions to see how this is supposed to be done.
> 
> Oh that's interesting, so does this imply that the infrastructure
> automatically calls format_mod_supported() during framebuffer
> creation? In that case, this entire patch might be unnecessary in the
> tip of tree kernel.

It /should/, but maybe a wheel fell off somewhere. So please double-check
that it doesn indeed work.

Also because we had to put the check into gem helpers, if your driver
doesn't use those but hand-rolls a bit of that code (the helpers predate a
bunch of drivers, not sure all got converted), then you might also have a
validation gap here.

Cheers, Sima

> 
> On Thu, Aug 3, 2023 at 4:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jul 24, 2023 at 01:58:39PM -0400, Justin Green wrote:
> > > Add a check to mtk_drm_mode_fb_create() that rejects any modifier that
> > > is not the AFBC mode supported by MT8195's display overlays.
> > >
> > > Tested by booting ChromeOS and verifying the UI works, and by running
> > > the ChromeOS kms_addfb_basic binary, which has a test called
> > > "addfb25-bad-modifier" that attempts to create a framebuffer with the
> > > modifier DRM_FORMAT_MOD_INVALID and verifies the ADDFB2 ioctl returns
> > > EINVAL.
> > >
> > > Signed-off-by: Justin Green <greenjustin@chromium.org>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > index cd5b18ef7951..2096e8a794ad 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > @@ -51,6 +51,13 @@ mtk_drm_mode_fb_create(struct drm_device *dev,
> > >       if (info->num_planes != 1)
> > >               return ERR_PTR(-EINVAL);
> > >
> > > +     if (cmd->modifier[0] &&
> > > +         cmd->modifier[0] != DRM_FORMAT_MOD_ARM_AFBC(
> > > +                                     AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> > > +                                     AFBC_FORMAT_MOD_SPLIT |
> > > +                                     AFBC_FORMAT_MOD_SPARSE))
> > > +             return ERR_PTR(-EINVAL);
> >
> > If you set the modifiers/format properties correctly and use all the
> > helpers then the drm core should already validate that only formats you
> > can use are allowed.
> >
> > See c91acda3a380 ("drm/gem: Check for valid formats") and the related gem
> > fb helper functions to see how this is supposed to be done. These kind of
> > checks in driver code for gem drivers (which really is everyone at this
> > point) should not be needed at all.
> >
> > Cheers, Sima
> >
> > > +
> > >       return drm_gem_fb_create(dev, file, cmd);
> > >  }
> > >
> > > --
> > > 2.41.0.162.gfafddb0af9-goog
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

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