diff mbox series

[RFC,2/2] drm/i915/display: Add sleep for power state connector

Message ID 20220314225837.42816-3-vinod.govindapillai@intel.com (mailing list archive)
State New, archived
Headers show
Series suppress the wrong long hotplug events | expand

Commit Message

Vinod Govindapillai March 14, 2022, 10:58 p.m. UTC 
From: Mohammed Khajapasha <mohammed.khajapasha@intel.com>

Add 2sec sleep for power state connector when a monitor
is in power sleep state before atomic commit enable.

Monitors like LG 27UL650-W, 27UK850 goes into power
sleep state and generates long duration hotplug events
even the monitor connected for display, sleep for 2sec
for power state monitor become available before enable
atomic commit.

Signed-off-by: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 80 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h |  8 ++
 2 files changed, 88 insertions(+)

Comments

Jani Nikula March 16, 2022, 8:28 a.m. UTC | #1 
On Tue, 15 Mar 2022, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> From: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
>
> Add 2sec sleep for power state connector when a monitor
> is in power sleep state before atomic commit enable.

We're absolutely not adding a sleep like this.

> Monitors like LG 27UL650-W, 27UK850 goes into power
> sleep state and generates long duration hotplug events
> even the monitor connected for display, sleep for 2sec
> for power state monitor become available before enable
> atomic commit.

Again, will need a better description of the failure mode and/or a
detailed bug report to even suggest a better alternative.

BR,
Jani.

>
> Signed-off-by: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 80 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h |  8 ++
>  2 files changed, 88 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 54db81c2cce6..a793f4234460 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -224,6 +224,81 @@ static int intel_compute_global_watermarks(struct intel_atomic_state *state)
>  	return 0;
>  }
>  
> +static void
> +intel_connectors_wakeup_hpd_suppress(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	struct i915_hotplug *hpd = &i915->hotplug;
> +	bool do_delay = false;
> +	struct intel_connector *connector;
> +	struct intel_digital_connector_state *conn_state;
> +	int i;
> +
> +	if (!hpd->suppress_wakeup_hpd_enabled)
> +		return;
> +
> +	for_each_new_intel_connector_in_state(state, connector,
> +					      conn_state, i) {
> +		struct intel_crtc *crtc = to_intel_crtc(conn_state->base.crtc);
> +		struct intel_crtc_state *crtc_state;
> +
> +		if (!crtc || !intel_connector_needs_modeset(state,
> +							    &connector->base))
> +			continue;
> +
> +		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> +		if (!crtc_state->hw.active)
> +			continue;
> +
> +		if (!intel_connector_need_suppress_wakeup_hpd(connector))
> +			continue;
> +
> +		if (time_is_before_jiffies64(connector->disabled_time +
> +					     msecs_to_jiffies(MSEC_PER_SEC * 10))) {
> +			drm_dbg_kms(&i915->drm,
> +				    "[CONNECTOR:%d:%s] Suppress wakeup HPD for 2 secs\n",
> +				    connector->base.base.id, connector->base.name);
> +			do_delay = true;
> +		}
> +	}
> +
> +	if (do_delay)
> +		msleep(2 * MSEC_PER_SEC);
> +}
> +
> +static void
> +intel_connectors_wakeup_hpd_track_disabling(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	struct i915_hotplug *hpd = &i915->hotplug;
> +	struct intel_connector *connector;
> +	struct intel_digital_connector_state *conn_state;
> +	int i;
> +
> +	if (!hpd->suppress_wakeup_hpd_enabled)
> +		return;
> +
> +	for_each_old_intel_connector_in_state(state, connector,
> +					      conn_state, i) {
> +		struct intel_crtc *crtc = to_intel_crtc(conn_state->base.crtc);
> +		struct intel_crtc_state *crtc_state;
> +
> +		if (!crtc || !intel_connector_needs_modeset(state,
> +							    &connector->base))
> +			continue;
> +
> +		crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> +		if (!crtc_state->hw.active)
> +			continue;
> +
> +		drm_dbg_kms(&i915->drm,
> +			    "[CONNECTOR:%d:%s] Update disabled time for wakeup HPD handling\n",
> +			    connector->base.base.id, connector->base.name);
> +
> +		connector->disabled_time = get_jiffies_64();
> +	}
> +}
> +
>  /* returns HPLL frequency in kHz */
>  int vlv_get_hpll_vco(struct drm_i915_private *dev_priv)
>  {
> @@ -8517,6 +8592,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  		}
>  	}
>  
> +	intel_connectors_wakeup_hpd_track_disabling(state);
> +
>  	intel_commit_modeset_disables(state);
>  
>  	/* FIXME: Eventually get rid of our crtc->config pointer */
> @@ -8560,6 +8637,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	dev_priv->display->commit_modeset_enables(state);
>  
> +	/* sleep for 2sec for power state connector become available */
> +	intel_connectors_wakeup_hpd_suppress(state);
> +
>  	intel_encoders_update_complete(state);
>  
>  	if (state->modeset)
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 8513703086b7..12ecf1497b07 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -539,6 +539,14 @@ enum hpd_pin {
>  			     ((connector) = to_intel_connector((__state)->base.connectors[__i].ptr), \
>  			     (new_connector_state) = to_intel_digital_connector_state((__state)->base.connectors[__i].new_state), 1))
>  
> +#define for_each_old_intel_connector_in_state(__state, connector, old_connector_state, __i) \
> +	for ((__i) = 0; \
> +		(__i) < (__state)->base.num_connector; \
> +		(__i)++) \
> +		for_each_if((__state)->base.connectors[__i].ptr && \
> +				((connector) = to_intel_connector((__state)->base.connectors[__i].ptr), \
> +				(old_connector_state) = to_intel_digital_connector_state((__state)->base.connectors[__i].old_state), 1))
> +
>  int intel_atomic_add_affected_planes(struct intel_atomic_state *state,
>  				     struct intel_crtc *crtc);
>  u8 intel_calc_active_pipes(struct intel_atomic_state *state,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 54db81c2cce6..a793f4234460 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -224,6 +224,81 @@  static int intel_compute_global_watermarks(struct intel_atomic_state *state)
 	return 0;
 }
 
