Re: [pull] radeon and amdgpu drm-next-4.9

From: Lukas Wunner <lukas@wunner.de>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: Alex Deucher <alexander.deucher@amd.com>,
	Maling list - DRI developers <dri-devel@lists.freedesktop.org>,
	amd-gfx list <amd-gfx@lists.freedesktop.org>
Subject: Re: [pull] radeon and amdgpu drm-next-4.9
Date: Sat, 17 Sep 2016 12:23:33 +0200	[thread overview]
Message-ID: <20160917102333.GA1422@wunner.de> (raw)
In-Reply-To: <CADnq5_NZ1+tgEoZKUgWxTYKCtNbvr4cCttpNMzbyGroHdePz=w@mail.gmail.com>

On Fri, Sep 16, 2016 at 06:07:36PM -0400, Alex Deucher wrote:
> On Fri, Sep 16, 2016 at 5:38 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Fri, Sep 16, 2016 at 04:42:43PM -0400, Alex Deucher wrote:
> >>       drm/amdgpu: skip suspend/resume on DRM_SWITCH_POWER_DYNAMIC_OFF
> >>       drm/radeon: skip suspend/resume on DRM_SWITCH_POWER_DYNAMIC_OFF
> >
> > Those two are unnecessary, it can't happen that the ->suspend hook
> > is executed with the device runtime suspended.
> >
> > Since commit d14d2a8453d6 ("drm: Remove dev_pm_ops from drm_class"),
> > DRM devices are afforded direct_complete, i.e. if the GPU is runtime
> > suspended upon system sleep, it is left in this state.  The only
> > callbacks that are executed are the ->prepare and the ->complete hook.
> > All the callbacks in-between, like ->suspend, are skipped.
> >
> > Even if direct_complete is not afforded for some reason, the PM core
> > will automatically runtime resume the device before executing ->suspend.
> >
> > That ->suspend is skipped in the DRM_SWITCH_POWER_OFF case was done
> > because the device is suspended behind the PM core's back if runpm=0
> > is set.  (And it doesn't work properly because the PCI core will
> > invalidate the saved_state during ->resume_noirq.  That could be
> > solved by returning 1 from the ->prepare hook in the DRM_SWITCH_POWER_OFF
> > case, but that's a different story.)
> >
> > (Sorry for not raising this earlier, I'm not subscribed to amd-gfx.)
> 
> Thanks for the heads up.  They shouldn't hurt anything and it matches
> what other drivers do.  If you feel strongly about it, I can revert
> them later.

Well, superfluous code shouldn't be included just because it doesn't hurt
or because others didn't know better either, or removed because of
someone else's feelings. ;-)

You can find the runtime resume before execution of the driver ->suspend
callback in drivers/pci/pci-driver.c:pci_pm_suspend():

        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
	...
        pm_runtime_resume(dev);
	...
        if (pm->suspend) {
		...
		error = pm->suspend(dev);

The device is prevented from auto-suspending because of a runtime PM ref
acquired beforehand in drivers/base/power/main.c:device_prepare().

Best regards,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel