Re: [PATCH 1/2] venus: use on-chip interconnect API

Re: [PATCH 1/2] venus: use on-chip interconnect API

[Georgi Djakov]

Hi Stan,

On 8/14/19 11:47, Stanimir Varbanov wrote:
> This aims to add a requests for bandwidth scaling depending
> on the resolution and framerate (macroblocks per second). The
> exact value ff the requested bandwidth is get from a

s/ff/of/

> pre-calculated tables for encoder and decoder.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c    | 34 +++++++++++
>  drivers/media/platform/qcom/venus/core.h    | 14 +++++
>  drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++-
>  3 files changed, 114 insertions(+), 1 deletion(-)

It looks like venus can be built-in, so how about the case when venus is
built-in and the interconnect provider is a module? Maybe add a dependency in
Kconfig to depend on INTERCONNECT || !INTERCONNECT?

> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 0acc7576cc58..19cbe9d5d028 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -5,6 +5,7 @@
>   */
>  #include <linux/clk.h>
>  #include <linux/init.h>
> +#include <linux/interconnect.h>
>  #include <linux/ioctl.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -239,6 +240,14 @@ static int venus_probe(struct platform_device *pdev)
>  	if (IS_ERR(core->base))
>  		return PTR_ERR(core->base);
>  
> +	core->video_path = of_icc_get(dev, "video-mem");
> +	if (IS_ERR(core->video_path))
> +		return PTR_ERR(core->video_path);
> +
> +	core->cpucfg_path = of_icc_get(dev, "cpu-cfg");
> +	if (IS_ERR(core->cpucfg_path))
> +		return PTR_ERR(core->cpucfg_path);
> +
>  	core->irq = platform_get_irq(pdev, 0);
>  	if (core->irq < 0)
>  		return core->irq;
> @@ -273,6 +282,10 @@ static int venus_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
> +	if (ret)
> +		return ret;
> +
>  	ret = hfi_create(core, &venus_core_ops);
>  	if (ret)
>  		return ret;
> @@ -355,6 +368,9 @@ static int venus_remove(struct platform_device *pdev)
>  	pm_runtime_put_sync(dev);
>  	pm_runtime_disable(dev);
>  
> +	icc_put(core->video_path);
> +	icc_put(core->cpucfg_path);
> +

Do you have any plans to scale the bandwidth on suspend/resume too?

Thanks,
Georgi

Re: [PATCH 1/2] venus: use on-chip interconnect API

[Stanimir Varbanov]

Hi Georgi,

Thanks for the review!

On 8/20/19 12:34 PM, Georgi Djakov wrote:
> Hi Stan,
> 
> On 8/14/19 11:47, Stanimir Varbanov wrote:
>> This aims to add a requests for bandwidth scaling depending
>> on the resolution and framerate (macroblocks per second). The
>> exact value ff the requested bandwidth is get from a
> 
> s/ff/of/
> 
>> pre-calculated tables for encoder and decoder.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.c    | 34 +++++++++++
>>  drivers/media/platform/qcom/venus/core.h    | 14 +++++
>>  drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++-
>>  3 files changed, 114 insertions(+), 1 deletion(-)
> 
> It looks like venus can be built-in, so how about the case when venus is
> built-in and the interconnect provider is a module? Maybe add a dependency in
> Kconfig to depend on INTERCONNECT || !INTERCONNECT?

yes, I forgot about that dependency.

> 
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
>> index 0acc7576cc58..19cbe9d5d028 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -5,6 +5,7 @@
>>   */
>>  #include <linux/clk.h>
>>  #include <linux/init.h>
>> +#include <linux/interconnect.h>
>>  #include <linux/ioctl.h>
>>  #include <linux/list.h>
>>  #include <linux/module.h>
>> @@ -239,6 +240,14 @@ static int venus_probe(struct platform_device *pdev)
>>  	if (IS_ERR(core->base))
>>  		return PTR_ERR(core->base);
>>  
>> +	core->video_path = of_icc_get(dev, "video-mem");
>> +	if (IS_ERR(core->video_path))
>> +		return PTR_ERR(core->video_path);
>> +
>> +	core->cpucfg_path = of_icc_get(dev, "cpu-cfg");
>> +	if (IS_ERR(core->cpucfg_path))
>> +		return PTR_ERR(core->cpucfg_path);
>> +
>>  	core->irq = platform_get_irq(pdev, 0);
>>  	if (core->irq < 0)
>>  		return core->irq;
>> @@ -273,6 +282,10 @@ static int venus_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
>> +	if (ret)
>> +		return ret;
>> +
>>  	ret = hfi_create(core, &venus_core_ops);
>>  	if (ret)
>>  		return ret;
>> @@ -355,6 +368,9 @@ static int venus_remove(struct platform_device *pdev)
>>  	pm_runtime_put_sync(dev);
>>  	pm_runtime_disable(dev);
>>  
>> +	icc_put(core->video_path);
>> +	icc_put(core->cpucfg_path);
>> +
> 
> Do you have any plans to scale the bandwidth on suspend/resume too?

Yes, we definitely need that in suspend/resume, but I guess the plan
should be add it once we implement pm_runtime autosuspend functionality
in order to easily test that.

-- 
regards,
Stan

Re: [PATCH 1/2] venus: use on-chip interconnect API

[Bjorn Andersson]

On Tue 20 Aug 02:34 PDT 2019, Georgi Djakov wrote:

> Hi Stan,
> 
> On 8/14/19 11:47, Stanimir Varbanov wrote:
> > This aims to add a requests for bandwidth scaling depending
> > on the resolution and framerate (macroblocks per second). The
> > exact value ff the requested bandwidth is get from a
> 
> s/ff/of/
> 
> > pre-calculated tables for encoder and decoder.
> > 
> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > ---
> >  drivers/media/platform/qcom/venus/core.c    | 34 +++++++++++
> >  drivers/media/platform/qcom/venus/core.h    | 14 +++++
> >  drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++-
> >  3 files changed, 114 insertions(+), 1 deletion(-)
> 
> It looks like venus can be built-in, so how about the case when venus is
> built-in and the interconnect provider is a module? Maybe add a dependency in
> Kconfig to depend on INTERCONNECT || !INTERCONNECT?
> 

I've been struggling down this road for remoteproc et al for a long
time, I strongly suggest that you make the INTERCONNECT config bool, to
ensure that we don't see this problem for every client.

The interconnect framework should hide the fact that the provider is
module.


But with this in place is there actually a dependency? Won't the include
file provide stubs in the case of !INTERCONNECT?

Regards,
Bjorn

> > 
> > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> > index 0acc7576cc58..19cbe9d5d028 100644
> > --- a/drivers/media/platform/qcom/venus/core.c
> > +++ b/drivers/media/platform/qcom/venus/core.c
> > @@ -5,6 +5,7 @@
> >   */
> >  #include <linux/clk.h>
> >  #include <linux/init.h>
> > +#include <linux/interconnect.h>
> >  #include <linux/ioctl.h>
> >  #include <linux/list.h>
> >  #include <linux/module.h>
> > @@ -239,6 +240,14 @@ static int venus_probe(struct platform_device *pdev)
> >  	if (IS_ERR(core->base))
> >  		return PTR_ERR(core->base);
> >  
> > +	core->video_path = of_icc_get(dev, "video-mem");
> > +	if (IS_ERR(core->video_path))
> > +		return PTR_ERR(core->video_path);
> > +
> > +	core->cpucfg_path = of_icc_get(dev, "cpu-cfg");
> > +	if (IS_ERR(core->cpucfg_path))
> > +		return PTR_ERR(core->cpucfg_path);
> > +
> >  	core->irq = platform_get_irq(pdev, 0);
> >  	if (core->irq < 0)
> >  		return core->irq;
> > @@ -273,6 +282,10 @@ static int venus_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = hfi_create(core, &venus_core_ops);
> >  	if (ret)
> >  		return ret;
> > @@ -355,6 +368,9 @@ static int venus_remove(struct platform_device *pdev)
> >  	pm_runtime_put_sync(dev);
> >  	pm_runtime_disable(dev);
> >  
> > +	icc_put(core->video_path);
> > +	icc_put(core->cpucfg_path);
> > +
> 
> Do you have any plans to scale the bandwidth on suspend/resume too?
> 
> Thanks,
> Georgi

Re: [PATCH 1/2] venus: use on-chip interconnect API

[Georgi Djakov]

Hi Bjorn,

On 9/4/19 07:22, Bjorn Andersson wrote:
> On Tue 20 Aug 02:34 PDT 2019, Georgi Djakov wrote:
> 
>> Hi Stan,
>>
>> On 8/14/19 11:47, Stanimir Varbanov wrote:
>>> This aims to add a requests for bandwidth scaling depending
>>> on the resolution and framerate (macroblocks per second). The
>>> exact value ff the requested bandwidth is get from a
>>
>> s/ff/of/
>>
>>> pre-calculated tables for encoder and decoder.
>>>
>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>> ---
>>>  drivers/media/platform/qcom/venus/core.c    | 34 +++++++++++
>>>  drivers/media/platform/qcom/venus/core.h    | 14 +++++
>>>  drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++-
>>>  3 files changed, 114 insertions(+), 1 deletion(-)
>>
>> It looks like venus can be built-in, so how about the case when venus is
>> built-in and the interconnect provider is a module? Maybe add a dependency in
>> Kconfig to depend on INTERCONNECT || !INTERCONNECT?
>>
> 
> I've been struggling down this road for remoteproc et al for a long
> time, I strongly suggest that you make the INTERCONNECT config bool, to
> ensure that we don't see this problem for every client.

Thanks for the comment. Well, i was expecting that we will have to make it bool
one day. Viresh actually already sent a patch [1]. Maybe you can add an Ack?

> 
> The interconnect framework should hide the fact that the provider is
> module.
> 

Correct.

> 
> But with this in place is there actually a dependency? Won't the include
> file provide stubs in the case of !INTERCONNECT?

There are stubs, so we are fine.

Thanks,
Georgi

[1]
https://lore.kernel.org/linux-pm/b789cce388dd1f2906492f307dea6780c398bc6a.1567065991.git.viresh.kumar@linaro.org/