Re: [PATCH 2/9] drm/i915: Redo plane sanitation during readout

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 11, 2017 at 07:04:48PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> Unify the plane disabling during state readout by pulling the code into
> a new helper intel_plane_disable_noatomic(). We'll also read out the
> state of all planes, so that we know which planes really need to be
> diabled.
> 
> Additonally we change the plane<->pipe mapping sanitation to work by
> simply disabling the offending planes instead of entire pipes. And
> we do it before we otherwise sanitize the crtcs, which means we don't
> have to worry about misassigned planes during crtc sanitation anymore.
> 
> v2: Reoder patches to not depend on enum old_plane_id
> 
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> Cc: Alex Villacís Lasso <alexvillacislasso@xxxxxxxxxxx>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103223
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 116 ++++++++++++++++++++---------------
>  1 file changed, 67 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 825ab00b6639..a9fd3b8fa922 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2729,6 +2729,23 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state,
>  		      crtc_state->active_planes);
>  }
>  
> +static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
> +					 struct intel_plane *plane)
> +{
> +	struct intel_crtc_state *crtc_state =
> +		to_intel_crtc_state(crtc->base.state);
> +	struct intel_plane_state *plane_state =
> +		to_intel_plane_state(plane->base.state);
> +
> +	intel_set_plane_visible(crtc_state, plane_state, false);
> +
> +	if (plane->id == PLANE_PRIMARY)
> +		intel_pre_disable_primary_noatomic(&crtc->base);
> +
> +	trace_intel_disable_plane(&plane->base, crtc);
> +	plane->disable_plane(plane, crtc);
> +}

Wooot, I asked Maarten ages ago to extract this, thanks for doing this!

Just for that:

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

(ah no I'm kidding, I did check a few things, but thankfully CI now at
least covers some gen3 fun).

Cheers, Daniel


> +
>  static void
>  intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  			     struct intel_initial_plane_config *plane_config)
> @@ -2786,12 +2803,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	 * simplest solution is to just disable the primary plane now and
>  	 * pretend the BIOS never had it enabled.
>  	 */
> -	intel_set_plane_visible(to_intel_crtc_state(crtc_state),
> -				to_intel_plane_state(plane_state),
> -				false);
> -	intel_pre_disable_primary_noatomic(&intel_crtc->base);
> -	trace_intel_disable_plane(primary, intel_crtc);
> -	intel_plane->disable_plane(intel_plane, intel_crtc);
> +	intel_plane_disable_noatomic(intel_crtc, intel_plane);
>  
>  	return;
>  
> @@ -5923,6 +5935,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	enum intel_display_power_domain domain;
> +	struct intel_plane *plane;
>  	u64 domains;
>  	struct drm_atomic_state *state;
>  	struct intel_crtc_state *crtc_state;
> @@ -5931,11 +5944,12 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
>  	if (!intel_crtc->active)
>  		return;
>  
> -	if (crtc->primary->state->visible) {
> -		intel_pre_disable_primary_noatomic(crtc);
> +	for_each_intel_plane_on_crtc(&dev_priv->drm, intel_crtc, plane) {
> +		const struct intel_plane_state *plane_state =
> +			to_intel_plane_state(plane->base.state);
>  
> -		intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary));
> -		crtc->primary->state->visible = false;
> +		if (plane_state->base.visible)
> +			intel_plane_disable_noatomic(intel_crtc, plane);


