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

Submitted by Ben Widawsky on May 3, 2017, 5:14 a.m.

Details

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

Not browsing as part of any series.

Commit Message

Ben Widawsky May 3, 2017, 5:14 a.m.
Updated blob layout (Rob, Daniel, Kristian, xerpi)

Cc: Rob Clark <robdclark@gmail.com>
Cc: Daniel Stone <daniels@collabora.com>
Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/drm_mode_config.c |   7 +++
 drivers/gpu/drm/drm_plane.c       | 119 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_mode_config.h     |   6 ++
 include/uapi/drm/drm_mode.h       |  26 +++++++++
 4 files changed, 158 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 286e183891e5..2e89e0e73435 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -62,6 +62,117 @@  static unsigned int drm_num_planes(struct drm_device *dev)
 	return num;
 }
 
+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[] */
+} __packed;
+
+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_plane_funcs *funcs,
+				 const uint32_t *formats, unsigned int format_count,
+				 const uint64_t *format_modifiers)
+{
+	const struct drm_mode_config *config = &dev->mode_config;
+	const uint64_t *temp_modifiers = format_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 (format_modifiers)
+		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
+			format_modifier_count++;
+
+	formats_size = sizeof(__u32) * 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 = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
+	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 = 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), formats, formats_size);
+
+	/* If we can't determine support, just bail */
+	if (!funcs->format_mod_supported)
+		goto done;
+
+	mod = modifiers_ptr(blob_data);
+	for (i = 0; i < format_modifier_count; i++) {
+		for (j = 0; j < format_count; j++) {
+			if (funcs->format_mod_supported(plane, formats[j],
+							format_modifiers[i])) {
+				mod->formats |= 1 << j;
+			}
+		}
+
+		mod->modifier = format_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
@@ -130,6 +241,10 @@  int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		return -ENOMEM;
 	}
 
+	/* First driver to need more than 64 formats needs to fix this */
+	if (WARN_ON(format_count > 64))
+		return -EINVAL;
+
 	if (name) {
 		va_list ap;
 
@@ -177,6 +292,10 @@  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, funcs, formats, format_count,
+				      format_modifiers);
+
 	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..dcdd04c55792 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -665,6 +665,32 @@  struct drm_mode_atomic {
 	__u64 user_data;
 };
 
+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
+	*
+	*/
+       uint64_t formats;
+       uint32_t offset;
+       uint32_t pad;
+
+       /* This modifier can be used with the format for this plane. */
+       uint64_t modifier;
+} __packed;
+
 /**
  * Create a new 'blob' data property, copying length bytes from data pointer,
  * and returning new blob ID.

Comments

Hi,

Having just caught up on IRC I'm sure I'm far too late for this party,
but...

Wouldn't this whole thing be a whole lot simpler if "IN_FORMATS"
stored "pointers" to separate blobs for the format and modifier lists?

struct drm_format_modifier_blob {
#define FORMAT_BLOB_CURRENT 1
	/* Version of this blob format */
	u32 version;

	/* Flags */
	u32 flags;

	/* ID of a blob storing an array of FourCCs */
	u32 formats_blob_id;

	/* ID of a blob storing an array of drm_format_modifiers */
	u32 modifiers_blob_id;
} __packed;

I've used/written a few interfaces with offsets to
variable-length-arrays and IMO the code to handle them is always
hideous.

Added bonus: With smart enough helper code the FourCC and modifiers
blobs can be shared across planes.

-Brian

