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