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@xxxxxxxxx>
> > Reported-and-tested-by: wangqr < wqr.prg@xxxxxxxxx>
> > Cc: Val Kulkov <val.kulkov@xxxxxxxxx>
> > Cc: wangqr < wqr.prg@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