On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>Updated blob layout (Rob, Daniel, Kristian, xerpi)
>
>Cc: Rob Clark <robdclark@gmail.com>
>Cc: Daniel Stone <daniels@collabora.com>
>Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
>Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>---
> drivers/gpu/drm/drm_mode_config.c |   7 +++
> drivers/gpu/drm/drm_plane.c       | 119 ++++++++++++++++++++++++++++++++++++++
> include/drm/drm_mode_config.h     |   6 ++
> include/uapi/drm/drm_mode.h       |  26 +++++++++
> 4 files changed, 158 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 286e183891e5..2e89e0e73435 100644
>--- a/drivers/gpu/drm/drm_plane.c
>+++ b/drivers/gpu/drm/drm_plane.c
>@@ -62,6 +62,117 @@ static unsigned int drm_num_planes(struct drm_device *dev)
> 	return num;
> }
>
>+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[] */
>+} __packed;
>+
>+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_plane_funcs *funcs,
>+				 const uint32_t *formats, unsigned int format_count,
>+				 const uint64_t *format_modifiers)
>+{
>+	const struct drm_mode_config *config = &dev->mode_config;
>+	const uint64_t *temp_modifiers = format_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 (format_modifiers)
>+		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
>+			format_modifier_count++;
>+
>+	formats_size = sizeof(__u32) * 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 = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>+	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 = 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), formats, formats_size);
>+
>+	/* If we can't determine support, just bail */
>+	if (!funcs->format_mod_supported)
>+		goto done;
>+
>+	mod = modifiers_ptr(blob_data);
>+	for (i = 0; i < format_modifier_count; i++) {
>+		for (j = 0; j < format_count; j++) {
>+			if (funcs->format_mod_supported(plane, formats[j],
>+							format_modifiers[i])) {
>+				mod->formats |= 1 << j;
>+			}
>+		}
>+
>+		mod->modifier = format_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
>@@ -130,6 +241,10 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> 		return -ENOMEM;
> 	}
>
>+	/* First driver to need more than 64 formats needs to fix this */
>+	if (WARN_ON(format_count > 64))
>+		return -EINVAL;
>+
> 	if (name) {
> 		va_list ap;
>
>@@ -177,6 +292,10 @@ 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, funcs, formats, format_count,
>+				      format_modifiers);
>+
> 	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..dcdd04c55792 100644
>--- a/include/uapi/drm/drm_mode.h
>+++ b/include/uapi/drm/drm_mode.h
>@@ -665,6 +665,32 @@ struct drm_mode_atomic {
> 	__u64 user_data;
> };
>
>+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
>+	*
>+	*/
>+       uint64_t formats;
>+       uint32_t offset;
>+       uint32_t pad;
>+
>+       /* This modifier can be used with the format for this plane. */
>+       uint64_t modifier;
>+} __packed;
>+
> /**
>  * Create a new 'blob' data property, copying length bytes from data pointer,
>  * and returning new blob ID.
>-- 
>2.12.2
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Brian,

On 3 May 2017 at 12:00, Brian Starkey <brian.starkey@arm.com> wrote:
> Having just caught up on IRC I'm sure I'm far too late for this party,
> but...
>
> Wouldn't this whole thing be a whole lot simpler if "IN_FORMATS"
> stored "pointers" to separate blobs for the format and modifier lists?
>
> I've used/written a few interfaces with offsets to
> variable-length-arrays and IMO the code to handle them is always
> hideous.

I don't agree ...

The idea is that the header can grow because the offsets allow it to;
adding a new member slots in between the end of static data and the
start of variable data, and since all the variable data is accessed by
an array, userspace doesn't have to be aware of the new members. The
code for that is very clean (modulo the casts for pointer maths), cf.
formats_ptr and modifiers_ptr; I'd expect userspace to copy and use
those with version guards:
    uint32_t *formats = formats_ptr(blob);
    struct drm_format_modifier *modifiers = modifiers_ptr(blob);
    struct drm_format_unifier *unifiiers = (blob->version >= 2) ?
unifiers_ptr(blob) : NULL;

That's shorter than fetching separate blobs, and doesn't have multiple
error paths either. What am I missing?

> Added bonus: With smart enough helper code the FourCC and modifiers
> blobs can be shared across planes.

I think this is a serious case of premature optimisation; the memory
savings might be outweighed by the additional number of objects to
track, and userspace would have to jump through serious hoops with a
blob ID cache - something which is a very bad idea regardless - to
take any advantage of it.

Cheers,
Daniel
Hi Daniel,

On Wed, May 03, 2017 at 12:47:18PM +0100, Daniel Stone wrote:
>Hi Brian,
>
>On 3 May 2017 at 12:00, Brian Starkey <brian.starkey@arm.com> wrote:
>> Having just caught up on IRC I'm sure I'm far too late for this party,
>> but...
>>
>> Wouldn't this whole thing be a whole lot simpler if "IN_FORMATS"
>> stored "pointers" to separate blobs for the format and modifier lists?
>>
>> I've used/written a few interfaces with offsets to
>> variable-length-arrays and IMO the code to handle them is always
>> hideous.
>
>I don't agree ...
>
>The idea is that the header can grow because the offsets allow it to;
>adding a new member slots in between the end of static data and the
>start of variable data, and since all the variable data is accessed by
>an array, userspace doesn't have to be aware of the new members. 

This is the same in both approaches.

> The
>code for that is very clean (modulo the casts for pointer maths), cf.
>formats_ptr and modifiers_ptr; I'd expect userspace to copy and use
>those with version guards:
>    uint32_t *formats = formats_ptr(blob);
>    struct drm_format_modifier *modifiers = modifiers_ptr(blob);
>    struct drm_format_unifier *unifiiers = (blob->version >= 2) ?
>unifiers_ptr(blob) : NULL;

I concede the tricky code is all confined to the single implementation
in the kernel (encoding is far harder than decoding) - I just think
create_in_format_blob() is cumbersome, ugly and error-prone.

>
>That's shorter than fetching separate blobs, and doesn't have multiple
>error paths either. What am I missing?

Yes, this is a convincing argument.

>
>> Added bonus: With smart enough helper code the FourCC and modifiers
>> blobs can be shared across planes.
>
>I think this is a serious case of premature optimisation; the memory
>savings might be outweighed by the additional number of objects to
>track, and userspace would have to jump through serious hoops with a
>blob ID cache - something which is a very bad idea regardless - to
>take any advantage of it.

Hey I'm just saying it's an option - not that it should be implemented
in V1.

Where both the formats and the modifiers are the same, Ben's approach
lets the blob be shared anyway.

Thanks,
Brian

>
>Cheers, Daniel
Hi,

On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>Updated blob layout (Rob, Daniel, Kristian, xerpi)
>
>Cc: Rob Clark <robdclark@gmail.com>
>Cc: Daniel Stone <daniels@collabora.com>
>Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
>Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>---
> drivers/gpu/drm/drm_mode_config.c |   7 +++
> drivers/gpu/drm/drm_plane.c       | 119 ++++++++++++++++++++++++++++++++++++++
> include/drm/drm_mode_config.h     |   6 ++
> include/uapi/drm/drm_mode.h       |  26 +++++++++
> 4 files changed, 158 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 286e183891e5..2e89e0e73435 100644
>--- a/drivers/gpu/drm/drm_plane.c
>+++ b/drivers/gpu/drm/drm_plane.c
>@@ -62,6 +62,117 @@ static unsigned int drm_num_planes(struct drm_device *dev)
> 	return num;
> }
>
>+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[] */
>+} __packed;
>+
>+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_plane_funcs *funcs,
>+				 const uint32_t *formats, unsigned int format_count,
>+				 const uint64_t *format_modifiers)
>+{
>+	const struct drm_mode_config *config = &dev->mode_config;
>+	const uint64_t *temp_modifiers = format_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 (format_modifiers)
>+		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
>+			format_modifier_count++;
>+
>+	formats_size = sizeof(__u32) * 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 = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>+	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 = format_count;
>+	blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);

