On Mon, Jan 4, 2010 at 11:12 PM, Luca Barbieri <l...@luca-barbieri.com> wrote:
> The current code revalidates based on whether width or height have changed.
> This is unreliable (it may change two times, with another context having got
> the buffers for the intermediate size in the meantime) and causes two DRI2
> calls.
> Instead, we add the notion of a drawable sequence number, which tracks the
> number of times the surface textures have changed.
> This sequence number, along with a pointer to the surface, is stored in
> egl_g3d_buffer and is passed to ->validate.
> ->validate for DRI2 updates an internal sequence number if the width or
> height changed.
> If it did not change, it checks whether it matches the one provided by the
> client and, if so, does not return any texture.
> Otherwise, it proceeds as normal.
> If context validation is being forced, the sequence numbers are reset to 0 to
> force it.
I like the idea of sequence number. More comments inline.
> ---
> .../state_trackers/egl_g3d/common/egl_g3d.c | 32 +++++++---------
> .../state_trackers/egl_g3d/common/egl_g3d.h | 2 +
> src/gallium/state_trackers/egl_g3d/common/native.h | 4 ++-
> .../state_trackers/egl_g3d/x11/native_dri2.c | 28 ++++++++++----
> .../state_trackers/egl_g3d/x11/native_ximage.c | 39
> +++++++++++++-------
> 5 files changed, 65 insertions(+), 40 deletions(-)
>
> diff --git a/src/gallium/state_trackers/egl_g3d/common/egl_g3d.c
> b/src/gallium/state_trackers/egl_g3d/common/egl_g3d.c
> index d68f49e..480f5a7 100644
> --- a/src/gallium/state_trackers/egl_g3d/common/egl_g3d.c
> +++ b/src/gallium/state_trackers/egl_g3d/common/egl_g3d.c
> @@ -47,11 +47,11 @@ egl_g3d_validate_context(_EGLDisplay *dpy, _EGLContext
> *ctx)
> EGLint s, i;
>
> /* validate draw and/or read buffers */
> - num_surfaces = (gctx->base.ReadSurface == gctx->base.DrawSurface) ? 1 : 2;
> - for (s = 0; s < num_surfaces; s++) {
> + for (s = 0; s < 2; s++) {
Why this change?
> struct pipe_texture *textures[NUM_NATIVE_ATTACHMENTS];
> struct egl_g3d_surface *gsurf;
> struct egl_g3d_buffer *gbuf;
> + int cur_seq_num;
>
> if (s == 0) {
> gsurf = egl_g3d_surface(gctx->base.DrawSurface);
> @@ -62,24 +62,20 @@ egl_g3d_validate_context(_EGLDisplay *dpy, _EGLContext
> *ctx)
> gbuf = &gctx->read;
> }
>
> - if (!gctx->force_validate) {
> - EGLint cur_w, cur_h;
> -
> - cur_w = gsurf->base.Width;
> - cur_h = gsurf->base.Height;
> - gsurf->native->validate(gsurf->native,
> - gbuf->native_atts, gbuf->num_atts,
> - NULL,
> - &gsurf->base.Width, &gsurf->base.Height);
> - /* validate only when the geometry changed */
> - if (gsurf->base.Width == cur_w && gsurf->base.Height == cur_h)
> - continue;
> - }
> + if (gctx->force_validate || gsurf != gbuf->surface)
> + gbuf->seq_num = 0;
> + cur_seq_num = gbuf->seq_num;
>
> gsurf->native->validate(gsurf->native,
> gbuf->native_atts, gbuf->num_atts,
> (struct pipe_texture **) textures,
> - &gsurf->base.Width, &gsurf->base.Height);
> + &gsurf->base.Width, &gsurf->base.Height, &gbuf->seq_num);
> +
> + if(gbuf->seq_num == cur_seq_num)
> + continue;
> +
> + gbuf->surface = gsurf;
> +
Can we keep the logic as simple as:
if (not forced) {
get_current_sequence_number();
if (seq_num has not changed)
continue;
}
validate_and_update_buffers();
There should be no need to reset the sequence number or comparing the surface.
> for (i = 0; i < gbuf->num_atts; i++) {
> struct pipe_texture *pt = textures[i];
> struct pipe_surface *ps;
> @@ -538,7 +534,7 @@ egl_g3d_create_window_surface(_EGLDriver *drv,
> _EGLDisplay *dpy,
> }
>
> if (!gsurf->native->validate(gsurf->native, NULL, 0, NULL,
> - &gsurf->base.Width, &gsurf->base.Height)) {
> + &gsurf->base.Width, &gsurf->base.Height, 0)) {
> gsurf->native->destroy(gsurf->native);
> free(gsurf);
> return NULL;
> @@ -579,7 +575,7 @@ egl_g3d_create_pixmap_surface(_EGLDriver *drv,
> _EGLDisplay *dpy,
> }
>
> if (!gsurf->native->validate(gsurf->native, NULL, 0, NULL,
> - &gsurf->base.Width, &gsurf->base.Height)) {
> + &gsurf->base.Width, &gsurf->base.Height, 0)) {
> gsurf->native->destroy(gsurf->native);
> free(gsurf);
> return NULL;
> diff --git a/src/gallium/state_trackers/egl_g3d/common/egl_g3d.h
> b/src/gallium/state_trackers/egl_g3d/common/egl_g3d.h
> index 7f6823f..223b881 100644
> --- a/src/gallium/state_trackers/egl_g3d/common/egl_g3d.h
> +++ b/src/gallium/state_trackers/egl_g3d/common/egl_g3d.h
> @@ -51,6 +51,8 @@ struct egl_g3d_display {
>
> struct egl_g3d_buffer {
> struct st_framebuffer *st_fb;
> + struct egl_g3d_surface* surface;
This might not be needed.
> + int seq_num;
> EGLint num_atts;
> enum native_attachment native_atts[NUM_NATIVE_ATTACHMENTS];
> uint st_atts[NUM_NATIVE_ATTACHMENTS];
> diff --git a/src/gallium/state_trackers/egl_g3d/common/native.h
> b/src/gallium/state_trackers/egl_g3d/common/native.h
> index a59ceac..24cc506 100644
> --- a/src/gallium/state_trackers/egl_g3d/common/native.h
> +++ b/src/gallium/state_trackers/egl_g3d/common/native.h
> @@ -67,12 +67,14 @@ struct native_surface {
> /**
> * Validate the buffers of the surface. Those not listed in the
> attachments
> * will be destroyed. The returned textures are owned by the caller.
> + * If *seq_num is not 0, it will do nothing if the surface sequence number
> + * did not change. The current sequence number will be returned.
Could we make it:
/**
* Validate the buffers of the surface. The returned textures are owned by
* the caller. A sequence number is also returned. The caller can use it
* to see if anything has changed since the last call. Any of the pointers
* may be NULL and it indicates the caller has no interest in those values.
*
* If this function is called multiple times with different attachments,
* those not listed in the latest call might be destroyed. This behavior
* might change in the future.
*/
That is, seq_num is no longer an input.
> */
> boolean (*validate)(struct native_surface *nsurf,
> const enum native_attachment *natts,
> unsigned num_natts,
> struct pipe_texture **textures,
> - int *width, int *height);
> + int *width, int *height, int* seq_num);
>
> /**
> * Wait until all native commands affecting the surface has been executed.
> diff --git a/src/gallium/state_trackers/egl_g3d/x11/native_dri2.c
> b/src/gallium/state_trackers/egl_g3d/x11/native_dri2.c
> index 890e11d..e38695b 100644
> --- a/src/gallium/state_trackers/egl_g3d/x11/native_dri2.c
> +++ b/src/gallium/state_trackers/egl_g3d/x11/native_dri2.c
> @@ -67,6 +67,7 @@ struct dri2_surface {
> unsigned names[NUM_NATIVE_ATTACHMENTS];
> boolean have_back, have_fake;
> int width, height;
> + int seq_num;
> };
>
> struct dri2_config {
> @@ -140,7 +141,7 @@ dri2_surface_validate(struct native_surface *nsurf,
> const enum native_attachment *natts,
> unsigned num_natts,
> struct pipe_texture **textures,
> - int *width, int *height)
> + int *width, int *height, int* seq_num)
> {
> struct dri2_surface *dri2surf = dri2_surface(nsurf);
> struct dri2_display *dri2dpy = dri2surf->dri2dpy;
> @@ -211,18 +212,26 @@ dri2_surface_validate(struct native_surface *nsurf,
> texture_indices[natts[i]] = i;
> }
>
> - dri2surf->have_back = FALSE;
> - dri2surf->have_fake = FALSE;
> -
> xbufs = x11_drawable_get_buffers(dri2dpy->xscr, dri2surf->drawable,
> - &dri2surf->width, &dri2surf->height,
> + &templ.width0, &templ.height0,
> dri2atts, FALSE, num_ins, &num_outs);
> if (!xbufs)
> return FALSE;
>
> - /* update width and height */
> - templ.width0 = dri2surf->width;
> - templ.height0 = dri2surf->height;
> + if(!dri2surf->seq_num || dri2surf->width != templ.width0 ||
> dri2surf->height != templ.height0)
> + {
> + /* update width and height */
> + dri2surf->width = templ.width0;
> + dri2surf->height = templ.height0;
> +
> + if(!++dri2surf->seq_num)
> + ++dri2surf->seq_num; /* wrap to 1, not 0 */
> + }
> + else if(seq_num && *seq_num == dri2surf->seq_num)
> + goto out;
If this function is called twice, one with a back attachement followed by one
without a back attachment, x11_drawable_get_buffers above will destroy the back
buffer in the second call. We need to update dri2surf->have_back
before bailing out.
> +
> + dri2surf->have_back = FALSE;
> + dri2surf->have_fake = FALSE;
>
> for (i = 0; i < num_outs; i++) {
> struct x11_drawable_buffer *xbuf = &xbufs[i];
> @@ -309,12 +318,15 @@ dri2_surface_validate(struct native_surface *nsurf,
> }
> }
>
> +out:
> free(xbufs);
>
> if (width)
> *width = dri2surf->width;
> if (height)
> *height = dri2surf->height;
> + if(seq_num)
> + *seq_num = dri2surf->seq_num;
>
> return TRUE;
> }
> diff --git a/src/gallium/state_trackers/egl_g3d/x11/native_ximage.c
> b/src/gallium/state_trackers/egl_g3d/x11/native_ximage.c
> index e02faa9..70e2ffb 100644
> --- a/src/gallium/state_trackers/egl_g3d/x11/native_ximage.c
> +++ b/src/gallium/state_trackers/egl_g3d/x11/native_ximage.c
> @@ -80,6 +80,7 @@ struct ximage_surface {
> struct ximage_display *xdpy;
>
> int width, height;
> + int seq_num;
> GC gc;
>
> struct ximage_buffer buffers[NUM_NATIVE_ATTACHMENTS];
> @@ -289,10 +290,11 @@ ximage_surface_validate(struct native_surface *nsurf,
> const enum native_attachment *natts,
> unsigned num_natts,
> struct pipe_texture **textures,
> - int *width, int *height)
> + int *width, int *height, int* seq_num)
> {
> struct ximage_surface *xsurf = ximage_surface(nsurf);
> boolean error = FALSE;
> + boolean changed = FALSE;
> unsigned i;
>
> ximage_surface_update_geometry(&xsurf->base);
> @@ -318,28 +320,39 @@ ximage_surface_validate(struct native_surface *nsurf,
> xbuf->ximage->height = xbuf->transfer->height;
> xbuf->ximage->bytes_per_line = xbuf->transfer->stride;
> }
> +
> + changed = TRUE;
> }
> - }
>
> - /* allocation failed */
> - if (!xbuf->texture) {
> - unsigned j;
> - for (j = 0; j < i; j++)
> - pipe_texture_reference(&textures[j], NULL);
> - for (j = i; j < num_natts; j++)
> - textures[j] = NULL;
> - error = TRUE;
> - break;
> + /* allocation failed */
> + if (!xbuf->texture)
> + return FALSE;
> }
> + }
> +
> + if(changed)
> + {
> + if(!++xsurf->seq_num)
> + ++xsurf->seq_num; /* wrap to 1 */
> + }
> + else if(seq_num && *seq_num == xsurf->seq_num)
> + return TRUE;
>
> - if (textures)
> - pipe_texture_reference(&textures[i], xbuf->texture);
> + if(textures) {
> + for (i = 0; i < num_natts; i++) {
> + struct ximage_buffer *xbuf = &xsurf->buffers[natts[i]];
> +
> + if (xbuf)
> + pipe_texture_reference(&textures[i], xbuf->texture);
> + }
> }
>
> if (width)
> *width = xsurf->width;
> if (height)
> *height = xsurf->height;
> + if(seq_num)
> + *seq_num = xsurf->seq_num;
>
> return !error;
> }
> --
> 1.6.3.3
>
>
------------------------------------------------------------------------------
This SF.Net email is sponsored by the Verizon Developer Community
Take advantage of Verizon's best-in-class app development support
A streamlined, 14 day to market process makes app distribution fast and easy
Join now and get one step closer to millions of Verizon customers
http://p.sf.net/sfu/verizon-dev2dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
