Hi, On Sun, Feb 11, 2018 at 03:07:44PM +0200, Laurent Pinchart wrote: > There's no reason to delay initialization of most of the driver (such as > mapping memory I/O, getting clocks or enabling runtime PM) to the > component master bind handler. > > This additionally fixes a real PM issue caused enabling runtime PM in > the bind handler. > > The bind handler performs the following sequence of PM operations: > > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); > > ... (access the hardware to read the device revision) ... > > pm_runtime_put_sync(dev); > > If a failure occurs at this point, the error path calls > pm_runtime_disable() to balance the pm_runtime_enable() call. > > To understand the problem, it should be noted that the bind handler is > called when one of the component registers itself, which happens in the > component's probe handler. Furthermore, as the components are children > of the DSS, the device core calls pm_runtime_get_sync() on the DSS > platform device before calling the component's probe handler. This > increases the DSS power usage count but doesn't runtime resume the > device, as runtime PM is disabled at that point. > > The bind handler is thus called with runtime PM disabled, with the > device runtime suspended, but with the power usage count larger than 0. > The pm_runtime_get_sync() call will thus further increase the power > usage count and runtime resume the device. The pm_runtime_put_sync() > handler will decrease the power usage count to a non-zero value and will > thus not suspend the device. Finally, the pm_runtime_disable() call will > disable runtime PM, preventing the pm_runtime_put() call in the device > core from runtime suspending the device. The DSS device is thus left > powered on. > > To fix this, move the initialization code from the bind handler to the > probe handler. > > Signed-off-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > ---
Reviewed-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
-- Sebastian
> drivers/gpu/drm/omapdrm/dss/dss.c | 193
> ++++++++++++++++++++------------------
> 1 file changed, 104 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c
> b/drivers/gpu/drm/omapdrm/dss/dss.c
> index f1c7ef3a2ec3..d086189263ef 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dss.c
> @@ -1300,88 +1300,18 @@ static const struct soc_device_attribute
> dss_soc_devices[] = {
>
> static int dss_bind(struct device *dev)
> {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct resource *dss_mem;
> - u32 rev;
> int r;
>
> - dss_mem = platform_get_resource(dss.pdev, IORESOURCE_MEM, 0);
> - dss.base = devm_ioremap_resource(&pdev->dev, dss_mem);
> - if (IS_ERR(dss.base))
> - return PTR_ERR(dss.base);
> -
> - r = dss_get_clocks();
> + r = component_bind_all(dev, NULL);
> if (r)
> return r;
>
> - r = dss_setup_default_clock();
> - if (r)
> - goto err_setup_clocks;
> -
> - r = dss_video_pll_probe(pdev);
> - if (r)
> - goto err_pll_init;
> -
> - r = dss_init_ports(pdev);
> - if (r)
> - goto err_init_ports;
> -
> - pm_runtime_enable(&pdev->dev);
> -
> - r = dss_runtime_get();
> - if (r)
> - goto err_runtime_get;
> -
> - dss.dss_clk_rate = clk_get_rate(dss.dss_clk);
> -
> - /* Select DPLL */
> - REG_FLD_MOD(DSS_CONTROL, 0, 0, 0);
> -
> - dss_select_dispc_clk_source(DSS_CLK_SRC_FCK);
> -
> -#ifdef CONFIG_OMAP2_DSS_VENC
> - REG_FLD_MOD(DSS_CONTROL, 1, 4, 4); /* venc dac demen */
> - REG_FLD_MOD(DSS_CONTROL, 1, 3, 3); /* venc clock 4x enable */
> - REG_FLD_MOD(DSS_CONTROL, 0, 2, 2); /* venc clock mode = normal */
> -#endif
> - dss.dsi_clk_source[0] = DSS_CLK_SRC_FCK;
> - dss.dsi_clk_source[1] = DSS_CLK_SRC_FCK;
> - dss.dispc_clk_source = DSS_CLK_SRC_FCK;
> - dss.lcd_clk_source[0] = DSS_CLK_SRC_FCK;
> - dss.lcd_clk_source[1] = DSS_CLK_SRC_FCK;
> -
> - rev = dss_read_reg(DSS_REVISION);
> - pr_info("OMAP DSS rev %d.%d\n", FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
> -
> - dss_runtime_put();
> -
> - r = component_bind_all(&pdev->dev, NULL);
> - if (r)
> - goto err_component;
> -
> - dss_debugfs_create_file("dss", dss_dump_regs);
> -
> pm_set_vt_switch(0);
>
> omapdss_gather_components(dev);
> omapdss_set_is_initialized(true);
>
> return 0;
> -
> -err_component:
> -err_runtime_get:
> - pm_runtime_disable(&pdev->dev);
> - dss_uninit_ports(pdev);
> -err_init_ports:
> - if (dss.video1_pll)
> - dss_video_pll_uninit(dss.video1_pll);
> -
> - if (dss.video2_pll)
> - dss_video_pll_uninit(dss.video2_pll);
> -err_pll_init:
> -err_setup_clocks:
> - dss_put_clocks();
> - return r;
> }
>
> static void dss_unbind(struct device *dev)
> @@ -1391,18 +1321,6 @@ static void dss_unbind(struct device *dev)
> omapdss_set_is_initialized(false);
>
> component_unbind_all(&pdev->dev, NULL);
> -
> - if (dss.video1_pll)
> - dss_video_pll_uninit(dss.video1_pll);
> -
> - if (dss.video2_pll)
> - dss_video_pll_uninit(dss.video2_pll);
> -
> - dss_uninit_ports(pdev);
> -
> - pm_runtime_disable(&pdev->dev);
> -
> - dss_put_clocks();
> }
>
> static const struct component_master_ops dss_component_ops = {
> @@ -1434,10 +1352,46 @@ static int dss_add_child_component(struct device
> *dev, void *data)
> return 0;
> }
>
> +static int dss_probe_hardware(void)
> +{
> + u32 rev;
> + int r;
> +
> + r = dss_runtime_get();
> + if (r)
> + return r;
> +
> + dss.dss_clk_rate = clk_get_rate(dss.dss_clk);
> +
> + /* Select DPLL */
> + REG_FLD_MOD(DSS_CONTROL, 0, 0, 0);
> +
> + dss_select_dispc_clk_source(DSS_CLK_SRC_FCK);
> +
> +#ifdef CONFIG_OMAP2_DSS_VENC
> + REG_FLD_MOD(DSS_CONTROL, 1, 4, 4); /* venc dac demen */
> + REG_FLD_MOD(DSS_CONTROL, 1, 3, 3); /* venc clock 4x enable */
> + REG_FLD_MOD(DSS_CONTROL, 0, 2, 2); /* venc clock mode = normal */
> +#endif
> + dss.dsi_clk_source[0] = DSS_CLK_SRC_FCK;
> + dss.dsi_clk_source[1] = DSS_CLK_SRC_FCK;
> + dss.dispc_clk_source = DSS_CLK_SRC_FCK;
> + dss.lcd_clk_source[0] = DSS_CLK_SRC_FCK;
> + dss.lcd_clk_source[1] = DSS_CLK_SRC_FCK;
> +
> + rev = dss_read_reg(DSS_REVISION);
> + pr_info("OMAP DSS rev %d.%d\n", FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
> +
> + dss_runtime_put();
> +
> + return 0;
> +}
> +
> static int dss_probe(struct platform_device *pdev)
> {
> const struct soc_device_attribute *soc;
> struct component_match *match = NULL;
> + struct resource *dss_mem;
> int r;
>
> dss.pdev = pdev;
> @@ -1458,20 +1412,69 @@ static int dss_probe(struct platform_device *pdev)
> else
> dss.feat = of_match_device(dss_of_match, &pdev->dev)->data;
>
> - r = dss_initialize_debugfs();
> + /* Map I/O registers, get and setup clocks. */
> + dss_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + dss.base = devm_ioremap_resource(&pdev->dev, dss_mem);
> + if (IS_ERR(dss.base))
> + return PTR_ERR(dss.base);
> +
> + r = dss_get_clocks();
> if (r)
> return r;
>
> - /* add all the child devices as components */
> + r = dss_setup_default_clock();
> + if (r)
> + goto err_put_clocks;
> +
> + /* Setup the video PLLs and the DPI and SDI ports. */
> + r = dss_video_pll_probe(pdev);
> + if (r)
> + goto err_put_clocks;
> +
> + r = dss_init_ports(pdev);
> + if (r)
> + goto err_uninit_plls;
> +
> + /* Enable runtime PM and probe the hardware. */
> + pm_runtime_enable(&pdev->dev);
> +
> + r = dss_probe_hardware();
> + if (r)
> + goto err_pm_runtime_disable;
> +
> + /* Initialize debugfs. */
> + r = dss_initialize_debugfs();
> + if (r)
> + goto err_pm_runtime_disable;
> +
> + dss_debugfs_create_file("dss", dss_dump_regs);
> +
> + /* Add all the child devices as components. */
> device_for_each_child(&pdev->dev, &match, dss_add_child_component);
>
> r = component_master_add_with_match(&pdev->dev, &dss_component_ops,
> match);
> - if (r) {
> - dss_uninitialize_debugfs();
> - return r;
> - }
> + if (r)
> + goto err_uninit_debugfs;
>
> return 0;
> +
> +err_uninit_debugfs:
> + dss_uninitialize_debugfs();
> +
> +err_pm_runtime_disable:
> + pm_runtime_disable(&pdev->dev);
> + dss_uninit_ports(pdev);
> +
> +err_uninit_plls:
> + if (dss.video1_pll)
> + dss_video_pll_uninit(dss.video1_pll);
> + if (dss.video2_pll)
> + dss_video_pll_uninit(dss.video2_pll);
> +
> +err_put_clocks:
> + dss_put_clocks();
> +
> + return r;
> }
>
> static int dss_remove(struct platform_device *pdev)
> @@ -1480,6 +1483,18 @@ static int dss_remove(struct platform_device *pdev)
>
> dss_uninitialize_debugfs();
>
> + pm_runtime_disable(&pdev->dev);
> +
> + dss_uninit_ports(pdev);
> +
> + if (dss.video1_pll)
> + dss_video_pll_uninit(dss.video1_pll);
> +
> + if (dss.video2_pll)
> + dss_video_pll_uninit(dss.video2_pll);
> +
> + dss_put_clocks();
> +
> return 0;
> }
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
