[PATCH 0/2] fixup pageflip for i8xx class hw

[PATCH 0/2] fixup pageflip for i8xx class hw

[Daniel Vetter]

Hi all,

Kde4 started using pageflips and I started noticing that it's decently
broken on my i855. These two patches fix things up, here.

Please review and merge for -next.

Thanks, Daniel

PS: Any news on a documentation drop for gen2/gen3? Switching on my X-ray
vision and deducing the correct magic dance from the chip's circuit
layout is slowly getting boring ... ;)

Daniel Vetter (2):
  drm/i915: fixup pageflip ringbuffer commands for i8xx
  drm/i915: i8xx also doesn't like multiple oustanding pageflips

 drivers/gpu/drm/i915/intel_display.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

[PATCH 1/2] drm/i915: fixup pageflip ringbuffer commands for i8xx

[Daniel Vetter]

Add a new path for 2nd gen chips that uses the commands for i81x
chips (where public docs do exist) augmented with the plane bits
from i915. It seems to work and doesn't result in a black screen
like before.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@kernel.org
---
 drivers/gpu/drm/i915/intel_display.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8359c50..8135ee0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4933,12 +4933,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		OUT_RING(obj_priv->gtt_offset | obj_priv->tiling_mode);
 		pipesrc = I915_READ(pipesrc_reg); 
 		OUT_RING(pipesrc & 0x0fff0fff);
-	} else {
+	} else if (IS_GEN3(dev)) {
 		OUT_RING(MI_DISPLAY_FLIP_I915 |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 		OUT_RING(fb->pitch);
 		OUT_RING(obj_priv->gtt_offset);
 		OUT_RING(MI_NOOP);
+	} else {
+		OUT_RING(MI_DISPLAY_FLIP |
+			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
+		OUT_RING(fb->pitch);
+		OUT_RING(obj_priv->gtt_offset);
+		OUT_RING(MI_NOOP);
 	}
 	ADVANCE_LP_RING();
 
-- 
1.7.1

[PATCH 2/2] drm/i915: i8xx also doesn't like multiple oustanding pageflips

[Daniel Vetter]

My i855GM suffers from a 80k/s interrupt storm without this.
So add 2nd gen to the list of things that don't like more than
one outstanding pageflip request.

Furthermore I've changed the busy loop into a ringbuffer wait.
Busy-loops that don't check whether the chip died are simply evil.
And performance should actually improve, because there's usually
a decent amount of rendering queued on the gpu, hopefully rendering
that MI_WAIT into a noop by the time it's executed.

The current code holds dev->struct_mutex while executing this loop,
hence stalling all other gem activity anyway.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stabel@kernel.org
---
 drivers/gpu/drm/i915/intel_display.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8135ee0..7b6035e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4916,14 +4916,17 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	work->pending_flip_obj = obj;
 
 	if (intel_crtc->plane)
-		flip_mask = I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
+		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
 	else
-		flip_mask = I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT;
+		flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
 
 	/* Wait for any previous flip to finish */
-	if (IS_GEN3(dev))
-		while (I915_READ(ISR) & flip_mask)
-			;
+	if (IS_GEN3(dev) || IS_GEN2(dev)) {
+		BEGIN_LP_RING(2);
+		OUT_RING(MI_WAIT_FOR_EVENT | flip_mask);
+		OUT_RING(0);
+		ADVANCE_LP_RING();
+	}
 
 	BEGIN_LP_RING(4);
 	if (IS_I965G(dev)) {
-- 
1.7.1

Re: [PATCH 1/2] drm/i915: fixup pageflip ringbuffer commands for i8xx

[Jesse Barnes]

On Wed,  4 Aug 2010 21:22:09 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Add a new path for 2nd gen chips that uses the commands for i81x
> chips (where public docs do exist) augmented with the plane bits
> from i915. It seems to work and doesn't result in a black screen
> like before.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@kernel.org
> ---
>  drivers/gpu/drm/i915/intel_display.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8359c50..8135ee0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4933,12 +4933,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  		OUT_RING(obj_priv->gtt_offset | obj_priv->tiling_mode);
>  		pipesrc = I915_READ(pipesrc_reg); 
>  		OUT_RING(pipesrc & 0x0fff0fff);
> -	} else {
> +	} else if (IS_GEN3(dev)) {
>  		OUT_RING(MI_DISPLAY_FLIP_I915 |
>  			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>  		OUT_RING(fb->pitch);
>  		OUT_RING(obj_priv->gtt_offset);
>  		OUT_RING(MI_NOOP);
> +	} else {
> +		OUT_RING(MI_DISPLAY_FLIP |
> +			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> +		OUT_RING(fb->pitch);
> +		OUT_RING(obj_priv->gtt_offset);
> +		OUT_RING(MI_NOOP);
>  	}
>  	ADVANCE_LP_RING();

Yep, looks fine.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: i8xx also doesn't like multiple oustanding pageflips

[Jesse Barnes]

On Wed,  4 Aug 2010 21:22:10 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> My i855GM suffers from a 80k/s interrupt storm without this.
> So add 2nd gen to the list of things that don't like more than
> one outstanding pageflip request.
> 
> Furthermore I've changed the busy loop into a ringbuffer wait.
> Busy-loops that don't check whether the chip died are simply evil.
> And performance should actually improve, because there's usually
> a decent amount of rendering queued on the gpu, hopefully rendering
> that MI_WAIT into a noop by the time it's executed.
> 
> The current code holds dev->struct_mutex while executing this loop,
> hence stalling all other gem activity anyway.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stabel@kernel.org
> ---
>  drivers/gpu/drm/i915/intel_display.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8135ee0..7b6035e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4916,14 +4916,17 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	work->pending_flip_obj = obj;
>  
>  	if (intel_crtc->plane)
> -		flip_mask = I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> +		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
>  	else
> -		flip_mask = I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT;
> +		flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
>  
>  	/* Wait for any previous flip to finish */
> -	if (IS_GEN3(dev))
> -		while (I915_READ(ISR) & flip_mask)
> -			;
> +	if (IS_GEN3(dev) || IS_GEN2(dev)) {
> +		BEGIN_LP_RING(2);
> +		OUT_RING(MI_WAIT_FOR_EVENT | flip_mask);
> +		OUT_RING(0);
> +		ADVANCE_LP_RING();
> +	}
>  
>  	BEGIN_LP_RING(4);
>  	if (IS_I965G(dev)) {

This looks nicer than what was there before.  Hope it still works on
9xx...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: i8xx also does not like multiple oustanding pageflips

[Chris Wilson]

On Wed,  4 Aug 2010 21:22:10 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> My i855GM suffers from a 80k/s interrupt storm without this.
> So add 2nd gen to the list of things that don't like more than
> one outstanding pageflip request.
> 
> Furthermore I've changed the busy loop into a ringbuffer wait.
> Busy-loops that don't check whether the chip died are simply evil.
> And performance should actually improve, because there's usually
> a decent amount of rendering queued on the gpu, hopefully rendering
> that MI_WAIT into a noop by the time it's executed.
> 
> The current code holds dev->struct_mutex while executing this loop,
> hence stalling all other gem activity anyway.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stabel@kernel.org
> ---
>  drivers/gpu/drm/i915/intel_display.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8135ee0..7b6035e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4916,14 +4916,17 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	work->pending_flip_obj = obj;
>  
>  	if (intel_crtc->plane)
> -		flip_mask = I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> +		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
>  	else
> -		flip_mask = I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT;
> +		flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
>  
>  	/* Wait for any previous flip to finish */
> -	if (IS_GEN3(dev))
> -		while (I915_READ(ISR) & flip_mask)
> -			;
> +	if (IS_GEN3(dev) || IS_GEN2(dev)) {
> +		BEGIN_LP_RING(2);
> +		OUT_RING(MI_WAIT_FOR_EVENT | flip_mask);
> +		OUT_RING(0);
> +		ADVANCE_LP_RING();
> +	}

If you move the if(intel_crtc->plane) inside the gen2/3 block, then I'll
test it on my netbooks. :)

Starting to look like intel_overlay.c ;-)

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: i8xx also does not like multiple oustanding pageflips

[Daniel Vetter]

On Wed, Aug 04, 2010 at 09:06:46PM +0100, Chris Wilson wrote:
> If you move the if(intel_crtc->plane) inside the gen2/3 block, then I'll
> test it on my netbooks. :)
I believe the compiler will happily do that for you ;)

> Starting to look like intel_overlay.c ;-)
Well, that's actually one of the things on my idea for drm/i915 projects:
To unify the overlay/display plane pageflip support code.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: i8xx also doesn't like multiple oustanding pageflips

