Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1261701 > unrolled thread

Re: [PATCH 1/6] dmaengine: tegra-apb: Correct runtime-pm usage

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/6] dmaengine: tegra-apb: Correct runtime-pm usage Jon Hunter <jonathanh@nvidia.com> - 2015-11-03 17:30 +0100
    Re: [PATCH 1/6] dmaengine: tegra-apb: Correct runtime-pm usage Kevin Hilman <khilman@kernel.org> - 2015-11-03 22:30 +0100
      Re: [PATCH 1/6] dmaengine: tegra-apb: Correct runtime-pm usage Vinod Koul <vinod.koul@intel.com> - 2015-11-04 09:40 +0100
        Re: [PATCH 1/6] dmaengine: tegra-apb: Correct runtime-pm usage Kevin Hilman <khilman@kernel.org> - 2015-11-04 18:00 +0100
          Re: [PATCH 1/6] dmaengine: tegra-apb: Correct runtime-pm usage "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-11-05 02:50 +0100
            Re: [PATCH 1/6] dmaengine: tegra-apb: Correct runtime-pm usage Vinod Koul <vinod.koul@intel.com> - 2015-11-05 13:20 +0100

#1261701 — Re: [PATCH 1/6] dmaengine: tegra-apb: Correct runtime-pm usage

On 29/10/15 01:57, Vinod Koul wrote:
> On Wed, Oct 28, 2015 at 01:32:12PM +0000, Jon Hunter wrote:
>>
>> On 28/10/15 07:03, Vinod Koul wrote:
>>> On Fri, Oct 16, 2015 at 09:25:52AM +0100, Jon Hunter wrote:
>>>> @@ -1182,14 +1182,11 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>>>  {
>>>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>>>  	struct tegra_dma *tdma = tdc->tdma;
>>>> -	int ret;
>>>>  
>>>>  	dma_cookie_init(&tdc->dma_chan);
>>>>  	tdc->config_init = false;
>>>> -	ret = clk_prepare_enable(tdma->dma_clk);
>>>> -	if (ret < 0)
>>>> -		dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
>>>> -	return ret;
>>>> +
>>>> +	return pm_runtime_get_sync(tdma->dev);
>>>
>>> Alloc channel is supposed to return number of descriptors allocated and if
>>> pm_runtime_get_sync() returns postive values we get wrong return!
>>
>> Yes I will fix. I assume that returning 0 is allowed if no descriptors
>> are allocated here. So much for correcting rpm usage ;-)
> 
> Yes 0 is allowed...
> 
>>>>  static int tegra_dma_pm_suspend(struct device *dev)
>>>>  {
>>>>  	struct tegra_dma *tdma = dev_get_drvdata(dev);
>>>> -	int i;
>>>> -	int ret;
>>>> +	int i, ret;
>>>>  
>>>>  	/* Enable clock before accessing register */
>>>> -	ret = tegra_dma_runtime_resume(dev);
>>>> +	ret = pm_runtime_get_sync(dev);
>>>
>>> If you are runtime suspended then core will runtime resume you before
>>> invoking suspend, so why do we need this
>>
>> Is this change now in the mainline? Do you have commit ID for that?
>>
>> I recall the last time we discussed this that Rafael said that they were
>> going to do that, but he said as a rule of thumb if you need to resume
>> it, resume it [0].
> 
> IIRC this has been always the behaviour, at least I see this when I test the
> devices

I have been doing some testing today and if the DMA is runtime
suspended, then I don't see it runtime resumed before suspend is called.

Can you elborate on "at least I see this when I test the devices"? What
are you looking at? Are you using kernel function tracers in some way?

Cheers
Jon

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1261905

Jon Hunter <jonathanh@nvidia.com> writes:

> On 29/10/15 01:57, Vinod Koul wrote:
>> On Wed, Oct 28, 2015 at 01:32:12PM +0000, Jon Hunter wrote:
>>>
>>> On 28/10/15 07:03, Vinod Koul wrote:
>>>> On Fri, Oct 16, 2015 at 09:25:52AM +0100, Jon Hunter wrote:

[...]

>>>>>  	/* Enable clock before accessing register */
>>>>> -	ret = tegra_dma_runtime_resume(dev);
>>>>> +	ret = pm_runtime_get_sync(dev);
>>>>
>>>> If you are runtime suspended then core will runtime resume you before
>>>> invoking suspend, so why do we need this
>>>
>>> Is this change now in the mainline? Do you have commit ID for that?
>>>
>>> I recall the last time we discussed this that Rafael said that they were
>>> going to do that, but he said as a rule of thumb if you need to resume
>>> it, resume it [0].
>> 
>> IIRC this has been always the behaviour, at least I see this when I test the
>> devices
>
> I have been doing some testing today and if the DMA is runtime
> suspended, then I don't see it runtime resumed before suspend is called.
>
> Can you elborate on "at least I see this when I test the devices"? What
> are you looking at? Are you using kernel function tracers in some way?

The PM core does a _get_noresume()[1] which tries to prevent runtime
suspends *during* a system suspend.  However, the PM core should not be
doing an actual runtime resume of the device, so if the device is
already runtime suspended, it will not be runtime resumed by the core,
so if the driver needs it to be runtime resumed, it needs to do it
itself.

Kevin


[1] c.f. drivers/base/power/main.c::device_prepare()
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262130

On Tue, Nov 03, 2015 at 01:25:09PM -0800, Kevin Hilman wrote:
> >>>>>  	/* Enable clock before accessing register */
> >>>>> -	ret = tegra_dma_runtime_resume(dev);
> >>>>> +	ret = pm_runtime_get_sync(dev);
> >>>>
> >>>> If you are runtime suspended then core will runtime resume you before
> >>>> invoking suspend, so why do we need this
> >>>
> >>> Is this change now in the mainline? Do you have commit ID for that?
> >>>
> >>> I recall the last time we discussed this that Rafael said that they were
> >>> going to do that, but he said as a rule of thumb if you need to resume
> >>> it, resume it [0].
> >> 
> >> IIRC this has been always the behaviour, at least I see this when I test the
> >> devices
> >
> > I have been doing some testing today and if the DMA is runtime
> > suspended, then I don't see it runtime resumed before suspend is called.
> >
> > Can you elborate on "at least I see this when I test the devices"? What
> > are you looking at? Are you using kernel function tracers in some way?
> 
> The PM core does a _get_noresume()[1] which tries to prevent runtime
> suspends *during* a system suspend.  However, the PM core should not be
> doing an actual runtime resume of the device, so if the device is
> already runtime suspended, it will not be runtime resumed by the core,
> so if the driver needs it to be runtime resumed, it needs to do it
> itself.

+ Rafael

This is contrariry to what I see, If my driver is runtime suspended and on
suspend, it gets runtime resumed and then suspended

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262441

Vinod Koul <vinod.koul@intel.com> writes:

> On Tue, Nov 03, 2015 at 01:25:09PM -0800, Kevin Hilman wrote:
>> >>>>>  	/* Enable clock before accessing register */
>> >>>>> -	ret = tegra_dma_runtime_resume(dev);
>> >>>>> +	ret = pm_runtime_get_sync(dev);
>> >>>>
>> >>>> If you are runtime suspended then core will runtime resume you before
>> >>>> invoking suspend, so why do we need this
>> >>>
>> >>> Is this change now in the mainline? Do you have commit ID for that?
>> >>>
>> >>> I recall the last time we discussed this that Rafael said that they were
>> >>> going to do that, but he said as a rule of thumb if you need to resume
>> >>> it, resume it [0].
>> >> 
>> >> IIRC this has been always the behaviour, at least I see this when I test the
>> >> devices
>> >
>> > I have been doing some testing today and if the DMA is runtime
>> > suspended, then I don't see it runtime resumed before suspend is called.
>> >
>> > Can you elborate on "at least I see this when I test the devices"? What
>> > are you looking at? Are you using kernel function tracers in some way?
>> 
>> The PM core does a _get_noresume()[1] which tries to prevent runtime
>> suspends *during* a system suspend.  However, the PM core should not be
>> doing an actual runtime resume of the device, so if the device is
>> already runtime suspended, it will not be runtime resumed by the core,
>> so if the driver needs it to be runtime resumed, it needs to do it
>> itself.
>
> + Rafael
>
> This is contrariry to what I see, If my driver is runtime suspended and on
> suspend, it gets runtime resumed and then suspended

Since I was late to the thread, can you explain what kind of driver and
on what bus type you're seeing this behavior?

It could be that your bus-type is doing something, but I don't think it
should be the PM core.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262808

On Wednesday, November 04, 2015 08:59:43 AM Kevin Hilman wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
> 
> > On Tue, Nov 03, 2015 at 01:25:09PM -0800, Kevin Hilman wrote:
> >> >>>>>  	/* Enable clock before accessing register */
> >> >>>>> -	ret = tegra_dma_runtime_resume(dev);
> >> >>>>> +	ret = pm_runtime_get_sync(dev);
> >> >>>>
> >> >>>> If you are runtime suspended then core will runtime resume you before
> >> >>>> invoking suspend, so why do we need this
> >> >>>
> >> >>> Is this change now in the mainline? Do you have commit ID for that?
> >> >>>
> >> >>> I recall the last time we discussed this that Rafael said that they were
> >> >>> going to do that, but he said as a rule of thumb if you need to resume
> >> >>> it, resume it [0].
> >> >> 
> >> >> IIRC this has been always the behaviour, at least I see this when I test the
> >> >> devices
> >> >
> >> > I have been doing some testing today and if the DMA is runtime
> >> > suspended, then I don't see it runtime resumed before suspend is called.
> >> >
> >> > Can you elborate on "at least I see this when I test the devices"? What
> >> > are you looking at? Are you using kernel function tracers in some way?
> >> 
> >> The PM core does a _get_noresume()[1] which tries to prevent runtime
> >> suspends *during* a system suspend.  However, the PM core should not be
> >> doing an actual runtime resume of the device, so if the device is
> >> already runtime suspended, it will not be runtime resumed by the core,
> >> so if the driver needs it to be runtime resumed, it needs to do it
> >> itself.
> >
> > + Rafael
> >
> > This is contrariry to what I see, If my driver is runtime suspended and on
> > suspend, it gets runtime resumed and then suspended
> 
> Since I was late to the thread, can you explain what kind of driver and
> on what bus type you're seeing this behavior?
> 
> It could be that your bus-type is doing something, but I don't think it
> should be the PM core.

Right.

Bus types do that, the core doesn't.  The ACPI PM domain does that too
for some devices.

So Vinod, more details, please.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263168

On Thu, Nov 05, 2015 at 03:15:18AM +0100, Rafael J. Wysocki wrote:

> > > + Rafael
> > >
> > > This is contrariry to what I see, If my driver is runtime suspended and on
> > > suspend, it gets runtime resumed and then suspended
> > 
> > Since I was late to the thread, can you explain what kind of driver and
> > on what bus type you're seeing this behavior?
> > 
> > It could be that your bus-type is doing something, but I don't think it
> > should be the PM core.
> 
> Right.
> 
> Bus types do that, the core doesn't.  The ACPI PM domain does that too
> for some devices.
> 
> So Vinod, more details, please.

Okay relooking at core I do think that runtime resume should not be invoked
while suspending, as core seems to call pm_runtime_get_noresume() but I am
still missing something here..

I do see this behaviour (runtime resume on suspend) on Intel audio drivers
which are PCI devices, is PCI or ACPI doing some magic here.

I have seen this as consistent behavior and actually an irritant, as we used
to download firmware in resume patch, but then we end up thrashing the
controller while going to suspend!

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web