[Mesa-dev,3/3] dri3: Add GLX_EXT_buffer_age support

Submitted by Adel Gadllah on Feb. 20, 2014, 10:15 a.m.

Details

Message ID 1392891351-14318-4-git-send-email-adel.gadllah@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Adel Gadllah Feb. 20, 2014, 10:15 a.m.
---
 src/glx/dri2_glx.c      |  1 +
 src/glx/dri3_glx.c      | 18 ++++++++++++++++++
 src/glx/dri3_priv.h     |  2 ++
 src/glx/glx_pbuffer.c   |  8 ++++++++
 src/glx/glxclient.h     |  1 +
 src/glx/glxextensions.c |  1 +
 src/glx/glxextensions.h |  1 +
 7 files changed, 32 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 67fe9c1..146802a 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -1288,6 +1288,7 @@  dri2CreateScreen(int screen, struct glx_display * priv)
    psp->waitForSBC = NULL;
    psp->setSwapInterval = NULL;
    psp->getSwapInterval = NULL;
+   psp->getBufferAge = NULL;
 
    if (pdp->driMinor >= 2) {
       psp->getDrawableMSC = dri2DrawableGetMSC;
diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
index 70ec057..697d448 100644
--- a/src/glx/dri3_glx.c
+++ b/src/glx/dri3_glx.c
@@ -1345,6 +1345,7 @@  dri3_swap_buffers(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t divisor,
          target_msc = priv->msc + priv->swap_interval * (priv->send_sbc - priv->recv_sbc);
 
       priv->buffers[buf_id]->busy = 1;
+      priv->buffers[buf_id]->last_swap = priv->swap_count;
       xcb_present_pixmap(c,
                          priv->base.xDrawable,
                          priv->buffers[buf_id]->pixmap,
@@ -1379,11 +1380,25 @@  dri3_swap_buffers(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t divisor,
       xcb_flush(c);
       if (priv->stamp)
          ++(*priv->stamp);
+
+      priv->swap_count++;
    }
 
    return ret;
 }
 
+static int
+dri3_get_buffer_age(__GLXDRIdrawable *pdraw)
+{
+  struct dri3_drawable *priv = (struct dri3_drawable *) pdraw;
+  const struct dri3_buffer *const back = dri3_back_buffer(priv);
+
+  if (back->last_swap != 0)
+     return priv->swap_count - back->last_swap;
+  else
+     return 0;
+}
+
 /** dri3_open
  *
  * Wrapper around xcb_dri3_open
@@ -1742,6 +1757,9 @@  dri3_create_screen(int screen, struct glx_display * priv)
    psp->copySubBuffer = dri3_copy_sub_buffer;
    __glXEnableDirectExtension(&psc->base, "GLX_MESA_copy_sub_buffer");
 
+   psp->getBufferAge = dri3_get_buffer_age;
+   __glXEnableDirectExtension(&psc->base, "GLX_EXT_buffer_age");
+
    free(driverName);
    free(deviceName);
 
diff --git a/src/glx/dri3_priv.h b/src/glx/dri3_priv.h
index 1d124f8..d00440a 100644
--- a/src/glx/dri3_priv.h
+++ b/src/glx/dri3_priv.h
@@ -97,6 +97,7 @@  struct dri3_buffer {
    uint32_t     cpp;
    uint32_t     flags;
    uint32_t     width, height;
+   uint32_t     last_swap;
 
    enum dri3_buffer_type        buffer_type;
 };
@@ -184,6 +185,7 @@  struct dri3_drawable {
    struct dri3_buffer *buffers[DRI3_NUM_BUFFERS];
    int cur_back;
    int num_back;
+   uint32_t swap_count;
 
    uint32_t *stamp;
 
diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
index 978730c..afb4206 100644
--- a/src/glx/glx_pbuffer.c
+++ b/src/glx/glx_pbuffer.c
@@ -373,6 +373,14 @@  GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
             if (!pdraw->textureFormat)
                pdraw->textureFormat =
                   determineTextureFormat((const int *) data, num_attributes);
+
+            if (attribute == GLX_BACK_BUFFER_AGE_EXT) {
+               struct glx_screen *psc = pdraw->psc;
+
+               if (psc->driScreen->getBufferAge != NULL)
+                 *value = psc->driScreen->getBufferAge(pdraw);
+            }
+
          }
 #endif
 
diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
index a7118af..74c19c4 100644
--- a/src/glx/glxclient.h
+++ b/src/glx/glxclient.h
@@ -125,6 +125,7 @@  struct __GLXDRIscreenRec {
 		     int64_t *msc, int64_t *sbc);
    int (*setSwapInterval)(__GLXDRIdrawable *pdraw, int interval);
    int (*getSwapInterval)(__GLXDRIdrawable *pdraw);
+   int (*getBufferAge)(__GLXDRIdrawable *pdraw);
 };
 
 struct __GLXDRIdrawableRec
diff --git a/src/glx/glxextensions.c b/src/glx/glxextensions.c
index f186c13..ac1b4a7 100644
--- a/src/glx/glxextensions.c
+++ b/src/glx/glxextensions.c
@@ -103,6 +103,7 @@  static const struct extension_info known_glx_extensions[] = {
    { GLX(SGIX_visual_select_group),    VER(0,0), Y, Y, N, N },
    { GLX(EXT_texture_from_pixmap),     VER(0,0), Y, N, N, N },
    { GLX(INTEL_swap_event),            VER(0,0), Y, N, N, N },
+   { GLX(EXT_buffer_age),              VER(0,0), Y, N, Y, Y },
    { NULL }
 };
 
diff --git a/src/glx/glxextensions.h b/src/glx/glxextensions.h
index 8436a94..37e4ccc 100644
--- a/src/glx/glxextensions.h
+++ b/src/glx/glxextensions.h
@@ -66,6 +66,7 @@  enum
    SGIX_visual_select_group_bit,
    EXT_texture_from_pixmap_bit,
    INTEL_swap_event_bit,
+   EXT_buffer_age_bit,
 };
 
 /* From the GLX perspective, the ARB and EXT extensions are identical.  Use a

Comments

Hi Adel,

Thanks for look at this; just a few comments...

> diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
> index 978730c..afb4206 100644
> --- a/src/glx/glx_pbuffer.c
> +++ b/src/glx/glx_pbuffer.c
> @@ -373,6 +373,14 @@ GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
>              if (!pdraw->textureFormat)
>                 pdraw->textureFormat =
>                    determineTextureFormat((const int *) data, num_attributes);
> +
> +            if (attribute == GLX_BACK_BUFFER_AGE_EXT) {
> +               struct glx_screen *psc = pdraw->psc;
> +
> +               if (psc->driScreen->getBufferAge != NULL)
> +                 *value = psc->driScreen->getBufferAge(pdraw);
> +            }
> +
>           }
>  #endif

Since with dri3 this is a client side only attribute, shouldn't we be
able to simply skip the redundant X round trip above to query the
drawable's attributes?

Technically we're supposed to report a GLXBadDrawable if the drawable
isn't currently bound to the current thread's context. I think the
requirement to be bound to a context was added to the spec considering
that if an app were switching between a direct/indirect context the
age state may switch from being client-side to server-side and they
shouldn't have to be kept in sync. glXSwapBuffers has a similar
requirement so it should be possible to borrow some checks from there.

Apart from that, this patch looks good to me:

Reviewed-by: Robert Bragg <robert@sixbynine.org>

--
Regards,
Robert
Adel Gadllah <adel.gadllah@gmail.com> writes:

> ---
>  src/glx/dri2_glx.c      |  1 +
>  src/glx/dri3_glx.c      | 18 ++++++++++++++++++
>  src/glx/dri3_priv.h     |  2 ++
>  src/glx/glx_pbuffer.c   |  8 ++++++++
>  src/glx/glxclient.h     |  1 +
>  src/glx/glxextensions.c |  1 +
>  src/glx/glxextensions.h |  1 +
>  7 files changed, 32 insertions(+)
>
> diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
> index 67fe9c1..146802a 100644
> --- a/src/glx/dri2_glx.c
> +++ b/src/glx/dri2_glx.c
> @@ -1288,6 +1288,7 @@ dri2CreateScreen(int screen, struct glx_display * priv)
>     psp->waitForSBC = NULL;
>     psp->setSwapInterval = NULL;
>     psp->getSwapInterval = NULL;
> +   psp->getBufferAge = NULL;
>  
>     if (pdp->driMinor >= 2) {
>        psp->getDrawableMSC = dri2DrawableGetMSC;
> diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
> index 70ec057..697d448 100644
> --- a/src/glx/dri3_glx.c
> +++ b/src/glx/dri3_glx.c
> @@ -1345,6 +1345,7 @@ dri3_swap_buffers(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t divisor,
>           target_msc = priv->msc + priv->swap_interval * (priv->send_sbc - priv->recv_sbc);
>  
>        priv->buffers[buf_id]->busy = 1;
> +      priv->buffers[buf_id]->last_swap = priv->swap_count;
>        xcb_present_pixmap(c,
>                           priv->base.xDrawable,
>                           priv->buffers[buf_id]->pixmap,
> @@ -1379,11 +1380,25 @@ dri3_swap_buffers(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t divisor,
>        xcb_flush(c);
>        if (priv->stamp)
>           ++(*priv->stamp);
> +
> +      priv->swap_count++;
>     }

Can't you just use priv->send_sbc for swap_count?  It looks like you've
got the order swapped of the two operations currently:

           "* The current back buffer's age is set to 1.
            * Any other color buffers' ages are incremented by 1 if
              their age was previously greater than 0."

As is, when an application gets the buffer from the first swap back, it
will get a 0 (invalid) age instead of a valid age.
Am 24.02.2014 17:58, schrieb Robert Bragg:
> Hi Adel,
Hi,
> Thanks for look at this; just a few comments...
>
>> diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
>> index 978730c..afb4206 100644
>> --- a/src/glx/glx_pbuffer.c
>> +++ b/src/glx/glx_pbuffer.c
>> @@ -373,6 +373,14 @@ GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
>>               if (!pdraw->textureFormat)
>>                  pdraw->textureFormat =
>>                     determineTextureFormat((const int *) data, num_attributes);
>> +
>> +            if (attribute == GLX_BACK_BUFFER_AGE_EXT) {
>> +               struct glx_screen *psc = pdraw->psc;
>> +
>> +               if (psc->driScreen->getBufferAge != NULL)
>> +                 *value = psc->driScreen->getBufferAge(pdraw);
>> +            }
>> +
>>            }
>>   #endif
> Since with dri3 this is a client side only attribute, shouldn't we be
> able to simply skip the redundant X round trip above to query the
> drawable's attributes?
Yeah good point.
>
> Technically we're supposed to report a GLXBadDrawable if the drawable
> isn't currently bound to the current thread's context. I think the
> requirement to be bound to a context was added to the spec considering
> that if an app were switching between a direct/indirect context the
> age state may switch from being client-side to server-side and they
> shouldn't have to be kept in sync. glXSwapBuffers has a similar
> requirement so it should be possible to borrow some checks from there.
That too.
> Apart from that, this patch looks good to me:
>
> Reviewed-by: Robert Bragg <robert@sixbynine.org>
>
Thanks for the review.

Adel
Am 24.02.2014 19:37, schrieb Eric Anholt:
> Adel Gadllah <adel.gadllah@gmail.com> writes:
>
>> ---
>>   src/glx/dri2_glx.c      |  1 +
>>   src/glx/dri3_glx.c      | 18 ++++++++++++++++++
>>   src/glx/dri3_priv.h     |  2 ++
>>   src/glx/glx_pbuffer.c   |  8 ++++++++
>>   src/glx/glxclient.h     |  1 +
>>   src/glx/glxextensions.c |  1 +
>>   src/glx/glxextensions.h |  1 +
>>   7 files changed, 32 insertions(+)
>>
>> diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
>> index 67fe9c1..146802a 100644
>> --- a/src/glx/dri2_glx.c
>> +++ b/src/glx/dri2_glx.c
>> @@ -1288,6 +1288,7 @@ dri2CreateScreen(int screen, struct glx_display * priv)
>>      psp->waitForSBC = NULL;
>>      psp->setSwapInterval = NULL;
>>      psp->getSwapInterval = NULL;
>> +   psp->getBufferAge = NULL;
>>   
>>      if (pdp->driMinor >= 2) {
>>         psp->getDrawableMSC = dri2DrawableGetMSC;
>> diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
>> index 70ec057..697d448 100644
>> --- a/src/glx/dri3_glx.c
>> +++ b/src/glx/dri3_glx.c
>> @@ -1345,6 +1345,7 @@ dri3_swap_buffers(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t divisor,
>>            target_msc = priv->msc + priv->swap_interval * (priv->send_sbc - priv->recv_sbc);
>>   
>>         priv->buffers[buf_id]->busy = 1;
>> +      priv->buffers[buf_id]->last_swap = priv->swap_count;
>>         xcb_present_pixmap(c,
>>                            priv->base.xDrawable,
>>                            priv->buffers[buf_id]->pixmap,
>> @@ -1379,11 +1380,25 @@ dri3_swap_buffers(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t divisor,
>>         xcb_flush(c);
>>         if (priv->stamp)
>>            ++(*priv->stamp);
>> +
>> +      priv->swap_count++;
>>      }
> Can't you just use priv->send_sbc for swap_count?
I could but then would introduce what you describe below.
> It looks like you've
> got the order swapped of the two operations currently:
>
>             "* The current back buffer's age is set to 1.
>              * Any other color buffers' ages are incremented by 1 if
>                their age was previously greater than 0."
>
> As is, when an application gets the buffer from the first swap back, it
> will get a 0 (invalid) age instead of a valid age.
I don't get what you mean by swapped. If you mean increment swap count and then assign to last_swap then no. If I do 
that the age will always be 0.

To avoid the case where last_swap is 0 even though it should be valid ... could simply initialize swap_count to 1.