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