[Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Suraj Kandpal]

On TGP, the RTC (always running) was reduced from 3MHz to 32KHz.
As a result of this change, when HPD active going low pulse or HPD IRQ
is presented and the refclk (19.2MHz) is not toggling already toggling,
there is a 60 to 90us synchronization delay which effectively reduces
the duration of the IRQ pulse to less than the programmed 500us filter
value and the hot plug interrupt is NOT registered.
Program 0xC7204 (PP_CONTROL) bit #0 to '1' to enable workaround and clear
to disable it.
Driver shall enable this WA when external display is connected and
remove WA when display is unplugged or before going into sleep to allow
CS entry.
Driver shall not enable WA when eDP is connected.
Wa_1508796671:adls

Cc: Arun Murthy <arun.r.murthy@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c  | 19 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_pps.c |  1 +
 drivers/gpu/drm/i915/i915_drv.h          |  2 ++
 3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 8e16745275f6..f93895d99fd1 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4689,6 +4689,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;
+	int32_t pp;
 
 	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
 		    connector->base.id, connector->name);
@@ -4792,6 +4793,22 @@ intel_dp_detect(struct drm_connector *connector,
 						 status,
 						 intel_dp->dpcd,
 						 intel_dp->downstream_ports);
+
+	/*
+	 * WA_150879661:adls
+	 * Driver shall enable this WA when external display is connected
+	 * and remove WA when display is unplugged
+	 */
+	if (IS_ALDERLAKE_S(dev_priv)) {
+		if (status == connector_status_connected &&
+		    !dev_priv->edp_present)
+			pp = PANEL_POWER_ON;
+		else if (status == connector_status_disconnected)
+			pp = 0;
+
+		intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
+	}
+
 	return status;
 }
 
@@ -5489,6 +5506,8 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
 	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
 		intel_dp_aux_fini(intel_dp);
 		goto fail;
+	} else {
+		dev_priv->edp_present = true;
 	}
 
 	intel_dp_set_source_rates(intel_dp);
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index 24b5b12f7732..21538338af3d 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -1726,4 +1726,5 @@ void assert_pps_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
 	I915_STATE_WARN(panel_pipe == pipe && locked,
 			"panel assertion failure, pipe %c regs locked\n",
 			pipe_name(pipe));
