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

[Liviu Dudau]

On Wed, May 03, 2017 at 02:45:26PM +0100, Daniel Stone wrote:
> 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.

Sorry, I'm not trying to be hostile. Try to read the bolded words with a long
(southern USA) intonation as if to draw attention to them. You will see that
it is just pointing to two facts: he does not warn anyone about the changes and
he is not making the patchset that obviously critical by having a commit message
that implies everyone should pay attention to it. So he is really hoping to be
lucky to get reviews (or doesn't actively seek them).

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

I get the overall goal. We need/want something similar for Mali DP and AFBC buffers.
What I don't understand is the current aproach (but I've found from Brian that somehow
I've missed the long discussion(s) around the subject). I was hoping to learn
from the commit message why he thinks the introduction of this code is the right
way of doing it. And the IRC logs seem to imply that he is mostly doing something
that others have agreed upon and he doesn't really care about the approach as long
as it ticks the "supported by intel driver" box.

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

Yeah, still doesn't answer the question. Maybe if the comment said something like:
"When building format modifiers bitmap we only have space to map up to 64 formats.
Check that the driver is not trying to exceed that." then it would've been more
obvious why the check is needed and also why one doesn't care about format_modifier_count.

To explain my mindset: the way the patch series was formatted and the way I've read it
initially I did not realised that the idea is to build a bitmask. Nothing in this patch
alludes to that so I assumed there was a 1:1 relationship between format modifiers and
format.

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

I cannot find that patch. Nothing in the 3 patches uses that. Have a look!

Best regards,
Liviu

> 
> Cheers,
> Daniel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