Re: [PATCH v2 2/3] drm/i915: Use the correct crtc when sanitizing plane mapping

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

 



On Wed, Oct 03, 2018 at 06:12:42PM +0200, Daniel Vetter wrote:
> On Wed, Oct 03, 2018 at 05:50:17PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > When we decide that a plane is attached to the wrong pipe we try
> > to turn off said plane. However we are passing around the crtc we
> > think that the plane is supposed to be using rather than the crtc
> > it is currently using. That doesn't work all that well because
> > we may have to do vblank waits etc. and the other pipe might
> > not even be enabled here. So let's pass the plane's current crtc to
> > intel_plane_disable_noatomic() so that it can its job correctly.
> > 
> > To do that semi-cleanly we also have to change the plane readout
> > to record the plane's visibility into the bitmasks of the crtc
> > where the plane is currently enabled rather than to the crtc
> > we want to use for the plane.
> > 
> > One caveat here is that our active_planes bitmask will get confused
> > if both planes are enabled on the same pipe. Fortunately we can use
> > plane_mask to reconstruct active_planes sufficiently since
> > plane_mask still has the same meaning (is the plane visible?)
> > during readout. We also have to do the same during the initial
> > plane readout as the second plane could clear the active_planes
> > bit the first plane had already set.
> > 
> > v2: Rely on fixup_active_planes() to populate active_planes fully (Daniel)
> >     Add Daniel's proposed comment to better document why we do this
> >     Drop the redundant intel_set_plane_visible() call
> > 
> > Cc: stable@xxxxxxxxxxxxxxx # fcba862e8428 drm/i915: Have plane->get_hw_state() return the current pipe
> > Cc: stable@xxxxxxxxxxxxxxx
> > Cc: Dennis <dennis.nezic@xxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > Tested-by: Dennis <dennis.nezic@xxxxxxxxxxx>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105637
> > Fixes: b1e01595a66d ("drm/i915: Redo plane sanitation during readout")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> I have the illusion of understanding this stuff now.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> 
> But let's see whether testers and CI agree :-)

Seems to be reasonably happy :)

Picked up another tested-by from the bug report
Tested-by: Peter Nowee <peter.nowee@xxxxxxxxx>

