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

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

[Justin Green]

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);
+
 	return drm_gem_fb_create(dev, file, cmd);
 }
 
-- 
2.41.0.162.gfafddb0af9-goog

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

[AngeloGioacchino Del Regno]

Il 24/07/23 19:58, Justin Green ha scritto:
> 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);

Would it make more sense to commmonize function mtk_plane_format_mod_supported()
and call that one here instead?

Regards,
Angelo

> +
>   	return drm_gem_fb_create(dev, file, cmd);
>   }
>   

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

[Fei Shao]

On Tue, Jul 25, 2023 at 1:59 AM Justin Green <greenjustin@chromium.org> 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>

Tested-by: Fei Shao <fshao@chromium.org>

Tested the UI and with the aforementioned binary on a MT8195
Chromebook (AFBC supported) and a MT8188 Chromebook (AFBC not
supported at this moment).

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

[Justin Green]

> Would it make more sense to commmonize function mtk_plane_format_mod_supported()
> and call that one here instead?
I had considered that, but mtk_plane_format_mod_supported() is
required to take a drm_plane as a parameter in order to conform to the
type signature defined in drm_plane_funcs, but
mtk_drm_mode_fb_create() does not have a drm_plane to provide, since
the framebuffer is created later in the function. Technically we don't
actually use the drm_plane in the implementation of
mtk_plane_format_mod_supported() today, so we could just use a null
pointer, but I figured we may one day need to add per-plane logic.

> This is not DRM_FORMAT_MOD_INVALID. Please either explicitly compare against INVALID if that's what you meant, or against LINEAR if that's what you meant, or both.
Ack, I meant to use LINEAR. Will update for the next version of the patch.

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

[AngeloGioacchino Del Regno]

Il 26/07/23 21:44, Justin Green ha scritto:
>> Would it make more sense to commmonize function mtk_plane_format_mod_supported()
>> and call that one here instead?
> I had considered that, but mtk_plane_format_mod_supported() is
> required to take a drm_plane as a parameter in order to conform to the
> type signature defined in drm_plane_funcs, but
> mtk_drm_mode_fb_create() does not have a drm_plane to provide, since
> the framebuffer is created later in the function. Technically we don't
> actually use the drm_plane in the implementation of
> mtk_plane_format_mod_supported() today, so we could just use a null
> pointer, but I figured we may one day need to add per-plane logic.
> 

My suggestion was not to use that function as-is, but rather to add a helper like

bool mtk_format_modifier_supported(u32 format, u32 modifier) { ... }

...so that a per-plane logic in mtk_drm_plane can be easily added, because...

static bool mtk_plane_format_mod_supported(struct drm_plane *plane,
					   u32 format, u32 modifier)
{
	return mtk_format_modifier_supported(format, modifier);
}

so apart from that, is there any other reason to not do that? :-)

Regards,
Angelo

>> This is not DRM_FORMAT_MOD_INVALID. Please either explicitly compare against INVALID if that's what you meant, or against LINEAR if that's what you meant, or both.
> Ack, I meant to use LINEAR. Will update for the next version of the patch.

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

[Justin Green]

> ...so that a per-plane logic in mtk_drm_plane can be easily added, because...
I think my concern is more that if we need to validate the format and
the modifier differently because of the plane data, then this method
would provide limited value. For example, on my MT8195, plane ID 38
supports AR30 and BA30, but plane ID 50 does not support any 10-bit
pixel formats.

On Thu, Jul 27, 2023 at 5:37 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 26/07/23 21:44, Justin Green ha scritto:
> >> Would it make more sense to commmonize function mtk_plane_format_mod_supported()
> >> and call that one here instead?
> > I had considered that, but mtk_plane_format_mod_supported() is
> > required to take a drm_plane as a parameter in order to conform to the
> > type signature defined in drm_plane_funcs, but
> > mtk_drm_mode_fb_create() does not have a drm_plane to provide, since
> > the framebuffer is created later in the function. Technically we don't
> > actually use the drm_plane in the implementation of
> > mtk_plane_format_mod_supported() today, so we could just use a null
> > pointer, but I figured we may one day need to add per-plane logic.
> >
>
> My suggestion was not to use that function as-is, but rather to add a helper like
>
> bool mtk_format_modifier_supported(u32 format, u32 modifier) { ... }
>
> ...so that a per-plane logic in mtk_drm_plane can be easily added, because...
>
> static bool mtk_plane_format_mod_supported(struct drm_plane *plane,
>                                            u32 format, u32 modifier)
> {
>         return mtk_format_modifier_supported(format, modifier);
> }
>
> so apart from that, is there any other reason to not do that? :-)
>
> Regards,
> Angelo
>
> >> This is not DRM_FORMAT_MOD_INVALID. Please either explicitly compare against INVALID if that's what you meant, or against LINEAR if that's what you meant, or both.
> > Ack, I meant to use LINEAR. Will update for the next version of the patch.
>
>

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

[Daniel Vetter]

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

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

[Justin Green]

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

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

[Daniel Vetter]

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

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

[Fei Shao]

On Thu, Aug 3, 2023 at 10:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> 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.

Thanks for shedding the light, Sima.
I think that change is exactly what we need here, which is somehow
recent and we don't have that in our downstream kernels.
Applying that also fixes the test failure I saw, so I agree with you
that this patch is not needed for the mainline kernel.

Thanks,
Fei


> >
> > 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