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

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

On Tue, 29 Jun 2021, "Souza, Jose" <jose.souza@intel.com> wrote:
> On Mon, 2021-06-28 at 16:50 -0700, Madhumitha Tolakanahalli Pradeep wrote:
>> PCH display HPD IRQ is not detected with default filter value.
>> So, PP_CONTROL is manually reprogrammed.
>> 
>> 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);
>> +	}
>
> Code style is all wrong, please fix it and run "dim checkpatch" to validate it before sending patches.
> Also PP_CONTROL(0) don't point to the same register that the workaround is talking about, between generations register address change that might be
> the case for this one.

In general, I've put a bunch of effort into moving most PPS stuff and
PP_CONTROL reg access into intel_pps.c, not least because you must hold
appropriate locks and power domain references to poke at this. You can't
just mess with it nilly willy. I don't want these abstractions bypassed.

BR,
Jani.

>
> This satisfy the "before going into sleep to allow CS entry" but it do not restore the workaround after waking up from suspend.
> Also you could improve the code, you are reading the register even for platforms that don't need the wa, also check intel_de_rmw() it is better suited
> to this case.
>
>>  		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);
>> +			}
>> +		}
>
> Not sure if this is the best place but anyways it is missing handle the case were tigerlake boots with the external display connected.
> No hotplug will happen and workaround will never be enabled.
>
>>  		return INTEL_HOTPLUG_CHANGED;
>>  	}
>>  	return INTEL_HOTPLUG_UNCHANGED;
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx