[PATCH 8/8] Add DRI3+Present loader

[Keith Packard]

Eric Anholt <eric at anholt.net> writes:

I've pushed a patch responding to these comments to my dri3 branch and
will send that out shortly. I will merge those changes with the original
DRI3+Present loader patch so that there is only one commit when the
review process is complete.

> I think I'm going to be griping about code duplication...

Yeah, I'm really not sure what to do about that. The alternative would
be to refactor the DRI2 backend and then land the DRI3 backend on top of
some shared code, but frankly I'm not that excited about breaking DRI2.

Do you want me to actually go ahead and try to do that?

> Patches need to land in XCB and get released before this can land.  I
> don't even see patches on the xcb list yet.

I sent out the patches to the xorg-devel list today after reworking and
cleaning them up. I'd forgotten that there was a separate xcb list.

>> +
>> +#define DRIVER_MAP_DRI3_ONLY
>
> What does this define do?

It is designed to be used in case there are two drivers for a chipset,
one DRI2 one and one DRI3 one. Much like the DRIVER_MAP_GALLIUM_ONLY
flag. It's not currently being used at all, so it should probably just
get removed until necessary.

> '{' on a separate line, please.

Fixed.

> This looks completely like dri2_create_context, except for missing
> rendertype validation and a different calloc size.

Yup. It's cult-n-paste. Note that the license at the top of the file
tries to make it clear that *lots* of the code in this file was copied
From the DRI2 versions. They all take new data structures now, with the
DRI2 stuff replaced with DRI3 stuff, so we can't just shared the same
binaries without a bunch of rework.

