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

[Kristian Høgsberg]

On Wed, Nov 6, 2013 at 6:55 AM, Keith Packard <keithp at keithp.com> wrote:
> Kristian Høgsberg <hoegsberg at gmail.com> writes:
>
>> Having written the GBM and Wayland suport for this, it's pretty clear
>> that we just want to use __DRIdri2Extension instead of duplicating
>> these functions.  Support for the __DRIimage based getBuffers is a
>> small incremental patch to src/egl/drivers/dri2:
>
> That would require drivers to support all of the DRI2 functions, rather
> than just the few needed for Image support though.

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.

>> The problem is that technically we would have to do:
>>
>>       if (dri2_dpy->image_driver) {
>>
>>          /* use dri2_dpy->image_driver->createContextAttribs
>>
>>       } else if (dri2_dpy->dri2 &&
>>                dri2_dpy->dri2->base.version >= 3) {
>>
>>          /* use dri2_dpy->dri2->createContextAttribs
>>
>> all over the place even though they end up calling the same function.
>> The only real purpose of __DRIimageDriverExtension is to let the
>> loader know that the DRI driver supports and will use
>> __DRIimageLoaderExtension if provided, but we can just expose an empty
>> dummy extension to indicate that.
>
> Alternatively, the loader could simply call through the DRI2 version,
> acknowledging that they must be the same function? I'd sure like to be
> able to create an ImageLoader-only driver, even if that's not practical
> today.
>
> A couple of other options:
>
>  * Move the shared functions into a new structure which is embedded
>    in both the DRIdri2ExtensionRec and the DRIimageDriverExtensionRec,
>    then the loader could set a pointer to whichever it wanted to use
>    in it's own structure.
>
>  * Have the loader pull the functions out of the ExtensionRec and stick
>    them directly into its own structure. Then it could use those
>    directly and avoid version checks while running.

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.

> But, if we just want to use the DRI2 driver interface and create an
> extension purely to mark that the driver will use an Image loader,
> that's easy too.

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

Kristian

>> I think we should only look at image.front if the
>> __DRI_IMAGE_BUFFER_FRONT bit is set.  We never want to set up a front
>> buffer that we didn't ask for and it seems wrong that the loader has
>> to clear image.front, if the driver didn't ask for front.
>
> I simply duplicated the logic from the DRI2 path which works exactly the
> same way (asks for buffers, then looks to see what it got). I didn't dig
> into the details further than this, I'm afraid. The only case where asks
> != received is for pixmap targets where you always get a front buffer. I
> assumed there was some deep semantic requirement for that...

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.