On Fri, Jun 23, 2017 at 09:45:42AM -0700, Ben Widawsky wrote:
> Updated blob layout (Rob, Daniel, Kristian, xerpi)
>
> v2:
> * Removed __packed, and alignment (.+)
> * Fix indent in drm_format_modifier fields (Liviu)
> * Remove duplicated modifier > 64 check (Liviu)
> * Change comment about modifier (Liviu)
> * Remove arguments to blob creation, use plane instead (Liviu)
> * Fix data types (Ben)
> * Make the blob part of uapi (Daniel)
>
> v3:
> Remove unused ret field.
> Change i, and j to unsigned int (Emil)
>
> v4:
> Use plane->modifier_count instead of recounting (Daniel)
>
> Cc: Rob Clark <robdcl...@gmail.com>
> Cc: Kristian H. Kristensen <hoegsb...@gmail.com>
> Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
> Reviewed-by: Daniel Stone <dani...@collabora.com> (v2)
> Reviewed-by: Liviu Dudau <li...@dudau.co.uk> (v2)
> Reviewed-by: Emil Velikov <emil.l.veli...@gmail.com> (v3)
> ---
> drivers/gpu/drm/drm_mode_config.c | 7 ++++
> drivers/gpu/drm/drm_plane.c | 83
> +++++++++++++++++++++++++++++++++++++++
> include/drm/drm_mode_config.h | 6 +++
> include/uapi/drm/drm_mode.h | 50 +++++++++++++++++++++++
> 4 files changed, 146 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> index d9862259a2a7..6bfbc3839df5 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -337,6 +337,13 @@ static int drm_mode_create_standard_properties(struct
> drm_device *dev)
> return -ENOMEM;
> dev->mode_config.gamma_lut_size_property = prop;
>
> + prop = drm_property_create(dev,
> + DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB,
> + "IN_FORMATS", 0);
> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.modifiers = prop;
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d3fc561d7b48..8a2d5765837a 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -62,6 +62,86 @@ static unsigned int drm_num_planes(struct drm_device *dev)
> return num;
> }
>
> +static inline u32 *
> +formats_ptr(struct drm_format_modifier_blob *blob)
> +{
> + return (u32 *)(((char *)blob) + blob->formats_offset);
> +}
> +
> +static inline struct drm_format_modifier *
> +modifiers_ptr(struct drm_format_modifier_blob *blob)
> +{
> + return (struct drm_format_modifier *)(((char *)blob) +
> blob->modifiers_offset);
> +}
> +
> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane
> *plane)
> +{
> + const struct drm_mode_config *config = &dev->mode_config;
> + struct drm_property_blob *blob;
> + struct drm_format_modifier *mod;
> + size_t blob_size, formats_size, modifiers_size;
> + struct drm_format_modifier_blob *blob_data;
> + unsigned int i, j;
> +
> + formats_size = sizeof(*plane->format_types) * plane->format_count;
sizeof(__u32) perhaps here since this is about the uabi, not about how
we store the formats internally.
> + if (WARN_ON(!formats_size)) {
> + /* 0 formats are never expected */
> + return 0;
> + }
> +
> + modifiers_size =
> + sizeof(struct drm_format_modifier) * plane->modifier_count;
> +
> + blob_size = sizeof(struct drm_format_modifier_blob);
> + /* Modifiers offset is a pointer to a struct with a 64 bit field so it
> + * should be naturally aligned to 8B.
> + */
> + blob_size += ALIGN(formats_size, 8);
Here it's ALIGN(format_size) ...
> + blob_size += modifiers_size;
> +
> + blob = drm_property_create_blob(dev, blob_size, NULL);
> + if (IS_ERR(blob))
> + return -1;
> +
> + blob_data = (struct drm_format_modifier_blob *)blob->data;
> + blob_data->version = FORMAT_BLOB_CURRENT;
> + blob_data->count_formats = plane->format_count;
> + blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
> + blob_data->count_modifiers = plane->modifier_count;
> +
> + blob_data->modifiers_offset =
> + ALIGN(blob_data->formats_offset + formats_size, 8);
... but here it's ALIGN(formats_offset+formats_size). I think we should
be aligning the same thing in both cases, or we add a BUILD_BUG_ON to
make sure the header size always stays a multiple of 8 bytes.
That's mainly because the design of the structure seems geared towards
expanding the header in the future (as in why else would you have the
offsets?).
> +
> + memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
> +
> + /* If we can't determine support, just bail */
> + if (!plane->funcs->format_mod_supported)
> + goto done;
> +
> + mod = modifiers_ptr(blob_data);
> + for (i = 0; i < plane->modifier_count; i++) {
> + for (j = 0; j < plane->format_count; j++) {
> + if (plane->funcs->format_mod_supported(plane,
> +
> plane->format_types[j],
> +
> plane->modifiers[i])) {
> +
> + mod->formats |= 1 << j;
> + }
> + }
> +
> + mod->modifier = plane->modifiers[i];
> + mod->offset = 0;
> + mod->pad = 0;
> + mod++;
> + }
> +
> +done:
> + drm_object_attach_property(&plane->base, config->modifiers,
> + blob->base.id);
> +
> + return 0;
> +}
> +
> /**
> * drm_universal_plane_init - Initialize a new universal plane object
> * @dev: DRM device
> @@ -181,6 +261,9 @@ int drm_universal_plane_init(struct drm_device *dev,
> struct drm_plane *plane,
> drm_object_attach_property(&plane->base, config->prop_src_h, 0);
> }
>
> + if (config->allow_fb_modifiers)
> + create_in_format_blob(dev, plane);
> +
> return 0;
> }
> EXPORT_SYMBOL(drm_universal_plane_init);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 42981711189b..03776e659811 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -757,6 +757,12 @@ struct drm_mode_config {
> */
> bool allow_fb_modifiers;
>
> + /**
> + * @modifiers: Plane property to list support modifier/format
> + * combination.
> + */
> + struct drm_property *modifiers;
I believe that if we follow the most common naming pattern this should
be "modifiers_property". Also might be good to group it next to the
other properties.
> +
> /* cursor size */
> uint32_t cursor_width, cursor_height;
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 403339f98a92..a2bb7161f020 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -712,6 +712,56 @@ struct drm_mode_atomic {
> __u64 user_data;
> };
>
> +struct drm_format_modifier_blob {
> +#define FORMAT_BLOB_CURRENT 1
> + /* Version of this blob format */
> + u32 version;
> +
> + /* Flags */
> + u32 flags;
> +
> + /* Number of fourcc formats supported */
> + u32 count_formats;
> +
> + /* Where in this blob the formats exist (in bytes) */
> + u32 formats_offset;
> +
> + /* Number of drm_format_modifiers */
> + u32 count_modifiers;
> +
> + /* Where in this blob the modifiers exist (in bytes) */
> + u32 modifiers_offset;
> +
> + /* u32 formats[] */
> + /* struct drm_format_modifier modifiers[] */
> +};
> +
> +struct drm_format_modifier {
> + /* Bitmask of formats in get_plane format list this info applies to. The
> + * offset allows a sliding window of which 64 formats (bits).
> + *
> + * Some examples:
> + * In today's world with < 65 formats, and formats 0, and 2 are
> + * supported
> + * 0x0000000000000005
> + * ^-offset = 0, formats = 5
> + *
> + * If the number formats grew to 128, and formats 98-102 are
> + * supported with the modifier:
> + *
> + * 0x0000003c00000000 0000000000000000
> + * ^
> + * |__offset = 64, formats = 0x3c00000000
> + *
> + */
> + __u64 formats;
> + __u32 offset;
> + __u32 pad;
> +
> + /* The modifier that applies to the >get_plane format list bitmask. */
> + __u64 modifier;
> +};
> +
> /**
> * Create a new 'blob' data property, copying length bytes from data pointer,
> * and returning new blob ID.
> --
> 2.13.1
>
> _______________________________________________
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