> tabs :(

Oh, looks like my xorg emacs settings for using spaces only aren't
applying to mesa. Sorry. And fixed.

> This looks like an exact copy of dri2_create_context_attribs except for
> the vtable, the calloc size being different, the reset initialization,
> and the createContextAttribs looking in image_driver instead of dri2.
>
> This sucks.

As above.

>> +//   pdraw->bufferCount = 0;
>
> Leftover debug code?

Already removed.

>> +
>> +   *ust = priv->ust;
>> +   *msc = priv->msc;
>> +
>
> funny extra newline.

Fixed.

> Some sort of comment about what's going on here?  Seems like sleep(1)
> would always be a wrong thing to execute.

I've added this comment:

/** dri3_wait_for_sbc
 *
 * Wait for the swap buffer count to increase. The only way this
 * can happen is if some other thread is doing swap buffers as
 * we no longer share swap buffer counts with other processes.
 *
 * I'm not sure this is actually useful as such, and so this
 * implementation is a kludge that just polls once a second
 */

I'm not sure what else to do here; presumably there would be locking
preventing multiple threads from getting into this code at the same
time. As none of this code is thread-safe, I'm pretty sure this would
never work anyways.

> I think we can drop this entirely thanks to flush_with_flags (see
> below).  The gallium-only implementation of this driver extension is
> just a call to flush_with_flags.

> I'd rather insist that the driver supports flush_with_flags if you do
> DRI3.

Done.

>
>> +static void
>> +dri3_copy_sub_buffer(__GLXDRIdrawable *pdraw, int x, int y,
>> +		  int width, int height, Bool flush)
>> +{
>> +   _dri3_copy_sub_buffer(pdraw, x, y, width, height,
>> +                         __DRI2_THROTTLE_COPYSUBBUFFER, flush);
>> +}
>
> This appears to be a pointless wrapper.

Removed.

>> +static void
>> +dri3_copy_drawable(struct dri3_drawable *priv, Drawable dest, Drawable src)
>> +{
>> +   struct dri3_screen *psc = (struct dri3_screen *) priv->base.psc;
>> +   xcb_connection_t     *c = XGetXCBConnection(priv->base.psc->dpy);
>> +
>> +   if (psc->f)
>> +      (*psc->f->flush) (priv->driDrawable);
>
> Use flush_with_flags instead.

I call dri3_flush. I've also removed the context argument to dri3_flush
as everyone was passing dri3_current_context, then removed
dri3_current_context and folded that code into dri3_flush.

>> +   dri3_copy_area(c,
>> +                  src, dest,
>> +                  dri3_drawable_gc(priv),
>> +                  0, 0, 0, 0, priv->width, priv->height);
>> +}
>
> DRI2CopyRegion round-tripped, while this call doesn't.  As a result, I
> think dri3_wait_x is broken because it doesn't ensure that the copyarea
> actually happens before your driver goes rendering again.  dri3_wait_gl
> may be similarly wrong in the other way.
>
> We don't have testing for glXWaitGL() or glXWaitX() at all, and that's
> bad.
...
> What's going on with fence reset/triggering being present in
> copysubbuffer but not these entrypoints?

Good catch. I've added fencing around the dri3_copy_area call to
synchronize with the X server.

>> +static struct dri3_buffer *
>> +dri3_alloc_render_buffer(struct glx_screen *glx_screen, Drawable draw, unsigned int format, int width, int height, int depth)
>
> 80-column wrap

Fixed.

> trailing whitespace

Fixed (everywhere)

>
>> +   if (!buffer->image)
>> +      goto no_image;
>> +
>> +   if (!(*psc->image->queryImage)(buffer->image, __DRI_IMAGE_ATTRIB_STRIDE, &stride))
>> +      goto no_buffer_attrib;
>> +
>> +   buffer->pitch = stride;
>> +
>> +   if (!(*psc->image->queryImage)(buffer->image, __DRI_IMAGE_ATTRIB_FD, &buffer_fd))
>> +      goto no_buffer_attrib;
>
>
>> +
>> +   xcb_dri3_pixmap_from_buffer(c,
>> +                               (pixmap = xcb_generate_id(c)),
>> +                               draw,
>> +                               buffer->size,
>> +                               width, height, buffer->pitch,
>> +                               depth, buffer->cpp * 8,
>
> I don't see buffer->cpp initialized anywhere.

Ick. Lost this in the conversion to __DRIimage. I've added a function
that computes cpp from the __DRI_IMAGE_FORMAT_* value and uses that.

> You should probably comment what's going on here.  Is an error going to
> be returned iff it's a pixmap?

Comments added (the select_input request returns a BadWindow error for
pixmaps, which this code interprets as 'that must be a pixmap')

> What's up with commented out formats?

They're just missing from the __DRI_IMAGE_FOURCC definitions. I've just
removed them for now. Krh promises to eliminate all of the
__DRI_IMAGE_FORMAT_* bits and use __DRI_IMAGE_FOURCC everywhere at some
point -- that will eliminate this function entirely.

>> +
>> +   image_planar = (*psc->image->createImageFromFds) (psc->driScreen,
>> +                                                     bp_reply->width,
>> +                                                     bp_reply->height,
>> +                                                     image_format_to_fourcc(format),
>> +                                                     fds, 1,
>> +                                                     &stride, &offset, buffer);
>> +   close(buffer_fd);
>
> just drop buffer_fd and reference fds[0] again?

Done.

>> +   if (!image_planar)
>> +      goto no_image;
>> +
>> +   buffer->image = (*psc->image->fromPlanar)(image_planar, 0, buffer);
>> +
>> +   (*psc->image->destroyImage)(image_planar);
>
> Is the fromPlanar step here actually needed?  It looks like since
> num_fds == 1 you get a functional image from the first step.

Kristian says that it is required -- the upper level __DRIimage might be
a wrapper that points at multiple __DRIimage objects, and in this case
it would point at precisely one of them. I note that the intel driver
doesn't work like that though.

> I think this early return leaks front.  Also have_fake_front's setting
> leaked in even though we're not updating priv->front, should it have?

It isn't "leaked", in that the dri3 code keeps track of the allocated
fake front and will re-use it in future allocations, and free it when
the drawable is destroyed.

However, you're right in asserting that the have_fake_front shouldn't be
set to TRUE until we are going to return success. I've moved that down.

> No need for NULL-checking free().

Fixed

> I'd really like to see less loader code duplication.  But if you have
> to, at least don't support old setTexBuffer when you know the driver's
> new enough that it's got DRI3.

Done. The DRI3 code now requires version 3 of the texBuffer extension
with setTexBuffer2 and releaseTexBuffer defined.

> Remove the #ifdef.  You're in the tree, you know its value.

Removed.

>> +//   if (pdp->swapAvailable && strcmp(driverName, "vmwgfx") != 0) {
>> +//      __glXEnableDirectExtension(&psc->base, "GLX_INTEL_swap_event");
>> +//   }
>
> more commented leftovers.  Are you dropping swap_event support?

Yes, DRI3/Present does not support GLX_INTEL_swap_event. I'd have to add
X server support to deliver that event. Is it useful enough that I
should do this?

> This is horribly duplicated with dri2, but that's also horribly
> duplicated with EGL's dri3.  I really think we need to do a
> dri_loader_common between all of them with a bunch of this crap.

I'm not sure where we would put code to share code between glx and egl,
but it sure would be nice.

>> +   tmp = getenv("LIBGL_SHOW_FPS");
>> +   psc->show_fps = tmp && strcmp(tmp, "1") == 0;
>
> Dead code.

Removed.

>> +
>> +struct dri3_buffer {
>> +   __DRIimage   *image;
>> +   uint32_t     pixmap;
>
>> +   uint32_t     sync_fence;
>> +   int32_t      *shm_fence;
>> +   GLboolean    busy;
>
> Can we get some comments on what these 3 fields do?  These
> synchronization details were a huge part of the dri3 discussions, and I
> know you've gone several ways about things in the process of
> development, but I see just a single comment about what fences do:

   /* Synchronization between the client and X server is done using an
    * xshmfence that is mapped into an X server SyncFence. This lets the
    * client check whether the X server is done using a buffer with a simple
    * xshmfence call, rather than going to read X events from the wire.
    *
    * However, we can only wait for one xshmfence to be triggered at a time,
    * so we need to know *which* buffer is going to be idle next. We do that
    * by waiting for a PresentIdleNotify event. When that event arrives, the
    * 'busy' flag gets cleared and the client knows that the fence has been
    * triggered, and that the wait call will not block.
    */

   uint32_t     sync_fence;     /* XID of X SyncFence object */
   int32_t      *shm_fence;     /* pointer to xshmfence object */
   GLboolean    busy;           /* Set on swap, cleared on IdleNotify */

>  +   /* Mark the buffer as idle */
>
> That's... not enough.

This is at allocation time; we know the buffer is idle. What else would
you like me to do here?

>
>> +struct dri3_drawable
>> +{
>
>> +   /* For WaitMSC */
>> +   uint32_t     present_msc_request_serial;
>> +   uint32_t     present_msc_event_serial;
>> +   
>
> whitespace

Fixed.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20131105/0f2801ad/attachment.pgp>