Re: [Intel-gfx] [PATCH] drm/i915: Avoid HPD poll detect triggering a new detect cycle
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
> > Reported-and-tested-by: wangqr < wqr@gmail.com>
> > Cc: Val Kulkov
> > Cc: wangqr < wqr@gmail.com>
> > Cc: Ville Syrjälä
> > Signed-off-by: Imre Deak
> > ---
> > 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 ?
Re: [Intel-gfx] [PATCH] drm/i915: Avoid HPD poll detect triggering a new detect cycle
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
> Reported-and-tested-by: wangqr < wqr@gmail.com>
> Cc: Val Kulkov
> Cc: wangqr < wqr@gmail.com>
> Cc: Ville Syrjälä
> Signed-off-by: Imre Deak
> ---
> 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.
> + 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)
>
Re: [Intel-gfx] [PATCH] drm/i915: Avoid HPD poll detect triggering a new detect cycle
On Mon, 28 Oct 2019 at 07:02, 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
> Reported-and-tested-by: wangqr < wqr@gmail.com>
> Cc: Val Kulkov
> Cc: wangqr < wqr@gmail.com>
> Cc: Ville Syrjälä
> Signed-off-by: Imre Deak
The patch has been tested with linux-drm-tip-git
5.4.865162.dd5bccfa3b5d-1 on Eglobal NUC Fanless Mini PC Intel N3150
and Intel NUC D34010WYK. In both cases, the reported problem was no
longer observed.
Tested-by: Val Kulkov
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Avoid HPD poll detect triggering a new detect cycle
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
Reported-and-tested-by: wangqr < wqr@gmail.com>
Cc: Val Kulkov
Cc: wangqr < wqr@gmail.com>
Cc: Ville Syrjälä
Signed-off-by: Imre Deak
---
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)
+ 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
___
