[Intel-gfx] [PATCH] drm/i915: Avoid HPD poll detect triggering a new detect cycle

[Imre Deak]

On Mon, Oct 28, 2019 at 07:45:09PM +0200, Ville Syrjälä wrote:
> On Mon, Oct 28, 2019 at 01:00:31PM +0200, Imre Deak wrote:
> > For the HPD interrupt functionality the HW depends on power wells in the
> > display core domain to be on. Accordingly when enabling these power
> > wells the HPD polling logic will force an HPD detection cycle to account
> > for hotplug events that may have happened when such a power well was
> > off.
> > 
> > Thus a detect cycle started by polling could start a new detect cycle if
> > a power well in the display core domain gets enabled during detect and
> > stays enabled after detect completes. That in turn can lead to a
> > detection cycle runaway.
> > 
> > To prevent re-triggering a poll-detect cycle make sure we drop all power
> > references we acquired during detect synchronously by the end of detect.
> > This will let the poll-detect logic continue with polling (matching the
> > off state of the corresponding power wells) instead of scheduling a new
> > detection cycle.
> > 
> > Fixes: 6cfe7ec02e85 ("drm/i915: Remove the unneeded AUX power ref from intel_dp_detect()")
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112125
> > Reported-by: Val Kulkov <val.kulkov at gmail.com>
> > Reported-and-tested-by: wangqr < wqr.prg at gmail.com>
> > Cc: Val Kulkov <val.kulkov at gmail.com>
> > Cc: wangqr < wqr.prg at gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crt.c  |  7 +++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c   | 24 ++++++++++++++---------
> >  drivers/gpu/drm/i915/display/intel_hdmi.c |  6 ++++++
> >  3 files changed, 28 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> > index ff6126ea793c..834bf1d43bb8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> > @@ -864,6 +864,13 @@ intel_crt_detect(struct drm_connector *connector,
> >  
> >  out:
> >  	intel_display_power_put(dev_priv, intel_encoder->power_domain, wakeref);
> > +
> > +	/*
> > +	 * Make sure the refs for power wells enabled during detect are
> > +	 * dropped to avoid a new detect cycle triggered by HPD polling.
> > +	 */
> > +	intel_display_power_flush_work(dev_priv);
> > +
> >  	return status;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 86989ec25bc6..f4e0ec05d7c9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5600,6 +5600,7 @@ intel_dp_detect(struct drm_connector *connector,
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  	struct intel_encoder *encoder = &dig_port->base;
> >  	enum drm_connector_status status;
> > +	int err = 0;
> >  
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> >  		      connector->base.id, connector->name);
> > @@ -5626,7 +5627,7 @@ intel_dp_detect(struct drm_connector *connector,
> >  							intel_dp->is_mst);
> >  		}
> >  
> > -		goto out;
> > +		goto out_update_edid;
> >  	}
> >  
> >  	if (intel_dp->reset_link_params) {
> > @@ -5654,7 +5655,7 @@ intel_dp_detect(struct drm_connector *connector,
> >  		 * with EDID on it
> >  		 */
> >  		status = connector_status_disconnected;
> > -		goto out;
> > +		goto out_update_edid;
> >  	}
> >  
> >  	/*
> > @@ -5662,11 +5663,9 @@ intel_dp_detect(struct drm_connector *connector,
> >  	 * with an IRQ_HPD, so force a link status check.
> >  	 */
> >  	if (!intel_dp_is_edp(intel_dp)) {
> > -		int ret;
> > -
> > -		ret = intel_dp_retrain_link(encoder, ctx);
> > -		if (ret)
> > -			return ret;
> > +		err = intel_dp_retrain_link(encoder, ctx);
> > +		if (err)
> 
> This should probably read 
> if (err == -EDEADLK)
> 
> Also I don't think we need to change this to a goto since it just
> means we're going to retry the whole thing again, so the straight
> return should be fine.

Ok, will resend it keeping this as-is. The retry will be done without
dropping mode_config.mutex, so keeping the power refs in that case
shouldn't cause a problem.

> 
> > +			goto out_sync_power;
> >  	}
> >  
> >  	/*
> > @@ -5684,11 +5683,18 @@ intel_dp_detect(struct drm_connector *connector,
> >  
> >  	intel_dp_check_service_irq(intel_dp);
> >  
> > -out:
> > +out_update_edid:
> >  	if (status != connector_status_connected && !intel_dp->is_mst)
> >  		intel_dp_unset_edid(intel_dp);
> >  
> > -	return status;
> > +out_sync_power:
> > +	/*
> > +	 * Make sure the refs for power wells enabled during detect are
> > +	 * dropped to avoid a new detect cycle triggered by HPD polling.
> > +	 */
> > +	intel_display_power_flush_work(dev_priv);
> > +
> > +	return err ? err : status;
> >  }
> >  
> >  static void
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index b54ccbb5aad5..ff71a4da3d00 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -2626,6 +2626,12 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >  	if (status != connector_status_connected)
> >  		cec_notifier_phys_addr_invalidate(intel_hdmi->cec_notifier);
> >  
> > +	/*
> > +	 * Make sure the refs for power wells enabled during detect are
> > +	 * dropped to avoid a new detect cycle triggered by HPD polling.
> > +	 */
> > +	intel_display_power_flush_work(dev_priv);
> > +
> >  	return status;
> >  }
> >  
> > -- 
> > 2.17.1
> 
> -- 
> Ville Syrjälä
> Intel