[Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init
Liviu Dudau
liviu at dudau.co.uk
Wed May 10 20:21:11 UTC 2017
On Wed, May 10, 2017 at 12:33:15PM -0700, Ben Widawsky wrote:
> 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;
> +}
> +
I guess the patchset's delta is going to go down significantly with this if
i915 is the only driver that explicitly calls the new function. I'll wait
until next revision to see, but I have a bias towards smaller patches.
>
>
> >- 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.
What about this one?
> >- 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.
OK, but plane->modifier_count is not used by any of your patches. OTOH,
plane->modifiers is allocated, memcpy-ed once into from the drm_universal_plane_init()
parameter and never used again until kfreed. There seems to be at least one place
where you could use plane->modifiers without having to pass the format_modifiers
again as parameter (in create_in_format_blob() in 2/3).
Best regards,
Liviu
>
> 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! /
> > ---------------
> > ¯\_(ツ)_/¯
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
_
_|_|_
('_')
(⊃ )⊃
|_|_|
More information about the Intel-gfx
mailing list