[Intel-gfx] [RFC PATCH 0/2] suppress the wrong long hotplug events

[Intel-gfx] [RFC PATCH 0/2] suppress the wrong long hotplug events

[Vinod Govindapillai]

Monitors like LG 27UL650-W, 27UK850 goes into power sleep state
and generates long duration hotplug events even when the monitor
is connected for display. Here is a proposal to detect and
suppress such hotplug events by "sleep" for 2 secs for power
state monitor become available before enable atomic commit.
A debugfs entry is created to enable the suppression of the
hotplug event in such scenarios.

Cc: Imre Deak <imre.deak@intel.com>

Mohammed Khajapasha (2):
  drm/i915/display: Add disable wait time for power state connector
  drm/i915/display: Add sleep for power state connector

 .../gpu/drm/i915/display/intel_connector.c    |  3 +
 drivers/gpu/drm/i915/display/intel_display.c  | 80 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h  |  8 ++
 .../drm/i915/display/intel_display_debugfs.c  | 58 ++++++++++++++
 .../drm/i915/display/intel_display_debugfs.h  |  7 ++
 .../drm/i915/display/intel_display_types.h    |  2 +
 drivers/gpu/drm/i915/i915_drv.h               |  2 +
 7 files changed, 160 insertions(+)

-- 
2.25.1

[Intel-gfx] [RFC PATCH 1/2] drm/i915/display: Add disable wait time for power state connector

[Vinod Govindapillai]

From: Mohammed Khajapasha <mohammed.khajapasha@intel.com>

Add connector disable wait time for a power state connector
for monitor power sleep state.

Monitors like LG 27UL650-W, 27UK850 goes into power sleep state
and generates long duration hotplug events even the monitor
connected for display, create a debugfs entry to enable sleep
while monitor is in power sleep state with hotplug event.

Signed-off-by: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
---
 .../gpu/drm/i915/display/intel_connector.c    |  3 +
 .../drm/i915/display/intel_display_debugfs.c  | 58 +++++++++++++++++++
 .../drm/i915/display/intel_display_debugfs.h  |  7 +++
 .../drm/i915/display/intel_display_types.h    |  2 +
 drivers/gpu/drm/i915/i915_drv.h               |  2 +
 5 files changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index c65f95a9a1ec..d7ad62df30e3 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -126,6 +126,9 @@ int intel_connector_register(struct drm_connector *connector)
 
 	intel_connector_debugfs_add(intel_connector);
 
+	intel_connector->disabled_time =
+		get_jiffies_64() - msecs_to_jiffies(MSEC_PER_SEC * 10);
+
 	return 0;
 
 err_backlight:
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 41b81d5dd5f4..e3fc42b53ea9 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -26,6 +26,17 @@
 #include "intel_psr.h"
 #include "intel_sprite.h"
 
+struct {
+	/* manufacturer and product code of connector from edid data */
+	u8 edid_manuf_code[2];
+	u8 edid_prod_code[2];
+} wakeup_hpd_monitor_list[] = {
+	/* LG 27UL650-W, 27UK850 */
+	{{0x1e, 0x6d}, {0x06, 0x77}},
+	{{0x1e, 0x6d}, {0x07, 0x77}},
+	{{0x26, 0xcd}, {0x40, 0x66}},
+};
+
 static inline struct drm_i915_private *node_to_i915(struct drm_info_node *node)
 {
 	return to_i915(node->minor->dev);
@@ -2021,6 +2032,52 @@ static const struct file_operations i915_fifo_underrun_reset_ops = {
 	.llseek = default_llseek,
 };
 
+bool intel_connector_need_suppress_wakeup_hpd(struct intel_connector *connector)
+{
+	int i;
+	struct edid *edid = connector->detect_edid;
+
+	if (!edid)
+		return false;
+
+	for (i = 0; i < ARRAY_SIZE(wakeup_hpd_monitor_list); i++) {
+		if (*((u16 *)&wakeup_hpd_monitor_list[i].edid_manuf_code) !=
+		    *((u16 *)&edid->mfg_id))
+			continue;
+
+		if (*((u16 *)&wakeup_hpd_monitor_list[i].edid_prod_code) !=
+		    *((u16 *)&edid->prod_code))
+			continue;
+
+		return true;
+	}
+
+	return false;
+}
+
+static int i915_suppress_wakeup_hpd_set(void *data, u64 val)
+{
+	struct drm_i915_private *i915 = data;
+
+	drm_dbg(&i915->drm, "Suppress wakeup HPDs enabled: %s\n", yesno(val));
+
+	i915->hotplug.suppress_wakeup_hpd_enabled = val;
+
+	return 0;
+}
+
+static int i915_suppress_wakeup_hpd_get(void *data, u64 *val)
+{
+	struct drm_i915_private *i915 = data;
+
+	*val = i915->hotplug.suppress_wakeup_hpd_enabled;
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_suppress_wakeup_hpd_fops, i915_suppress_wakeup_hpd_get,
+			i915_suppress_wakeup_hpd_set, "%llu\n");
+
 static const struct drm_info_list intel_display_debugfs_list[] = {
 	{"i915_frontbuffer_tracking", i915_frontbuffer_tracking, 0},
 	{"i915_ips_status", i915_ips_status, 0},
@@ -2055,6 +2112,7 @@ static const struct {
 	{"i915_ipc_status", &i915_ipc_status_fops},
 	{"i915_drrs_ctl", &i915_drrs_ctl_fops},
 	{"i915_edp_psr_debug", &i915_edp_psr_debug_fops},
+	{"i915_suppress_wakeup_hpd", &i915_suppress_wakeup_hpd_fops}
 };
 
 void intel_display_debugfs_register(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.h b/drivers/gpu/drm/i915/display/intel_display_debugfs.h
index d3a79c07c384..58be26fcdf46 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.h
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.h
@@ -6,6 +6,8 @@
 #ifndef __INTEL_DISPLAY_DEBUGFS_H__
 #define __INTEL_DISPLAY_DEBUGFS_H__
 
+#include <linux/types.h>
+
 struct drm_crtc;
 struct drm_i915_private;
 struct intel_connector;
@@ -14,10 +16,15 @@ struct intel_connector;
 void intel_display_debugfs_register(struct drm_i915_private *i915);
 void intel_connector_debugfs_add(struct intel_connector *connector);
 void intel_crtc_debugfs_add(struct drm_crtc *crtc);
+bool intel_connector_need_suppress_wakeup_hpd(struct intel_connector *connector);
 #else
 static inline void intel_display_debugfs_register(struct drm_i915_private *i915) {}
 static inline void intel_connector_debugfs_add(struct intel_connector *connector) {}
 static inline void intel_crtc_debugfs_add(struct drm_crtc *crtc) {}
+static inline bool intel_connector_need_suppress_wakeup_hpd(struct intel_connector *connector)
+{
+	return false;
+}
 #endif
 
 #endif /* __INTEL_DISPLAY_DEBUGFS_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 5e8d7394a394..deac7cea82c7 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -538,6 +538,8 @@ struct intel_connector {
 	struct work_struct modeset_retry_work;
 
 	struct intel_hdcp hdcp;
+	/* Timestamp when the connector got disabled */
+	u64 disabled_time;
 };
 
 struct intel_digital_connector_state {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 943267393ecb..522c9a278172 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -145,6 +145,8 @@ struct i915_hotplug {
 	 * blocked behind the non-DP one.
 	 */
 	struct workqueue_struct *dp_wq;
+
+	bool suppress_wakeup_hpd_enabled;
 };
 
 #define I915_GEM_GPU_DOMAINS \
-- 
2.25.1

[Intel-gfx] [RFC PATCH 2/2] drm/i915/display: Add sleep for power state connector

[Vinod Govindapillai]

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(+)

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,
-- 
2.25.1

[Intel-gfx] ✗ Fi.CI.BUILD: failure for suppress the wrong long hotplug events

[Patchwork]

== Series Details ==

Series: suppress the wrong long hotplug events
URL   : https://patchwork.freedesktop.org/series/101366/
State : failure

== Summary ==

CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  DESCEND objtool
  CHK     include/generated/compile.h
  CC [M]  drivers/gpu/drm/i915/display/intel_display_debugfs.o
In file included from ./include/drm/ttm/ttm_resource.h:32,
                 from ./include/drm/ttm/ttm_device.h:30,
                 from ./drivers/gpu/drm/i915/i915_drv.h:41,
                 from drivers/gpu/drm/i915/display/intel_de.h:9,
                 from drivers/gpu/drm/i915/display/intel_display_debugfs.c:12:
drivers/gpu/drm/i915/display/intel_display_debugfs.c: In function ‘i915_suppress_wakeup_hpd_set’:
drivers/gpu/drm/i915/display/intel_display_debugfs.c:1984:60: error: implicit declaration of function ‘yesno’ [-Werror=implicit-function-declaration]
  drm_dbg(&i915->drm, "Suppress wakeup HPDs enabled: %s\n", yesno(val));
                                                            ^~~~~
./include/drm/drm_print.h:461:63: note: in definition of macro ‘drm_dbg’
  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
                                                               ^~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display_debugfs.c:1984:22: error: format ‘%s’ expects argument of type ‘char *’, but argument 4 has type ‘int’ [-Werror=format=]
  drm_dbg(&i915->drm, "Suppress wakeup HPDs enabled: %s\n", yesno(val));
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~~~
./include/drm/drm_print.h:461:56: note: in definition of macro ‘drm_dbg’
  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
                                                        ^~~
cc1: all warnings being treated as errors
scripts/Makefile.build:288: recipe for target 'drivers/gpu/drm/i915/display/intel_display_debugfs.o' failed
make[4]: *** [drivers/gpu/drm/i915/display/intel_display_debugfs.o] Error 1
scripts/Makefile.build:550: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:550: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:550: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1831: recipe for target 'drivers' failed
make: *** [drivers] Error 2

Re: [Intel-gfx] [RFC PATCH 0/2] suppress the wrong long hotplug events

[Jani Nikula]

On Tue, 15 Mar 2022, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> Monitors like LG 27UL650-W, 27UK850 goes into power sleep state
> and generates long duration hotplug events even when the monitor
> is connected for display. Here is a proposal to detect and
> suppress such hotplug events by "sleep" for 2 secs for power
> state monitor become available before enable atomic commit.

I don't understand the failure mode. Please elaborate. Do we have a bug
report?

BR,
Jani.

> A debugfs entry is created to enable the suppression of the
> hotplug event in such scenarios.
>
> Cc: Imre Deak <imre.deak@intel.com>
>
> Mohammed Khajapasha (2):
>   drm/i915/display: Add disable wait time for power state connector
>   drm/i915/display: Add sleep for power state connector
>
>  .../gpu/drm/i915/display/intel_connector.c    |  3 +
>  drivers/gpu/drm/i915/display/intel_display.c  | 80 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h  |  8 ++
>  .../drm/i915/display/intel_display_debugfs.c  | 58 ++++++++++++++
>  .../drm/i915/display/intel_display_debugfs.h  |  7 ++
>  .../drm/i915/display/intel_display_types.h    |  2 +
>  drivers/gpu/drm/i915/i915_drv.h               |  2 +
>  7 files changed, 160 insertions(+)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [RFC PATCH 1/2] drm/i915/display: Add disable wait time for power state connector

[Jani Nikula]

On Tue, 15 Mar 2022, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> From: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
>
> Add connector disable wait time for a power state connector
> for monitor power sleep state.
>
> Monitors like LG 27UL650-W, 27UK850 goes into power sleep state
> and generates long duration hotplug events even the monitor
> connected for display, create a debugfs entry to enable sleep
> while monitor is in power sleep state with hotplug event.

Basically this patch adds three things that don't have any connection
between them in code, and don't actually achieve any of the things
mentioned in the commit message. Apart from adding the debugfs, but it's
not used for anything.

More comments inline.

BR,
Jani.

>
> Signed-off-by: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_connector.c    |  3 +
>  .../drm/i915/display/intel_display_debugfs.c  | 58 +++++++++++++++++++
>  .../drm/i915/display/intel_display_debugfs.h  |  7 +++
>  .../drm/i915/display/intel_display_types.h    |  2 +
>  drivers/gpu/drm/i915/i915_drv.h               |  2 +
>  5 files changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
> index c65f95a9a1ec..d7ad62df30e3 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> @@ -126,6 +126,9 @@ int intel_connector_register(struct drm_connector *connector)
>  
>  	intel_connector_debugfs_add(intel_connector);
>  
> +	intel_connector->disabled_time =
> +		get_jiffies_64() - msecs_to_jiffies(MSEC_PER_SEC * 10);
> +

In this patch, this is just unused code.

>  	return 0;
>  
>  err_backlight:
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 41b81d5dd5f4..e3fc42b53ea9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -26,6 +26,17 @@
>  #include "intel_psr.h"
>  #include "intel_sprite.h"
>  
> +struct {
> +	/* manufacturer and product code of connector from edid data */
> +	u8 edid_manuf_code[2];
> +	u8 edid_prod_code[2];
> +} wakeup_hpd_monitor_list[] = {
> +	/* LG 27UL650-W, 27UK850 */
> +	{{0x1e, 0x6d}, {0x06, 0x77}},
> +	{{0x1e, 0x6d}, {0x07, 0x77}},
> +	{{0x26, 0xcd}, {0x40, 0x66}},
> +};
> +
>  static inline struct drm_i915_private *node_to_i915(struct drm_info_node *node)
>  {
>  	return to_i915(node->minor->dev);
> @@ -2021,6 +2032,52 @@ static const struct file_operations i915_fifo_underrun_reset_ops = {
>  	.llseek = default_llseek,
>  };
>  
> +bool intel_connector_need_suppress_wakeup_hpd(struct intel_connector *connector)
> +{
> +	int i;
> +	struct edid *edid = connector->detect_edid;
> +
> +	if (!edid)
> +		return false;
> +
> +	for (i = 0; i < ARRAY_SIZE(wakeup_hpd_monitor_list); i++) {
> +		if (*((u16 *)&wakeup_hpd_monitor_list[i].edid_manuf_code) !=
> +		    *((u16 *)&edid->mfg_id))
> +			continue;
> +
> +		if (*((u16 *)&wakeup_hpd_monitor_list[i].edid_prod_code) !=
> +		    *((u16 *)&edid->prod_code))
> +			continue;
> +
> +		return true;
> +	}
> +
> +	return false;
> +}

Please let's not duplicate the EDID quirk mechanism from drm_edid.c.

> +
> +static int i915_suppress_wakeup_hpd_set(void *data, u64 val)
> +{
> +	struct drm_i915_private *i915 = data;
> +
> +	drm_dbg(&i915->drm, "Suppress wakeup HPDs enabled: %s\n", yesno(val));
> +
> +	i915->hotplug.suppress_wakeup_hpd_enabled = val;
> +
> +	return 0;
> +}
> +
> +static int i915_suppress_wakeup_hpd_get(void *data, u64 *val)
> +{
> +	struct drm_i915_private *i915 = data;
> +
> +	*val = i915->hotplug.suppress_wakeup_hpd_enabled;
> +
> +	return 0;
> +}

So this debugfs file is completely separated from the quirk thing above,
which seems odd. Up to the caller to handle all of them?

> +
> +DEFINE_SIMPLE_ATTRIBUTE(i915_suppress_wakeup_hpd_fops, i915_suppress_wakeup_hpd_get,
> +			i915_suppress_wakeup_hpd_set, "%llu\n");
> +
>  static const struct drm_info_list intel_display_debugfs_list[] = {
>  	{"i915_frontbuffer_tracking", i915_frontbuffer_tracking, 0},
>  	{"i915_ips_status", i915_ips_status, 0},
> @@ -2055,6 +2112,7 @@ static const struct {
>  	{"i915_ipc_status", &i915_ipc_status_fops},
>  	{"i915_drrs_ctl", &i915_drrs_ctl_fops},
>  	{"i915_edp_psr_debug", &i915_edp_psr_debug_fops},
> +	{"i915_suppress_wakeup_hpd", &i915_suppress_wakeup_hpd_fops}
>  };
>  
>  void intel_display_debugfs_register(struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.h b/drivers/gpu/drm/i915/display/intel_display_debugfs.h
> index d3a79c07c384..58be26fcdf46 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.h
> @@ -6,6 +6,8 @@
>  #ifndef __INTEL_DISPLAY_DEBUGFS_H__
>  #define __INTEL_DISPLAY_DEBUGFS_H__
>  
> +#include <linux/types.h>
> +
>  struct drm_crtc;
>  struct drm_i915_private;
>  struct intel_connector;
> @@ -14,10 +16,15 @@ struct intel_connector;
>  void intel_display_debugfs_register(struct drm_i915_private *i915);
>  void intel_connector_debugfs_add(struct intel_connector *connector);
>  void intel_crtc_debugfs_add(struct drm_crtc *crtc);
> +bool intel_connector_need_suppress_wakeup_hpd(struct intel_connector *connector);
>  #else
>  static inline void intel_display_debugfs_register(struct drm_i915_private *i915) {}
>  static inline void intel_connector_debugfs_add(struct intel_connector *connector) {}
>  static inline void intel_crtc_debugfs_add(struct drm_crtc *crtc) {}
> +static inline bool intel_connector_need_suppress_wakeup_hpd(struct intel_connector *connector)
> +{
> +	return false;
> +}

A functional feature should not depend on CONFIG_DEBUG_FS.

>  #endif
>  
>  #endif /* __INTEL_DISPLAY_DEBUGFS_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 5e8d7394a394..deac7cea82c7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -538,6 +538,8 @@ struct intel_connector {
>  	struct work_struct modeset_retry_work;
>  
>  	struct intel_hdcp hdcp;
> +	/* Timestamp when the connector got disabled */
> +	u64 disabled_time;
>  };
>  
>  struct intel_digital_connector_state {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 943267393ecb..522c9a278172 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -145,6 +145,8 @@ struct i915_hotplug {
>  	 * blocked behind the non-DP one.
>  	 */
>  	struct workqueue_struct *dp_wq;
> +
> +	bool suppress_wakeup_hpd_enabled;
>  };
>  
>  #define I915_GEM_GPU_DOMAINS \

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915/display: Add sleep for power state connector

[Jani Nikula]

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,

-- 
Jani Nikula, Intel Open Source Graphics Center