| Message ID | 20231107004424.2112698-1-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm/dp: call dp_display_get_next_bridge() during probe | expand |
On 7.11.2023 01:43, Dmitry Baryshkov wrote: > The funcion dp_display_get_next_bridge() can return -EPROBE_DEFER if the > next bridge is not (yet) available. However returning -EPROBE_DEFER from > msm_dp_modeset_init() is not ideal. This leads to -EPROBE return from > component_bind, which can easily result in -EPROBE_DEFR loops. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> # sc8180x-primus Konrad
On Tue, Nov 07, 2023 at 02:43:33AM +0200, Dmitry Baryshkov wrote: > The funcion dp_display_get_next_bridge() can return -EPROBE_DEFER if the > next bridge is not (yet) available. However returning -EPROBE_DEFER from > msm_dp_modeset_init() is not ideal. This leads to -EPROBE return from > component_bind, which can easily result in -EPROBE_DEFR loops. > Nice! > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > > Dependencies: https://patchwork.freedesktop.org/series/120375/ > > --- > drivers/gpu/drm/msm/dp/dp_display.c | 36 +++++++++++++++++------------ > 1 file changed, 21 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index d542db37763a..ddb3c84f39a2 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1197,15 +1197,27 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde > return NULL; > } > > -static int dp_auxbus_done_probe(struct drm_dp_aux *aux) > +static int dp_display_get_next_bridge(struct msm_dp *dp); > + > +static int dp_display_probe_tail(struct device *dev) > { > - int rc; > + struct msm_dp *dp = dev_get_drvdata(dev); > + int ret; > > - rc = component_add(aux->dev, &dp_display_comp_ops); > - if (rc) > - DRM_ERROR("eDP component add failed, rc=%d\n", rc); > + ret = dp_display_get_next_bridge(dp); > + if (ret) > + return ret; > > - return rc; > + ret = component_add(dev, &dp_display_comp_ops); > + if (ret) > + DRM_ERROR("component add failed, rc=%d\n", ret); > + > + return ret; > +} > + > +static int dp_auxbus_done_probe(struct drm_dp_aux *aux) > +{ > + return dp_display_probe_tail(aux->dev); > } > > static int dp_display_probe(struct platform_device *pdev) > @@ -1280,11 +1292,9 @@ static int dp_display_probe(struct platform_device *pdev) > goto err; > } > } else { > - rc = component_add(&pdev->dev, &dp_display_comp_ops); > - if (rc) { > - DRM_ERROR("component add failed, rc=%d\n", rc); > + rc = dp_display_probe_tail(&pdev->dev); > + if (rc) > goto err; > - } > } > > return rc; > @@ -1415,7 +1425,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) > * For DisplayPort interfaces external bridges are optional, so > * silently ignore an error if one is not present (-ENODEV). > */ > - rc = devm_dp_parser_find_next_bridge(dp->drm_dev->dev, dp_priv->parser); > + rc = devm_dp_parser_find_next_bridge(&dp->pdev->dev, dp_priv->parser); This transition worried me, but after reading the code the current model of mixing devices for devres scares me more. So, nice cleanup! But I think we have a few more of these... That said, &dp->pdev->dev is dp_priv->parser->dev, the function no longer relate to the "parser module", and we stash the return value of devm_drm_of_get_bridge(dev, dev->of_node, 1, 0) in parser->next_brigde, so that we 5 lines below this call can move it into dp->next_bridge. As such, I'd like to propose that we change devm_dp_parser_find_next_bridge() to just take &dp->pdev->dev and return the next_bridge, in an ERR_PTR(). But that's follow-up-patch material. Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com> Regards, Bjorn > if (!dp->is_edp && rc == -ENODEV) > return 0; > > @@ -1435,10 +1445,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > > dp_priv = container_of(dp_display, struct dp_display_private, dp_display); > > - ret = dp_display_get_next_bridge(dp_display); > - if (ret) > - return ret; > - > ret = dp_bridge_init(dp_display, dev, encoder); > if (ret) { > DRM_DEV_ERROR(dev->dev, > -- > 2.42.0 > >
On 11/6/2023 4:43 PM, Dmitry Baryshkov wrote: > The funcion dp_display_get_next_bridge() can return -EPROBE_DEFER if the > next bridge is not (yet) available. However returning -EPROBE_DEFER from > msm_dp_modeset_init() is not ideal. This leads to -EPROBE return from > component_bind, which can easily result in -EPROBE_DEFR loops. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > > Dependencies: https://patchwork.freedesktop.org/series/120375/ > > --- Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > drivers/gpu/drm/msm/dp/dp_display.c | 36 +++++++++++++++++------------ > 1 file changed, 21 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index d542db37763a..ddb3c84f39a2 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1197,15 +1197,27 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde > return NULL; > } > > -static int dp_auxbus_done_probe(struct drm_dp_aux *aux) > +static int dp_display_get_next_bridge(struct msm_dp *dp); > + > +static int dp_display_probe_tail(struct device *dev) > { > - int rc; > + struct msm_dp *dp = dev_get_drvdata(dev); > + int ret; > > - rc = component_add(aux->dev, &dp_display_comp_ops); > - if (rc) > - DRM_ERROR("eDP component add failed, rc=%d\n", rc); > + ret = dp_display_get_next_bridge(dp); > + if (ret) > + return ret; > > - return rc; > + ret = component_add(dev, &dp_display_comp_ops); > + if (ret) > + DRM_ERROR("component add failed, rc=%d\n", ret); > + > + return ret; > +} > + > +static int dp_auxbus_done_probe(struct drm_dp_aux *aux) > +{ > + return dp_display_probe_tail(aux->dev); > } > > static int dp_display_probe(struct platform_device *pdev) > @@ -1280,11 +1292,9 @@ static int dp_display_probe(struct platform_device *pdev) > goto err; > } > } else { > - rc = component_add(&pdev->dev, &dp_display_comp_ops); > - if (rc) { > - DRM_ERROR("component add failed, rc=%d\n", rc); > + rc = dp_display_probe_tail(&pdev->dev); > + if (rc) > goto err; > - } > } > > return rc; > @@ -1415,7 +1425,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) > * For DisplayPort interfaces external bridges are optional, so > * silently ignore an error if one is not present (-ENODEV). > */ > - rc = devm_dp_parser_find_next_bridge(dp->drm_dev->dev, dp_priv->parser); > + rc = devm_dp_parser_find_next_bridge(&dp->pdev->dev, dp_priv->parser); > if (!dp->is_edp && rc == -ENODEV) > return 0; > > @@ -1435,10 +1445,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > > dp_priv = container_of(dp_display, struct dp_display_private, dp_display); > > - ret = dp_display_get_next_bridge(dp_display); > - if (ret) > - return ret; > - > ret = dp_bridge_init(dp_display, dev, encoder); > if (ret) { > DRM_DEV_ERROR(dev->dev,
On Tue, 07 Nov 2023 02:43:33 +0200, Dmitry Baryshkov wrote: > The funcion dp_display_get_next_bridge() can return -EPROBE_DEFER if the > next bridge is not (yet) available. However returning -EPROBE_DEFER from > msm_dp_modeset_init() is not ideal. This leads to -EPROBE return from > component_bind, which can easily result in -EPROBE_DEFR loops. > > Applied, thanks! [1/1] drm/msm/dp: call dp_display_get_next_bridge() during probe https://gitlab.freedesktop.org/lumag/msm/-/commit/6a242720884f Best regards,
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index d542db37763a..ddb3c84f39a2 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1197,15 +1197,27 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde return NULL; } -static int dp_auxbus_done_probe(struct drm_dp_aux *aux) +static int dp_display_get_next_bridge(struct msm_dp *dp); + +static int dp_display_probe_tail(struct device *dev) { - int rc; + struct msm_dp *dp = dev_get_drvdata(dev); + int ret; - rc = component_add(aux->dev, &dp_display_comp_ops); - if (rc) - DRM_ERROR("eDP component add failed, rc=%d\n", rc); + ret = dp_display_get_next_bridge(dp); + if (ret) + return ret; - return rc; + ret = component_add(dev, &dp_display_comp_ops); + if (ret) + DRM_ERROR("component add failed, rc=%d\n", ret); + + return ret; +} + +static int dp_auxbus_done_probe(struct drm_dp_aux *aux) +{ + return dp_display_probe_tail(aux->dev); } static int dp_display_probe(struct platform_device *pdev) @@ -1280,11 +1292,9 @@ static int dp_display_probe(struct platform_device *pdev) goto err; } } else { - rc = component_add(&pdev->dev, &dp_display_comp_ops); - if (rc) { - DRM_ERROR("component add failed, rc=%d\n", rc); + rc = dp_display_probe_tail(&pdev->dev); + if (rc) goto err; - } } return rc; @@ -1415,7 +1425,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) * For DisplayPort interfaces external bridges are optional, so * silently ignore an error if one is not present (-ENODEV). */ - rc = devm_dp_parser_find_next_bridge(dp->drm_dev->dev, dp_priv->parser); + rc = devm_dp_parser_find_next_bridge(&dp->pdev->dev, dp_priv->parser); if (!dp->is_edp && rc == -ENODEV) return 0; @@ -1435,10 +1445,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, dp_priv = container_of(dp_display, struct dp_display_private, dp_display); - ret = dp_display_get_next_bridge(dp_display); - if (ret) - return ret; - ret = dp_bridge_init(dp_display, dev, encoder); if (ret) { DRM_DEV_ERROR(dev->dev,
The funcion dp_display_get_next_bridge() can return -EPROBE_DEFER if the next bridge is not (yet) available. However returning -EPROBE_DEFER from msm_dp_modeset_init() is not ideal. This leads to -EPROBE return from component_bind, which can easily result in -EPROBE_DEFR loops. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Dependencies: https://patchwork.freedesktop.org/series/120375/ --- drivers/gpu/drm/msm/dp/dp_display.c | 36 +++++++++++++++++------------ 1 file changed, 21 insertions(+), 15 deletions(-)