Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 14 Dec 2011 19:44:34 +0800, "Zhigang Gong" <zhigang.gong@linux.intel.com> wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Wednesday, December 14, 2011 7:12 PM
> > To: Zhigang Gong
> > Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com
> > Subject: RE: [PATCH] uxa/glamor: Enable the rest glamor rendering
> > functions.
> > 
> > On Wed, 14 Dec 2011 12:08:30 +0800, "Zhigang Gong"
> > <zhigang.gong@linux.intel.com> wrote:
> > > > -----Original Message-----
> > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > Sent: Wednesday, December 14, 2011 2:45 AM
> > > > To: zhigang.gong@linux.intel.com
> > > > Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com;
> > > > zhigang.gong@linux.intel.com
> > > > Subject: Re: [PATCH] uxa/glamor: Enable the rest glamor rendering
> > > > functions.
> > > >
> > > > On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com
> > > > wrote:
> > > > > From: Zhigang Gong <zhigang.gong@linux.intel.com>
> > > > >
> > > > > This commit enable all the rest glamor rendering functions.
> > > > > Tested with latest glamor master branch, can pass rendercheck.
> > > >
> > > > Hmm, it exposes an issue with keeping a bo cache independent of
> > mesa
> > > > and trying to feed it our own handles:
> > > >
> > > >  Region for name 6 already exists but is not compatible
> > > >
> > > > The w/a for this would be:
> > > >
> > > > diff --git a/src/intel_glamor.c b/src/intel_glamor.c index
> > > 0cf8ed7..2757fd6
> > > > 100644
> > > > --- a/src/intel_glamor.c
> > > > +++ b/src/intel_glamor.c
> > > > @@ -91,6 +91,7 @@
> > intel_glamor_create_textured_pixmap(PixmapPtr
> > > > pixmap)
> > > >         priv = intel_get_pixmap_private(pixmap);
> > > >         if (glamor_egl_create_textured_pixmap(pixmap,
> > > > priv->bo->handle,
> > > >                                               priv->stride)) {
> > > > +               drm_intel_bo_disable_reuse(priv->bo);
> > > >                 priv->pinned = 1;
> > > >                 return TRUE;
> > > >         } else
> > > >
> > > > but that gives up all pretense of maintaining a bo cache.
> > >
> > > Yes, I think this impacts the performance. Actually, I noticed this
> > > problem and I spent some time to track the root cause. If everything
> > > is ok, this error should not be triggered. Although the same BO maybe
> > > reused to create a new pixmap, the previous pixmap which own this BO
> > > should be already destroyed. And the previous image created with the
> > > previous pixmap should be destroyed either.
> > 
> > Certainly it looks like glamor is taking all necessary steps to decouple
> the
> > bo from the textures and renderbuffer upon pixmap finish. There is one
> > other potential race here in that the ddx will mark the bo as purgeable as
> > soon as it releases it back to the cache, but it may not yet have been
> > submitted by mesa in its execbuffer. The kernel may choose to free the
> > memory associated with the bo before that happens, and may rightfully
> > complain the userspace is doing something silly.
> 
> Right, we do have this race if the kernel free the BO's memory prior to
> The mesa submit its execbuffer. Hmm. But I think that may not be a real
> problem, as once we call intel_set_pixmap_bo(pixmap, NULL) to unlink
> the bo from the pixmap, the BO will not be released at DRM layer
> immediately, 
> instead, it will be put on a in_flight list. And intel_batch_submit will
> empty the
> list, considering after switching to glamor, each pixmap's batch buffer
> should
> be empty, then the driver will only call intel_batch_submit at
> intel_flush_rendering
> which is called from intel_uxa_block_handler and is after the
> intel_glamor_flush.
> At intel_glamor_flush, it will do a glFlush, my understanding is glFlush
> should make sure the execbuffer was submitted to GPU. But I'm not very sure.
> Can you confirm that? Thanks.

It shouldn't go on the in-flight list because we're not using
intel_batchbuffer any more and so it will not be referenced by the
batch.

This patch along with calling dispatch->glFlush() after deleting the
textured pixmap is enough to silence the warning inside mesa and
prevent the purgeable race:

diff --git a/src/mesa/drivers/dri/intel/intel_context.c
b/src/mesa/drivers/dri/intel/intel_context.c
index 068b305..347a5d6 100644
--- a/src/mesa/drivers/dri/intel/intel_context.c
+++ b/src/mesa/drivers/dri/intel/intel_context.c
@@ -34,6 +34,7 @@
 #include "main/imports.h"
 #include "main/points.h"
 #include "main/renderbuffer.h"
+#include "main/state.h"
 
 #include "swrast/swrast.h"
 #include "swrast_setup/swrast_setup.h"
@@ -527,6 +528,8 @@ intel_glFlush(struct gl_context *ctx)
    intel_flush_front(ctx);
    if (intel->is_front_buffer_rendering)
       intel->need_throttle = true;
+
+   _mesa_update_state(ctx);
 }

diff --git a/src/mesa/main/texstate.c b/src/mesa/main/texstate.c
index 7cd2858..4bcaec0 100644
--- a/src/mesa/main/texstate.c
+++ b/src/mesa/main/texstate.c
@@ -557,6 +557,7 @@ update_texture_state( struct gl_context *ctx )
 
       if (enabledTargets == 0x0) {
          /* neither vertex nor fragment processing uses this unit */
+	 _mesa_reference_texobj(&texUnit->_Current, NULL);
          continue;
       }
 
@@ -592,6 +593,7 @@ update_texture_state( struct gl_context *ctx )
          }
          else {
             /* fixed-function: texture unit is really disabled */
+	    _mesa_reference_texobj(&texUnit->_Current, NULL);
             continue;
          }
       }

-- 
Chris Wilson, Intel Open Source Technology Centre