On Mon, 12 Aug 2024, "Borah, Chaitanya Kumar" <chaitanya.kumar.borah@xxxxxxxxx> wrote: >> -----Original Message----- >> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> >> Sent: Monday, August 12, 2024 5:24 PM >> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>; intel- >> gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx >> Cc: Deak, Imre <imre.deak@xxxxxxxxx>; Borah, Chaitanya Kumar >> <chaitanya.kumar.borah@xxxxxxxxx> >> 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@xxxxxxxxx> 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@xxxxxxxxx> >> > --- >> > 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
