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

[Jani Nikula <jani.nikula@linux.intel.com>]

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