>  	}
>  
>  	state = drm_atomic_state_alloc(crtc->dev);
> @@ -14678,22 +14692,38 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	POSTING_READ(DPLL(pipe));
>  }
>  
> -static bool
> -intel_check_plane_mapping(struct intel_crtc *crtc)
> +static bool intel_plane_mapping_ok(struct intel_crtc *crtc,
> +				   struct intel_plane *primary)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	u32 val;
> +	enum plane plane = primary->plane;
> +	u32 val = I915_READ(DSPCNTR(plane));
>  
> -	if (INTEL_INFO(dev_priv)->num_pipes == 1)
> -		return true;
> +	return (val & DISPLAY_PLANE_ENABLE) == 0 ||
> +		(val & DISPPLANE_SEL_PIPE_MASK) == DISPPLANE_SEL_PIPE(crtc->pipe);
> +}
>  
> -	val = I915_READ(DSPCNTR(!crtc->plane));
> +static void
> +intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
> +{
> +	enum pipe pipe;
>  
> -	if ((val & DISPLAY_PLANE_ENABLE) &&
> -	    (!!(val & DISPPLANE_SEL_PIPE_MASK) == crtc->pipe))
> -		return false;
> +	if (INTEL_GEN(dev_priv) >= 4)
> +		return;
>  
> -	return true;
> +	for_each_pipe(dev_priv, pipe) {
> +		struct intel_crtc *crtc =
> +			intel_get_crtc_for_pipe(dev_priv, pipe);
> +		struct intel_plane *plane =
> +			to_intel_plane(crtc->base.primary);
> +
> +		if (intel_plane_mapping_ok(crtc, plane))
> +			continue;
> +
> +		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
> +			      plane->base.name);
> +		intel_plane_disable_noatomic(crtc, plane);
> +	}
>  }
>  
>  static bool intel_crtc_has_encoders(struct intel_crtc *crtc)
> @@ -14749,33 +14779,15 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  
>  		/* Disable everything but the primary plane */
>  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> -				continue;
> +			const struct intel_plane_state *plane_state =
> +				to_intel_plane_state(plane->base.state);
>  
> -			trace_intel_disable_plane(&plane->base, crtc);
> -			plane->disable_plane(plane, crtc);
> +			if (plane_state->base.visible &&
> +			    plane->base.type != DRM_PLANE_TYPE_PRIMARY)
> +				intel_plane_disable_noatomic(crtc, plane);
>  		}
>  	}
>  
> -	/* We need to sanitize the plane -> pipe mapping first because this will
> -	 * disable the crtc (and hence change the state) if it is wrong. Note
> -	 * that gen4+ has a fixed plane -> pipe mapping.  */
> -	if (INTEL_GEN(dev_priv) < 4 && !intel_check_plane_mapping(crtc)) {
> -		bool plane;
> -
> -		DRM_DEBUG_KMS("[CRTC:%d:%s] wrong plane connection detected!\n",
> -			      crtc->base.base.id, crtc->base.name);
> -
> -		/* Pipe has the wrong plane attached and the plane is active.
> -		 * Temporarily change the plane mapping and disable everything
> -		 * ...  */
> -		plane = crtc->plane;
> -		crtc->base.primary->state->visible = true;
> -		crtc->plane = !plane;
> -		intel_crtc_disable_noatomic(&crtc->base, ctx);
> -		crtc->plane = plane;
> -	}
> -
>  	/* Adjust the state of the output pipe according to whether we
>  	 * have active connectors/encoders. */
>  	if (crtc->active && !intel_crtc_has_encoders(crtc))
> @@ -14883,14 +14895,18 @@ void i915_redisable_vga(struct drm_i915_private *dev_priv)
>  /* FIXME read out full plane state for all planes */
>  static void readout_plane_state(struct intel_crtc *crtc)
>  {
> -	struct intel_plane *primary = to_intel_plane(crtc->base.primary);
> -	bool visible;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_crtc_state *crtc_state =
> +		to_intel_crtc_state(crtc->base.state);
> +	struct intel_plane *plane;
>  
> -	visible = crtc->active && primary->get_hw_state(primary);
> +	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> +		struct intel_plane_state *plane_state =
> +			to_intel_plane_state(plane->base.state);
> +		bool visible = plane->get_hw_state(plane);
>  
> -	intel_set_plane_visible(to_intel_crtc_state(crtc->base.state),
> -				to_intel_plane_state(primary->base.state),
> -				visible);
> +		intel_set_plane_visible(crtc_state, plane_state, visible);
> +	}
>  }
>  
>  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> @@ -15079,6 +15095,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  	/* HW state is read out, now we need to sanitize this mess. */
>  	get_encoder_power_domains(dev_priv);
>  
> +	intel_sanitize_plane_mapping(dev_priv);
> +
>  	for_each_intel_encoder(dev, encoder) {
>  		intel_sanitize_encoder(encoder);
>  	}
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux