Re: [RFT PATCH 03/15] drm/ingenic: Call drm_atomic_helper_shutdown() at shutdown time
From: Paul Cercueil
Date: Mon Sep 04 2023 - 05:15:31 EST
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)