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

[Liviu Dudau <Liviu.Dudau@arm.com>]

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx