drm/i915: skl_update_scaler() wants a rotation bitmask instead of bit number

Commit Message

Ville Syrjala Oct. 15, 2015, 2:01 p.m. UTC

From: Ville Syrjälä <[e-mail]>

Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler().
The former is a mask, the latter just the bit number.

Fortunately the only thing skl_update_scaler() does with the rotation
is check if it's 90/270 degrees or not, and so in this case it would
still do the right thing.

Cc: Chandra Konduru <[e-mail]>
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ville Syrjala Jan. 15, 2016, 6:48 p.m. UTC | #1

On Thu, Oct 15, 2015 at 05:01:58PM +0300, [e-mail] wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler().
> The former is a mask, the latter just the bit number.
> 
> Fortunately the only thing skl_update_scaler() does with the rotation
> is check if it's 90/270 degrees or not, and so in this case it would
> still do the right thing.
> 
> Cc: Chandra Konduru <[e-mail]>
> Signed-off-by: Ville Syrjälä <[e-mail]>

Ping, anyone care to r-b this one?

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7498c9d..02316d0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4673,7 +4673,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
>  		      intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
>  
>  	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
> -		&state->scaler_state.scaler_id, DRM_ROTATE_0,
> +		&state->scaler_state.scaler_id, BIT(DRM_ROTATE_0),
>  		state->pipe_src_w, state->pipe_src_h,
>  		adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay);
>  }
> -- 
> 2.4.9

Matt Roper Jan. 15, 2016, 11:15 p.m. UTC | #2

On Fri, Jan 15, 2016 at 08:48:26PM +0200, Ville Syrjälä wrote:
> On Thu, Oct 15, 2015 at 05:01:58PM +0300, [e-mail] wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler().
> > The former is a mask, the latter just the bit number.
> > 
> > Fortunately the only thing skl_update_scaler() does with the rotation
> > is check if it's 90/270 degrees or not, and so in this case it would
> > still do the right thing.
> > 
> > Cc: Chandra Konduru <[e-mail]>
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> Ping, anyone care to r-b this one?

Reviewed-by: Matt Roper <[e-mail]>

Looks like this bug has been present since scalers were first added in
6156a45602f9 ("drm/i915: skylake primary plane scaling using shared scalers")


Matt

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7498c9d..02316d0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4673,7 +4673,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
> >  		      intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
> >  
> >  	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
> > -		&state->scaler_state.scaler_id, DRM_ROTATE_0,
> > +		&state->scaler_state.scaler_id, BIT(DRM_ROTATE_0),
> >  		state->pipe_src_w, state->pipe_src_h,
> >  		adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay);
> >  }
> > -- 
> > 2.4.9
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjala Jan. 18, 2016, 2:21 p.m. UTC | #3

On Fri, Jan 15, 2016 at 03:15:00PM -0800, Matt Roper wrote:
> On Fri, Jan 15, 2016 at 08:48:26PM +0200, Ville Syrjälä wrote:
> > On Thu, Oct 15, 2015 at 05:01:58PM +0300, [e-mail] wrote:
> > > From: Ville Syrjälä <[e-mail]>
> > > 
> > > Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler().
> > > The former is a mask, the latter just the bit number.
> > > 
> > > Fortunately the only thing skl_update_scaler() does with the rotation
> > > is check if it's 90/270 degrees or not, and so in this case it would
> > > still do the right thing.
> > > 
> > > Cc: Chandra Konduru <[e-mail]>
> > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > 
> > Ping, anyone care to r-b this one?
> 
> Reviewed-by: Matt Roper <[e-mail]>
> 
> Looks like this bug has been present since scalers were first added in
> 6156a45602f9 ("drm/i915: skylake primary plane scaling using shared scalers")

Pushed to dinq an appropriate Fixes: comment added. Thanks for the review.

> 
> 
> Matt
> 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 7498c9d..02316d0 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4673,7 +4673,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
> > >  		      intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
> > >  
> > >  	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
> > > -		&state->scaler_state.scaler_id, DRM_ROTATE_0,
> > > +		&state->scaler_state.scaler_id, BIT(DRM_ROTATE_0),
> > >  		state->pipe_src_w, state->pipe_src_h,
> > >  		adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay);
> > >  }
> > > -- 
> > > 2.4.9
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]

Daniel Vetter Jan. 19, 2016, 8:03 a.m. UTC | #4

