Hi Douglas, Le vendredi 01 septembre 2023 à 16:41 -0700, Douglas Anderson a écrit : > Based on grepping through the source code this driver appears to be > missing a call to drm_atomic_helper_shutdown() at system shutdown > time. Among other things, this means that if a panel is in use that > it > won't be cleanly powered off at system shutdown time. > > The fact that we should call drm_atomic_helper_shutdown() in the case > of OS shutdown/restart comes straight out of the kernel doc "driver > instance overview" in drm_drv.c. > > Since this driver uses the component model and shutdown happens at > the > base driver, we communicate whether we have to call > drm_atomic_helper_shutdown() by seeing if drvdata is non-NULL. > > Suggested-by: Maxime Ripard <mripard@xxxxxxxxxx> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> LGTM. Acked-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx> > --- > This commit is only compile-time tested. > > NOTE: this patch touches a lot more than other similar patches since > the bind() function is long and we want to make sure that we unset > the > drvdata if bind() fails. > > While making this patch, I noticed that the bind() function of this > driver is using "devm" and thus assumes it doesn't need to do much > explicit error handling. That's actually a bug. As per kernel docs > [1] > "the lifetime of the aggregate driver does not align with any of the > underlying struct device instances. Therefore devm cannot be used and > all resources acquired or allocated in this callback must be > explicitly released in the unbind callback". Fixing that is outside > the scope of this commit. > > [1] https://docs.kernel.org/driver-api/component.html > Noted, thanks. Cheers, -Paul > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 66 +++++++++++++++------ > -- > 1 file changed, 44 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index 8dbd4847d3a6..51995a0cd568 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -1130,7 +1130,7 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > > ret = drmm_mode_config_init(drm); > if (ret) > - return ret; > + goto err_drvdata; > > drm->mode_config.min_width = 0; > drm->mode_config.min_height = 0; > @@ -1142,7 +1142,8 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(base)) { > dev_err(dev, "Failed to get memory resource\n"); > - return PTR_ERR(base); > + ret = PTR_ERR(base); > + goto err_drvdata; > } > > regmap_config = ingenic_drm_regmap_config; > @@ -1151,33 +1152,40 @@ static int ingenic_drm_bind(struct device > *dev, bool has_components) > ®map_config); > if (IS_ERR(priv->map)) { > dev_err(dev, "Failed to create regmap\n"); > - return PTR_ERR(priv->map); > + ret = PTR_ERR(priv->map); > + goto err_drvdata; > } > > irq = platform_get_irq(pdev, 0); > - if (irq < 0) > - return irq; > + if (irq < 0) { > + ret = irq; > + goto err_drvdata; > + } > > if (soc_info->needs_dev_clk) { > priv->lcd_clk = devm_clk_get(dev, "lcd"); > if (IS_ERR(priv->lcd_clk)) { > dev_err(dev, "Failed to get lcd clock\n"); > - return PTR_ERR(priv->lcd_clk); > + ret = PTR_ERR(priv->lcd_clk); > + goto err_drvdata; > } > } > > priv->pix_clk = devm_clk_get(dev, "lcd_pclk"); > if (IS_ERR(priv->pix_clk)) { > dev_err(dev, "Failed to get pixel clock\n"); > - return PTR_ERR(priv->pix_clk); > + ret = PTR_ERR(priv->pix_clk); > + goto err_drvdata; > } > > priv->dma_hwdescs = dmam_alloc_coherent(dev, > sizeof(*priv- > >dma_hwdescs), > &priv- > >dma_hwdescs_phys, > GFP_KERNEL); > - if (!priv->dma_hwdescs) > - return -ENOMEM; > + if (!priv->dma_hwdescs) { > + ret = -ENOMEM; > + goto err_drvdata; > + } > > /* Configure DMA hwdesc for foreground0 plane */ > ingenic_drm_configure_hwdesc_plane(priv, 0); > @@ -1199,7 +1207,7 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > NULL, DRM_PLANE_TYPE_PRIMARY, > NULL); > if (ret) { > dev_err(dev, "Failed to register plane: %i\n", ret); > - return ret; > + goto err_drvdata; > } > > if (soc_info->map_noncoherent) > @@ -1211,7 +1219,7 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > NULL, > &ingenic_drm_crtc_funcs, NULL); > if (ret) { > dev_err(dev, "Failed to init CRTC: %i\n", ret); > - return ret; > + goto err_drvdata; > } > > drm_crtc_enable_color_mgmt(&priv->crtc, 0, false, > @@ -1230,7 +1238,7 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > if (ret) { > dev_err(dev, "Failed to register overlay > plane: %i\n", > ret); > - return ret; > + goto err_drvdata; > } > > if (soc_info->map_noncoherent) > @@ -1241,17 +1249,18 @@ static int ingenic_drm_bind(struct device > *dev, bool has_components) > if (ret) { > if (ret != -EPROBE_DEFER) > dev_err(dev, "Failed to bind > components: %i\n", ret); > - return ret; > + goto err_drvdata; > } > > ret = devm_add_action_or_reset(dev, > ingenic_drm_unbind_all, priv); > if (ret) > - return ret; > + goto err_drvdata; > > priv->ipu_plane = drm_plane_from_index(drm, > 2); > if (!priv->ipu_plane) { > dev_err(dev, "Failed to retrieve IPU > plane\n"); > - return -EINVAL; > + ret = -EINVAL; > + goto err_drvdata; > } > } > } > @@ -1263,7 +1272,7 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > break; /* we're done */ > if (ret != -EPROBE_DEFER) > dev_err(dev, "Failed to get bridge > handle\n"); > - return ret; > + goto err_drvdata; > } > > if (panel) > @@ -1275,7 +1284,7 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > if (IS_ERR(ib)) { > ret = PTR_ERR(ib); > dev_err(dev, "Failed to init encoder: %d\n", > ret); > - return ret; > + goto err_drvdata; > } > > encoder = &ib->encoder; > @@ -1290,13 +1299,14 @@ static int ingenic_drm_bind(struct device > *dev, bool has_components) > DRM_BRIDGE_ATTACH_NO_CONNECTO > R); > if (ret) { > dev_err(dev, "Unable to attach bridge\n"); > - return ret; > + goto err_drvdata; > } > > connector = drm_bridge_connector_init(drm, encoder); > if (IS_ERR(connector)) { > dev_err(dev, "Unable to init connector\n"); > - return PTR_ERR(connector); > + ret = PTR_ERR(connector); > + goto err_drvdata; > } > > drm_connector_attach_encoder(connector, encoder); > @@ -1313,13 +1323,13 @@ static int ingenic_drm_bind(struct device > *dev, bool has_components) > ret = devm_request_irq(dev, irq, ingenic_drm_irq_handler, 0, > drm->driver->name, drm); > if (ret) { > dev_err(dev, "Unable to install IRQ handler\n"); > - return ret; > + goto err_drvdata; > } > > ret = drm_vblank_init(drm, 1); > if (ret) { > dev_err(dev, "Failed calling drm_vblank_init()\n"); > - return ret; > + goto err_drvdata; > } > > drm_mode_config_reset(drm); > @@ -1327,7 +1337,7 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > ret = clk_prepare_enable(priv->pix_clk); > if (ret) { > dev_err(dev, "Unable to start pixel clock\n"); > - return ret; > + goto err_drvdata; > } > > if (priv->lcd_clk) { > @@ -1402,6 +1412,8 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > clk_disable_unprepare(priv->lcd_clk); > err_pixclk_disable: > clk_disable_unprepare(priv->pix_clk); > +err_drvdata: > + platform_set_drvdata(pdev, NULL); > return ret; > } > > @@ -1422,6 +1434,7 @@ static void ingenic_drm_unbind(struct device > *dev) > > drm_dev_unregister(&priv->drm); > drm_atomic_helper_shutdown(&priv->drm); > + dev_set_drvdata(dev, NULL); > } > > static const struct component_master_ops ingenic_master_ops = { > @@ -1461,6 +1474,14 @@ static int ingenic_drm_remove(struct > platform_device *pdev) > return 0; > } > > +static void ingenic_drm_shutdown(struct platform_device *pdev) > +{ > + struct ingenic_drm *priv = platform_get_drvdata(pdev); > + > + if (priv) > + drm_atomic_helper_shutdown(&priv->drm); > +} > + > static int ingenic_drm_suspend(struct device *dev) > { > struct ingenic_drm *priv = dev_get_drvdata(dev); > @@ -1612,6 +1633,7 @@ static struct platform_driver > ingenic_drm_driver = { > }, > .probe = ingenic_drm_probe, > .remove = ingenic_drm_remove, > + .shutdown = ingenic_drm_shutdown, > }; > > static int ingenic_drm_init(void)
