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

[Ben Widawsky <ben@bwidawsk.net>]

On 17-05-10 18:24:52, Liviu Dudau wrote:
>On Wed, May 10, 2017 at 09:34:40AM -0700, Ben Widawsky wrote:
>> On 17-05-03 18:30:07, Liviu Dudau wrote:
>> > On Wed, May 03, 2017 at 06:45:05PM +0200, Daniel Vetter wrote:
>> > > On Wed, May 03, 2017 at 03:52:23PM +0100, Liviu Dudau wrote:
>> > > > On Wed, May 03, 2017 at 03:14:56PM +0100, Daniel Stone wrote:
>> > > > > On 3 May 2017 at 15:07, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>> > > > > > On Wed, May 03, 2017 at 02:45:26PM +0100, Daniel Stone wrote:
>> > > > > >> 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.
>> > > > >
>> > > > > Or, with another interpretation, he thinks the various approaches of
>> > > > > doing it are all equally good. He took guidance from a couple of
>> > > > > userspace developers (Weston, ChromeOS), a Freedreno developer and
>> > > > > also I believe an AFBC developer, to end up where he is now, which he
>> > > > > still thinks is fine. In doing so, he's been through several
>> > > > > iterations, always modifying the driver to suit. I think that's a
>> > > > > pretty good way to do development of new uABI, if you ask me. (And
>> > > > > again, I have trouble reading your last sentence as anything other
>> > > > > than hostile.)
>> > > >
>> > > > I am getting the message. You think I am too strong here, so I'm going to back off.
>> > > > Also look forward to see the next version where he takes into account the technical
>> > > > comments that I have sent.
>> > >
>> > > Just to make it really clear: Google has an implementation for AFBC using
>> > > exactly this scheme for cros. The only reasons it's not floating around
>> > > here in upstream is that one of the critical pieces is missing (*hint*,
>> > > *hint* you really want an open mail driver ...).
>> >
>> > <tongue_in_cheek>
>> > Don't know about open _mail_ drivers but there are plenty of open mail apps that one can use
>> > </tongue_in_cheek>
>> >
>> > Joke aside, the Mali GPU *kernel* driver *is* open source. Just not in the mainline and
>> > not enough for what you want. But I'm not the right person to fix all that.
>> >
>> > And GPU is not the only IP capable of producing AFBC data, so there might be another driver
>> > in the making that will be open source.
>> >
>> > Best regards,
>> > Liviu
>> >
>> > > But besides that, it works
>> > > perfectly fine for arm render compression format too.
>> > > -Daniel
>> > > --
>> > > Daniel Vetter
>> > > Software Engineer, Intel Corporation
>> > > http://blog.ffwll.ch
>> >
>> > --
>>
>> So are we good with patch 1, sorry if I missed any real valid changes I need to
>> make, but can we please capture that here.
>
>Sure,
>

Thanks. Reordered your comments a bit...

>- documentation needs to use the actual macro name: DRM_FORMAT_MOD_INVALID

Okay. It originally wasn't this way because it forces the sentence to be two
lines, but I've changed it.

>- some extraneous empty lines could be trimmed

I think I got them all..

>- drm_universal_plane_init comment about first driver with > 64 formats could do
>  with some verbose explanation on why ("because we encode each format as a bit
>  in the format_modifiers field and we have only reserved 64 bits for that")


>- most (all?) drivers are passing NULL as the format_modifier pointer to
>  drm_universal_plane_init() which makes me wonder if it is not better to have a
>  different function for drivers that want to pass a non-NULL format_modifier.

I can wrap this, that might be the best way and then I wouldn't need to touch
the other drivers. Here is the relevant part of the diff:

I'm fine with that if other people are. It only is the way it is because
Kristian originally did this for GET_PLANE2.

+__printf(9, 10)
+int drm_universal_plane_init_with_modifiers(struct drm_device *dev,
+                                           struct drm_plane *plane,
+                                           uint32_t possible_crtcs,
+                                           const struct drm_plane_funcs *funcs,
+                                           const uint32_t *formats,
+                                           unsigned int format_count,
+                                           const uint64_t *format_modifiers,
+                                           enum drm_plane_type type,
+                                           const char *name, ...);
 __printf(8, 9)
-int drm_universal_plane_init(struct drm_device *dev,
-                            struct drm_plane *plane,
-                            uint32_t possible_crtcs,
-                            const struct drm_plane_funcs *funcs,
-                            const uint32_t *formats,
-                            unsigned int format_count,
-                            enum drm_plane_type type,
-                            const char *name, ...);
+static inline int
+drm_universal_plane_init(struct drm_device *dev,
+                        struct drm_plane *plane,
+                        uint32_t possible_crtcs,
+                        const struct drm_plane_funcs *funcs,
+                        const uint32_t *formats,
+                        unsigned int format_count,
+                        enum drm_plane_type type,
+                        const char *name, ...)
+{
+       va_list ap;
+       int ret;
+
+       va_start(ap, name);
+       ret =  drm_universal_plane_init_with_modifiers(dev, plane,
+                                                      possible_crtcs, funcs,
+                                                      formats, format_count,
+                                                      NULL, type, name, ap);
+       va_end(ap);
+       return ret;
+}
+


>- plane->modifier_count is never used
>- memcpy()-ing the format_modifiers in drm_universal_plane_init doesn't check for
>  NULL even if that is what most drivers pass on when they call the function.
>- format_mod_supported function is not used in 1/3 patch, you can introduce it
>  in the patch that actually uses it
>- drm_plane's formats field doesn't look used either.
>

Regarding the unused fields, they are used in later patches by the
implementation. I specifically split out this so that we can deal with all the
DRM hardware specific driver changes in one [supposed to be] trivial patch.
Generally I agree that introducing fields without using them is not very useful
to reviewers, but I consider this a special case.

I see two primary reasons for this:
1. It made rebase easier (remember, this series is > 6 months old), and as new
   DRM drivers were added, it made a nice simple patch to change.
2. Easy to spot kbuild failures/resolutions.
3. Review should be trivial for all the other hardware people. If they don't
   care about modifiers, they can pretty much ignore the patch entire.

>You can look in the first reply to your 1/3 patch if you want the details.
>
>Best regards,
>Liviu
>
>>
>> --
>> Ben Widawsky, Intel Open Source Technology Center
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>-- 
>====================
>| I would like to |
>| fix the world,  |
>| but they're not |
>| giving me the   |
> \ source code!  /
>  ---------------
>    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel