> 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.
On Thu, Aug 3, 2023 at 4:24 AM Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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