This looks to be missing some alignment.

Definitely needs to be at least to 4 bytes, but given you aligned
it up to 8 for the initial "blob_size" I assume the intention was to
put the formats on the next 8-byte aligned address after the end of
the struct, e.g.:

	blob_data->formats_offset = ALIGN(sizeof(struct drm_format_modifier_blob), 8);

-Brian

>+	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), formats, formats_size);
>+
>+	/* If we can't determine support, just bail */
>+	if (!funcs->format_mod_supported)
>+		goto done;
>+
>+	mod = modifiers_ptr(blob_data);
>+	for (i = 0; i < format_modifier_count; i++) {
>+		for (j = 0; j < format_count; j++) {
>+			if (funcs->format_mod_supported(plane, formats[j],
>+							format_modifiers[i])) {
>+				mod->formats |= 1 << j;
>+			}
>+		}
>+
>+		mod->modifier = format_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
>@@ -130,6 +241,10 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> 		return -ENOMEM;
> 	}
>
>+	/* First driver to need more than 64 formats needs to fix this */
>+	if (WARN_ON(format_count > 64))
>+		return -EINVAL;
>+
> 	if (name) {
> 		va_list ap;
>
>@@ -177,6 +292,10 @@ 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, funcs, formats, format_count,
>+				      format_modifiers);
>+
> 	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..dcdd04c55792 100644
>--- a/include/uapi/drm/drm_mode.h
>+++ b/include/uapi/drm/drm_mode.h
>@@ -665,6 +665,32 @@ struct drm_mode_atomic {
> 	__u64 user_data;
> };
>
>+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
>+	*
>+	*/
>+       uint64_t formats;
>+       uint32_t offset;
>+       uint32_t pad;
>+
>+       /* This modifier can be used with the format for this plane. */
>+       uint64_t modifier;
>+} __packed;
>+
> /**
>  * Create a new 'blob' data property, copying length bytes from data pointer,
>  * and returning new blob ID.
>-- 
>2.12.2
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
> Updated blob layout (Rob, Daniel, Kristian, xerpi)
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/drm_mode_config.c |   7 +++
>  drivers/gpu/drm/drm_plane.c       | 119 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h     |   6 ++
>  include/uapi/drm/drm_mode.h       |  26 +++++++++
>  4 files changed, 158 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 286e183891e5..2e89e0e73435 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -62,6 +62,117 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>  	return num;
>  }
>  
> +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[] */
> +} __packed;
> +
> +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_plane_funcs *funcs,
> +				 const uint32_t *formats, unsigned int format_count,
> +				 const uint64_t *format_modifiers)

Why do you have to pass the format_modifiers again when you have plane->modifiers?
Or the reverse question: why do you need plane->modifiers when you are passing the
modifiers as parameters all the time?

> +{
> +	const struct drm_mode_config *config = &dev->mode_config;
> +	const uint64_t *temp_modifiers = format_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 (format_modifiers)
> +		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> +			format_modifier_count++;
> +
> +	formats_size = sizeof(__u32) * 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 = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
> +	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 = 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), formats, formats_size);
> +
> +	/* If we can't determine support, just bail */
> +	if (!funcs->format_mod_supported)
> +		goto done;
> +
> +	mod = modifiers_ptr(blob_data);
> +	for (i = 0; i < format_modifier_count; i++) {
> +		for (j = 0; j < format_count; j++) {
> +			if (funcs->format_mod_supported(plane, formats[j],
> +							format_modifiers[i])) {
> +				mod->formats |= 1 << j;
> +			}
> +		}
> +
> +		mod->modifier = format_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
> @@ -130,6 +241,10 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		return -ENOMEM;
>  	}
>  
> +	/* First driver to need more than 64 formats needs to fix this */
> +	if (WARN_ON(format_count > 64))
> +		return -EINVAL;
> +

Applying this patch makes the above check appear twice in drm_universal_plane_init() function.

