diff mbox

[2/4] drm: Create a format/modifier blob

Message ID 20170623164544.7362-3-ben@bwidawsk.net (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky June 23, 2017, 4:45 p.m. UTC
Updated blob layout (Rob, Daniel, Kristian, xerpi)

v2:
* Removed __packed, and alignment (.+)
* Fix indent in drm_format_modifier fields (Liviu)
* Remove duplicated modifier > 64 check (Liviu)
* Change comment about modifier (Liviu)
* Remove arguments to blob creation, use plane instead (Liviu)
* Fix data types (Ben)
* Make the blob part of uapi (Daniel)

v3:
Remove unused ret field.
Change i, and j to unsigned int (Emil)

v4:
Use plane->modifier_count instead of recounting (Daniel)

Cc: Rob Clark <robdclark@gmail.com>
Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Daniel Stone <daniels@collabora.com> (v2)
Reviewed-by: Liviu Dudau <liviu@dudau.co.uk> (v2)
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> (v3)
---
 drivers/gpu/drm/drm_mode_config.c |  7 ++++
 drivers/gpu/drm/drm_plane.c       | 83 +++++++++++++++++++++++++++++++++++++++
 include/drm/drm_mode_config.h     |  6 +++
 include/uapi/drm/drm_mode.h       | 50 +++++++++++++++++++++++
 4 files changed, 146 insertions(+)

Comments

Ville Syrjala June 29, 2017, 7:49 p.m. UTC | #1
On Fri, Jun 23, 2017 at 09:45:42AM -0700, Ben Widawsky wrote:
> Updated blob layout (Rob, Daniel, Kristian, xerpi)
> 
> v2:
> * Removed __packed, and alignment (.+)
> * Fix indent in drm_format_modifier fields (Liviu)
> * Remove duplicated modifier > 64 check (Liviu)
> * Change comment about modifier (Liviu)
> * Remove arguments to blob creation, use plane instead (Liviu)
> * Fix data types (Ben)
> * Make the blob part of uapi (Daniel)
> 
> v3:
> Remove unused ret field.
> Change i, and j to unsigned int (Emil)
> 
> v4:
> Use plane->modifier_count instead of recounting (Daniel)
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Daniel Stone <daniels@collabora.com> (v2)
> Reviewed-by: Liviu Dudau <liviu@dudau.co.uk> (v2)
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> (v3)
> ---
>  drivers/gpu/drm/drm_mode_config.c |  7 ++++
>  drivers/gpu/drm/drm_plane.c       | 83 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h     |  6 +++
>  include/uapi/drm/drm_mode.h       | 50 +++++++++++++++++++++++
>  4 files changed, 146 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index d9862259a2a7..6bfbc3839df5 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -337,6 +337,13 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.gamma_lut_size_property = prop;
>  
> +	prop = drm_property_create(dev,
> +				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
> +				   "IN_FORMATS", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.modifiers = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d3fc561d7b48..8a2d5765837a 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -62,6 +62,86 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>  	return num;
>  }
>  
> +static inline u32 *
> +formats_ptr(struct drm_format_modifier_blob *blob)
> +{
> +	return (u32 *)(((char *)blob) + blob->formats_offset);
> +}
> +
> +static inline struct drm_format_modifier *
> +modifiers_ptr(struct drm_format_modifier_blob *blob)
> +{
> +	return (struct drm_format_modifier *)(((char *)blob) + blob->modifiers_offset);
> +}
> +
> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
> +{
> +	const struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_property_blob *blob;
> +	struct drm_format_modifier *mod;
> +	size_t blob_size, formats_size, modifiers_size;
> +	struct drm_format_modifier_blob *blob_data;
> +	unsigned int i, j;
> +
> +	formats_size = sizeof(*plane->format_types) * plane->format_count;

sizeof(__u32) perhaps here since this is about the uabi, not about how
we store the formats internally.

> +	if (WARN_ON(!formats_size)) {
> +		/* 0 formats are never expected */
> +		return 0;
> +	}
> +
> +	modifiers_size =
> +		sizeof(struct drm_format_modifier) * plane->modifier_count;
> +
> +	blob_size = sizeof(struct drm_format_modifier_blob);
> +	/* Modifiers offset is a pointer to a struct with a 64 bit field so it
> +	 * should be naturally aligned to 8B.
> +	 */
> +	blob_size += ALIGN(formats_size, 8);

Here it's ALIGN(format_size) ...

> +	blob_size += modifiers_size;
> +
> +	blob = drm_property_create_blob(dev, blob_size, NULL);
> +	if (IS_ERR(blob))
> +		return -1;
> +
> +	blob_data = (struct drm_format_modifier_blob *)blob->data;
> +	blob_data->version = FORMAT_BLOB_CURRENT;
> +	blob_data->count_formats = plane->format_count;
> +	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> +	blob_data->count_modifiers = plane->modifier_count;
> +
> +	blob_data->modifiers_offset =
> +		ALIGN(blob_data->formats_offset + formats_size, 8);

... but here it's ALIGN(formats_offset+formats_size). I think we should
be aligning the same thing in both cases, or we add a BUILD_BUG_ON to
make sure the header size always stays a multiple of 8 bytes.

That's mainly because the design of the structure seems geared towards
expanding the header in the future (as in why else would you have the
offsets?).

> +
> +	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,
> +							       plane->format_types[j],
> +							       plane->modifiers[i])) {
> +
> +				mod->formats |= 1 << j;
> +			}
> +		}
> +
> +		mod->modifier = plane->modifiers[i];
> +		mod->offset = 0;
> +		mod->pad = 0;
> +		mod++;
> +	}
> +
> +done:
> +	drm_object_attach_property(&plane->base, config->modifiers,
> +				   blob->base.id);
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_universal_plane_init - Initialize a new universal plane object
>   * @dev: DRM device
> @@ -181,6 +261,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
>  	}
>  
> +	if (config->allow_fb_modifiers)
> +		create_in_format_blob(dev, plane);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_universal_plane_init);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 42981711189b..03776e659811 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -757,6 +757,12 @@ struct drm_mode_config {
>  	 */
>  	bool allow_fb_modifiers;
>  
> +	/**
> +	 * @modifiers: Plane property to list support modifier/format
> +	 * combination.
> +	 */
> +	struct drm_property *modifiers;

I believe that if we follow the most common naming pattern this should
be "modifiers_property". Also might be good to group it next to the
other properties.

> +
>  	/* cursor size */
>  	uint32_t cursor_width, cursor_height;
>  
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 403339f98a92..a2bb7161f020 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -712,6 +712,56 @@ struct drm_mode_atomic {
>  	__u64 user_data;
>  };
>  
> +struct drm_format_modifier_blob {
> +#define FORMAT_BLOB_CURRENT 1
> +	/* Version of this blob format */
> +	u32 version;
> +
> +	/* Flags */
> +	u32 flags;
> +
> +	/* Number of fourcc formats supported */
> +	u32 count_formats;
> +
> +	/* Where in this blob the formats exist (in bytes) */
> +	u32 formats_offset;
> +
> +	/* Number of drm_format_modifiers */
> +	u32 count_modifiers;
> +
> +	/* Where in this blob the modifiers exist (in bytes) */
> +	u32 modifiers_offset;
> +
> +	/* u32 formats[] */
> +	/* struct drm_format_modifier modifiers[] */
> +};
> +
> +struct drm_format_modifier {
> +	/* Bitmask of formats in get_plane format list this info applies to. The
> +	 * offset allows a sliding window of which 64 formats (bits).
> +	 *
> +	 * Some examples:
> +	 * In today's world with < 65 formats, and formats 0, and 2 are
> +	 * supported
> +	 * 0x0000000000000005
> +	 *		  ^-offset = 0, formats = 5
> +	 *
> +	 * If the number formats grew to 128, and formats 98-102 are
> +	 * supported with the modifier:
> +	 *
> +	 * 0x0000003c00000000 0000000000000000
> +	 *		  ^
> +	 *		  |__offset = 64, formats = 0x3c00000000
> +	 *
> +	 */
> +	__u64 formats;
> +	__u32 offset;
> +	__u32 pad;
> +
> +	/* The modifier that applies to the >get_plane format list bitmask. */
> +	__u64 modifier;
> +};
> +
>  /**
>   * Create a new 'blob' data property, copying length bytes from data pointer,
>   * and returning new blob ID.
> -- 
> 2.13.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ben Widawsky July 14, 2017, 6:41 p.m. UTC | #2
On 17-06-29 22:49:44, Ville Syrjälä wrote:

[snip]

>
>... but here it's ALIGN(formats_offset+formats_size). I think we should
>be aligning the same thing in both cases, or we add a BUILD_BUG_ON to
>make sure the header size always stays a multiple of 8 bytes.
>
>That's mainly because the design of the structure seems geared towards
>expanding the header in the future (as in why else would you have the
>offsets?).
>

I guess I don't quite understand what you're asking for. The first thing is
determining a size, the second is finding an offset into the blob.

I don't mind changing this, but tell me what you want.

BUILD_BUG_ON sounds good to me regardless.

[snip]
Ville Syrjala July 14, 2017, 7:10 p.m. UTC | #3
On Fri, Jul 14, 2017 at 11:41:49AM -0700, Ben Widawsky wrote:
> On 17-06-29 22:49:44, Ville Syrjälä wrote:
> 
> [snip]
> 
> >
> >... but here it's ALIGN(formats_offset+formats_size). I think we should
> >be aligning the same thing in both cases, or we add a BUILD_BUG_ON to
> >make sure the header size always stays a multiple of 8 bytes.
> >
> >That's mainly because the design of the structure seems geared towards
> >expanding the header in the future (as in why else would you have the
> >offsets?).
> >
> 
> I guess I don't quite understand what you're asking for. The first thing is
> determining a size, the second is finding an offset into the blob.

If I remember my thinking correctly, then what I was trying to say was

ALIGN(multiple_of_4, 8) + ALIGN(multiple_of_4, 8) != ALIGN(multiple_of_4 + multiple_of_4, 8)

but the code was assuming that it's true, or that at least one of those
things is a multiple of 8 already.

> I don't mind changing this, but tell me what you want.
> 
> BUILD_BUG_ON sounds good to me regardless.
> 
> [snip]
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index d9862259a2a7..6bfbc3839df5 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -337,6 +337,13 @@  static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.gamma_lut_size_property = prop;
 
+	prop = drm_property_create(dev,
+				   DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
+				   "IN_FORMATS", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.modifiers = prop;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index d3fc561d7b48..8a2d5765837a 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -62,6 +62,86 @@  static unsigned int drm_num_planes(struct drm_device *dev)
 	return num;
 }
 
+static inline u32 *
+formats_ptr(struct drm_format_modifier_blob *blob)
+{
+	return (u32 *)(((char *)blob) + blob->formats_offset);
+}
+
+static inline struct drm_format_modifier *
+modifiers_ptr(struct drm_format_modifier_blob *blob)
+{
+	return (struct drm_format_modifier *)(((char *)blob) + blob->modifiers_offset);
+}
+
+static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
+{
+	const struct drm_mode_config *config = &dev->mode_config;
+	struct drm_property_blob *blob;
+	struct drm_format_modifier *mod;
+	size_t blob_size, formats_size, modifiers_size;
+	struct drm_format_modifier_blob *blob_data;
+	unsigned int i, j;
+
+	formats_size = sizeof(*plane->format_types) * plane->format_count;
+	if (WARN_ON(!formats_size)) {
+		/* 0 formats are never expected */
+		return 0;
+	}
+
+	modifiers_size =
+		sizeof(struct drm_format_modifier) * plane->modifier_count;
+
+	blob_size = sizeof(struct drm_format_modifier_blob);
+	/* Modifiers offset is a pointer to a struct with a 64 bit field so it
+	 * should be naturally aligned to 8B.
+	 */
+	blob_size += ALIGN(formats_size, 8);
+	blob_size += modifiers_size;
+
+	blob = drm_property_create_blob(dev, blob_size, NULL);
+	if (IS_ERR(blob))
+		return -1;
+
+	blob_data = (struct drm_format_modifier_blob *)blob->data;
+	blob_data->version = FORMAT_BLOB_CURRENT;
+	blob_data->count_formats = plane->format_count;
+	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
+	blob_data->count_modifiers = plane->modifier_count;
+
+	blob_data->modifiers_offset =
+		ALIGN(blob_data->formats_offset + formats_size, 8);
+
+	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,
+							       plane->format_types[j],
+							       plane->modifiers[i])) {
+
+				mod->formats |= 1 << j;
+			}
+		}
+
+		mod->modifier = plane->modifiers[i];
+		mod->offset = 0;
+		mod->pad = 0;
+		mod++;
+	}
+
+done:
+	drm_object_attach_property(&plane->base, config->modifiers,
+				   blob->base.id);
+
+	return 0;
+}
+
 /**
  * drm_universal_plane_init - Initialize a new universal plane object
  * @dev: DRM device
@@ -181,6 +261,9 @@  int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
 	}
 
+	if (config->allow_fb_modifiers)
+		create_in_format_blob(dev, plane);
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_universal_plane_init);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 42981711189b..03776e659811 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -757,6 +757,12 @@  struct drm_mode_config {
 	 */
 	bool allow_fb_modifiers;
 
