On Wed, Nov 6, 2013 at 10:09 AM, Keith Packard <keithp@xxxxxxxxxx> wrote:
> Kristian Høgsberg <hoegsberg@xxxxxxxxx> 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@xxxxxxxxx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