>  	if (name) {
>  		va_list ap;
>  
> @@ -177,6 +292,10 @@ 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, funcs, formats, format_count,
> +				      format_modifiers);
> +
>  	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.

Comment says "Plane property" yet this is struct drm_mode_config.

> +	 */
> +	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..dcdd04c55792 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -665,6 +665,32 @@ struct drm_mode_atomic {
>  	__u64 user_data;
>  };
>  
> +struct drm_format_modifier {
> +       /* Bitmask of formats in get_plane format list this info applies to. The

You have some unusual indent of the comment here. Also the file uses a different
style for multi-line comments.

> +	* 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
> +	*
> +	*/
> +       uint64_t formats;
> +       uint32_t offset;
> +       uint32_t pad;

s/pad/__pad/ or s/pad/__reserved/ ?

> +
> +       /* This modifier can be used with the format for this plane. */

I'm having trouble parsing the comment. How about: "The modifier that applies to the
get_plane format list bitmask." ?

Best regards,
Liviu

> +       uint64_t modifier;
> +} __packed;
> +
>  /**
>   * Create a new 'blob' data property, copying length bytes from data pointer,
>   * and returning new blob ID.
> -- 
> 2.12.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Ben,

On 3 May 2017 at 06:14, Ben Widawsky <ben@bwidawsk.net> wrote:

> +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[] */
> +} __packed;
> +
Why do we need packing here? Both layout and member offsets are
identical with and w/o it.
If there's something subtle to it, please add a comment.


> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane,
> +                                const struct drm_plane_funcs *funcs,
> +                                const uint32_t *formats, unsigned int format_count,
> +                                const uint64_t *format_modifiers)
> +{

> +       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
> +       blob_size += ALIGN(formats_size, 8);
The 8 is "sizeof(void *) isn't it? Shouldn't we use that instead since
it expands to 4 for 32bit systems?

> +       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 = format_count;

> +       blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
Shouldn't we reuse the ALIGN(...) from above?


> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 8c67fc03d53d..dcdd04c55792 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -665,6 +665,32 @@ struct drm_mode_atomic {
>         __u64 user_data;
>  };
>
> +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
> +       *
> +       */
> +       uint64_t formats;
> +       uint32_t offset;
> +       uint32_t pad;
> +
> +       /* This modifier can be used with the format for this plane. */
> +       uint64_t modifier;
> +} __packed;
> +
As above - why __packed?

Regards,
Emil
Hi Brian,

On 3 May 2017 at 13:51, Brian Starkey <brian.starkey@arm.com> wrote:
> On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>> +       modifiers_size =
>> +               sizeof(struct drm_format_modifier) *
>> format_modifier_count;
>> +
>> +       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>> +       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 = format_count;
>> +       blob_data->formats_offset = sizeof(struct
>> drm_format_modifier_blob);
>
> This looks to be missing some alignment.
>
> Definitely needs to be at least to 4 bytes, but given you aligned
> it up to 8 for the initial "blob_size" I assume the intention was to
> put the formats on the next 8-byte aligned address after the end of
> the struct, e.g.:
>
>         blob_data->formats_offset = ALIGN(sizeof(struct
> drm_format_modifier_blob), 8);

It's fairly subtle, but I think it's correct.

formats_offset is the end of the fixed-size element, which is
currently aligned to 32 bytes, and practically speaking would always
have to be anyway. As it is an array of uint32_t, this gives natural
alignment.

If we have an odd number of formats supported, the formats[] array
will end on a 4-byte rather than 8-byte boundary, so the ALIGN() on
formats_size guarantees that modifiers_offset will be aligned to an
8-byte quantity, which is required as it has 64-bit elements.

The size of a pointer is not relevant since we're not passing pointers
across the kernel/userspace boundary, just offsets within a struct.
The alignment of those offsets has to correspond to the types located
at those offsets, i.e. 4 bytes for formats (guaranteed by fixed header
layout), and 8 bytes for modifiers (guaranteed by explicit alignment).

Cheers,
Daniel
On Wed, May 03, 2017 at 02:51:18PM +0100, Daniel Stone wrote:
>Hi Brian,
>
>On 3 May 2017 at 13:51, Brian Starkey <brian.starkey@arm.com> wrote:
>> On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>>> +       modifiers_size =
>>> +               sizeof(struct drm_format_modifier) *
>>> format_modifier_count;
>>> +
>>> +       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>>> +       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 = format_count;
>>> +       blob_data->formats_offset = sizeof(struct
>>> drm_format_modifier_blob);
>>
>> This looks to be missing some alignment.
>>
>> Definitely needs to be at least to 4 bytes, but given you aligned
>> it up to 8 for the initial "blob_size" I assume the intention was to
>> put the formats on the next 8-byte aligned address after the end of
>> the struct, e.g.:
>>
>>         blob_data->formats_offset = ALIGN(sizeof(struct
>> drm_format_modifier_blob), 8);
>
>It's fairly subtle, but I think it's correct.

It's the exact subtlety that I was concerned about.

>
>formats_offset is the end of the fixed-size element, which is
>currently aligned to 32 bytes, and practically speaking would always
>have to be anyway. As it is an array of uint32_t, this gives natural
>alignment.

Why must it always be? The __packed attribute means it'll have no
padding - so any u16 or u8 added to the end will break it - putting
the formats array on a non-aligned boundary.

If the assumption is that the struct will always be made of only
u32/u64 members (and the implementation will break otherwise) then
there had better be a really big comment saying so, and preferably a
compile-time assertion too.

I'm missing the reason for it being __packed in the first place -
perhaps that's just a left over and needs to be removed.

Either way, this line aligns to 8:

+       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);

...and the rest of the blob_size calculation looks like it assumes the
formats array starts at that 8-byte boundary. So, for clarity and
consistency I reckon the blob_size code and the code that builds the
blob should do the same thing.

Cheers,
-Brian

>
>If we have an odd number of formats supported, the formats[] array
>will end on a 4-byte rather than 8-byte boundary, so the ALIGN() on
>formats_size guarantees that modifiers_offset will be aligned to an
>8-byte quantity, which is required as it has 64-bit elements.
>
>The size of a pointer is not relevant since we're not passing pointers
>across the kernel/userspace boundary, just offsets within a struct.
>The alignment of those offsets has to correspond to the types located
>at those offsets, i.e. 4 bytes for formats (guaranteed by fixed header
>layout), and 8 bytes for modifiers (guaranteed by explicit alignment).
>
>Cheers,
>Daniel
Hi Brian,

On 3 May 2017 at 15:03, Brian Starkey <brian.starkey@arm.com> wrote:
> On Wed, May 03, 2017 at 02:51:18PM +0100, Daniel Stone wrote:
>> formats_offset is the end of the fixed-size element, which is
>> currently aligned to 32 bytes, and practically speaking would always
>> have to be anyway. As it is an array of uint32_t, this gives natural
>> alignment.
>
> Why must it always be? The __packed attribute means it'll have no
> padding - so any u16 or u8 added to the end will break it - putting
> the formats array on a non-aligned boundary.
>
> If the assumption is that the struct will always be made of only
> u32/u64 members (and the implementation will break otherwise) then
> there had better be a really big comment saying so, and preferably a
> compile-time assertion too.

Indeed that's the case, for most ioctls at least. A comment would
definitely be in order.

> I'm missing the reason for it being __packed in the first place -
> perhaps that's just a left over and needs to be removed.
>
> Either way, this line aligns to 8:
>
> +       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>
> ...and the rest of the blob_size calculation looks like it assumes the
> formats array starts at that 8-byte boundary. So, for clarity and
> consistency I reckon the blob_size code and the code that builds the
> blob should do the same thing.

All this is correct - the __packed declaration is unnecessary, and so
is the rounding up when calculating the size. And with that fixed, I
believe it should be correct, no?

Cheers,
Daniel
Hi Daniel,

On Wed, May 03, 2017 at 03:07:40PM +0100, Daniel Stone wrote:
>Hi Brian,
>
>On 3 May 2017 at 15:03, Brian Starkey <brian.starkey@arm.com> wrote:
>> On Wed, May 03, 2017 at 02:51:18PM +0100, Daniel Stone wrote:
>>> formats_offset is the end of the fixed-size element, which is
>>> currently aligned to 32 bytes, and practically speaking would always
>>> have to be anyway. As it is an array of uint32_t, this gives natural
>>> alignment.
>>
>> Why must it always be? The __packed attribute means it'll have no
>> padding - so any u16 or u8 added to the end will break it - putting
>> the formats array on a non-aligned boundary.
>>
>> If the assumption is that the struct will always be made of only
>> u32/u64 members (and the implementation will break otherwise) then
>> there had better be a really big comment saying so, and preferably a
>> compile-time assertion too.
>
>Indeed that's the case, for most ioctls at least. A comment would
>definitely be in order.
>

No need for a comment if the code is fixed to do the right thing
irrespective of the value of sizeof(drm_formats_modifier_blob) - which
IMO is mandatory.

>> I'm missing the reason for it being __packed in the first place -
>> perhaps that's just a left over and needs to be removed.
>>
>> Either way, this line aligns to 8:
>>
>> +       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>>
>> ...and the rest of the blob_size calculation looks like it assumes the
>> formats array starts at that 8-byte boundary. So, for clarity and
>> consistency I reckon the blob_size code and the code that builds the
>> blob should do the same thing.
>
>All this is correct - the __packed declaration is unnecessary, and so
>is the rounding up when calculating the size. And with that fixed, I
>believe it should be correct, no?

Yes, with those problems fixed it looks correct to me.

Thanks,
-Brian

>
>Cheers,
>Daniel
On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
> Updated blob layout (Rob, Daniel, Kristian, xerpi)
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/drm_mode_config.c |   7 +++
>  drivers/gpu/drm/drm_plane.c       | 119 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h     |   6 ++
>  include/uapi/drm/drm_mode.h       |  26 +++++++++
>  4 files changed, 158 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 286e183891e5..2e89e0e73435 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -62,6 +62,117 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>  	return num;
>  }
>  
> +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[] */
> +} __packed;

The struct should be in the uapi header. Otherwise it won't show up in
libdrm headers when following the proper process.
-Daniel

> +
> +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_plane_funcs *funcs,
> +				 const uint32_t *formats, unsigned int format_count,
> +				 const uint64_t *format_modifiers)
> +{
> +	const struct drm_mode_config *config = &dev->mode_config;
> +	const uint64_t *temp_modifiers = format_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 (format_modifiers)
> +		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> +			format_modifier_count++;
> +
> +	formats_size = sizeof(__u32) * 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 = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
> +	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 = 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), formats, formats_size);
> +
> +	/* If we can't determine support, just bail */
> +	if (!funcs->format_mod_supported)
> +		goto done;
> +
> +	mod = modifiers_ptr(blob_data);
> +	for (i = 0; i < format_modifier_count; i++) {
> +		for (j = 0; j < format_count; j++) {
> +			if (funcs->format_mod_supported(plane, formats[j],
> +							format_modifiers[i])) {
> +				mod->formats |= 1 << j;
> +			}
> +		}
> +
> +		mod->modifier = format_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
> @@ -130,6 +241,10 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		return -ENOMEM;
>  	}
>  
> +	/* First driver to need more than 64 formats needs to fix this */
> +	if (WARN_ON(format_count > 64))
> +		return -EINVAL;
> +
>  	if (name) {
>  		va_list ap;
>  
> @@ -177,6 +292,10 @@ 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, funcs, formats, format_count,
> +				      format_modifiers);
> +
>  	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..dcdd04c55792 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -665,6 +665,32 @@ struct drm_mode_atomic {
>  	__u64 user_data;
>  };
>  
> +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
> +	*
> +	*/
> +       uint64_t formats;
> +       uint32_t offset;
> +       uint32_t pad;
> +
> +       /* This modifier can be used with the format for this plane. */
> +       uint64_t modifier;
> +} __packed;
> +
>  /**
>   * Create a new 'blob' data property, copying length bytes from data pointer,
>   * and returning new blob ID.
> -- 
> 2.12.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi,

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

In terms of the blob as uABI, we've got an implementation inside
Weston which works:
https://git.collabora.com/cgit/user/daniels/weston.git/commit/?h=wip/2017-04/atomic-v11-WIP&id=0a47cb63947e

That was authored by Sergi and reviewed by me. We both think it's
entirely acceptable and future-proof uABI, and it does exactly what we
want. We use it to both allocate with a suitable set of modifiers, as
well as a high-pass filter to avoid assigning FBs to planes which
won't accept the FB modifiers. So this gets my:
Acked-by: Daniel Stone <daniels@collabora.com>

And a future revision with the fixups found here would get my R-b.

Cheers,
Daniel
On 17-05-03 14:15:15, Liviu Dudau wrote:
>On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>> Updated blob layout (Rob, Daniel, Kristian, xerpi)
>>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Daniel Stone <daniels@collabora.com>
>> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>  drivers/gpu/drm/drm_mode_config.c |   7 +++
>>  drivers/gpu/drm/drm_plane.c       | 119 ++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_mode_config.h     |   6 ++
>>  include/uapi/drm/drm_mode.h       |  26 +++++++++
>>  4 files changed, 158 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 286e183891e5..2e89e0e73435 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -62,6 +62,117 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>>  	return num;
>>  }
>>
>> +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[] */
>> +} __packed;
>> +
>> +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_plane_funcs *funcs,
>> +				 const uint32_t *formats, unsigned int format_count,
>> +				 const uint64_t *format_modifiers)
>
>Why do you have to pass the format_modifiers again when you have plane->modifiers?
>Or the reverse question: why do you need plane->modifiers when you are passing the
>modifiers as parameters all the time?
>

I've dropped most of the parameters here and just used plane->*

>> +{
>> +	const struct drm_mode_config *config = &dev->mode_config;
>> +	const uint64_t *temp_modifiers = format_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 (format_modifiers)
>> +		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
>> +			format_modifier_count++;
>> +
>> +	formats_size = sizeof(__u32) * 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 = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>> +	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 = 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), formats, formats_size);
>> +
>> +	/* If we can't determine support, just bail */
>> +	if (!funcs->format_mod_supported)
>> +		goto done;
>> +
>> +	mod = modifiers_ptr(blob_data);
>> +	for (i = 0; i < format_modifier_count; i++) {
>> +		for (j = 0; j < format_count; j++) {
>> +			if (funcs->format_mod_supported(plane, formats[j],
>> +							format_modifiers[i])) {
>> +				mod->formats |= 1 << j;
>> +			}
>> +		}
>> +
>> +		mod->modifier = format_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
>> @@ -130,6 +241,10 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>>  		return -ENOMEM;
>>  	}
>>
>> +	/* First driver to need more than 64 formats needs to fix this */
>> +	if (WARN_ON(format_count > 64))
>> +		return -EINVAL;
>> +
>
>Applying this patch makes the above check appear twice in drm_universal_plane_init() function.
>

Fixed. Thanks.

>>  	if (name) {
>>  		va_list ap;
>>
>> @@ -177,6 +292,10 @@ 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, funcs, formats, format_count,
>> +				      format_modifiers);
>> +
>>  	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.
>
>Comment says "Plane property" yet this is struct drm_mode_config.
>

Yeah, it is a per plane property. It's just in the mode config for blob
creation. AFAICT, all props work this way. It's attached to the plane though:
drm_object_attach_property(&plane->base, config->modifiers, blob->base.id);

What would you like me to change here?


>> +	 */
>> +	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..dcdd04c55792 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -665,6 +665,32 @@ struct drm_mode_atomic {
>>  	__u64 user_data;
>>  };
>>
>> +struct drm_format_modifier {
>> +       /* Bitmask of formats in get_plane format list this info applies to. The
>
>You have some unusual indent of the comment here. Also the file uses a different
>style for multi-line comments.
>

Fixed the indent issue. Thanks for spotting that, it was a copy-paste problem on
my end. The file is inconsistent in multiline comments, actually. Not changed
anything there.

>> +	* 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
>> +	*
>> +	*/
>> +       uint64_t formats;
>> +       uint32_t offset;
>> +       uint32_t pad;
>
>s/pad/__pad/ or s/pad/__reserved/ ?
>

Every other structure in the file with a pad field is called "pad" however, you
made me realize my data types should be __u32 and __u64, those are fixed.

>> +
>> +       /* This modifier can be used with the format for this plane. */
>
>I'm having trouble parsing the comment. How about: "The modifier that applies to the
>get_plane format list bitmask." ?
>

Sure. Done.

>Best regards,
>Liviu
>
>> +       uint64_t modifier;
>> +} __packed;
>> +
>>  /**
>>   * Create a new 'blob' data property, copying length bytes from data pointer,
>>   * and returning new blob ID.
>> --
>> 2.12.2
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>-- 
>====================
>| I would like to |
>| fix the world,  |
>| but they're not |
>| giving me the   |
> \ source code!  /
>  ---------------
>    ¯\_(ツ)_/¯
On 17-05-03 17:08:27, Daniel Vetter wrote:
>On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>> Updated blob layout (Rob, Daniel, Kristian, xerpi)
>>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Daniel Stone <daniels@collabora.com>
>> Cc: Kristian H. Kristensen <hoegsberg@gmail.com>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>  drivers/gpu/drm/drm_mode_config.c |   7 +++
>>  drivers/gpu/drm/drm_plane.c       | 119 ++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_mode_config.h     |   6 ++
>>  include/uapi/drm/drm_mode.h       |  26 +++++++++
>>  4 files changed, 158 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 286e183891e5..2e89e0e73435 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -62,6 +62,117 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>>  	return num;
>>  }
>>
>> +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[] */
>> +} __packed;
>
>The struct should be in the uapi header. Otherwise it won't show up in
>libdrm headers when following the proper process.
>-Daniel
>

I don't agree that blobs are ever really part of the API, but it doesn't hurt to
move it... in other words, done.

>> +
>> +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_plane_funcs *funcs,
>> +				 const uint32_t *formats, unsigned int format_count,
>> +				 const uint64_t *format_modifiers)
>> +{
>> +	const struct drm_mode_config *config = &dev->mode_config;
>> +	const uint64_t *temp_modifiers = format_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 (format_modifiers)
>> +		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
>> +			format_modifier_count++;
>> +
>> +	formats_size = sizeof(__u32) * 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 = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
>> +	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 = 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), formats, formats_size);
>> +
>> +	/* If we can't determine support, just bail */
>> +	if (!funcs->format_mod_supported)
>> +		goto done;
>> +
>> +	mod = modifiers_ptr(blob_data);
>> +	for (i = 0; i < format_modifier_count; i++) {
>> +		for (j = 0; j < format_count; j++) {
>> +			if (funcs->format_mod_supported(plane, formats[j],
>> +							format_modifiers[i])) {
>> +				mod->formats |= 1 << j;
>> +			}
>> +		}
>> +
>> +		mod->modifier = format_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
>> @@ -130,6 +241,10 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>>  		return -ENOMEM;
>>  	}
>>
>> +	/* First driver to need more than 64 formats needs to fix this */
>> +	if (WARN_ON(format_count > 64))
>> +		return -EINVAL;
>> +
>>  	if (name) {
>>  		va_list ap;
>>
>> @@ -177,6 +292,10 @@ 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, funcs, formats, format_count,
>> +				      format_modifiers);
>> +
>>  	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..dcdd04c55792 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -665,6 +665,32 @@ struct drm_mode_atomic {
>>  	__u64 user_data;
>>  };
>>
>> +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
>> +	*
>> +	*/
>> +       uint64_t formats;
>> +       uint32_t offset;
>> +       uint32_t pad;
>> +
>> +       /* This modifier can be used with the format for this plane. */
>> +       uint64_t modifier;
>> +} __packed;
>> +
>>  /**
>>   * Create a new 'blob' data property, copying length bytes from data pointer,
>>   * and returning new blob ID.
>> --
>> 2.12.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
On Tue, May 16, 2017 at 02:19:12PM -0700, Ben Widawsky wrote:
> On 17-05-03 17:08:27, Daniel Vetter wrote:
> > On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
> > > +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[] */
> > > +} __packed;
> > 
> > The struct should be in the uapi header. Otherwise it won't show up in
> > libdrm headers when following the proper process.
> > -Daniel
> > 
> 
> I don't agree that blobs are ever really part of the API, but it doesn't hurt to
> move it... in other words, done.

