| Message ID | 20230724175839.675911-1-greenjustin@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] drm/mediatek: Add valid modifier check | expand |
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); > } >
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).
> 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.
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.
> ...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. > >
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 >
> 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
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
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
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); }
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(+)