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

[Manasi Navare]

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?

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