Am Mittwoch, dem 06.04.2022 um 23:45 +0200 schrieb Marek Vasut:
> 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 ?
>
There is an even stronger argument: runtime PM does not guarantee that
the runtime_suspend is actually called after you put your last
reference. A simple "echo on > /sys/your-device/power/control" will
prevent the device from ever entering runtime suspend. So if you have a
clock like the pixel clock that _needs_ to be disabled for
configuration purposes you _must_ not handle this clock via RPM, but
via explicit clock handling in the driver.
> [...]
>
> > > @@ -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.
>
See above. Same argument applies. The driver should work without RPM
support.
> [...]
>
> > > @@ -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.
Hm, still don't see why this would be necessary. But as this was just a
passing comment, we should revisit this later, not in the context of
this patch.
Regards,
Lucas
