[Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane

2014-10-30 Thread Paulo Zanoni 
From: Paulo Zanoni 

Commit "drm/i915: create a prepare phase for sprite plane updates"
changed the old_obj pointer we use when committing sprite planes,
which caused a WARN() and a BUG() to be triggered. This patch should
revert the code back to the previous behavior, fixing the regression.

Regression introduced by:
commit ec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan 
Date:   Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updates

Credits to Imre Deak for pointing out the exact lines that were wrong.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Credits-to: Imre Deak 
Cc: Gustavo Padovan 
Cc: Ville Syrjälä 
Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 8b80d68..1c874309 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
struct drm_device *dev = plane->dev;
struct drm_crtc *crtc = state->crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+   struct intel_plane *intel_plane = to_intel_plane(plane);
enum pipe pipe = intel_crtc->pipe;
struct drm_framebuffer *fb = state->fb;
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-   struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+   struct drm_i915_gem_object *old_obj = intel_plane->obj;
int ret;
 
if (old_obj != obj) {
-- 
2.1.1

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

[Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane

2014-11-10 Thread Paulo Zanoni 
From: Paulo Zanoni 

Commit "drm/i915: create a prepare phase for sprite plane updates"
changed the old_obj pointer we use when committing sprite planes,
which caused a WARN() and a BUG() to be triggered. Later, commit
"drm/i915: use intel_fb_obj() macros to assign gem objects" introduced
the same problem to function intel_commit_sprite_plane().

Regression introduced by:
commit ec82cb793c9224e0692eed904f43490cf70e8258
Author: Gustavo Padovan 
Date:   Fri Oct 24 14:51:32 2014 +0100
drm/i915: create a prepare phase for sprite plane updates
and:
commit 77cde95217484e845743818691df026cec2534f4
Author: Gustavo Padovan 
Date:   Fri Oct 24 14:51:33 2014 +0100
drm/i915: use intel_fb_obj() macros to assign gem objects

Credits to Imre Deak for pointing out the exact lines that were wrong.

v2: Also fix intel_commit_sprite_plane() (Ville)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Credits-to: Imre Deak 
Cc: Gustavo Padovan 
Cc: Ville Syrjälä 
Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/intel_sprite.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 64076555..7d9c340 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1264,10 +1264,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
struct drm_device *dev = plane->dev;
struct drm_crtc *crtc = state->crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+   struct intel_plane *intel_plane = to_intel_plane(plane);
enum pipe pipe = intel_crtc->pipe;
struct drm_framebuffer *fb = state->fb;
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-   struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+   struct drm_i915_gem_object *old_obj = intel_plane->obj;
int ret;
 
if (old_obj != obj) {
@@ -1302,7 +1303,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
enum pipe pipe = intel_crtc->pipe;
struct drm_framebuffer *fb = state->fb;
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-   struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+   struct drm_i915_gem_object *old_obj = intel_plane->obj;
int crtc_x, crtc_y;
unsigned int crtc_w, crtc_h;
uint32_t src_x, src_y, src_w, src_h;
-- 
2.1.1

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

Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane

2014-10-30 Thread Ville Syrjälä 
On Thu, Oct 30, 2014 at 04:02:01PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni 
> 
> Commit "drm/i915: create a prepare phase for sprite plane updates"
> changed the old_obj pointer we use when committing sprite planes,
> which caused a WARN() and a BUG() to be triggered. This patch should
> revert the code back to the previous behavior, fixing the regression.
> 
> Regression introduced by:
> commit ec82cb793c9224e0692eed904f43490cf70e8258
> Author: Gustavo Padovan 
> Date:   Fri Oct 24 14:51:32 2014 +0100
> drm/i915: create a prepare phase for sprite plane updates
> 
> Credits to Imre Deak for pointing out the exact lines that were wrong.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
> Testcase: igt/pm_rpm/legacy-planes
> Testcase: igt/pm_rpm/legacy-planes-dpms
> Testcase: igt/pm_rpm/universal-planes
> Testcase: igt/pm_rpm/universal-planes-dpms
> Credits-to: Imre Deak 
> Cc: Gustavo Padovan 
> Cc: Ville Syrjälä 
> Signed-off-by: Paulo Zanoni 
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 8b80d68..1c874309 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
>   struct drm_device *dev = plane->dev;
>   struct drm_crtc *crtc = state->crtc;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_plane *intel_plane = to_intel_plane(plane);
>   enum pipe pipe = intel_crtc->pipe;
>   struct drm_framebuffer *fb = state->fb;
>   struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> + struct drm_i915_gem_object *old_obj = intel_plane->obj;

You need to change intel_commit_sprite_plane() too. Othwerwise something
like this could happen:

1. .update_plane(fb=A)
   // note plane->fb=NULL here
   -> prepare() -> pin(A) -> obj=A
   -> commit() -> nop
   ...
   plane->fb=A

2. .crtc_disable()
   -> disable() -> unpin(A) -> obj=NULL

3. .update_plane(fb=B)
   // note plane->fb=A here still
   -> prepare() -> pin(B)
   -> commit() -> unpin(A) -> obj=B
   ...
   plane->fb=B

So we still get the double unpin of A.

I take it our basic plane tests didn't catch this?

>   int ret;
>  
>   if (old_obj != obj) {
> -- 
> 2.1.1

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane

2014-10-30 Thread Paulo Zanoni 
2014-10-30 17:10 GMT-02:00 Ville Syrjälä :
> On Thu, Oct 30, 2014 at 04:02:01PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni 
>>
>> Commit "drm/i915: create a prepare phase for sprite plane updates"
>> changed the old_obj pointer we use when committing sprite planes,
>> which caused a WARN() and a BUG() to be triggered. This patch should
>> revert the code back to the previous behavior, fixing the regression.
>>
>> Regression introduced by:
>> commit ec82cb793c9224e0692eed904f43490cf70e8258
>> Author: Gustavo Padovan 
>> Date:   Fri Oct 24 14:51:32 2014 +0100
>> drm/i915: create a prepare phase for sprite plane updates
>>
>> Credits to Imre Deak for pointing out the exact lines that were wrong.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
>> Testcase: igt/pm_rpm/legacy-planes
>> Testcase: igt/pm_rpm/legacy-planes-dpms
>> Testcase: igt/pm_rpm/universal-planes
>> Testcase: igt/pm_rpm/universal-planes-dpms
>> Credits-to: Imre Deak 
>> Cc: Gustavo Padovan 
>> Cc: Ville Syrjälä 
>> Signed-off-by: Paulo Zanoni 
>> ---
>>  drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
>> b/drivers/gpu/drm/i915/intel_sprite.c
>> index 8b80d68..1c874309 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
>>   struct drm_device *dev = plane->dev;
>>   struct drm_crtc *crtc = state->crtc;
>>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> + struct intel_plane *intel_plane = to_intel_plane(plane);
>>   enum pipe pipe = intel_crtc->pipe;
>>   struct drm_framebuffer *fb = state->fb;
>>   struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>> - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>> + struct drm_i915_gem_object *old_obj = intel_plane->obj;
>
> You need to change intel_commit_sprite_plane() too. Othwerwise something
> like this could happen:
>
> 1. .update_plane(fb=A)
>// note plane->fb=NULL here
>-> prepare() -> pin(A) -> obj=A
>-> commit() -> nop
>...
>plane->fb=A
>
> 2. .crtc_disable()
>-> disable() -> unpin(A) -> obj=NULL
>
> 3. .update_plane(fb=B)
>// note plane->fb=A here still
>-> prepare() -> pin(B)
>-> commit() -> unpin(A) -> obj=B
>...
>plane->fb=B
>
> So we still get the double unpin of A.
>
> I take it our basic plane tests didn't catch this?

I didn't run any, nor associated any of the recent bug reports with
that. Feel free to write tests if you can think of an easy way to
exploit the bug. I also won't mind if someone else writes/sends v2 of
this patch :)

>
>>   int ret;
>>
>>   if (old_obj != obj) {
>> --
>> 2.1.1
>
> --
> Ville Syrjälä
> Intel OTC



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

Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane

2014-11-10 Thread Ville Syrjälä 
On Mon, Nov 10, 2014 at 02:47:30PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni 
> 
> Commit "drm/i915: create a prepare phase for sprite plane updates"
> changed the old_obj pointer we use when committing sprite planes,
> which caused a WARN() and a BUG() to be triggered. Later, commit
> "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced
> the same problem to function intel_commit_sprite_plane().
> 
> Regression introduced by:
> commit ec82cb793c9224e0692eed904f43490cf70e8258
> Author: Gustavo Padovan 
> Date:   Fri Oct 24 14:51:32 2014 +0100
> drm/i915: create a prepare phase for sprite plane updates
> and:
> commit 77cde95217484e845743818691df026cec2534f4
> Author: Gustavo Padovan 
> Date:   Fri Oct 24 14:51:33 2014 +0100
> drm/i915: use intel_fb_obj() macros to assign gem objects
> 
> Credits to Imre Deak for pointing out the exact lines that were wrong.
> 
> v2: Also fix intel_commit_sprite_plane() (Ville)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
> Testcase: igt/pm_rpm/legacy-planes
> Testcase: igt/pm_rpm/legacy-planes-dpms
> Testcase: igt/pm_rpm/universal-planes
> Testcase: igt/pm_rpm/universal-planes-dpms
> Credits-to: Imre Deak 
> Cc: Gustavo Padovan 
> Cc: Ville Syrjälä 
> Signed-off-by: Paulo Zanoni 

Yep, I believe that should do it.

Reviewed-by: Ville Syrjälä 

As a side note if someone is looking for stuff to do, then the pin/unpin
logic might be good thing to look at. We're currently a bit inconsistent
whether we have the buffer pinned when the plane is disabled, or just
otherwise invisible, or when the crtc itself is disabled. And I guess
cooking up some tests to poke at planes with disabled crtcs might be in
order too, as well as all kinds of variations on the
crtc_enable->plane_enable->crtc_disable->plane_disable theme.

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 64076555..7d9c340 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1264,10 +1264,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
>   struct drm_device *dev = plane->dev;
>   struct drm_crtc *crtc = state->crtc;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_plane *intel_plane = to_intel_plane(plane);
>   enum pipe pipe = intel_crtc->pipe;
>   struct drm_framebuffer *fb = state->fb;
>   struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> + struct drm_i915_gem_object *old_obj = intel_plane->obj;
>   int ret;
>  
>   if (old_obj != obj) {
> @@ -1302,7 +1303,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   enum pipe pipe = intel_crtc->pipe;
>   struct drm_framebuffer *fb = state->fb;
>   struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> + struct drm_i915_gem_object *old_obj = intel_plane->obj;
>   int crtc_x, crtc_y;
>   unsigned int crtc_w, crtc_h;
>   uint32_t src_x, src_y, src_w, src_h;
> -- 
> 2.1.1

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane

2014-11-11 Thread Daniel Vetter 
On Mon, Nov 10, 2014 at 07:15:04PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 10, 2014 at 02:47:30PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni 
> > 
> > Commit "drm/i915: create a prepare phase for sprite plane updates"
> > changed the old_obj pointer we use when committing sprite planes,
> > which caused a WARN() and a BUG() to be triggered. Later, commit
> > "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced
> > the same problem to function intel_commit_sprite_plane().
> > 
> > Regression introduced by:
> > commit ec82cb793c9224e0692eed904f43490cf70e8258
> > Author: Gustavo Padovan 
> > Date:   Fri Oct 24 14:51:32 2014 +0100
> > drm/i915: create a prepare phase for sprite plane updates
> > and:
> > commit 77cde95217484e845743818691df026cec2534f4
> > Author: Gustavo Padovan 
> > Date:   Fri Oct 24 14:51:33 2014 +0100
> > drm/i915: use intel_fb_obj() macros to assign gem objects
> > 
> > Credits to Imre Deak for pointing out the exact lines that were wrong.
> > 
> > v2: Also fix intel_commit_sprite_plane() (Ville)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634
> > Testcase: igt/pm_rpm/legacy-planes
> > Testcase: igt/pm_rpm/legacy-planes-dpms
> > Testcase: igt/pm_rpm/universal-planes
> > Testcase: igt/pm_rpm/universal-planes-dpms
> > Credits-to: Imre Deak 
> > Cc: Gustavo Padovan 
> > Cc: Ville Syrjälä 
> > Signed-off-by: Paulo Zanoni 
> 
> Yep, I believe that should do it.
> 
> Reviewed-by: Ville Syrjälä 

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx