Re: [PATCH 2/3] glamor: turn on glamor.

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

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Monday, November 14, 2011 5:07 PM
> To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> 
> On Mon, 14 Nov 2011 13:01:36 +0800, "Zhigang Gong"
> <zhigang.gong@linux.intel.com> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Friday, November 11, 2011 9:13 PM
> > > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > > Subject: RE: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> > >
> > > On Fri, 11 Nov 2011 18:52:11 +0800, "Zhigang Gong"
> > > <zhigang.gong@linux.intel.com> wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > > Sent: Friday, November 11, 2011 5:12 PM
> > > > > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > > > > Subject: Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> > > > >
> > > > > On Fri, 11 Nov 2011 16:31:20 +0800, Zhigang Gong
> > > > > <zhigang.gong@linux.intel.com> wrote:
> > > > > > @@ -965,6 +969,9 @@ void
> > > > > intel_uxa_block_handler(intel_screen_private *intel)
> > > > > >  	 * framebuffer until significantly later.
> > > > > >  	 */
> > > > > >  	intel_flush_rendering(intel);
> > > > > > +#ifdef GLAMOR
> > > > > > +	intel_glamor_block_handler(intel);
> > > > > > +#endif
> > > > > >  }
> > > > >
> > > > > I suspect this is the wrong way around as we are not flushing
> > > > > the render cache of glamor's rendering to the scanout until the
> > > > > next block
> > > handler.
> > > > I don't understand here. Would you please explain more detail?
> Thanks.
> > >
> > > Whenever we render, the data ends up in the Render Cache and
> needs
> > > to be flushed out to memory before it is coherent with the CPU or in
> > > this case the Display Engine (i.e. scanout).
> > >
> > > intel_flush_rendering() does two tasks. The first is to submit any
> > > pending batch, and the second is to flush the Render Cache so that
> > > the modifications land on the scanout in a timely manner. It is
> > > probably best
> > if
> > > those two tasks were separated so that we do:
> > >
> > >   intel_uxa_block_handler(intel); // flush the UXA batch
> > >   intel_glamor_block_handler(intel); // flush the GL batch
> > >   intel_flush_rendering(intel); // flush the RenderCache to scanout
> > >
> > > However, you can simply rearrange the code and achieve it with the
> > > existing functions:
> > >
> > >   intel_glamor_block_handler(intel); // mark the front bo as dirty
> > > as needbe
> > >   intel_flush_rendering(intel); // flush UXA batch along with
> > > RenderCache
> > Thanks for the explanation here. But I still don't think the original
> > code is wrong regard to this cache flushing issue. Here is my
> > analysis:
> > intel_glamor_block_handler calls to glFlush(), and glFlush is similar
> > with the intel_flush_rendering, it calls intel_flush to flush the
> > batch buffers and then call intel_flush_frontbuffer to flush the
> > frontbuffer which flushes the scan out buffer. So when the screen
> > pixmap is accessed by glamor, and after we call
> > intel_glamor_block_handler, the Display Engine should see the correct
> > data
> >
> > Right?
> 
> No.
> 
> glFlush() does call intel_flush_front(). However that in turn calls
> dri2->flushFrontBuffer which is implemented for EGL with
> 
> static void
> dri2_flush_front_buffer(__DRIdrawable * driDrawable, void
> *loaderPrivate) {
>    /* FIXME: Does EGL support front buffer rendering at all? */ }
> 
> Neither does it perform the intended action via GLX (except that flushing
> the scanout is handled by the DDX as a normal part of its operation).

You are right. EGL layer will not do a really front buffer flushing. We have
to
let it be done in DDX layer. In my version 2 patch set, I already rearrange
the
code sequence as you suggested please review it again. The remaining work
for this issue is that I need to add new code to set the needs_flush
according
to the access type of glamor. Will do that soon.

Thanks. 

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