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

["Zhigang Gong" <zhigang.gong@linux.intel.com>]

> -----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.

> 
> > And then, when we create a new pixmap/image with this BO, MESA
> should
> > not find any exist image/region for this BO. But it does happen. I
> > tracked further into mesa internal and found that the previous image
> > was not destroyed when we call eglDestroyImageKHR, as its reference
> > count is decreased to zero. It's weird for me. Further tracking shows
> > that the root cause is when I use the texture(bind to the image) as a
> > shader's source texture, and call glDrawArrays to perform the
> > rendering, the texture's reference count will be increased by 1 before
> > return from glDrawArrays. And I failed to find any API to decrease it.
> > Then this texture can't be freed when destroy that texture and thus
> > the image's reference count will also remain 1 and can't be freed
> > either.
> 
> I'm looking at update_texture_state() which appears to hold onto a
> reference to the texobj until its slot is reused. If the object is simply
> destroyed and the unit disabled, the slot is skipped and the old reference
> remains. _mesa_update_state (which calls into
> update_texture_state) would seem to be only invoked upon a draw
> primitives. Binding a dummy texture/fb and doing a dummy render might
> be a little extreme as a workaround!
Thanks for providing this workaround, I did try that way with my simple test
case, it works. I just wonder whether this is a bug of MESA. 
I will implement it in glamor before we get a better solution.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre