Hi all,
This patchset supersedes [1]. Now the title is a bit misleading, but I left it this way to (hopefully) facilitate the maintainers' work.
A little context: Originally, I sent a patch adding modifiers to the VKMS driver and Simon Ser kindly reviewed it and pointed out that "format_mod_supported" was missing [2]. I asked if the docs were incorrect or if it was a bug in "create_in_format_blob".
In the first version of this series, Simon Ser and Dmitry Baryshkov agreed [1] that the code should behave as documented and "create_in_format_blob" should be changed.
The second version implemented the required changes and drops the "format_mod_supported" in the drivers that can use the default implementation. [3]
This third version fixes a compiler warning and adds the reviewed by tags.
Thanks, José Expósito
[1] https://lore.kernel.org/dri-devel/CAA8EJpqJ-tWmb5Ba6XSK59toCtLb3nRRmVH8da4Ud... [2] https://lore.kernel.org/dri-devel/20211216170532.GA16349@elementary/T/ [3] https://lore.kernel.org/dri-devel/20211222090552.25972-1-jose.exposito89@gma...
José Expósito (6): drm/plane: Make format_mod_supported truly optional drm/plane: Fix typo in format_mod_supported documentation drm/simple-kms: Drop format_mod_supported function drm/i915/display: Drop format_mod_supported function drm: mxsfb: Drop format_mod_supported function drm/stm: ltdc: Drop format_mod_supported function
drivers/gpu/drm/drm_plane.c | 9 ++------- drivers/gpu/drm/drm_simple_kms_helper.c | 8 -------- drivers/gpu/drm/i915/display/intel_cursor.c | 8 -------- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 8 -------- drivers/gpu/drm/stm/ltdc.c | 11 ----------- include/drm/drm_plane.h | 2 +- 6 files changed, 3 insertions(+), 43 deletions(-)
The documentation for "drm_plane_funcs.format_mod_supported" reads:
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 validate 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.
However, where the function is not present, an invalid IN_FORMATS blob property with modifiers but no formats is exposed to user-space.
This breaks the latest Weston [1]. For testing purposes, I extracted the affected code to a standalone program [2].
Make "create_in_format_blob" behave as documented.
[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 Reviewed-by: Simon Ser contact@emersion.fr
---
v2:
- Remove unused "done:" label to fix compile warning Reported-by: kernel test robot lkp@intel.com
- Add Reviewed-by (thanks to Simon Ser) --- drivers/gpu/drm/drm_plane.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 82afb854141b..deeec60a3315 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -202,17 +202,13 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
- /* If we can't determine support, just bail */ - if (!plane->funcs->format_mod_supported) - goto done; - mod = modifiers_ptr(blob_data); for (i = 0; i < plane->modifier_count; i++) { for (j = 0; j < plane->format_count; j++) { - if (plane->funcs->format_mod_supported(plane, + if (!plane->funcs->format_mod_supported || + plane->funcs->format_mod_supported(plane, plane->format_types[j], plane->modifiers[i])) { - mod->formats |= 1ULL << j; } } @@ -223,7 +219,6 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane mod++; }
-done: drm_object_attach_property(&plane->base, config->modifiers_property, blob->base.id);
Fix minor typo: "valdiate" -> "validate".
Signed-off-by: José Expósito jose.exposito89@gmail.com Reviewed-by: Simon Ser contact@emersion.fr --- include/drm/drm_plane.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 0c1102dc4d88..06759badf99f 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -516,7 +516,7 @@ struct drm_plane_funcs { * 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. + * which modifier), and to validate 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.
Pushed patches 1 & 2 to drm-misc-next. Thanks for your contribution!
Hi Simon,
On Wed, Jan 05, 2022 at 11:54:43PM +0000, Simon Ser wrote:
Pushed patches 1 & 2 to drm-misc-next. Thanks for your contribution!
Thanks a lot for the review and for applying the changes, appreciate it.
Is there something that needs to improve in the other 4 patches? Or just waiting on maintainers input?
Thanks, José Expósito
On Friday, January 7th, 2022 at 18:26, José Expósito jose.exposito89@gmail.com wrote:
Is there something that needs to improve in the other 4 patches? Or just waiting on maintainers input?
Yeah, these patches look good to me. Feel free to add my R-b.
Maintainers for these drivers still need to review/ack/apply them.
On 1/7/22 6:26 PM, José Expósito wrote:
Hi Simon,
On Wed, Jan 05, 2022 at 11:54:43PM +0000, Simon Ser wrote:
Pushed patches 1 & 2 to drm-misc-next. Thanks for your contribution!
Thanks a lot for the review and for applying the changes, appreciate it.
Is there something that needs to improve in the other 4 patches? Or just waiting on maintainers input?
Thanks, José Expósito
Hi José, for the drm/stm part, Applied on drm-misc-next. Many thanks for your patches, Philippe :-)
The "drm_plane_funcs.format_mod_supported" can be removed in favor of the default implementation.
Signed-off-by: José Expósito jose.exposito89@gmail.com --- drivers/gpu/drm/drm_simple_kms_helper.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 72989ed1baba..2c6aa67c6956 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -284,13 +284,6 @@ static void drm_simple_kms_plane_cleanup_fb(struct drm_plane *plane, pipe->funcs->cleanup_fb(pipe, state); }
-static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane, - uint32_t format, - uint64_t modifier) -{ - return modifier == DRM_FORMAT_MOD_LINEAR; -} - static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = { .prepare_fb = drm_simple_kms_plane_prepare_fb, .cleanup_fb = drm_simple_kms_plane_cleanup_fb, @@ -339,7 +332,6 @@ static const struct drm_plane_funcs drm_simple_kms_plane_funcs = { .reset = drm_simple_kms_plane_reset, .atomic_duplicate_state = drm_simple_kms_plane_duplicate_state, .atomic_destroy_state = drm_simple_kms_plane_destroy_state, - .format_mod_supported = drm_simple_kms_format_mod_supported, };
/**
The "drm_plane_funcs.format_mod_supported" can be removed in favor of the default implementation.
Signed-off-by: José Expósito jose.exposito89@gmail.com --- drivers/gpu/drm/i915/display/intel_cursor.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c index 11842f212613..6a5e022f5e21 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.c +++ b/drivers/gpu/drm/i915/display/intel_cursor.c @@ -602,13 +602,6 @@ static bool i9xx_cursor_get_hw_state(struct intel_plane *plane, return ret; }
-static bool intel_cursor_format_mod_supported(struct drm_plane *_plane, - u32 format, u64 modifier) -{ - return modifier == DRM_FORMAT_MOD_LINEAR && - format == DRM_FORMAT_ARGB8888; -} - static int intel_legacy_cursor_update(struct drm_plane *_plane, struct drm_crtc *_crtc, @@ -745,7 +738,6 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = { .destroy = intel_plane_destroy, .atomic_duplicate_state = intel_plane_duplicate_state, .atomic_destroy_state = intel_plane_destroy_state, - .format_mod_supported = intel_cursor_format_mod_supported, };
struct intel_plane *
The "drm_plane_funcs.format_mod_supported" can be removed in favor of the default implementation.
Signed-off-by: José Expósito jose.exposito89@gmail.com --- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 0655582ae8ed..df32e1c3cc5d 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -554,13 +554,6 @@ static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane, writel(ctrl, mxsfb->base + LCDC_AS_CTRL); }
-static bool mxsfb_format_mod_supported(struct drm_plane *plane, - uint32_t format, - uint64_t modifier) -{ - return modifier == DRM_FORMAT_MOD_LINEAR; -} - static const struct drm_plane_helper_funcs mxsfb_plane_primary_helper_funcs = { .atomic_check = mxsfb_plane_atomic_check, .atomic_update = mxsfb_plane_primary_atomic_update, @@ -572,7 +565,6 @@ static const struct drm_plane_helper_funcs mxsfb_plane_overlay_helper_funcs = { };
static const struct drm_plane_funcs mxsfb_plane_funcs = { - .format_mod_supported = mxsfb_format_mod_supported, .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, .destroy = drm_plane_cleanup,
The "drm_plane_funcs.format_mod_supported" can be removed in favor of the default implementation.
Signed-off-by: José Expósito jose.exposito89@gmail.com --- drivers/gpu/drm/stm/ltdc.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index dbdee954692a..ef909e50f0e4 100644 --- a/drivers/gpu/drm/stm/ltdc.c +++ b/drivers/gpu/drm/stm/ltdc.c @@ -925,16 +925,6 @@ static void ltdc_plane_atomic_print_state(struct drm_printer *p, fpsi->counter = 0; }
-static bool ltdc_plane_format_mod_supported(struct drm_plane *plane, - u32 format, - u64 modifier) -{ - if (modifier == DRM_FORMAT_MOD_LINEAR) - return true; - - return false; -} - static const struct drm_plane_funcs ltdc_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, @@ -943,7 +933,6 @@ static const struct drm_plane_funcs ltdc_plane_funcs = { .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, .atomic_print_state = ltdc_plane_atomic_print_state, - .format_mod_supported = ltdc_plane_format_mod_supported, };
static const struct drm_plane_helper_funcs ltdc_plane_helper_funcs = {
dri-devel@lists.freedesktop.org
-
José Expósito -
Philippe CORNU -
Simon Ser