On Mon, Jan 18, 2016 at 04:21:40PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 15, 2016 at 03:15:00PM -0800, Matt Roper wrote:
> > On Fri, Jan 15, 2016 at 08:48:26PM +0200, Ville Syrjälä wrote:
> > > On Thu, Oct 15, 2015 at 05:01:58PM +0300, [e-mail] wrote:
> > > > From: Ville Syrjälä <[e-mail]>
> > > > 
> > > > Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler().
> > > > The former is a mask, the latter just the bit number.
> > > > 
> > > > Fortunately the only thing skl_update_scaler() does with the rotation
> > > > is check if it's 90/270 degrees or not, and so in this case it would
> > > > still do the right thing.
> > > > 
> > > > Cc: Chandra Konduru <[e-mail]>
> > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > 
> > > Ping, anyone care to r-b this one?
> > 
> > Reviewed-by: Matt Roper <[e-mail]>
> > 
> > Looks like this bug has been present since scalers were first added in
> > 6156a45602f9 ("drm/i915: skylake primary plane scaling using shared scalers")
> 
> Pushed to dinq an appropriate Fixes: comment added. Thanks for the review.

Do we have an igt for this? If not need to capture it and make it
something we must fixe before more scaler stuff lands.
-Daniel

Ville Syrjala Jan. 19, 2016, 1:18 p.m. UTC | #5

On Tue, Jan 19, 2016 at 09:03:13AM +0100, Daniel Vetter wrote:
> On Mon, Jan 18, 2016 at 04:21:40PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 15, 2016 at 03:15:00PM -0800, Matt Roper wrote:
> > > On Fri, Jan 15, 2016 at 08:48:26PM +0200, Ville Syrjälä wrote:
> > > > On Thu, Oct 15, 2015 at 05:01:58PM +0300, [e-mail] wrote:
> > > > > From: Ville Syrjälä <[e-mail]>
> > > > > 
> > > > > Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler().
> > > > > The former is a mask, the latter just the bit number.
> > > > > 
> > > > > Fortunately the only thing skl_update_scaler() does with the rotation
> > > > > is check if it's 90/270 degrees or not, and so in this case it would
> > > > > still do the right thing.
> > > > > 
> > > > > Cc: Chandra Konduru <[e-mail]>
> > > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > > 
> > > > Ping, anyone care to r-b this one?
> > > 
> > > Reviewed-by: Matt Roper <[e-mail]>
> > > 
> > > Looks like this bug has been present since scalers were first added in
> > > 6156a45602f9 ("drm/i915: skylake primary plane scaling using shared scalers")
> > 
> > Pushed to dinq an appropriate Fixes: comment added. Thanks for the review.
> 
> Do we have an igt for this? If not need to capture it and make it
> something we must fixe before more scaler stuff lands.

There's no change in behavior from this fix, so there's nothing to test.

Daniel Vetter Jan. 19, 2016, 2:01 p.m. UTC | #6

On Tue, Jan 19, 2016 at 03:18:25PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 19, 2016 at 09:03:13AM +0100, Daniel Vetter wrote:
> > On Mon, Jan 18, 2016 at 04:21:40PM +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 15, 2016 at 03:15:00PM -0800, Matt Roper wrote:
> > > > On Fri, Jan 15, 2016 at 08:48:26PM +0200, Ville Syrjälä wrote:
> > > > > On Thu, Oct 15, 2015 at 05:01:58PM +0300, [e-mail] wrote:
> > > > > > From: Ville Syrjälä <[e-mail]>
> > > > > > 
> > > > > > Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler().
> > > > > > The former is a mask, the latter just the bit number.
> > > > > > 
> > > > > > Fortunately the only thing skl_update_scaler() does with the rotation
> > > > > > is check if it's 90/270 degrees or not, and so in this case it would
> > > > > > still do the right thing.
> > > > > > 
> > > > > > Cc: Chandra Konduru <[e-mail]>
> > > > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > > > 
> > > > > Ping, anyone care to r-b this one?
> > > > 
> > > > Reviewed-by: Matt Roper <[e-mail]>
> > > > 
> > > > Looks like this bug has been present since scalers were first added in
> > > > 6156a45602f9 ("drm/i915: skylake primary plane scaling using shared scalers")
> > > 
> > > Pushed to dinq an appropriate Fixes: comment added. Thanks for the review.
> > 
> > Do we have an igt for this? If not need to capture it and make it
> > something we must fixe before more scaler stuff lands.
> 
> There's no change in behavior from this fix, so there's nothing to test.

Ah, should have read things more carefully, I thought this was the fix for
the other recent rotation fail you've patched. _That_ on definitely should
come with an igt.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7498c9d..02316d0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4673,7 +4673,7 @@  int skl_update_scaler_crtc(struct intel_crtc_state *state)
 		      intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
 
 	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
-		&state->scaler_state.scaler_id, DRM_ROTATE_0,
+		&state->scaler_state.scaler_id, BIT(DRM_ROTATE_0),
 		state->pipe_src_w, state->pipe_src_h,
 		adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay);
 }