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

[Nautiyal, Ankit K]

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