Hi Thierry,
On Fri, 4 May 2018 12:18:52 +0200
Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> On Thu, May 03, 2018 at 06:40:05PM +0200, Boris Brezillon wrote:
> > Right now, the DRM panel logic returns NULL when a panel pointing to
> > the passed OF node is not present in the list of registered panels.
> >
> > Most drivers interpret this NULL value as -EPROBE_DEFER, but we are
> > about to modify the semantic of of_drm_find_panel() and let the
> > framework return -ENODEV when the device node we're pointing to has
> > a status property that is not equal to "okay" or "ok".
> >
> > Let's first patch the of_drm_find_panel() implementation to return
> > ERR_PTR(-EPROBE_DEFER) instead of NULL and patch all callers to replace
> > the '!panel' check by an 'IS_ERR(panel)' one.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/bridge/cdns-dsi.c | 2 +-
> > drivers/gpu/drm/bridge/lvds-encoder.c | 4 ++--
> > drivers/gpu/drm/drm_of.c | 8 ++++++--
> > drivers/gpu/drm/drm_panel.c | 6 ++++--
> > drivers/gpu/drm/exynos/exynos_dp.c | 9 ++++++---
> > drivers/gpu/drm/exynos/exynos_drm_dpi.c | 9 ++++++---
> > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 6 ++++--
> > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 8 +++++---
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 4 ++--
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 10 +++++++---
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 9 ++++++---
> > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
> > drivers/gpu/drm/sti/sti_dvo.c | 7 +++++--
> > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 ++--
> > drivers/gpu/drm/tegra/dsi.c | 6 ++++--
> > drivers/gpu/drm/tegra/output.c | 18 +++++++++++-------
> > include/drm/drm_panel.h | 2 +-
> > 18 files changed, 74 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
> > index c255fc3e1be5..2c5991cf5397 100644
> > --- a/drivers/gpu/drm/bridge/cdns-dsi.c
> > +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> > @@ -1152,7 +1152,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
> > np = of_node_get(dev->dev.of_node);
> >
> > panel = of_drm_find_panel(np);
> > - if (panel) {
> > + if (!IS_ERR(panel)) {
> > bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DSI);
> > } else {
> > bridge = of_drm_find_bridge(dev->dev.of_node);
> > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> > index 75b0d3f6e4de..f56c92f7af7c 100644
> > --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> > @@ -68,9 +68,9 @@ static int lvds_encoder_probe(struct platform_device *pdev)
> >
> > panel = of_drm_find_panel(panel_node);
> > of_node_put(panel_node);
> > - if (!panel) {
> > + if (IS_ERR(panel)) {
> > dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
> > - return -EPROBE_DEFER;
> > + return PTR_ERR(panel);
> > }
> >
> > lvds_encoder->panel_bridge =
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 1fe122461298..f413fae6f6dc 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -239,9 +239,13 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> > return -ENODEV;
> >
> > if (panel) {
> > - *panel = of_drm_find_panel(remote);
> > - if (*panel)
> > + struct drm_panel *tmp_panel;
> > +
> > + tmp_panel = of_drm_find_panel(remote);
> > + if (!IS_ERR(tmp_panel)) {
> > + *panel = tmp_panel;
> > ret = 0;
> > + }
>
> I think the introduction of this temporary variable makes the code hard
> to read and the diff difficult to understand. Why not just stick with
> the original style and make this:
>
> *panel = of_drm_find_panel(remote);
> if (IS_ERR(*panel))
> *panel = NULL;
> else
> ret = 0;
>
> ?
Sure.
>
> > }
> >
> > /* No panel found yet, check for a bridge next. */
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index 308d442a531b..2d9e2684c6c8 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -135,7 +135,9 @@ EXPORT_SYMBOL(drm_panel_detach);
> > * tree node. If a matching panel is found, return a pointer to it.
> > *
> > * Return: A pointer to the panel registered for the specified device tree
> > - * node or NULL if no panel matching the device tree node can be found.
> > + * node or an ERR_PTR() if no panel matching the device tree node can be found.
> > + * The only error that can be reported is -EPROBE_DEFER, meaning that the panel
> > + * device has not been probed yet, and the caller should re-retry later.
>
> I think you can drop the extra re- from re-retry.
Yep, will fix that.
>
> > */
> > struct drm_panel *of_drm_find_panel(const struct device_node *np)
> > {
> > @@ -151,7 +153,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
> > }
> >
> > mutex_unlock(&panel_lock);
> > - return NULL;
> > + return ERR_PTR(-EPROBE_DEFER);
> > }
> > EXPORT_SYMBOL(of_drm_find_panel);
> > #endif
> > diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> > index 86330f396784..962cad0276e5 100644
> > --- a/drivers/gpu/drm/exynos/exynos_dp.c
> > +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> > @@ -231,10 +231,13 @@ static int exynos_dp_probe(struct platform_device *pdev)
> > /* This is for the backward compatibility. */
> > np = of_parse_phandle(dev->of_node, "panel", 0);
> > if (np) {
> > - dp->plat_data.panel = of_drm_find_panel(np);
> > + struct drm_panel *panel = of_drm_find_panel(np);
> > +
> > of_node_put(np);
> > - if (!dp->plat_data.panel)
> > - return -EPROBE_DEFER;
> > + if (IS_ERR(panel))
> > + return PTR_ERR(panel);
>
> I don't see the point in the extra variable here. dp is allocated using
> devm_kzalloc(), so it will go away on the error return. You shouldn't
> need to worry about keeping the dp->plat_data.panel untouched in case of
> error.
Also true. I'll assign dp->plat_data.panel directly.
>
> > +
> > + dp->plat_data.panel = panel;
> > goto out;
> > }
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> > index 66945e0dc57f..e9c7d1c1cf8f 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> > @@ -239,9 +239,12 @@ struct drm_encoder *exynos_dpi_probe(struct device *dev)
> > }
> >
> > if (ctx->panel_node) {
> > - ctx->panel = of_drm_find_panel(ctx->panel_node);
> > - if (!ctx->panel)
> > - return ERR_PTR(-EPROBE_DEFER);
> > + struct drm_panel *panel = of_drm_find_panel(ctx->panel_node);
> > +
> > + if (IS_ERR(panel))
> > + return ERR_CAST(panel);
> > +
> > + ctx->panel = panel;
>
> Same here.
Yep.
>
> > }
> >
> > return &ctx->encoder;
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index 7904ffa9abfb..96206deb86a0 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -1520,6 +1520,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
> > {
> > struct exynos_dsi *dsi = host_to_dsi(host);
> > struct drm_device *drm = dsi->connector.dev;
> > + struct drm_panel *panel;
> >
> > /*
> > * This is a temporary solution and should be made by more generic way.
> > @@ -1538,8 +1539,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
> > dsi->lanes = device->lanes;
> > dsi->format = device->format;
> > dsi->mode_flags = device->mode_flags;
> > - dsi->panel = of_drm_find_panel(device->dev.of_node);
> > - if (dsi->panel) {
> > + panel = of_drm_find_panel(device->dev.of_node);
> > + if (!IS_ERR(panel)) {
> > + dsi->panel = panel;
> > drm_panel_attach(dsi->panel, &dsi->connector);
> > dsi->connector.status = connector_status_connected;
> > }
>
> It seems to be a potential problem here if dsi->panel is an ERR_PTR()-
> encoded value, but I still think this becomes clearer if you do:
>
> dsi->panel = of_drm_find_panel(...);
> if (!IS_ERR(dsi->panel)) {
> ...
> } else {
> dsi->panel = NULL;
> }
>
> Or maybe even:
>
> dsi->panel = of_drm_find_panel(...);
> if (IS_ERR(dsi->panel))
> dsi->panel = NULL;
>
> if (dsi->panel) {
> }
>
> That's more explicitly saying that the panel support is optional.
I'll go for the 2nd option.
>
> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> > index c54806d08dd7..7753b3093472 100644
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
> > @@ -146,10 +146,12 @@ int fsl_dcu_create_outputs(struct fsl_dcu_drm_device *fsl_dev)
> > /* This is for backward compatibility */
> > panel_node = of_parse_phandle(fsl_dev->np, "fsl,panel", 0);
> > if (panel_node) {
> > - fsl_dev->connector.panel = of_drm_find_panel(panel_node);
> > + panel = of_drm_find_panel(panel_node);
> > of_node_put(panel_node);
> > - if (!fsl_dev->connector.panel)
> > - return -EPROBE_DEFER;
> > + if (IS_ERR(panel))
> > + return PTR_ERR(panel);
> > +
> > + fsl_dev->connector.panel = panel;
>
> Same here, fsl_dev goes away after the error return.
Okay, will assign fsl_dev->connector.panel directly.
>
> > return fsl_dcu_attach_panel(fsl_dev, fsl_dev->connector.panel);
> > }
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> > index 4a645926edb7..2bfb39082f54 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> > @@ -341,7 +341,7 @@ static void mdp4_lcdc_encoder_disable(struct drm_encoder *encoder)
> > mdp4_write(mdp4_kms, REG_MDP4_LCDC_ENABLE, 0);
> >
> > panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node);
> > - if (panel) {
> > + if (!IS_ERR(panel)) {
> > drm_panel_disable(panel);
> > drm_panel_unprepare(panel);
> > }
> > @@ -410,7 +410,7 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder)
> > dev_err(dev->dev, "failed to enable lcdc_clk: %d\n", ret);
> >
> > panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node);
> > - if (panel) {
> > + if (!IS_ERR(panel)) {
> > drm_panel_prepare(panel);
> > drm_panel_enable(panel);
> > }
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> > index e3b1c86b7aae..c7dd72b428f8 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> > @@ -34,9 +34,13 @@ static enum drm_connector_status mdp4_lvds_connector_detect(
> > struct mdp4_lvds_connector *mdp4_lvds_connector =
> > to_mdp4_lvds_connector(connector);
> >
> > - if (!mdp4_lvds_connector->panel)
> > - mdp4_lvds_connector->panel =
> > - of_drm_find_panel(mdp4_lvds_connector->panel_node);
> > + if (!mdp4_lvds_connector->panel) {
> > + struct drm_panel *panel;
> > +
> > + panel = of_drm_find_panel(mdp4_lvds_connector->panel_node);
> > + if (!IS_ERR(panel))
> > + mdp4_lvds_connector->panel = panel;
> > + }
>
> Huh... this is hacky to begin with. Seems like this driver doesn't care
> about waiting for the panel, it'll just retry probing the panel
> everytime ->detect() is called on the connector until the panel has
> shown up. That's not really how this is supposed to work, but it's been
> there before your patch, so should be addressed separately.
Yes, I'm not trying to address this kind of issues here, and I fear
it's not the only driver to do that.
>
> >
> > return mdp4_lvds_connector->panel ?
> > connector_status_connected :
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 7a03a9489708..fffc80b73966 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -1881,7 +1881,7 @@ int msm_dsi_host_register(struct mipi_dsi_host *host, bool check_defer)
> > * output
> > */
> > if (check_defer && msm_host->device_node) {
> > - if (!of_drm_find_panel(msm_host->device_node))
> > + if (IS_ERR(of_drm_find_panel(msm_host->device_node)))
> > if (!of_drm_find_bridge(msm_host->device_node))
> > return -EPROBE_DEFER;
> > }
>
> Again, pretty weird stuff going on here, prior to the patch. But I think
> this needs to be changed to take the -ENODEV into account in the next
> patch. As it is, this will continue to defer probe even if the panel
> node is disabled.
Not sure this is a problem. I mean, the code was working before, and we
had no way to differentiate the -ENODEV vs -EPROBE_DEFER, which in turn
means that, any driver that assumed that the device would appear at some
point were just as broken as they are after this patch when the node
they're pointing to has its status set to "disabled".
I'm not trying to patch all drivers to take the return code into
account, just those that might take advantage of it.
>
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 3d2d3bbd1342..d77b8f8bcf94 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -430,9 +430,12 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > if (!lvds->next_bridge)
> > ret = -EPROBE_DEFER;
> > } else {
> > - lvds->panel = of_drm_find_panel(remote);
> > - if (!lvds->panel)
> > - ret = -EPROBE_DEFER;
> > + struct drm_panel *panel = of_drm_find_panel(remote);
> > +
> > + if (IS_ERR(panel))
> > + ret = PTR_ERR(panel);
> > + else
> > + lvds->panel = panel;
> > }
>
> Similar to others above, I think this can easily be done without the
> extra temporary variable.
Sure (same goes for all occurrences).
[...]
>
> > output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 14ac240a1f64..3bb91519462e 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -199,7 +199,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np);
> > #else
> > static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
> > {
> > - return NULL;
> > + return ERR_PTR(-ENOTSUPP);
> > }
> > #endif
>
> Maybe make this return ERR_PTR(-ENODEV) for consistency with the real
> function? If we absolutely do need to differentiate, then you should
> probably update the kerneldoc for of_drm_find_panel() to mention the
> -ENOTSUPP case.
Will use -ENODEV here.
Thanks for the review.
Boris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
