* Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 7:23 [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value Suraj Kandpal
@ 2023-04-14 8:06 ` Ville Syrjälä
2023-04-14 8:12 ` Kandpal, Suraj
2023-04-14 8:19 ` Jani Nikula
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2023-04-14 8:06 UTC (permalink / raw)
To: Suraj Kandpal; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 8:06 ` Ville Syrjälä
@ 2023-04-14 8:12 ` Kandpal, Suraj
0 siblings, 0 replies; 10+ messages in thread
From: Kandpal, Suraj @ 2023-04-14 8:12 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx@lists.freedesktop.org
> -----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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 7:23 [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value Suraj Kandpal
2023-04-14 8:06 ` Ville Syrjälä
@ 2023-04-14 8:19 ` Jani Nikula
2023-04-14 8:47 ` Kandpal, Suraj
2023-04-14 8:37 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2023-04-14 8:19 UTC (permalink / raw)
To: Suraj Kandpal, intel-gfx
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 8:19 ` Jani Nikula
@ 2023-04-14 8:47 ` Kandpal, Suraj
2023-04-14 8:58 ` Jani Nikula
0 siblings, 1 reply; 10+ messages in thread
From: Kandpal, Suraj @ 2023-04-14 8:47 UTC (permalink / raw)
To: Jani Nikula, intel-gfx@lists.freedesktop.org
>
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 8:47 ` Kandpal, Suraj
@ 2023-04-14 8:58 ` Jani Nikula
2023-04-14 9:04 ` Kandpal, Suraj
0 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2023-04-14 8:58 UTC (permalink / raw)
To: Kandpal, Suraj, intel-gfx@lists.freedesktop.org
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 8:58 ` Jani Nikula
@ 2023-04-14 9:04 ` Kandpal, Suraj
0 siblings, 0 replies; 10+ messages in thread
From: Kandpal, Suraj @ 2023-04-14 9:04 UTC (permalink / raw)
To: Jani Nikula, intel-gfx@lists.freedesktop.org
>
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 7:23 [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value Suraj Kandpal
2023-04-14 8:06 ` Ville Syrjälä
2023-04-14 8:19 ` Jani Nikula
@ 2023-04-14 8:37 ` Patchwork
2023-04-14 9:35 ` [Intel-gfx] [PATCH] " kernel test robot
2023-04-14 10:06 ` kernel test robot
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-04-14 8:37 UTC (permalink / raw)
To: Kandpal, Suraj; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 7:23 [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value Suraj Kandpal
` (2 preceding siblings ...)
2023-04-14 8:37 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
@ 2023-04-14 9:35 ` kernel test robot
2023-04-14 10:06 ` kernel test robot
4 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-04-14 9:35 UTC (permalink / raw)
To: Suraj Kandpal, intel-gfx; +Cc: oe-kbuild-all
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value
2023-04-14 7:23 [Intel-gfx] [PATCH] drm/i915/display: PCH display HPD IRQ is not detected with default filter value Suraj Kandpal
` (3 preceding siblings ...)
2023-04-14 9:35 ` [Intel-gfx] [PATCH] " kernel test robot
@ 2023-04-14 10:06 ` kernel test robot
4 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-04-14 10:06 UTC (permalink / raw)
To: Suraj Kandpal, intel-gfx; +Cc: llvm, oe-kbuild-all
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
^ permalink raw reply [flat|nested] 10+ messages in thread