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;
?
> }
>
> /* 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.
> */
> 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.
> +
> + 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.
> }
>
> 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.
> 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.
> 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.
>
> 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.
> 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.
>
> done:
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index d53d5a09547f..01642aaf6127 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -595,7 +595,7 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
> dsi->format = device->format;
> dsi->mode_flags = device->mode_flags;
> dsi->panel = of_drm_find_panel(device->dev.of_node);
> - if (dsi->panel)
> + if (!IS_ERR(dsi->panel))
> return drm_panel_attach(dsi->panel, &dsi->connector);
>
> return -EINVAL;
> diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
> index a5979cd25cc7..3579c0e1ca1c 100644
> --- a/drivers/gpu/drm/sti/sti_dvo.c
> +++ b/drivers/gpu/drm/sti/sti_dvo.c
> @@ -386,9 +386,12 @@ sti_dvo_connector_detect(struct drm_connector *connector, bool force)
> DRM_DEBUG_DRIVER("\n");
>
> if (!dvo->panel) {
> - dvo->panel = of_drm_find_panel(dvo->panel_node);
> - if (dvo->panel)
> + struct drm_panel *panel = of_drm_find_panel(dvo->panel_node);
> +
> + if (!IS_ERR(panel)) {
> + dvo->panel = panel;
> drm_panel_attach(dvo->panel, connector);
> + }
> }
Same here.
>
> if (dvo->panel)
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index bfbf761f0c1d..ce388d7cebaa 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -812,8 +812,8 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>
> dsi->device = device;
> dsi->panel = of_drm_find_panel(device->dev.of_node);
> - if (!dsi->panel)
> - return -EINVAL;
> + if (IS_ERR(dsi->panel))
> + return PTR_ERR(dsi->panel);
>
> dev_info(host->dev, "Attached device %s\n", device->name);
>
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index 87c5d89bc9ba..0b1eee4b2fb1 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -1409,9 +1409,11 @@ static int tegra_dsi_host_attach(struct mipi_dsi_host *host,
> */
> if (!dsi->master) {
> struct tegra_output *output = &dsi->output;
> + struct drm_panel *panel;
>
> - output->panel = of_drm_find_panel(device->dev.of_node);
> - if (output->panel && output->connector.dev) {
> + panel = of_drm_find_panel(device->dev.of_node);
> + if (!IS_ERR(panel) && output->connector.dev) {
> + output->panel = panel;
And here.
> drm_panel_attach(output->panel, &output->connector);
> drm_helper_hpd_irq_event(output->connector.dev);
> }
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index 676fd394836f..21d8737f8b49 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -101,18 +101,22 @@ static irqreturn_t hpd_irq(int irq, void *data)
>
> int tegra_output_probe(struct tegra_output *output)
> {
> - struct device_node *ddc, *panel;
> + struct device_node *ddc, *panelnp;
> int err, size;
>
> if (!output->of_node)
> output->of_node = output->dev->of_node;
>
> - panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
> - if (panel) {
> - output->panel = of_drm_find_panel(panel);
> - of_node_put(panel);
> - if (!output->panel)
> - return -EPROBE_DEFER;
> + panelnp = of_parse_phandle(output->of_node, "nvidia,panel", 0);
> + if (panelnp) {
> + struct drm_panel *panel = of_drm_find_panel(panelnp);
> +
> + of_node_put(panelnp);
> +
> + if (IS_ERR(panel))
> + return PTR_ERR(panel);
> +
> + output->panel = panel;
> }
And here.
> 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.
Thierry
Attachment:
signature.asc
Description: PGP signature
