Re: [PATCH v2 04/20] drm/i915: Do not update pfit state when toggling crtc enabled.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 07, 2015 at 09:08:15AM +0200, Maarten Lankhorst wrote:
> This must be done in advance, and during crtc_disable all scalers

"in advance" ... before what exactly? Yes I'm harping a bit about commit
messages today ;-)

> can be force disabled.

Why does it matter that all scalers can be force disabled?

> This means intel_atomic_setup_scalers is only called in 1 place now,
> during crtc_check.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

tbh I don't really understand what's the problem and what's the solution
implemented here. Judging from comments the trouble is that we don't
correctly recover the pfit state for skl scalers at hw readout time?

I guess we should fix that for at lest the crtc level scaler (including
cross-checking of all scaler state) and in sanitize_plane force-disabling
any plane that is using a scaler. But really not much clue here.

> ---
>  drivers/gpu/drm/i915/intel_atomic.c  | 14 ++------
>  drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_dp.c      |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  4 files changed, 48 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 0aeced82201e..429677a111d5 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -272,17 +272,12 @@ int intel_atomic_setup_scalers(struct drm_device *dev,

This function is way too big. Yeah not your doing, but imo would be good
to split it up a bit. Especially since it already has a comment explaining
the high-level flow which would be a good pattern to structure the
subfunction extraction after.

>  	struct drm_plane *plane = NULL;
>  	struct intel_plane *intel_plane;
>  	struct intel_plane_state *plane_state = NULL;
> -	struct intel_crtc_scaler_state *scaler_state;
> -	struct drm_atomic_state *drm_state;
> +	struct intel_crtc_scaler_state *scaler_state =
> +		&crtc_state->scaler_state;
> +	struct drm_atomic_state *drm_state = crtc_state->base.state;
>  	int num_scalers_need;
>  	int i, j;
>  
> -	if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state)
> -		return 0;
> -
> -	scaler_state = &crtc_state->scaler_state;
> -	drm_state = crtc_state->base.state;
> -
>  	num_scalers_need = hweight32(scaler_state->scaler_users);
>  	DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users = 0x%x\n",
>  		crtc_state, num_scalers_need, intel_crtc->num_scalers,
> @@ -327,9 +322,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>  			name = "PLANE";
>  			idx = plane->base.id;
>  
> -			if (!drm_state)
> -				continue;
> -
>  			/* plane scaler case: assign as a plane scaler */
>  			/* find the plane that set the bit as scaler_user */
>  			plane = drm_state->planes[i];
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d1a474e0d13..8cd3a7eb1e30 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2909,29 +2909,32 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>  	return i915_gem_obj_ggtt_offset_view(obj, view);
>  }
>  
> +static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id)
> +{
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, id), 0);
> +	I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, id), 0);
> +	I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, id), 0);
> +	DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n",
> +		intel_crtc->base.base.id, intel_crtc->pipe, id);
> +}
> +
>  /*
>   * This function detaches (aka. unbinds) unused scalers in hardware
>   */
>  static void skl_detach_scalers(struct intel_crtc *intel_crtc)
>  {
> -	struct drm_device *dev;
> -	struct drm_i915_private *dev_priv;
>  	struct intel_crtc_scaler_state *scaler_state;
>  	int i;
>  
> -	dev = intel_crtc->base.dev;
> -	dev_priv = dev->dev_private;
>  	scaler_state = &intel_crtc->config->scaler_state;
>  
>  	/* loop through and disable scalers that aren't in use */
>  	for (i = 0; i < intel_crtc->num_scalers; i++) {
> -		if (!scaler_state->scalers[i].in_use) {
> -			I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, i), 0);
> -			I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, i), 0);
> -			I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, i), 0);
> -			DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n",
> -				intel_crtc->base.base.id, intel_crtc->pipe, i);
> -		}
> +		if (!scaler_state->scalers[i].in_use)
> +			skl_detach_scaler(intel_crtc, i);
>  	}
>  }
>  
> @@ -4362,13 +4365,12 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>   * skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
>   *
>   * @state: crtc's scaler state
> - * @force_detach: whether to forcibly disable scaler
>   *
>   * Return
>   *     0 - scaler_usage updated successfully
>   *    error - requested scaling cannot be supported or other error condition
>   */
> -int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
> +int skl_update_scaler_crtc(struct intel_crtc_state *state)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
>  	struct drm_display_mode *adjusted_mode =
> @@ -4377,7 +4379,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
>  	DRM_DEBUG_KMS("Updating scaler for [CRTC:%i] scaler_user index %u.%u\n",
>  		      intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
>  
> -	return skl_update_scaler(state, force_detach, SKL_CRTC_INDEX,
> +	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
>  		&state->scaler_state.scaler_id, DRM_ROTATE_0,
>  		state->pipe_src_w, state->pipe_src_h,
>  		adjusted_mode->hdisplay, adjusted_mode->vdisplay);
> @@ -4451,7 +4453,15 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
>  	return 0;
>  }
>  
> -static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
> +static void skylake_scaler_disable(struct intel_crtc *crtc)
> +{
> +	int i;
> +
> +	for (i = 0; i < crtc->num_scalers; i++)
> +		skl_detach_scaler(crtc, i);
> +}
> +
> +static void skylake_pfit_enable(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4461,13 +4471,6 @@ static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
>  
>  	DRM_DEBUG_KMS("for crtc_state = %p\n", crtc->config);
>  
> -	/* To update pfit, first update scaler state */
> -	skl_update_scaler_crtc(crtc->config, !enable);
> -	intel_atomic_setup_scalers(crtc->base.dev, crtc, crtc->config);
> -	skl_detach_scalers(crtc);
> -	if (!enable)
> -		return;
> -
>  	if (crtc->config->pch_pfit.enabled) {
>  		int id;
>  
> @@ -4941,7 +4944,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	intel_ddi_enable_pipe_clock(intel_crtc);
>  
>  	if (INTEL_INFO(dev)->gen == 9)
> -		skylake_pfit_update(intel_crtc, 1);
> +		skylake_pfit_enable(intel_crtc);
>  	else if (INTEL_INFO(dev)->gen < 9)
>  		ironlake_pfit_enable(intel_crtc);
>  	else
> @@ -5075,7 +5078,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
>  
>  	if (INTEL_INFO(dev)->gen == 9)
> -		skylake_pfit_update(intel_crtc, 0);
> +		skylake_scaler_disable(intel_crtc);
>  	else if (INTEL_INFO(dev)->gen < 9)
>  		ironlake_pfit_disable(intel_crtc);
>  	else
> @@ -11823,7 +11826,17 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  			return ret;
>  	}
>  
> -	return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
> +	ret = 0;
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		if (mode_changed)
> +			ret = skl_update_scaler_crtc(pipe_config);
> +
> +		if (!ret)
> +			ret = intel_atomic_setup_scalers(dev, intel_crtc,
> +							 pipe_config);
> +	}
> +
> +	return ret;
>  }
>  
>  static const struct drm_crtc_helper_funcs intel_helper_funcs = {
> @@ -15327,6 +15340,11 @@ static void readout_plane_state(struct intel_crtc *crtc,
>  			continue;
>  
>  		drm_plane_state = p->base.state;
> +
> +		/* Plane scaler state is not touched here. The first atomic
> +		 * commit will restore all plane scalers to its old state.
> +		 */
> +
>  		if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) {
>  			visible = primary_get_hw_state(crtc);
>  			to_intel_plane_state(drm_plane_state)->visible = visible;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fcc64e5ef9c8..f1b9f939b435 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1384,7 +1384,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  
>  		if (INTEL_INFO(dev)->gen >= 9) {
>  			int ret;
> -			ret = skl_update_scaler_crtc(pipe_config, 0);
> +			ret = skl_update_scaler_crtc(pipe_config);
>  			if (ret)
>  				return ret;
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 63d7d32e6123..2ed618f78fe6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1152,7 +1152,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  
> -int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
> +int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
>  int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
>  
>  unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx