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 _______________________________________________ firstname.lastname@example.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel