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@xxxxxxxxxxxx> 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@xxxxxxxxx>
> >> Cc: Daniel Stone <daniels@xxxxxxxxxxxxx>
> >> Cc: Kristian H. Kristensen <hoegsberg@xxxxxxxxx>
> >> Cc: Liviu Dudau <liviu@xxxxxxxxxxx>
> >> Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> >> Reviewed-by: Daniel Stone <daniels@xxxxxxxxxxxxx>
> >
> >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@xxxxxxxxx>
> >
> >
> >> --- 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.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
