| Message ID | 20231101-tidss-probe-v1-7-45149e0f9415@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Probe related fixes and cleanups | expand |
Hi Tomi, Thank you for the patch. On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote: > The probe function calls dispc_softreset() before runtime PM is enabled > and without enabling any of the DSS clocks. This happens to work by > luck, and we need to make sure the DSS HW is active and the fclk is > enabled. > > To fix the above, add a new function, dispc_init_hw(), which does: > > - pm_runtime_set_active() > - clk_prepare_enable(fclk) > - dispc_softreset(). > > This ensures that the reset can be successfully accomplished. > > Note that we use pm_runtime_set_active(), not the normal > pm_runtime_get(). The reason for this is that at this point we haven't > enabled the runtime PM yet and also we don't want the normal resume > callback to be called: the dispc resume callback does some initial HW > setup, and it expects that the HW was off (no video ports are > streaming). If the bootloader has enabled the DSS and has set up a > boot time splash-screen, the DSS would be enabled and streaming which > might lead to issues with the normal resume callback. I think the right way to do this would be, in probe(), to - power on the device - enable runtime PM, masking the device as active - at end of probe, calling pm_runtime_put_autosuspend() Please also see my review of patch 02/10. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/gpu/drm/tidss/tidss_dispc.c | 45 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c > index f204a0701e9f..13db062892e3 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -2724,6 +2724,49 @@ static int dispc_softreset(struct dispc_device *dispc) > return 0; > } > > +static int dispc_init_hw(struct dispc_device *dispc) > +{ > + struct device *dev = dispc->dev; > + int ret; > + > + ret = pm_runtime_set_active(dev); > + if (ret) { > + dev_err(dev, "Failed to set DSS PM to active\n"); > + return ret; > + } > + > + ret = clk_prepare_enable(dispc->fclk); > + if (ret) { > + dev_err(dev, "Failed to enable DSS fclk\n"); > + goto err_runtime_suspend; > + } > + > + ret = dispc_softreset(dispc); > + if (ret) > + goto err_clk_disable; > + > + clk_disable_unprepare(dispc->fclk); > + ret = pm_runtime_set_suspended(dev); > + if (ret) { > + dev_err(dev, "Failed to set DSS PM to suspended\n"); > + return ret; > + } > + > + return 0; > + > +err_clk_disable: > + clk_disable_unprepare(dispc->fclk); > + > +err_runtime_suspend: > + ret = pm_runtime_set_suspended(dev); > + if (ret) { > + dev_err(dev, "Failed to set DSS PM to suspended\n"); > + return ret; > + } > + > + return ret; > +} > + > int dispc_init(struct tidss_device *tidss) > { > struct device *dev = tidss->dev; > @@ -2835,7 +2878,7 @@ int dispc_init(struct tidss_device *tidss) > > tidss->dispc = dispc; > > - r = dispc_softreset(dispc); > + r = dispc_init_hw(dispc); > if (r) > return r; >
On 01/11/2023 16:30, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote: >> The probe function calls dispc_softreset() before runtime PM is enabled >> and without enabling any of the DSS clocks. This happens to work by >> luck, and we need to make sure the DSS HW is active and the fclk is >> enabled. >> >> To fix the above, add a new function, dispc_init_hw(), which does: >> >> - pm_runtime_set_active() >> - clk_prepare_enable(fclk) >> - dispc_softreset(). >> >> This ensures that the reset can be successfully accomplished. >> >> Note that we use pm_runtime_set_active(), not the normal >> pm_runtime_get(). The reason for this is that at this point we haven't >> enabled the runtime PM yet and also we don't want the normal resume >> callback to be called: the dispc resume callback does some initial HW >> setup, and it expects that the HW was off (no video ports are >> streaming). If the bootloader has enabled the DSS and has set up a >> boot time splash-screen, the DSS would be enabled and streaming which >> might lead to issues with the normal resume callback. > > I think the right way to do this would be, in probe(), to > > - power on the device > - enable runtime PM, masking the device as active > - at end of probe, calling pm_runtime_put_autosuspend() Can you explain what that would accomplish, or why the code in this patch is wrong? If I understand it right, you're suggesting a more "normal" power up at the probe time, and then leaving the DSS enabled, but with autosuspend. That would require powering up, doing a reset, and calling dispc_runtime_resume. Which can be done, but I'm not sure why it's better, as we're not interested in "normal" power up at probe time. But I can see that my approach looks perhaps a bit odd just by looking at these patches. This work was related to keeping the bootloader's splash screen on the screen for a longer time, i.e. delaying reset. For that, I wanted an early function (dispc_init_hw) which would, instead of always resetting the DSS as it does in this version, peek at the DSS hardware, and see if the DSS is already streaming. If no, do a reset and proceed normally. If yes, skip the reset, leave the clocks enabled, and keep DSS PM active. Later, when we'd be doing the first modeset, the driver would do the initial reset. So, that's why I wanted an independent function for the HW probing/init, which is called before runtime PM is enabled, and I did not want normal runtime resume to be called as dispc_runtime_resume() would break the display. I think a better solution would be to set up the fb of tidss's fbdev to use the reserved memory, used for the boot splash screen. But I didn't figure out a way to do this. But even there we'd like to delay the reset until the first modeset (when the fbdev display is getting enabled). Tomi
On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote: > The probe function calls dispc_softreset() before runtime PM is enabled > and without enabling any of the DSS clocks. This happens to work by > luck, and we need to make sure the DSS HW is active and the fclk is > enabled. Not sure on the exact implication here, however from the description this seems a candidate for stable and would need a Fixes tag. Francesco
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index f204a0701e9f..13db062892e3 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -2724,6 +2724,49 @@ static int dispc_softreset(struct dispc_device *dispc) return 0; } +static int dispc_init_hw(struct dispc_device *dispc) +{ + struct device *dev = dispc->dev; + int ret; + + ret = pm_runtime_set_active(dev); + if (ret) { + dev_err(dev, "Failed to set DSS PM to active\n"); + return ret; + } + + ret = clk_prepare_enable(dispc->fclk); + if (ret) { + dev_err(dev, "Failed to enable DSS fclk\n"); + goto err_runtime_suspend; + } + + ret = dispc_softreset(dispc); + if (ret) + goto err_clk_disable; + + clk_disable_unprepare(dispc->fclk); + ret = pm_runtime_set_suspended(dev); + if (ret) { + dev_err(dev, "Failed to set DSS PM to suspended\n"); + return ret; + } + + return 0; + +err_clk_disable: + clk_disable_unprepare(dispc->fclk); + +err_runtime_suspend: + ret = pm_runtime_set_suspended(dev); + if (ret) { + dev_err(dev, "Failed to set DSS PM to suspended\n"); + return ret; + } + + return ret; +} + int dispc_init(struct tidss_device *tidss) { struct device *dev = tidss->dev; @@ -2835,7 +2878,7 @@ int dispc_init(struct tidss_device *tidss) tidss->dispc = dispc; - r = dispc_softreset(dispc); + r = dispc_init_hw(dispc); if (r) return r;
The probe function calls dispc_softreset() before runtime PM is enabled and without enabling any of the DSS clocks. This happens to work by luck, and we need to make sure the DSS HW is active and the fclk is enabled. To fix the above, add a new function, dispc_init_hw(), which does: - pm_runtime_set_active() - clk_prepare_enable(fclk) - dispc_softreset(). This ensures that the reset can be successfully accomplished. Note that we use pm_runtime_set_active(), not the normal pm_runtime_get(). The reason for this is that at this point we haven't enabled the runtime PM yet and also we don't want the normal resume callback to be called: the dispc resume callback does some initial HW setup, and it expects that the HW was off (no video ports are streaming). If the bootloader has enabled the DSS and has set up a boot time splash-screen, the DSS would be enabled and streaming which might lead to issues with the normal resume callback. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/gpu/drm/tidss/tidss_dispc.c | 45 ++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)