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

[Liviu Dudau <Liviu.Dudau@arm.com>]

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,

- documentation needs to use the actual macro name: DRM_FORMAT_MOD_INVALID
- 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.
- 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.
- some extraneous empty lines could be trimmed
- 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.

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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx