On Mon, Nov 01, 2021 at 09:45:23AM -0700, Kulkarni, Vandita wrote:
> > -----Original Message-----
> > From: Navare, Manasi D <manasi.d.navare@xxxxxxxxx>
> > Sent: Saturday, October 30, 2021 4:43 AM
> > To: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nikula, Jani <jani.nikula@xxxxxxxxx>
> > Subject: Re: [PATCH] drm/i915/dsc: Fix the usage of uncompressed bpp
> >
> > On Wed, Oct 27, 2021 at 09:37:10PM -0700, Kulkarni, Vandita wrote:
> > > > -----Original Message-----
> > > > From: Navare, Manasi D <manasi.d.navare@xxxxxxxxx>
> > > > Sent: Thursday, October 28, 2021 12:57 AM
> > > > To: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>
> > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nikula, Jani
> > > > <jani.nikula@xxxxxxxxx>
> > > > 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?
> > >
> >
> > Yes so this driver policy was decided based on some feedback that I had got
> > from the community as well as internal feedback from Ville that the decision
> > of input_bpc should be based on max_bpc requested by the user through the
> > connector property and max_bpc supported by the platform.
> > Here we take the min of the two so that we dont violate either of the max
> > constrains.
> > This was primarily suggested by Ville since he said that we should always
> > respect what user has set as the upper limit in the bpc because this could be
> > for example driven by the fact that OEM's panel has a limitation or issues
> > with higher bpcs or a display requirement for certain use case.
> > Hence while we want to support as high bpc as supported by the platform
> > and sink to have better display quality, it should not exceed the max limit set
> > by the user through the property.
> >
> > 8 and 10bpc will be supported but we want to start with supporting the max
> > we can, going down from there if needed.
> > So for compression, we chose the maximum input bpc and determine the
> > compressed bpp for that.
> >
> >
> > > 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.
> >
> > Well the only time we force the dsc_enable is for IGT DSC tests and thats
> > okay for compression bpp to be higher since there we are just validation the
> > functionality and its not for actual user/use case.
>
> We are having these test cases as part of CI. We hit FIFO underrun in such cases and that's treated as a fail.
>
> >
> > In the actual use case we will only enable DSC when the available max link
> > rate/lane count does not support the minimum bpp of 18 (min bpc of 6 * 3)
> > So then in that case we say that lets keep the pipe_bpp or input bpp as
> > max_supported_input_bpc * 3
> > >
> > > 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.
> >
> > So the Bspec says that if the input BPP is (8 * 3) = 24, then based on the
> > engine's compression ratio capability it can support output compressed BPP
> > of: 8 - 23 in the increments of 1 BPP And the maximum that we can pick
> > between 8 - 23 will depend on the min(max bits_per_pixel that the max link
> > rate, lane count can handle, max bits per pixel by small joiner RAM calc).
>
> In case of force dsc, as part of the IGT tests, the link can actually support more than
> 24bpp, since we are forcing have seen a case where it could support 28bpp.
>
>From IGT we are forcing DSC for all supported input BPCs right, so for bpc = 8,
so input bpp = 24. So IMO in the kernel code, when we calculate pipe_bpp when force_dsc is set in that case we should use the bpc requested
from IGT as inpput bpc and not care for max_connector_bpc
So then input bpp = 24 and the limits of compressed bpp would be 8 - 23
Ideally now the link rate and lane count should be lowered to use the least required to support that compressed bpp
Then we wont see any underruns.
Manasi
> So min of (28,(36-1)) would be 28 and not as per the spec.
>
> Hence in either of the places, kernel or igt we need to honour the limits
> Of bpc*3 as per the DP 1.4 spec.
>
> If you look at the IGT we have cases of 8bpc, 10 bpc
> We need to address this either in the driver by getting the actual uncompressed bpp limits
> Or in the user space by changing the max_requested_bpc for 8bpc and 10bpc cases.
>
> This patch is tested and resolves the FIFO underruns, in force dsc cases.
> The problem is only when we are forcing DSC and the link rate can actually support higher bpp.
> And we have multiple igts trying to use 8bpc and 10bpc.
>
> Thanks
> Vandita
>
> >
> > So we are good here in terms of how we calculate our compressed bpp.
> > However what you should once double check is for XeLPD (>=13) platforms,
> > you just pick min (bits_per_pixel, pipe_bpp - 1) But I remember seeing that
> > the spec still says target bpp has to be 8 to 27 even when supported input bpc
> > are 8/10/12 (please check if it actually goes all the way from 8 to 35)
> >
> > Manasi
> >
> > >
> > > Thanks,
> > > Vandita
> > >
> > > > Manasi
> > > >
> > > > >
> > > > > Fixes: 831d5aa96c97 ("drm/i915/xelpd: Support DP1.4 compression
> > > > > BPPs")
> > > > > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> > > > > ---
> > > > > 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
> > > > >