Userspace writes them, the kernel reads them (or maybe even the other way
round). How exactly is a specific blob and its layout not part of uapi?
Can you explain your reasoning here pls?
-Daniel
On Wed, May 17, 2017 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 16, 2017 at 02:19:12PM -0700, Ben Widawsky wrote:
>> On 17-05-03 17:08:27, Daniel Vetter wrote:
>> > On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>> > > +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[] */
>> > > +} __packed;
>> >
>> > The struct should be in the uapi header. Otherwise it won't show up in
>> > libdrm headers when following the proper process.
>> > -Daniel
>> >
>>
>> I don't agree that blobs are ever really part of the API, but it doesn't hurt to
>> move it... in other words, done.
>
> Userspace writes them, the kernel reads them (or maybe even the other way
> round). How exactly is a specific blob and its layout not part of uapi?
> Can you explain your reasoning here pls?

Ok, this is the other way round, kernel writes this, userspace reads
it. Question still stands.
-Daniel
On 17-05-17 13:31:44, Daniel Vetter wrote:
>On Tue, May 16, 2017 at 02:19:12PM -0700, Ben Widawsky wrote:
>> On 17-05-03 17:08:27, Daniel Vetter wrote:
>> > On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>> > > +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[] */
>> > > +} __packed;
>> >
>> > The struct should be in the uapi header. Otherwise it won't show up in
>> > libdrm headers when following the proper process.
>> > -Daniel
>> >
>>
>> I don't agree that blobs are ever really part of the API, but it doesn't hurt to
>> move it... in other words, done.
>
>Userspace writes them, the kernel reads them (or maybe even the other way
>round). How exactly is a specific blob and its layout not part of uapi?
>Can you explain your reasoning here pls?
>-Daniel

The API says there is a blob. If we wanted the fields in the blob to be explicit
we'd make an API that has the exact structure.
On Wed, May 17, 2017 at 8:00 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On 17-05-17 13:31:44, Daniel Vetter wrote:
>>
>> On Tue, May 16, 2017 at 02:19:12PM -0700, Ben Widawsky wrote:
>>>
>>> On 17-05-03 17:08:27, Daniel Vetter wrote:
>>> > On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>>> > > +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[] */
>>> > > +} __packed;
>>> >
>>> > The struct should be in the uapi header. Otherwise it won't show up in
>>> > libdrm headers when following the proper process.
>>> > -Daniel
>>> >
>>>
>>> I don't agree that blobs are ever really part of the API, but it doesn't
>>> hurt to
>>> move it... in other words, done.
>>
>>
>> Userspace writes them, the kernel reads them (or maybe even the other way
>> round). How exactly is a specific blob and its layout not part of uapi?
>> Can you explain your reasoning here pls?
>> -Daniel
>
>
> The API says there is a blob. If we wanted the fields in the blob to be
> explicit
> we'd make an API that has the exact structure.
>

well, maybe don't confuse uabi and what can be represented in a 'C' struct..

we've designed the blob layout w/ uabi in mind (ie. w/ offsets to the
different sections, etc).. doesn't necessarily mean it is something
representable as a 'C' struct.. but it's still a form of uabi..

BR,
-R
On Wed, May 17, 2017 at 8:38 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, May 17, 2017 at 8:00 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> On 17-05-17 13:31:44, Daniel Vetter wrote:
>>>
>>> On Tue, May 16, 2017 at 02:19:12PM -0700, Ben Widawsky wrote:
>>>>
>>>> On 17-05-03 17:08:27, Daniel Vetter wrote:
>>>> > On Tue, May 02, 2017 at 10:14:27PM -0700, Ben Widawsky wrote:
>>>> > > +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[] */
>>>> > > +} __packed;
>>>> >
>>>> > The struct should be in the uapi header. Otherwise it won't show up in
>>>> > libdrm headers when following the proper process.
>>>> > -Daniel
>>>> >
>>>>
>>>> I don't agree that blobs are ever really part of the API, but it doesn't
>>>> hurt to
>>>> move it... in other words, done.
>>>
>>>
>>> Userspace writes them, the kernel reads them (or maybe even the other way
>>> round). How exactly is a specific blob and its layout not part of uapi?
>>> Can you explain your reasoning here pls?
>>> -Daniel
>>
>>
>> The API says there is a blob. If we wanted the fields in the blob to be
>> explicit
>> we'd make an API that has the exact structure.
>>
>
> well, maybe don't confuse uabi and what can be represented in a 'C' struct..
>
> we've designed the blob layout w/ uabi in mind (ie. w/ offsets to the
> different sections, etc).. doesn't necessarily mean it is something
> representable as a 'C' struct.. but it's still a form of uabi..
>

and by "we've" I really mean Ben plus irc and/or list bikeshedding
from daniels and myself and various others..

BR,
-R