Re: [Intel-gfx] [PATCH] drm/i915/dsc: Fix the usage of uncompressed bpp

["Kulkarni, Vandita" <vandita.kulkarni@intel.com>]

> -----Original Message-----
> From: Navare, Manasi D <manasi.d.navare@intel.com>
> Sent: Thursday, October 28, 2021 12:57 AM
> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>
> Subject: Re: [PATCH] drm/i915/dsc: Fix the usage of uncompressed bpp
> 
> On Wed, Oct 27, 2021 at 03:23:16PM +0530, Vandita Kulkarni wrote:
> > DP 1.4 spec limits max compression bpp to uncompressed bpp -1, which
> > is supported from XELPD onwards.
> > Instead of uncompressed bpp, max dsc input bpp was being used to limit
> > the max compression bpp.
> 
> So the input Pipe BPP which is the uncompressed bpp is decided by the input
> bpc and when this was initially written, we had designed it to respect the
> max_req_bpc by the user.
> So that is what we use to decide the input bpc and hence the pipe_bpp This
> input pipe_bpp decides the compressed bpp that we calculate based on all
> the supported output bpps which are supported all the way upto
> uncompressed_output_bpp - 1.
> 
> So I dont see the need to change the logic here. Moreover I dont see any
> change in the dsc_compute_bpp function So I dont understand the purpose
> of introducing the new max_dsc_pipe_bpp variable here

Thanks for the comments, I had few more opens around this along with this patch.

AFAIU about max_requested_bpc it is to limit the max_bpc
"drm: Add connector property to limit max bpc"
And the driver is supposed to program the default bpc as per the connector limitation.
Which is 12 as per the current driver implementation.

I had few queries around this design:
So it means that max_dsc_input_bpp would be set to 36 if supported by the sink and the platform.
And now we make this as our pipe_bpp,
1. Does this mean that we are assuming 12bpc as i/p always?
2. What happens to those with formats 8bpc, 10 bpc?

We do not consider the actual pipe_bpp while computing the compression_bpp, 
We reverse calculate it from the link_rate,  and small joiner bpp limits.
In cases of forcing dsc, we might have a situation where the link can actually support the current bpp, or even more.
But we are forcing the dsc enable.
In such cases we might end up with a compression bpp which is higher than the actual i/p bpp.

Now, even if we take a min of  higher compression bpp against max_requested_bpc *3 -1, we still have
Compression bpp > actual pipe_bpp 

As per the spec when they say uncompressed bpp, they actually mean 3 * bpc
If we go ahead with this approach of using max_requested_bpc , which is 12 always we cannot
adhere to the spec.

Thanks,
Vandita

> Manasi
> 
> >
> > Fixes: 831d5aa96c97 ("drm/i915/xelpd: Support DP1.4 compression BPPs")
> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 9d8132dd4cc5..1f7e666ae490 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1322,7 +1322,7 @@ static int intel_dp_dsc_compute_config(struct
> intel_dp *intel_dp,
> >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
> >base.base.dev);
> >  	const struct drm_display_mode *adjusted_mode =
> >  		&pipe_config->hw.adjusted_mode;
> > -	int pipe_bpp;
> > +	int pipe_bpp, max_dsc_pipe_bpp;
> >  	int ret;
> >
> >  	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && @@ -
> 1331,7
> > +1331,8 @@ static int intel_dp_dsc_compute_config(struct intel_dp
> *intel_dp,
> >  	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
> >  		return -EINVAL;
> >
> > -	pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state-
> >max_requested_bpc);
> > +	pipe_bpp = pipe_config->pipe_bpp;
> > +	max_dsc_pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp,
> > +conn_state->max_requested_bpc);
> >
> >  	/* Min Input BPC for ICL+ is 8 */
> >  	if (pipe_bpp < 8 * 3) {
> > @@ -1345,7 +1346,7 @@ static int intel_dp_dsc_compute_config(struct
> intel_dp *intel_dp,
> >  	 * Optimize this later for the minimum possible link rate/lane count
> >  	 * with DSC enabled for the requested mode.
> >  	 */
> > -	pipe_config->pipe_bpp = pipe_bpp;
> > +	pipe_config->pipe_bpp = max_dsc_pipe_bpp;
> >  	pipe_config->port_clock = limits->max_rate;
> >  	pipe_config->lane_count = limits->max_lane_count;
> >
> > --
> > 2.32.0
> >