Re: [PATCH 03/12] drm/i915/dp: Separate out helper for compute fec_enable

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 11/20/2024 5:22 PM, Jani Nikula wrote:
> On Wed, 20 Nov 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> Make a separate function for setting fec_enable in crtc_state.
>> Drop the check for FEC support as its already checked while checking for
>> DSC support.
> That's two changes that generally shouldn't be bundled together.
>
> Aim for separating non-functional refactoring and functional
> changes. (Well, arguably dropping the FEC check should also be
> non-functional, but you know what I mean.)

Moving to separate helper should indeed have non functional change and 
dropping the check can be another patch.

Initially I was going with a separate patch for dropping the FEC check, 
but couldn't make up my mind, and merged the two things. :)

Will do as suggested.


>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dp.c | 30 +++++++++++++++++--------
>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index dee15a05e7fd..d82e25d0dc5a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -2352,6 +2352,26 @@ static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
>>   	return 0;
>>   }
>>   
>> +static void intel_dp_compute_fec_config(struct intel_dp *intel_dp,
>> +					struct intel_crtc_state *pipe_config)
> I think all encoder->callback_name hooks should be named
> something_something_callback_name(), and the same goes for helpers
> specifically aimed at this.
>
> This would make the function intel_dp_fec_compute_config().
>
> Yes, in many ways "compute fec config" reads better, but there's value
> in being able to search for "_compute_config", and to know this is only
> for he ->compute_config path.

Makes sense to have _fec_compute_config. Will change this.

Thanks Jani, for the comments and suggestions.

Regards,

Ankit



>
> BR,
> Jani.
>
>
>> +{
>> +	if (pipe_config->fec_enable)
>> +		return;
>> +
>> +	/*
>> +	 * Though eDP v1.5 supports FEC with DSC, unlike DP, it is optional.
>> +	 * Since, FEC is a bandwidth overhead, continue to not enable it for
>> +	 * eDP. Until, there is a good reason to do so.
>> +	 */
>> +	if (intel_dp_is_edp(intel_dp))
>> +		return;
>> +
>> +	if (intel_dp_is_uhbr(pipe_config))
>> +		return;
>> +
>> +	pipe_config->fec_enable = true;
>> +}
>> +
>>   int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>   				struct intel_crtc_state *pipe_config,
>>   				struct drm_connector_state *conn_state,
>> @@ -2368,15 +2388,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>   	int num_joined_pipes = intel_crtc_num_joined_pipes(pipe_config);
>>   	int ret;
>>   
>> -	/*
>> -	 * Though eDP v1.5 supports FEC with DSC, unlike DP, it is optional.
>> -	 * Since, FEC is a bandwidth overhead, continue to not enable it for
>> -	 * eDP. Until, there is a good reason to do so.
>> -	 */
>> -	pipe_config->fec_enable = pipe_config->fec_enable ||
>> -		(!intel_dp_is_edp(intel_dp) &&
>> -		 intel_dp_supports_fec(intel_dp, connector, pipe_config) &&
>> -		 !intel_dp_is_uhbr(pipe_config));
>> +	intel_dp_compute_fec_config(intel_dp, pipe_config);
>>   
>>   	if (!intel_dp_dsc_supports_format(connector, pipe_config->output_format))
>>   		return -EINVAL;