Re: [Intel-gfx] [PATCH] drm/i915/display/tgl: Implement Wa_14013120569

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

On Mon, 28 Jun 2021, Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com> wrote:
> PCH display HPD IRQ is not detected with default filter value.
> So, PP_CONTROL is manually reprogrammed.

Returning to this workaround.

You're not supposed to enable the workaround when there's eDP
connected. This is also crucial in avoiding issues with eDP PPS.

The workaround is specific to Tiger Lake PCH, so you need to check
against the PCH, not the GPU.

Also see comments inline.

>
> Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_power.c   |  8 ++++++++
>  drivers/gpu/drm/i915/display/intel_hotplug.c     | 16 ++++++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 285380079aab..e44323cc76f5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -6385,8 +6385,16 @@ static void intel_power_domains_verify_state(struct drm_i915_private *i915)
>  
>  void intel_display_power_suspend_late(struct drm_i915_private *i915)
>  {
> +    struct drm_i915_private *dev_priv = i915;
> +    u32 val;
>  	if (DISPLAY_VER(i915) >= 11 || IS_GEMINILAKE(i915) ||
>  	    IS_BROXTON(i915)) {
> +		val = intel_de_read(dev_priv, PP_CONTROL(0));
> +		/* Wa_14013120569:tgl */
> +		if (IS_TIGERLAKE(i915)) {
> +			val &= ~PANEL_POWER_ON;
> +			intel_de_write(dev_priv, PP_CONTROL(0), val);
> +	}

As José said, how do you enable the workaround after resume if external
displays are still connected?

>  		bxt_enable_dc9(i915);
>  		/* Tweaked Wa_14010685332:icp,jsp,mcc */
>  		if (INTEL_PCH_TYPE(i915) >= PCH_ICP && INTEL_PCH_TYPE(i915) <= PCH_MCC)
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> index 47c85ac97c87..8e3f84100daf 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> @@ -26,6 +26,7 @@
>  #include "i915_drv.h"
>  #include "intel_display_types.h"
>  #include "intel_hotplug.h"
> +#include "intel_de.h"
>  
>  /**
>   * DOC: Hotplug
> @@ -266,7 +267,9 @@ intel_encoder_hotplug(struct intel_encoder *encoder,
>  		      struct intel_connector *connector)
>  {
>  	struct drm_device *dev = connector->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum drm_connector_status old_status;
> +	u32 val;
>  	u64 old_epoch_counter;
>  	bool ret = false;
>  
> @@ -288,6 +291,19 @@ intel_encoder_hotplug(struct intel_encoder *encoder,
>  			      drm_get_connector_status_name(connector->base.status),
>  			      old_epoch_counter,
>  			      connector->base.epoch_counter);
> +
> +		/* Wa_14013120569:tgl */
> +		if (IS_TIGERLAKE(dev_priv)) {
> +			val = intel_de_read(dev_priv, PP_CONTROL(0));
> +			if (connector->base.status == connector_status_connected) {
> +				val |= PANEL_POWER_ON;
> +				intel_de_write(dev_priv, PP_CONTROL(0), val);
> +			}
> +			else if (connector->base.status == connector_status_disconnected) {
> +				val &= ~PANEL_POWER_ON;
> +				intel_de_write(dev_priv, PP_CONTROL(0), val);
> +			}
> +		}

First off, usually if you have a clean, generic, high level function,
it's a hint you shouldn't stick low level register access there.

If you plug in two external displays and then unplug one of them, you
end up disabling the workaround, while it's supposed to remain enabled
if there's an external display connected. This is likely the most
annoying part about the workaround.

This does not seem like a trivial workaround to implement.


BR,
Jani.


>  		return INTEL_HOTPLUG_CHANGED;
>  	}
>  	return INTEL_HOTPLUG_UNCHANGED;

-- 
Jani Nikula, Intel Open Source Graphics Center