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(-)
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-d... [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
Reviewed-by: Simon Ser contact@emersion.fr
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 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-d... [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 modifierpairs 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 drivercapability for general modifier support. If this flag is set then every-- 2.25.1
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_Ez...
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_Ez...
I'd still suggest to fix create_in_format_blob()
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.
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,
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, };
/*
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
dri-devel@lists.freedesktop.org
-
Dmitry Baryshkov -
José Expósito -
Simon Ser