[Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Daniel Stone]

Hi Liviu,

On 3 May 2017 at 11:34, Liviu Dudau <Liviu.Dudau at arm.com> wrote:
> On Tue, May 02, 2017 at 10:14:26PM -0700, Ben Widawsky wrote:
>> v2: A minor addition from Daniel
>
> You are *really* pushing your luck by not Cc-ing *any* of the maintainers of
> the drivers you touch. You do want reviews, don't you?

Ouch. I'm very sure Ben does want reviews, but he mainly works in Mesa
where you can rely on the list rather than having to CC everyone
individually. It was a mistake, so please be more gentle with him;
your whole mail comes across as quite hostile to me.

> Finally, the questions I should've started with: why the change? What are you trying to
> achieve? It is not very clear from the commit message at all.

It does deserve a much better commit message than what it has, but as
he is on holiday for the rest of the week, I can answer. Currently, we
advertise which formats each plane is capable of displaying. In order
for userspace to be able to allocate tiled/compressed buffers for
scanout, we want userspace to be able to discover which modifiers each
plane supports as well.

>> @@ -105,6 +108,28 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>>               return -ENOMEM;
>>       }
>>
>> +     /* First driver to need more than 64 formats needs to fix this */
>> +     if (WARN_ON(format_count > 64))
>> +             return -EINVAL;
>
> What is the link between format_count and format modifiers? Why are you introducing
> this artificial restriction on the format_count? Also, there doesn't seem to be any
> check if format_modifier_count == format_count. What happens when they don't match?

Inside the blob, each modifier carries a bitmask of formats (specified
by index) that the modifier applies to, i.e. with:
  .formats = { ARGB8888, XRGB8888, RGB565 },

a modifier which applied only to the 32-bit formats would specify a
bitmask of 0x3. The uABI supports this just fine, but the internal
plumbing does not yet, because no-one supports more than 64 formats.

>> +
>> +     if (format_modifiers) {
>> +             const uint64_t *temp_modifiers = format_modifiers;
>> +             while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
>> +                     format_modifier_count++;
>> +     }
>> +
>> +     plane->modifier_count = format_modifier_count;
>
> Why do you need to remember this? It is not used anywhere else!

It is used to populate the blob in a later patch!

Cheers,
Daniel