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

Abhinav Kumar quic_abhinavk at quicinc.com
Tue Jan 10 02:56:39 UTC 2023



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.

>>
>> 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);
>>>>> +        }
>>>>> +        dev_pm_opp_put(opp);
>>>>> +        dev_pm_opp_put_opp_table(opp_tbl);
>>>>> +    }
>>>>>       return msm_dsi_host_check_dsc(host, mode);
>>>>>   }
>>>>
>>>
> 


More information about the dri-devel mailing list