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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Reviewed-by: Simon Ser <[email protected]>
Please ping me in a week or so if nobody objected and this isn't merged.
On Tue, 21 Dec 2021 at 13:13, José Expósito <[email protected]> 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 <[email protected]>
> ---
> 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
On Tue, 21 Dec 2021 at 13:13, José Expósito <[email protected]> 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
On Tuesday, December 21st, 2021 at 11:48, Dmitry Baryshkov <[email protected]> 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
On Tue, 21 Dec 2021 at 13:50, Simon Ser <[email protected]> wrote:
>
> On Tuesday, December 21st, 2021 at 11:48, Dmitry Baryshkov <[email protected]> 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
On Tuesday, December 21st, 2021 at 13:33, Dmitry Baryshkov <[email protected]> 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.