Message-ID: <20200314094904.GP4885@vkoul-mobl>
Date:   Sat, 14 Mar 2020 15:19:04 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     alsa-devel@...a-project.org, tiwai@...e.de,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Hui Wang <hui.wang@...onical.com>, broonie@...nel.org,
        srinivas.kandagatla@...aro.org, jank@...ence.com,
        slawomir.blauciak@...el.com, Sanyog Kale <sanyog.r.kale@...el.com>,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>
Subject: Re: [PATCH 1/8] soundwire: bus_type: add master_device/driver support

On 13-03-20, 11:54, Pierre-Louis Bossart wrote:
> 
> > > > > the ASoC layer does require a driver with a 'name' for the components
> > > > > registered with the master device. So if you don't have a driver for the
> > > > > master device, the DAIs will be associated with the PCI device.
> > > > > 
> > > > > But the ASoC core does make pm_runtime calls on its own,
> > > > > 
> > > > > soc_pcm_open(struct snd_pcm_substream *substream)
> > > > > {
> > > > > ...
> > > > > 	for_each_rtd_components(rtd, i, component)
> > > > > 		pm_runtime_get_sync(component->dev);
> > > > > 
> > > > > and if the device that's associated with the DAI is the PCI device, then
> > > > > that will not result in the relevant master IP being activated, only the PCI
> > > > > device refcount will be increased - meaning there is no hook that would tell
> > > > > the PCI layer to turn on a specific link.
> > > > > 
> > > > > What you are recommending would be an all-or-nothing solution with all links
> > > > > on or all links off, which beats the purpose of having independent
> > > > > link-level power management.
> > > > 
> > > > Why can't you use dai .startup callback for this?
> > > > 
> > > > The ASoC core will do pm_runtime calls that will ensure PCI device is
> > > > up, DSP firmware downloaded and running.
> > > > 
> > > > You can use .startup() to turn on your link and .shutdown to turn off
> > > > the link.
> > > 
> > > There are multiple dais per link, and multiple Slave per link, so we would
> > > have to refcount and track active dais to understand when the link needs to
> > > be turned on/off. It's a duplication of what the pm framework can do at the
> > > device/link level, and will likely introduce race conditions.
> > > 
> > > Not to mention that we'd need to introduce workqueues to turn the link off
> > > with a delay, with pm_runtime_put_autosuspend() does for free.
> > 
> > Yes sure, that seems to be the cost unfortunately. While it might feel I
> > am blocking but the real block here is the hw design which gives you a
> > monolith whereas it should have been different devices. If you have a
> > 'device' for sdw or a standalone controller we would not be debating
> > this..
> 
> The hardware is what it is. The ACPI spec is what it is.
> 
> I am just pragmatic and making platforms work with that's available *today*,
> and I don't have time or interest in revisiting what might have been.
> 
> > > Linux is all about frameworks. For power management, we shall use the power
> > > management framework, not reinvent it.
> > 
> > This reminds me, please talk to Mika and Rafael, they had similar
> > problems with lpss etc and IIRC they were working on splices to solve
> > this.. Its been some time (few years now) so maybe they have a
> > solution..
> 
> We've been discussing this since October, I don't really have any appetite
> for looking into new concepts when the existing framework just does what we
> need.

yes they do but add an intrusive platform specific change into soundwire
core, something I would not like to add.

You should really be willing to talk to your colleagues to see if there
is something you can reuse.

> It's really down to your objection to the use of 'struct driver'... For ASoC
> support we only need the .name and .pm_ops, so there's really no possible
> path forward otherwise.

It means that we cannot have a solution which is Intel specific into
core. If you has a standalone controller you do not need this.

> Like I said, we have 3 options

Repeating the already discussed doesn't help. I have already told you the
constraint to work is not to add Intel specific change into core.

I have already said that expect the driver part I dont have objections
to rest of this series and am ready to merge

> a) stay with platform devices for now. You will need to have a conversation
> with Greg on this.
> 
> b) use a minimal sdw_master_device with a minimal 'struct driver' use.
> 
> c) use a more elaborate solution suggested in this patchset and yes that
> means the Qualcomm driver would need to change a bit.
> 
> Pick one or suggest something that is implementable. The first version of
> the patches was provided in October, the last RFC was provided on January
> 31, time's up. At the moment you are preventing ASoC integration from moving
> forward.

In opensource review we go back and forth and we debate and come to a
common conclusion. Choosing a specific set of solutions and constraining
yourself to pick one does not help.

I have only _one_ constraint no platform specific change in core. If that
is satisfied I will go with it. Sorry but this is non-negotiable for me.

Ask yourself, do you need this intrusive core change if you had this
exact same controller(s) but only as standalone one... 

-- 
~Vinod

Powered by blists - more mailing lists