Re: [PATCH v2 10/16] drm/i915/ddi: write payload for 128b/132b SST

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

On Tue, 31 Dec 2024, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, Dec 19, 2024 at 11:33:59PM +0200, Jani Nikula wrote:
>> Write the payload allocation table for 128b/132b SST. Use VCPID 1 and
>> start from slot 0, with dp_m_n.tu slots.
>> 
>> This is preparation for enabling 128b/132b SST. This path is not
>> reachable yet. Indeed, we don't yet compute TU for 128b/132b SST.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_ddi.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 6f813bf85b23..64528ff8856e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -2669,6 +2669,12 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>  	/* 6.o Configure and enable FEC if needed */
>>  	intel_ddi_enable_fec(encoder, crtc_state);
>>  
>> +	/* 7.a 128b/132b SST. */
>> +	if (!is_mst && intel_dp_is_uhbr(crtc_state)) {
>> +		/* VCPID 1, start slot 0 for 128b/132b, tu slots */
>> +		drm_dp_dpcd_write_payload(&intel_dp->aux, 1, 0, crtc_state->dp_m_n.tu);
>
> I would handle the error by sending a modeset retry uevent. Either way:

Mmmh. The MST code calls intel_dp_queue_modeset_retry_for_link() in
response to drm_dp_add_payload_part{1,2}() failures, but nothing
else. As in, we don't do anything else (we just plunge on with the
enable), and we don't even do that much in response to any other errors.

Our options are limited anyway.

Arguably the MST path payload update has more failure modes than SST,
but you'd like to do this:

	ret = drm_dp_dpcd_write_payload(&intel_dp->aux, 1, 0, crtc_state->dp_m_n.tu);
	if (ret < 0)
		intel_dp_queue_modeset_retry_for_link(state, encoder, crtc_state);

in mtl_ddi_pre_enable_dp() and tgl_ddi_pre_enable_dp()?

Overall I'm wondering if this too needs an overhaul. Instead of queueing
the retry all over the place, maybe we should just flag it in
crtc_state, and have intel_atomic_cleanup_work() do it once afterwards?


BR,
Jani.

>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
>> +	}
>> +
>>  	if (!is_mst)
>>  		intel_dsc_dp_pps_write(encoder, crtc_state);
>>  }
>> @@ -2808,6 +2814,11 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>>  	/* 7.l Configure and enable FEC if needed */
>>  	intel_ddi_enable_fec(encoder, crtc_state);
>>  
>> +	if (!is_mst && intel_dp_is_uhbr(crtc_state)) {
>> +		/* VCPID 1, start slot 0 for 128b/132b, tu slots */
>> +		drm_dp_dpcd_write_payload(&intel_dp->aux, 1, 0, crtc_state->dp_m_n.tu);
>> +	}
>> +
>>  	if (!is_mst)
>>  		intel_dsc_dp_pps_write(encoder, crtc_state);
>>  }
>> -- 
>> 2.39.5
>> 

-- 
Jani Nikula, Intel