[v2,2/3] drm: Create a format/modifier blob

Submitted by Ben Widawsky on May 16, 2017, 9:31 p.m.

Details

Message ID 20170516213126.8269-2-ben@bwidawsk.net
State New
Headers show
Series "Series without cover letter" ( rev: 2 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ben Widawsky May 16, 2017, 9:31 p.m.
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)

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

Patch hide | download patch | download mbox

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 20203871e06d..f5b032b5f761 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -62,6 +62,92 @@  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;
+	const uint64_t *temp_modifiers = plane->modifiers;
+	unsigned int format_modifier_count = 0;
+	struct drm_property_blob *blob = NULL;
+	struct drm_format_modifier *mod;
+	size_t blob_size = 0, formats_size, modifiers_size;
+	struct drm_format_modifier_blob *blob_data;
+	int i, j, ret = 0;
+
+	if (plane->modifiers)
+		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
+			format_modifier_count++;
+
+	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) * format_modifier_count;
+
+	blob_size = sizeof(struct drm_format_modifier_blob);
+	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 = format_modifier_count;
+
+	/* Modifiers offset is a pointer to a struct with a 64 bit field so it
+	 * should be naturally aligned to 8B.
+	 */
+	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 < format_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 ret;
+}
+
 /**
  * drm_universal_plane_init - Initialize a new universal plane object
  * @dev: DRM device
@@ -180,6 +266,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 8c67fc03d53d..7ad567903fdc 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -665,6 +665,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.

Comments

On Tue, May 16, 2017 at 02:31:25PM -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)

Thanks for updating the patch!

> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> Cc: Liviu Dudau <liviu@dudau.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Daniel Stone <daniels@collabora.com>

Looks good to me!
Reviewed-by: Liviu Dudau <liviu@dudau.co.uk>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/drm_mode_config.c |  7 +++
>  drivers/gpu/drm/drm_plane.c       | 89 +++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h     |  6 +++
>  include/uapi/drm/drm_mode.h       | 50 ++++++++++++++++++++++
>  4 files changed, 152 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 20203871e06d..f5b032b5f761 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -62,6 +62,92 @@ 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;
> +	const uint64_t *temp_modifiers = plane->modifiers;
> +	unsigned int format_modifier_count = 0;
> +	struct drm_property_blob *blob = NULL;
> +	struct drm_format_modifier *mod;
> +	size_t blob_size = 0, formats_size, modifiers_size;
> +	struct drm_format_modifier_blob *blob_data;
> +	int i, j, ret = 0;
> +
> +	if (plane->modifiers)
> +		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> +			format_modifier_count++;
> +
> +	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) * format_modifier_count;
> +
> +	blob_size = sizeof(struct drm_format_modifier_blob);
> +	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 = format_modifier_count;
> +
> +	/* Modifiers offset is a pointer to a struct with a 64 bit field so it
> +	 * should be naturally aligned to 8B.
> +	 */
> +	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 < format_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 ret;
> +}
> +
>  /**
>   * drm_universal_plane_init - Initialize a new universal plane object
>   * @dev: DRM device
> @@ -180,6 +266,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 8c67fc03d53d..7ad567903fdc 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -665,6 +665,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.0
>
Hi Ben,

On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> 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)
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> Cc: Liviu Dudau <liviu@dudau.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Daniel Stone <daniels@collabora.com>

I think this is almost perfect, barring the UAPI nitpick.
The rest is somewhat of a bikeshedding.

With the UAPI resolved, regardless of the rest
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>


> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c

> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
> +{
> +       const struct drm_mode_config *config = &dev->mode_config;
> +       const uint64_t *temp_modifiers = plane->modifiers;
> +       unsigned int format_modifier_count = 0;
> +       struct drm_property_blob *blob = NULL;
> +       struct drm_format_modifier *mod;
> +       size_t blob_size = 0, formats_size, modifiers_size;
There's no need to initialize blob and blob_size here.

> +       struct drm_format_modifier_blob *blob_data;
> +       int i, j, ret = 0;
Make i and j unsigned to match format_modifier_count and
plane->format_count respectively.
Then expand ret in the only place where it's used?

> +
> +       if (plane->modifiers)
> +               while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> +                       format_modifier_count++;
> +
> +       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) * format_modifier_count;
> +
> +       blob_size = sizeof(struct drm_format_modifier_blob);
> +       blob_size += ALIGN(formats_size, 8);
Worth having the "Modifiers offset is a pointer..." comment moved/copied here?

> +       blob_size += modifiers_size;
> +
> +       blob = drm_property_create_blob(dev, blob_size, NULL);
> +       if (IS_ERR(blob))
> +               return -1;
> +
Maybe propagate the exact error... Hmm we don't seem to check if
create_in_format_blob fails either so perhaps it's not worth it.


> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -665,6 +665,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;
Please drop the leading __ from the type names in UAPI headers.

Regards,
Emil
On 17-05-17 01:06:16, Emil Velikov wrote:
>Hi Ben,
>
>On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> 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)
>>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Daniel Stone <daniels@collabora.com>
>> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
>> Cc: Liviu Dudau <liviu@dudau.co.uk>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> Reviewed-by: Daniel Stone <daniels@collabora.com>
>
>I think this is almost perfect, barring the UAPI nitpick.
>The rest is somewhat of a bikeshedding.
>
>With the UAPI resolved, regardless of the rest
>Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>
>
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>
>> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
>> +{
>> +       const struct drm_mode_config *config = &dev->mode_config;
>> +       const uint64_t *temp_modifiers = plane->modifiers;
>> +       unsigned int format_modifier_count = 0;
>> +       struct drm_property_blob *blob = NULL;
>> +       struct drm_format_modifier *mod;
>> +       size_t blob_size = 0, formats_size, modifiers_size;
>There's no need to initialize blob and blob_size here.
>
>> +       struct drm_format_modifier_blob *blob_data;
>> +       int i, j, ret = 0;
>Make i and j unsigned to match format_modifier_count and
>plane->format_count respectively.
>Then expand ret in the only place where it's used?
>

Oh. ret has lost it's utility over the iterations of this patch. Make i and j
unsigned and dropped ret.

>> +
>> +       if (plane->modifiers)
>> +               while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
>> +                       format_modifier_count++;
>> +
>> +       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) * format_modifier_count;
>> +
>> +       blob_size = sizeof(struct drm_format_modifier_blob);
>> +       blob_size += ALIGN(formats_size, 8);
>Worth having the "Modifiers offset is a pointer..." comment moved/copied here?
>

Yes it is.

>> +       blob_size += modifiers_size;
>> +
>> +       blob = drm_property_create_blob(dev, blob_size, NULL);
>> +       if (IS_ERR(blob))
>> +               return -1;
>> +
>Maybe propagate the exact error... Hmm we don't seem to check if
>create_in_format_blob fails either so perhaps it's not worth it.
>
>

In this case it's almost definitely ENOMEM. All values should be verified - I
think the other errors are there for when userspace is utilizing blob creation.

So I'll just leave it.

>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -665,6 +665,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:
G>> +        *
>> +        * 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;
>Please drop the leading __ from the type names in UAPI headers.
>

Many other structures have the __, can you explain why please (this has probably
been beaten to death already; but I don't know)?

>Regards,
>Emil
On Wed, May 17, 2017 at 05:46:18PM -0700, Ben Widawsky wrote:
> On 17-05-17 01:06:16, Emil Velikov wrote:
> >Hi Ben,
> >
> >On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> 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)
> >>
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Cc: Daniel Stone <daniels@collabora.com>
> >> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> >> Cc: Liviu Dudau <liviu@dudau.co.uk>
> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >> Reviewed-by: Daniel Stone <daniels@collabora.com>
> >
> >I think this is almost perfect, barring the UAPI nitpick.
> >The rest is somewhat of a bikeshedding.
> >
> >With the UAPI resolved, regardless of the rest
> >Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> >
> >
> >> --- a/drivers/gpu/drm/drm_plane.c
> >> +++ b/drivers/gpu/drm/drm_plane.c
> >
> >> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
> >> +{
> >> +       const struct drm_mode_config *config = &dev->mode_config;
> >> +       const uint64_t *temp_modifiers = plane->modifiers;
> >> +       unsigned int format_modifier_count = 0;
> >> +       struct drm_property_blob *blob = NULL;
> >> +       struct drm_format_modifier *mod;
> >> +       size_t blob_size = 0, formats_size, modifiers_size;
> >There's no need to initialize blob and blob_size here.
> >
> >> +       struct drm_format_modifier_blob *blob_data;
> >> +       int i, j, ret = 0;
> >Make i and j unsigned to match format_modifier_count and
> >plane->format_count respectively.
> >Then expand ret in the only place where it's used?
> >
> 
> Oh. ret has lost it's utility over the iterations of this patch. Make i and j
> unsigned and dropped ret.

Unsigned loop iterators will likely bite someone some day, especially
if they're called something like 'i', 'j', etc. IMO it's best to keep
them signed.

> 
> >> +
> >> +       if (plane->modifiers)
> >> +               while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> >> +                       format_modifier_count++;
> >> +
> >> +       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) * format_modifier_count;
> >> +
> >> +       blob_size = sizeof(struct drm_format_modifier_blob);
> >> +       blob_size += ALIGN(formats_size, 8);
> >Worth having the "Modifiers offset is a pointer..." comment moved/copied here?
> >
> 
> Yes it is.
> 
> >> +       blob_size += modifiers_size;
> >> +
> >> +       blob = drm_property_create_blob(dev, blob_size, NULL);
> >> +       if (IS_ERR(blob))
> >> +               return -1;
> >> +
> >Maybe propagate the exact error... Hmm we don't seem to check if
> >create_in_format_blob fails either so perhaps it's not worth it.
> >
> >
> 
> In this case it's almost definitely ENOMEM. All values should be verified - I
> think the other errors are there for when userspace is utilizing blob creation.
> 
> So I'll just leave it.
> 
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -665,6 +665,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:
> G>> +        *
> >> +        * 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;
> >Please drop the leading __ from the type names in UAPI headers.
> >
> 
> Many other structures have the __, can you explain why please (this has probably
> been beaten to death already; but I don't know)?

__ is the rigth choice for uapi headers, unless the rules have changed
recently.
On 18 May 2017 at 01:46, Ben Widawsky <ben@bwidawsk.net> wrote:

>>> +       blob_size += modifiers_size;
>>> +
>>> +       blob = drm_property_create_blob(dev, blob_size, NULL);
>>> +       if (IS_ERR(blob))
>>> +               return -1;
>>> +
>>
>> Maybe propagate the exact error... Hmm we don't seem to check if
>> create_in_format_blob fails either so perhaps it's not worth it.
>>
>>
>
> In this case it's almost definitely ENOMEM. All values should be verified -
> I
> think the other errors are there for when userspace is utilizing blob
> creation.
>
> So I'll just leave it.
>
Ack, sure thing.

>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -665,6 +665,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:
>
> G>> +        *
>>>
>>> +        * 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;
>>
>> Please drop the leading __ from the type names in UAPI headers.
>>
>
> Many other structures have the __, can you explain why please (this has
> probably
> been beaten to death already; but I don't know)?
>
Got confused there for a moment, apologies.

Using the __ types is correct. It is drm_format_modifier_blob that
should be updated to follow suit.

-Emil
Hi Ben,

On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> wrote:
> Updated blob layout (Rob, Daniel, Kristian, xerpi)

If you take the attached fixup, this is:
Reviewed-by: Daniel Stone <daniels@collabora.com>

The rest of the series looks fine to me, so you can take my Acked-by,
but I'm currently off work sick and am away next week, so please don't
block on me for merging.

Cheers,
Daniel
commit 250f3b2c2a824516b18fe21623ddc86ccf31eddb
Author: Daniel Stone <daniels@collabora.com>
Date:   Tue May 23 17:36:55 2017 +0100

    fixup! drm: Create a format/modifier blob