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.
This second version implements the required changes and drops the
"format_mod_supported" in the drivers that can use the default
implementation.
Jose
[1] https://lore.kernel.org/dri-devel/CAA8EJpqJ-tWmb5Ba6XSK59toCtLb3nRRmVH8da4Ud_rrRYytmw@mail.gmail.com/T/
[2] https://lore.kernel.org/dri-devel/20211216170532.GA16349@elementary/T/
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 | 8 ++------
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(+), 42 deletions(-)
--
2.25.1
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-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 | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 82afb854141b..c1186b7215ee 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;
}
}
--
2.25.1
Fix minor typo: "valdiate" -> "validate".
Signed-off-by: José Expósito <[email protected]>
---
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.
--
2.25.1
The "drm_plane_funcs.format_mod_supported" can be removed in favor of
the default implementation.
Signed-off-by: José Expósito <[email protected]>
---
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,
};
/**
--
2.25.1
The "drm_plane_funcs.format_mod_supported" can be removed in favor of
the default implementation.
Signed-off-by: José Expósito <[email protected]>
---
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 *
--
2.25.1
The "drm_plane_funcs.format_mod_supported" can be removed in favor of
the default implementation.
Signed-off-by: José Expósito <[email protected]>
---
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,
--
2.25.1
The "drm_plane_funcs.format_mod_supported" can be removed in favor of
the default implementation.
Signed-off-by: José Expósito <[email protected]>
---
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 = {
--
2.25.1
Reviewed-by: Simon Ser <[email protected]>
On Wednesday, December 22nd, 2021 at 10:05, José Expósito <[email protected]> wrote:
> Make "create_in_format_blob" behave as documented.
LGTM, nice!
Reviewed-by: Simon Ser <[email protected]>
CC Ville just in case
On Wed, Dec 22, 2021 at 10:05:47AM +0100, Jos? Exp?sito wrote:
> 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-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 | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 82afb854141b..c1186b7215ee 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])) {
So instead of skipping the whole loop you just skip doing anything
inside the loop? Can't see how that achieves anything at all.
https://patchwork.freedesktop.org/series/83018/
is what I had in mind earlier but no one reviewed it and
the discussion veered off track IIRC.
--
Ville Syrj?l?
Intel
On Thursday, December 23rd, 2021 at 12:56, Ville Syrjälä <[email protected]> wrote:
> > - /* 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])) {
>
> So instead of skipping the whole loop you just skip doing anything
> inside the loop? Can't see how that achieves anything at all.
No, the check is skipped when the function isn't populated by the driver.
On Thu, Dec 23, 2021 at 01:42:32PM +0000, Simon Ser wrote:
> On Thursday, December 23rd, 2021 at 12:56, Ville Syrj?l? <[email protected]> wrote:
>
> > > - /* 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])) {
> >
> > So instead of skipping the whole loop you just skip doing anything
> > inside the loop? Can't see how that achieves anything at all.
>
> No, the check is skipped when the function isn't populated by the driver.
Ah right. So we advertise all modifiers in that case. Looks like
drm_plane_check_pixel_format() does support that model, so seems OK.
Another related thing that might be worth checking is whether
drivers generally do anything to validate the modifiers in
the addfb2 ioctl. Looks like i915 and amdgpu are the only ones
to use drm_any_plane_has_format() for that, so all the other
drivers must either be checking it manually (or they're just
potentially broken when handed unexpected modifiers by evil
userspace).
--
Ville Syrj?l?
Intel
Thanks for your reviews :) I'll wait a couple of days to see
if somebody else wants to comment and I'll send v3 adding the
reviewed by tags and fixing the compiler warning.
On Thu, Dec 23, 2021 at 05:03:19PM +0200, Ville Syrj?l? wrote:
> Another related thing that might be worth checking is whether
> drivers generally do anything to validate the modifiers in
> the addfb2 ioctl. Looks like i915 and amdgpu are the only ones
> to use drm_any_plane_has_format() for that, so all the other
> drivers must either be checking it manually (or they're just
> potentially broken when handed unexpected modifiers by evil
> userspace).
I'm pretty new to this subsystem, so please correct me if I'm
wrong, but after looking into a couple of drivers I think you
are right, this check is missing in some drivers.
This possible bug reminds me of this ToDo task [1]:
> Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> atomic drivers we could check for valid formats by calling
> drm_plane_check_pixel_format() against all planes, and pass if any plane
> supports the format. For non-atomic that's not possible since like the format
> list for the primary plane is fake and we'd therefor reject valid formats.
I had a look to the Raspberry Pi driver (mainly because I'm trying
to understand it) and it looks like the check is missing. Other
drivers, for example Mali, are checking the format modifier manually.
I'll try to do some actual testing during Christmas and see
how it goes.
Jos? Exp?sito
[1] https://www.kernel.org/doc/html/latest/gpu/todo.html#drm-framebuffer-funcs-and-drm-mode-config-funcs-fb-create-cleanup
Hello José,
thanks for your patch.
Reviewed-by: Yannick Fertre <[email protected]>
Tested-by: Yannick Fertre <[email protected]>
On 12/22/21 10:05 AM, José Expósito wrote:
> The "drm_plane_funcs.format_mod_supported" can be removed in favor of
> the default implementation.
>
> Signed-off-by: José Expósito <[email protected]>
> ---
> 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 = {
>
On Wed, Dec 22, 2021 at 2:36 PM José Expósito <[email protected]> wrote:
>
> The "drm_plane_funcs.format_mod_supported" can be removed in favor of
> the default implementation.
>
> Signed-off-by: José Expósito <[email protected]>
> ---
Reviewed-by: Jagan Teki <[email protected]>
Tested-by: Jagan Teki <[email protected]> # i.Core STM32MP1