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

[Dmitry Baryshkov]

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.

> 
> 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);
>>>>   }
>>>
>>

-- 
With best wishes
Dmitry