[PATCH 7/8] dri: add __DRIimageLoaderExtension and __DRIimageDriverExtension

[Kristian Høgsberg]

On Wed, Nov 6, 2013 at 10:09 AM, Keith Packard <keithp at keithp.com> wrote:
> Kristian Høgsberg <hoegsberg at gmail.com> writes:
>
>> It just the two older create context functions (which fall back to
>> calling driCreateContextAtribs) and allocateBuffer and releaseBuffer.
>> The two buffer functions are __DRIbuffer specific of course, but we
>> can implement them in terms of __DRIimage in dri_util.c now.
>
> I guess the benefit is that we could remove the DRIdri2Extension
> functions from each driver and just use the DRIimage-based wrappers in
> dri_util then?
>
> We're still stuck with leaving the DRIdri2Extension as the interface
> From the loader though.
>
>> There is a third option, which is to pull the functions we need from
>> __DRIcoreExtension into the __DRIimageDriverExtension, which then is
>> all you need along with __DRIimageExtension.  This is as painful in
>> the short term as the current __DRIimageDriverExtension, but it lets
>> of cut loose of the DRI1 (__DRIcoreExtension has the DRI1
>> createNewScreen in it) and DRI2 baggage properly later on.  And
>> pulling out the functions into a loader private struct as you suggest
>> will make it a lot less painful.  The functions we move from core to
>> _DRIimageDriverExtension will share the same implementation in
>> dri_util.c so there's no new code.
>
> That doesn't seem like a crazy plan; at least Image-based loaders would
> be simple then; find the DRIimageDriverExtension and that's it.
>
>> Right - I actually like the clean break idea, but if we're going to
>> take that pain I want to get rid of all the junk and avoid the awkward
>> "use some stuff from __DRIcoreExtension and other stuff from
>> __DRIimageDriverExtension" setup.  So we should either 1) make
>> __DRIimageDriverExtension completely replace __DRIcoreExtension and
>> __DRIdri2Extension, or 2) just do a minimal, incremental change (just
>> the extension to indicate the support for __DRIimage based
>> getbuffers).
>
> If we're going to get drivers to add DRIimageExtension to the list of
> exported extensions, then it doesn't seem like it matters which way we
> go here -- we can move from 2) to 1) in the future without changing any
> drivers, only the dri_util bits and the loaders.
>
> However, if we think that 1) is the way to go, we might as well just do
> it as that'd avoid having to ever fix the loaders.

I'm OK with either approach. It does seem like cleaning up the DRI
driver interface is orthogonal to enabling the __DRIimage based
getBuffer callout though.

>> The difference is that there the loader returns a packed array of the
>> buffers the driver asked for. Now we're using a struct which can be
>> sparsely populated, so the driver should only look at the fields it
>> asked for.
>
> My concern is that the DRI2 drivers always return a front buffer for
> pixmap drawables, and I think this is actually required for things to
> work right (I have vague memories of hacking at this when I started
> this).
>
> How about I just stick the set of returned images back into the
> DRIimageList struct:

I think that's fine.  I was going to say that if we expect the
requested and the returned set of buffers to differ, we might as well
just memset the struct and let non-NULL images indicate returned
images.  But in case of a driver with a newer interface that extends
the struct (stereoscopic buffers), the loader can't memset the entire
struct (it only knows the smaller, previous version), and the driver
will think the non-NULL garbage fields are valid images.  So the
image_mask makes sense.

Kristian

> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
> index 2601249..2a873d8 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -1319,6 +1319,7 @@ enum __DRIimageBufferMask {
>  };
>
>  struct __DRIimageList {
> +   uint32_t image_mask;
>     __DRIimage *back;
>     __DRIimage *front;
>  };
> diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
> index 1bb8241..7de7abd 100644
> --- a/src/glx/dri3_glx.c
> +++ b/src/glx/dri3_glx.c
> @@ -1208,6 +1208,7 @@ dri3_get_buffers(__DRIdrawable *driDrawable,
>     struct dri3_drawable *priv = loaderPrivate;
>     struct dri3_buffer   *front, *back;
>
> +   buffers->image_mask = 0;
>     buffers->front = NULL;
>     buffers->back = NULL;
>
> @@ -1254,12 +1255,15 @@ dri3_get_buffers(__DRIdrawable *driDrawable,
>     }
>
>     if (front) {
> +      buffers->image_mask |= __DRI_IMAGE_BUFFER_FRONT;
>        buffers->front = front->image;
>        priv->have_fake_front = !priv->is_pixmap;
>     }
>
> -   if (back)
> +   if (back) {
> +      buffers->image_mask |= __DRI_IMAGE_BUFFER_BACK;
>        buffers->back = back->image;
> +   }
>
>     priv->stamp = stamp;
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index 90bbbfc..273d455 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -1338,7 +1338,7 @@ intel_update_image_buffers(struct brw_context *brw, __DRIdrawable *drawable)
>                                          buffer_mask,
>                                          &images);
>
> -   if (images.front) {
> +   if (images.image_mask & __DRI_IMAGE_BUFFER_FRONT) {
>        assert(front_rb);
>        drawable->w = images.front->width;
>        drawable->h = images.front->height;
> @@ -1348,7 +1348,7 @@ intel_update_image_buffers(struct brw_context *brw, __DRIdrawable *drawable)
>                                  images.front,
>                                  __DRI_IMAGE_BUFFER_FRONT);
>     }
> -   if (images.back) {
> +   if (images.image_mask & __DRI_IMAGE_BUFFER_BACK) {
>        drawable->w = images.back->width;
>        drawable->h = images.back->height;
>        intel_update_image_buffer(brw,
>
> --
> keith.packard at intel.com