[Intel-gfx] [PATCH 2/3] drm: Create a format/modifier blob
Ben Widawsky
ben at bwidawsk.net
Thu May 11 17:58:36 UTC 2017
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 at gmail.com>
>> Cc: Daniel Stone <daniels at collabora.com>
>> Cc: Kristian H. Kristensen <hoegsberg at gmail.com>
>> Signed-off-by: Ben Widawsky <ben at 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 at 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! /
> ---------------
> ¯\_(ツ)_/¯
--
Ben Widawsky, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list