Hi,

On Wed, 2018-02-21 at 11:32 -0800, Keith Packard wrote:
> Louis-Francis Ratté-Boulianne <l...@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
> >                       kei...@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
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to