| Message ID | 20170516213126.8269-2-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 16, 2017 at 02:31:25PM -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) Thanks for updating the patch! > > Cc: Rob Clark <robdclark@gmail.com> > Cc: Daniel Stone <daniels@collabora.com> > Cc: Kristian H. Kristensen <hoegsberg@gmail.com> > Cc: Liviu Dudau <liviu@dudau.co.uk> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > Reviewed-by: Daniel Stone <daniels@collabora.com> Looks good to me! Reviewed-by: Liviu Dudau <liviu@dudau.co.uk> Best regards, Liviu > --- > drivers/gpu/drm/drm_mode_config.c | 7 +++ > drivers/gpu/drm/drm_plane.c | 89 +++++++++++++++++++++++++++++++++++++++ > include/drm/drm_mode_config.h | 6 +++ > include/uapi/drm/drm_mode.h | 50 ++++++++++++++++++++++ > 4 files changed, 152 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 20203871e06d..f5b032b5f761 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -62,6 +62,92 @@ 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; > + const uint64_t *temp_modifiers = plane->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 (plane->modifiers) > + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID) > + format_modifier_count++; > + > + 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) * format_modifier_count; > + > + blob_size = sizeof(struct drm_format_modifier_blob); > + 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 = 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), 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 < format_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 ret; > +} > + > /** > * drm_universal_plane_init - Initialize a new universal plane object > * @dev: DRM device > @@ -180,6 +266,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 8c67fc03d53d..7ad567903fdc 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -665,6 +665,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.0 >
Hi Ben, On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> 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) > > Cc: Rob Clark <robdclark@gmail.com> > Cc: Daniel Stone <daniels@collabora.com> > Cc: Kristian H. Kristensen <hoegsberg@gmail.com> > Cc: Liviu Dudau <liviu@dudau.co.uk> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > Reviewed-by: Daniel Stone <daniels@collabora.com> I think this is almost perfect, barring the UAPI nitpick. The rest is somewhat of a bikeshedding. With the UAPI resolved, regardless of the rest Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane) > +{ > + const struct drm_mode_config *config = &dev->mode_config; > + const uint64_t *temp_modifiers = plane->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; There's no need to initialize blob and blob_size here. > + struct drm_format_modifier_blob *blob_data; > + int i, j, ret = 0; Make i and j unsigned to match format_modifier_count and plane->format_count respectively. Then expand ret in the only place where it's used? > + > + if (plane->modifiers) > + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID) > + format_modifier_count++; > + > + 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) * format_modifier_count; > + > + blob_size = sizeof(struct drm_format_modifier_blob); > + blob_size += ALIGN(formats_size, 8); Worth having the "Modifiers offset is a pointer..." comment moved/copied here? > + blob_size += modifiers_size; > + > + blob = drm_property_create_blob(dev, blob_size, NULL); > + if (IS_ERR(blob)) > + return -1; > + Maybe propagate the exact error... Hmm we don't seem to check if create_in_format_blob fails either so perhaps it's not worth it. > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -665,6 +665,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; Please drop the leading __ from the type names in UAPI headers. Regards, Emil
On 17-05-17 01:06:16, Emil Velikov wrote: >Hi Ben, > >On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> 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) >> >> Cc: Rob Clark <robdclark@gmail.com> >> Cc: Daniel Stone <daniels@collabora.com> >> Cc: Kristian H. Kristensen <hoegsberg@gmail.com> >> Cc: Liviu Dudau <liviu@dudau.co.uk> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> >> Reviewed-by: Daniel Stone <daniels@collabora.com> > >I think this is almost perfect, barring the UAPI nitpick. >The rest is somewhat of a bikeshedding. > >With the UAPI resolved, regardless of the rest >Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > > >> --- a/drivers/gpu/drm/drm_plane.c >> +++ b/drivers/gpu/drm/drm_plane.c > >> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane) >> +{ >> + const struct drm_mode_config *config = &dev->mode_config; >> + const uint64_t *temp_modifiers = plane->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; >There's no need to initialize blob and blob_size here. > >> + struct drm_format_modifier_blob *blob_data; >> + int i, j, ret = 0; >Make i and j unsigned to match format_modifier_count and >plane->format_count respectively. >Then expand ret in the only place where it's used? > Oh. ret has lost it's utility over the iterations of this patch. Make i and j unsigned and dropped ret. >> + >> + if (plane->modifiers) >> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID) >> + format_modifier_count++; >> + >> + 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) * format_modifier_count; >> + >> + blob_size = sizeof(struct drm_format_modifier_blob); >> + blob_size += ALIGN(formats_size, 8); >Worth having the "Modifiers offset is a pointer..." comment moved/copied here? > Yes it is. >> + blob_size += modifiers_size; >> + >> + blob = drm_property_create_blob(dev, blob_size, NULL); >> + if (IS_ERR(blob)) >> + return -1; >> + >Maybe propagate the exact error... Hmm we don't seem to check if >create_in_format_blob fails either so perhaps it's not worth it. > > In this case it's almost definitely ENOMEM. All values should be verified - I think the other errors are there for when userspace is utilizing blob creation. So I'll just leave it. >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> @@ -665,6 +665,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: G>> + * >> + * 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; >Please drop the leading __ from the type names in UAPI headers. > Many other structures have the __, can you explain why please (this has probably been beaten to death already; but I don't know)? >Regards, >Emil
On Wed, May 17, 2017 at 05:46:18PM -0700, Ben Widawsky wrote: > On 17-05-17 01:06:16, Emil Velikov wrote: > >Hi Ben, > > > >On 16 May 2017 at 22:31, Ben Widawsky <ben@bwidawsk.net> 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) > >> > >> Cc: Rob Clark <robdclark@gmail.com> > >> Cc: Daniel Stone <daniels@collabora.com> > >> Cc: Kristian H. Kristensen <hoegsberg@gmail.com> > >> Cc: Liviu Dudau <liviu@dudau.co.uk> > >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > >> Reviewed-by: Daniel Stone <daniels@collabora.com> > > > >I think this is almost perfect, barring the UAPI nitpick. > >The rest is somewhat of a bikeshedding. > > > >With the UAPI resolved, regardless of the rest > >Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > > > > > >> --- a/drivers/gpu/drm/drm_plane.c > >> +++ b/drivers/gpu/drm/drm_plane.c > > > >> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane) > >> +{ > >> + const struct drm_mode_config *config = &dev->mode_config; > >> + const uint64_t *temp_modifiers = plane->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; > >There's no need to initialize blob and blob_size here. > > > >> + struct drm_format_modifier_blob *blob_data; > >> + int i, j, ret = 0; > >Make i and j unsigned to match format_modifier_count and > >plane->format_count respectively. > >Then expand ret in the only place where it's used? > > > > Oh. ret has lost it's utility over the iterations of this patch. Make i and j > unsigned and dropped ret. Unsigned loop iterators will likely bite someone some day, especially if they're called something like 'i', 'j', etc. IMO it's best to keep them signed. > > >> + > >> + if (plane->modifiers) > >> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID) > >> + format_modifier_count++; > >> + > >> + 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) * format_modifier_count; > >> + > >> + blob_size = sizeof(struct drm_format_modifier_blob); > >> + blob_size += ALIGN(formats_size, 8); > >Worth having the "Modifiers offset is a pointer..." comment moved/copied here? > > > > Yes it is. > > >> + blob_size += modifiers_size; > >> + > >> + blob = drm_property_create_blob(dev, blob_size, NULL); > >> + if (IS_ERR(blob)) > >> + return -1; > >> + > >Maybe propagate the exact error... Hmm we don't seem to check if > >create_in_format_blob fails either so perhaps it's not worth it. > > > > > > In this case it's almost definitely ENOMEM. All values should be verified - I > think the other errors are there for when userspace is utilizing blob creation. > > So I'll just leave it. > > >> --- a/include/uapi/drm/drm_mode.h > >> +++ b/include/uapi/drm/drm_mode.h > >> @@ -665,6 +665,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: > G>> + * > >> + * 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; > >Please drop the leading __ from the type names in UAPI headers. > > > > Many other structures have the __, can you explain why please (this has probably > been beaten to death already; but I don't know)? __ is the rigth choice for uapi headers, unless the rules have changed recently.
On 18 May 2017 at 01:46, Ben Widawsky <ben@bwidawsk.net> wrote: >>> + blob_size += modifiers_size; >>> + >>> + blob = drm_property_create_blob(dev, blob_size, NULL); >>> + if (IS_ERR(blob)) >>> + return -1; >>> + >> >> Maybe propagate the exact error... Hmm we don't seem to check if >> create_in_format_blob fails either so perhaps it's not worth it. >> >> > > In this case it's almost definitely ENOMEM. All values should be verified - > I > think the other errors are there for when userspace is utilizing blob > creation. > > So I'll just leave it. > Ack, sure thing. >>> --- a/include/uapi/drm/drm_mode.h >>> +++ b/include/uapi/drm/drm_mode.h >>> @@ -665,6 +665,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: > > G>> + * >>> >>> + * 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; >> >> Please drop the leading __ from the type names in UAPI headers. >> > > Many other structures have the __, can you explain why please (this has > probably > been beaten to death already; but I don't know)? > Got confused there for a moment, apologies. Using the __ types is correct. It is drm_format_modifier_blob that should be updated to follow suit. -Emil
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 20203871e06d..f5b032b5f761 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -62,6 +62,92 @@ 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; + const uint64_t *temp_modifiers = plane->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 (plane->modifiers) + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID) + format_modifier_count++; + + 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) * format_modifier_count; + + blob_size = sizeof(struct drm_format_modifier_blob); + 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 = 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), 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 < format_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 ret; +} + /** * drm_universal_plane_init - Initialize a new universal plane object * @dev: DRM device @@ -180,6 +266,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 8c67fc03d53d..7ad567903fdc 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -665,6 +665,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.