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

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Wednesday, August 14, 2024 2:15 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>; intel-
> gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> Cc: Deak, Imre <imre.deak at intel.com>
> 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 at intel.com> wrote:
> >> -----Original Message-----
> >> From: Jani Nikula <jani.nikula at linux.intel.com>
> >> Sent: Monday, August 12, 2024 5:24 PM
> >> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>; intel-
> >> gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> >> Cc: Deak, Imre <imre.deak at intel.com>; Borah, Chaitanya Kumar
> >> <chaitanya.kumar.borah at 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 at 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 at 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.
> 

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