Re: [PATCH 2/2] drm/i915/lttpr: Enable Extended Wake Timeout

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

On Tue, 31 Dec 2024, "Vodapalli, Ravi Kumar" <ravi.kumar.vodapalli@intel.com> wrote:
> On 12/13/2024 11:33 AM, Suraj Kandpal wrote:
>> Usually retimers take around 30 to 40ms to exit all devices from
>> sleep state. Extended wake timeout request helps to give additional
>> time by reading the DPCD register through which sink requests the
>> minimal amount of time required to wake the sink up and giving the
>> same amount of wait requested by sink device.
>> Spec: DP v2.1 Section 3.6.12.3
>>
>> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_ddi.c      |  2 +
>>   .../drm/i915/display/intel_dp_link_training.c | 46 +++++++++++++++++++
>>   .../drm/i915/display/intel_dp_link_training.h |  1 +
>>   3 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 4f9c50996446..d092c3ba0ccf 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -2624,6 +2624,8 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>   	if (!is_mst)
>>   		intel_dp_set_power(intel_dp, DP_SET_POWER_D0);
>>   
>> +	intel_dp_lttpr_wake_timeout_setup(intel_dp);
>> +
>>   	intel_dp_configure_protocol_converter(intel_dp, crtc_state);
>>   	if (!is_mst)
>>   		intel_dp_sink_enable_decompression(state,
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> index ea9b4730a176..d0f0da78794e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> @@ -134,6 +134,52 @@ static bool intel_dp_lttpr_transparent_mode_enabled(struct intel_dp *intel_dp)
>>   		DP_PHY_REPEATER_MODE_TRANSPARENT;
>>   }
>>   
>> +void intel_dp_lttpr_wake_timeout_setup(struct intel_dp *intel_dp)
>> +{
>> +	struct intel_display *display = to_intel_display(intel_dp);
>> +	u8 val = 1;
>> +	int ret;
>> +
>> +	if (intel_dp_lttpr_transparent_mode_enabled(intel_dp)) {
>> +		static const u8 timeout_mapping[] = {
>> +			[DP_DPRX_SLEEP_WAKE_TIMEOUT_PERIOD_1_MS] = 1,
>> +			[DP_DPRX_SLEEP_WAKE_TIMEOUT_PERIOD_20_MS] = 20,
>> +			[DP_DPRX_SLEEP_WAKE_TIMEOUT_PERIOD_40_MS] = 40,
>> +			[DP_DPRX_SLEEP_WAKE_TIMEOUT_PERIOD_20_MS] = 20,
>> +			[DP_DPRX_SLEEP_WAKE_TIMEOUT_PERIOD_80_MS] = 80,
>> +			[DP_DPRX_SLEEP_WAKE_TIMEOUT_PERIOD_100_MS] = 100,
>> +		};

Btw this kind of stuff totally belongs in generic DP helpers instead of
our driver.

>> +
>> +		ret = drm_dp_dpcd_readb(&intel_dp->aux,
>> +					DP_EXTENDED_DPRX_SLEEP_WAKE_TIMEOUT_REQUEST, &val);
>> +		if (ret != 1) {
>> +			drm_dbg_kms(display->drm,
>> +				    "Failed to read Extended sleep wake timeout request\n");
>> +			return;
>
> Returning from function without return type, better to declare int in 
> place of void and return the error  value.

That depends on what you're going to do with that error value. We're not
going to check it anyway, are we?

>
> int intel_dp_lttpr_wake_timeout_setup(struct intel_dp *intel_dp)
>
>
> Regards,
> Ravi Kumar V
>> +		}
>> +
>> +		val = (val < sizeof(timeout_mapping) && timeout_mapping[val]) ?
>> +			timeout_mapping[val] : 1;

What's the point with this? We don't do anything with val?

BR,
Jani.

>> +
>> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EXTENDED_DPRX_SLEEP_WAKE_TIMEOUT_GRANT,
>> +				   DP_DPRX_SLEEP_WAKE_TIMEOUT_PERIOD_GRANTED);
>> +	} else {
>> +		ret = drm_dp_dpcd_readb(&intel_dp->aux,
>> +					DP_PHY_REPEATER_EXTENDED_WAIT_TIMEOUT, &val);
>> +		if (ret != 1) {
>> +			drm_dbg_kms(display->drm,
>> +				    "Failed to read Extended sleep wake timeout request\n");
>> +			return;
>> +		}
>> +
>> +		val = (val & DP_EXTENDED_WAKE_TIMEOUT_REQUEST_MASK) ?
>> +			(val & DP_EXTENDED_WAKE_TIMEOUT_REQUEST_MASK) * 10 : 1;
>> +
>> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PHY_REPEATER_EXTENDED_WAIT_TIMEOUT,
>> +				   DP_EXTENDED_WAKE_TIMEOUT_GRANT);
>> +	}
>> +}
>> +
>>   /*
>>    * Read the LTTPR common capabilities and switch the LTTPR PHYs to
>>    * non-transparent mode if this is supported. Preserve the
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.h b/drivers/gpu/drm/i915/display/intel_dp_link_training.h
>> index 2066b9146762..cd4e0d6db6ed 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.h
>> @@ -15,6 +15,7 @@ struct intel_dp;
>>   
>>   int intel_dp_read_dprx_caps(struct intel_dp *intel_dp, u8 dpcd[DP_RECEIVER_CAP_SIZE]);
>>   int intel_dp_init_lttpr_and_dprx_caps(struct intel_dp *intel_dp);
>> +void intel_dp_lttpr_wake_timeout_setup(struct intel_dp *intel_dp);
>>   
>>   void intel_dp_link_training_set_mode(struct intel_dp *intel_dp,
>>   				     int link_rate, bool is_vrr);
>

-- 
Jani Nikula, Intel