> -----Original Message----- > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > Sent: Wednesday, August 14, 2024 2:15 PM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx > Cc: Deak, Imre <imre.deak@xxxxxxxxx> > Subject: RE: [PATCH] drm/i915: Preserve value of fec_enable calculated before > DSC compute config > > 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. > I agree. if I understand correctly this should be covered by the larger separation of DP-MST and MTP planned for UHBR. Until that time, hopefully [1] is acceptable. [1] https://patchwork.freedesktop.org/series/137611/ Regards Chaitanya > BR, > Jani. > > > > > > Regards > > > > Chaitanya > > > >> > >> BR, > >> Jani. > >> > >> > >> -- > >> Jani Nikula, Intel > > -- > Jani Nikula, Intel
