[08/24] drm/i915: Do not add planes from intel_atomic_setup_scalers.

Submitted by Maarten Lankhorst on June 1, 2015, 1:27 p.m.

Details

Message ID 1433165247-15928-9-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Maarten Lankhorst June 1, 2015, 1:27 p.m.
This may postpone going to HQ mode until the plane is in the
drm_atomic_state if it's not using scaler 0, but it does allow moving
intel_atomic_setup_scalers to the crtc check function.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 3 files changed, 39 insertions(+), 31 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 1edd1651c045..a8202fa0daa8 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -100,14 +100,6 @@  int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	/*
-	 * FIXME: move to crtc atomic check function once this is
-	 * more atomic friendly.
-	 */
-	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
-	if (ret)
-		return ret;
-
 	return ret;
 }
 
@@ -336,21 +328,10 @@  int intel_atomic_setup_scalers(struct drm_device *dev,
 			/* find the plane that set the bit as scaler_user */
 			plane = drm_state->planes[i];
 
-			/*
-			 * to enable/disable hq mode, add planes that are using scaler
-			 * into this transaction
-			 */
 			if (!plane) {
-				struct drm_plane_state *state;
-				plane = drm_plane_from_index(dev, i);
-				state = drm_atomic_get_plane_state(drm_state, plane);
-				if (IS_ERR(state)) {
-					DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
-						plane->base.id);
-					return PTR_ERR(state);
-				}
+				DRM_DEBUG_KMS("Failed to find [PLANE:%d] in drm_state\n", plane->base.id);
+				continue;
 			}
-
 			intel_plane = to_intel_plane(plane);
 
 			/* plane on different crtc cannot be a scaler user of this crtc */
@@ -396,6 +377,24 @@  int intel_atomic_setup_scalers(struct drm_device *dev,
 		}
 	}
 
+	/* plane not part of mask must leave hq mode? */
+	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
+	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
+		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
+
+		intel_crtc->atomic.skl_update_scaler0 =
+			PS_SCALER_EN | PS_SCALER_MODE_DYN;
+	}
+
+	/* plane not part of mask can enter hq mode? */
+	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
+	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode != PS_SCALER_MODE_HQ) {
+		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
+
+		intel_crtc->atomic.skl_update_scaler0 =
+			PS_SCALER_EN | PS_SCALER_MODE_HQ;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0060784525dc..94101d1f784c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6530,7 +6530,6 @@  static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
-	int ret;
 
 	/* FIXME should check pixel clock limits on all platforms */
 	if (INTEL_INFO(dev)->gen < 4) {
@@ -6577,14 +6576,7 @@  static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
 
-	/* FIXME: remove below call once atomic mode set is place and all crtc
-	 * related checks called from atomic_crtc_check function */
-	ret = 0;
-	DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config->base.state) = %p\n",
-		crtc, pipe_config->base.state);
-	ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
-
-	return ret;
+	return 0;
 }
 
 static int skylake_get_display_clock_speed(struct drm_device *dev)
@@ -11457,7 +11449,10 @@  static bool check_encoder_cloning(struct drm_atomic_state *state,
 static int intel_atomic_check_crtc(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {
+	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc_state);
 	struct drm_atomic_state *state = crtc_state->state;
 	int idx = crtc->base.id;
 	bool is_crtc_enabled = crtc_state->active;
@@ -11476,7 +11471,7 @@  static int intel_atomic_check_crtc(struct drm_crtc *crtc,
 	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
 			 idx, was_crtc_enabled, is_crtc_enabled);
 
-	return 0;
+	return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
 }
 
 static const struct drm_crtc_helper_funcs intel_helper_funcs = {
@@ -13423,6 +13418,18 @@  static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
+
+	if (intel_crtc->atomic.skl_update_scaler0) {
+		uint32_t ps_ctrl, ps_win_sz;
+
+		ps_ctrl = I915_READ(SKL_PS_CTRL(intel_crtc->pipe, 0));
+		ps_ctrl &= ~PS_SCALER_MODE_MASK;
+		ps_ctrl |= intel_crtc->atomic.skl_update_scaler0 & PS_SCALER_MODE_MASK;
+		I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, 0), ps_ctrl);
+
+		ps_win_sz = I915_READ(SKL_PS_WIN_SZ(intel_crtc->pipe, 0));
+		I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, 0), ps_win_sz);
+	}
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1d9feb8727ea..71ad8a1e13ab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -487,6 +487,8 @@  struct intel_crtc_atomic_commit {
 	bool update_wm;
 	unsigned disabled_planes;
 
+	unsigned skl_update_scaler0;
+
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
 	bool wait_vblank;

Comments

On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> This may postpone going to HQ mode until the plane is in the
> drm_atomic_state if it's not using scaler 0, but it does allow moving
> intel_atomic_setup_scalers to the crtc check function.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  3 files changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 1edd1651c045..a8202fa0daa8 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * FIXME: move to crtc atomic check function once this is
> -	 * more atomic friendly.
> -	 */
> -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> -	if (ret)
> -		return ret;
> -
>  	return ret;
>  }
>  
> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>  			/* find the plane that set the bit as scaler_user */
>  			plane = drm_state->planes[i];
>  
> -			/*
> -			 * to enable/disable hq mode, add planes that are using scaler
> -			 * into this transaction
> -			 */
>  			if (!plane) {
> -				struct drm_plane_state *state;
> -				plane = drm_plane_from_index(dev, i);
> -				state = drm_atomic_get_plane_state(drm_state, plane);
> -				if (IS_ERR(state)) {
> -					DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
> -						plane->base.id);
> -					return PTR_ERR(state);
> -				}
> +				DRM_DEBUG_KMS("Failed to find [PLANE:%d] in drm_state\n", plane->base.id);
> +				continue;
>  			}
> -
>  			intel_plane = to_intel_plane(plane);
>  
>  			/* plane on different crtc cannot be a scaler user of this crtc */
> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>  		}
>  	}
>  
> +	/* plane not part of mask must leave hq mode? */
> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
> +
> +		intel_crtc->atomic.skl_update_scaler0 =
> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> +	}
> +
> +	/* plane not part of mask can enter hq mode? */
> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode != PS_SCALER_MODE_HQ) {
> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> +
> +		intel_crtc->atomic.skl_update_scaler0 =
> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> +	}
> +

I don't have access to the hw spec at the moment; is scaler #0 the only
one that can ever go into HQ mode?  If there isn't a hardware
requirement about this, then it seems like we're missing the case where
planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
disabled, which should allow scaler 1 to go into HQ mode.

I guess it's not immediately clear to me why we need to not pull the
other planes into the transaction.  Is this just to avoid doing some
extra work for a plane that hasn't changed, or does it cause a problem
somehow?

Chandra should probably take a look at this patch as well since he's the
scaler expert; adding him to the Cc.


Matt

>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0060784525dc..94101d1f784c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6530,7 +6530,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> -	int ret;
>  
>  	/* FIXME should check pixel clock limits on all platforms */
>  	if (INTEL_INFO(dev)->gen < 4) {
> @@ -6577,14 +6576,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  	if (pipe_config->has_pch_encoder)
>  		return ironlake_fdi_compute_config(crtc, pipe_config);
>  
> -	/* FIXME: remove below call once atomic mode set is place and all crtc
> -	 * related checks called from atomic_crtc_check function */
> -	ret = 0;
> -	DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config->base.state) = %p\n",
> -		crtc, pipe_config->base.state);
> -	ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static int skylake_get_display_clock_speed(struct drm_device *dev)
> @@ -11457,7 +11449,10 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
>  static int intel_atomic_check_crtc(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *crtc_state)
>  {
> +	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc_state);
>  	struct drm_atomic_state *state = crtc_state->state;
>  	int idx = crtc->base.id;
>  	bool is_crtc_enabled = crtc_state->active;
> @@ -11476,7 +11471,7 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
>  	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
>  			 idx, was_crtc_enabled, is_crtc_enabled);
>  
> -	return 0;
> +	return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
>  }
>  
>  static const struct drm_crtc_helper_funcs intel_helper_funcs = {
> @@ -13423,6 +13418,18 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
> +
> +	if (intel_crtc->atomic.skl_update_scaler0) {
> +		uint32_t ps_ctrl, ps_win_sz;
> +
> +		ps_ctrl = I915_READ(SKL_PS_CTRL(intel_crtc->pipe, 0));
> +		ps_ctrl &= ~PS_SCALER_MODE_MASK;
> +		ps_ctrl |= intel_crtc->atomic.skl_update_scaler0 & PS_SCALER_MODE_MASK;
> +		I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, 0), ps_ctrl);
> +
> +		ps_win_sz = I915_READ(SKL_PS_WIN_SZ(intel_crtc->pipe, 0));
> +		I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, 0), ps_win_sz);
> +	}
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1d9feb8727ea..71ad8a1e13ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -487,6 +487,8 @@ struct intel_crtc_atomic_commit {
>  	bool update_wm;
>  	unsigned disabled_planes;
>  
> +	unsigned skl_update_scaler0;
> +
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
>  	bool wait_vblank;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, June 02, 2015 6:30 PM
> To: Maarten Lankhorst
> Cc: intel-gfx@lists.freedesktop.org; Konduru, Chandra
> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> intel_atomic_setup_scalers.
> 
> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> > This may postpone going to HQ mode until the plane is in the
> > drm_atomic_state if it's not using scaler 0, but it does allow moving
> > intel_atomic_setup_scalers to the crtc check function.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++----------------
> --
> >  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >  3 files changed, 39 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> > index 1edd1651c045..a8202fa0daa8 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		return ret;
> >
> > -	/*
> > -	 * FIXME: move to crtc atomic check function once this is
> > -	 * more atomic friendly.
> > -	 */
> > -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> > -	if (ret)
> > -		return ret;
> > -
> >  	return ret;
> >  }
> >
> > @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
> *dev,
> >  			/* find the plane that set the bit as scaler_user */
> >  			plane = drm_state->planes[i];
> >
> > -			/*
> > -			 * to enable/disable hq mode, add planes that are using
> scaler
> > -			 * into this transaction
> > -			 */
> >  			if (!plane) {
> > -				struct drm_plane_state *state;
> > -				plane = drm_plane_from_index(dev, i);
> > -				state =
> drm_atomic_get_plane_state(drm_state, plane);
> > -				if (IS_ERR(state)) {
> > -					DRM_DEBUG_KMS("Failed to add
> [PLANE:%d] to drm_state\n",
> > -						plane->base.id);
> > -					return PTR_ERR(state);
> > -				}
> > +				DRM_DEBUG_KMS("Failed to find [PLANE:%d]
> in drm_state\n", plane->base.id);
> > +				continue;
> >  			}
> > -
> >  			intel_plane = to_intel_plane(plane);
> >
> >  			/* plane on different crtc cannot be a scaler user of this
> crtc */
> > @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device
> *dev,
> >  		}
> >  	}
> >
> > +	/* plane not part of mask must leave hq mode? */
> > +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> > +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> > +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
> > +
> > +		intel_crtc->atomic.skl_update_scaler0 =
> > +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> > +	}
> > +
> > +	/* plane not part of mask can enter hq mode? */
> > +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
> > +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode !=
> PS_SCALER_MODE_HQ) {
> > +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> > +
> > +		intel_crtc->atomic.skl_update_scaler0 =
> > +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> > +	}
> > +
> 
> I don't have access to the hw spec at the moment; is scaler #0 the only
> one that can ever go into HQ mode?  

Yes

> If there isn't a hardware
> requirement about this, then it seems like we're missing the case where
> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> disabled, which should allow scaler 1 to go into HQ mode.

In this case, scaler 0 to be allocated to plane B to operate in HQ mode.

> 
> I guess it's not immediately clear to me why we need to not pull the
> other planes into the transaction.  Is this just to avoid doing some
> extra work for a plane that hasn't changed, or does it cause a problem
> somehow?

Per atomic design, unchanged planes can be added to transaction.
And scaler implementation is using this design feature.
Not sure what the issue here, but we need this feature continue
to available.