+	/**
+	 * @modifiers: Plane property to list support modifier/format
+	 * combination.
+	 */
+	struct drm_property *modifiers;
+
 	/* cursor size */
 	uint32_t cursor_width, cursor_height;
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 403339f98a92..a2bb7161f020 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -712,6 +712,56 @@  struct drm_mode_atomic {
 	__u64 user_data;
 };
 
+struct drm_format_modifier_blob {
+#define FORMAT_BLOB_CURRENT 1
+	/* Version of this blob format */
+	u32 version;
+
+	/* Flags */
+	u32 flags;
+
+	/* Number of fourcc formats supported */
+	u32 count_formats;
+
+	/* Where in this blob the formats exist (in bytes) */
+	u32 formats_offset;
+
+	/* Number of drm_format_modifiers */
+	u32 count_modifiers;
+
+	/* Where in this blob the modifiers exist (in bytes) */
+	u32 modifiers_offset;
+
+	/* u32 formats[] */
+	/* struct drm_format_modifier modifiers[] */
+};
+
+struct drm_format_modifier {
+	/* Bitmask of formats in get_plane format list this info applies to. The
+	 * offset allows a sliding window of which 64 formats (bits).
+	 *
+	 * Some examples:
+	 * In today's world with < 65 formats, and formats 0, and 2 are
+	 * supported
+	 * 0x0000000000000005
+	 *		  ^-offset = 0, formats = 5
+	 *
+	 * If the number formats grew to 128, and formats 98-102 are
+	 * supported with the modifier:
+	 *
+	 * 0x0000003c00000000 0000000000000000
+	 *		  ^
+	 *		  |__offset = 64, formats = 0x3c00000000
+	 *
+	 */
+	__u64 formats;
+	__u32 offset;
+	__u32 pad;
+
+	/* The modifier that applies to the >get_plane format list bitmask. */
+	__u64 modifier;
+};
+
 /**
  * Create a new 'blob' data property, copying length bytes from data pointer,
  * and returning new blob ID.