| Message ID | 20230411173408.1945921-1-navaremanasi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Return correct err code for bpc < 0 | expand |
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. > Fix this by returning > -EINVAL in case bpc < 0 which would be an error. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > Cc: Sean Paul <sean@poorly.run> > Signed-off-by: Manasi Navare <navaremanasi@chromium.org> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index f0bace9d98a1..f6546292e7c6 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -1280,7 +1280,7 @@ static int intel_dp_max_bpp(struct intel_dp *intel_dp, > max_hdmi_bpc = intel_dp_hdmi_compute_bpc(intel_dp, crtc_state, bpc, > respect_downstream_limits); > if (max_hdmi_bpc < 0) > - return 0; > + return -EINVAL; > > bpc = min(bpc, max_hdmi_bpc); > } > -- > 2.40.0.577.gac1e443424-goog
On Tue, Apr 11, 2023 at 10:42 AM Ville Syrjälä <ville.syrjala@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. Manasi > > > Fix this by returning > > -EINVAL in case bpc < 0 which would be an error. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > Cc: Sean Paul <sean@poorly.run> > > Signed-off-by: Manasi Navare <navaremanasi@chromium.org> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index f0bace9d98a1..f6546292e7c6 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -1280,7 +1280,7 @@ static int intel_dp_max_bpp(struct intel_dp *intel_dp, > > max_hdmi_bpc = intel_dp_hdmi_compute_bpc(intel_dp, crtc_state, bpc, > > respect_downstream_limits); > > if (max_hdmi_bpc < 0) > > - return 0; > > + return -EINVAL; > > > > bpc = min(bpc, max_hdmi_bpc); > > } > > -- > > 2.40.0.577.gac1e443424-goog > > -- > Ville Syrjälä > Intel
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@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.
On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä <ville.syrjala@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@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
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@chromium.org> wrote: > > On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä > <ville.syrjala@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@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
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 > > Manasi > > On Thu, Apr 13, 2023 at 8:23 AM Manasi Navare <navaremanasi@chromium.org> wrote: > > > > On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä > > <ville.syrjala@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@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
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? [1] https://patchwork.freedesktop.org/patch/519346/?series=111391&rev=3 > >> Manasi >> >> On Thu, Apr 13, 2023 at 8:23 AM Manasi Navare <navaremanasi@chromium.org> wrote: >>> On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä >>> <ville.syrjala@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@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
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@chromium.org> wrote: > >>> On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä > >>> <ville.syrjala@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@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
Hi Ville and Ankit, I actually do not think this is a problem with the DSC logic, but it is a problem with the intel_dp_link_compute_config() where we should do something if max_bpp is 0 instead of just returning a -EINVAL directly. My question here: - In case of PCON, yuv format, is it a valid case to have max bpp set to 0? - This is where I am seeing it as set to 0 - If it isnt then the problem is probably where it computes max bpp for hdmi case for yuv format Manasi On Tue, Apr 18, 2023 at 7:54 AM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > 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@chromium.org> wrote: > > >>> On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä > > >>> <ville.syrjala@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@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
On 4/19/2023 4:12 AM, Manasi Navare wrote: > Hi Ville and Ankit, > > I actually do not think this is a problem with the DSC logic, but it > is a problem with the intel_dp_link_compute_config() where we should > do something if max_bpp is 0 instead of just returning a -EINVAL > directly. > My question here: > - In case of PCON, yuv format, is it a valid case to have max bpp set to 0? > - This is where I am seeing it as set to 0 > - If it isnt then the problem is probably where it computes max bpp > for hdmi case for yuv format I got your point. if limits.max_bpp is set to 0 (as given by intel_dp_max_bpp) and we cannot discard it as mentioned by Ville, then even with DSC we cant do anything. In such a case perhaps it makes sense to check if limit.max_bpp is 0 and return -EINVAL from intel_dp_compute_link_config. Regards, Ankit > > Manasi > > On Tue, Apr 18, 2023 at 7:54 AM Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: >> 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@chromium.org> wrote: >>>>>> On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä >>>>>> <ville.syrjala@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@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
On 4/19/2023 10:29 AM, Nautiyal, Ankit K wrote: > > On 4/19/2023 4:12 AM, Manasi Navare wrote: >> Hi Ville and Ankit, >> >> I actually do not think this is a problem with the DSC logic, but it >> is a problem with the intel_dp_link_compute_config() where we should >> do something if max_bpp is 0 instead of just returning a -EINVAL >> directly. >> My question here: >> - In case of PCON, yuv format, is it a valid case to have max bpp set >> to 0? >> - This is where I am seeing it as set to 0 >> - If it isnt then the problem is probably where it computes max bpp >> for hdmi case for yuv format > > I got your point. if limits.max_bpp is set to 0 (as given by > intel_dp_max_bpp) > > and we cannot discard it as mentioned by Ville, then even with DSC we > cant do anything. > > In such a case perhaps it makes sense to check if limit.max_bpp is 0 > and return -EINVAL from intel_dp_compute_link_config. Though just this change is not sufficient for that. I mean we will need to handle in intel_dp_compute_link_config. > > Regards, > > Ankit > >> >> Manasi >> >> On Tue, Apr 18, 2023 at 7:54 AM Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >>> 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@chromium.org> wrote: >>>>>>> On Tue, Apr 11, 2023 at 10:22 PM Ville Syrjälä >>>>>>> <ville.syrjala@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@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
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index f0bace9d98a1..f6546292e7c6 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -1280,7 +1280,7 @@ static int intel_dp_max_bpp(struct intel_dp *intel_dp, max_hdmi_bpc = intel_dp_hdmi_compute_bpc(intel_dp, crtc_state, bpc, respect_downstream_limits); if (max_hdmi_bpc < 0) - return 0; + return -EINVAL; bpc = min(bpc, max_hdmi_bpc); }
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. Fix this by returning -EINVAL in case bpc < 0 which would be an error. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> Cc: Sean Paul <sean@poorly.run> Signed-off-by: Manasi Navare <navaremanasi@chromium.org> --- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)