> 
> Chandra should probably take a look at this patch as well since he's the
> scaler expert; adding him to the Cc.
> 
> 
> Matt
> 
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> > index 0060784525dc..94101d1f784c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6530,7 +6530,6 @@ static int intel_crtc_compute_config(struct
> intel_crtc *crtc,
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_display_mode *adjusted_mode = &pipe_config-
> >base.adjusted_mode;
> > -	int ret;
> >
> >  	/* FIXME should check pixel clock limits on all platforms */
> >  	if (INTEL_INFO(dev)->gen < 4) {
> > @@ -6577,14 +6576,7 @@ static int intel_crtc_compute_config(struct
> intel_crtc *crtc,
> >  	if (pipe_config->has_pch_encoder)
> >  		return ironlake_fdi_compute_config(crtc, pipe_config);
> >
> > -	/* FIXME: remove below call once atomic mode set is place and all crtc
> > -	 * related checks called from atomic_crtc_check function */
> > -	ret = 0;
> > -	DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config-
> >base.state) = %p\n",
> > -		crtc, pipe_config->base.state);
> > -	ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
> > -
> > -	return ret;
> > +	return 0;
> >  }
> >
> >  static int skylake_get_display_clock_speed(struct drm_device *dev)
> > @@ -11457,7 +11449,10 @@ static bool check_encoder_cloning(struct
> drm_atomic_state *state,
> >  static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> >  				   struct drm_crtc_state *crtc_state)
> >  {
> > +	struct drm_device *dev = crtc->dev;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_crtc_state *pipe_config =
> > +		to_intel_crtc_state(crtc_state);
> >  	struct drm_atomic_state *state = crtc_state->state;
> >  	int idx = crtc->base.id;
> >  	bool is_crtc_enabled = crtc_state->active;
> > @@ -11476,7 +11471,7 @@ static int intel_atomic_check_crtc(struct
> drm_crtc *crtc,
> >  	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
> >  			 idx, was_crtc_enabled, is_crtc_enabled);
> >
> > -	return 0;
> > +	return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
> >  }
> >
> >  static const struct drm_crtc_helper_funcs intel_helper_funcs = {
> > @@ -13423,6 +13418,18 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc)
> >  		intel_crtc->atomic.evade =
> >  			intel_pipe_update_start(intel_crtc,
> >  						&intel_crtc-
> >atomic.start_vbl_count);
> > +
> > +	if (intel_crtc->atomic.skl_update_scaler0) {
> > +		uint32_t ps_ctrl, ps_win_sz;
> > +
> > +		ps_ctrl = I915_READ(SKL_PS_CTRL(intel_crtc->pipe, 0));
> > +		ps_ctrl &= ~PS_SCALER_MODE_MASK;
> > +		ps_ctrl |= intel_crtc->atomic.skl_update_scaler0 &
> PS_SCALER_MODE_MASK;
> > +		I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, 0), ps_ctrl);
> > +
> > +		ps_win_sz = I915_READ(SKL_PS_WIN_SZ(intel_crtc->pipe, 0));
> > +		I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, 0), ps_win_sz);
> > +	}
> >  }
> >
> >  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> > index 1d9feb8727ea..71ad8a1e13ab 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -487,6 +487,8 @@ struct intel_crtc_atomic_commit {
> >  	bool update_wm;
> >  	unsigned disabled_planes;
> >
> > +	unsigned skl_update_scaler0;
> > +
> >  	/* Sleepable operations to perform after commit */
> >  	unsigned fb_bits;
> >  	bool wait_vblank;
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
Op 03-06-15 om 03:52 schreef Konduru, Chandra:
>
>> -----Original Message-----
>> From: Roper, Matthew D
>> Sent: Tuesday, June 02, 2015 6:30 PM
>> To: Maarten Lankhorst
>> Cc: intel-gfx@lists.freedesktop.org; Konduru, Chandra
>> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
>> intel_atomic_setup_scalers.
>>
>> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
>>> This may postpone going to HQ mode until the plane is in the
>>> drm_atomic_state if it's not using scaler 0, but it does allow moving
>>> intel_atomic_setup_scalers to the crtc check function.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++----------------
>> --
>>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>>>  3 files changed, 39 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>>> index 1edd1651c045..a8202fa0daa8 100644
>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
>>>  	if (ret)
>>>  		return ret;
>>>
>>> -	/*
>>> -	 * FIXME: move to crtc atomic check function once this is
>>> -	 * more atomic friendly.
>>> -	 */
>>> -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>>  	return ret;
>>>  }
>>>
>>> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
>> *dev,
>>>  			/* find the plane that set the bit as scaler_user */
>>>  			plane = drm_state->planes[i];
>>>
>>> -			/*
>>> -			 * to enable/disable hq mode, add planes that are using
>> scaler
>>> -			 * into this transaction
>>> -			 */
>>>  			if (!plane) {
>>> -				struct drm_plane_state *state;
>>> -				plane = drm_plane_from_index(dev, i);
>>> -				state =
>> drm_atomic_get_plane_state(drm_state, plane);
>>> -				if (IS_ERR(state)) {
>>> -					DRM_DEBUG_KMS("Failed to add
>> [PLANE:%d] to drm_state\n",
>>> -						plane->base.id);
>>> -					return PTR_ERR(state);
>>> -				}
>>> +				DRM_DEBUG_KMS("Failed to find [PLANE:%d]
>> in drm_state\n", plane->base.id);
>>> +				continue;
>>>  			}
>>> -
>>>  			intel_plane = to_intel_plane(plane);
>>>
>>>  			/* plane on different crtc cannot be a scaler user of this
>> crtc */
>>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device
>> *dev,
>>>  		}
>>>  	}
>>>
>>> +	/* plane not part of mask must leave hq mode? */
>>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
>>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
>>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
>>> +
>>> +		intel_crtc->atomic.skl_update_scaler0 =
>>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
>>> +	}
>>> +
>>> +	/* plane not part of mask can enter hq mode? */
>>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
>>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode !=
>> PS_SCALER_MODE_HQ) {
>>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
>>> +
>>> +		intel_crtc->atomic.skl_update_scaler0 =
>>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
>>> +	}
>>> +
>> I don't have access to the hw spec at the moment; is scaler #0 the only
>> one that can ever go into HQ mode?  
> Yes
>
>> If there isn't a hardware
>> requirement about this, then it seems like we're missing the case where
>> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
>> disabled, which should allow scaler 1 to go into HQ mode.
> In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
Is it really bad to keep it on scaler 1 for a while until the next time the plane is added?

>> I guess it's not immediately clear to me why we need to not pull the
>> other planes into the transaction.  Is this just to avoid doing some
>> extra work for a plane that hasn't changed, or does it cause a problem
>> somehow?
> Per atomic design, unchanged planes can be added to transaction.
> And scaler implementation is using this design feature.
> Not sure what the issue here, but we need this feature continue
> to available.
>
Unchanged planes can be added, but this could pull in a primary plane, which would need
to set atomic.wait_for_flips then. I can do that as special case when adding a plane if
that's preferred.