+static void
+intel_connectors_wakeup_hpd_suppress(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct i915_hotplug *hpd = &i915->hotplug;
+	bool do_delay = false;
+	struct intel_connector *connector;
+	struct intel_digital_connector_state *conn_state;
+	int i;
+
+	if (!hpd->suppress_wakeup_hpd_enabled)
+		return;
+
+	for_each_new_intel_connector_in_state(state, connector,
+					      conn_state, i) {
+		struct intel_crtc *crtc = to_intel_crtc(conn_state->base.crtc);
+		struct intel_crtc_state *crtc_state;
+
+		if (!crtc || !intel_connector_needs_modeset(state,
+							    &connector->base))
+			continue;
+
+		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
+		if (!crtc_state->hw.active)
+			continue;
+
+		if (!intel_connector_need_suppress_wakeup_hpd(connector))
+			continue;
+
+		if (time_is_before_jiffies64(connector->disabled_time +
+					     msecs_to_jiffies(MSEC_PER_SEC * 10))) {
+			drm_dbg_kms(&i915->drm,
+				    "[CONNECTOR:%d:%s] Suppress wakeup HPD for 2 secs\n",
+				    connector->base.base.id, connector->base.name);
+			do_delay = true;
+		}
+	}
+
+	if (do_delay)
+		msleep(2 * MSEC_PER_SEC);
+}
+
+static void
+intel_connectors_wakeup_hpd_track_disabling(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct i915_hotplug *hpd = &i915->hotplug;
+	struct intel_connector *connector;
+	struct intel_digital_connector_state *conn_state;
+	int i;
+
+	if (!hpd->suppress_wakeup_hpd_enabled)
+		return;
+
+	for_each_old_intel_connector_in_state(state, connector,
+					      conn_state, i) {
+		struct intel_crtc *crtc = to_intel_crtc(conn_state->base.crtc);
+		struct intel_crtc_state *crtc_state;
+
+		if (!crtc || !intel_connector_needs_modeset(state,
+							    &connector->base))
+			continue;
+
+		crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
+		if (!crtc_state->hw.active)
+			continue;
+
+		drm_dbg_kms(&i915->drm,
+			    "[CONNECTOR:%d:%s] Update disabled time for wakeup HPD handling\n",
+			    connector->base.base.id, connector->base.name);
+
+		connector->disabled_time = get_jiffies_64();
+	}
+}
+
 /* returns HPLL frequency in kHz */
 int vlv_get_hpll_vco(struct drm_i915_private *dev_priv)
 {
@@ -8517,6 +8592,8 @@  static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 		}
 	}
 
+	intel_connectors_wakeup_hpd_track_disabling(state);
+
 	intel_commit_modeset_disables(state);
 
 	/* FIXME: Eventually get rid of our crtc->config pointer */
@@ -8560,6 +8637,9 @@  static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	dev_priv->display->commit_modeset_enables(state);
 
+	/* sleep for 2sec for power state connector become available */
+	intel_connectors_wakeup_hpd_suppress(state);
+
 	intel_encoders_update_complete(state);
 
 	if (state->modeset)
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 8513703086b7..12ecf1497b07 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -539,6 +539,14 @@  enum hpd_pin {
 			     ((connector) = to_intel_connector((__state)->base.connectors[__i].ptr), \
 			     (new_connector_state) = to_intel_digital_connector_state((__state)->base.connectors[__i].new_state), 1))
 
+#define for_each_old_intel_connector_in_state(__state, connector, old_connector_state, __i) \
+	for ((__i) = 0; \
+		(__i) < (__state)->base.num_connector; \
+		(__i)++) \
+		for_each_if((__state)->base.connectors[__i].ptr && \
+				((connector) = to_intel_connector((__state)->base.connectors[__i].ptr), \
+				(old_connector_state) = to_intel_digital_connector_state((__state)->base.connectors[__i].old_state), 1))
+
 int intel_atomic_add_affected_planes(struct intel_atomic_state *state,
 				     struct intel_crtc *crtc);
 u8 intel_calc_active_pipes(struct intel_atomic_state *state,