[PATCH] drm/i915: Fix current fb blocking for page flip.

[PATCH] drm/i915: Fix current fb blocking for page flip.

[Chia-I Wu]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

Hi list,

According to the doc for page_flip, intel_crtc_page_flip should

   ... block all rendering to the current fb until the flip has
   completed.

I am not entirely sure, but it seems that it is
work->old_fb_obj->pending_flip that needs to be incremented instead of
work->pending_flip_obj->pending_flip.  This patch does fix the
rendering artifacts with my Android on i915 project.  Any thought?

-- 
olv@LunarG.com

[-- Attachment #2: 0001-drm-i915-Fix-current-fb-blocking-for-page-flip.patch --]
[-- Type: text/x-diff, Size: 1351 bytes --]

From fd72d779b84d70bac104d5d46541e3ac1ced6f35 Mon Sep 17 00:00:00 2001
From: Chia-I Wu <olvaffe@gmail.com>
Date: Thu, 21 Oct 2010 16:39:14 +0800
Subject: [PATCH] drm/i915: Fix current fb blocking for page flip.

Block execbuffer for the fb to be flipped away, not the one that is to
be flipped in.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9792285..41bf75a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4928,7 +4928,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
-	obj_priv = to_intel_bo(work->pending_flip_obj);
+	obj_priv = to_intel_bo(work->old_fb_obj);
 
 	/* Initial scanout buffer will have a 0 pending flip count */
 	if ((atomic_read(&obj_priv->pending_flip) == 0) ||
@@ -5031,8 +5031,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_objs;
 
-	obj_priv = to_intel_bo(obj);
+	obj_priv = to_intel_bo(work->old_fb_obj);
 	atomic_inc(&obj_priv->pending_flip);
+
+	obj_priv = to_intel_bo(obj);
 	work->pending_flip_obj = obj;
 
 	if (IS_GEN3(dev) || IS_GEN2(dev)) {
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix current fb blocking for page flip.

[Chris Wilson]

On Thu, 21 Oct 2010 17:55:13 +0800, Chia-I Wu <olvaffe@gmail.com> wrote:
> Hi list,
> 
> According to the doc for page_flip, intel_crtc_page_flip should
> 
>    ... block all rendering to the current fb until the flip has
>    completed.
> 
> I am not entirely sure, but it seems that it is
> work->old_fb_obj->pending_flip that needs to be incremented instead of
> work->pending_flip_obj->pending_flip.  This patch does fix the
> rendering artifacts with my Android on i915 project.  Any thought?

My understanding is that we want to block any attempts to render to the
new back-buffer, i.e. the current front-buffer, until the flip has
occurred. Following that logic we need to increment pending flip on the
existing fb, which at first glance is indeed the fix you propose.

Jesse, Kirstian, thoughts?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix current fb blocking for page flip.

[Jesse Barnes]

On Thu, 21 Oct 2010 17:55:13 +0800
Chia-I Wu <olvaffe@gmail.com> wrote:

> Hi list,
> 
> According to the doc for page_flip, intel_crtc_page_flip should
> 
>    ... block all rendering to the current fb until the flip has
>    completed.
> 
> I am not entirely sure, but it seems that it is
> work->old_fb_obj->pending_flip that needs to be incremented instead of
> work->pending_flip_obj->pending_flip.  This patch does fix the
> rendering artifacts with my Android on i915 project.  Any thought?

In intel_crtc_page_flip()?  It *looks* like incrementing the flip count
for the fb passed into the routine is the right thing to do; we want to
make sure the fb passed in isn't used again until its flip is complete.

But maybe we're decrementing it incorrectly in the buffer exec path or
allowing rendering to continue too soon?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Fix current fb blocking for page flip.

[Chia-I Wu]

On Fri, Oct 22, 2010 at 12:18 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 21 Oct 2010 17:55:13 +0800
> Chia-I Wu <olvaffe@gmail.com> wrote:
>
>> Hi list,
>>
>> According to the doc for page_flip, intel_crtc_page_flip should
>>
>>    ... block all rendering to the current fb until the flip has
>>    completed.
>>
>> I am not entirely sure, but it seems that it is
>> work->old_fb_obj->pending_flip that needs to be incremented instead of
>> work->pending_flip_obj->pending_flip.  This patch does fix the
>> rendering artifacts with my Android on i915 project.  Any thought?
> In intel_crtc_page_flip()?  It *looks* like incrementing the flip count
> for the fb passed into the routine is the right thing to do; we want to
> make sure the fb passed in isn't used again until its flip is complete.
If one flips a buffer and immediately renders to it, the caller should
know that the buffer is the front buffer and there will be artifacts.
Waiting for the flip here makes less sense.

A common use of page flip looks like

     [X being the front buffer]
  1) render to Y
  2) flip Y
  3) render to X
  4) flip X
  5) render to Y

