Date:   Mon, 4 Feb 2019 15:00:24 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jon Hunter <jonathanh@...dia.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Takashi Iwai <tiwai@...e.de>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Sameer Pujar <spujar@...dia.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@...a-project.org>, mkumard@...dia.com,
        rlokhande@...dia.com, sharadg@...dia.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-tegra@...r.kernel.org, Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v2] ALSA: hda/tegra: enable clock during probe

On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
> 04.02.2019 14:05, Thierry Reding пишет:
> > On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
> >>
> >>
> >> On 04/02/2019 08:45, Thierry Reding wrote:
> >>
> >> ...
> >>
> >>> The idea was, as I was saying below, to reuse dev_pm_ops even if
> >>> !CONFIG_PM. So pm_runtime_enable() could be something like this:
> >>>
> >>> 	pm_runtime_enable(dev)
> >>> 	{
> >>> 		if (!CONFIG_PM)
> >>> 			if (dev->pm_ops->resume)
> >>> 				dev->pm_ops->resume(dev);
> >>>
> >>> 		...
> >>> 	}
> >>>
> >>> But that's admittedly somewhat of a stretch. This could of course be
> >>> made somewhat nicer by adding an explicit variant, say:
> >>>
> >>> 	pm_runtime_enable_foo(dev)
> >>> 	{
> >>> 		if (!CONFIG_PM && dev->pm_ops->resume)
> >>> 			return dev->pm_ops->resume(dev);
> >>>
> >>> 		return 0;
> >>> 	}
> >>>
> >>> Maybe the fact that I couldn't come up with a good name is a good
> >>> indication that this is a bad idea...
> >>
> >> How about some new APIs called ...
> >>
> >> pm_runtime_enable_get()
> >> pm_runtime_enable_get_sync()
> >> pm_runtime_put_disable() (implies a put_sync)
> >>
> >> ... and in these APIs we add ...
> >>
> >> pm_runtime_enable_get(dev)
> >> {
> >> 	if (!CONFIG_PM && dev->pm_ops->resume)
> >> 		return dev->pm_ops->resume(dev);
> >>
> >> 	pm_runtime_enable(dev);
> >> 	return pm_runtime_get(dev);
> >> }
> > 
> > Yeah, those sound sensible. I'm still a bit torn between this and just
> > enforcing PM. At least on the display side I think we already require PM
> > and with all the power domain work that you did we'd be much better off
> > if we could just get rid of the !PM workarounds.
> > 
> >>>>> This would be somewhat tricky because drivers
> >>>>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and
> >>>>> that would result in an empty structure if !CONFIG_PM, but we could
> >>>>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would
> >>>>> never be compiled out for this kind of case. Or such drivers could even
> >>>>> manually set .runtime_suspend and .runtime_resume to make sure they're
> >>>>> always populated.
> >>>>>
> >>>>> Another way out of this would be to make sure we never run into the case
> >>>>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM
> >>>>> should always be available. But is it guaranteed that runtime PM for the
> >>>>> devices is functional in that case? From a cursory look at the code it
> >>>>> would seem that way.
> >>>>
> >>>> If you select PM, then all of the requisite code should be there.
> >>>
> >>> We do this on 64-bit ARM, but there had been some pushback when we had
> >>> proposed to do the same thing on 32-bit ARM. I think there were two
> >>> concerns:
> >>>
> >>> 	1) select PM would force the setting for all platforms on multi-
> >>> 	   platforms builds
> >>>
> >>> 	2) prevents anyone from disabling PM for debugging purposes
> >>>
> >>> 1) no longer seems to be valid because Rockchip already selects PM
> >>> unconditionally. I'm not sure if 2) is valid anymore either. I haven't
> >>> run a build with !PM in a very long time and I wouldn't be surprised if
> >>> that was completely broken.
> >>>
> >>> Maybe we need to try this again since a couple of years have elapsed and
> >>> runtime PM support on Tegra is much more mature at this point.
> >>>
> >>>> Alternatively, you can make the driver depend on PM.
> >>>
> >>> That's probably the easiest way out, but to be honest I think I'd prefer
> >>> to just enforce PM and keep things simple.
> >>>
> >>> Jon, any objections?
> >>
> >> None, but seems overkill just for this case.
> > 
> > But that's precisely the point. It's not just about this case. We've
> > already got some drivers where we do a similar dance just to be able to
> > support the, let's admit it, exotic case where somebody turns off PM. I
> > think supporting !PM might have made sense at a point where we had no
> > support for power management at all. But I think we've come a long way
> > since then, and while we may still have some ways to go in some areas,
> > we do fairly decent runtime PM most of the time, to the point where I no
> > longer see any benefits in !PM.
> 
> I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a
> useful debug feature, it can't just go away. 

What is it about disabling PM that you consider useful? I can understand
why you'd want that option if power management is broken, but as far as
I can tell, power management on Tegra is in pretty good state, and it's
more likely that !PM would actually be broken (though I haven't built a
configuration like that in a couple of years, so I'm speculating). We
already can't disable PM on 64-bit ARM, so I don't understand why 32-bit
ARM should be treated any differently.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists