Re: [PATCH v5 0/8] drm: rcar-du: Add Color Management Module (CMM)

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Eugeniu,

On Fri, Jun 12, 2020 at 05:36:07PM +0200, Eugeniu Rosca wrote:
> On Fri, Jun 12, 2020 at 06:10:05PM +0300, Laurent Pinchart wrote:
> > On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
> > > On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
> > > > On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> > > > > Note that the CMM driver is controlled by the DU driver. As the DU
> > > > > driver will reenable the display during resume, it will call
> > > > > rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> > > > > should thus be no need for manual suspend/resume handling in the CMM as
> > > > > far as I can tell, but we need to ensure that the CMM is suspended
> > > > > before and resumed after the DU. I believe this could be implemented
> > > > > using device links.
> > > >
> > > > Based on below quote [*] from Jacopo's commit [**], isn't the device
> > > > link relationship already in place?
> > > 
> > > Yes, it's in place already.
> > > 
> > > I added pm_ops to cmm just to be able to printout when suspend/resume
> > > happens and the sequence is what comment [*] reports
> > > 
> > > [  222.909002] rcar_du_pm_suspend:505
> > > [  223.145497] rcar_cmm_pm_suspend:193
> > > 
> > > [  223.208053] rcar_cmm_pm_resume:200
> > > [  223.460094] rcar_du_pm_resume:513
> > > 
> > > However, Laurent mentioned that in his comment here that he expects
> > > the opposite sequence to happen (CMM to suspend before and resume after
> > > DU).
> > > 
> > > I still think what is implemented is correct:
> > > - CMM is suspended after DU: when CMM is suspended, DU is not feeding
> > >   it with data
> > > - CMM is resumed before: once DU restart operations CMM is ready to
> > >   receive data.
> > > 
> > > Laurent, what do you think ?
> > 
> > I think I shouldn't have written the previous e-mail in the middle of
> > the night :-) Suspending CMM after DU is obviously correct.
> 
> Thanks to Renesas team (kudos to Gotthard and Michael), we've
> figured out that below sequence of clock handling (happening during
> concurrent suspend and HDMI display unplug) leads to SoC lockup:
> 
> cmm1 OFF 	(caused by HDMI unplug)
> x21-clock OFF 	(caused by HDMI unplug)
> du1 OFF 	(caused by HDMI unplug)
> cmm1 ON (caused by suspend to ram, as preparation for CMM register save)
> # Freeze happens
> 
> That seems to be explained by Chapter 35A.4.3 "Restriction of enabling
> clock signal of the CMM" of HW User's manual (Rev.2.00 Jul 2019):
> 
>  -----8<-----
>  When the clock signal of the CMM is enabled (RMSTPCR7.CMMn or
>  SMSTPCR7.CMMn = 0), the clock signal of the DU should be also enabled
>  (RMSTPCR7.DUn or SMSTPCR7.DUn = 0).
>  -----8<-----
> 
> So, the lesson learned from the above is: do not enable the CMMi clock
> while the DUi clock is disabled. I expect this to also potentially
> give some input w.r.t. what to suspend/resume first, CMM or DU.

This may be an ugly one. The DU driver needs to disable the CMM when
suspending, and enabling the CMM when resuming. To do so, it calls
functions of the CMM driver, and those functions access CMM registers.
We can't do so while the CMM is suspended, so the DU has to suspend
before the CMM, and resume after the CMM.

On the other hand, as you state here, we need to make sure the CMM clock
is disabled first. The CMM thus needs to suspend before the DU, and
resume after the DU.

Those are conflicting requirements.

One option could be to use the .suspend_late() and .resume_early() PM
operations for the DU, to turn the DU clock off late and turn it back on
early. Integrating it with the DRM suspend/resume helpers will likely be
complicated though. I wonder if we could find a more elegant solution.

I the above sequence, you list "cmm1 ON" as a preparation for CMM
register save. We don't need to save any register, the CMM driver has no
.suspend() handler. The PM core should really skip waking up the device
at that point (I recall complaining about the spurious wake ups at
suspend time a while ago, but that neevr got addressed). Geert, any
opinion on that ?

> > > > [*] Quote from commit [**]
> > > >    Enforce the probe and suspend/resume ordering of DU and CMM by
> > > >    creating a stateless device link between the two.
> > > >
> > > > [**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8de707aeb45241
> > > >     ("drm: rcar-du: kms: Initialize CMM instances")

-- 
Regards,

Laurent Pinchart