and pushed the series to dinq. Thanks to everyone involved.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 78 +++++++++++++++++++++---------------
> >  1 file changed, 46 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d2828159f6c8..f0d004641b0d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2764,20 +2764,33 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state,
> >  
> >  	plane_state->base.visible = visible;
> >  
> > -	/* FIXME pre-g4x don't work like this */
> > -	if (visible) {
> > +	if (visible)
> >  		crtc_state->base.plane_mask |= drm_plane_mask(&plane->base);
> > -		crtc_state->active_planes |= BIT(plane->id);
> > -	} else {
> > +	else
> >  		crtc_state->base.plane_mask &= ~drm_plane_mask(&plane->base);
> > -		crtc_state->active_planes &= ~BIT(plane->id);
> > -	}
> >  
> >  	DRM_DEBUG_KMS("%s active planes 0x%x\n",
> >  		      crtc_state->base.crtc->name,
> >  		      crtc_state->active_planes);
> >  }
> >  
> > +static void fixup_active_planes(struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > +	struct drm_plane *plane;
> > +
> > +	/*
> > +	 * Active_planes aliases if multiple "primary" or cursor planes
> > +	 * have been used on the same (or wrong) pipe. plane_mask uses
> > +	 * unique ids, hence we can use that to reconstruct active_planes.
> > +	 */
> > +	crtc_state->active_planes = 0;
> > +
> > +	drm_for_each_plane_mask(plane, &dev_priv->drm,
> > +				crtc_state->base.plane_mask)
> > +		crtc_state->active_planes |= BIT(to_intel_plane(plane)->id);
> > +}
> > +
> >  static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
> >  					 struct intel_plane *plane)
> >  {
> > @@ -2787,6 +2800,7 @@ static void intel_plane_disable_noatomic(struct intel_crtc *crtc,
> >  		to_intel_plane_state(plane->base.state);
> >  
> >  	intel_set_plane_visible(crtc_state, plane_state, false);
> > +	fixup_active_planes(crtc_state);
> >  
> >  	if (plane->id == PLANE_PRIMARY)
> >  		intel_pre_disable_primary_noatomic(&crtc->base);
> > @@ -2805,7 +2819,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >  	struct drm_i915_gem_object *obj;
> >  	struct drm_plane *primary = intel_crtc->base.primary;
> >  	struct drm_plane_state *plane_state = primary->state;
> > -	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
> >  	struct intel_plane *intel_plane = to_intel_plane(primary);
> >  	struct intel_plane_state *intel_state =
> >  		to_intel_plane_state(plane_state);
> > @@ -2900,10 +2913,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >  	plane_state->fb = fb;
> >  	plane_state->crtc = &intel_crtc->base;
> >  
> > -	intel_set_plane_visible(to_intel_crtc_state(crtc_state),
> > -				to_intel_plane_state(plane_state),
> > -				true);
> > -
> >  	atomic_or(to_intel_plane(primary)->frontbuffer_bit,
> >  		  &obj->frontbuffer_bits);
> >  }
> > @@ -15494,17 +15503,6 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  	POSTING_READ(DPLL(pipe));
> >  }
> >  
> > -static bool intel_plane_mapping_ok(struct intel_crtc *crtc,
> > -				   struct intel_plane *plane)
> > -{
> > -	enum pipe pipe;
> > -
> > -	if (!plane->get_hw_state(plane, &pipe))
> > -		return true;
> > -
> > -	return pipe == crtc->pipe;
> > -}
> > -
> >  static void
> >  intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
> >  {
> > @@ -15516,13 +15514,20 @@ intel_sanitize_plane_mapping(struct drm_i915_private *dev_priv)
> >  	for_each_intel_crtc(&dev_priv->drm, crtc) {
> >  		struct intel_plane *plane =
> >  			to_intel_plane(crtc->base.primary);
> > +		struct intel_crtc *plane_crtc;
> > +		enum pipe pipe;
> >  
> > -		if (intel_plane_mapping_ok(crtc, plane))
> > +		if (!plane->get_hw_state(plane, &pipe))
> > +			continue;
> > +
> > +		if (pipe == crtc->pipe)
> >  			continue;
> >  
> >  		DRM_DEBUG_KMS("%s attached to the wrong pipe, disabling plane\n",
> >  			      plane->base.name);
> > -		intel_plane_disable_noatomic(crtc, plane);
> > +
> > +		plane_crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > +		intel_plane_disable_noatomic(plane_crtc, plane);
> >  	}
> >  }
> >  
> > @@ -15690,23 +15695,32 @@ 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)
> > +static void readout_plane_state(struct drm_i915_private *dev_priv)
> >  {
> > -	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;
> > +	struct intel_crtc *crtc;
> >  
> > -	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > +	for_each_intel_plane(&dev_priv->drm, plane) {
> >  		struct intel_plane_state *plane_state =
> >  			to_intel_plane_state(plane->base.state);
> > -		enum pipe pipe;
> > +		struct intel_crtc_state *crtc_state;
> > +		enum pipe pipe = PIPE_A;
> >  		bool visible;
> >  
> >  		visible = plane->get_hw_state(plane, &pipe);
> >  
> > +		crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > +		crtc_state = to_intel_crtc_state(crtc->base.state);
> > +
> >  		intel_set_plane_visible(crtc_state, plane_state, visible);
> >  	}
> > +
> > +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> > +		struct intel_crtc_state *crtc_state =
> > +			to_intel_crtc_state(crtc->base.state);
> > +
> > +		fixup_active_planes(crtc_state);
> > +	}
> >  }
> >  
> >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > @@ -15738,13 +15752,13 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  		if (crtc_state->base.active)
> >  			dev_priv->active_crtcs |= 1 << crtc->pipe;
> >  
> > -		readout_plane_state(crtc);
> > -
> >  		DRM_DEBUG_KMS("[CRTC:%d:%s] hw state readout: %s\n",
> >  			      crtc->base.base.id, crtc->base.name,
> >  			      enableddisabled(crtc_state->base.active));
> >  	}
> >  
> > +	readout_plane_state(dev_priv);
> > +
> >  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> >  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
> >  
> > -- 
> > 2.16.4
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux