Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
On Mon, 14 Nov 2011 13:01:36 +0800, Zhigang Gong
zhigang.g...@linux.intel.com wrote:
-Original Message-
From: Chris Wilson [mailto:ch...@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.g...@linux.intel.com wrote:
-Original Message-
From: Chris Wilson [mailto:ch...@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.g...@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).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
-Original Message-
From: Chris Wilson [mailto:ch...@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.g...@linux.intel.com wrote:
-Original Message-
From: Chris Wilson [mailto:ch...@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.g...@linux.intel.com wrote:
-Original Message-
From: Chris Wilson [mailto:ch...@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.g...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
-Original Message- From: Chris Wilson [mailto:ch...@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.g...@linux.intel.com wrote: -Original Message- From: Chris Wilson [mailto:ch...@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.g...@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? - Zhigang -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
Thanks for the review. Fixed that empty #ifdef block, missed one line code
there to free glamor screen.
From: eugeni.dodo...@gmail.com [mailto:eugeni.dodo...@gmail.com] On Behalf
Of Eugeni Dodonov
Sent: Friday, November 11, 2011 8:59 PM
To: Zhigang Gong
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
On Fri, Nov 11, 2011 at 06:31, Zhigang Gong zhigang.g...@linux.intel.com
wrote:
@@ -1109,7 +1127,8 @@ static void I830FreeScreen(int scrnIndex, int flags)
{
ScrnInfoPtr scrn = xf86Screens[scrnIndex];
intel_screen_private *intel = intel_get_screen_private(scrn);
-
+#ifdef GLAMOR
+#endif
Empty #ifdef block?
--
Eugeni Dodonov http://eugeni.dodonov.net/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
On Fri, 11 Nov 2011 16:31:20 +0800, Zhigang Gong zhigang.g...@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. In general, try to keep the #ifdef out of the body of the code. In this case, and others, make intel_glamor_block_handler() be a no-op if GLAMOR is not enabled. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
-Original Message- From: Chris Wilson [mailto:ch...@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.g...@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. In general, try to keep the #ifdef out of the body of the code. In this case, and others, make intel_glamor_block_handler() be a no-op if GLAMOR is not enabled. Agreed, will fix it next version. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
On Fri, Nov 11, 2011 at 06:31, Zhigang Gong zhigang.g...@linux.intel.comwrote:
@@ -1109,7 +1127,8 @@ static void I830FreeScreen(int scrnIndex, int flags)
{
ScrnInfoPtr scrn = xf86Screens[scrnIndex];
intel_screen_private *intel = intel_get_screen_private(scrn);
-
+#ifdef GLAMOR
+#endif
Empty #ifdef block?
--
Eugeni Dodonov
http://eugeni.dodonov.net/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
On Fri, 11 Nov 2011 18:52:11 +0800, Zhigang Gong zhigang.g...@linux.intel.com wrote: -Original Message- From: Chris Wilson [mailto:ch...@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.g...@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 -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
