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

Wed Feb 21 19:32:43 UTC 2018

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.

> @@ -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.

> +      ▶
> +	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.

> +
> +┌───
> +    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.

> +	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.

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

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

> +	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?

> +	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?

> +	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.

> +┌───
> +    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?

> +	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?

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20180221/908419c6/attachment.sig>