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@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
