Re: [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Tue, Feb 13, 2018 at 03:46:08PM +0000, Liviu Dudau wrote:
> On Tue, Feb 13, 2018 at 12:52:06PM +0100, Lukas Wunner wrote:
> > On Tue, Feb 13, 2018 at 10:55:06AM +0000, Liviu Dudau wrote:
> > > On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> > > > DRM drivers poll connectors in 10 sec intervals.  The poll worker is
> > > > stopped on ->runtime_suspend with cancel_delayed_work_sync().  However
> > > > the poll worker invokes the DRM drivers' ->detect callbacks, which call
> > > > pm_runtime_get_sync().  If the poll worker starts after runtime suspend
> > > > has begun, pm_runtime_get_sync() will wait for runtime suspend to finish
> > > > with the intention of runtime resuming the device afterwards.  The result
> > > > is a circular wait between poll worker and autosuspend worker.
> > > 
> > > I think I understand the problem you are trying to solve, but I'm
> > > struggling to understand where malidp makes any specific mistakes. First
> > > of all, malidp is only a display engine, so there is no GPU attached to
> > > it, but that is only a small clarification. Second, malidp doesn't use
> > > directly any of the callbacks that you are referring to, it uses the
> > > drm_cma_xxxx() API plus the generic drm_xxxx() call. So if there are any
> > > issues there (as they might well be) I think they would apply to a lot
> > > more drivers and the fix will involve more than just malidp, i915 and
> > > msm.
[snip]
> > There are no ->detect hooks declared
> > in drivers/gpu/drm/arm/, so it's unclear to me whether you're able to probe
> > during runtime suspend.
> 
> That's because the drivers in drivers/gpu/drm/arm do not have
> connectors, they are only the CRTC part of the driver. Both hdlcd and
> mali-dp use the component framework to locate an encoder in device tree
> that will then provide the connectors.
> 
> > 
> > hdlcd_drv.c and malidp_drv.c both enable output polling.  Output polling
> > is only necessary if you don't get HPD interrupts.
> 
> That's right, hdlcd and mali-dp don't receive HPD interrupts because
> they don't have any. And because we don't know ahead of time which
> encoder/connector will be paired with the driver, we enable polling as a
> safe fallback.
> 

Looking e.g. at inno_hdmi.c (used by rk3036.dtsi), this calls
drm_helper_hpd_irq_event() on receiving an HPD interrupt, and that
function returns immediately if polling is not enabled.  So you *have*
to enable polling to receive HPD events.

You seem to keep the crtc runtime active as long as it's bound to an
encoder.  If you do not ever intend to runtime suspend the crtc while
an encoder is attached, you don't need to keep polling enabled during
runtime suspend (because there's nothing to poll), but it shouldn't
hurt either.

If you would runtime suspend while an encoder is attached, then you
would only runtime resume every 10 sec (upon polling) if the encoder
was a child of the crtc and would support runtime suspend as well.
That's because the PM core wakes the parent by default when a child
runtime resumes.  However in the DT's I've looked at, the encoder
is never a child of the crtc and at least inno_hdmi.c doesn't use
runtime suspend.

So I think you're all green, I can't spot any grave issues here.
Just be aware of the above-mentioned constraints.

Thanks,

Lukas