[PATCH 0/3] Add missing format_mod_supported functions

[PATCH 0/3] Add missing format_mod_supported functions

[José Expósito]

Hi all,

When setting IN_FORMATS, implementing the
"drm_plane_funcs.format_mod_supported" function is mandatory to avoid
exposing a bogus blob.

This patchset adds a bit of documentation and fixes the issue in a
couple of drivers affected by the bug.

I reviewed all the other drivers and I didn't find more instances of
the issue.

Jose

José Expósito (3):
  drm/plane: Mention format_mod_supported in IN_FORMATS docs
  drm/msm/mdp4: Add format_mod_supported function
  drm/sun4i: Add format_mod_supported function

 drivers/gpu/drm/drm_plane.c                | 7 +++++--
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 8 ++++++++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c     | 7 +++++++
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c     | 7 +++++++
 4 files changed, 27 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/3] drm/plane: Mention format_mod_supported in IN_FORMATS docs

[José Expósito]

Adding format modifiers without implementing the function
"drm_plane_funcs.format_mod_supported" exposes an invalid IN_FORMATS
blob with modifiers but no formats to user-space.

This breaks the latest Weston [1]. For testing purposes, I extracted the
affected code to a standalone program [2].

Make clear in the IN_FORMATS documentation that implementing
"format_mod_supported" is mandatory.

[1] https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0/libweston/backend-drm/kms.c#L431
[2] https://github.com/JoseExposito/drm-sandbox/blob/main/in_formats.c

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/drm_plane.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 82afb854141b..347571f8909a 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -126,8 +126,11 @@
  * IN_FORMATS:
  *     Blob property which contains the set of buffer format and modifier
  *     pairs supported by this plane. The blob is a struct
- *     drm_format_modifier_blob. Without this property the plane doesn't
- *     support buffers with modifiers. Userspace cannot change this property.
+ *     drm_format_modifier_blob and the drm_plane_funcs.format_mod_supported
+ *     function must be implemented by the driver to determine if the given
+ *     format/modifier combination is valid for the plane. Without this property
+ *     the plane doesn't support buffers with modifiers. Userspace cannot change
+ *     this property.
  *
  *     Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver
  *     capability for general modifier support. If this flag is set then every
-- 
2.25.1

Re: [PATCH 1/3] drm/plane: Mention format_mod_supported in IN_FORMATS docs

[Simon Ser]

Reviewed-by: Simon Ser <contact@emersion.fr>

Please ping me in a week or so if nobody objected and this isn't merged.

Re: [PATCH 1/3] drm/plane: Mention format_mod_supported in IN_FORMATS docs

[Dmitry Baryshkov]

On Tue, 21 Dec 2021 at 13:13, José Expósito <jose.exposito89@gmail.com> wrote:
>
> Adding format modifiers without implementing the function
> "drm_plane_funcs.format_mod_supported" exposes an invalid IN_FORMATS
> blob with modifiers but no formats to user-space.

I think the fix should be applied to the generic code. The docs at
drm_plane.h clearly state that this callback is optional:

         * This optional hook is used for the DRM to determine if the given
         * format/modifier combination is valid for the plane. This allows the
         * DRM to generate the correct format bitmask (which formats apply to
         * which modifier), and to valdiate modifiers at atomic_check time.
         *
         * If not present, then any modifier in the plane's modifier
         * list is allowed with any of the plane's formats.

So, one should fix the core code in create_in_format_blob() to include
all combinations.

>
> This breaks the latest Weston [1]. For testing purposes, I extracted the
> affected code to a standalone program [2].
>
> Make clear in the IN_FORMATS documentation that implementing
> "format_mod_supported" is mandatory.

format_mod_supported() being optional or mandatory should be
documented in the format_mod_supported() docs, not in the IN_FORMAT
docs.

>
> [1] https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0/libweston/backend-drm/kms.c#L431
> [2] https://github.com/JoseExposito/drm-sandbox/blob/main/in_formats.c
>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 82afb854141b..347571f8909a 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -126,8 +126,11 @@
>   * IN_FORMATS:
>   *     Blob property which contains the set of buffer format and modifier
>   *     pairs supported by this plane. The blob is a struct
> - *     drm_format_modifier_blob. Without this property the plane doesn't
> - *     support buffers with modifiers. Userspace cannot change this property.
> + *     drm_format_modifier_blob and the drm_plane_funcs.format_mod_supported
> + *     function must be implemented by the driver to determine if the given
> + *     format/modifier combination is valid for the plane. Without this property
> + *     the plane doesn't support buffers with modifiers. Userspace cannot change
> + *     this property.
>   *
>   *     Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver
>   *     capability for general modifier support. If this flag is set then every
> --
> 2.25.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/plane: Mention format_mod_supported in IN_FORMATS docs

[Simon Ser]

On Tuesday, December 21st, 2021 at 11:48, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> I think the fix should be applied to the generic code.

Related:
https://lore.kernel.org/dri-devel/t1g_xNE6hgj1nTQfx2UWob1wbsCAxElBvWRwNSY_EzmlEe_9WWpq8-vQKyJPK6wZY8q8BqHl-KoGwS5V91VgN8lGIl3PJt7s2fkdsRd3y70=@emersion.fr/T/#u

Re: [PATCH 1/3] drm/plane: Mention format_mod_supported in IN_FORMATS docs

[Dmitry Baryshkov]

