[7/8] dri: add __DRIimageLoaderExtension and __DRIimageDriverExtension

Commit Message

Keith Packard Nov. 6, 2013, 6:09 p.m. UTC

Kristian Høgsberg <[e-mail]> 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.

> 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:

Comments

Kristian Høgsberg Nov. 6, 2013, 7:06 p.m. UTC | #1

On Wed, Nov 6, 2013 at 10:09 AM, Keith Packard <[e-mail]> wrote:
> Kristian Høgsberg <[e-mail]> 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,
>
> --
> [e-mail]

Keith Packard Nov. 6, 2013, 7:29 p.m. UTC | #2

Kristian Høgsberg <[e-mail]> writes:

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

We should probably not merge what we don't want to maintain though;
let's decide over lunch. I don't think it matters very much to the
current code, it'll only bug us in small ways in the future.

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

That's what I was thinking.

Patch

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,