[Intel-gfx] [PATCH] drm/i915/display: Return correct err code for bpc < 0

[Ville Syrjälä]

On Tue, Apr 18, 2023 at 08:09:16PM +0530, Nautiyal, Ankit K wrote:
> 
> On 4/18/2023 6:16 PM, Ville Syrjälä wrote:
> > On Mon, Apr 17, 2023 at 03:48:12PM -0700, Manasi Navare wrote:
> >> Hi Ville,
> >>
> >> Could you suggest how to handle the intel_dp_link_compute_config()
> >> when the max_bpp is returned as 0, currently
> >> it just exits the loop and returns a -EINVAL and this triggers the DSC path.
> >> While we should be completely failing the modeset and encoder_config
> >> in this case instead of trying DSC, correct?
> > The DSC path needs to handle the bpp limits correctly:
> > 1. Take the baseline limits already computed
> > 2. Further restrict them based on sink/source DSC capabilities/etc.
> > 3. Make sure the uncompressed bpp value chosen is between the min/max
> 
> I have some older patch to try similar thing [1]. We try to iterate over 
> bpc to find pipe_bpp in the limits, then try to find out compressed_bpp.
> 
> But if intel_dp_max_bpp returns 0, limits.max_bpp is set to 0, so we 
> discard this for dsc case and re-calculate the limits.max_bpp?

You shouldn't discard anything. DSC should take the already computed
limits and potentially just shrink them further based on DSC specific
constraints.

Or is there some weird case where DSC would allow lower/higher bpp
than what our uncompressed bpp limits declare?

> 
> 
> [1] https://patchwork.freedesktop.org/patch/519346/?series=111391&rev=3
> 
> >
> >> Manasi
> >>
> >> On Thu, Apr 13, 2023 at 8:23 AM Manasi Navare <navaremanasi at chromium.org> wrote:
> >>> On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä
> >>> <ville.syrjala at linux.intel.com> wrote:
> >>>> On Tue, Apr 11, 2023 at 05:07:01PM -0700, Manasi Navare wrote:
> >>>>> On Tue, Apr 11, 2023 at 10:42 AM Ville Syrjälä
> >>>>> <ville.syrjala at linux.intel.com> wrote:
> >>>>>> On Tue, Apr 11, 2023 at 05:34:08PM +0000, Manasi Navare wrote:
> >>>>>>> In the function intel_dp_max_bpp(), currently if bpc < 0 in case of error,
> >>>>>>> we return 0 instead of returning an err code of -EINVAL.
> >>>>>>> This throws off the logic in the calling function.
> >>>>>> What logic? The caller doesn't expect to get an error.
> >>>>> If this returns a 0, we end up using limits.max_bpp = 0 and in
> >>>>> intel_dp_compute_link_config_wide(),
> >>>>> since max_bpp is 0, it exits this for loop:
> >>>>>
> >>>>> for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) and returns
> >>>>> -EINVAL which then wrongly goes to enable DSC even when link BW is
> >>>>> sufficient without DSC.
> >>>> And how woud max_bpp<0 prevent that?
> >>>>
> >>>> The real problem seems to be that the DSC code totally
> >>>> ignores bpp limits.
> >>> Hi Ville,
> >>>
> >>> So I see a few concerns/questions:
> >>> - Why is the Max bpp value 0 in intel_dp_max_bpp, is that a valid case
> >>> and how should our link configurations handle that case when max_bpp
> >>> is 0?
> >>> - This is happening in a bug I am looking at with HDMI PCON, @Ankit
> >>> Nautiyal  have we ever seen something similar where max_bpp for HDMi
> >>> PCON
> >>> is returned 0?
> >>> - I dont think its a problem with DSC code, but rather
> >>> intel_dp_compute_link_config() outer for loop where we vary
> >>> from max_bpp to min_bpp and see if any bpp works with available link
> >>> bw, how should we handle this when max_bpp = 0 if you are saying thats
> >>> a valid case?
> >>> - In this patch if I return -EINVAL instead of 0, then atleast the
> >>> entire encoder_config will fail and that will fail the modeset, since
> >>> it assumes max_bpp cannot be 0
> >>>
> >>> Could you please help answer above concerns and how to handle max bpp
> >>> = 0 case if that is valid? This patch is simply making that invalid
> >>> resulting into modeset failure
> >>>
> >>> Manasi
> >>>> --
> >>>> Ville Syrjälä
> >>>> Intel

-- 
Ville Syrjälä
Intel