| Message ID | 20230111225257.7510-2-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/msm/dsi: add a helper method to compute the dsi byte clk | expand |
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@gmail.com> > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > 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. > + 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. > + } else { > + dev_pm_opp_put(opp); > + } > > return msm_dsi_host_check_dsc(host, mode); > }
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@gmail.com> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> --- >> 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); >> } >
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 + */ + 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; + } else { + dev_pm_opp_put(opp); + } return msm_dsi_host_check_dsc(host, mode); }
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@gmail.com> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)