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

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 1/11/2023 2:56 PM, Dmitry Baryshkov wrote:
> On 12/01/2023 00:52, 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.
> >
> > changes in v2:
> >     - drop dev_pm_opp_get_opp_table() usage
> >
> > Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
> > Reported-by: Rob Clark <robdclark@xxxxxxxxx>
> > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> > b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 3a1417397283..c4c24dabfd6f 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -450,6 +450,31 @@ 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;
> > +    unsigned long byte_clk_rate;
> > +
> > +    byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), mode);
> > +
> > +    /*
> > +     * If dev_pm_opp_find_freq_ceil() returns -EINVAL, its a bad
> > +     * pointer being passed, so treat as an error and return MODE_ERROR
> > +     *
> > +     * If dev_pm_opp_find_freq_ceil() returns -ERANGE, no clock
> > +     * was found matching the byte_clk, so return MODE_CLOCK_RANGE
> > +     *
> > +     * If dev_pm_opp_find_freq_ceil() returns -ENODEV, don't treat
> > +     * it as an error as it could mean opp table is not implemented
> > +     */
>
> I'd say, too lengthy and verbose.
hmmm .... just wanted to document the basis of the policy, I can make it 
shorter.

> > +    opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
> > +    if (IS_ERR(opp)) {
> > +        if (PTR_ERR(opp) == -EINVAL)
> > +            return MODE_ERROR;
> > +        else if (PTR_ERR(opp) == -ERANGE)
> > +            return MODE_CLOCK_RANGE;
>
> I think the logic should be quite opposite. Ignore -ENODEV, handle 
> -ERANGE and return MODE_ERROR for all other errors. In your code you are 
> ignoring not only -ENODEV, but all other possible errors which can be 
> added in future.

Atm, those were the only ones handled but you are right.

this is better.

if (IS_ERR(opp)) {
	if (PTR_ERR(opp) == -ERANGE)
		return MODE_CLOCK_RANGE;
	else
		return MODE_ERROR;
} else {
	dev_pm_opp_put(opp);
}

> > +    } else {
> > +        dev_pm_opp_put(opp);
> > +    }
> >       return msm_dsi_host_check_dsc(host, mode);
> >   }