Re: [PATCH] drm/i915/mst: use intel_dp_compute_config_limits() for DP MST

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

On Fri, 13 Dec 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:
> On 12/11/2024 8:13 PM, Jani Nikula wrote:
>> There's a lot of duplication between mst_stream_compute_config_limits()
>> and intel_dp_compute_config_limits(). Adjust the latter to suit the
>> needs of the former, and use the same function for both. This reduces
>> duplication and highlights the differences for SST and MST and UHBR.
>>
>> Remove the kernel-doc for intel_dp_compute_config_link_bpp_limits()
>> which now becomes static.
>>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> LGTM.
>
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Thanks for the reviews, pushed to din.

BR,
Jani.

>
>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dp.c     | 44 +++++++++++++--------
>>   drivers/gpu/drm/i915/display/intel_dp.h     | 10 ++---
>>   drivers/gpu/drm/i915/display/intel_dp_mst.c | 29 +-------------
>>   3 files changed, 34 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index fbe6b77d642e..4b6676e785bf 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -2455,19 +2455,11 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>   	return 0;
>>   }
>>   
>> -/**
>> - * intel_dp_compute_config_link_bpp_limits - compute output link bpp limits
>> - * @intel_dp: intel DP
>> - * @crtc_state: crtc state
>> - * @dsc: DSC compression mode
>> - * @limits: link configuration limits
>> - *
>> - * Calculates the output link min, max bpp values in @limits based on the
>> - * pipe bpp range, @crtc_state and @dsc mode.
>> - *
>> - * Returns %true in case of success.
>> +/*
>> + * Calculate the output link min, max bpp values in limits based on the pipe bpp
>> + * range, crtc_state and dsc mode. Return true on success.
>>    */
>> -bool
>> +static bool
>>   intel_dp_compute_config_link_bpp_limits(struct intel_dp *intel_dp,
>>   					const struct intel_crtc_state *crtc_state,
>>   					bool dsc,
>> @@ -2515,29 +2507,47 @@ intel_dp_compute_config_link_bpp_limits(struct intel_dp *intel_dp,
>>   	return true;
>>   }
>>   
>> -static bool
>> +bool
>>   intel_dp_compute_config_limits(struct intel_dp *intel_dp,
>>   			       struct intel_crtc_state *crtc_state,
>>   			       bool respect_downstream_limits,
>>   			       bool dsc,
>>   			       struct link_config_limits *limits)
>>   {
>> +	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
>> +
>>   	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 */
>> -	limits->max_rate = min(limits->max_rate, 810000);
>> +	if (!is_mst)
>> +		limits->max_rate = min(limits->max_rate, 810000);
>>   	limits->min_rate = min(limits->min_rate, limits->max_rate);
>>   
>>   	limits->min_lane_count = intel_dp_min_lane_count(intel_dp);
>>   	limits->max_lane_count = intel_dp_max_lane_count(intel_dp);
>>   
>>   	limits->pipe.min_bpp = intel_dp_min_bpp(crtc_state->output_format);
>> -	limits->pipe.max_bpp = intel_dp_max_bpp(intel_dp, crtc_state,
>> -						     respect_downstream_limits);
>> +	if (is_mst) {
>> +		/*
>> +		 * FIXME: If all the streams can't fit into the link with their
>> +		 * current pipe_bpp we should reduce pipe_bpp across the board
>> +		 * until things start to fit. Until then we limit to <= 8bpc
>> +		 * since that's what was hardcoded for all MST streams
>> +		 * previously. This hack should be removed once we have the
>> +		 * proper retry logic in place.
>> +		 */
>> +		limits->pipe.max_bpp = min(crtc_state->pipe_bpp, 24);
>> +	} else {
>> +		limits->pipe.max_bpp = intel_dp_max_bpp(intel_dp, crtc_state,
>> +							respect_downstream_limits);
>> +	}
>>   
>> -	if (intel_dp->use_max_params) {
>> +	if (is_mst || intel_dp->use_max_params) {
>>   		/*
>> +		 * For MST we always configure max link bw - the spec doesn't
>> +		 * seem to suggest we should do otherwise.
>> +		 *
>>   		 * Use the maximum clock and number of lanes the eDP panel
>>   		 * advertizes being capable of in case the initial fast
>>   		 * optimal params failed us. The panels are generally
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>> index 48f10876be65..8572d7df5335 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>> @@ -193,11 +193,11 @@ void intel_dp_invalidate_source_oui(struct intel_dp *intel_dp);
>>   void intel_dp_wait_source_oui(struct intel_dp *intel_dp);
>>   int intel_dp_output_bpp(enum intel_output_format output_format, int bpp);
>>   
>> -bool
>> -intel_dp_compute_config_link_bpp_limits(struct intel_dp *intel_dp,
>> -					const struct intel_crtc_state *crtc_state,
>> -					bool dsc,
>> -					struct link_config_limits *limits);
>> +bool intel_dp_compute_config_limits(struct intel_dp *intel_dp,
>> +				    struct intel_crtc_state *crtc_state,
>> +				    bool respect_downstream_limits,
>> +				    bool dsc,
>> +				    struct link_config_limits *limits);
>>   
>>   void intel_dp_get_dsc_sink_cap(u8 dpcd_rev, struct intel_connector *connector);
>>   bool intel_dp_has_gamut_metadata_dip(struct intel_encoder *encoder);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> index 123c4ece6268..d77ebcb1432e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> @@ -585,33 +585,8 @@ mst_stream_compute_config_limits(struct intel_dp *intel_dp,
>>   				 bool dsc,
>>   				 struct link_config_limits *limits)
>>   {
>> -	/*
>> -	 * for MST we always configure max link bw - the spec doesn't
>> -	 * seem to suggest we should do otherwise.
>> -	 */
>> -	limits->min_rate = limits->max_rate =
>> -		intel_dp_max_link_rate(intel_dp);
>> -
>> -	limits->min_lane_count = limits->max_lane_count =
>> -		intel_dp_max_lane_count(intel_dp);
>> -
>> -	limits->pipe.min_bpp = intel_dp_min_bpp(crtc_state->output_format);
>> -	/*
>> -	 * FIXME: If all the streams can't fit into the link with
>> -	 * their current pipe_bpp we should reduce pipe_bpp across
>> -	 * the board until things start to fit. Until then we
>> -	 * limit to <= 8bpc since that's what was hardcoded for all
>> -	 * MST streams previously. This hack should be removed once
>> -	 * we have the proper retry logic in place.
>> -	 */
>> -	limits->pipe.max_bpp = min(crtc_state->pipe_bpp, 24);
>> -
>> -	intel_dp_test_compute_config(intel_dp, crtc_state, limits);
>> -
>> -	if (!intel_dp_compute_config_link_bpp_limits(intel_dp,
>> -						     crtc_state,
>> -						     dsc,
>> -						     limits))
>> +	if (!intel_dp_compute_config_limits(intel_dp, crtc_state, false, dsc,
>> +					    limits))
>>   		return false;
>>   
>>   	return adjust_limits_for_dsc_hblank_expansion_quirk(connector,

-- 
Jani Nikula, Intel