[PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Abhinav Kumar]

On 1/9/2023 7:00 PM, Dmitry Baryshkov wrote:
> On 10/01/2023 04:56, Abhinav Kumar wrote:
>>
>>
>> On 1/9/2023 6:47 PM, Dmitry Baryshkov wrote:
>>> On 10/01/2023 04:40, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/9/2023 5:19 PM, Dmitry Baryshkov wrote:
>>>>> On 27/10/2022 20:36, Dmitry Baryshkov wrote:
>>>>>> On 22/09/2022 03:49, Abhinav Kumar wrote:
>>>>>>> Currently there is no protection against a user trying to set
>>>>>>> an unsupported mode on DSI. Implement a check based on the opp
>>>>>>> table whether the byte clock for the mode can be supported by
>>>>>>> validating whether an opp table entry exists.
>>>>>>>
>>>>>>> For devices which have not added opp table support yet, skip
>>>>>>> this check otherwise it will break bootup on those devices.
>>>>>>>
>>>>>>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
>>>>>>> Reported-by: Rob Clark <robdclark at gmail.com>
>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
>>>>>>>   1 file changed, 23 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>>> index 3a1417397283..87b518c42965 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>>> @@ -450,6 +450,29 @@ static enum drm_mode_status 
>>>>>>> dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>>>>>>       int id = dsi_mgr_bridge_get_id(bridge);
>>>>>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>>>>>       struct mipi_dsi_host *host = msm_dsi->host;
>>>>>>> +    struct platform_device *pdev = msm_dsi->pdev;
>>>>>>> +    struct dev_pm_opp *opp;
>>>>>>> +    struct opp_table *opp_tbl;
>>>>>>> +    unsigned long byte_clk_rate;
>>>>>>> +
>>>>>>> +    byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), 
>>>>>>> mode);
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * first check if there is an opp table available for the 
>>>>>>> calculated
>>>>>>> +     * byte clock and then check DSC related info. Some devices 
>>>>>>> have not
>>>>>>> +     * added support for OPP table. Skip the check for those.
>>>>>>> +     */
>>>>>>> +    opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);
>>>>>>
>>>>>> Can we store the table inside the msm_dsi during the init? Then we 
>>>>>> won't have to get it again and again during each mode_valid call.
>>>>>
>>>>> I checked other drivers. I think we can skip the get_opp_table 
>>>>> completely, can we not? Just handle ENODEV returned from 
>>>>> dev_pm_opp_find_freq_ceil().
>>>>>
>>>>
>>>> Your point is valid but I had a doubt on that API.
>>>>
>>>> As per the documentation of that API, it says
>>>>
>>>> 639  * Return: matching *opp and refreshes *freq accordingly, else 
>>>> returns
>>>> 640  * ERR_PTR in case of error and should be handled using IS_ERR. 
>>>> Error return
>>>> 641  * values can be:
>>>> 642  * EINVAL:    for bad pointer
>>>> 643  * ERANGE:    no match found for search
>>>> 644  * ENODEV:    if device not found in list of registered devices
>>>> 645  *
>>>> 646  * The callers are required to call dev_pm_opp_put() for the 
>>>> returned OPP after
>>>> 647  * use.
>>>> 648  */
>>>> 649 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
>>>> 650                          unsigned long *freq)
>>>> 651 {
>>>>
>>>> So ideally yes, ENODEV means that table was not found but .... that 
>>>> API uses _find_opp_table under the hood.
>>>>
>>>> which says
>>>>
>>>> 79  * Return: pointer to 'struct opp_table' if found, otherwise 
>>>> -ENODEV or
>>>> 80  * -EINVAL based on type of error.
>>>> 81  *
>>>> 82  * The callers must call dev_pm_opp_put_opp_table() after the 
>>>> table is used.
>>>>
>>>> Now, how would we know if the failure was due to table not found OR 
>>>> entry not found.
>>>>
>>>> Table now found means that SOC has probably not started using OPP 
>>>> table which is alright and not an error.
>>>>
>>>> But EINVAL could mean an entry not found which means it exceeds the 
>>>> opp table limits.
>>>
>>> I think this would be -ERANGE as documented.
>>>
>>> EINVAL means dev is null or something of the same kind.
>>>
>>
>> Okay, so EINVAL and ERANGE are genuine errors but ENODEV is not in our 
>> case as all SOCs might not have opp table yet.
>>
>> Do you still think selective handling of these two errors is better 
>> than the current implementation? That way its separation is clear.
> 
> I think that we should drop the opp_table handling.
> Then select basing on the returned error:
>   ERANGE => MODE_CLOCK_HIGH
>   EINVAL and any other error => MODE_ERROR
>   ENODEV => skip the check, continue with dsc
> 
> See below.
> 

Ack, lets do it this way.

I am clear now, will spin a v2.

>>
>>>>
>>>> So there was some ambiguity on this. So I broke it down into two calls.
>>>>
>>>> If my concern is invalid, let me know.
>>>>
>>>> But I do agree with you that we can cache the opp table once rather 
>>>> than doing it in every mode_valid().
>>>>
>>>>>>
>>>>>>> +    if (opp_tbl) {
>>>>>>> +        opp = dev_pm_opp_find_freq_ceil(&pdev->dev, 
>>>>>>> &byte_clk_rate);
>>>>>>> +        if (IS_ERR(opp)) {
>>>>>>> +            pr_err("opp table not found for freq %lu err: %ld\n",
>>>>>>> +                    byte_clk_rate, PTR_ERR(opp));
>>>>>>> +            return PTR_ERR(opp);
> 
> Note: mode_valid returns `enum drm_mode_status`, so we can not return 
> PTR_ERR here.
> 
>>>>>>> +        }
>>>>>>> +        dev_pm_opp_put(opp);
>>>>>>> +        dev_pm_opp_put_opp_table(opp_tbl);
>>>>>>> +    }
>>>>>>>       return msm_dsi_host_check_dsc(host, mode);
>>>>>>>   }
>>>>>>
>>>>>
>>>
>