On 4/6/22 21:32, Lucas Stach wrote:
Hi Marek,
Hi,
Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
The current clock handling in the LCDIF driver is a convoluted mess.
Here we agree...
Implement runtime PM ops which turn the clock ON and OFF and let the pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable and .atomic_disable callbacks turn the clock ON and OFF at the right time.
This requires slight reordering in mxsfb_crtc_atomic_enable() and mxsfb_crtc_atomic_disable(), since all the register accesses must happen only with clock enabled and clock frequency configuration must happen with clock disabled.
... on that one I don't. Please don't move the pixel clock into the RPM calls. We have a very well defined point between atomic enable/disable where the pixel clock is actually needed. All the other configuration accesses don't need the pixel clock to be active.
On the other hand, "all the other" configuration happens during probe, at which point all the clock are enabled anyway. And then during runtime, the pixel clock of this IP are either enabled or this IP is completely shut down.
So, where exactly does this patch make the clock handling any worse than it currently is ?
[...]
@@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm,
ret = platform_get_irq(pdev, 0); if (ret < 0)
goto err_vblank;
return ret;
- pm_runtime_get_sync(drm->dev); ret = mxsfb_irq_install(drm, mxsfb->irq);
- pm_runtime_put_sync(drm->dev);
Here you do a bunch of stuff resulting in register access without enabling the clocks. I don't really see how this works, maybe because the clocks are still on when you run the probe?
Right
Better enable the register access clocks here and then tell RPM about the fact that the device is running by calling pm_runtime_set_active before pm_runtime_enable. This way theoretically allows to build a kernel without CONFIG_PM and things still work, even if the RPM calls are stubs.
I would much rather move this driver to RPM and have RPM handle the clock altogether in one place.
[...]
@@ -411,9 +405,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc, } spin_unlock_irq(&drm->event_lock);
- drm_crtc_vblank_off(crtc);
- mxsfb_disable_axi_clk(mxsfb); pm_runtime_put_sync(drm->dev);
Not the fault of your patch, but why is this a _sync call?
See 4201f4e848450 ("drm/mxsfb: Add pm_runtime calls to pipe_enable/disable"), likely the intention was for this call to complete before existing the atomic_disable.