| Message ID | 20170623164544.7362-3-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <robdclark@gmail.com> > Cc: Kristian H. Kristensen <hoegsberg@gmail.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > Reviewed-by: Daniel Stone <daniels@collabora.com> (v2) > Reviewed-by: Liviu Dudau <liviu@dudau.co.uk> (v2) > Reviewed-by: Emil Velikov <emil.l.velikov@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-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 17-06-29 22:49:44, Ville Syrjälä wrote: [snip] > >... 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?). > I guess I don't quite understand what you're asking for. The first thing is determining a size, the second is finding an offset into the blob. I don't mind changing this, but tell me what you want. BUILD_BUG_ON sounds good to me regardless. [snip]
On Fri, Jul 14, 2017 at 11:41:49AM -0700, Ben Widawsky wrote: > On 17-06-29 22:49:44, Ville Syrjälä wrote: > > [snip] > > > > >... 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?). > > > > I guess I don't quite understand what you're asking for. The first thing is > determining a size, the second is finding an offset into the blob. If I remember my thinking correctly, then what I was trying to say was ALIGN(multiple_of_4, 8) + ALIGN(multiple_of_4, 8) != ALIGN(multiple_of_4 + multiple_of_4, 8) but the code was assuming that it's true, or that at least one of those things is a multiple of 8 already. > I don't mind changing this, but tell me what you want. > > BUILD_BUG_ON sounds good to me regardless. > > [snip] > > -- > Ben Widawsky, Intel Open Source Technology Center
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; + 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); + 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); + + 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; + /* 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.