[v5,11/11] drm/i915: Calling rotate and inverse rotate transformations after clipping

Commit Message

[e-mail] Feb. 10, 2014, 7:31 a.m. UTC

From: Sagar Kamble <[e-mail]>

With clipped sprites these transformations are not working. these
functions transform complete sprite irrespective of clipping present.
This leads to invisible portion of sprite show up when rotate 180 if
it was out of visible area before.

v4: Moved rotate transform for source rectangle after clipping.
Added rotate and inverse rotate transform for destination rect.

Cc: Daniel Vetter <[e-mail]>
Cc: Jani Nikula <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Sagar Kamble <[e-mail]>
---
 drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Ville Syrjälä Feb. 10, 2014, 1:32 p.m. UTC | #1

On Mon, Feb 10, 2014 at 01:01:18PM +0530, [e-mail] wrote:
> From: Sagar Kamble <[e-mail]>
> 
> With clipped sprites these transformations are not working. these
> functions transform complete sprite irrespective of clipping present.
> This leads to invisible portion of sprite show up when rotate 180 if
> it was out of visible area before.
> 
> v4: Moved rotate transform for source rectangle after clipping.
> Added rotate and inverse rotate transform for destination rect.

Still NAK.

I just pushed rotation support to my glplane test app [1], and with
with that my rotated clipping code works exactly as intended.

[1] git://gitorious.org/vsyrjala/glplane.git

> 
> Cc: Daniel Vetter <[e-mail]>
> Cc: Jani Nikula <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Sagar Kamble <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 62b9f84..799f6a9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -769,9 +769,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	max_scale = intel_plane->max_downscale << 16;
>  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
>  
> -	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> -			intel_plane->rotation);
> -
>  	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
>  	BUG_ON(hscale < 0);
>  
> @@ -785,6 +782,13 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	crtc_w = drm_rect_width(&dst);
>  	crtc_h = drm_rect_height(&dst);
>  
> +	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> +				intel_plane->rotation);
> +
> +	drm_rect_rotate(&dst, intel_crtc->config.pipe_src_w,
> +				intel_crtc->config.pipe_src_h,
> +				intel_plane->rotation);
> +
>  	if (visible) {
>  		/* check again in case clipping clamped the results */
>  		hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
> @@ -811,7 +815,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  				     drm_rect_height(&dst) * vscale - drm_rect_height(&src));
>  
>  		drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16,
> -				    intel_plane->rotation);
> +					intel_plane->rotation);
> +
> +		drm_rect_rotate_inv(&dst, intel_crtc->config.pipe_src_w,
> +					intel_crtc->config.pipe_src_h,
> +					intel_plane->rotation);
>  
>  		/* sanity check to make sure the src viewport wasn't enlarged */
>  		WARN_ON(src.x1 < (int) src_x ||
> -- 
> 1.8.5
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[e-mail] Feb. 11, 2014, 11:32 a.m. UTC | #2

On Mon, 2014-02-10 at 15:32 +0200, Ville Syrjälä wrote:
> On Mon, Feb 10, 2014 at 01:01:18PM +0530, [e-mail] wrote:
> > From: Sagar Kamble <[e-mail]>
> > 
> > With clipped sprites these transformations are not working. these
> > functions transform complete sprite irrespective of clipping present.
> > This leads to invisible portion of sprite show up when rotate 180 if
> > it was out of visible area before.
> > 
> > v4: Moved rotate transform for source rectangle after clipping.
> > Added rotate and inverse rotate transform for destination rect.
> 
> Still NAK.
> 
> I just pushed rotation support to my glplane test app [1], and with
> with that my rotated clipping code works exactly as intended.
> 
> [1] git://gitorious.org/vsyrjala/glplane.git
I tried this app. I think I am considering 180 degree rotation of
clipped sprite plane differently. I have captured output with these
rotate transforms moved before(clip-rotated) and after(rotate-clipped)
clipping code.

Which is valid? Rotating entire sprite and then clipping or Rotating
clipped portion?

Reference and Rotated output is attached FYI.

If rotating entire sprite is correct then this patch 11/11 is not needed
and can be abandoned.

thanks,
Sagar
> > 
> > Cc: Daniel Vetter <[e-mail]>
> > Cc: Jani Nikula <[e-mail]>
> > Cc: David Airlie <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Signed-off-by: Sagar Kamble <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 62b9f84..799f6a9 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -769,9 +769,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	max_scale = intel_plane->max_downscale << 16;
> >  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> >  
> > -	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> > -			intel_plane->rotation);
> > -
> >  	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
> >  	BUG_ON(hscale < 0);
> >  
> > @@ -785,6 +782,13 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	crtc_w = drm_rect_width(&dst);
> >  	crtc_h = drm_rect_height(&dst);
> >  
> > +	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> > +				intel_plane->rotation);
> > +
> > +	drm_rect_rotate(&dst, intel_crtc->config.pipe_src_w,
> > +				intel_crtc->config.pipe_src_h,
> > +				intel_plane->rotation);
> > +
> >  	if (visible) {
> >  		/* check again in case clipping clamped the results */
> >  		hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
> > @@ -811,7 +815,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  				     drm_rect_height(&dst) * vscale - drm_rect_height(&src));
> >  
> >  		drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16,
> > -				    intel_plane->rotation);
> > +					intel_plane->rotation);
> > +
> > +		drm_rect_rotate_inv(&dst, intel_crtc->config.pipe_src_w,
> > +					intel_crtc->config.pipe_src_h,
> > +					intel_plane->rotation);
> >  
> >  		/* sanity check to make sure the src viewport wasn't enlarged */
> >  		WARN_ON(src.x1 < (int) src_x ||
> > -- 
> > 1.8.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Daniel Vetter Feb. 11, 2014, 11:59 a.m. UTC | #3

On Tue, Feb 11, 2014 at 05:02:31PM +0530, Sagar Arun Kamble wrote:
> On Mon, 2014-02-10 at 15:32 +0200, Ville Syrjälä wrote:
> > On Mon, Feb 10, 2014 at 01:01:18PM +0530, [e-mail] wrote:
> > > From: Sagar Kamble <[e-mail]>
> > > 
> > > With clipped sprites these transformations are not working. these
> > > functions transform complete sprite irrespective of clipping present.
> > > This leads to invisible portion of sprite show up when rotate 180 if
> > > it was out of visible area before.
> > > 
> > > v4: Moved rotate transform for source rectangle after clipping.
> > > Added rotate and inverse rotate transform for destination rect.
> > 
> > Still NAK.
> > 
> > I just pushed rotation support to my glplane test app [1], and with
> > with that my rotated clipping code works exactly as intended.
> > 
> > [1] git://gitorious.org/vsyrjala/glplane.git
> I tried this app. I think I am considering 180 degree rotation of
> clipped sprite plane differently. I have captured output with these
> rotate transforms moved before(clip-rotated) and after(rotate-clipped)
> clipping code.
> 
> Which is valid? Rotating entire sprite and then clipping or Rotating
> clipped portion?
> 
> Reference and Rotated output is attached FYI.
> 
> If rotating entire sprite is correct then this patch 11/11 is not needed
> and can be abandoned.

I think doing the clipping first (as a viewport) and then rotating the
viewport makes more sense to me. Dunno what other people think, but I
guess we should document that somewhere.
-Daniel

> 
> thanks,
> Sagar
> > > 
> > > Cc: Daniel Vetter <[e-mail]>
> > > Cc: Jani Nikula <[e-mail]>
> > > Cc: David Airlie <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Signed-off-by: Sagar Kamble <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++----
> > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 62b9f84..799f6a9 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -769,9 +769,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  	max_scale = intel_plane->max_downscale << 16;
> > >  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> > >  
> > > -	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> > > -			intel_plane->rotation);
> > > -
> > >  	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
> > >  	BUG_ON(hscale < 0);
> > >  
> > > @@ -785,6 +782,13 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  	crtc_w = drm_rect_width(&dst);
> > >  	crtc_h = drm_rect_height(&dst);
> > >  
> > > +	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> > > +				intel_plane->rotation);
> > > +
> > > +	drm_rect_rotate(&dst, intel_crtc->config.pipe_src_w,
> > > +				intel_crtc->config.pipe_src_h,
> > > +				intel_plane->rotation);
> > > +
> > >  	if (visible) {
> > >  		/* check again in case clipping clamped the results */
> > >  		hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
> > > @@ -811,7 +815,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  				     drm_rect_height(&dst) * vscale - drm_rect_height(&src));
> > >  
> > >  		drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16,
> > > -				    intel_plane->rotation);
> > > +					intel_plane->rotation);
> > > +
> > > +		drm_rect_rotate_inv(&dst, intel_crtc->config.pipe_src_w,
> > > +					intel_crtc->config.pipe_src_h,
> > > +					intel_plane->rotation);
> > >  
> > >  		/* sanity check to make sure the src viewport wasn't enlarged */
> > >  		WARN_ON(src.x1 < (int) src_x ||
> > > -- 
> > > 1.8.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
>

[e-mail] Feb. 11, 2014, 12:09 p.m. UTC | #4

On Tue, 2014-02-11 at 12:59 +0100, Daniel Vetter wrote:
> On Tue, Feb 11, 2014 at 05:02:31PM +0530, Sagar Arun Kamble wrote:
> > On Mon, 2014-02-10 at 15:32 +0200, Ville Syrjälä wrote:
> > > On Mon, Feb 10, 2014 at 01:01:18PM +0530, [e-mail] wrote:
> > > > From: Sagar Kamble <[e-mail]>
> > > > 
> > > > With clipped sprites these transformations are not working. these
> > > > functions transform complete sprite irrespective of clipping present.
> > > > This leads to invisible portion of sprite show up when rotate 180 if
> > > > it was out of visible area before.
> > > > 
> > > > v4: Moved rotate transform for source rectangle after clipping.
> > > > Added rotate and inverse rotate transform for destination rect.
> > > 
> > > Still NAK.
> > > 
> > > I just pushed rotation support to my glplane test app [1], and with
> > > with that my rotated clipping code works exactly as intended.
> > > 
> > > [1] git://gitorious.org/vsyrjala/glplane.git
> > I tried this app. I think I am considering 180 degree rotation of
> > clipped sprite plane differently. I have captured output with these
> > rotate transforms moved before(clip-rotated) and after(rotate-clipped)
> > clipping code.
> > 
> > Which is valid? Rotating entire sprite and then clipping or Rotating
> > clipped portion?
> > 
> > Reference and Rotated output is attached FYI.
> > 
> > If rotating entire sprite is correct then this patch 11/11 is not needed
> > and can be abandoned.
> 
> I think doing the clipping first (as a viewport) and then rotating the
> viewport makes more sense to me. Dunno what other people think, but I
> guess we should document that somewhere.
> -Daniel

Agree. Thought of use-case where if media player(that will use sprite
for video) is moved such that it is off the screen partially, after
rotation as well we should see same portion of clip inside(rotated
though).

> 
> > 
> > thanks,
> > Sagar
> > > > 
> > > > Cc: Daniel Vetter <[e-mail]>
> > > > Cc: Jani Nikula <[e-mail]>
> > > > Cc: David Airlie <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Signed-off-by: Sagar Kamble <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++----
> > > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index 62b9f84..799f6a9 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -769,9 +769,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > >  	max_scale = intel_plane->max_downscale << 16;
> > > >  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> > > >  
> > > > -	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> > > > -			intel_plane->rotation);
> > > > -
> > > >  	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
> > > >  	BUG_ON(hscale < 0);
> > > >  
> > > > @@ -785,6 +782,13 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > >  	crtc_w = drm_rect_width(&dst);
> > > >  	crtc_h = drm_rect_height(&dst);
> > > >  
> > > > +	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> > > > +				intel_plane->rotation);
> > > > +
> > > > +	drm_rect_rotate(&dst, intel_crtc->config.pipe_src_w,
> > > > +				intel_crtc->config.pipe_src_h,
> > > > +				intel_plane->rotation);
> > > > +
> > > >  	if (visible) {
> > > >  		/* check again in case clipping clamped the results */
> > > >  		hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
> > > > @@ -811,7 +815,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > >  				     drm_rect_height(&dst) * vscale - drm_rect_height(&src));
> > > >  
> > > >  		drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16,
> > > > -				    intel_plane->rotation);
> > > > +					intel_plane->rotation);
> > > > +
> > > > +		drm_rect_rotate_inv(&dst, intel_crtc->config.pipe_src_w,
> > > > +					intel_crtc->config.pipe_src_h,
> > > > +					intel_plane->rotation);
> > > >  
> > > >  		/* sanity check to make sure the src viewport wasn't enlarged */
> > > >  		WARN_ON(src.x1 < (int) src_x ||
> > > > -- 
> > > > 1.8.5
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > [e-mail]
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > 
> 
> 
> 
> 
> 
>

Ville Syrjälä Feb. 11, 2014, 2:56 p.m. UTC | #5

On Tue, Feb 11, 2014 at 05:02:31PM +0530, Sagar Arun Kamble wrote:
> On Mon, 2014-02-10 at 15:32 +0200, Ville Syrjälä wrote:
> > On Mon, Feb 10, 2014 at 01:01:18PM +0530, [e-mail] wrote:
> > > From: Sagar Kamble <[e-mail]>
> > > 
> > > With clipped sprites these transformations are not working. these
> > > functions transform complete sprite irrespective of clipping present.
> > > This leads to invisible portion of sprite show up when rotate 180 if
> > > it was out of visible area before.
> > > 
> > > v4: Moved rotate transform for source rectangle after clipping.
> > > Added rotate and inverse rotate transform for destination rect.
> > 
> > Still NAK.
> > 
> > I just pushed rotation support to my glplane test app [1], and with
> > with that my rotated clipping code works exactly as intended.
> > 
> > [1] git://gitorious.org/vsyrjala/glplane.git
> I tried this app. I think I am considering 180 degree rotation of
> clipped sprite plane differently. I have captured output with these
> rotate transforms moved before(clip-rotated) and after(rotate-clipped)
> clipping code.
> 
> Which is valid? Rotating entire sprite and then clipping or Rotating
> clipped portion?
> 
> Reference and Rotated output is attached FYI.
> 
> If rotating entire sprite is correct then this patch 11/11 is not needed
> and can be abandoned.

The way I think of these things is roughly this:

You have the user specified source rectangle, where the coordinates specify
the viewport into the framebuffer. This coordinate space is oriented the
same was as the framebuffer itself, ie. the first pixel of the
framebuffer is at coordinates 0,0. So plane rotation doesn't affect this
at all.

Then you have the user specified destination/crtc rectangle, where the
coordinates specify the position of the plane within the crtc coordinate
space. So the first visible pixel the pipe will push out is at
coordinates 0,0. So again plane rotation doesn't affect this.

Then you have the rotation which simply specifies the transformation to
be applied to the pixels when they "move" from the source rectangle to
the destination rectangle. So w/ 0 degree rotation the pixel at
src_x,src_y in the framebuffer will appear at position crtc_x,crtc_y
on the crtc output. With 180 degree rotation the pixel at src_x,src_y
will appear at crtc_x+crtc_w-1,crtc_y+crtc_h-1.

As clipping happens in the crtc coordinate space, we need to orient
the source coordindates the same way to get the correct clipping result.
So for example with 0 degrees rotation clipping the left side of the
destination rectangle must result in clipping the left side of the source
rectangle as well. And with 180 degree rotation clipping the destination
rectangle on the left side must result in clipping the source rectangle
on the right side. Left and right in each case referring to the original
unrotate coordinates.

So let's say we have the following situation w/ 180 degree rotation.
The letters inside the rects represented specific named pixels,
the FB rectangle represents the FB as specified by addfb2 ioctl,
the CRTC rectangle represents the pipe output (0,0 -> PIPESRC.w,h):

FB:                 CRTC:
0,0 ___________     0.0 __________
    |  abcd   |         |         |
    |  efgh   |         |         |
    |_________|         |hgfe     |
                        |dcba_____|
unclipped coordinates specified by user:
src_x=2,src_y=0     crtc_x=0,crtc_y=2
src_w=4.src_h=2     crtc_w=4,crtc_h=2

clipped coordinates:
src_x=2,src_y=0     crtc_x=0,crtc_y=2
src_w=4.src_h=2     crtc_w=4,crtc_h=2


Then the user moves the sprite one pixel to the left resulting on some
clipping (the X pixels). Note that the unclipped source coordinates do
not change here at all, in fact crtc_x is the only thing changed by the
user:

FB:                 CRTC:
0,0 ___________     0.0 __________
    |  abcX   |         |         |
    |  efgX   |         |         |
    |_________|         Xgfe      |
                        Xcba______|
unclipped coordinates specified by user:
src_x=2,src_y=0     crtc_x=-1,crtc_y=2
src_w=4.src_h=2     crtc_w= 4,crtc_h=2

clipped coordinates:
src_x=2,src_y=0     crtc_x=0,crtc_y=2
src_w=3.src_h=2     crtc_w=3,crtc_h=2

[e-mail] Feb. 11, 2014, 5:36 p.m. UTC | #6

On Tue, 2014-02-11 at 16:56 +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2014 at 05:02:31PM +0530, Sagar Arun Kamble wrote:
> > On Mon, 2014-02-10 at 15:32 +0200, Ville Syrjälä wrote:
> > > On Mon, Feb 10, 2014 at 01:01:18PM +0530, [e-mail] wrote:
> > > > From: Sagar Kamble <[e-mail]>
> > > > 
> > > > With clipped sprites these transformations are not working. these
> > > > functions transform complete sprite irrespective of clipping present.
> > > > This leads to invisible portion of sprite show up when rotate 180 if
> > > > it was out of visible area before.
> > > > 
> > > > v4: Moved rotate transform for source rectangle after clipping.
> > > > Added rotate and inverse rotate transform for destination rect.
> > > 
> > > Still NAK.
> > > 
> > > I just pushed rotation support to my glplane test app [1], and with
> > > with that my rotated clipping code works exactly as intended.
> > > 
> > > [1] git://gitorious.org/vsyrjala/glplane.git
> > I tried this app. I think I am considering 180 degree rotation of
> > clipped sprite plane differently. I have captured output with these
> > rotate transforms moved before(clip-rotated) and after(rotate-clipped)
> > clipping code.
> > 
> > Which is valid? Rotating entire sprite and then clipping or Rotating
> > clipped portion?
> > 
> > Reference and Rotated output is attached FYI.
> > 
> > If rotating entire sprite is correct then this patch 11/11 is not needed
> > and can be abandoned.
> 
> The way I think of these things is roughly this:
> 
> You have the user specified source rectangle, where the coordinates specify
> the viewport into the framebuffer. This coordinate space is oriented the
> same was as the framebuffer itself, ie. the first pixel of the
> framebuffer is at coordinates 0,0. So plane rotation doesn't affect this
> at all.
> 
> Then you have the user specified destination/crtc rectangle, where the
> coordinates specify the position of the plane within the crtc coordinate
> space. So the first visible pixel the pipe will push out is at
> coordinates 0,0. So again plane rotation doesn't affect this.
> 
> Then you have the rotation which simply specifies the transformation to
> be applied to the pixels when they "move" from the source rectangle to
> the destination rectangle. So w/ 0 degree rotation the pixel at
> src_x,src_y in the framebuffer will appear at position crtc_x,crtc_y
> on the crtc output. With 180 degree rotation the pixel at src_x,src_y
> will appear at crtc_x+crtc_w-1,crtc_y+crtc_h-1.
> 
> As clipping happens in the crtc coordinate space, we need to orient
> the source coordindates the same way to get the correct clipping result.
> So for example with 0 degrees rotation clipping the left side of the
> destination rectangle must result in clipping the left side of the source
> rectangle as well. And with 180 degree rotation clipping the destination
> rectangle on the left side must result in clipping the source rectangle
> on the right side. Left and right in each case referring to the original
> unrotate coordinates.
> 
> So let's say we have the following situation w/ 180 degree rotation.
> The letters inside the rects represented specific named pixels,
> the FB rectangle represents the FB as specified by addfb2 ioctl,
> the CRTC rectangle represents the pipe output (0,0 -> PIPESRC.w,h):
> 
> FB:                 CRTC:
> 0,0 ___________     0.0 __________
>     |  abcd   |         |         |
>     |  efgh   |         |         |
>     |_________|         |hgfe     |
>                         |dcba_____|
> unclipped coordinates specified by user:
> src_x=2,src_y=0     crtc_x=0,crtc_y=2
> src_w=4.src_h=2     crtc_w=4,crtc_h=2
> 
> clipped coordinates:
> src_x=2,src_y=0     crtc_x=0,crtc_y=2
> src_w=4.src_h=2     crtc_w=4,crtc_h=2
> 
> 
> Then the user moves the sprite one pixel to the left resulting on some
> clipping (the X pixels). Note that the unclipped source coordinates do
> not change here at all, in fact crtc_x is the only thing changed by the
> user:
> 
> FB:                 CRTC:
> 0,0 ___________     0.0 __________
>     |  abcX   |         |         |
>     |  efgX   |         |         |
>     |_________|         Xgfe      |
>                         Xcba______|
> unclipped coordinates specified by user:
> src_x=2,src_y=0     crtc_x=-1,crtc_y=2
> src_w=4.src_h=2     crtc_w= 4,crtc_h=2
> 
> clipped coordinates:
> src_x=2,src_y=0     crtc_x=0,crtc_y=2
> src_w=3.src_h=2     crtc_w=3,crtc_h=2
> 


Understood this now. Thank you Ville for providing this elaborate
graphical view of the rotation cases.

-Sagar

Patch

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 62b9f84..799f6a9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -769,9 +769,6 @@  intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	max_scale = intel_plane->max_downscale << 16;
 	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
 
-	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
-			intel_plane->rotation);
-
 	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
 	BUG_ON(hscale < 0);
 
@@ -785,6 +782,13 @@  intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	crtc_w = drm_rect_width(&dst);
 	crtc_h = drm_rect_height(&dst);
 
+	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
+				intel_plane->rotation);
+
+	drm_rect_rotate(&dst, intel_crtc->config.pipe_src_w,
+				intel_crtc->config.pipe_src_h,
+				intel_plane->rotation);
+
 	if (visible) {
 		/* check again in case clipping clamped the results */
 		hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
@@ -811,7 +815,11 @@  intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 				     drm_rect_height(&dst) * vscale - drm_rect_height(&src));
 
 		drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16,
-				    intel_plane->rotation);
+					intel_plane->rotation);
+
+		drm_rect_rotate_inv(&dst, intel_crtc->config.pipe_src_w,
+					intel_crtc->config.pipe_src_h,
+					intel_plane->rotation);
 
 		/* sanity check to make sure the src viewport wasn't enlarged */
 		WARN_ON(src.x1 < (int) src_x ||