Re: [PATCH 1/3] drm: Plumb modifiers through plane init

[Ben Widawsky <ben@bwidawsk.net>]

On 17-05-03 14:45:26, Daniel Stone wrote:
>Hi Liviu,
>
>On 3 May 2017 at 11:34, Liviu Dudau <Liviu.Dudau@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.
>

It was not a mistake. The whole point of a mailing list is so that people can
participate without needing to Cc every individual. dri-devel has been the
location, and while many hardware vendors have set up their own list, dri-devel
is still the mailing list for patches like this. There are a ridiculous number
of DRM driver maintainers. There isn't even an easy way to script extracting all
the relevant people from MAINTAINERS (happy to be corrected on that) I don't
believe anybody Cc's them all, ever - but normally there isn't such a globally
invasive patch.

I'm sorry Liviu you obviously felt slighted. I did Cc the Intel mailing list
because my implementation for this was on Intel.

>> 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

I'm back now. Thanks Daniel for summing it up for me.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx