[RFC dri3proto v6 01/14] Add modifier/multi-plane requests, bump to v1.1

Thu Feb 22 08:52:08 UTC 2018

Hi,

On Wed, 2018-02-21 at 11:32 -0800, Keith Packard wrote:
> Louis-Francis Ratté-Boulianne <lfrb at collabora.com> writes:
> 
> I'll review just the specification today; once we've got that wired,
> I'll go ahead and verify that the encoding matches this spec.
> 
> > diff --git a/dri3proto.txt b/dri3proto.txt
> > index dac11d3..636c789 100644
> > --- a/dri3proto.txt
> > +++ b/dri3proto.txt
> > @@ -1,11 +1,12 @@
> >  			  The DRI3 Extension
> > -			     Version 1.0
> > -			       2013-6-4
> > +			     Version 1.1
> > +			      2017-06-27
> >        
> >  			    Keith Packard
> >  			  keithp at keithp.com
> >  			  Intel Corporation
> 
> You should add yourself to the authors list here.

Actually, Daniel is the one who should add his name :)

> > @@ -199,6 +202,125 @@ The name of this extension is "DRI3"
> >  	associated with a direct rendering device that 'fence' can
> >  	work with, otherwise a Match error results.
> >  
> > +┌───
> > +    DRI3GetSupportedModifiers
> > +	window: WINDOW
> > +	depth: CARD8
> > +	bpp: CARD8
> 
> Hrm. You've uncovered a weird "feature" of the existing DRI3
> protocol. Why does it include both depth and bpp? In the server, each
> depth has a defined bpp for image format purposes, and (currently at
> least), image bpp always matches the pixmap bpp. I'm afraid the
> original
> DRI3 proposal has no clues for us, and I'm afraid the author doesn't
> remember...
> 
> The only thing I can think is that including both allows us to catch
> circumstances where the client guesses wrong about the bpp and
> creates
> an image in the wrong format. Checking a few drivers, I find that
> modesetting uses it to detect this kind of error, but (at least)
> nouveau
> simply ignores it.
> 
> Should you use a visual here instead of depth? While visuals are
> actually allowed to be shared across depths in the core protocol
> specification, we currently rely on them being unique, and in fact
> the
> server implementation has always enforced that.
> 
> I think using a visual would also let you support DeepColor visuals
> and
> gain access to those extended pixel formats. I'm not sure what bpp
> would
> need to be in those cases. Alternatively, we could explicitly allow
> depth to be more than 32 here?
> 
> Using a visual would also allow other visual-dependent modifiers to
> be
> supported; one can imagine that TrueColor and DirectColor having a
> different set of modifiers supported. Of course, anything other than
> TrueColor isn't well supported these days anyways, so that's probably
> less important.

Wouldn't that be weird to have a totally different way of specifying
the format from the old requests. I honestly don't know enough about
DeepColor/HDR to really have an opinion about this though.

> > +      ▶
> > +	num_drawable_modifiers: CARD32
> > +	num_screen_modifiers: CARD32
> > +	drawable_modifiers: ListOfCARD64
> > +	screen_modifiers: ListOfCARD64
> > +└───
> > +	Errors: Window, Match
> > +
> > +	For the Screen associated with 'window', return two lists
> > of
> > +	supported DRM FourCC modifiers, as defined in
> > drm_fourcc.h,
> > +	supported for DRI3 pixmap/buffer interchange. The first
> > list
> > +	'drawable_modifiers' contains modifiers that are specific
> > to
> > +	this window and should be used over the more generic
> > +	'screen_modifiers' set.
> 
> This is somewhat confusing. We use 'window' as a proxy for 'screen'
> in
> many requests, but in this request it doesn't seem to be just a proxy
> as
> you also have a set of modifiers that are specific to it.
> 
> I think what makes sense is to use 'window' only to name the screen,
> and
> then to have the specific modifiers depend on the depth requested,
> not
> the drawable specified. That way you can reliably request information
> about how to create pixmaps without needing to create a temporary
> window
> of the right format before using DRI3PixmapFromBuffers.

Like was mentionned by Daniel on #xorg-devel, we really need to have
the 'drawable' here and can't easily retrieve the drawable-specific
modifiers just from the 'depth'. (We check if the window is a possible
candidate for scanout to avoid useless client buffer reallocation).

We don't explicitely state that in the spec though to be as general as
possible. I guess it wouldn't hurt to add an example though if you
think it would make things clearer.

> > +┌───
> > +    DRI3PixmapFromBuffers
> > +	pixmap: PIXMAP
> > +	drawable: DRAWABLE
> > +	num_buffers: CARD8
> > +	width, height: CARD16
> > +	stride0, offset0: CARD32
> > +	stride1, offset1: CARD32
> > +	stride2, offset2: CARD32
> > +	stride3, offset3: CARD32
> 
> Of course, it's weird to have a fixed list of values here. I suspect
> we
> will avoid numerous buffer overflow bugs in applications by doing it
> this way though. Given that KMS only supports 4, I guess this will be
> fine.

Yeah, using lists for requests was really cumbersome.

> 
> > +	depth, bpp: CARD8
> 
> See above for questions about these fields...
> 
> > +	modifier: CARD64
> > +	buffers: ListOfFD
> > +└───
> > +	Errors: Alloc, Drawable, IDChoice, Value, Match
> > +
> > +	Creates a pixmap for the direct rendering object
> > associated
> > +	with 'buffers'. Changes to pixmap will be visible in that
> > +	direct rendered object and changes to the direct rendered
> > +	object will be visible in the pixmap.

That paragraph was directly copy/pasted from PixmapFromBuffer. So if
things are confusing, we might want to update the documentation there
as well.

> Do we want to define synchronization mechanisms for this? We might
> explicitly state the ordering between X rendering and Damage events,
> for
> instance?

Do you mean adding a new synchronization mechanism (between X rendering
and client rendering) that would only apply to this request (not
PixapFromBuffer)? Was it considered problematic with DRI3 v1.0?

> I assume that the pixmap is created for the screen associated with
> 'drawable'?

Indeed

> > +	DRM_FORMAT_MOD_INVALID may be passed for 'modifier', in
> > which
> > +	case the driver may make its own inference as to the exact
> > +	layout of the buffer(s).
> 
> Presumably using information acquired externally?

Or internally; some drivers add metadata to the buffer to know about
tiling, etc.

> > +	Precisely how any additional information about the buffer
> > is
> > +	shared is outside the scope of this extension.
> 
> I think this applies to the MOD_INVALID mechanism. Does it also apply
> to
> anything else?

This was copy/pasted from PixmapFromBuffer as well. I still think it's
valid though and would mean, in that context, any additional
information except the modifier (if specified). It's a blanket
statement to avoid adding too much metadata in this API.

> > +	If the buffer(s) cannot be used with the screen associated
> > with
> > +	'pixmap', a Match error is returned.
> 
> Screen associated with 'drawable' -- 'pixmap' is a new resource ID,
> you're creating it here.

Absolutely right.

> > +┌───
> > +    DRI3BuffersFromPixmap
> > +	pixmap: PIXMAP
> > +      ▶
> > +	nfd: CARD8
> > +	width, height: CARD16
> > +	depth, bpp: CARD8
> > +	modifier: CARD64
> > +	strides: ListOfCARD32
> > +	offsets: ListOfCARD32
> > +	buffers: ListOfFD
> > +└───
> > +	Errors: Pixmap, Match
> > +
> > +	Returns direct rendering objects associated with 'pixmap'.
> > +	Changes to 'pixmap' will be visible in the direct rendered
> > +	objects and changes to the direct rendered objects will be
> > +	visible in 'pixmap' after flushing and synchronization.
> 
> What kind of flushing and synchronization? Maybe synchronization
> stuff
> should be added to the overall description of the extension so that
> it
> applies equally to all buffer/pixmap associations?
> 
> > +	See PixmapFromBuffers for more details on DRM modifiers
> > usage.
> 
> In what case might this return DRM_FORMAT_MOD_INVALID?

If the buffers don't have any modifier attached (e.g. if the driver
doesn't support them).

> 
> > +	Precisely how any additional information about the buffer
> > is
> > +	shared is outside the scope of this extension.
> 
> What additional information about the buffer might be relevant?

Memory alignment, compression, tiling (when modifier is not
used/supported).

--
Louis-Francis