RE: [PATCH] drm/i915: Preserve value of fec_enable calculated before DSC compute config

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

On Mon, 12 Aug 2024, "Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@linux.intel.com>
>> Sent: Monday, August 12, 2024 5:24 PM
>> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; intel-
>> gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Cc: Deak, Imre <imre.deak@intel.com>; Borah, Chaitanya Kumar
>> <chaitanya.kumar.borah@intel.com>
>> Subject: Re: [PATCH] drm/i915: Preserve value of fec_enable calculated before
>> DSC compute config
>> 
>> On Mon, 12 Aug 2024, Chaitanya Kumar Borah
>> <chaitanya.kumar.borah@intel.com> wrote:
>> > Before DSC compute config, fec_enable value is set in other functions
>> > (e.g. intel_dp_mst_find_vcpi_slots_for_bpp). We are ignoring the value
>> > by OR'ing its value in DSC compute config. One unintended effect of
>> > this is setting fec_enable in UHBR use-cases which is not needed for
>> > Intel hardware. Therefore, change operator to AND.
>> >
>> > While at it, add a comment explaining why we don't enable FEC in eDP v1.5.
>> >
>> > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_dp.c | 7 ++++++-
>> >  1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index 49a37b996530..3fbf9f33c3e4 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -2298,7 +2298,12 @@ int intel_dp_dsc_compute_config(struct
>> intel_dp *intel_dp,
>> >  		&pipe_config->hw.adjusted_mode;
>> >  	int ret;
>> >
>> > -	pipe_config->fec_enable = pipe_config->fec_enable ||
>> > +	/*
>> > +	 * 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));
>> 
>> With this change, FEC will only be enabled for non-UHBR MST DP and nothing
>> else. This is the place where SST DP DSC gets FEC enabled.
>
> Thank you Jani for pointing it out.
>
> Something like this should work?
>
> 	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));

Technically yes, although I dislike the fact that we now have fec_enable
and its rules split to multiple locations.

BR,
Jani.


>
> Regards
>
> Chaitanya
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel