Re: [PATCH v2 18/27] drm/i915: Handle disabling planes better.

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

On Thu, Jun 04, 2015 at 02:47:48PM +0200, Maarten Lankhorst wrote:
> Read out the initial state, and add a quirk to force disable all plane
> that were initially active. Use this to disable planes during modeset
> too.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |   7 ++
>  drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   7 ++
>  3 files changed, 114 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 8447a1fef332..5627df2807b0 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -96,6 +96,13 @@ int intel_atomic_check(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	if (crtc_state &&
> +	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE) {
> +		ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = drm_atomic_helper_check_planes(dev, state);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b72724121f57..3b5d23692935 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4830,11 +4830,20 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>  	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
>  }
>  
> +static void intel_disable_planes_on_crtc(struct drm_crtc *c)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(c);
> +	struct drm_plane *plane;
> +
> +	drm_for_each_plane_mask(plane, crtc->base.dev,
> +				crtc->atomic.force_disabled_planes)
> +		to_intel_plane(plane)->disable_plane(plane, c);
> +}
> +
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
>  	int pipe = intel_crtc->pipe;
>  
>  	intel_crtc_wait_for_pending_flips(crtc);
> @@ -4842,14 +4851,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  	intel_pre_disable_primary(crtc);
>  
>  	intel_crtc_dpms_overlay_disable(intel_crtc);
> -	for_each_intel_plane(dev, intel_plane) {
> -		if (intel_plane->pipe == pipe) {
> -			struct drm_crtc *from = intel_plane->base.crtc;
> -
> -			intel_plane->disable_plane(&intel_plane->base,
> -						   from ?: crtc);
> -		}
> -	}
> +	intel_disable_planes_on_crtc(crtc);
>  
>  	/*
>  	 * FIXME: Once we grow proper nuclear flip support out of this we need
> @@ -11554,6 +11556,21 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	if (!intel_crtc->active || mode_changed)
>  		return 0;
>  
> +	if (to_intel_crtc_state(crtc_state)->quirks &
> +	    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> +		if (!plane_state->crtc) {
> +			intel_crtc->atomic.force_disabled_planes |=
> +				1 << drm_plane_index(plane);
> +			crtc_state->plane_mask &=
> +				~(1 << drm_plane_index(plane));
> +		}
> +
> +		/* Initial state for sprite planes is unknown,
> +		 * no need to update sprite watermarks */
> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> +			mode_changed = true;
> +	}
> +
>  	was_visible = old_plane_state->visible;
>  	visible = to_intel_plane_state(plane_state)->visible;
>  
> @@ -11633,7 +11650,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			intel_crtc->atomic.fb_bits |=
>  			    INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
>  
> -		if (turn_off && is_crtc_enabled) {
> +		if (turn_off && !mode_changed) {
>  			intel_crtc->atomic.wait_vblank = true;
>  			intel_crtc->atomic.update_sprite_watermarks |=
>  				1 << i;
> @@ -11714,6 +11731,11 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
>  		idx, crtc->state->active, intel_crtc->active);
>  
> +	/* plane mask is fixed up after all initial planes are calculated */
> +	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> +	if (mode_changed)
> +		intel_crtc->atomic.force_disabled_planes = crtc->state->plane_mask;
> +
>  	if (mode_changed && crtc_state->enable &&
>  	    dev_priv->display.crtc_compute_clock &&
>  	    !WARN_ON(pipe_config->shared_dpll != DPLL_ID_PRIVATE)) {
> @@ -12883,6 +12905,13 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>  		return ret;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (to_intel_crtc_state(crtc_state)->quirks &
> +		    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> +			ret = drm_atomic_add_affected_planes(state, crtc);
> +			if (ret)
> +				return ret;
> +		}
> +
>  		if (!needs_modeset(crtc_state))
>  			continue;
>  
> @@ -13529,13 +13558,17 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	intel_runtime_pm_get(dev_priv);
>  
>  	/* Perform vblank evasion around commit operation */
> -	if (crtc->state->active && !needs_modeset(crtc->state))
> +	if (crtc->state->active && !needs_modeset(crtc->state)) {
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
>  
> +		if (intel_crtc->atomic.force_disabled_planes)
> +			intel_disable_planes_on_crtc(crtc);
> +	}
> +
>  	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> -		skl_detach_scalers(intel_crtc);
> +                skl_detach_scalers(intel_crtc);

Unintentional change from tabs to spaces on this line?

>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> @@ -15040,14 +15073,63 @@ void i915_redisable_vga(struct drm_device *dev)
>  	i915_redisable_vga_power_on(dev);
>  }
>  
> -static bool primary_get_hw_state(struct intel_crtc *crtc)
> +static bool intel_read_hw_plane_state(struct intel_crtc *crtc,
> +				      struct intel_plane *intel_plane)
>  {
> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!crtc->base.enabled)
> -		return false;
> +	switch (intel_plane->base.type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> +
> +	case DRM_PLANE_TYPE_CURSOR:
> +		if (IS_845G(dev) || IS_I865G(dev))
> +			return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> +		else
> +			return I915_READ(CURCNTR(crtc->plane)) & CURSOR_MODE;
> +

If we're not trying to seamlessly inherit the cursor image, I'm not sure
I understand what the benefit of figuring out whether the cursor is
actually on or not is.  Just assuming true, as we do for sprites, would
be enough to make the cursor always turn off, right?

> +	default:
> +		return true;
> +	}
> +}
> +
> +static int readout_plane_state(struct drm_atomic_state *state,
> +			       struct intel_crtc *crtc,
> +			       struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_plane *p;
> +	struct drm_plane_state *drm_plane_state;
> +	bool active = crtc_state->base.active;
> +
> +	if (active) {
> +		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>  
> -	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> +		/* apply to previous sw state too */
> +		to_intel_crtc_state(crtc->base.state)->quirks |=
> +			PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> +	}
> +
> +	for_each_intel_plane(state->dev, p) {
> +		if (crtc->plane != p->plane)
> +			continue;

Was this supposed to be comparing ->pipe rather than ->plane?  For
primary and cursor planes I believe this works out okay since ->plane
and ->pipe are basically the same (except for pre-gen4 FBC swapping),
but for sprite planes, the ->plane pointer indicates whether it's the
first, second, third, etc. sprite of that specific CRTC.

As written, I think you'll only be operating on the first sprite plane
of CRTC 0, the second sprite plane of CRTC 1, etc.


Matt

> +
> +		drm_plane_state = drm_atomic_get_plane_state(state, &p->base);
> +		if (IS_ERR(drm_plane_state))
> +			return PTR_ERR(drm_plane_state);
> +
> +		if (active && intel_read_hw_plane_state(crtc, p)) {
> +			to_intel_plane_state(drm_plane_state)->visible = true;
> +			crtc_state->base.plane_mask |= 1 << drm_plane_index(&p->base);
> +			p->base.crtc = &crtc->base;
> +		} else {
> +			to_intel_plane_state(drm_plane_state)->visible = false;
> +			crtc_state->base.plane_mask &= ~(1 << drm_plane_index(&p->base));
> +			p->base.crtc = drm_plane_state->crtc;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static int readout_hw_crtc_state(struct drm_atomic_state *state,
> @@ -15055,19 +15137,11 @@ static int readout_hw_crtc_state(struct drm_atomic_state *state,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_crtc_state *crtc_state;
> -	struct drm_plane *primary = crtc->base.primary;
> -	struct drm_plane_state *drm_plane_state;
> -	struct intel_plane_state *plane_state;
> -	int ret;
>  
>  	crtc_state = intel_atomic_get_crtc_state(state, crtc);
>  	if (IS_ERR(crtc_state))
>  		return PTR_ERR(crtc_state);
>  
> -	ret = drm_atomic_add_affected_planes(state, &crtc->base);
> -	if (ret)
> -		return ret;
> -
>  	memset(crtc_state, 0, sizeof(*crtc_state));
>  	crtc_state->base.crtc = &crtc->base;
>  	crtc_state->base.state = state;
> @@ -15081,24 +15155,11 @@ static int readout_hw_crtc_state(struct drm_atomic_state *state,
>  	crtc->active = crtc_state->base.active;
>  	crtc->config = crtc_state;
>  
> -	drm_plane_state = drm_atomic_get_plane_state(state, primary);
> -	if (IS_ERR(drm_plane_state))
> -		return PTR_ERR(drm_plane_state);
> -
> -	plane_state = to_intel_plane_state(drm_plane_state);
> -	plane_state->visible = primary_get_hw_state(crtc);
> -
> -	if (plane_state->visible) {
> -		primary->crtc = &crtc->base;
> -		crtc_state->base.plane_mask |= 1 << drm_plane_index(primary);
> -	} else
> -		crtc_state->base.plane_mask &= ~(1 << drm_plane_index(primary));
> -
>  	DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>  		      crtc->base.base.id,
>  		      crtc_state->base.active ? "enabled" : "disabled");
>  
> -	return 0;
> +	return readout_plane_state(state, crtc, crtc_state);
>  }
>  
>  static int readout_hw_pll_state(struct drm_atomic_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d96c9a24b94b..7b1a13f63872 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -327,6 +327,7 @@ struct intel_crtc_state {
>  	 */
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
>  #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
> +#define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
>  	unsigned long quirks;
>  
>  	/* Pipe source size (ie. panel fitter input size)
> @@ -485,8 +486,14 @@ struct intel_crtc_atomic_commit {
>  	bool disable_fbc;
>  	bool pre_disable_primary;
>  	bool update_wm;
> +
> +	/* planes that will be disabled in a normal way */
>  	unsigned disabled_planes;
>  
> +	/* planes that will be disabled during modeset,
> +	 * or initially enabled planes */
> +	unsigned force_disabled_planes;
> +
>  	/* 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