On Wed, 14 Dec 2011 19:44:34 +0800, "Zhigang Gong" <zhigang.gong at linux.intel.com> wrote: > > -----Original Message----- > > From: Chris Wilson [mailto:chris at chris-wilson.co.uk] > > Sent: Wednesday, December 14, 2011 7:12 PM > > To: Zhigang Gong > > Cc: intel-gfx at lists.freedesktop.org; zhigang.gong at 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 at linux.intel.com> wrote: > > > > -----Original Message----- > > > > From: Chris Wilson [mailto:chris at chris-wilson.co.uk] > > > > Sent: Wednesday, December 14, 2011 2:45 AM > > > > To: zhigang.gong at linux.intel.com > > > > Cc: intel-gfx at lists.freedesktop.org; zhigang.gong at gmail.com; > > > > zhigang.gong at 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 at linux.intel.com > > > > wrote: > > > > > From: Zhigang Gong <zhigang.gong at 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
