[Mesa-dev] [PATCH 05/16] glx/dri2: rework __DRIextension handling

[Kristian Høgsberg]

On Fri, Apr 25, 2014 at 11:40 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 25/04/14 19:02, Kristian Høgsberg wrote:
>> On Sun, Mar 16, 2014 at 6:48 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> Use a const arrays with the extensions, and use the correct one at
>>> runtime. This removes the assumption that the dri2_screen has a
>>> fixed size dri extension list and avoids the individual assignment
>>> of each extension at runtime.
>>
>> This change seems like unnecessary churn.  Your new mechanism doesn't
>> scale to multiple optional extensions, where it turns into a
>> combinatorial explosion.  The array size is of course correlated with
>> the max number of extensions you might add to the array and we could
>> add an assert to make sure we don't overwrite it.
>>
> Not sure what explosion are you referring to here. Want to add an extra
> extension - just add two lines of code.

If you have five extension that all depend on different things you end
up with 2^5 different combinations of extension arrays. By building
the extension list dynamically, you can add each of these extensions
to the list as you check the conditions they depend on.

> Idea behind this is, as we already know the complete set of extensions, there
> is little to no benefit in looping like crazy at runtime. The benefit may be
> ~0% although it makes code _a bit_ easier to follow.

Let's tone down the rhetoric a bit, there's no "looping like crazy"
here, we're building an array of five extension at runtime, there's no
performance problem here and the code isn't harder to follow.

> -Emil
>
>>> While we're here make sure that the DRI*Extensions report the
>>> version of the interface implemented over the listed in the headers.
>>> While both are currently the same, this may change in the future.
>>
>> This part is good.
>>
>>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>>> ---
>>>  src/glx/dri2_glx.c | 37 +++++++++++++++++++++----------------
>>>  1 file changed, 21 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
>>> index acd8427..84ae875 100644
>>> --- a/src/glx/dri2_glx.c
>>> +++ b/src/glx/dri2_glx.c
>>> @@ -74,7 +74,7 @@ struct dri2_display
>>>
>>>     __glxHashTable *dri2Hash;
>>>
>>> -   const __DRIextension *loader_extensions[4];
>>> +   const __DRIextension **loader_extensions;
>>>  };
>>>
>>>  struct dri2_context
>>> @@ -968,7 +968,21 @@ static const __DRIdri2LoaderExtension dri2LoaderExtension_old = {
>>>  };
>>>
>>>  static const __DRIuseInvalidateExtension dri2UseInvalidate = {
>>> -   { __DRI_USE_INVALIDATE, __DRI_USE_INVALIDATE_VERSION }
>>> +   .base = { __DRI_USE_INVALIDATE, 1 }
>>> +};
>>> +
>>> +static const __DRIextension *loader_extensions_old[] = {
>>> +   &dri2LoaderExtension_old.base,
>>> +   &systemTimeExtension.base,
>>> +   &dri2UseInvalidate.base,
>>> +   NULL,
>>> +};
>>> +
>>> +static const __DRIextension *loader_extensions[] = {
>>> +   &dri2LoaderExtension.base,
>>> +   &systemTimeExtension.base,
>>> +   &dri2UseInvalidate.base,
>>> +   NULL,
>>>  };
>>>
>>>  _X_HIDDEN void
>>> @@ -1240,15 +1254,13 @@ dri2CreateScreen(int screen, struct glx_display * priv)
>>>     if (psc->dri2->base.version >= 4) {
>>>        psc->driScreen =
>>>           psc->dri2->createNewScreen2(screen, psc->fd,
>>> -                                     (const __DRIextension **)
>>> -                                     &pdp->loader_extensions[0],
>>> +                                     pdp->loader_extensions,
>>>                                       extensions,
>>>                                       &driver_configs, psc);
>>>     } else {
>>>        psc->driScreen =
>>>           psc->dri2->createNewScreen(screen, psc->fd,
>>> -                                    (const __DRIextension **)
>>> -                                    &pdp->loader_extensions[0],
>>> +                                    pdp->loader_extensions,
>>>                                      &driver_configs, psc);
>>>     }
>>>
>>> @@ -1365,7 +1377,7 @@ _X_HIDDEN __GLXDRIdisplay *
>>>  dri2CreateDisplay(Display * dpy)
>>>  {
>>>     struct dri2_display *pdp;
>>> -   int eventBase, errorBase, i;
>>> +   int eventBase, errorBase;
>>>
>>>     if (!DRI2QueryExtension(dpy, &eventBase, &errorBase))
>>>        return NULL;
>>> @@ -1386,17 +1398,10 @@ dri2CreateDisplay(Display * dpy)
>>>     pdp->base.destroyDisplay = dri2DestroyDisplay;
>>>     pdp->base.createScreen = dri2CreateScreen;
>>>
>>> -   i = 0;
>>>     if (pdp->driMinor < 1)
>>> -      pdp->loader_extensions[i++] = &dri2LoaderExtension_old.base;
>>> +      pdp->loader_extensions = loader_extensions_old;
>>>     else
>>> -      pdp->loader_extensions[i++] = &dri2LoaderExtension.base;
>>> -
>>> -   pdp->loader_extensions[i++] = &systemTimeExtension.base;
>>> -
>>> -   pdp->loader_extensions[i++] = &dri2UseInvalidate.base;
>>> -
>>> -   pdp->loader_extensions[i++] = NULL;
>>> +      pdp->loader_extensions = loader_extensions;
>>>
>>>     pdp->dri2Hash = __glxHashCreate();
>>>     if (pdp->dri2Hash == NULL) {
>>> --
>>> 1.9.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>