~Maarten
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Wednesday, June 03, 2015 12:02 AM
> To: Konduru, Chandra; Roper, Matthew D
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> intel_atomic_setup_scalers.
> 
> Op 03-06-15 om 03:52 schreef Konduru, Chandra:
> >
> >> -----Original Message-----
> >> From: Roper, Matthew D
> >> Sent: Tuesday, June 02, 2015 6:30 PM
> >> To: Maarten Lankhorst
> >> Cc: intel-gfx@lists.freedesktop.org; Konduru, Chandra
> >> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> >> intel_atomic_setup_scalers.
> >>
> >> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> >>> This may postpone going to HQ mode until the plane is in the
> >>> drm_atomic_state if it's not using scaler 0, but it does allow moving
> >>> intel_atomic_setup_scalers to the crtc check function.
> >>>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++-------------
> ---
> >> --
> >>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
> >>>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >>>  3 files changed, 39 insertions(+), 31 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >> b/drivers/gpu/drm/i915/intel_atomic.c
> >>> index 1edd1651c045..a8202fa0daa8 100644
> >>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
> >>>  	if (ret)
> >>>  		return ret;
> >>>
> >>> -	/*
> >>> -	 * FIXME: move to crtc atomic check function once this is
> >>> -	 * more atomic friendly.
> >>> -	 */
> >>> -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> >>> -	if (ret)
> >>> -		return ret;
> >>> -
> >>>  	return ret;
> >>>  }
> >>>
> >>> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
> >> *dev,
> >>>  			/* find the plane that set the bit as scaler_user */
> >>>  			plane = drm_state->planes[i];
> >>>
> >>> -			/*
> >>> -			 * to enable/disable hq mode, add planes that are using
> >> scaler
> >>> -			 * into this transaction
> >>> -			 */
> >>>  			if (!plane) {
> >>> -				struct drm_plane_state *state;
> >>> -				plane = drm_plane_from_index(dev, i);
> >>> -				state =
> >> drm_atomic_get_plane_state(drm_state, plane);
> >>> -				if (IS_ERR(state)) {
> >>> -					DRM_DEBUG_KMS("Failed to add
> >> [PLANE:%d] to drm_state\n",
> >>> -						plane->base.id);
> >>> -					return PTR_ERR(state);
> >>> -				}
> >>> +				DRM_DEBUG_KMS("Failed to find [PLANE:%d]
> >> in drm_state\n", plane->base.id);
> >>> +				continue;
> >>>  			}
> >>> -
> >>>  			intel_plane = to_intel_plane(plane);
> >>>
> >>>  			/* plane on different crtc cannot be a scaler user of this
> >> crtc */
> >>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device
> >> *dev,
> >>>  		}
> >>>  	}
> >>>
> >>> +	/* plane not part of mask must leave hq mode? */
> >>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> >>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> >>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
> >>> +
> >>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> >>> +	}
> >>> +
> >>> +	/* plane not part of mask can enter hq mode? */
> >>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
> >>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode !=
> >> PS_SCALER_MODE_HQ) {
> >>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> >>> +
> >>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> >>> +	}
> >>> +
> >> I don't have access to the hw spec at the moment; is scaler #0 the only
> >> one that can ever go into HQ mode?
> > Yes
> >
> >> If there isn't a hardware
> >> requirement about this, then it seems like we're missing the case where
> >> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> >> disabled, which should allow scaler 1 to go into HQ mode.
> > In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
> Is it really bad to keep it on scaler 1 for a while until the next time the plane is
> added?
> 
> >> I guess it's not immediately clear to me why we need to not pull the
> >> other planes into the transaction.  Is this just to avoid doing some
> >> extra work for a plane that hasn't changed, or does it cause a problem
> >> somehow?
> > Per atomic design, unchanged planes can be added to transaction.
> > And scaler implementation is using this design feature.
> > Not sure what the issue here, but we need this feature continue
> > to available.
> >
> Unchanged planes can be added, but this could pull in a primary plane, which
> would need
> to set atomic.wait_for_flips then. I can do that as special case when adding a
> plane if
> that's preferred.

Here primary plane can get added if that is the only plane using scaler which
isn't part of the transaction. But here addition of primary plane isn't adding
or changing its FB. So why it needs to set atomic.wait_for_flips?

> 
> ~Maarten
On Wed, Jun 03, 2015 at 12:32:43PM -0700, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> > Sent: Wednesday, June 03, 2015 12:02 AM
> > To: Konduru, Chandra; Roper, Matthew D
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> > intel_atomic_setup_scalers.
> > 
> > Op 03-06-15 om 03:52 schreef Konduru, Chandra:
> > >
> > >> -----Original Message-----
> > >> From: Roper, Matthew D
> > >> Sent: Tuesday, June 02, 2015 6:30 PM
> > >> To: Maarten Lankhorst
> > >> Cc: intel-gfx@lists.freedesktop.org; Konduru, Chandra
> > >> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> > >> intel_atomic_setup_scalers.
> > >>
> > >> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> > >>> This may postpone going to HQ mode until the plane is in the
> > >>> drm_atomic_state if it's not using scaler 0, but it does allow moving
> > >>> intel_atomic_setup_scalers to the crtc check function.
> > >>>
> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >>> ---
> > >>>  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++-------------
> > ---
> > >> --
> > >>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
> > >>>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> > >>>  3 files changed, 39 insertions(+), 31 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > >> b/drivers/gpu/drm/i915/intel_atomic.c
> > >>> index 1edd1651c045..a8202fa0daa8 100644
> > >>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> > >>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > >>> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
> > >>>  	if (ret)
> > >>>  		return ret;
> > >>>
> > >>> -	/*
> > >>> -	 * FIXME: move to crtc atomic check function once this is
> > >>> -	 * more atomic friendly.
> > >>> -	 */
> > >>> -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> > >>> -	if (ret)
> > >>> -		return ret;
> > >>> -
> > >>>  	return ret;
> > >>>  }
> > >>>
> > >>> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
> > >> *dev,
> > >>>  			/* find the plane that set the bit as scaler_user */
> > >>>  			plane = drm_state->planes[i];
> > >>>
> > >>> -			/*
> > >>> -			 * to enable/disable hq mode, add planes that are using
> > >> scaler
> > >>> -			 * into this transaction
> > >>> -			 */
> > >>>  			if (!plane) {
> > >>> -				struct drm_plane_state *state;
> > >>> -				plane = drm_plane_from_index(dev, i);
> > >>> -				state =
> > >> drm_atomic_get_plane_state(drm_state, plane);
> > >>> -				if (IS_ERR(state)) {
> > >>> -					DRM_DEBUG_KMS("Failed to add
> > >> [PLANE:%d] to drm_state\n",
> > >>> -						plane->base.id);
> > >>> -					return PTR_ERR(state);
> > >>> -				}
> > >>> +				DRM_DEBUG_KMS("Failed to find [PLANE:%d]
> > >> in drm_state\n", plane->base.id);
> > >>> +				continue;
> > >>>  			}
> > >>> -
> > >>>  			intel_plane = to_intel_plane(plane);
> > >>>
> > >>>  			/* plane on different crtc cannot be a scaler user of this
> > >> crtc */
> > >>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device
> > >> *dev,
> > >>>  		}
> > >>>  	}
> > >>>
> > >>> +	/* plane not part of mask must leave hq mode? */
> > >>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> > >>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> > >>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
> > >>> +
> > >>> +		intel_crtc->atomic.skl_update_scaler0 =
> > >>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> > >>> +	}
> > >>> +
> > >>> +	/* plane not part of mask can enter hq mode? */
> > >>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
> > >>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode !=
> > >> PS_SCALER_MODE_HQ) {
> > >>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> > >>> +
> > >>> +		intel_crtc->atomic.skl_update_scaler0 =
> > >>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> > >>> +	}
> > >>> +
> > >> I don't have access to the hw spec at the moment; is scaler #0 the only
> > >> one that can ever go into HQ mode?
> > > Yes
> > >
> > >> If there isn't a hardware
> > >> requirement about this, then it seems like we're missing the case where
> > >> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> > >> disabled, which should allow scaler 1 to go into HQ mode.
> > > In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
> > Is it really bad to keep it on scaler 1 for a while until the next time the plane is
> > added?
> > 
> > >> I guess it's not immediately clear to me why we need to not pull the
> > >> other planes into the transaction.  Is this just to avoid doing some
> > >> extra work for a plane that hasn't changed, or does it cause a problem
> > >> somehow?
> > > Per atomic design, unchanged planes can be added to transaction.
> > > And scaler implementation is using this design feature.
> > > Not sure what the issue here, but we need this feature continue
> > > to available.
> > >
> > Unchanged planes can be added, but this could pull in a primary plane, which
> > would need
> > to set atomic.wait_for_flips then. I can do that as special case when adding a
> > plane if
> > that's preferred.
> 
> Here primary plane can get added if that is the only plane using scaler which
> isn't part of the transaction. But here addition of primary plane isn't adding
> or changing its FB. So why it needs to set atomic.wait_for_flips?

In the atomic case, intel_check_primary_plane() sets
atomic.wait_for_flips if the primary plane is part of the original
transaction (no matter what about the primary plane is changing).
However if we pull in the primary plane via
intel_atomic_setup_scalers(), that gets called after we've finished
checking all of the planes (that were originally part of the
transaction), so I don't think wait_for_flips will get set in that case.
So I think pulling in the plane as Chandra was will still avoid an
unnecessary wait.  Even though it's in the transaction, I believe the
'check' step is bypassed (which means we need to be careful about doing
stuff like this if it could affect derived state...but I think in this
case it's safe).

N.B.  The wait we're talking about here will only be triggered when a
SKL or BXT platform disables a "sprite" plane (thus triggering the
primary's scaler to switch to HQ mode) while there's a pending legacy
pageflip on the primary plane.  I don't think we have a userspace today
that can trigger this...Weston won't use sprites at all in a non-atomic
manner and although SNA has an Xv sprite adapter, it won't ever be
performing scaling of the primary plane afaik.


Matt

> 
> > 
> > ~Maarten
Hey,

Op 04-06-15 om 01:33 schreef Matt Roper:
> On Wed, Jun 03, 2015 at 12:32:43PM -0700, Konduru, Chandra wrote:
>>
>>> -----Original Message-----
>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>>> Sent: Wednesday, June 03, 2015 12:02 AM
>>> To: Konduru, Chandra; Roper, Matthew D
>>> Cc: intel-gfx@lists.freedesktop.org
>>> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
>>> intel_atomic_setup_scalers.
>>>
>>> Op 03-06-15 om 03:52 schreef Konduru, Chandra:
>>>>> -----Original Message-----
>>>>> From: Roper, Matthew D
>>>>> Sent: Tuesday, June 02, 2015 6:30 PM
>>>>> To: Maarten Lankhorst
>>>>> Cc: intel-gfx@lists.freedesktop.org; Konduru, Chandra
>>>>> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
>>>>> intel_atomic_setup_scalers.
>>>>>
>>>>> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
>>>>>> This may postpone going to HQ mode until the plane is in the
>>>>>> drm_atomic_state if it's not using scaler 0, but it does allow moving
>>>>>> intel_atomic_setup_scalers to the crtc check function.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++-------------
>>> ---
>>>>> --
>>>>>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
>>>>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>>>>>>  3 files changed, 39 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>>>>> index 1edd1651c045..a8202fa0daa8 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>>>> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
>>>>>>  	if (ret)
>>>>>>  		return ret;
>>>>>>
>>>>>> -	/*
>>>>>> -	 * FIXME: move to crtc atomic check function once this is
>>>>>> -	 * more atomic friendly.
>>>>>> -	 */
>>>>>> -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
>>>>>> -	if (ret)
>>>>>> -		return ret;
>>>>>> -
>>>>>>  	return ret;
>>>>>>  }
>>>>>>
>>>>>> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
>>>>> *dev,
>>>>>>  			/* find the plane that set the bit as scaler_user */
>>>>>>  			plane = drm_state->planes[i];
>>>>>>
>>>>>> -			/*
>>>>>> -			 * to enable/disable hq mode, add planes that are using
>>>>> scaler
>>>>>> -			 * into this transaction
>>>>>> -			 */
>>>>>>  			if (!plane) {
>>>>>> -				struct drm_plane_state *state;
>>>>>> -				plane = drm_plane_from_index(dev, i);
>>>>>> -				state =
>>>>> drm_atomic_get_plane_state(drm_state, plane);
>>>>>> -				if (IS_ERR(state)) {
>>>>>> -					DRM_DEBUG_KMS("Failed to add
>>>>> [PLANE:%d] to drm_state\n",
>>>>>> -						plane->base.id);
>>>>>> -					return PTR_ERR(state);
>>>>>> -				}
>>>>>> +				DRM_DEBUG_KMS("Failed to find [PLANE:%d]
>>>>> in drm_state\n", plane->base.id);
>>>>>> +				continue;
>>>>>>  			}
>>>>>> -
>>>>>>  			intel_plane = to_intel_plane(plane);
>>>>>>
>>>>>>  			/* plane on different crtc cannot be a scaler user of this
>>>>> crtc */
>>>>>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device
>>>>> *dev,
>>>>>>  		}
>>>>>>  	}
>>>>>>
>>>>>> +	/* plane not part of mask must leave hq mode? */
>>>>>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
>>>>>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
>>>>>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
>>>>>> +
>>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
>>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
>>>>>> +	}
>>>>>> +
>>>>>> +	/* plane not part of mask can enter hq mode? */
>>>>>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
>>>>>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode !=
>>>>> PS_SCALER_MODE_HQ) {
>>>>>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
>>>>>> +
>>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
>>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
>>>>>> +	}
>>>>>> +
>>>>> I don't have access to the hw spec at the moment; is scaler #0 the only
>>>>> one that can ever go into HQ mode?
>>>> Yes
>>>>
>>>>> If there isn't a hardware
>>>>> requirement about this, then it seems like we're missing the case where
>>>>> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
>>>>> disabled, which should allow scaler 1 to go into HQ mode.
>>>> In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
>>> Is it really bad to keep it on scaler 1 for a while until the next time the plane is
>>> added?
>>>
>>>>> I guess it's not immediately clear to me why we need to not pull the
>>>>> other planes into the transaction.  Is this just to avoid doing some
>>>>> extra work for a plane that hasn't changed, or does it cause a problem
>>>>> somehow?
>>>> Per atomic design, unchanged planes can be added to transaction.
>>>> And scaler implementation is using this design feature.
>>>> Not sure what the issue here, but we need this feature continue
>>>> to available.
>>>>
>>> Unchanged planes can be added, but this could pull in a primary plane, which
>>> would need
>>> to set atomic.wait_for_flips then. I can do that as special case when adding a
>>> plane if
>>> that's preferred.
>> Here primary plane can get added if that is the only plane using scaler which
>> isn't part of the transaction. But here addition of primary plane isn't adding
>> or changing its FB. So why it needs to set atomic.wait_for_flips?
> In the atomic case, intel_check_primary_plane() sets
> atomic.wait_for_flips if the primary plane is part of the original
> transaction (no matter what about the primary plane is changing).
> However if we pull in the primary plane via
> intel_atomic_setup_scalers(), that gets called after we've finished
> checking all of the planes (that were originally part of the
> transaction), so I don't think wait_for_flips will get set in that case.
> So I think pulling in the plane as Chandra was will still avoid an
> unnecessary wait.  Even though it's in the transaction, I believe the
> 'check' step is bypassed (which means we need to be careful about doing
> stuff like this if it could affect derived state...but I think in this
> case it's safe).
>
> N.B.  The wait we're talking about here will only be triggered when a
> SKL or BXT platform disables a "sprite" plane (thus triggering the
> primary's scaler to switch to HQ mode) while there's a pending legacy
> pageflip on the primary plane.  I don't think we have a userspace today
> that can trigger this...Weston won't use sprites at all in a non-atomic
> manner and although SNA has an Xv sprite adapter, it won't ever be
> performing scaling of the primary plane afaik.
Correct. The wait will be a nop if there are no legacy page flips. But for completeness sake I included it. :-)

~Maarten
> >>>>>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct
> drm_device
> >>>>> *dev,
> >>>>>>  		}
> >>>>>>  	}
> >>>>>>
> >>>>>> +	/* plane not part of mask must leave hq mode? */
> >>>>>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> >>>>>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> >>>>>> +		scaler_state->scalers[0].mode =
> PS_SCALER_MODE_DYN;
> >>>>>> +
> >>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	/* plane not part of mask can enter hq mode? */
> >>>>>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use
> &&
> >>>>>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode
> !=
> >>>>> PS_SCALER_MODE_HQ) {
> >>>>>> +		scaler_state->scalers[0].mode =
> PS_SCALER_MODE_HQ;
> >>>>>> +
> >>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> >>>>>> +	}
> >>>>>> +
> >>>>> I don't have access to the hw spec at the moment; is scaler #0 the only
> >>>>> one that can ever go into HQ mode?
> >>>> Yes
> >>>>
> >>>>> If there isn't a hardware
> >>>>> requirement about this, then it seems like we're missing the case where
> >>>>> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> >>>>> disabled, which should allow scaler 1 to go into HQ mode.
> >>>> In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
> >>> Is it really bad to keep it on scaler 1 for a while until the next time the plane
> is
> >>> added?
> >>>
> >>>>> I guess it's not immediately clear to me why we need to not pull the
> >>>>> other planes into the transaction.  Is this just to avoid doing some
> >>>>> extra work for a plane that hasn't changed, or does it cause a problem
> >>>>> somehow?
> >>>> Per atomic design, unchanged planes can be added to transaction.
> >>>> And scaler implementation is using this design feature.
> >>>> Not sure what the issue here, but we need this feature continue
> >>>> to available.
> >>>>
> >>> Unchanged planes can be added, but this could pull in a primary plane,
> which
> >>> would need
> >>> to set atomic.wait_for_flips then. I can do that as special case when adding
> a
> >>> plane if
> >>> that's preferred.
> >> Here primary plane can get added if that is the only plane using scaler which
> >> isn't part of the transaction. But here addition of primary plane isn't adding
> >> or changing its FB. So why it needs to set atomic.wait_for_flips?
> > In the atomic case, intel_check_primary_plane() sets
> > atomic.wait_for_flips if the primary plane is part of the original
> > transaction (no matter what about the primary plane is changing).
> > However if we pull in the primary plane via
> > intel_atomic_setup_scalers(), that gets called after we've finished
> > checking all of the planes (that were originally part of the
> > transaction), so I don't think wait_for_flips will get set in that case.
> > So I think pulling in the plane as Chandra was will still avoid an
> > unnecessary wait.  Even though it's in the transaction, I believe the
> > 'check' step is bypassed (which means we need to be careful about doing
> > stuff like this if it could affect derived state...but I think in this
> > case it's safe).
> >
> > N.B.  The wait we're talking about here will only be triggered when a
> > SKL or BXT platform disables a "sprite" plane (thus triggering the
> > primary's scaler to switch to HQ mode) while there's a pending legacy
> > pageflip on the primary plane.  I don't think we have a userspace today
> > that can trigger this...Weston won't use sprites at all in a non-atomic
> > manner and although SNA has an Xv sprite adapter, it won't ever be
> > performing scaling of the primary plane afaik.
> Correct. The wait will be a nop if there are no legacy page flips. But for
> completeness sake I included it. :-)

So you will be keeping current code to add plane as-is and let respective 
update_plane function take care of updating scaler quality instead 
of the above update_scaler0?

> 
> ~Maarten
Op 05-06-15 om 21:05 schreef Konduru, Chandra:
>>>>>>>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct
>> drm_device
>>>>>>> *dev,
>>>>>>>>  		}
>>>>>>>>  	}
>>>>>>>>
>>>>>>>> +	/* plane not part of mask must leave hq mode? */
>>>>>>>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
>>>>>>>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
>>>>>>>> +		scaler_state->scalers[0].mode =
>> PS_SCALER_MODE_DYN;
>>>>>>>> +
>>>>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
>>>>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	/* plane not part of mask can enter hq mode? */
>>>>>>>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use
>> &&
>>>>>>>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode
>> !=
>>>>>>> PS_SCALER_MODE_HQ) {
>>>>>>>> +		scaler_state->scalers[0].mode =
>> PS_SCALER_MODE_HQ;
>>>>>>>> +
>>>>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
>>>>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>> I don't have access to the hw spec at the moment; is scaler #0 the only
>>>>>>> one that can ever go into HQ mode?
>>>>>> Yes
>>>>>>
>>>>>>> If there isn't a hardware
>>>>>>> requirement about this, then it seems like we're missing the case where
>>>>>>> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
>>>>>>> disabled, which should allow scaler 1 to go into HQ mode.
>>>>>> In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
>>>>> Is it really bad to keep it on scaler 1 for a while until the next time the plane
>> is
>>>>> added?
>>>>>
>>>>>>> I guess it's not immediately clear to me why we need to not pull the
>>>>>>> other planes into the transaction.  Is this just to avoid doing some
>>>>>>> extra work for a plane that hasn't changed, or does it cause a problem
>>>>>>> somehow?
>>>>>> Per atomic design, unchanged planes can be added to transaction.
>>>>>> And scaler implementation is using this design feature.
>>>>>> Not sure what the issue here, but we need this feature continue
>>>>>> to available.
>>>>>>
>>>>> Unchanged planes can be added, but this could pull in a primary plane,
>> which
>>>>> would need
>>>>> to set atomic.wait_for_flips then. I can do that as special case when adding
>> a
>>>>> plane if
>>>>> that's preferred.
>>>> Here primary plane can get added if that is the only plane using scaler which
>>>> isn't part of the transaction. But here addition of primary plane isn't adding
>>>> or changing its FB. So why it needs to set atomic.wait_for_flips?
>>> In the atomic case, intel_check_primary_plane() sets
>>> atomic.wait_for_flips if the primary plane is part of the original
>>> transaction (no matter what about the primary plane is changing).
>>> However if we pull in the primary plane via
>>> intel_atomic_setup_scalers(), that gets called after we've finished
>>> checking all of the planes (that were originally part of the
>>> transaction), so I don't think wait_for_flips will get set in that case.
>>> So I think pulling in the plane as Chandra was will still avoid an
>>> unnecessary wait.  Even though it's in the transaction, I believe the
>>> 'check' step is bypassed (which means we need to be careful about doing
>>> stuff like this if it could affect derived state...but I think in this
>>> case it's safe).
>>>
>>> N.B.  The wait we're talking about here will only be triggered when a
>>> SKL or BXT platform disables a "sprite" plane (thus triggering the
>>> primary's scaler to switch to HQ mode) while there's a pending legacy
>>> pageflip on the primary plane.  I don't think we have a userspace today
>>> that can trigger this...Weston won't use sprites at all in a non-atomic
>>> manner and although SNA has an Xv sprite adapter, it won't ever be
>>> performing scaling of the primary plane afaik.
>> Correct. The wait will be a nop if there are no legacy page flips. But for
>> completeness sake I included it. :-)
> So you will be keeping current code to add plane as-is and let respective 
> update_plane function take care of updating scaler quality instead 
> of the above update_scaler0?
>
Yes, see "[PATCH v2 08/27] drm/i915: Move scaler setup to check crtc function, v2."
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Friday, June 05, 2015 11:39 PM
> To: Konduru, Chandra; Roper, Matthew D
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> intel_atomic_setup_scalers.
> 
> Op 05-06-15 om 21:05 schreef Konduru, Chandra:
> >>>>>>>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct
> >> drm_device
> >>>>>>> *dev,
> >>>>>>>>  		}
> >>>>>>>>  	}
> >>>>>>>>
> >>>>>>>> +	/* plane not part of mask must leave hq mode? */
> >>>>>>>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> >>>>>>>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> >>>>>>>> +		scaler_state->scalers[0].mode =
> >> PS_SCALER_MODE_DYN;
> >>>>>>>> +
> >>>>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>>>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	/* plane not part of mask can enter hq mode? */
> >>>>>>>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use
> >> &&
> >>>>>>>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode
> >> !=
> >>>>>>> PS_SCALER_MODE_HQ) {
> >>>>>>>> +		scaler_state->scalers[0].mode =
> >> PS_SCALER_MODE_HQ;
> >>>>>>>> +
> >>>>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>>>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>> I don't have access to the hw spec at the moment; is scaler #0 the only
> >>>>>>> one that can ever go into HQ mode?
> >>>>>> Yes
> >>>>>>
> >>>>>>> If there isn't a hardware
> >>>>>>> requirement about this, then it seems like we're missing the case where
> >>>>>>> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> >>>>>>> disabled, which should allow scaler 1 to go into HQ mode.
> >>>>>> In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
> >>>>> Is it really bad to keep it on scaler 1 for a while until the next time the
> plane
> >> is
> >>>>> added?
> >>>>>
> >>>>>>> I guess it's not immediately clear to me why we need to not pull the
> >>>>>>> other planes into the transaction.  Is this just to avoid doing some
> >>>>>>> extra work for a plane that hasn't changed, or does it cause a problem
> >>>>>>> somehow?
> >>>>>> Per atomic design, unchanged planes can be added to transaction.
> >>>>>> And scaler implementation is using this design feature.
> >>>>>> Not sure what the issue here, but we need this feature continue
> >>>>>> to available.
> >>>>>>
> >>>>> Unchanged planes can be added, but this could pull in a primary plane,
> >> which
> >>>>> would need
> >>>>> to set atomic.wait_for_flips then. I can do that as special case when
> adding
> >> a
> >>>>> plane if
> >>>>> that's preferred.
> >>>> Here primary plane can get added if that is the only plane using scaler
> which
> >>>> isn't part of the transaction. But here addition of primary plane isn't adding
> >>>> or changing its FB. So why it needs to set atomic.wait_for_flips?
> >>> In the atomic case, intel_check_primary_plane() sets
> >>> atomic.wait_for_flips if the primary plane is part of the original
> >>> transaction (no matter what about the primary plane is changing).
> >>> However if we pull in the primary plane via
> >>> intel_atomic_setup_scalers(), that gets called after we've finished
> >>> checking all of the planes (that were originally part of the
> >>> transaction), so I don't think wait_for_flips will get set in that case.
> >>> So I think pulling in the plane as Chandra was will still avoid an
> >>> unnecessary wait.  Even though it's in the transaction, I believe the
> >>> 'check' step is bypassed (which means we need to be careful about doing
> >>> stuff like this if it could affect derived state...but I think in this
> >>> case it's safe).
> >>>
> >>> N.B.  The wait we're talking about here will only be triggered when a
> >>> SKL or BXT platform disables a "sprite" plane (thus triggering the
> >>> primary's scaler to switch to HQ mode) while there's a pending legacy
> >>> pageflip on the primary plane.  I don't think we have a userspace today
> >>> that can trigger this...Weston won't use sprites at all in a non-atomic
> >>> manner and although SNA has an Xv sprite adapter, it won't ever be
> >>> performing scaling of the primary plane afaik.
> >> Correct. The wait will be a nop if there are no legacy page flips. But for
> >> completeness sake I included it. :-)
> > So you will be keeping current code to add plane as-is and let respective
> > update_plane function take care of updating scaler quality instead
> > of the above update_scaler0?
> >
> Yes, see "[PATCH v2 08/27] drm/i915: Move scaler setup to check crtc function,
> v2."
OK. Please run kms_plane_scaling and kms_panel_fitting 
visual tests to make sure they are passing.
On Thu, Jun 04, 2015 at 05:39:52AM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 04-06-15 om 01:33 schreef Matt Roper:
> > On Wed, Jun 03, 2015 at 12:32:43PM -0700, Konduru, Chandra wrote:
> >>
> >>> -----Original Message-----
> >>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >>> Sent: Wednesday, June 03, 2015 12:02 AM
> >>> To: Konduru, Chandra; Roper, Matthew D
> >>> Cc: intel-gfx@lists.freedesktop.org
> >>> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> >>> intel_atomic_setup_scalers.
> >>>
> >>> Op 03-06-15 om 03:52 schreef Konduru, Chandra:
> >>>>> -----Original Message-----
> >>>>> From: Roper, Matthew D
> >>>>> Sent: Tuesday, June 02, 2015 6:30 PM
> >>>>> To: Maarten Lankhorst
> >>>>> Cc: intel-gfx@lists.freedesktop.org; Konduru, Chandra
> >>>>> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> >>>>> intel_atomic_setup_scalers.
> >>>>>
> >>>>> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> >>>>>> This may postpone going to HQ mode until the plane is in the
> >>>>>> drm_atomic_state if it's not using scaler 0, but it does allow moving
> >>>>>> intel_atomic_setup_scalers to the crtc check function.
> >>>>>>
> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/i915/intel_atomic.c  | 41 ++++++++++++++++++-------------
> >>> ---
> >>>>> --
> >>>>>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
> >>>>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >>>>>>  3 files changed, 39 insertions(+), 31 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >>>>> b/drivers/gpu/drm/i915/intel_atomic.c
> >>>>>> index 1edd1651c045..a8202fa0daa8 100644
> >>>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>>>>> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
> >>>>>>  	if (ret)
> >>>>>>  		return ret;
> >>>>>>
> >>>>>> -	/*
> >>>>>> -	 * FIXME: move to crtc atomic check function once this is
> >>>>>> -	 * more atomic friendly.
> >>>>>> -	 */
> >>>>>> -	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> >>>>>> -	if (ret)
> >>>>>> -		return ret;
> >>>>>> -
> >>>>>>  	return ret;
> >>>>>>  }
> >>>>>>
> >>>>>> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
> >>>>> *dev,
> >>>>>>  			/* find the plane that set the bit as scaler_user */
> >>>>>>  			plane = drm_state->planes[i];
> >>>>>>
> >>>>>> -			/*
> >>>>>> -			 * to enable/disable hq mode, add planes that are using
> >>>>> scaler
> >>>>>> -			 * into this transaction
> >>>>>> -			 */
> >>>>>>  			if (!plane) {
> >>>>>> -				struct drm_plane_state *state;
> >>>>>> -				plane = drm_plane_from_index(dev, i);
> >>>>>> -				state =
> >>>>> drm_atomic_get_plane_state(drm_state, plane);
> >>>>>> -				if (IS_ERR(state)) {
> >>>>>> -					DRM_DEBUG_KMS("Failed to add
> >>>>> [PLANE:%d] to drm_state\n",
> >>>>>> -						plane->base.id);
> >>>>>> -					return PTR_ERR(state);
> >>>>>> -				}
> >>>>>> +				DRM_DEBUG_KMS("Failed to find [PLANE:%d]
> >>>>> in drm_state\n", plane->base.id);
> >>>>>> +				continue;
> >>>>>>  			}
> >>>>>> -
> >>>>>>  			intel_plane = to_intel_plane(plane);
> >>>>>>
> >>>>>>  			/* plane on different crtc cannot be a scaler user of this
> >>>>> crtc */
> >>>>>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device
> >>>>> *dev,
> >>>>>>  		}
> >>>>>>  	}
> >>>>>>
> >>>>>> +	/* plane not part of mask must leave hq mode? */
> >>>>>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> >>>>>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> >>>>>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
> >>>>>> +
> >>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	/* plane not part of mask can enter hq mode? */
> >>>>>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
> >>>>>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode !=
> >>>>> PS_SCALER_MODE_HQ) {
> >>>>>> +		scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> >>>>>> +
> >>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> >>>>>> +	}
> >>>>>> +
> >>>>> I don't have access to the hw spec at the moment; is scaler #0 the only
> >>>>> one that can ever go into HQ mode?
> >>>> Yes
> >>>>
> >>>>> If there isn't a hardware
> >>>>> requirement about this, then it seems like we're missing the case where
> >>>>> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> >>>>> disabled, which should allow scaler 1 to go into HQ mode.
> >>>> In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
> >>> Is it really bad to keep it on scaler 1 for a while until the next time the plane is
> >>> added?
> >>>
> >>>>> I guess it's not immediately clear to me why we need to not pull the
> >>>>> other planes into the transaction.  Is this just to avoid doing some
> >>>>> extra work for a plane that hasn't changed, or does it cause a problem
> >>>>> somehow?
> >>>> Per atomic design, unchanged planes can be added to transaction.
> >>>> And scaler implementation is using this design feature.
> >>>> Not sure what the issue here, but we need this feature continue
> >>>> to available.
> >>>>
> >>> Unchanged planes can be added, but this could pull in a primary plane, which
> >>> would need
> >>> to set atomic.wait_for_flips then. I can do that as special case when adding a
> >>> plane if
> >>> that's preferred.
> >> Here primary plane can get added if that is the only plane using scaler which
> >> isn't part of the transaction. But here addition of primary plane isn't adding
> >> or changing its FB. So why it needs to set atomic.wait_for_flips?
> > In the atomic case, intel_check_primary_plane() sets
> > atomic.wait_for_flips if the primary plane is part of the original
> > transaction (no matter what about the primary plane is changing).
> > However if we pull in the primary plane via
> > intel_atomic_setup_scalers(), that gets called after we've finished
> > checking all of the planes (that were originally part of the
> > transaction), so I don't think wait_for_flips will get set in that case.
> > So I think pulling in the plane as Chandra was will still avoid an
> > unnecessary wait.  Even though it's in the transaction, I believe the
> > 'check' step is bypassed (which means we need to be careful about doing
> > stuff like this if it could affect derived state...but I think in this
> > case it's safe).
> >
> > N.B.  The wait we're talking about here will only be triggered when a
> > SKL or BXT platform disables a "sprite" plane (thus triggering the
> > primary's scaler to switch to HQ mode) while there's a pending legacy
> > pageflip on the primary plane.  I don't think we have a userspace today
> > that can trigger this...Weston won't use sprites at all in a non-atomic
> > manner and although SNA has an Xv sprite adapter, it won't ever be
> > performing scaling of the primary plane afaik.
> Correct. The wait will be a nop if there are no legacy page flips. But for completeness sake I included it. :-)

Just an aside: Bending over backwards for wait_for_flips is imo pointless
since we'll ditch all that code anyway. Well as soon as we've moved
pageflips over to atomic at least. So a bit of temporary hacks should be
fine here.
-Daniel