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

[Matt Roper <matthew.d.roper@intel.com>]

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx