[Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init
Ben Widawsky
ben at bwidawsk.net
Wed May 10 19:33:15 UTC 2017
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 at 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 at 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! /
> ---------------
> ¯\_(ツ)_/¯
More information about the dri-devel
mailing list