Re: [PATCH v2 07/16] drm/i915/mst: adapt intel_dp_mtp_tu_compute_config() 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:56PM +0200, Jani Nikula wrote:
>> Handle 128b/132b SST in intel_dp_mtp_tu_compute_config(). The remote
>> bandwidth overhead and time slot allocation are only relevant for MST;
>> SST only needs the local bandwidth and a check that 64 slots isn't
>> exceeded.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 109 +++++++++++---------
>>  1 file changed, 61 insertions(+), 48 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> index d08824d2fefe..3fbf9163272b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> @@ -257,10 +257,7 @@ int intel_dp_mtp_tu_compute_config(struct intel_dp *intel_dp,
>>  
>>  	for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
>>  		int local_bw_overhead;
>> -		int remote_bw_overhead;
>>  		int link_bpp_x16;
>> -		int remote_tu;
>> -		fixed20_12 pbn;
>>  
>>  		drm_dbg_kms(display->drm, "Trying bpp %d\n", bpp);
>>  
>> @@ -269,57 +266,73 @@ int intel_dp_mtp_tu_compute_config(struct intel_dp *intel_dp,
>>  
>>  		local_bw_overhead = intel_dp_mst_bw_overhead(crtc_state,
>>  							     false, dsc_slice_count, link_bpp_x16);
>> -		remote_bw_overhead = intel_dp_mst_bw_overhead(crtc_state,
>> -							      true, dsc_slice_count, link_bpp_x16);
>> -
>>  		intel_dp_mst_compute_m_n(crtc_state,
>>  					 local_bw_overhead,
>>  					 link_bpp_x16,
>>  					 &crtc_state->dp_m_n);
>>  
>> -		/*
>> -		 * The TU size programmed to the HW determines which slots in
>> -		 * an MTP frame are used for this stream, which needs to match
>> -		 * the payload size programmed to the first downstream branch
>> -		 * device's payload table.
>> -		 *
>> -		 * Note that atm the payload's PBN value DRM core sends via
>> -		 * the ALLOCATE_PAYLOAD side-band message matches the payload
>> -		 * size (which it calculates from the PBN value) it programs
>> -		 * to the first branch device's payload table. The allocation
>> -		 * in the payload table could be reduced though (to
>> -		 * crtc_state->dp_m_n.tu), provided that the driver doesn't
>> -		 * enable SSC on the corresponding link.
>> -		 */
>> -		pbn.full = dfixed_const(intel_dp_mst_calc_pbn(adjusted_mode->crtc_clock,
>> -							      link_bpp_x16,
>> -							      remote_bw_overhead));
>> -		remote_tu = DIV_ROUND_UP(pbn.full, pbn_div.full);
>> -
>> -		/*
>> -		 * Aligning the TUs ensures that symbols consisting of multiple
>> -		 * (4) symbol cycles don't get split between two consecutive
>> -		 * MTPs, as required by Bspec.
>> -		 * TODO: remove the alignment restriction for 128b/132b links
>> -		 * on some platforms, where Bspec allows this.
>> -		 */
>> -		remote_tu = ALIGN(remote_tu, 4 / crtc_state->lane_count);
>> -
>> -		/*
>> -		 * Also align PBNs accordingly, since MST core will derive its
>> -		 * own copy of TU from the PBN in drm_dp_atomic_find_time_slots().
>> -		 * The above comment about the difference between the PBN
>> -		 * allocated for the whole path and the TUs allocated for the
>> -		 * first branch device's link also applies here.
>> -		 */
>> -		pbn.full = remote_tu * pbn_div.full;
>> -
>> -		drm_WARN_ON(display->drm, remote_tu < crtc_state->dp_m_n.tu);
>> -		crtc_state->dp_m_n.tu = remote_tu;
>> +		if (intel_dp->is_mst) {
>> +			int remote_bw_overhead;
>> +			int remote_tu;
>> +			fixed20_12 pbn;
>
> Nit: pbn_div is only used for MST, so would (calculate/look it up from
> mst_state) here. Also this MST block could be in a separate function,
> perhaps for symmetry with the SST part in a function too, but this could
> be a follow-up as well. Either way:

I guess I'm just not sure yet which direction this should be taken. It
would be easy enough to add the functions, but is that the right
division? How to handle pbn_div in that case? How is UHBR SST DSC going
to impact all this?

I know I'm kind of weaseling out of fixing this up front, but I also try
to avoid adding stuff that we may have to back out of later.

I think I'd let this be for now.

> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thanks,
Jani.

>
>> +
>> +			remote_bw_overhead = intel_dp_mst_bw_overhead(crtc_state,
>> +								      true, dsc_slice_count, link_bpp_x16);
>> +
>> +			/*
>> +			 * The TU size programmed to the HW determines which slots in
>> +			 * an MTP frame are used for this stream, which needs to match
>> +			 * the payload size programmed to the first downstream branch
>> +			 * device's payload table.
>> +			 *
>> +			 * Note that atm the payload's PBN value DRM core sends via
>> +			 * the ALLOCATE_PAYLOAD side-band message matches the payload
>> +			 * size (which it calculates from the PBN value) it programs
>> +			 * to the first branch device's payload table. The allocation
>> +			 * in the payload table could be reduced though (to
>> +			 * crtc_state->dp_m_n.tu), provided that the driver doesn't
>> +			 * enable SSC on the corresponding link.
>> +			 */
>> +			pbn.full = dfixed_const(intel_dp_mst_calc_pbn(adjusted_mode->crtc_clock,
>> +								      link_bpp_x16,
>> +								      remote_bw_overhead));
>> +			remote_tu = DIV_ROUND_UP(pbn.full, pbn_div.full);
>> +
>> +			/*
>> +			 * Aligning the TUs ensures that symbols consisting of multiple
>> +			 * (4) symbol cycles don't get split between two consecutive
>> +			 * MTPs, as required by Bspec.
>> +			 * TODO: remove the alignment restriction for 128b/132b links
>> +			 * on some platforms, where Bspec allows this.
>> +			 */
>> +			remote_tu = ALIGN(remote_tu, 4 / crtc_state->lane_count);
>> +
>> +			/*
>> +			 * Also align PBNs accordingly, since MST core will derive its
>> +			 * own copy of TU from the PBN in drm_dp_atomic_find_time_slots().
>> +			 * The above comment about the difference between the PBN
>> +			 * allocated for the whole path and the TUs allocated for the
>> +			 * first branch device's link also applies here.
>> +			 */
>> +			pbn.full = remote_tu * pbn_div.full;
>> +
>> +			drm_WARN_ON(display->drm, remote_tu < crtc_state->dp_m_n.tu);
>> +			crtc_state->dp_m_n.tu = remote_tu;
>> +
>> +			slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>> +							      connector->port,
>> +							      dfixed_trunc(pbn));
>> +		} else {
>> +			/* Same as above for remote_tu */
>> +			crtc_state->dp_m_n.tu = ALIGN(crtc_state->dp_m_n.tu,
>> +						      4 / crtc_state->lane_count);
>> +
>> +			if (crtc_state->dp_m_n.tu <= 64)
>> +				slots = crtc_state->dp_m_n.tu;
>> +			else
>> +				slots = -EINVAL;
>> +		}
>>  
>> -		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>> -						      connector->port,
>> -						      dfixed_trunc(pbn));
>>  		if (slots == -EDEADLK)
>>  			return slots;
>>  
>> -- 
>> 2.39.5
>> 

-- 
Jani Nikula, Intel