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

[Emil Velikov]

Hi Ben,

On 3 May 2017 at 06:14, Ben Widawsky <ben at bwidawsk.net> wrote:

> +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[] */
> +} __packed;
> +
Why do we need packing here? Both layout and member offsets are
identical with and w/o it.
If there's something subtle to it, please add a comment.


> +static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane,
> +                                const struct drm_plane_funcs *funcs,
> +                                const uint32_t *formats, unsigned int format_count,
> +                                const uint64_t *format_modifiers)
> +{

> +       blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8);
> +       blob_size += ALIGN(formats_size, 8);
The 8 is "sizeof(void *) isn't it? Shouldn't we use that instead since
it expands to 4 for 32bit systems?

> +       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 = format_count;

> +       blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
Shouldn't we reuse the ALIGN(...) from above?


> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 8c67fc03d53d..dcdd04c55792 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -665,6 +665,32 @@ struct drm_mode_atomic {
>         __u64 user_data;
>  };
>
> +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
> +       *
> +       */
> +       uint64_t formats;
> +       uint32_t offset;
> +       uint32_t pad;
> +
> +       /* This modifier can be used with the format for this plane. */
> +       uint64_t modifier;
> +} __packed;
> +
As above - why __packed?

Regards,
Emil