On Thu, Aug 18, 2016 at 9:28 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 18 August 2016 at 14:12, Rob Clark <robdcl...@gmail.com> wrote:
>> On Thu, Aug 18, 2016 at 8:45 AM, Emil Velikov <emil.l.veli...@gmail.com>
>> wrote:
>>> On 16 August 2016 at 18:01, Rob Clark <robdcl...@gmail.com> wrote:
>>>> I noticed that fence extension wasn't exposed for drm/gbm, which sort
>>>> of defeats the purpose of using fence fd's for synchronizing rendering
>>>> and atomic pageflip.
>>>>
>>> Afaict this is a slightly more complete version of Philipp's patch.
>>>
>>> While my earlier suggestion still stands, I should have mentioned that
>>> it's not a show stopper for this patch.
>>> Namely: we really want a way to check and bail out on ABI mismatch
>>> between libEGL and libgbm.
>>>
>>>> I suppose that somewhere or other, there needs to be a similar change
>>>> to .../state_trackers/dri/dri2.c to avoid breaking things on i965.
>>> Kind of ... see below.
>>>
>>>
>>>> ---
>>>> src/egl/drivers/dri2/platform_drm.c | 1 +
>>>> src/gallium/state_trackers/dri/dri2.c | 1 +
>>>> src/gbm/backends/dri/gbm_dri.c | 1 +
>>>> src/gbm/backends/dri/gbm_driint.h | 1 +
>>>> 4 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/src/egl/drivers/dri2/platform_drm.c
>>>> b/src/egl/drivers/dri2/platform_drm.c
>>>> index 1ce282f..7bea8e1 100644
>>>> --- a/src/egl/drivers/dri2/platform_drm.c
>>>> +++ b/src/egl/drivers/dri2/platform_drm.c
>>>> @@ -645,6 +645,7 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>>> dri2_dpy->dri_screen = dri2_dpy->gbm_dri->screen;
>>>> dri2_dpy->core = dri2_dpy->gbm_dri->core;
>>>> dri2_dpy->dri2 = dri2_dpy->gbm_dri->dri2;
>>>> + dri2_dpy->fence = dri2_dpy->gbm_dri->fence;
>>> Nice catch. Philipp's patch was missing this hunk, thus one could not
>>> have really used the fence extension from EGL.
>>>
>>>> dri2_dpy->image = dri2_dpy->gbm_dri->image;
>>>> dri2_dpy->flush = dri2_dpy->gbm_dri->flush;
>>>> dri2_dpy->swrast = dri2_dpy->gbm_dri->swrast;
>>>> diff --git a/src/gallium/state_trackers/dri/dri2.c
>>>> b/src/gallium/state_trackers/dri/dri2.c
>>>> index 75d740b..8ad8701 100644
>>>> --- a/src/gallium/state_trackers/dri/dri2.c
>>>> +++ b/src/gallium/state_trackers/dri/dri2.c
>>>> @@ -2042,6 +2042,7 @@ const __DRIextension *galliumdrm_driver_extensions[]
>>>> = {
>>>> &driImageDriverExtension.base,
>>>> &driDRI2Extension.base,
>>>> &gallium_config_options.base,
>>>> + &dri2FenceExtension.base,
>>> While things are a bit confusing I think this shouldn't be here, and
>>> thus there's no 'missing' i965 hunk.
>>>
>>> galliumdrm_driver_extensions are the "core" extensions which among
>>> other are responsible for:
>>> - setting up the screen (which itself sets up the screen extensions)
>>> - query screen extensions
>>>
>>> As a simple test - both gallium and i965 already expose the fence
>>> extension and it seems to be working fine afaict.
>>
>> Ok, in that case I think we need this hunk squashed in (but untested so far):
>>
>> --------
>> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
>> index d4602d1..3cd8783 100644
>> --- a/src/gbm/backends/dri/gbm_dri.c
>> +++ b/src/gbm/backends/dri/gbm_dri.c
>> @@ -244,13 +244,13 @@ struct dri_extension_match {
>> static struct dri_extension_match dri_core_extensions[] = {
>> { __DRI2_FLUSH, 1, offsetof(struct gbm_dri_device, flush) },
>> { __DRI_IMAGE, 1, offsetof(struct gbm_dri_device, image) },
>> + { __DRI2_FENCE, 2, offsetof(struct gbm_dri_device, fence) },
>> { NULL, 0, 0 }
>> };
>>
>> static struct dri_extension_match gbm_dri_device_extensions[] = {
>> { __DRI_CORE, 1, offsetof(struct gbm_dri_device, core) },
>> { __DRI_DRI2, 1, offsetof(struct gbm_dri_device, dri2) },
>> - { __DRI2_FENCE, 2, offsetof(struct gbm_dri_device, fence) },
>> { NULL, 0, 0 }
>> };
>> --------
>>
> Yes you're correct - we'll need to expose it (why did I read
> DRI2_FLUSH as DRI2_FENCE?).
>
> Adding it to the array makes it compulsory, due to the nature of
> dri_bind_extensions(). We could do that but it'll break all the
> classic drivers but i965.
>
> Something like the egl handling would be better:
>
> for (i = 0; extensions[i]; i++) {
> if (strcmp(extensions[i]->name, __DRI2_FENCE) == 0)
> dri2_dpy->fence = (__DRI2fenceExtension *) extensions[i];
> }
or maybe just add a 'bool optional' flag in dri_extension_match table?
BR,
-R
>
> -Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
