Re: [RFC PATH 1/3] drm: add support modifiers for drivers whose planes only support linear layout
Thank you for your reviews.
On 2022/01/15 1:50, Daniel Vetter wrote:
On Wed, Dec 22, 2021 at 02:27:25PM +0900, Tomohito Esaki wrote:
The LINEAR modifier is advertised as default if a driver doesn't specify
modifiers. However, there are legacy drivers such as radeon that do not
support modifiers but infer the actual layout of the underlying buffer.
Therefore, a new flag not_support_fb_modifires is introduced for these
legacy drivers. Allow_fb_modifiers will be replaced with this new flag.
Signed-off-by: Tomohito Esaki
---
drivers/gpu/drm/drm_plane.c | 34 ++
include/drm/drm_mode_config.h | 10 ++
include/drm/drm_plane.h | 3 +++
3 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 82afb854141b..75308ee240c0 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -161,6 +161,16 @@ modifiers_ptr(struct drm_format_modifier_blob *blob)
return (struct drm_format_modifier *)(((char *)blob) +
blob->modifiers_offset);
}
+static bool check_format_modifier(struct drm_plane *plane, uint32_t format,
+ uint64_t modifier)
+{
+ if (plane->funcs->format_mod_supported)
+ return plane->funcs->format_mod_supported(plane, format,
+ modifier);
+
+ return modifier == DRM_FORMAT_MOD_LINEAR;
+}
+
static int create_in_format_blob(struct drm_device *dev, struct drm_plane
*plane)
{
const struct drm_mode_config *config = >mode_config;
@@ -203,16 +213,15 @@ 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)
+ if (config->fb_modifiers_not_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,
-
plane->format_types[j],
-
plane->modifiers[i])) {
-
+ if (check_format_modifier(plane,
+ plane->format_types[j],
+ plane->modifiers[i])) {
mod->formats |= 1ULL << j;
}
}
@@ -242,6 +251,10 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
const char *name, va_list ap)
{
struct drm_mode_config *config = >mode_config;
+ const uint64_t default_modifiers[] = {
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+ };
unsigned int format_modifier_count = 0;
int ret;
@@ -282,6 +295,11 @@ static int __drm_universal_plane_init(struct drm_device *dev,
while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
format_modifier_count++;
+ } else {
+ if (!dev->mode_config.fb_modifiers_not_supported) {
+ format_modifiers = default_modifiers;
+ format_modifier_count = 1;
+ }
}
/* autoset the cap and check for consistency across all planes */
@@ -346,7 +364,7 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
drm_object_attach_property(>base, config->prop_src_h, 0);
}
- if (config->allow_fb_modifiers)
+ if (format_modifier_count)
create_in_format_blob(dev, plane);
return 0;
@@ -373,8 +391,8 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
* drm_universal_plane_init() to let the DRM managed resource infrastructure
* take care of cleanup and deallocation.
*
- * Drivers supporting modifiers must set @format_modifiers on all their planes,
- * even those that only support DRM_FORMAT_MOD_LINEAR.
+ * For drivers supporting modifiers, all planes will advertise
+ * DRM_FORMAT_MOD_LINEAR support, if @format_modifiers is not set.
*
* Returns:
* Zero on success, error code on failure.
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 48b7de80daf5..c56f298c55bd 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -920,6 +920,16 @@ struct drm_mode_config {
*/
bool allow_fb_modifiers;
+ /**
+* @fb_modifiers_not_supported:
+*
+* This flag is for legacy drivers such as radeon that do not support
Maybe don't put specific driver names into kerneldoc (in commit message to
motivate your changes it's fine). It's unlikely
Re: [RFC PATH 1/3] drm: add support modifiers for drivers whose planes only support linear layout
On Wed, Dec 22, 2021 at 02:27:25PM +0900, Tomohito Esaki wrote:
> The LINEAR modifier is advertised as default if a driver doesn't specify
> modifiers. However, there are legacy drivers such as radeon that do not
> support modifiers but infer the actual layout of the underlying buffer.
> Therefore, a new flag not_support_fb_modifires is introduced for these
> legacy drivers. Allow_fb_modifiers will be replaced with this new flag.
>
> Signed-off-by: Tomohito Esaki
> ---
> drivers/gpu/drm/drm_plane.c | 34 ++
> include/drm/drm_mode_config.h | 10 ++
> include/drm/drm_plane.h | 3 +++
> 3 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 82afb854141b..75308ee240c0 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -161,6 +161,16 @@ modifiers_ptr(struct drm_format_modifier_blob *blob)
> return (struct drm_format_modifier *)(((char *)blob) +
> blob->modifiers_offset);
> }
>
> +static bool check_format_modifier(struct drm_plane *plane, uint32_t format,
> + uint64_t modifier)
> +{
> + if (plane->funcs->format_mod_supported)
> + return plane->funcs->format_mod_supported(plane, format,
> + modifier);
> +
> + return modifier == DRM_FORMAT_MOD_LINEAR;
> +}
> +
> static int create_in_format_blob(struct drm_device *dev, struct drm_plane
> *plane)
> {
> const struct drm_mode_config *config = >mode_config;
> @@ -203,16 +213,15 @@ 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)
> + if (config->fb_modifiers_not_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,
> -
> plane->format_types[j],
> -
> plane->modifiers[i])) {
> -
> + if (check_format_modifier(plane,
> + plane->format_types[j],
> + plane->modifiers[i])) {
> mod->formats |= 1ULL << j;
> }
> }
> @@ -242,6 +251,10 @@ static int __drm_universal_plane_init(struct drm_device
> *dev,
> const char *name, va_list ap)
> {
> struct drm_mode_config *config = >mode_config;
> + const uint64_t default_modifiers[] = {
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID
> + };
> unsigned int format_modifier_count = 0;
> int ret;
>
> @@ -282,6 +295,11 @@ static int __drm_universal_plane_init(struct drm_device
> *dev,
>
> while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> format_modifier_count++;
> + } else {
> + if (!dev->mode_config.fb_modifiers_not_supported) {
> + format_modifiers = default_modifiers;
> + format_modifier_count = 1;
> + }
> }
>
> /* autoset the cap and check for consistency across all planes */
> @@ -346,7 +364,7 @@ static int __drm_universal_plane_init(struct drm_device
> *dev,
> drm_object_attach_property(>base, config->prop_src_h, 0);
> }
>
> - if (config->allow_fb_modifiers)
> + if (format_modifier_count)
> create_in_format_blob(dev, plane);
>
> return 0;
> @@ -373,8 +391,8 @@ static int __drm_universal_plane_init(struct drm_device
> *dev,
> * drm_universal_plane_init() to let the DRM managed resource infrastructure
> * take care of cleanup and deallocation.
> *
> - * Drivers supporting modifiers must set @format_modifiers on all their
> planes,
> - * even those that only support DRM_FORMAT_MOD_LINEAR.
> + * For drivers supporting modifiers, all planes will advertise
> + * DRM_FORMAT_MOD_LINEAR support, if @format_modifiers is not set.
> *
> * Returns:
> * Zero on success, error code on failure.
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 48b7de80daf5..c56f298c55bd 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -920,6 +920,16 @@ struct drm_mode_config {
>*/
> bool allow_fb_modifiers;
>
> + /**
> + * @fb_modifiers_not_supported:
> + *
> + * This flag is for legacy drivers such as radeon that do not support
Maybe don't put specific driver names into kerneldoc
[RFC PATH 1/3] drm: add support modifiers for drivers whose planes only support linear layout
The LINEAR modifier is advertised as default if a driver doesn't specify
modifiers. However, there are legacy drivers such as radeon that do not
support modifiers but infer the actual layout of the underlying buffer.
Therefore, a new flag not_support_fb_modifires is introduced for these
legacy drivers. Allow_fb_modifiers will be replaced with this new flag.
Signed-off-by: Tomohito Esaki
---
drivers/gpu/drm/drm_plane.c | 34 ++
include/drm/drm_mode_config.h | 10 ++
include/drm/drm_plane.h | 3 +++
3 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 82afb854141b..75308ee240c0 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -161,6 +161,16 @@ modifiers_ptr(struct drm_format_modifier_blob *blob)
return (struct drm_format_modifier *)(((char *)blob) +
blob->modifiers_offset);
}
+static bool check_format_modifier(struct drm_plane *plane, uint32_t format,
+ uint64_t modifier)
+{
+ if (plane->funcs->format_mod_supported)
+ return plane->funcs->format_mod_supported(plane, format,
+ modifier);
+
+ return modifier == DRM_FORMAT_MOD_LINEAR;
+}
+
static int create_in_format_blob(struct drm_device *dev, struct drm_plane
*plane)
{
const struct drm_mode_config *config = >mode_config;
@@ -203,16 +213,15 @@ 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)
+ if (config->fb_modifiers_not_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,
-
plane->format_types[j],
-
plane->modifiers[i])) {
-
+ if (check_format_modifier(plane,
+ plane->format_types[j],
+ plane->modifiers[i])) {
mod->formats |= 1ULL << j;
}
}
@@ -242,6 +251,10 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
const char *name, va_list ap)
{
struct drm_mode_config *config = >mode_config;
+ const uint64_t default_modifiers[] = {
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+ };
unsigned int format_modifier_count = 0;
int ret;
@@ -282,6 +295,11 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
format_modifier_count++;
+ } else {
+ if (!dev->mode_config.fb_modifiers_not_supported) {
+ format_modifiers = default_modifiers;
+ format_modifier_count = 1;
+ }
}
/* autoset the cap and check for consistency across all planes */
@@ -346,7 +364,7 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
drm_object_attach_property(>base, config->prop_src_h, 0);
}
- if (config->allow_fb_modifiers)
+ if (format_modifier_count)
create_in_format_blob(dev, plane);
return 0;
@@ -373,8 +391,8 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
* drm_universal_plane_init() to let the DRM managed resource infrastructure
* take care of cleanup and deallocation.
*
- * Drivers supporting modifiers must set @format_modifiers on all their planes,
- * even those that only support DRM_FORMAT_MOD_LINEAR.
+ * For drivers supporting modifiers, all planes will advertise
+ * DRM_FORMAT_MOD_LINEAR support, if @format_modifiers is not set.
*
* Returns:
* Zero on success, error code on failure.
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 48b7de80daf5..c56f298c55bd 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -920,6 +920,16 @@ struct drm_mode_config {
*/
bool allow_fb_modifiers;
+ /**
+* @fb_modifiers_not_supported:
+*
+* This flag is for legacy drivers such as radeon that do not support
+* modifiers but infer the actual layout of the underlying buffer.
+* Generally, each drivers must support modifiers, this flag should not
+* be set.
+*/
+ bool fb_modifiers_not_supported;
+
/**
*