A caller expects 3) to be back buffer rendering.  It is reasonable to
insert a wait between 2) and 3).  That is, have pending_flip of X,
instead of Y, set at 2).
> But maybe we're decrementing it incorrectly in the buffer exec path or
> allowing rendering to continue too soon?

> --
> Jesse Barnes, Intel Open Source Technology Center
>



-- 
olv@LunarG.com

Re: [PATCH] drm/i915: Fix current fb blocking for page flip.

[Jesse Barnes]

On Fri, 22 Oct 2010 01:49:17 +0800
Chia-I Wu <olvaffe@gmail.com> wrote:

> On Fri, Oct 22, 2010 at 12:18 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Thu, 21 Oct 2010 17:55:13 +0800
> > Chia-I Wu <olvaffe@gmail.com> wrote:
> >
> >> Hi list,
> >>
> >> According to the doc for page_flip, intel_crtc_page_flip should
> >>
> >>    ... block all rendering to the current fb until the flip has
> >>    completed.
> >>
> >> I am not entirely sure, but it seems that it is
> >> work->old_fb_obj->pending_flip that needs to be incremented instead of
> >> work->pending_flip_obj->pending_flip.  This patch does fix the
> >> rendering artifacts with my Android on i915 project.  Any thought?
> > In intel_crtc_page_flip()?  It *looks* like incrementing the flip count
> > for the fb passed into the routine is the right thing to do; we want to
> > make sure the fb passed in isn't used again until its flip is complete.
> If one flips a buffer and immediately renders to it, the caller should
> know that the buffer is the front buffer and there will be artifacts.
> Waiting for the flip here makes less sense.
> 
> A common use of page flip looks like
> 
>      [X being the front buffer]
>   1) render to Y
>   2) flip Y
>   3) render to X
>   4) flip X
>   5) render to Y
> 
> A caller expects 3) to be back buffer rendering.  It is reasonable to
> insert a wait between 2) and 3).  That is, have pending_flip of X,
> instead of Y, set at 2).

You're right, I had it backwards; I think your fix is correct.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix current fb blocking for page flip.

[Kristian Høgsberg]

On Thu, Oct 21, 2010 at 1:49 PM, Chia-I Wu <olvaffe@gmail.com> wrote:
> On Fri, Oct 22, 2010 at 12:18 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> On Thu, 21 Oct 2010 17:55:13 +0800
>> Chia-I Wu <olvaffe@gmail.com> wrote:
>>
>>> Hi list,
>>>
>>> According to the doc for page_flip, intel_crtc_page_flip should
>>>
>>>    ... block all rendering to the current fb until the flip has
>>>    completed.
>>>
>>> I am not entirely sure, but it seems that it is
>>> work->old_fb_obj->pending_flip that needs to be incremented instead of
>>> work->pending_flip_obj->pending_flip.  This patch does fix the
>>> rendering artifacts with my Android on i915 project.  Any thought?
>> In intel_crtc_page_flip()?  It *looks* like incrementing the flip count
>> for the fb passed into the routine is the right thing to do; we want to
>> make sure the fb passed in isn't used again until its flip is complete.
> If one flips a buffer and immediately renders to it, the caller should
> know that the buffer is the front buffer and there will be artifacts.
> Waiting for the flip here makes less sense.
>
> A common use of page flip looks like
>
>     [X being the front buffer]
>  1) render to Y
>  2) flip Y
>  3) render to X
>  4) flip X
>  5) render to Y
>
> A caller expects 3) to be back buffer rendering.  It is reasonable to
> insert a wait between 2) and 3).  That is, have pending_flip of X,
> instead of Y, set at 2).

Yes, that's how it was supposed to work.  When an application queues a
flip to a new buffer, the next thing it wants to do is typically to
start rendering the next frame to the new backbuffer.  But in a
typical double buffer mechanism, the new backbuffer is the current
scanout buffer, until the flip is done, so we have to block the
application if it touches the current front until the flip is done.

Kristian
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix current fb blocking for page flip.

[Chris Wilson]

Chia-I,

I've pushed an updated version of the patch to -staging and added acks
from both Kristian and Jesse since they both agreed with your assessment
of the bug.

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre