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;
mxsfb->irq = 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.
