All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers
@ 2017-12-08 12:30 Vivek Gautam
  2017-12-08 13:21 ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: Vivek Gautam @ 2017-12-08 12:30 UTC (permalink / raw)
  To: rjw, len.brown, pavel, gregkh
  Cc: linux-pm, linux-kernel, sboyd, robdclark, robin.murphy,
	will.deacon, linux-arm-msm, Vivek Gautam

The device link allows the pm framework to tie the supplier and
consumer. So, whenever the consumer is powered-on, the supplier
is powered-on first.

There are however cases in which the consumer wants to power-on
the supplier, but not itself.
E.g., A Graphics or multimedia driver wants to power-on the SMMU
to unmap a buffer and finish the TLB operations without powering
on itself. Some of these unmap requests are coming from the
user space when the controller itself is not powered-up, and it
can be huge penalty in terms of power and latency to power-up
the graphics/mm controllers.
There can be an argument that the supplier should handle this case
on its own and there should not be a need for the consumer to
power-on the supplier. But as discussed on the thread [1] about
ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
in atomic paths, such as in arm_smmu_unmap.

[1] https://patchwork.kernel.org/patch/9827825/

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/base/power/runtime.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 027d159ac381..af169304ca13 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1578,6 +1578,7 @@ void pm_runtime_get_suppliers(struct device *dev)
 
 	device_links_read_unlock(idx);
 }
+EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
 
 /**
  * pm_runtime_put_suppliers - Drop references to supplier devices.
@@ -1596,6 +1597,7 @@ void pm_runtime_put_suppliers(struct device *dev)
 
 	device_links_read_unlock(idx);
 }
+EXPORT_SYMBOL_GPL(pm_runtime_put_suppliers);
 
 void pm_runtime_new_link(struct device *dev)
 {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers
  2017-12-08 12:30 [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers Vivek Gautam
@ 2017-12-08 13:21 ` Greg KH
  2017-12-08 13:39   ` Vivek Gautam
  0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2017-12-08 13:21 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, sboyd, robdclark,
	robin.murphy, will.deacon, linux-arm-msm

On Fri, Dec 08, 2017 at 06:00:47PM +0530, Vivek Gautam wrote:
> The device link allows the pm framework to tie the supplier and
> consumer. So, whenever the consumer is powered-on, the supplier
> is powered-on first.
> 
> There are however cases in which the consumer wants to power-on
> the supplier, but not itself.
> E.g., A Graphics or multimedia driver wants to power-on the SMMU
> to unmap a buffer and finish the TLB operations without powering
> on itself. Some of these unmap requests are coming from the
> user space when the controller itself is not powered-up, and it
> can be huge penalty in terms of power and latency to power-up
> the graphics/mm controllers.
> There can be an argument that the supplier should handle this case
> on its own and there should not be a need for the consumer to
> power-on the supplier. But as discussed on the thread [1] about
> ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
> in atomic paths, such as in arm_smmu_unmap.
> 
> [1] https://patchwork.kernel.org/patch/9827825/
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  drivers/base/power/runtime.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 027d159ac381..af169304ca13 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1578,6 +1578,7 @@ void pm_runtime_get_suppliers(struct device *dev)
>  
>  	device_links_read_unlock(idx);
>  }
> +EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);

We do not export symbols unless there are in-kernel users of them.
Where is the patch that adds a user for these functions?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers
  2017-12-08 13:21 ` Greg KH
@ 2017-12-08 13:39   ` Vivek Gautam
  2017-12-08 14:07     ` Rafael J. Wysocki
  0 siblings, 1 reply; 7+ messages in thread
From: Vivek Gautam @ 2017-12-08 13:39 UTC (permalink / raw)
  To: Greg KH
  Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, Stephen Boyd,
	Rob Clark, Robin Murphy, Will Deacon, linux-arm-msm

Hi Greg,


On Fri, Dec 8, 2017 at 6:51 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Dec 08, 2017 at 06:00:47PM +0530, Vivek Gautam wrote:
>> The device link allows the pm framework to tie the supplier and
>> consumer. So, whenever the consumer is powered-on, the supplier
>> is powered-on first.
>>
>> There are however cases in which the consumer wants to power-on
>> the supplier, but not itself.
>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
>> to unmap a buffer and finish the TLB operations without powering
>> on itself. Some of these unmap requests are coming from the
>> user space when the controller itself is not powered-up, and it
>> can be huge penalty in terms of power and latency to power-up
>> the graphics/mm controllers.
>> There can be an argument that the supplier should handle this case
>> on its own and there should not be a need for the consumer to
>> power-on the supplier. But as discussed on the thread [1] about
>> ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
>> in atomic paths, such as in arm_smmu_unmap.
>>
>> [1] https://patchwork.kernel.org/patch/9827825/
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>  drivers/base/power/runtime.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index 027d159ac381..af169304ca13 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -1578,6 +1578,7 @@ void pm_runtime_get_suppliers(struct device *dev)
>>
>>       device_links_read_unlock(idx);
>>  }
>> +EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
>
> We do not export symbols unless there are in-kernel users of them.
> Where is the patch that adds a user for these functions?

My apologies for not putting the changes for the user of these APIs.
I will be sending a patch for the user (which would be:
"drivers/gpu/drm/msm/msm_iommu.c"). The patch will be included
with the arm-smmu runtime patch series. Right now I am facing issues
with the use of clk_bulk_*() APIs on 4.15-rc kernel.

But, I wanted to get opinions about this change since we had been
discussing about this in the arm-smmu runtime patch thread [1].

[1] https://patchwork.kernel.org/patch/9827825/


P.S.: A snippet of the change in the user of these APIs:

 diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
 index b23d33622f37..1ab629bbee69 100644
 --- a/drivers/gpu/drm/msm/msm_iommu.c
 +++ b/drivers/gpu/drm/msm/msm_iommu.c
 @@ -76,9 +76,9 @@ static int msm_iommu_unmap(struct msm_mmu *mmu,
uint64_t iova,
  {
         struct msm_iommu *iommu = to_msm_iommu(mmu);

 -       pm_runtime_get_sync(mmu->dev);
 +       pm_runtime_get_suppliers(mmu->dev);
         iommu_unmap(iommu->domain, iova, len);
 -       pm_runtime_put_sync(mmu->dev);
 +       pm_runtime_put_suppliers(mmu->dev);

         return 0;
  }


regards
Vivek

>
> thanks,
>
> greg k-h



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers
  2017-12-08 13:39   ` Vivek Gautam
@ 2017-12-08 14:07     ` Rafael J. Wysocki
  2017-12-08 17:03       ` Vivek Gautam
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2017-12-08 14:07 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Greg KH, Rafael J. Wysocki, Len Brown, Pavel Machek, Linux PM,
	linux-kernel, Stephen Boyd, Rob Clark, Robin Murphy, Will Deacon,
	linux-arm-msm

On Fri, Dec 8, 2017 at 2:39 PM, Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:
> Hi Greg,
>
>
> On Fri, Dec 8, 2017 at 6:51 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Fri, Dec 08, 2017 at 06:00:47PM +0530, Vivek Gautam wrote:
>>> The device link allows the pm framework to tie the supplier and
>>> consumer. So, whenever the consumer is powered-on, the supplier
>>> is powered-on first.
>>>
>>> There are however cases in which the consumer wants to power-on
>>> the supplier, but not itself.
>>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
>>> to unmap a buffer and finish the TLB operations without powering
>>> on itself. Some of these unmap requests are coming from the
>>> user space when the controller itself is not powered-up, and it
>>> can be huge penalty in terms of power and latency to power-up
>>> the graphics/mm controllers.
>>> There can be an argument that the supplier should handle this case
>>> on its own and there should not be a need for the consumer to
>>> power-on the supplier. But as discussed on the thread [1] about
>>> ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
>>> in atomic paths, such as in arm_smmu_unmap.
>>>
>>> [1] https://patchwork.kernel.org/patch/9827825/
>>>
>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>>> ---
>>>  drivers/base/power/runtime.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>> index 027d159ac381..af169304ca13 100644
>>> --- a/drivers/base/power/runtime.c
>>> +++ b/drivers/base/power/runtime.c
>>> @@ -1578,6 +1578,7 @@ void pm_runtime_get_suppliers(struct device *dev)
>>>
>>>       device_links_read_unlock(idx);
>>>  }
>>> +EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
>>
>> We do not export symbols unless there are in-kernel users of them.
>> Where is the patch that adds a user for these functions?
>
> My apologies for not putting the changes for the user of these APIs.
> I will be sending a patch for the user (which would be:
> "drivers/gpu/drm/msm/msm_iommu.c"). The patch will be included
> with the arm-smmu runtime patch series. Right now I am facing issues
> with the use of clk_bulk_*() APIs on 4.15-rc kernel.
>
> But, I wanted to get opinions about this change since we had been
> discussing about this in the arm-smmu runtime patch thread [1].
>
> [1] https://patchwork.kernel.org/patch/9827825/
>
>
> P.S.: A snippet of the change in the user of these APIs:
>
>  diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
>  index b23d33622f37..1ab629bbee69 100644
>  --- a/drivers/gpu/drm/msm/msm_iommu.c
>  +++ b/drivers/gpu/drm/msm/msm_iommu.c
>  @@ -76,9 +76,9 @@ static int msm_iommu_unmap(struct msm_mmu *mmu,
> uint64_t iova,
>   {
>          struct msm_iommu *iommu = to_msm_iommu(mmu);
>
>  -       pm_runtime_get_sync(mmu->dev);
>  +       pm_runtime_get_suppliers(mmu->dev);
>          iommu_unmap(iommu->domain, iova, len);
>  -       pm_runtime_put_sync(mmu->dev);
>  +       pm_runtime_put_suppliers(mmu->dev);
>
>          return 0;
>   }
>

Well, pm_runtime_get/put_suppliers() were not designed to be used
outside of the runtime PM core code.  I need to have a deeper look
into things at this point, so give me some time.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers
  2017-12-08 14:07     ` Rafael J. Wysocki
@ 2017-12-08 17:03       ` Vivek Gautam
  2018-01-03 12:01         ` Rafael J. Wysocki
  0 siblings, 1 reply; 7+ messages in thread
From: Vivek Gautam @ 2017-12-08 17:03 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Greg KH, Rafael J. Wysocki, Len Brown, Pavel Machek, Linux PM,
	linux-kernel, Stephen Boyd, Rob Clark, Robin Murphy, Will Deacon,
	linux-arm-msm

On Fri, Dec 8, 2017 at 7:37 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Dec 8, 2017 at 2:39 PM, Vivek Gautam
> <vivek.gautam@codeaurora.org> wrote:
>> Hi Greg,
>>
>>
>> On Fri, Dec 8, 2017 at 6:51 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> On Fri, Dec 08, 2017 at 06:00:47PM +0530, Vivek Gautam wrote:
>>>> The device link allows the pm framework to tie the supplier and
>>>> consumer. So, whenever the consumer is powered-on, the supplier
>>>> is powered-on first.
>>>>
>>>> There are however cases in which the consumer wants to power-on
>>>> the supplier, but not itself.
>>>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
>>>> to unmap a buffer and finish the TLB operations without powering
>>>> on itself. Some of these unmap requests are coming from the
>>>> user space when the controller itself is not powered-up, and it
>>>> can be huge penalty in terms of power and latency to power-up
>>>> the graphics/mm controllers.
>>>> There can be an argument that the supplier should handle this case
>>>> on its own and there should not be a need for the consumer to
>>>> power-on the supplier. But as discussed on the thread [1] about
>>>> ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
>>>> in atomic paths, such as in arm_smmu_unmap.
>>>>
>>>> [1] https://patchwork.kernel.org/patch/9827825/
>>>>
>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>>>> ---
>>>>  drivers/base/power/runtime.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>>> index 027d159ac381..af169304ca13 100644
>>>> --- a/drivers/base/power/runtime.c
>>>> +++ b/drivers/base/power/runtime.c
>>>> @@ -1578,6 +1578,7 @@ void pm_runtime_get_suppliers(struct device *dev)
>>>>
>>>>       device_links_read_unlock(idx);
>>>>  }
>>>> +EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
>>>
>>> We do not export symbols unless there are in-kernel users of them.
>>> Where is the patch that adds a user for these functions?
>>
>> My apologies for not putting the changes for the user of these APIs.
>> I will be sending a patch for the user (which would be:
>> "drivers/gpu/drm/msm/msm_iommu.c"). The patch will be included
>> with the arm-smmu runtime patch series. Right now I am facing issues
>> with the use of clk_bulk_*() APIs on 4.15-rc kernel.
>>
>> But, I wanted to get opinions about this change since we had been
>> discussing about this in the arm-smmu runtime patch thread [1].
>>
>> [1] https://patchwork.kernel.org/patch/9827825/
>>
>>
>> P.S.: A snippet of the change in the user of these APIs:
>>
>>  diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
>>  index b23d33622f37..1ab629bbee69 100644
>>  --- a/drivers/gpu/drm/msm/msm_iommu.c
>>  +++ b/drivers/gpu/drm/msm/msm_iommu.c
>>  @@ -76,9 +76,9 @@ static int msm_iommu_unmap(struct msm_mmu *mmu,
>> uint64_t iova,
>>   {
>>          struct msm_iommu *iommu = to_msm_iommu(mmu);
>>
>>  -       pm_runtime_get_sync(mmu->dev);
>>  +       pm_runtime_get_suppliers(mmu->dev);
>>          iommu_unmap(iommu->domain, iova, len);
>>  -       pm_runtime_put_sync(mmu->dev);
>>  +       pm_runtime_put_suppliers(mmu->dev);
>>
>>          return 0;
>>   }
>>
>
> Well, pm_runtime_get/put_suppliers() were not designed to be used
> outside of the runtime PM core code.  I need to have a deeper look
> into things at this point, so give me some time.

Thanks Rafael.

regards
Vivek

>
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers
  2017-12-08 17:03       ` Vivek Gautam
@ 2018-01-03 12:01         ` Rafael J. Wysocki
  2018-01-03 12:42           ` Vivek Gautam
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2018-01-03 12:01 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Rafael J. Wysocki, Greg KH, Len Brown, Pavel Machek, Linux PM,
	linux-kernel, Stephen Boyd, Rob Clark, Robin Murphy, Will Deacon,
	linux-arm-msm

On Friday, December 8, 2017 6:03:37 PM CET Vivek Gautam wrote:
> On Fri, Dec 8, 2017 at 7:37 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Fri, Dec 8, 2017 at 2:39 PM, Vivek Gautam
> > <vivek.gautam@codeaurora.org> wrote:
> >> Hi Greg,
> >>
> >>
> >> On Fri, Dec 8, 2017 at 6:51 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>> On Fri, Dec 08, 2017 at 06:00:47PM +0530, Vivek Gautam wrote:
> >>>> The device link allows the pm framework to tie the supplier and
> >>>> consumer. So, whenever the consumer is powered-on, the supplier
> >>>> is powered-on first.
> >>>>
> >>>> There are however cases in which the consumer wants to power-on
> >>>> the supplier, but not itself.
> >>>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
> >>>> to unmap a buffer and finish the TLB operations without powering
> >>>> on itself. Some of these unmap requests are coming from the
> >>>> user space when the controller itself is not powered-up, and it
> >>>> can be huge penalty in terms of power and latency to power-up
> >>>> the graphics/mm controllers.
> >>>> There can be an argument that the supplier should handle this case
> >>>> on its own and there should not be a need for the consumer to
> >>>> power-on the supplier. But as discussed on the thread [1] about
> >>>> ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
> >>>> in atomic paths, such as in arm_smmu_unmap.
> >>>>
> >>>> [1] https://patchwork.kernel.org/patch/9827825/
> >>>>
> >>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> >>>> ---
> >>>>  drivers/base/power/runtime.c | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> >>>> index 027d159ac381..af169304ca13 100644
> >>>> --- a/drivers/base/power/runtime.c
> >>>> +++ b/drivers/base/power/runtime.c
> >>>> @@ -1578,6 +1578,7 @@ void pm_runtime_get_suppliers(struct device *dev)
> >>>>
> >>>>       device_links_read_unlock(idx);
> >>>>  }
> >>>> +EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
> >>>
> >>> We do not export symbols unless there are in-kernel users of them.
> >>> Where is the patch that adds a user for these functions?
> >>
> >> My apologies for not putting the changes for the user of these APIs.
> >> I will be sending a patch for the user (which would be:
> >> "drivers/gpu/drm/msm/msm_iommu.c"). The patch will be included
> >> with the arm-smmu runtime patch series. Right now I am facing issues
> >> with the use of clk_bulk_*() APIs on 4.15-rc kernel.
> >>
> >> But, I wanted to get opinions about this change since we had been
> >> discussing about this in the arm-smmu runtime patch thread [1].
> >>
> >> [1] https://patchwork.kernel.org/patch/9827825/
> >>
> >>
> >> P.S.: A snippet of the change in the user of these APIs:
> >>
> >>  diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> >>  index b23d33622f37..1ab629bbee69 100644
> >>  --- a/drivers/gpu/drm/msm/msm_iommu.c
> >>  +++ b/drivers/gpu/drm/msm/msm_iommu.c
> >>  @@ -76,9 +76,9 @@ static int msm_iommu_unmap(struct msm_mmu *mmu,
> >> uint64_t iova,
> >>   {
> >>          struct msm_iommu *iommu = to_msm_iommu(mmu);
> >>
> >>  -       pm_runtime_get_sync(mmu->dev);
> >>  +       pm_runtime_get_suppliers(mmu->dev);
> >>          iommu_unmap(iommu->domain, iova, len);
> >>  -       pm_runtime_put_sync(mmu->dev);
> >>  +       pm_runtime_put_suppliers(mmu->dev);
> >>
> >>          return 0;
> >>   }
> >>
> >
> > Well, pm_runtime_get/put_suppliers() were not designed to be used
> > outside of the runtime PM core code.  I need to have a deeper look
> > into things at this point, so give me some time.
> 
> Thanks Rafael.

No problem, sorry for the delay.

It should be OK to export these routines as you proposed, but again,
please post this patch along with the driver changes depending on it.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers
  2018-01-03 12:01         ` Rafael J. Wysocki
@ 2018-01-03 12:42           ` Vivek Gautam
  0 siblings, 0 replies; 7+ messages in thread
From: Vivek Gautam @ 2018-01-03 12:42 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Greg KH, Len Brown, Pavel Machek, Linux PM,
	linux-kernel, Stephen Boyd, Rob Clark, Robin Murphy, Will Deacon,
	linux-arm-msm

Hi Rafael,

On Wed, Jan 3, 2018 at 5:31 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, December 8, 2017 6:03:37 PM CET Vivek Gautam wrote:
>> On Fri, Dec 8, 2017 at 7:37 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Fri, Dec 8, 2017 at 2:39 PM, Vivek Gautam
>> > <vivek.gautam@codeaurora.org> wrote:
>> >> Hi Greg,
>> >>
>> >>
>> >> On Fri, Dec 8, 2017 at 6:51 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >>> On Fri, Dec 08, 2017 at 06:00:47PM +0530, Vivek Gautam wrote:
>> >>>> The device link allows the pm framework to tie the supplier and
>> >>>> consumer. So, whenever the consumer is powered-on, the supplier
>> >>>> is powered-on first.
>> >>>>
>> >>>> There are however cases in which the consumer wants to power-on
>> >>>> the supplier, but not itself.
>> >>>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
>> >>>> to unmap a buffer and finish the TLB operations without powering
>> >>>> on itself. Some of these unmap requests are coming from the
>> >>>> user space when the controller itself is not powered-up, and it
>> >>>> can be huge penalty in terms of power and latency to power-up
>> >>>> the graphics/mm controllers.
>> >>>> There can be an argument that the supplier should handle this case
>> >>>> on its own and there should not be a need for the consumer to
>> >>>> power-on the supplier. But as discussed on the thread [1] about
>> >>>> ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
>> >>>> in atomic paths, such as in arm_smmu_unmap.
>> >>>>
>> >>>> [1] https://patchwork.kernel.org/patch/9827825/
>> >>>>
>> >>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> >>>> ---
>> >>>>  drivers/base/power/runtime.c | 2 ++
>> >>>>  1 file changed, 2 insertions(+)
>> >>>>
>> >>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> >>>> index 027d159ac381..af169304ca13 100644
>> >>>> --- a/drivers/base/power/runtime.c
>> >>>> +++ b/drivers/base/power/runtime.c
>> >>>> @@ -1578,6 +1578,7 @@ void pm_runtime_get_suppliers(struct device *dev)
>> >>>>
>> >>>>       device_links_read_unlock(idx);
>> >>>>  }
>> >>>> +EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
>> >>>
>> >>> We do not export symbols unless there are in-kernel users of them.
>> >>> Where is the patch that adds a user for these functions?
>> >>
>> >> My apologies for not putting the changes for the user of these APIs.
>> >> I will be sending a patch for the user (which would be:
>> >> "drivers/gpu/drm/msm/msm_iommu.c"). The patch will be included
>> >> with the arm-smmu runtime patch series. Right now I am facing issues
>> >> with the use of clk_bulk_*() APIs on 4.15-rc kernel.
>> >>
>> >> But, I wanted to get opinions about this change since we had been
>> >> discussing about this in the arm-smmu runtime patch thread [1].
>> >>
>> >> [1] https://patchwork.kernel.org/patch/9827825/
>> >>
>> >>
>> >> P.S.: A snippet of the change in the user of these APIs:
>> >>
>> >>  diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
>> >>  index b23d33622f37..1ab629bbee69 100644
>> >>  --- a/drivers/gpu/drm/msm/msm_iommu.c
>> >>  +++ b/drivers/gpu/drm/msm/msm_iommu.c
>> >>  @@ -76,9 +76,9 @@ static int msm_iommu_unmap(struct msm_mmu *mmu,
>> >> uint64_t iova,
>> >>   {
>> >>          struct msm_iommu *iommu = to_msm_iommu(mmu);
>> >>
>> >>  -       pm_runtime_get_sync(mmu->dev);
>> >>  +       pm_runtime_get_suppliers(mmu->dev);
>> >>          iommu_unmap(iommu->domain, iova, len);
>> >>  -       pm_runtime_put_sync(mmu->dev);
>> >>  +       pm_runtime_put_suppliers(mmu->dev);
>> >>
>> >>          return 0;
>> >>   }
>> >>
>> >
>> > Well, pm_runtime_get/put_suppliers() were not designed to be used
>> > outside of the runtime PM core code.  I need to have a deeper look
>> > into things at this point, so give me some time.
>>
>> Thanks Rafael.
>
> No problem, sorry for the delay.
>
> It should be OK to export these routines as you proposed, but again,
> please post this patch along with the driver changes depending on it.

Thanks. Sure, i will post the patch as part of the patch series for
the smmu driver.

Regards
Vivek

>
> Thanks,
> Rafael
>



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-01-03 12:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-08 12:30 [PATCH 1/1] base: power: runtime: Export pm_runtime_get/put_suppliers Vivek Gautam
2017-12-08 13:21 ` Greg KH
2017-12-08 13:39   ` Vivek Gautam
2017-12-08 14:07     ` Rafael J. Wysocki
2017-12-08 17:03       ` Vivek Gautam
2018-01-03 12:01         ` Rafael J. Wysocki
2018-01-03 12:42           ` Vivek Gautam

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.