[Intel-gfx] [PATCH v2 2/3] drm: Create a format/modifier blob

[Ben Widawsky]

On 17-05-17 01:06:16, Emil Velikov wrote:
>Hi Ben,
>
>On 16 May 2017 at 22:31, Ben Widawsky <ben at 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 at gmail.com>
>> Cc: Daniel Stone <daniels at collabora.com>
>> Cc: Kristian H. Kristensen <hoegsberg at gmail.com>
>> Cc: Liviu Dudau <liviu at dudau.co.uk>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> Reviewed-by: Daniel Stone <daniels at 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 at 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

-- 
Ben Widawsky, Intel Open Source Technology Center