[PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[Lucas Stach]

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