+
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6254aa977398..ebe16aee0ca8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -352,6 +352,8 @@ struct drm_i915_private {
 	/* The TTM device structure. */
 	struct ttm_device bdev;
 
+	bool edp_present;
+
 	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
 
 	/*
-- 
2.25.1

Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Ville Syrjälä]

On Fri, Apr 14, 2023 at 12:53:45PM +0530, Suraj Kandpal wrote:
> On TGP, the RTC (always running) was reduced from 3MHz to 32KHz.
> As a result of this change, when HPD active going low pulse or HPD IRQ
> is presented and the refclk (19.2MHz) is not toggling already toggling,
> there is a 60 to 90us synchronization delay which effectively reduces
> the duration of the IRQ pulse to less than the programmed 500us filter
> value and the hot plug interrupt is NOT registered.
> Program 0xC7204 (PP_CONTROL) bit #0 to '1' to enable workaround and clear
> to disable it.
> Driver shall enable this WA when external display is connected and
> remove WA when display is unplugged or before going into sleep to allow
> CS entry.
> Driver shall not enable WA when eDP is connected.
> Wa_1508796671:adls
> 
> Cc: Arun Murthy <arun.r.murthy@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c  | 19 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_pps.c |  1 +
>  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8e16745275f6..f93895d99fd1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4689,6 +4689,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;
> +	int32_t pp;
>  
>  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
>  		    connector->base.id, connector->name);
> @@ -4792,6 +4793,22 @@ intel_dp_detect(struct drm_connector *connector,
>  						 status,
>  						 intel_dp->dpcd,
>  						 intel_dp->downstream_ports);
> +
> +	/*
> +	 * WA_150879661:adls
> +	 * Driver shall enable this WA when external display is connected
> +	 * and remove WA when display is unplugged
> +	 */
> +	if (IS_ALDERLAKE_S(dev_priv)) {
> +		if (status == connector_status_connected &&
> +		    !dev_priv->edp_present)
> +			pp = PANEL_POWER_ON;
> +		else if (status == connector_status_disconnected)
> +			pp = 0;
> +
> +		intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
> +	}

This whole w/a is just utter nonsense. The better fix would be to 
adjust the SHPD_FILTER length. There was some internal discussion 
around this the last time it was raised, but the only clear conclusion
was to reduce the default SHPD_FILTER from 500 to 250 usec (which is
what the DP spec actually cals for) on future hw.

We already adjust that SHPD_FILTER down a little bit on some PCHs
(due to some other w/a presumably), but I think just dropping it
further to ~400 usec (if we don't want to go all the way down to
250 on old hw as well) should cover this issue.

> +
>  	return status;
>  }
>  
> @@ -5489,6 +5506,8 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
>  		intel_dp_aux_fini(intel_dp);
>  		goto fail;
> +	} else {
> +		dev_priv->edp_present = true;
>  	}
>  
>  	intel_dp_set_source_rates(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 24b5b12f7732..21538338af3d 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -1726,4 +1726,5 @@ void assert_pps_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	I915_STATE_WARN(panel_pipe == pipe && locked,
>  			"panel assertion failure, pipe %c regs locked\n",
>  			pipe_name(pipe));
> +
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6254aa977398..ebe16aee0ca8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -352,6 +352,8 @@ struct drm_i915_private {
>  	/* The TTM device structure. */
>  	struct ttm_device bdev;
>  
> +	bool edp_present;
> +
>  	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
>  
>  	/*
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Kandpal, Suraj]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Friday, April 14, 2023 1:36 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not
> detected with default filter value
> 
> On Fri, Apr 14, 2023 at 12:53:45PM +0530, Suraj Kandpal wrote:
> > On TGP, the RTC (always running) was reduced from 3MHz to 32KHz.
> > As a result of this change, when HPD active going low pulse or HPD IRQ
> > is presented and the refclk (19.2MHz) is not toggling already
> > toggling, there is a 60 to 90us synchronization delay which
> > effectively reduces the duration of the IRQ pulse to less than the
> > programmed 500us filter value and the hot plug interrupt is NOT
> registered.
> > Program 0xC7204 (PP_CONTROL) bit #0 to '1' to enable workaround and
> > clear to disable it.
> > Driver shall enable this WA when external display is connected and
> > remove WA when display is unplugged or before going into sleep to
> > allow CS entry.
> > Driver shall not enable WA when eDP is connected.
> > Wa_1508796671:adls
> >
> > Cc: Arun Murthy <arun.r.murthy@intel.com>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c  | 19 +++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_pps.c |  1 +
> >  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
> >  3 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 8e16745275f6..f93895d99fd1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4689,6 +4689,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;
> > +	int32_t pp;
> >
> >  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
> >  		    connector->base.id, connector->name); @@ -4792,6
> +4793,22 @@
> > intel_dp_detect(struct drm_connector *connector,
> >  						 status,
> >  						 intel_dp->dpcd,
> >  						 intel_dp-
> >downstream_ports);
> > +
> > +	/*
> > +	 * WA_150879661:adls
> > +	 * Driver shall enable this WA when external display is connected
> > +	 * and remove WA when display is unplugged
> > +	 */
> > +	if (IS_ALDERLAKE_S(dev_priv)) {
> > +		if (status == connector_status_connected &&
> > +		    !dev_priv->edp_present)
> > +			pp = PANEL_POWER_ON;
> > +		else if (status == connector_status_disconnected)
> > +			pp = 0;
> > +
> > +		intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
> > +	}
> 
> This whole w/a is just utter nonsense. The better fix would be to adjust the
> SHPD_FILTER length. There was some internal discussion around this the last
> time it was raised, but the only clear conclusion was to reduce the default
> SHPD_FILTER from 500 to 250 usec (which is what the DP spec actually cals
> for) on future hw.
> 
> We already adjust that SHPD_FILTER down a little bit on some PCHs (due to
> some other w/a presumably), but I think just dropping it further to ~400 usec
> (if we don't want to go all the way down to
> 250 on old hw as well) should cover this issue.
> 

Thanks for pointing that out let me check and float a fix for this Wa

Regards,
Suraj Kandpal
> > +
> >  	return status;
> >  }
> >
> > @@ -5489,6 +5506,8 @@ intel_dp_init_connector(struct intel_digital_port
> *dig_port,
> >  	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
> >  		intel_dp_aux_fini(intel_dp);
> >  		goto fail;
> > +	} else {
> > +		dev_priv->edp_present = true;
> >  	}
> >
> >  	intel_dp_set_source_rates(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> > b/drivers/gpu/drm/i915/display/intel_pps.c
> > index 24b5b12f7732..21538338af3d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > @@ -1726,4 +1726,5 @@ void assert_pps_unlocked(struct
> drm_i915_private *dev_priv, enum pipe pipe)
> >  	I915_STATE_WARN(panel_pipe == pipe && locked,
> >  			"panel assertion failure, pipe %c regs locked\n",
> >  			pipe_name(pipe));
> > +
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 6254aa977398..ebe16aee0ca8
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -352,6 +352,8 @@ struct drm_i915_private {
> >  	/* The TTM device structure. */
> >  	struct ttm_device bdev;
> >
> > +	bool edp_present;
> > +
> >  	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
> >
> >  	/*
> > --
> > 2.25.1
> 
> --
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Jani Nikula]

On Fri, 14 Apr 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> On TGP, the RTC (always running) was reduced from 3MHz to 32KHz.
> As a result of this change, when HPD active going low pulse or HPD IRQ
> is presented and the refclk (19.2MHz) is not toggling already toggling,
> there is a 60 to 90us synchronization delay which effectively reduces
> the duration of the IRQ pulse to less than the programmed 500us filter
> value and the hot plug interrupt is NOT registered.
> Program 0xC7204 (PP_CONTROL) bit #0 to '1' to enable workaround and clear
> to disable it.
> Driver shall enable this WA when external display is connected and
> remove WA when display is unplugged or before going into sleep to allow
> CS entry.
> Driver shall not enable WA when eDP is connected.
> Wa_1508796671:adls
>
> Cc: Arun Murthy <arun.r.murthy@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>

I don't know what the right fix should eventually be, but this, as it
is, is absolutely not it.

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c  | 19 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_pps.c |  1 +
>  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
>  3 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8e16745275f6..f93895d99fd1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4689,6 +4689,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;
> +	int32_t pp;

For registers, this should be u32. There isn't a single int32_t use in
the driver.

>  
>  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
>  		    connector->base.id, connector->name);
> @@ -4792,6 +4793,22 @@ intel_dp_detect(struct drm_connector *connector,
>  						 status,
>  						 intel_dp->dpcd,
>  						 intel_dp->downstream_ports);
> +
> +	/*
> +	 * WA_150879661:adls
> +	 * Driver shall enable this WA when external display is connected
> +	 * and remove WA when display is unplugged
> +	 */
> +	if (IS_ALDERLAKE_S(dev_priv)) {
> +		if (status == connector_status_connected &&
> +		    !dev_priv->edp_present)
> +			pp = PANEL_POWER_ON;
> +		else if (status == connector_status_disconnected)
> +			pp = 0;
> +
> +		intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);

You're not supposed to cook up register offsets inline in code like
that. The *PPS_BASE macros are not for general use. There's PP_CONTROL
macro for that particular register.

Why would you use magic 1 for clearing and PANEL_POWER_ON macro for
setting the bit in the rmw call?

For the most part, only the PPS code in intel_pps.c is supposed to touch
the PPS registers.

> +	}
> +
>  	return status;
>  }
>  
> @@ -5489,6 +5506,8 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
>  		intel_dp_aux_fini(intel_dp);
>  		goto fail;
> +	} else {
> +		dev_priv->edp_present = true;
>  	}
>  
>  	intel_dp_set_source_rates(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 24b5b12f7732..21538338af3d 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -1726,4 +1726,5 @@ void assert_pps_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	I915_STATE_WARN(panel_pipe == pipe && locked,
>  			"panel assertion failure, pipe %c regs locked\n",
>  			pipe_name(pipe));
> +
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6254aa977398..ebe16aee0ca8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -352,6 +352,8 @@ struct drm_i915_private {
>  	/* The TTM device structure. */
>  	struct ttm_device bdev;
>  
> +	bool edp_present;

Please don't add global state flags that duplicate info.

And when you actually need to, struct drm_i915_private is no longer the
dumping ground for random info anyway.

BR,
Jani.

> +
>  	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
>  
>  	/*

-- 
Jani Nikula, Intel Open Source Graphics Center

[Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Patchwork]

== Series Details ==

Series: drm/i915/display: PCH display HPD IRQ is not detected with default filter value
URL   : https://patchwork.freedesktop.org/series/116468/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/display/intel_dp.o
drivers/gpu/drm/i915/display/intel_dp.c: In function ‘intel_dp_detect’:
drivers/gpu/drm/i915/display/intel_dp.c:4856:9: error: ‘PANEL_POWER_ON’ undeclared (first use in this function); did you mean ‘MIPI_SEQ_POWER_ON’?
 4856 |    pp = PANEL_POWER_ON;
      |         ^~~~~~~~~~~~~~
      |         MIPI_SEQ_POWER_ON
drivers/gpu/drm/i915/display/intel_dp.c:4856:9: note: each undeclared identifier is reported only once for each function it appears in
In file included from drivers/gpu/drm/i915/display/g4x_dp.h:11,
                 from drivers/gpu/drm/i915/display/intel_dp.c:46:
drivers/gpu/drm/i915/display/intel_dp.c:4860:32: error: ‘PCH_PPS_BASE’ undeclared (first use in this function); did you mean ‘PCI_IO_BASE’?
 4860 |   intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
      |                                ^~~~~~~~~~~~
./drivers/gpu/drm/i915/i915_reg_defs.h:162:47: note: in definition of macro ‘_MMIO’
  162 | #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
      |                                               ^
make[5]: *** [scripts/Makefile.build:252: drivers/gpu/drm/i915/display/intel_dp.o] Error 1
make[4]: *** [scripts/Makefile.build:494: drivers/gpu/drm/i915] Error 2
make[3]: *** [scripts/Makefile.build:494: drivers/gpu/drm] Error 2
make[2]: *** [scripts/Makefile.build:494: drivers/gpu] Error 2
make[1]: *** [scripts/Makefile.build:494: drivers] Error 2
make: *** [Makefile:2025: .] Error 2
Build failed, no error log produced

Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Kandpal, Suraj]

> 
> On Fri, 14 Apr 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> > On TGP, the RTC (always running) was reduced from 3MHz to 32KHz.
> > As a result of this change, when HPD active going low pulse or HPD IRQ
> > is presented and the refclk (19.2MHz) is not toggling already
> > toggling, there is a 60 to 90us synchronization delay which
> > effectively reduces the duration of the IRQ pulse to less than the
> > programmed 500us filter value and the hot plug interrupt is NOT
> registered.
> > Program 0xC7204 (PP_CONTROL) bit #0 to '1' to enable workaround and
> > clear to disable it.
> > Driver shall enable this WA when external display is connected and
> > remove WA when display is unplugged or before going into sleep to
> > allow CS entry.
> > Driver shall not enable WA when eDP is connected.
> > Wa_1508796671:adls
> >
> > Cc: Arun Murthy <arun.r.murthy@intel.com>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> 
> I don't know what the right fix should eventually be, but this, as it is, is
> absolutely not it.

I guess we should open a discussion on how we should go ahead implementing this fix

> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c  | 19 +++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_pps.c |  1 +
> >  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
> >  3 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 8e16745275f6..f93895d99fd1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4689,6 +4689,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;
> > +	int32_t pp;
> 
> For registers, this should be u32. There isn't a single int32_t use in the driver.
> 
> >
> >  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
> >  		    connector->base.id, connector->name); @@ -4792,6
> +4793,22 @@
> > intel_dp_detect(struct drm_connector *connector,
> >  						 status,
> >  						 intel_dp->dpcd,
> >  						 intel_dp-
> >downstream_ports);
> > +
> > +	/*
> > +	 * WA_150879661:adls
> > +	 * Driver shall enable this WA when external display is connected
> > +	 * and remove WA when display is unplugged
> > +	 */
> > +	if (IS_ALDERLAKE_S(dev_priv)) {
> > +		if (status == connector_status_connected &&
> > +		    !dev_priv->edp_present)
> > +			pp = PANEL_POWER_ON;
> > +		else if (status == connector_status_disconnected)
> > +			pp = 0;
> > +
> > +		intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
> 
> You're not supposed to cook up register offsets inline in code like that. The
> *PPS_BASE macros are not for general use. There's PP_CONTROL macro for
> that particular register.
> 

According to the WA we need to write in the register PP_CONTROL 0xC7204
But the PP_CONTROL macro depends on the value assigned to mmio_base value
In pps struct.

> Why would you use magic 1 for clearing and PANEL_POWER_ON macro for
> setting the bit in the rmw call?
> 

Since I wanted to set the first bit to be set as 0 or 1 hence used clear value as 1 to just clear
The LSB and then intel_de_rmw OR's the read value with provided value.

> For the most part, only the PPS code in intel_pps.c is supposed to touch the
> PPS registers.
> 
> > +	}
> > +
> >  	return status;
> >  }
> >
> > @@ -5489,6 +5506,8 @@ intel_dp_init_connector(struct intel_digital_port
> *dig_port,
> >  	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
> >  		intel_dp_aux_fini(intel_dp);
> >  		goto fail;
> > +	} else {
> > +		dev_priv->edp_present = true;
> >  	}
> >
> >  	intel_dp_set_source_rates(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> > b/drivers/gpu/drm/i915/display/intel_pps.c
> > index 24b5b12f7732..21538338af3d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > @@ -1726,4 +1726,5 @@ void assert_pps_unlocked(struct
> drm_i915_private *dev_priv, enum pipe pipe)
> >  	I915_STATE_WARN(panel_pipe == pipe && locked,
> >  			"panel assertion failure, pipe %c regs locked\n",
> >  			pipe_name(pipe));
> > +
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 6254aa977398..ebe16aee0ca8
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -352,6 +352,8 @@ struct drm_i915_private {
> >  	/* The TTM device structure. */
> >  	struct ttm_device bdev;
> >
> > +	bool edp_present;
> 
> Please don't add global state flags that duplicate info.
> 
> And when you actually need to, struct drm_i915_private is no longer the
> dumping ground for random info anyway.
> 


This edp_present variable was to check if edp is connected to machine
But other than iterate over all connectors to see if edp is present I couldn't find a way
Making me think drm_i915_private is the place where I can put this variable

Regards,
Suraj Kandpal

> BR,
> Jani.
> 
> > +
> >  	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
> >
> >  	/*
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Jani Nikula]

On Fri, 14 Apr 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> 
>> On Fri, 14 Apr 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
>> > On TGP, the RTC (always running) was reduced from 3MHz to 32KHz.
>> > As a result of this change, when HPD active going low pulse or HPD IRQ
>> > is presented and the refclk (19.2MHz) is not toggling already
>> > toggling, there is a 60 to 90us synchronization delay which
>> > effectively reduces the duration of the IRQ pulse to less than the
>> > programmed 500us filter value and the hot plug interrupt is NOT
>> registered.
>> > Program 0xC7204 (PP_CONTROL) bit #0 to '1' to enable workaround and
>> > clear to disable it.
>> > Driver shall enable this WA when external display is connected and
>> > remove WA when display is unplugged or before going into sleep to
>> > allow CS entry.
>> > Driver shall not enable WA when eDP is connected.
>> > Wa_1508796671:adls
>> >
>> > Cc: Arun Murthy <arun.r.murthy@intel.com>
>> > Cc: Uma Shankar <uma.shankar@intel.com>
>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> 
>> I don't know what the right fix should eventually be, but this, as it is, is
>> absolutely not it.
>
> I guess we should open a discussion on how we should go ahead implementing this fix

Ville's reply is relevant here.

>
>> 
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_dp.c  | 19 +++++++++++++++++++
>> > drivers/gpu/drm/i915/display/intel_pps.c |  1 +
>> >  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
>> >  3 files changed, 22 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index 8e16745275f6..f93895d99fd1 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -4689,6 +4689,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;
>> > +	int32_t pp;
>> 
>> For registers, this should be u32. There isn't a single int32_t use in the driver.
>> 
>> >
>> >  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
>> >  		    connector->base.id, connector->name); @@ -4792,6
>> +4793,22 @@
>> > intel_dp_detect(struct drm_connector *connector,
>> >  						 status,
>> >  						 intel_dp->dpcd,
>> >  						 intel_dp-
>> >downstream_ports);
>> > +
>> > +	/*
>> > +	 * WA_150879661:adls
>> > +	 * Driver shall enable this WA when external display is connected
>> > +	 * and remove WA when display is unplugged
>> > +	 */
>> > +	if (IS_ALDERLAKE_S(dev_priv)) {
>> > +		if (status == connector_status_connected &&
>> > +		    !dev_priv->edp_present)
>> > +			pp = PANEL_POWER_ON;
>> > +		else if (status == connector_status_disconnected)
>> > +			pp = 0;
>> > +
>> > +		intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
>> 
>> You're not supposed to cook up register offsets inline in code like that. The
>> *PPS_BASE macros are not for general use. There's PP_CONTROL macro for
>> that particular register.
>> 
>
> According to the WA we need to write in the register PP_CONTROL 0xC7204
> But the PP_CONTROL macro depends on the value assigned to mmio_base value
> In pps struct.

Yeah, it depends on the mmio_base value to make it independent of the
platform. It would not be different on ADL-S.

>
>> Why would you use magic 1 for clearing and PANEL_POWER_ON macro for
>> setting the bit in the rmw call?
>> 
>
> Since I wanted to set the first bit to be set as 0 or 1 hence used clear value as 1 to just clear
> The LSB and then intel_de_rmw OR's the read value with provided value.

Yeah, but if you're using PP_CONTROL to set the bit, why not also to
clear it? That's kind of the idea with the macros.

>
>> For the most part, only the PPS code in intel_pps.c is supposed to touch the
>> PPS registers.
>> 
>> > +	}
>> > +
>> >  	return status;
>> >  }
>> >
>> > @@ -5489,6 +5506,8 @@ intel_dp_init_connector(struct intel_digital_port
>> *dig_port,
>> >  	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
>> >  		intel_dp_aux_fini(intel_dp);
>> >  		goto fail;
>> > +	} else {
>> > +		dev_priv->edp_present = true;
>> >  	}
>> >
>> >  	intel_dp_set_source_rates(intel_dp);
>> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
>> > b/drivers/gpu/drm/i915/display/intel_pps.c
>> > index 24b5b12f7732..21538338af3d 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
>> > @@ -1726,4 +1726,5 @@ void assert_pps_unlocked(struct
>> drm_i915_private *dev_priv, enum pipe pipe)
>> >  	I915_STATE_WARN(panel_pipe == pipe && locked,
>> >  			"panel assertion failure, pipe %c regs locked\n",
>> >  			pipe_name(pipe));
>> > +
>> >  }
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > b/drivers/gpu/drm/i915/i915_drv.h index 6254aa977398..ebe16aee0ca8
>> > 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -352,6 +352,8 @@ struct drm_i915_private {
>> >  	/* The TTM device structure. */
>> >  	struct ttm_device bdev;
>> >
>> > +	bool edp_present;
>> 
>> Please don't add global state flags that duplicate info.
>> 
>> And when you actually need to, struct drm_i915_private is no longer the
>> dumping ground for random info anyway.
>> 
>
>
> This edp_present variable was to check if edp is connected to machine
> But other than iterate over all connectors to see if edp is present I couldn't find a way

Generally you should follow the Single Point of Truth (SPOT)
principle. Only cache stuff like that if you need the
performance. You'll quickly get into trouble duplicating state.

> Making me think drm_i915_private is the place where I can put this variable

I've been trying hard to clean up struct drm_i915_private of all display
stuff, and moving them to the display sub-struct in a more organized
manner.

BR,
Jani.

>
> Regards,
> Suraj Kandpal
>
>> BR,
>> Jani.
>> 
>> > +
>> >  	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
>> >
>> >  	/*
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[Kandpal, Suraj]

> 
> On Fri, 14 Apr 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
> >>
> >> On Fri, 14 Apr 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> >> > On TGP, the RTC (always running) was reduced from 3MHz to 32KHz.
> >> > As a result of this change, when HPD active going low pulse or HPD
> >> > IRQ is presented and the refclk (19.2MHz) is not toggling already
> >> > toggling, there is a 60 to 90us synchronization delay which
> >> > effectively reduces the duration of the IRQ pulse to less than the
> >> > programmed 500us filter value and the hot plug interrupt is NOT
> >> registered.
> >> > Program 0xC7204 (PP_CONTROL) bit #0 to '1' to enable workaround and
> >> > clear to disable it.
> >> > Driver shall enable this WA when external display is connected and
> >> > remove WA when display is unplugged or before going into sleep to
> >> > allow CS entry.
> >> > Driver shall not enable WA when eDP is connected.
> >> > Wa_1508796671:adls
> >> >
> >> > Cc: Arun Murthy <arun.r.murthy@intel.com>
> >> > Cc: Uma Shankar <uma.shankar@intel.com>
> >> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >>
> >> I don't know what the right fix should eventually be, but this, as it
> >> is, is absolutely not it.
> >
> > I guess we should open a discussion on how we should go ahead
> > implementing this fix
> 
> Ville's reply is relevant here.
> 
> >
> >>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_dp.c  | 19 +++++++++++++++++++
> >> > drivers/gpu/drm/i915/display/intel_pps.c |  1 +
> >> >  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
> >> >  3 files changed, 22 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > index 8e16745275f6..f93895d99fd1 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > @@ -4689,6 +4689,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;
> >> > +	int32_t pp;
> >>
> >> For registers, this should be u32. There isn't a single int32_t use in the
> driver.
> >>
> >> >
> >> >  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
> >> >  		    connector->base.id, connector->name); @@ -4792,6
> >> +4793,22 @@
> >> > intel_dp_detect(struct drm_connector *connector,
> >> >  						 status,
> >> >  						 intel_dp->dpcd,
> >> >  						 intel_dp-
> >> >downstream_ports);
> >> > +
> >> > +	/*
> >> > +	 * WA_150879661:adls
> >> > +	 * Driver shall enable this WA when external display is connected
> >> > +	 * and remove WA when display is unplugged
> >> > +	 */
> >> > +	if (IS_ALDERLAKE_S(dev_priv)) {
> >> > +		if (status == connector_status_connected &&
> >> > +		    !dev_priv->edp_present)
> >> > +			pp = PANEL_POWER_ON;
> >> > +		else if (status == connector_status_disconnected)
> >> > +			pp = 0;
> >> > +
> >> > +		intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
> >>
> >> You're not supposed to cook up register offsets inline in code like
> >> that. The *PPS_BASE macros are not for general use. There's
> >> PP_CONTROL macro for that particular register.
> >>
> >
> > According to the WA we need to write in the register PP_CONTROL
> > 0xC7204 But the PP_CONTROL macro depends on the value assigned to
> > mmio_base value In pps struct.
> 
> Yeah, it depends on the mmio_base value to make it independent of the
> platform. It would not be different on ADL-S.
> 

Ohh I see, got it.

> >
> >> Why would you use magic 1 for clearing and PANEL_POWER_ON macro
> for
> >> setting the bit in the rmw call?
> >>
> >
> > Since I wanted to set the first bit to be set as 0 or 1 hence used
> > clear value as 1 to just clear The LSB and then intel_de_rmw OR's the read
> value with provided value.
> 
> Yeah, but if you're using PP_CONTROL to set the bit, why not also to clear it?
> That's kind of the idea with the macros.
> 

Ahh got it, thanks for pointing this out

Regards,
Suraj Kandpal
> >
> >> For the most part, only the PPS code in intel_pps.c is supposed to
> >> touch the PPS registers.
> >>
> >> > +	}
> >> > +
> >> >  	return status;
> >> >  }
> >> >
> >> > @@ -5489,6 +5506,8 @@ intel_dp_init_connector(struct
> >> > intel_digital_port
> >> *dig_port,
> >> >  	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
> >> >  		intel_dp_aux_fini(intel_dp);
> >> >  		goto fail;
> >> > +	} else {
> >> > +		dev_priv->edp_present = true;
> >> >  	}
> >> >
> >> >  	intel_dp_set_source_rates(intel_dp);
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> >> > b/drivers/gpu/drm/i915/display/intel_pps.c
> >> > index 24b5b12f7732..21538338af3d 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> >> > @@ -1726,4 +1726,5 @@ void assert_pps_unlocked(struct
> >> drm_i915_private *dev_priv, enum pipe pipe)
> >> >  	I915_STATE_WARN(panel_pipe == pipe && locked,
> >> >  			"panel assertion failure, pipe %c regs locked\n",
> >> >  			pipe_name(pipe));
> >> > +
> >> >  }
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> > b/drivers/gpu/drm/i915/i915_drv.h index 6254aa977398..ebe16aee0ca8
> >> > 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -352,6 +352,8 @@ struct drm_i915_private {
> >> >  	/* The TTM device structure. */
> >> >  	struct ttm_device bdev;
> >> >
> >> > +	bool edp_present;
> >>
> >> Please don't add global state flags that duplicate info.
> >>
> >> And when you actually need to, struct drm_i915_private is no longer
> >> the dumping ground for random info anyway.
> >>
> >
> >
> > This edp_present variable was to check if edp is connected to machine
> > But other than iterate over all connectors to see if edp is present I
> > couldn't find a way
> 
> Generally you should follow the Single Point of Truth (SPOT) principle. Only
> cache stuff like that if you need the performance. You'll quickly get into
> trouble duplicating state.
> 
> > Making me think drm_i915_private is the place where I can put this
> > variable
> 
> I've been trying hard to clean up struct drm_i915_private of all display stuff,
> and moving them to the display sub-struct in a more organized manner.
> 
> BR,
> Jani.
> 
> >
> > Regards,
> > Suraj Kandpal
> >
> >> BR,
> >> Jani.
> >>
> >> > +
> >> >  	I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
> >> >
> >> >  	/*
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-i915-display-PCH-display-HPD-IRQ-is-not-detected-with-default-filter-value/20230414-152733
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/20230414072345.1041605-1-suraj.kandpal%40intel.com
patch subject: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
config: x86_64-randconfig-a016-20230410 (https://download.01.org/0day-ci/archive/20230414/202304141732.MFJIclz0-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/bf0d69db2e4066bb221d69355d2a1b27cb3a0f57
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Suraj-Kandpal/drm-i915-display-PCH-display-HPD-IRQ-is-not-detected-with-default-filter-value/20230414-152733
        git checkout bf0d69db2e4066bb221d69355d2a1b27cb3a0f57
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304141732.MFJIclz0-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/display/intel_dp.c: In function 'intel_dp_detect':
>> drivers/gpu/drm/i915/display/intel_dp.c:4856:30: error: 'PANEL_POWER_ON' undeclared (first use in this function); did you mean 'MIPI_SEQ_POWER_ON'?
    4856 |                         pp = PANEL_POWER_ON;
         |                              ^~~~~~~~~~~~~~
         |                              MIPI_SEQ_POWER_ON
   drivers/gpu/drm/i915/display/intel_dp.c:4856:30: note: each undeclared identifier is reported only once for each function it appears in
   In file included from drivers/gpu/drm/i915/display/g4x_dp.h:11,
                    from drivers/gpu/drm/i915/display/intel_dp.c:46:
>> drivers/gpu/drm/i915/display/intel_dp.c:4860:46: error: 'PCH_PPS_BASE' undeclared (first use in this function); did you mean 'PCI_IO_BASE'?
    4860 |                 intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
         |                                              ^~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_reg_defs.h:162:47: note: in definition of macro '_MMIO'
     162 | #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
         |                                               ^


vim +4856 drivers/gpu/drm/i915/display/intel_dp.c

  4732	
  4733	static int
  4734	intel_dp_detect(struct drm_connector *connector,
  4735			struct drm_modeset_acquire_ctx *ctx,
  4736			bool force)
  4737	{
  4738		struct drm_i915_private *dev_priv = to_i915(connector->dev);
  4739		struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
  4740		struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
  4741		struct intel_encoder *encoder = &dig_port->base;
  4742		enum drm_connector_status status;
  4743		int32_t pp;
  4744	
  4745		drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
  4746			    connector->base.id, connector->name);
  4747		drm_WARN_ON(&dev_priv->drm,
  4748			    !drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
  4749	
  4750		if (!INTEL_DISPLAY_ENABLED(dev_priv))
  4751			return connector_status_disconnected;
  4752	
  4753		/* Can't disconnect eDP */
  4754		if (intel_dp_is_edp(intel_dp))
  4755			status = edp_detect(intel_dp);
  4756		else if (intel_digital_port_connected(encoder))
  4757			status = intel_dp_detect_dpcd(intel_dp);
  4758		else
  4759			status = connector_status_disconnected;
  4760	
  4761		if (status == connector_status_disconnected) {
  4762			memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
  4763			memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
  4764	
  4765			if (intel_dp->is_mst) {
  4766				drm_dbg_kms(&dev_priv->drm,
  4767					    "MST device may have disappeared %d vs %d\n",
  4768					    intel_dp->is_mst,
  4769					    intel_dp->mst_mgr.mst_state);
  4770				intel_dp->is_mst = false;
  4771				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
  4772								intel_dp->is_mst);
  4773			}
  4774	
  4775			goto out;
  4776		}
  4777	
  4778		/* Read DP Sink DSC Cap DPCD regs for DP v1.4 */
  4779		if (HAS_DSC(dev_priv))
  4780			intel_dp_get_dsc_sink_cap(intel_dp);
  4781	
  4782		intel_dp_configure_mst(intel_dp);
  4783	
  4784		/*
  4785		 * TODO: Reset link params when switching to MST mode, until MST
  4786		 * supports link training fallback params.
  4787		 */
  4788		if (intel_dp->reset_link_params || intel_dp->is_mst) {
  4789			intel_dp_reset_max_link_params(intel_dp);
  4790			intel_dp->reset_link_params = false;
  4791		}
  4792	
  4793		intel_dp_print_rates(intel_dp);
  4794	
  4795		if (intel_dp->is_mst) {
  4796			/*
  4797			 * If we are in MST mode then this connector
  4798			 * won't appear connected or have anything
  4799			 * with EDID on it
  4800			 */
  4801			status = connector_status_disconnected;
  4802			goto out;
  4803		}
  4804	
  4805		/*
  4806		 * Some external monitors do not signal loss of link synchronization
  4807		 * with an IRQ_HPD, so force a link status check.
  4808		 */
  4809		if (!intel_dp_is_edp(intel_dp)) {
  4810			int ret;
  4811	
  4812			ret = intel_dp_retrain_link(encoder, ctx);
  4813			if (ret)
  4814				return ret;
  4815		}
  4816	
  4817		/*
  4818		 * Clearing NACK and defer counts to get their exact values
  4819		 * while reading EDID which are required by Compliance tests
  4820		 * 4.2.2.4 and 4.2.2.5
  4821		 */
  4822		intel_dp->aux.i2c_nack_count = 0;
  4823		intel_dp->aux.i2c_defer_count = 0;
  4824	
  4825		intel_dp_set_edid(intel_dp);
  4826		if (intel_dp_is_edp(intel_dp) ||
  4827		    to_intel_connector(connector)->detect_edid)
  4828			status = connector_status_connected;
  4829	
  4830		intel_dp_check_device_service_irq(intel_dp);
  4831	
  4832	out:
  4833		if (status != connector_status_connected && !intel_dp->is_mst)
  4834			intel_dp_unset_edid(intel_dp);
  4835	
  4836		/*
  4837		 * Make sure the refs for power wells enabled during detect are
  4838		 * dropped to avoid a new detect cycle triggered by HPD polling.
  4839		 */
  4840		intel_display_power_flush_work(dev_priv);
  4841	
  4842		if (!intel_dp_is_edp(intel_dp))
  4843			drm_dp_set_subconnector_property(connector,
  4844							 status,
  4845							 intel_dp->dpcd,
  4846							 intel_dp->downstream_ports);
  4847	
  4848		/*
  4849		 * WA_150879661:adls
  4850		 * Driver shall enable this WA when external display is connected
  4851		 * and remove WA when display is unplugged
  4852		 */
  4853		if (IS_ALDERLAKE_S(dev_priv)) {
  4854			if (status == connector_status_connected &&
  4855			    !dev_priv->edp_present)
> 4856				pp = PANEL_POWER_ON;
  4857			else if (status == connector_status_disconnected)
  4858				pp = 0;
  4859	
> 4860			intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
  4861		}
  4862	
  4863		return status;
  4864	}
  4865	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-i915-display-PCH-display-HPD-IRQ-is-not-detected-with-default-filter-value/20230414-152733
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/20230414072345.1041605-1-suraj.kandpal%40intel.com
patch subject: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20230414/202304141736.ZKKtGows-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/bf0d69db2e4066bb221d69355d2a1b27cb3a0f57
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Suraj-Kandpal/drm-i915-display-PCH-display-HPD-IRQ-is-not-detected-with-default-filter-value/20230414-152733
        git checkout bf0d69db2e4066bb221d69355d2a1b27cb3a0f57
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304141736.ZKKtGows-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_dp.c:4856:9: error: use of undeclared identifier 'PANEL_POWER_ON'
                           pp = PANEL_POWER_ON;
                                ^
>> drivers/gpu/drm/i915/display/intel_dp.c:4860:32: error: use of undeclared identifier 'PCH_PPS_BASE'
                   intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
                                                ^
   2 errors generated.


vim +/PANEL_POWER_ON +4856 drivers/gpu/drm/i915/display/intel_dp.c

  4732	
  4733	static int
  4734	intel_dp_detect(struct drm_connector *connector,
  4735			struct drm_modeset_acquire_ctx *ctx,
  4736			bool force)
  4737	{
  4738		struct drm_i915_private *dev_priv = to_i915(connector->dev);
  4739		struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
  4740		struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
  4741		struct intel_encoder *encoder = &dig_port->base;
  4742		enum drm_connector_status status;
  4743		int32_t pp;
  4744	
  4745		drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
  4746			    connector->base.id, connector->name);
  4747		drm_WARN_ON(&dev_priv->drm,
  4748			    !drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
  4749	
  4750		if (!INTEL_DISPLAY_ENABLED(dev_priv))
  4751			return connector_status_disconnected;
  4752	
  4753		/* Can't disconnect eDP */
  4754		if (intel_dp_is_edp(intel_dp))
  4755			status = edp_detect(intel_dp);
  4756		else if (intel_digital_port_connected(encoder))
  4757			status = intel_dp_detect_dpcd(intel_dp);
  4758		else
  4759			status = connector_status_disconnected;
  4760	
  4761		if (status == connector_status_disconnected) {
  4762			memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
  4763			memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
  4764	
  4765			if (intel_dp->is_mst) {
  4766				drm_dbg_kms(&dev_priv->drm,
  4767					    "MST device may have disappeared %d vs %d\n",
  4768					    intel_dp->is_mst,
  4769					    intel_dp->mst_mgr.mst_state);
  4770				intel_dp->is_mst = false;
  4771				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
  4772								intel_dp->is_mst);
  4773			}
  4774	
  4775			goto out;
  4776		}
  4777	
  4778		/* Read DP Sink DSC Cap DPCD regs for DP v1.4 */
  4779		if (HAS_DSC(dev_priv))
  4780			intel_dp_get_dsc_sink_cap(intel_dp);
  4781	
  4782		intel_dp_configure_mst(intel_dp);
  4783	
  4784		/*
  4785		 * TODO: Reset link params when switching to MST mode, until MST
  4786		 * supports link training fallback params.
  4787		 */
  4788		if (intel_dp->reset_link_params || intel_dp->is_mst) {
  4789			intel_dp_reset_max_link_params(intel_dp);
  4790			intel_dp->reset_link_params = false;
  4791		}
  4792	
  4793		intel_dp_print_rates(intel_dp);
  4794	
  4795		if (intel_dp->is_mst) {
  4796			/*
  4797			 * If we are in MST mode then this connector
  4798			 * won't appear connected or have anything
  4799			 * with EDID on it
  4800			 */
  4801			status = connector_status_disconnected;
  4802			goto out;
  4803		}
  4804	
  4805		/*
  4806		 * Some external monitors do not signal loss of link synchronization
  4807		 * with an IRQ_HPD, so force a link status check.
  4808		 */
  4809		if (!intel_dp_is_edp(intel_dp)) {
  4810			int ret;
  4811	
  4812			ret = intel_dp_retrain_link(encoder, ctx);
  4813			if (ret)
  4814				return ret;
  4815		}
  4816	
  4817		/*
  4818		 * Clearing NACK and defer counts to get their exact values
  4819		 * while reading EDID which are required by Compliance tests
  4820		 * 4.2.2.4 and 4.2.2.5
  4821		 */
  4822		intel_dp->aux.i2c_nack_count = 0;
  4823		intel_dp->aux.i2c_defer_count = 0;
  4824	
  4825		intel_dp_set_edid(intel_dp);
  4826		if (intel_dp_is_edp(intel_dp) ||
  4827		    to_intel_connector(connector)->detect_edid)
  4828			status = connector_status_connected;
  4829	
  4830		intel_dp_check_device_service_irq(intel_dp);
  4831	
  4832	out:
  4833		if (status != connector_status_connected && !intel_dp->is_mst)
  4834			intel_dp_unset_edid(intel_dp);
  4835	
  4836		/*
  4837		 * Make sure the refs for power wells enabled during detect are
  4838		 * dropped to avoid a new detect cycle triggered by HPD polling.
  4839		 */
  4840		intel_display_power_flush_work(dev_priv);
  4841	
  4842		if (!intel_dp_is_edp(intel_dp))
  4843			drm_dp_set_subconnector_property(connector,
  4844							 status,
  4845							 intel_dp->dpcd,
  4846							 intel_dp->downstream_ports);
  4847	
  4848		/*
  4849		 * WA_150879661:adls
  4850		 * Driver shall enable this WA when external display is connected
  4851		 * and remove WA when display is unplugged
  4852		 */
  4853		if (IS_ALDERLAKE_S(dev_priv)) {
  4854			if (status == connector_status_connected &&
  4855			    !dev_priv->edp_present)
> 4856				pp = PANEL_POWER_ON;
  4857			else if (status == connector_status_disconnected)
  4858				pp = 0;
  4859	
> 4860			intel_de_rmw(dev_priv, _MMIO(PCH_PPS_BASE + 4), 1, pp);
  4861		}
  4862	
  4863		return status;
  4864	}
  4865	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests