Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 13 Feb 2018 15:46:08 +0000
From:   Liviu Dudau <Liviu.Dudau@arm.com>
To:     Lukas Wunner <lukas@wunner.de>
Cc:     Tejun Heo <tj@kernel.org>, Lai Jiangshan <jiangshanlai@gmail.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        Dave Airlie <airlied@redhat.com>,
        Ben Skeggs <bskeggs@redhat.com>,
        dri-devel@lists.freedesktop.org, Peter Wu <peter@lekensteyn.nl>,
        nouveau@lists.freedesktop.org, Lyude Paul <lyude@redhat.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Pierre Moreau <pierre.morrow@free.fr>,
        linux-kernel@vger.kernel.org,
        Ismo Toijala <ismo.toijala@gmail.com>,
        intel-gfx@lists.freedesktop.org,
        Archit Taneja <architt@codeaurora.org>
Subject: Re: [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers
Message-ID: <20180213154608.GI9111@e110455-lin.cambridge.arm.com>
References: <cover.1518338788.git.lukas@wunner.de>
 <20180213105506.GF9111@e110455-lin.cambridge.arm.com>
 <20180213115206.GA16075@wunner.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20180213115206.GA16075@wunner.de>
User-Agent: Mutt/1.9.3 (2018-01-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.
> 
> I don't know if malidp makes any specific mistakes and didn't mean to
> cast it in a negative light, sorry.

I did not take what you've said as a negative thing, only wanted to
understand how you came to your conclusions.

> 
> So a lot of DRM drivers acquire a runtime PM ref in the connector ->detect
> hook because they can't probe connectors while runtime suspended.
> E.g. for a PCI device, probing might require mmio access, which isn't
> possible outside of power state D0.  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.

> 
> You're not disabling polling upon runtime suspend.  Thus, if a runtime PM
> ref is acquired during polling (such as in a ->detect hook), the GPU will
> be runtime resumed every 10 secs.  You may want to verify that's not the
> case.  If you decide that you do want to stop polling during runtime
> suspend because it runtime resumes the GPU continuously, you'll need the
> helper introduced in this series.  So by cc'ing you I just wanted to keep
> you in the loop about an issue that may potentially affect your driver.

Again, we have no GPU linked to us and the polling during runtime
suspend should be handled by the driver for the paired encoder, not by
us. I understand the potential issue but I'm struggling to understand if
it really applies to the drivers/gpu/drm/arm drivers other than in an
abstract way.

Best regards,
Liviu

> 
> Let me know if there are any questions.
> 
> Thanks,
> 
> Lukas

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