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

[Nautiyal, Ankit K]

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 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