On Thu, 02 Jan 2025, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> On Thu, Dec 19, 2024 at 11:34:05PM +0200, Jani Nikula wrote:
>> Enable basic 128b/132b SST functionality without compression. Reuse
>> intel_dp_mtp_tu_compute_config() to figure out the TU after we've
>> determined we need to use an UHBR rate.
>>
>> It's slightly complicated as the M/N computation is done in different
>> places in MST and SST paths, so we need to avoid trashing the values
>> later for UHBR.
>>
>> If uncompressed UHBR fails, we drop to compressed non-UHBR, which is
>> quite likely to fail as well. We still lack 128b/132b SST+DSC.
>>
>> We need mst_master_transcoder also for 128b/132b SST. Use cpu_transcoder
>> directly. Enhanced framing is "don't care" for 128b/132b link.
>>
>> v2: mst_master_transcoder, enhanced framing (Imre)
>>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/display/intel_dp.c | 34 +++++++++++++++++--------
>> 1 file changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index fba3af338280..d14a42f02ba8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -2525,8 +2525,8 @@ intel_dp_compute_config_limits(struct intel_dp *intel_dp,
>> limits->min_rate = intel_dp_min_link_rate(intel_dp);
>> limits->max_rate = intel_dp_max_link_rate(intel_dp);
>>
>> - /* FIXME 128b/132b SST support missing */
>> - if (!is_mst)
>> + /* FIXME 128b/132b SST+DSC support missing */
>> + if (!is_mst && dsc)
>> limits->max_rate = min(limits->max_rate, 810000);
>> limits->min_rate = min(limits->min_rate, limits->max_rate);
>>
>> @@ -2640,6 +2640,13 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>> */
>> ret = intel_dp_compute_link_config_wide(intel_dp, pipe_config,
>> conn_state, &limits);
>> + if (!ret && intel_dp_is_uhbr(pipe_config))
>> + ret = intel_dp_mtp_tu_compute_config(intel_dp,
>> + pipe_config,
>> + pipe_config->pipe_bpp,
>> + pipe_config->pipe_bpp,
>> + conn_state,
>> + 0, false);
>> if (ret)
>> dsc_needed = true;
>> }
>> @@ -3148,8 +3155,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>> pipe_config->limited_color_range =
>> intel_dp_limited_color_range(pipe_config, conn_state);
>>
>> - pipe_config->enhanced_framing =
>> - drm_dp_enhanced_frame_cap(intel_dp->dpcd);
>> + if (intel_dp_is_uhbr(pipe_config)) {
>
> Nit: no need for {} here and below.
I like this:
if (foo)
action();
but not this:
if (foo)
/* comment */
action();
so I prefer to add the {} for clarity.
Below I think it's just because the action() spans six lines...
>
> The patch looks ok:
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
Thanks!
>
>> + /* 128b/132b SST also needs this */
>> + pipe_config->mst_master_transcoder = pipe_config->cpu_transcoder;
>> + } else {
>> + pipe_config->enhanced_framing =
>> + drm_dp_enhanced_frame_cap(intel_dp->dpcd);
>> + }
>>
>> if (pipe_config->dsc.compression_enable)
>> link_bpp_x16 = pipe_config->dsc.compressed_bpp_x16;
>> @@ -3180,12 +3192,14 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>
>> intel_dp_audio_compute_config(encoder, pipe_config, conn_state);
>>
>> - intel_link_compute_m_n(link_bpp_x16,
>> - pipe_config->lane_count,
>> - adjusted_mode->crtc_clock,
>> - pipe_config->port_clock,
>> - intel_dp_bw_fec_overhead(pipe_config->fec_enable),
>> - &pipe_config->dp_m_n);
>> + if (!intel_dp_is_uhbr(pipe_config)) {
>> + intel_link_compute_m_n(link_bpp_x16,
>> + pipe_config->lane_count,
>> + adjusted_mode->crtc_clock,
>> + pipe_config->port_clock,
>> + intel_dp_bw_fec_overhead(pipe_config->fec_enable),
>> + &pipe_config->dp_m_n);
>> + }
>>
>> /* FIXME: abstract this better */
>> if (pipe_config->splitter.enable)
>> --
>> 2.39.5
>>
--
Jani Nikula, Intel