[Pedro Ribeiro]

On 4 August 2010 20:49, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wed,  4 Aug 2010 21:22:10 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
>> My i855GM suffers from a 80k/s interrupt storm without this.
>> So add 2nd gen to the list of things that don't like more than
>> one outstanding pageflip request.
>>
>> Furthermore I've changed the busy loop into a ringbuffer wait.
>> Busy-loops that don't check whether the chip died are simply evil.
>> And performance should actually improve, because there's usually
>> a decent amount of rendering queued on the gpu, hopefully rendering
>> that MI_WAIT into a noop by the time it's executed.
>>
>> The current code holds dev->struct_mutex while executing this loop,
>> hence stalling all other gem activity anyway.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: stabel@kernel.org
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |   13 ++++++++-----
>>  1 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8135ee0..7b6035e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4916,14 +4916,17 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>       work->pending_flip_obj = obj;
>>
>>       if (intel_crtc->plane)
>> -             flip_mask = I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
>> +             flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
>>       else
>> -             flip_mask = I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT;
>> +             flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
>>
>>       /* Wait for any previous flip to finish */
>> -     if (IS_GEN3(dev))
>> -             while (I915_READ(ISR) & flip_mask)
>> -                     ;
>> +     if (IS_GEN3(dev) || IS_GEN2(dev)) {
>> +             BEGIN_LP_RING(2);
>> +             OUT_RING(MI_WAIT_FOR_EVENT | flip_mask);
>> +             OUT_RING(0);
>> +             ADVANCE_LP_RING();
>> +     }
>>
>>       BEGIN_LP_RING(4);
>>       if (IS_I965G(dev)) {
>
> This looks nicer than what was there before.  Hope it still works on
> 9xx...
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> --
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Just to warn: you sent the patch to the wrong address
(stabel@kernel.org instead of stable@kernel.org).

Regards,
Pedro