On Tue, 21 Dec 2021 at 13:50, Simon Ser <contact@emersion.fr> wrote:
>
> On Tuesday, December 21st, 2021 at 11:48, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > I think the fix should be applied to the generic code.
>
> Related:
> https://lore.kernel.org/dri-devel/t1g_xNE6hgj1nTQfx2UWob1wbsCAxElBvWRwNSY_EzmlEe_9WWpq8-vQKyJPK6wZY8q8BqHl-KoGwS5V91VgN8lGIl3PJt7s2fkdsRd3y70=@emersion.fr/T/#u

I'd still suggest to fix create_in_format_blob()

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/plane: Mention format_mod_supported in IN_FORMATS docs

[Simon Ser]

On Tuesday, December 21st, 2021 at 13:33, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> I'd still suggest to fix create_in_format_blob()

Yeah, I agree. I thought there were a good reason why create_in_format_blob()
behaves this way but can't find anything in the Git history, so fixing it to
behave as the documentation says would be best.

[PATCH 2/3] drm/msm/mdp4: Add format_mod_supported function

[José Expósito]

Implement the missing "drm_plane_funcs.format_mod_supported" function
to avoid exposing an invalid IN_FORMATS blob with modifiers but no
formats.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
index 49bdabea8ed5..8809f1633786 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
@@ -80,6 +80,13 @@ static int mdp4_plane_set_property(struct drm_plane *plane,
 	return -EINVAL;
 }
 
+static bool mdp4_plane_format_mod_supported(struct drm_plane *plane, u32 format,
+		u64 modifier)
+{
+	return (modifier == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE) ||
+	       (modifier == DRM_FORMAT_MOD_LINEAR);
+}
+
 static const struct drm_plane_funcs mdp4_plane_funcs = {
 		.update_plane = drm_atomic_helper_update_plane,
 		.disable_plane = drm_atomic_helper_disable_plane,
@@ -88,6 +95,7 @@ static const struct drm_plane_funcs mdp4_plane_funcs = {
 		.reset = drm_atomic_helper_plane_reset,
 		.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 		.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+		.format_mod_supported = mdp4_plane_format_mod_supported,
 };
 
 static void mdp4_plane_cleanup_fb(struct drm_plane *plane,
-- 
2.25.1

[PATCH 3/3] drm/sun4i: Add format_mod_supported function

[José Expósito]

Implement the missing "drm_plane_funcs.format_mod_supported" function
to avoid exposing an invalid IN_FORMATS blob with modifiers but no
formats.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 7 +++++++
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index 7845c2a53a7f..728563f23cd6 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -331,6 +331,12 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
 			      true, zpos, old_zpos);
 }
 
+static bool sun8i_ui_layer_format_mod_supported(struct drm_plane *plane,
+						u32 format, u64 modifier)
+{
+	return (modifier == DRM_FORMAT_MOD_LINEAR);
+}
+
 static const struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
 	.atomic_check	= sun8i_ui_layer_atomic_check,
 	.atomic_disable	= sun8i_ui_layer_atomic_disable,
@@ -344,6 +350,7 @@ static const struct drm_plane_funcs sun8i_ui_layer_funcs = {
 	.disable_plane		= drm_atomic_helper_disable_plane,
 	.reset			= drm_atomic_helper_plane_reset,
 	.update_plane		= drm_atomic_helper_update_plane,
+	.format_mod_supported	= sun8i_ui_layer_format_mod_supported,
 };
 
 static const u32 sun8i_ui_layer_formats[] = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index bb7c43036dfa..d17813a7cac3 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -435,6 +435,12 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
 			      true, zpos, old_zpos);
 }
 
+static bool sun8i_vi_layer_format_mod_supported(struct drm_plane *plane,
+						u32 format, u64 modifier)
+{
+	return (modifier == DRM_FORMAT_MOD_LINEAR);
+}
+
 static const struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
 	.atomic_check	= sun8i_vi_layer_atomic_check,
 	.atomic_disable	= sun8i_vi_layer_atomic_disable,
@@ -448,6 +454,7 @@ static const struct drm_plane_funcs sun8i_vi_layer_funcs = {
 	.disable_plane		= drm_atomic_helper_disable_plane,
 	.reset			= drm_atomic_helper_plane_reset,
 	.update_plane		= drm_atomic_helper_update_plane,
+	.format_mod_supported	= sun8i_vi_layer_format_mod_supported,
 };
 
 /*
-- 
2.25.1

Re: [PATCH 0/3] Add missing format_mod_supported functions

[Dmitry Baryshkov]

On Tue, 21 Dec 2021 at 13:13, José Expósito <jose.exposito89@gmail.com> wrote:
>
> Hi all,
>
> When setting IN_FORMATS, implementing the
> "drm_plane_funcs.format_mod_supported" function is mandatory to avoid
> exposing a bogus blob.
>
> This patchset adds a bit of documentation and fixes the issue in a
> couple of drivers affected by the bug.
>
> I reviewed all the other drivers and I didn't find more instances of
> the issue.
>
> Jose
>
> José Expósito (3):
>   drm/plane: Mention format_mod_supported in IN_FORMATS docs
>   drm/msm/mdp4: Add format_mod_supported function
>   drm/sun4i: Add format_mod_supported function
>
>  drivers/gpu/drm/drm_plane.c                | 7 +++++--
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 8 ++++++++

You missed mdp5_plane.c here. But I assume that the proposed fix is
not correct, see my comments on patch 1.

>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c     | 7 +++++++
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c     | 7 +++++++
>  4 files changed, 27 insertions(+), 2 deletions(-)
>
> --
> 2.25.1
>


-- 
With best wishes
Dmitry