[PATCH v3 00/19] Venus cleanups

[PATCH v3 00/19] Venus cleanups

[Konrad Dybcio]

With the driver supporting multiple generations of hardware, some mold
has definitely grown over the code..

This series attempts to amend this situation a bit by commonizing some
code paths and fixing some bugs while at it.

Only tested on SM8250.

Definitely needs testing on:

- SDM845 with old bindings
- SDM845 with new bindings or 7180
- MSM8916
- MSM8996

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Changes in v3:
- Drop const within const patch
- Pick up tags
- Some stylistic fixes in kerneldoc
- Link to v2: https://lore.kernel.org/r/20230911-topic-mars-v2-0-3dac84b88c4b@linaro.org

Changes in v2:
- Fix "set but unused" warning in "Drop cache properties in resource struct"
- Fix modular build with "Commonize vdec_get()"
- Rebase
- Test again on 8250, since nobody else tested other platforms since the last
  submission (or at least hasn't reported that), I'm assuming nobody cares
- Needs to be tested atop [1] and similar, it's in latest -next already
- Link to v1: https://lore.kernel.org/r/20230911-topic-mars-v1-0-a7d38bf87bdb@linaro.org

[1] https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=d2cd22c9c384aa50c0b4530e842bd078427e6279

---
Konrad Dybcio (19):
      media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable
      media: venus: pm_helpers: Rename core_clks_get to venus_clks_get
      media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
      media: venus: core: Set OPP clkname in a common code path
      media: venus: pm_helpers: Kill dead code
      media: venus: pm_helpers: Move reset acquisition to common code
      media: venus: core: Deduplicate OPP genpd names
      media: venus: core: Get rid of vcodec_num
      media: venus: core: Drop cache properties in resource struct
      media: venus: core: Use GENMASK for dma_mask
      media: venus: core: Remove cp_start
      media: venus: pm_helpers: Commonize core_power
      media: venus: pm_helpers: Remove pm_ops->core_put
      media: venus: core: Define a pointer to core->res
      media: venus: pm_helpers: Simplify vcodec clock handling
      media: venus: pm_helpers: Commonize getting clocks and GenPDs
      media: venus: pm_helpers: Commonize vdec_get()
      media: venus: pm_helpers: Commonize venc_get()
      media: venus: pm_helpers: Use reset_bulk API

 drivers/media/platform/qcom/venus/core.c       | 139 ++++-------
 drivers/media/platform/qcom/venus/core.h       |  22 +-
 drivers/media/platform/qcom/venus/firmware.c   |   3 +-
 drivers/media/platform/qcom/venus/hfi_venus.c  |  10 +-
 drivers/media/platform/qcom/venus/pm_helpers.c | 323 +++++++++----------------
 drivers/media/platform/qcom/venus/pm_helpers.h |  10 +-
 drivers/media/platform/qcom/venus/vdec.c       |   9 +-
 drivers/media/platform/qcom/venus/venc.c       |   9 +-
 8 files changed, 191 insertions(+), 334 deletions(-)
---
base-commit: 26074e1be23143b2388cacb36166766c235feb7c
change-id: 20230911-topic-mars-e60bb2269411

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH v3 01/19] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable

[Konrad Dybcio]

Commit c22b1a29497c ("media: venus: core,pm: Vote for min clk freq
during venus boot") intended to up the rate of the Venus core clock
from the XO minimum to something more reasonable, based on the per-
SoC frequency table.

Unfortunately, it ended up calling set_rate with that same argument
on all clocks in res->clks. Fix that using the OPP API.

Fixes: c22b1a29497c ("media: venus: core,pm: Vote for min clk freq during venus boot")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 502822059498..8bd0ce4ce69d 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -41,24 +41,23 @@ static int core_clks_get(struct venus_core *core)
 static int core_clks_enable(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;
-	const struct freq_tbl *freq_tbl = core->res->freq_tbl;
-	unsigned int freq_tbl_size = core->res->freq_tbl_size;
-	unsigned long freq;
+	struct dev_pm_opp *opp;
+	unsigned long freq = 0;
 	unsigned int i;
 	int ret;
 
-	if (!freq_tbl)
-		return -EINVAL;
+	if (core->has_opp_table) {
+		opp = dev_pm_opp_find_freq_ceil(core->dev, &freq);
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+		dev_pm_opp_put(opp);
 
-	freq = freq_tbl[freq_tbl_size - 1].freq;
+		ret = dev_pm_opp_set_rate(core->dev, freq);
+		if (ret)
+			return ret;
+	}
 
 	for (i = 0; i < res->clks_num; i++) {
-		if (IS_V6(core)) {
-			ret = clk_set_rate(core->clks[i], freq);
-			if (ret)
-				goto err;
-		}
-
 		ret = clk_prepare_enable(core->clks[i]);
 		if (ret)
 			goto err;

-- 
2.44.0

Re: [PATCH v3 01/19] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> Commit c22b1a29497c ("media: venus: core,pm: Vote for min clk freq
> during venus boot") intended to up the rate of the Venus core clock
> from the XO minimum to something more reasonable, based on the per-
> SoC frequency table.
> 
> Unfortunately, it ended up calling set_rate with that same argument
> on all clocks in res->clks. Fix that using the OPP API.
> 
> Fixes: c22b1a29497c ("media: venus: core,pm: Vote for min clk freq during venus boot")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 502822059498..8bd0ce4ce69d 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -41,24 +41,23 @@ static int core_clks_get(struct venus_core *core)
>  static int core_clks_enable(struct venus_core *core)
>  {
>  	const struct venus_resources *res = core->res;
> -	const struct freq_tbl *freq_tbl = core->res->freq_tbl;
> -	unsigned int freq_tbl_size = core->res->freq_tbl_size;
> -	unsigned long freq;
> +	struct dev_pm_opp *opp;
> +	unsigned long freq = 0;
>  	unsigned int i;
>  	int ret;
>  
> -	if (!freq_tbl)
> -		return -EINVAL;
> +	if (core->has_opp_table) {
> +		opp = dev_pm_opp_find_freq_ceil(core->dev, &freq);
> +		if (IS_ERR(opp))
> +			return PTR_ERR(opp);
> +		dev_pm_opp_put(opp);
>  
> -	freq = freq_tbl[freq_tbl_size - 1].freq;
> +		ret = dev_pm_opp_set_rate(core->dev, freq);
> +		if (ret)
> +			return ret;
> +	}
Earlier clk_set_rate is called for only V6 target, this change is calling
it unconditionally. Opp table is available for v4 target as well.
>  
>  	for (i = 0; i < res->clks_num; i++) {
> -		if (IS_V6(core)) {
> -			ret = clk_set_rate(core->clks[i], freq);
> -			if (ret)
> -				goto err;
> -		}
> -
>  		ret = clk_prepare_enable(core->clks[i]);
>  		if (ret)
>  			goto err;
> 

Thanks,
Dikshita

Re: [PATCH v3 01/19] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable

[Vikash Garodia]

Hi Konrad,

On 4/5/2024 1:01 PM, Dikshita Agarwal wrote:
> 
> 
> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>> Commit c22b1a29497c ("media: venus: core,pm: Vote for min clk freq
>> during venus boot") intended to up the rate of the Venus core clock
>> from the XO minimum to something more reasonable, based on the per-
>> SoC frequency table.
>>
>> Unfortunately, it ended up calling set_rate with that same argument
>> on all clocks in res->clks. Fix that using the OPP API.
It called with same argument, but not with same frequency. The argument is
platform specific and would have different values. Do not see it fixing anything
in existing code, so "Fixes" does not look applicable here. OR, am i  missing
something ?

Thanks,
Vikash
>>
>> Fixes: c22b1a29497c ("media: venus: core,pm: Vote for min clk freq during venus boot")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++------------
>>  1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index 502822059498..8bd0ce4ce69d 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -41,24 +41,23 @@ static int core_clks_get(struct venus_core *core)
>>  static int core_clks_enable(struct venus_core *core)
>>  {
>>  	const struct venus_resources *res = core->res;
>> -	const struct freq_tbl *freq_tbl = core->res->freq_tbl;
>> -	unsigned int freq_tbl_size = core->res->freq_tbl_size;
>> -	unsigned long freq;
>> +	struct dev_pm_opp *opp;
>> +	unsigned long freq = 0;
>>  	unsigned int i;
>>  	int ret;
>>  
>> -	if (!freq_tbl)
>> -		return -EINVAL;
>> +	if (core->has_opp_table) {
>> +		opp = dev_pm_opp_find_freq_ceil(core->dev, &freq);
>> +		if (IS_ERR(opp))
>> +			return PTR_ERR(opp);
>> +		dev_pm_opp_put(opp);
>>  
>> -	freq = freq_tbl[freq_tbl_size - 1].freq;
>> +		ret = dev_pm_opp_set_rate(core->dev, freq);
>> +		if (ret)
>> +			return ret;
>> +	}
> Earlier clk_set_rate is called for only V6 target, this change is calling
> it unconditionally. Opp table is available for v4 target as well.
>>  
>>  	for (i = 0; i < res->clks_num; i++) {
>> -		if (IS_V6(core)) {
>> -			ret = clk_set_rate(core->clks[i], freq);
>> -			if (ret)
>> -				goto err;
>> -		}
>> -
>>  		ret = clk_prepare_enable(core->clks[i]);
>>  		if (ret)
>>  			goto err;
>>
> 
> Thanks,
> Dikshita

[PATCH v3 02/19] media: venus: pm_helpers: Rename core_clks_get to venus_clks_get

[Konrad Dybcio]

"core" is used in multiple contexts when talking about Venus, rename
the function to save on confusion.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 8bd0ce4ce69d..ac7c83404c6e 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -23,7 +23,7 @@
 
 static bool legacy_binding;
 
-static int core_clks_get(struct venus_core *core)
+static int venus_clks_get(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;
 	struct device *dev = core->dev;
@@ -294,7 +294,7 @@ static int core_get_v1(struct venus_core *core)
 {
 	int ret;
 
-	ret = core_clks_get(core);
+	ret = venus_clks_get(core);
 	if (ret)
 		return ret;
 
@@ -961,7 +961,7 @@ static int core_get_v4(struct venus_core *core)
 	const struct venus_resources *res = core->res;
 	int ret;
 
-	ret = core_clks_get(core);
+	ret = venus_clks_get(core);
 	if (ret)
 		return ret;
 

-- 
2.44.0

Re: [PATCH v3 02/19] media: venus: pm_helpers: Rename core_clks_get to venus_clks_get

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> "core" is used in multiple contexts when talking about Venus, rename
> the function to save on confusion.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 8bd0ce4ce69d..ac7c83404c6e 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -23,7 +23,7 @@
>  
>  static bool legacy_binding;
>  
> -static int core_clks_get(struct venus_core *core)
> +static int venus_clks_get(struct venus_core *core)
>  {
>  	const struct venus_resources *res = core->res;
>  	struct device *dev = core->dev;
> @@ -294,7 +294,7 @@ static int core_get_v1(struct venus_core *core)
>  {
>  	int ret;
>  
> -	ret = core_clks_get(core);
> +	ret = venus_clks_get(core);
>  	if (ret)
>  		return ret;
>  
> @@ -961,7 +961,7 @@ static int core_get_v4(struct venus_core *core)
>  	const struct venus_resources *res = core->res;
>  	int ret;
>  
> -	ret = core_clks_get(core);
> +	ret = venus_clks_get(core);
>  	if (ret)
>  		return ret;
>  
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 03/19] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()

[Konrad Dybcio]

To make it easier to understand the various clock requirements within
this driver, add kerneldoc to venus_clk_get() explaining the fluff.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ac7c83404c6e..cf91f50a33aa 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -23,6 +23,34 @@
 
 static bool legacy_binding;
 
+/**
+ * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
+ * @core: A pointer to the venus core resource
+ *
+ * The Venus block (depending on the generation) can be split into a couple
+ * of clock domains: one for main logic and one for each video core (0-2 instances).
+ *
+ * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
+ * domain, so this function is the only kind if clk_get necessary there.
+ *
+ * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
+ * statically defined a decoder and core1 an encoder, with both having
+ * their own clock domains.
+ *
+ * SDM845 features two video cores, each one of which may or may not be
+ * subdivided into two encoder/decoder threads.
+ *
+ * Other SoCs either feature a single video core (with its own clock domain)
+ * or one video core and one CVP (Computer Vision Processor) core. In both cases
+ * we treat it the same way (CVP only happens to live near-by Venus on the SoC).
+ *
+ * Due to unfortunate developments in the past, we need to support legacy
+ * bindings (MSM8996, SDM660, SDM845) that require specifying the clocks and
+ * power-domains associated with a video core domain in a bogus sub-node,
+ * which means that additional fluff is necessary..
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
 static int venus_clks_get(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;

-- 
2.44.0

Re: [PATCH v3 03/19] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> To make it easier to understand the various clock requirements within
> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index ac7c83404c6e..cf91f50a33aa 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -23,6 +23,34 @@
>  
>  static bool legacy_binding;
>  
> +/**
> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
> + * @core: A pointer to the venus core resource
> + *
> + * The Venus block (depending on the generation) can be split into a couple
> + * of clock domains: one for main logic and one for each video core (0-2 instances).
> + *
> + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
> + * domain, so this function is the only kind if clk_get necessary there.
> + *
> + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
> + * statically defined a decoder and core1 an encoder, with both having
> + * their own clock domains.
> + *
> + * SDM845 features two video cores, each one of which may or may not be
> + * subdivided into two encoder/decoder threads.
> + *
> + * Other SoCs either feature a single video core (with its own clock domain)
> + * or one video core and one CVP (Computer Vision Processor) core. In both cases
> + * we treat it the same way (CVP only happens to live near-by Venus on the SoC).
> + *
> + * Due to unfortunate developments in the past, we need to support legacy
why unfortunate? please re-phrase this.
> + * bindings (MSM8996, SDM660, SDM845) that require specifying the clocks and
> + * power-domains associated with a video core domain in a bogus sub-node,
> + * which means that additional fluff is necessary..
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
>  static int venus_clks_get(struct venus_core *core)
>  {
>  	const struct venus_resources *res = core->res;
> 

Thanks,
Dikshita

Re: [PATCH v3 03/19] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()

[Vikash Garodia]

Hi Konrad,

On 4/5/2024 1:56 PM, Dikshita Agarwal wrote:
> 
> 
> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>> To make it easier to understand the various clock requirements within
>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index ac7c83404c6e..cf91f50a33aa 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -23,6 +23,34 @@
>>  
>>  static bool legacy_binding;
>>  
>> +/**
>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>> + * @core: A pointer to the venus core resource
>> + *
>> + * The Venus block (depending on the generation) can be split into a couple
>> + * of clock domains: one for main logic and one for each video core (0-2 instances).
s/main logic/controller. Applies to below places as well.

>> + *
>> + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
>> + * domain, so this function is the only kind if clk_get necessary there.
To be checked, unable to get the clock document to see why only core clock
(VENUS0_VCODEC0_CLK). Will update.

>> + *
>> + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
>> + * statically defined a decoder and core1 an encoder, with both having
>> + * their own clock domains.
>> + *
>> + * SDM845 features two video cores, each one of which may or may not be
s/two video cores/two identical video cores
>> + * subdivided into two encoder/decoder threads.
decoder cannot be split into core threads. you can keep it like "each of which
is capable to do any encode or decode"

>> + *
>> + * Other SoCs either feature a single video core (with its own clock domain)
>> + * or one video core and one CVP (Computer Vision Processor) core. In both cases
>> + * we treat it the same way (CVP only happens to live near-by Venus on the SoC).
>> + *
>> + * Due to unfortunate developments in the past, we need to support legacy
> why unfortunate? please re-phrase this.
>> + * bindings (MSM8996, SDM660, SDM845) that require specifying the clocks and
>> + * power-domains associated with a video core domain in a bogus sub-node,
>> + * which means that additional fluff is necessary.
Some background:
It was done that way to support decoder core with specific clocks and similarly
for encoder. Earlier architectures use to have different clock source for these
specific decoder/encoder core clocks, now there is a common clock source for
both the cores. Hence if any one is enabled, others gets enabled as it is
derived from same source.
So if we see the later bindings, the clocks were moved out of sub node to main
venus node.

Regards,
Vikash
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>>  static int venus_clks_get(struct venus_core *core)
>>  {
>>  	const struct venus_resources *res = core->res;
>>
> 
> Thanks,
> Dikshita

Re: [PATCH v3 03/19] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()

[Konrad Dybcio]

On 4/5/24 14:44, Vikash Garodia wrote:
> Hi Konrad,
> 
> On 4/5/2024 1:56 PM, Dikshita Agarwal wrote:
>>
>>
>> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>>> To make it easier to understand the various clock requirements within
>>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>>
>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>   drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> index ac7c83404c6e..cf91f50a33aa 100644
>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> @@ -23,6 +23,34 @@
>>>   
>>>   static bool legacy_binding;
>>>   
>>> +/**
>>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>>> + * @core: A pointer to the venus core resource
>>> + *
>>> + * The Venus block (depending on the generation) can be split into a couple
>>> + * of clock domains: one for main logic and one for each video core (0-2 instances).
> s/main logic/controller. Applies to below places as well.

Ok - so "controller" is the cortex-m3 (m5?) that power-sequences
the DSP etc.?

> 
>>> + *
>>> + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
>>> + * domain, so this function is the only kind if clk_get necessary there.
> To be checked, unable to get the clock document to see why only core clocks 
> (VENUS0_VCODEC0_CLK). Will update.

FWIW 8916 only has GCC_VENUS0_VCODEC0_CLK (and _SRC) and AHB/AXI/TBU clocks,
no (currently registered) clock for the entire block.

> 
>>> + *
>>> + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
>>> + * statically defined a decoder and core1 an encoder, with both having
>>> + * their own clock domains.
>>> + *
>>> + * SDM845 features two video cores, each one of which may or may not be
> s/two video cores/two identical video cores
>>> + * subdivided into two encoder/decoder threads.
> decoder cannot be split into core threads. you can keep it like "each of which
> is capable to do any encode or decode"

So it's not about any splitting, but rather the ability to do both encode
and decode, sort of like how the displayport controller can nowadays do both
eDP and DP, depending on what init data you send to it?

> 
>>> + *
>>> + * Other SoCs either feature a single video core (with its own clock domain)
>>> + * or one video core and one CVP (Computer Vision Processor) core. In both cases
>>> + * we treat it the same way (CVP only happens to live near-by Venus on the SoC).
>>> + *
>>> + * Due to unfortunate developments in the past, we need to support legacy
>> why unfortunate? please re-phrase this.
>>> + * bindings (MSM8996, SDM660, SDM845) that require specifying the clocks and
>>> + * power-domains associated with a video core domain in a bogus sub-node,
>>> + * which means that additional fluff is necessary.
> Some background:
> It was done that way to support decoder core with specific clocks and similarly
> for encoder. Earlier architectures use to have different clock source for these
> specific decoder/encoder core clocks, now there is a common clock source for
> both the cores. Hence if any one is enabled, others gets enabled as it is
> derived from same source.
> So if we see the later bindings, the clocks were moved out of sub node to main
> venus node.

Yeah and I don't really see the reason why the binding needed to be changed
for this, you can simply get the clocks by name and poke at the specific clk*
(or an array of them), no matter where they were _get()-ed from.

Konrad

Re: [PATCH v3 03/19] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()

[Vikash Garodia]

On 4/9/2024 11:52 PM, Konrad Dybcio wrote:
> 
> 
> On 4/5/24 14:44, Vikash Garodia wrote:
>> Hi Konrad,
>>
>> On 4/5/2024 1:56 PM, Dikshita Agarwal wrote:
>>>
>>>
>>> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>>>> To make it easier to understand the various clock requirements within
>>>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>>>
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>   drivers/media/platform/qcom/venus/pm_helpers.c | 28
>>>> ++++++++++++++++++++++++++
>>>>   1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> b/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> index ac7c83404c6e..cf91f50a33aa 100644
>>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> @@ -23,6 +23,34 @@
>>>>     static bool legacy_binding;
>>>>   +/**
>>>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>>>> + * @core: A pointer to the venus core resource
>>>> + *
>>>> + * The Venus block (depending on the generation) can be split into a couple
>>>> + * of clock domains: one for main logic and one for each video core (0-2
>>>> instances).
>> s/main logic/controller. Applies to below places as well.
> 
> Ok - so "controller" is the cortex-m3 (m5?) that power-sequences
> the DSP etc.?
Thats correct. The firmware runs on the controller and takes care of many
aspects of hardware (dsp or core) programming.

>>
>>>> + *
>>>> + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
>>>> + * domain, so this function is the only kind if clk_get necessary there.
>> To be checked, unable to get the clock document to see why only core clocks
>> (VENUS0_VCODEC0_CLK). Will update.
> 
> FWIW 8916 only has GCC_VENUS0_VCODEC0_CLK (and _SRC) and AHB/AXI/TBU clocks,
> no (currently registered) clock for the entire block.
> 
>>
>>>> + *
>>>> + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
>>>> + * statically defined a decoder and core1 an encoder, with both having
>>>> + * their own clock domains.
>>>> + *
>>>> + * SDM845 features two video cores, each one of which may or may not be
>> s/two video cores/two identical video cores
>>>> + * subdivided into two encoder/decoder threads.
>> decoder cannot be split into core threads. you can keep it like "each of which
>> is capable to do any encode or decode"
> 
> So it's not about any splitting, but rather the ability to do both encode
> and decode, sort of like how the displayport controller can nowadays do both
> eDP and DP, depending on what init data you send to it?
It is precisely that way, just that there are cases of cores with dedicated
codec support, hence identical implies that each of them can do same processing.

>>
>>>> + *
>>>> + * Other SoCs either feature a single video core (with its own clock domain)
>>>> + * or one video core and one CVP (Computer Vision Processor) core. In both
>>>> cases
>>>> + * we treat it the same way (CVP only happens to live near-by Venus on the
>>>> SoC).
>>>> + *
>>>> + * Due to unfortunate developments in the past, we need to support legacy
>>> why unfortunate? please re-phrase this.
>>>> + * bindings (MSM8996, SDM660, SDM845) that require specifying the clocks and
>>>> + * power-domains associated with a video core domain in a bogus sub-node,
>>>> + * which means that additional fluff is necessary.
>> Some background:
>> It was done that way to support decoder core with specific clocks and similarly
>> for encoder. Earlier architectures use to have different clock source for these
>> specific decoder/encoder core clocks, now there is a common clock source for
>> both the cores. Hence if any one is enabled, others gets enabled as it is
>> derived from same source.
>> So if we see the later bindings, the clocks were moved out of sub node to main
>> venus node.
> 
> Yeah and I don't really see the reason why the binding needed to be changed
> for this, you can simply get the clocks by name and poke at the specific clk*
> (or an array of them), no matter where they were _get()-ed from.
I think the reason for not doing a name based clock as it might be possible that
the clock is not available or applicable to subsequent SOC.

Regards,
Vikash

Re: [PATCH v3 03/19] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()

[Konrad Dybcio]

On 4/5/24 10:26, Dikshita Agarwal wrote:
> 
> 
> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>> To make it easier to understand the various clock requirements within
>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index ac7c83404c6e..cf91f50a33aa 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -23,6 +23,34 @@
>>   
>>   static bool legacy_binding;
>>   
>> +/**
>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>> + * @core: A pointer to the venus core resource
>> + *
>> + * The Venus block (depending on the generation) can be split into a couple
>> + * of clock domains: one for main logic and one for each video core (0-2 instances).
>> + *
>> + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
>> + * domain, so this function is the only kind if clk_get necessary there.
>> + *
>> + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
>> + * statically defined a decoder and core1 an encoder, with both having
>> + * their own clock domains.
>> + *
>> + * SDM845 features two video cores, each one of which may or may not be
>> + * subdivided into two encoder/decoder threads.
>> + *
>> + * Other SoCs either feature a single video core (with its own clock domain)
>> + * or one video core and one CVP (Computer Vision Processor) core. In both cases
>> + * we treat it the same way (CVP only happens to live near-by Venus on the SoC).
>> + *
>> + * Due to unfortunate developments in the past, we need to support legacy
> why unfortunate? please re-phrase this.

It's unfortunate because another binding has been created to
represent the same hardware to solve a "problem" that could have
been dealt with using a couple lines of C and that we now need to
carry support for.

Konrad

[PATCH v3 04/19] media: venus: core: Set OPP clkname in a common code path

[Konrad Dybcio]

Calling devm_pm_opp_set_clkname() is repeated for all HFI versions in
pm_ops->core_power.

Move it to the common codepath.

This also lets us get rid of core_get_v1.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       |  5 +++++
 drivers/media/platform/qcom/venus/pm_helpers.c | 23 ++---------------------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index ce206b709754..5ab3c414ec0f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -14,6 +14,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/pm_domain.h>
@@ -319,6 +320,10 @@ static int venus_probe(struct platform_device *pdev)
 	if (!core->pm_ops)
 		return -ENODEV;
 
+	ret = devm_pm_opp_set_clkname(dev, "core");
+	if (ret)
+		return ret;
+
 	if (core->pm_ops->core_get) {
 		ret = core->pm_ops->core_get(core);
 		if (ret)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index cf91f50a33aa..ef4b0f0da36f 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -318,21 +318,6 @@ static int load_scale_v1(struct venus_inst *inst)
 	return ret;
 }
 
-static int core_get_v1(struct venus_core *core)
-{
-	int ret;
-
-	ret = venus_clks_get(core);
-	if (ret)
-		return ret;
-
-	ret = devm_pm_opp_set_clkname(core->dev, "core");
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
 static void core_put_v1(struct venus_core *core)
 {
 }
@@ -350,7 +335,7 @@ static int core_power_v1(struct venus_core *core, int on)
 }
 
 static const struct venus_pm_ops pm_ops_v1 = {
-	.core_get = core_get_v1,
+	.core_get = venus_clks_get,
 	.core_put = core_put_v1,
 	.core_power = core_power_v1,
 	.load_scale = load_scale_v1,
@@ -423,7 +408,7 @@ static int venc_power_v3(struct device *dev, int on)
 }
 
 static const struct venus_pm_ops pm_ops_v3 = {
-	.core_get = core_get_v1,
+	.core_get = venus_clks_get,
 	.core_put = core_put_v1,
 	.core_power = core_power_v1,
 	.vdec_get = vdec_get_v3,
@@ -1013,10 +998,6 @@ static int core_get_v4(struct venus_core *core)
 	if (legacy_binding)
 		return 0;
 
-	ret = devm_pm_opp_set_clkname(dev, "core");
-	if (ret)
-		return ret;
-
 	ret = vcodec_domains_get(core);
 	if (ret)
 		return ret;

-- 
2.44.0

Re: [PATCH v3 04/19] media: venus: core: Set OPP clkname in a common code path

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> Calling devm_pm_opp_set_clkname() is repeated for all HFI versions in
> pm_ops->core_power.
> 
> Move it to the common codepath.
> 
> This also lets us get rid of core_get_v1.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c       |  5 +++++
>  drivers/media/platform/qcom/venus/pm_helpers.c | 23 ++---------------------
>  2 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index ce206b709754..5ab3c414ec0f 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -14,6 +14,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/pm_domain.h>
> @@ -319,6 +320,10 @@ static int venus_probe(struct platform_device *pdev)
>  	if (!core->pm_ops)
>  		return -ENODEV;
>  
> +	ret = devm_pm_opp_set_clkname(dev, "core");
> +	if (ret)
> +		return ret;
> +
>  	if (core->pm_ops->core_get) {
>  		ret = core->pm_ops->core_get(core);
>  		if (ret)
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index cf91f50a33aa..ef4b0f0da36f 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -318,21 +318,6 @@ static int load_scale_v1(struct venus_inst *inst)
>  	return ret;
>  }
>  
> -static int core_get_v1(struct venus_core *core)
> -{
> -	int ret;
> -
> -	ret = venus_clks_get(core);
> -	if (ret)
> -		return ret;
> -
> -	ret = devm_pm_opp_set_clkname(core->dev, "core");
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> -}
> -
>  static void core_put_v1(struct venus_core *core)
>  {
>  }
> @@ -350,7 +335,7 @@ static int core_power_v1(struct venus_core *core, int on)
>  }
>  
>  static const struct venus_pm_ops pm_ops_v1 = {
> -	.core_get = core_get_v1,
> +	.core_get = venus_clks_get,
>  	.core_put = core_put_v1,
>  	.core_power = core_power_v1,
>  	.load_scale = load_scale_v1,
> @@ -423,7 +408,7 @@ static int venc_power_v3(struct device *dev, int on)
>  }
>  
>  static const struct venus_pm_ops pm_ops_v3 = {
> -	.core_get = core_get_v1,
> +	.core_get = venus_clks_get,
>  	.core_put = core_put_v1,
>  	.core_power = core_power_v1,
>  	.vdec_get = vdec_get_v3,
> @@ -1013,10 +998,6 @@ static int core_get_v4(struct venus_core *core)
>  	if (legacy_binding)
>  		return 0;
>  
> -	ret = devm_pm_opp_set_clkname(dev, "core");
> -	if (ret)
> -		return ret;
> -
>  	ret = vcodec_domains_get(core);
>  	if (ret)
>  		return ret;
> 
>
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 05/19] media: venus: pm_helpers: Kill dead code

[Konrad Dybcio]

A situation like:

if (!foo)
	goto bar;

for (i = 0; i < foo; i++)
	...1...

bar:
	...2...

is totally identical to:

for (i = 0; i < 0; i++) // === if (0)
	...1...

...2...

Get rid of such boilerplate.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ef4b0f0da36f..730c4b593cec 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -878,14 +878,10 @@ static int vcodec_domains_get(struct venus_core *core)
 		.pd_flags = PD_FLAG_NO_DEV_LINK,
 	};
 
-	if (!res->vcodec_pmdomains_num)
-		goto skip_pmdomains;
-
 	ret = dev_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
 	if (ret < 0)
 		return ret;
 
-skip_pmdomains:
 	if (!core->res->opp_pmdomain)
 		return 0;
 
@@ -928,9 +924,6 @@ static int core_resets_reset(struct venus_core *core)
 	unsigned int i;
 	int ret;
 
-	if (!res->resets_num)
-		return 0;
-
 	for (i = 0; i < res->resets_num; i++) {
 		ret = reset_control_assert(core->resets[i]);
 		if (ret)
@@ -953,9 +946,6 @@ static int core_resets_get(struct venus_core *core)
 	unsigned int i;
 	int ret;
 
-	if (!res->resets_num)
-		return 0;
-
 	for (i = 0; i < res->resets_num; i++) {
 		core->resets[i] =
 			devm_reset_control_get_exclusive(dev, res->resets[i]);

-- 
2.44.0

Re: [PATCH v3 05/19] media: venus: pm_helpers: Kill dead code

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> A situation like:
> 
> if (!foo)
> 	goto bar;
> 
> for (i = 0; i < foo; i++)
> 	...1...
> 
> bar:
> 	...2...
> 
> is totally identical to:
> 
> for (i = 0; i < 0; i++) // === if (0)
> 	...1...
> 
> ...2...
> 
> Get rid of such boilerplate.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index ef4b0f0da36f..730c4b593cec 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -878,14 +878,10 @@ static int vcodec_domains_get(struct venus_core *core)
>  		.pd_flags = PD_FLAG_NO_DEV_LINK,
>  	};
>  
> -	if (!res->vcodec_pmdomains_num)
> -		goto skip_pmdomains;
> -
this condition should still be there to skip calling
dev_pm_domain_attach_list if vcodec_pmdomains_num is 0.
>  	ret = dev_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
>  	if (ret < 0)
>  		return ret;
>  
> -skip_pmdomains:
>  	if (!core->res->opp_pmdomain)
>  		return 0;
>  
> @@ -928,9 +924,6 @@ static int core_resets_reset(struct venus_core *core)
>  	unsigned int i;
>  	int ret;
>  
> -	if (!res->resets_num)
> -		return 0;
> -
ACK
>  	for (i = 0; i < res->resets_num; i++) {
>  		ret = reset_control_assert(core->resets[i]);
>  		if (ret)
> @@ -953,9 +946,6 @@ static int core_resets_get(struct venus_core *core)
>  	unsigned int i;
>  	int ret;
>  
> -	if (!res->resets_num)
> -		return 0;
> -
ACK
>  	for (i = 0; i < res->resets_num; i++) {
>  		core->resets[i] =
>  			devm_reset_control_get_exclusive(dev, res->resets[i]);
> 

Thanks,
Dikshita

Re: [PATCH v3 05/19] media: venus: pm_helpers: Kill dead code

[Konrad Dybcio]

On 4/5/24 09:49, Dikshita Agarwal wrote:
> 
> 
> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>> A situation like:
>>
>> if (!foo)
>> 	goto bar;
>>
>> for (i = 0; i < foo; i++)
>> 	...1...
>>
>> bar:
>> 	...2...
>>
>> is totally identical to:
>>
>> for (i = 0; i < 0; i++) // === if (0)
>> 	...1...
>>
>> ...2...
>>
>> Get rid of such boilerplate.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 10 ----------
>>   1 file changed, 10 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index ef4b0f0da36f..730c4b593cec 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -878,14 +878,10 @@ static int vcodec_domains_get(struct venus_core *core)
>>   		.pd_flags = PD_FLAG_NO_DEV_LINK,
>>   	};
>>   
>> -	if (!res->vcodec_pmdomains_num)
>> -		goto skip_pmdomains;
>> -
> this condition should still be there to skip calling
> dev_pm_domain_attach_list if vcodec_pmdomains_num is 0.

That should be totally fine, within that function there's this bit

if (num_pds <= 0)
	return 0;

which bails out gracefully

Konrad

Re: [PATCH v3 05/19] media: venus: pm_helpers: Kill dead code

[Dikshita Agarwal]

On 4/9/2024 11:54 PM, Konrad Dybcio wrote:
> 
> 
> On 4/5/24 09:49, Dikshita Agarwal wrote:
>>
>>
>> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>>> A situation like:
>>>
>>> if (!foo)
>>>     goto bar;
>>>
>>> for (i = 0; i < foo; i++)
>>>     ...1...
>>>
>>> bar:
>>>     ...2...
>>>
>>> is totally identical to:
>>>
>>> for (i = 0; i < 0; i++) // === if (0)
>>>     ...1...
>>>
>>> ...2...
>>>
>>> Get rid of such boilerplate.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>   drivers/media/platform/qcom/venus/pm_helpers.c | 10 ----------
>>>   1 file changed, 10 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c
>>> b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> index ef4b0f0da36f..730c4b593cec 100644
>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> @@ -878,14 +878,10 @@ static int vcodec_domains_get(struct venus_core
>>> *core)
>>>           .pd_flags = PD_FLAG_NO_DEV_LINK,
>>>       };
>>>   -    if (!res->vcodec_pmdomains_num)
>>> -        goto skip_pmdomains;
>>> -
>> this condition should still be there to skip calling
>> dev_pm_domain_attach_list if vcodec_pmdomains_num is 0.
> 
> That should be totally fine, within that function there's this bit
> 
> if (num_pds <= 0)
>     return 0;
> 
> which bails out gracefully
> 
AFAIU, this condition won't be true eg: for 8916 and 8996.
because in the else condition[1], num_pds will be non-zero, as there is one
entry for power-domains in dtsi file for 8916, 8996 as well.

[1]https://elixir.bootlin.com/linux/v6.9-rc5/source/drivers/base/power/common.c#L213
Am I missing something here?

> Konrad

[PATCH v3 06/19] media: venus: pm_helpers: Move reset acquisition to common code

[Konrad Dybcio]

There is no reason to keep reset_get code local to HFIv4/v6.

Move it to the common part.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       |  9 ++++++++-
 drivers/media/platform/qcom/venus/pm_helpers.c | 23 -----------------------
 2 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 5ab3c414ec0f..0652065cb113 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -15,6 +15,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/pm_domain.h>
@@ -286,7 +287,7 @@ static int venus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct venus_core *core;
-	int ret;
+	int i, ret;
 
 	core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
 	if (!core)
@@ -324,6 +325,12 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	for (i = 0; i < core->res->resets_num; i++) {
+		core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
+		if (IS_ERR(core->resets[i]))
+			return PTR_ERR(core->resets[i]);
+	}
+
 	if (core->pm_ops->core_get) {
 		ret = core->pm_ops->core_get(core);
 		if (ret)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 730c4b593cec..5b2a40a2f524 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -939,25 +939,6 @@ static int core_resets_reset(struct venus_core *core)
 	return ret;
 }
 
-static int core_resets_get(struct venus_core *core)
-{
-	struct device *dev = core->dev;
-	const struct venus_resources *res = core->res;
-	unsigned int i;
-	int ret;
-
-	for (i = 0; i < res->resets_num; i++) {
-		core->resets[i] =
-			devm_reset_control_get_exclusive(dev, res->resets[i]);
-		if (IS_ERR(core->resets[i])) {
-			ret = PTR_ERR(core->resets[i]);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static int core_get_v4(struct venus_core *core)
 {
 	struct device *dev = core->dev;
@@ -981,10 +962,6 @@ static int core_get_v4(struct venus_core *core)
 	if (ret)
 		return ret;
 
-	ret = core_resets_get(core);
-	if (ret)
-		return ret;
-
 	if (legacy_binding)
 		return 0;
 

-- 
2.44.0

Re: [PATCH v3 06/19] media: venus: pm_helpers: Move reset acquisition to common code

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> There is no reason to keep reset_get code local to HFIv4/v6.
> 
> Move it to the common part.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c       |  9 ++++++++-
>  drivers/media/platform/qcom/venus/pm_helpers.c | 23 -----------------------
>  2 files changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 5ab3c414ec0f..0652065cb113 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -15,6 +15,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_opp.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/pm_domain.h>
> @@ -286,7 +287,7 @@ static int venus_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct venus_core *core;
> -	int ret;
> +	int i, ret;
>  
>  	core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
>  	if (!core)
> @@ -324,6 +325,12 @@ static int venus_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	for (i = 0; i < core->res->resets_num; i++) {
> +		core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
> +		if (IS_ERR(core->resets[i]))
> +			return PTR_ERR(core->resets[i]);
> +	}
> +
>  	if (core->pm_ops->core_get) {
>  		ret = core->pm_ops->core_get(core);
>  		if (ret)
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 730c4b593cec..5b2a40a2f524 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -939,25 +939,6 @@ static int core_resets_reset(struct venus_core *core)
>  	return ret;
>  }
>  
> -static int core_resets_get(struct venus_core *core)
> -{
> -	struct device *dev = core->dev;
> -	const struct venus_resources *res = core->res;
> -	unsigned int i;
> -	int ret;
> -
> -	for (i = 0; i < res->resets_num; i++) {
> -		core->resets[i] =
> -			devm_reset_control_get_exclusive(dev, res->resets[i]);
> -		if (IS_ERR(core->resets[i])) {
> -			ret = PTR_ERR(core->resets[i]);
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static int core_get_v4(struct venus_core *core)
>  {
>  	struct device *dev = core->dev;
> @@ -981,10 +962,6 @@ static int core_get_v4(struct venus_core *core)
>  	if (ret)
>  		return ret;
>  
> -	ret = core_resets_get(core);
> -	if (ret)
> -		return ret;
> -
>  	if (legacy_binding)
>  		return 0;
>  
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 07/19] media: venus: core: Deduplicate OPP genpd names

[Konrad Dybcio]

Instead of redefining the same literals over and over again, define
them once and point the reference to that definition.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 0652065cb113..5e7cb54e6088 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -538,6 +538,9 @@ static const struct dev_pm_ops venus_pm_ops = {
 	SET_RUNTIME_PM_OPS(venus_runtime_suspend, venus_runtime_resume, NULL)
 };
 
+static const char *pd_names_cx[] = { "cx", NULL };
+static const char *pd_names_mx[] = { "mx", NULL };
+
 static const struct freq_tbl msm8916_freq_table[] = {
 	{ 352800, 228570000 },	/* 1920x1088 @ 30 + 1280x720 @ 30 */
 	{ 244800, 160000000 },	/* 1920x1088 @ 30 */
@@ -721,7 +724,7 @@ static const struct venus_resources sdm845_res_v2 = {
 	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
 	.vcodec_pmdomains_num = 3,
-	.opp_pmdomain = (const char *[]) { "cx", NULL },
+	.opp_pmdomain = pd_names_cx,
 	.vcodec_num = 2,
 	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_4XX,
@@ -770,7 +773,7 @@ static const struct venus_resources sc7180_res = {
 	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
-	.opp_pmdomain = (const char *[]) { "cx", NULL },
+	.opp_pmdomain = pd_names_cx,
 	.vcodec_num = 1,
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
@@ -827,7 +830,7 @@ static const struct venus_resources sm8250_res = {
 	.vcodec_clks_num = 1,
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
-	.opp_pmdomain = (const char *[]) { "mx", NULL },
+	.opp_pmdomain = pd_names_mx,
 	.vcodec_num = 1,
 	.max_load = 7833600,
 	.hfi_version = HFI_VERSION_6XX,
@@ -886,7 +889,7 @@ static const struct venus_resources sc7280_res = {
 	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
-	.opp_pmdomain = (const char *[]) { "cx", NULL },
+	.opp_pmdomain = pd_names_cx,
 	.vcodec_num = 1,
 	.hfi_version = HFI_VERSION_6XX,
 	.vpu_version = VPU_VERSION_IRIS2_1,

-- 
2.44.0

Re: [PATCH v3 07/19] media: venus: core: Deduplicate OPP genpd names

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> Instead of redefining the same literals over and over again, define
> them once and point the reference to that definition.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 0652065cb113..5e7cb54e6088 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -538,6 +538,9 @@ static const struct dev_pm_ops venus_pm_ops = {
>  	SET_RUNTIME_PM_OPS(venus_runtime_suspend, venus_runtime_resume, NULL)
>  };
>  
> +static const char *pd_names_cx[] = { "cx", NULL };
> +static const char *pd_names_mx[] = { "mx", NULL };
> +
>  static const struct freq_tbl msm8916_freq_table[] = {
>  	{ 352800, 228570000 },	/* 1920x1088 @ 30 + 1280x720 @ 30 */
>  	{ 244800, 160000000 },	/* 1920x1088 @ 30 */
> @@ -721,7 +724,7 @@ static const struct venus_resources sdm845_res_v2 = {
>  	.vcodec_clks_num = 2,
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
>  	.vcodec_pmdomains_num = 3,
> -	.opp_pmdomain = (const char *[]) { "cx", NULL },
> +	.opp_pmdomain = pd_names_cx,
>  	.vcodec_num = 2,
>  	.max_load = 3110400,	/* 4096x2160@90 */
>  	.hfi_version = HFI_VERSION_4XX,
> @@ -770,7 +773,7 @@ static const struct venus_resources sc7180_res = {
>  	.vcodec_clks_num = 2,
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
> -	.opp_pmdomain = (const char *[]) { "cx", NULL },
> +	.opp_pmdomain = pd_names_cx,
>  	.vcodec_num = 1,
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> @@ -827,7 +830,7 @@ static const struct venus_resources sm8250_res = {
>  	.vcodec_clks_num = 1,
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
> -	.opp_pmdomain = (const char *[]) { "mx", NULL },
> +	.opp_pmdomain = pd_names_mx,
>  	.vcodec_num = 1,
>  	.max_load = 7833600,
>  	.hfi_version = HFI_VERSION_6XX,
> @@ -886,7 +889,7 @@ static const struct venus_resources sc7280_res = {
>  	.vcodec_clks_num = 2,
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
> -	.opp_pmdomain = (const char *[]) { "cx", NULL },
> +	.opp_pmdomain = pd_names_cx,
>  	.vcodec_num = 1,
>  	.hfi_version = HFI_VERSION_6XX,
>  	.vpu_version = VPU_VERSION_IRIS2_1,
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 08/19] media: venus: core: Get rid of vcodec_num

[Konrad Dybcio]

That field was only introduced to differentiate between the legacy and
non-legacy SDM845 binding. Get rid of it.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       | 5 -----
 drivers/media/platform/qcom/venus/core.h       | 1 -
 drivers/media/platform/qcom/venus/pm_helpers.c | 2 +-
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 5e7cb54e6088..26a0c264685a 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -651,7 +651,6 @@ static const struct venus_resources sdm660_res = {
 	.vcodec0_clks = { "vcodec0_core" },
 	.vcodec1_clks = { "vcodec0_core" },
 	.vcodec_clks_num = 1,
-	.vcodec_num = 1,
 	.max_load = 1036800,
 	.hfi_version = HFI_VERSION_3XX,
 	.vmem_id = VIDC_RESOURCE_NONE,
@@ -725,7 +724,6 @@ static const struct venus_resources sdm845_res_v2 = {
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
 	.vcodec_pmdomains_num = 3,
 	.opp_pmdomain = pd_names_cx,
-	.vcodec_num = 2,
 	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
@@ -774,7 +772,6 @@ static const struct venus_resources sc7180_res = {
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = pd_names_cx,
-	.vcodec_num = 1,
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
 	.vmem_id = VIDC_RESOURCE_NONE,
@@ -831,7 +828,6 @@ static const struct venus_resources sm8250_res = {
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = pd_names_mx,
-	.vcodec_num = 1,
 	.max_load = 7833600,
 	.hfi_version = HFI_VERSION_6XX,
 	.vpu_version = VPU_VERSION_IRIS2,
@@ -890,7 +886,6 @@ static const struct venus_resources sc7280_res = {
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = pd_names_cx,
-	.vcodec_num = 1,
 	.hfi_version = HFI_VERSION_6XX,
 	.vpu_version = VPU_VERSION_IRIS2_1,
 	.num_vpp_pipes = 1,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 6a77de374454..376de1161114 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -74,7 +74,6 @@ struct venus_resources {
 	const char **vcodec_pmdomains;
 	unsigned int vcodec_pmdomains_num;
 	const char **opp_pmdomain;
-	unsigned int vcodec_num;
 	const char * const resets[VIDC_RESETS_NUM_MAX];
 	unsigned int resets_num;
 	enum hfi_version hfi_version;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 5b2a40a2f524..ba63e6427eb9 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -622,7 +622,7 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo
 			VIDC_CORE_ID_1 : VIDC_CORE_ID_2;
 	*min_load = min(core1_load, core2_load);
 
-	if (cores_max < VIDC_CORE_ID_2 || core->res->vcodec_num < 2) {
+	if (cores_max < VIDC_CORE_ID_2 || legacy_binding) {
 		*min_coreid = VIDC_CORE_ID_1;
 		*min_load = core1_load;
 	}

-- 
2.44.0

Re: [PATCH v3 08/19] media: venus: core: Get rid of vcodec_num

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> That field was only introduced to differentiate between the legacy and
> non-legacy SDM845 binding. Get rid of it.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c       | 5 -----
>  drivers/media/platform/qcom/venus/core.h       | 1 -
>  drivers/media/platform/qcom/venus/pm_helpers.c | 2 +-
>  3 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 5e7cb54e6088..26a0c264685a 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -651,7 +651,6 @@ static const struct venus_resources sdm660_res = {
>  	.vcodec0_clks = { "vcodec0_core" },
>  	.vcodec1_clks = { "vcodec0_core" },
>  	.vcodec_clks_num = 1,
> -	.vcodec_num = 1,
>  	.max_load = 1036800,
>  	.hfi_version = HFI_VERSION_3XX,
>  	.vmem_id = VIDC_RESOURCE_NONE,
> @@ -725,7 +724,6 @@ static const struct venus_resources sdm845_res_v2 = {
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
>  	.vcodec_pmdomains_num = 3,
>  	.opp_pmdomain = pd_names_cx,
> -	.vcodec_num = 2,
>  	.max_load = 3110400,	/* 4096x2160@90 */
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> @@ -774,7 +772,6 @@ static const struct venus_resources sc7180_res = {
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
>  	.opp_pmdomain = pd_names_cx,
> -	.vcodec_num = 1,
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
>  	.vmem_id = VIDC_RESOURCE_NONE,
> @@ -831,7 +828,6 @@ static const struct venus_resources sm8250_res = {
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
>  	.opp_pmdomain = pd_names_mx,
> -	.vcodec_num = 1,
>  	.max_load = 7833600,
>  	.hfi_version = HFI_VERSION_6XX,
>  	.vpu_version = VPU_VERSION_IRIS2,
> @@ -890,7 +886,6 @@ static const struct venus_resources sc7280_res = {
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
>  	.opp_pmdomain = pd_names_cx,
> -	.vcodec_num = 1,
>  	.hfi_version = HFI_VERSION_6XX,
>  	.vpu_version = VPU_VERSION_IRIS2_1,
>  	.num_vpp_pipes = 1,
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 6a77de374454..376de1161114 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -74,7 +74,6 @@ struct venus_resources {
>  	const char **vcodec_pmdomains;
>  	unsigned int vcodec_pmdomains_num;
>  	const char **opp_pmdomain;
> -	unsigned int vcodec_num;
>  	const char * const resets[VIDC_RESETS_NUM_MAX];
>  	unsigned int resets_num;
>  	enum hfi_version hfi_version;
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 5b2a40a2f524..ba63e6427eb9 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -622,7 +622,7 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo
>  			VIDC_CORE_ID_1 : VIDC_CORE_ID_2;
>  	*min_load = min(core1_load, core2_load);
>  
> -	if (cores_max < VIDC_CORE_ID_2 || core->res->vcodec_num < 2> +	if (cores_max < VIDC_CORE_ID_2 || legacy_binding) {
core->res->vcodec_num < 2 doesn't mean legacy binding.
7180, 8250 and 7280 have vcodec num as 1 but they don't follow legacy
binding and they still have one core which is VIDC_CORE_ID_1.
>  		*min_coreid = VIDC_CORE_ID_1;
>  		*min_load = core1_load;
>  	}
> 

Thanks,
Dikshita

Re: [PATCH v3 08/19] media: venus: core: Get rid of vcodec_num

[Vikash Garodia]

Hi Konrad,

On 4/5/2024 2:48 PM, Dikshita Agarwal wrote:
> 
> 
> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>> That field was only introduced to differentiate between the legacy and
>> non-legacy SDM845 binding. Get rid of it.
>>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.c       | 5 -----
>>  drivers/media/platform/qcom/venus/core.h       | 1 -
>>  drivers/media/platform/qcom/venus/pm_helpers.c | 2 +-
>>  3 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
>> index 5e7cb54e6088..26a0c264685a 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -651,7 +651,6 @@ static const struct venus_resources sdm660_res = {
>>  	.vcodec0_clks = { "vcodec0_core" },
>>  	.vcodec1_clks = { "vcodec0_core" },
>>  	.vcodec_clks_num = 1,
>> -	.vcodec_num = 1,
>>  	.max_load = 1036800,
>>  	.hfi_version = HFI_VERSION_3XX,
>>  	.vmem_id = VIDC_RESOURCE_NONE,
>> @@ -725,7 +724,6 @@ static const struct venus_resources sdm845_res_v2 = {
>>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
>>  	.vcodec_pmdomains_num = 3,
>>  	.opp_pmdomain = pd_names_cx,
>> -	.vcodec_num = 2,
>>  	.max_load = 3110400,	/* 4096x2160@90 */
>>  	.hfi_version = HFI_VERSION_4XX,
>>  	.vpu_version = VPU_VERSION_AR50,
>> @@ -774,7 +772,6 @@ static const struct venus_resources sc7180_res = {
>>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>>  	.vcodec_pmdomains_num = 2,
>>  	.opp_pmdomain = pd_names_cx,
>> -	.vcodec_num = 1,
>>  	.hfi_version = HFI_VERSION_4XX,
>>  	.vpu_version = VPU_VERSION_AR50,
>>  	.vmem_id = VIDC_RESOURCE_NONE,
>> @@ -831,7 +828,6 @@ static const struct venus_resources sm8250_res = {
>>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>>  	.vcodec_pmdomains_num = 2,
>>  	.opp_pmdomain = pd_names_mx,
>> -	.vcodec_num = 1,
>>  	.max_load = 7833600,
>>  	.hfi_version = HFI_VERSION_6XX,
>>  	.vpu_version = VPU_VERSION_IRIS2,
>> @@ -890,7 +886,6 @@ static const struct venus_resources sc7280_res = {
>>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>>  	.vcodec_pmdomains_num = 2,
>>  	.opp_pmdomain = pd_names_cx,
>> -	.vcodec_num = 1,
>>  	.hfi_version = HFI_VERSION_6XX,
>>  	.vpu_version = VPU_VERSION_IRIS2_1,
>>  	.num_vpp_pipes = 1,
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index 6a77de374454..376de1161114 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -74,7 +74,6 @@ struct venus_resources {
>>  	const char **vcodec_pmdomains;
>>  	unsigned int vcodec_pmdomains_num;
>>  	const char **opp_pmdomain;
>> -	unsigned int vcodec_num;
>>  	const char * const resets[VIDC_RESETS_NUM_MAX];
>>  	unsigned int resets_num;
>>  	enum hfi_version hfi_version;
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index 5b2a40a2f524..ba63e6427eb9 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -622,7 +622,7 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo
>>  			VIDC_CORE_ID_1 : VIDC_CORE_ID_2;
>>  	*min_load = min(core1_load, core2_load);
>>  
>> -	if (cores_max < VIDC_CORE_ID_2 || core->res->vcodec_num < 2> +	if (cores_max < VIDC_CORE_ID_2 || legacy_binding) {
> core->res->vcodec_num < 2 doesn't mean legacy binding.
> 7180, 8250 and 7280 have vcodec num as 1 but they don't follow legacy
> binding and they still have one core which is VIDC_CORE_ID_1.
+1 to above comments. The change is misusing legacy bindings to decide the
cores, while its more readable to keep it with number of vcodec cores.

Thanks,
Vikash
>>  		*min_coreid = VIDC_CORE_ID_1;
>>  		*min_load = core1_load;
>>  	}
>>
> 
> Thanks,
> Dikshita

[PATCH v3 09/19] media: venus: core: Drop cache properties in resource struct

[Konrad Dybcio]

Currently VMEM/OCMEM/LLCC is disabled on all platforms.

Make it unconditional to save on space.

These caches will not be enabled until the Venus driver can reference
them as chunks of SRAM (they're modelled as separate devices) to avoid
hardcoding magic addresses and rougely accessing the hardware,
bypassing the normal accessors.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c      | 24 ------------------------
 drivers/media/platform/qcom/venus/core.h      |  3 ---
 drivers/media/platform/qcom/venus/hfi_venus.c | 10 ++++------
 3 files changed, 4 insertions(+), 33 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 26a0c264685a..51ac9eff244c 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -562,9 +562,6 @@ static const struct venus_resources msm8916_res = {
 	.clks_num = 3,
 	.max_load = 352800, /* 720p@30 + 1080p@30 */
 	.hfi_version = HFI_VERSION_1XX,
-	.vmem_id = VIDC_RESOURCE_NONE,
-	.vmem_size = 0,
-	.vmem_addr = 0,
 	.dma_mask = 0xddc00000 - 1,
 	.fwname = "qcom/venus-1.8/venus.mbn",
 };
@@ -595,9 +592,6 @@ static const struct venus_resources msm8996_res = {
 	.vcodec_clks_num = 1,
 	.max_load = 2563200,
 	.hfi_version = HFI_VERSION_3XX,
-	.vmem_id = VIDC_RESOURCE_NONE,
-	.vmem_size = 0,
-	.vmem_addr = 0,
 	.dma_mask = 0xddc00000 - 1,
 	.fwname = "qcom/venus-4.2/venus.mbn",
 };
@@ -653,9 +647,6 @@ static const struct venus_resources sdm660_res = {
 	.vcodec_clks_num = 1,
 	.max_load = 1036800,
 	.hfi_version = HFI_VERSION_3XX,
-	.vmem_id = VIDC_RESOURCE_NONE,
-	.vmem_size = 0,
-	.vmem_addr = 0,
 	.cp_start = 0,
 	.cp_size = 0x79000000,
 	.cp_nonpixel_start = 0x1000000,
@@ -702,9 +693,6 @@ static const struct venus_resources sdm845_res = {
 	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
-	.vmem_id = VIDC_RESOURCE_NONE,
-	.vmem_size = 0,
-	.vmem_addr = 0,
 	.dma_mask = 0xe0000000 - 1,
 	.fwname = "qcom/venus-5.2/venus.mbn",
 };
@@ -727,9 +715,6 @@ static const struct venus_resources sdm845_res_v2 = {
 	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
-	.vmem_id = VIDC_RESOURCE_NONE,
-	.vmem_size = 0,
-	.vmem_addr = 0,
 	.dma_mask = 0xe0000000 - 1,
 	.cp_start = 0,
 	.cp_size = 0x70800000,
@@ -774,9 +759,6 @@ static const struct venus_resources sc7180_res = {
 	.opp_pmdomain = pd_names_cx,
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
-	.vmem_id = VIDC_RESOURCE_NONE,
-	.vmem_size = 0,
-	.vmem_addr = 0,
 	.dma_mask = 0xe0000000 - 1,
 	.cp_start = 0,
 	.cp_size = 0x70800000,
@@ -832,9 +814,6 @@ static const struct venus_resources sm8250_res = {
 	.hfi_version = HFI_VERSION_6XX,
 	.vpu_version = VPU_VERSION_IRIS2,
 	.num_vpp_pipes = 4,
-	.vmem_id = VIDC_RESOURCE_NONE,
-	.vmem_size = 0,
-	.vmem_addr = 0,
 	.dma_mask = 0xe0000000 - 1,
 	.fwname = "qcom/vpu-1.0/venus.mbn",
 };
@@ -889,9 +868,6 @@ static const struct venus_resources sc7280_res = {
 	.hfi_version = HFI_VERSION_6XX,
 	.vpu_version = VPU_VERSION_IRIS2_1,
 	.num_vpp_pipes = 1,
-	.vmem_id = VIDC_RESOURCE_NONE,
-	.vmem_size = 0,
-	.vmem_addr = 0,
 	.dma_mask = 0xe0000000 - 1,
 	.cp_start = 0,
 	.cp_size = 0x25800000,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 376de1161114..e083ebb3ab4b 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -80,9 +80,6 @@ struct venus_resources {
 	enum vpu_version vpu_version;
 	u8 num_vpp_pipes;
 	u32 max_load;
-	unsigned int vmem_id;
-	u32 vmem_size;
-	u32 vmem_addr;
 	u32 cp_start;
 	u32 cp_size;
 	u32 cp_nonpixel_start;
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f9437b6412b9..42ff96f71235 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1067,17 +1067,14 @@ static void venus_process_msg_sys_error(struct venus_hfi_device *hdev,
 static irqreturn_t venus_isr_thread(struct venus_core *core)
 {
 	struct venus_hfi_device *hdev = to_hfi_priv(core);
-	const struct venus_resources *res;
 	void *pkt;
 	u32 msg_ret;
 
 	if (!hdev)
 		return IRQ_NONE;
 
-	res = hdev->core->res;
 	pkt = hdev->pkt_buf;
 
-
 	while (!venus_iface_msgq_read(hdev, pkt)) {
 		msg_ret = hfi_process_msg_packet(core, pkt);
 		switch (msg_ret) {
@@ -1085,9 +1082,10 @@ static irqreturn_t venus_isr_thread(struct venus_core *core)
 			venus_process_msg_sys_error(hdev, pkt);
 			break;
 		case HFI_MSG_SYS_INIT:
-			venus_hfi_core_set_resource(core, res->vmem_id,
-						    res->vmem_size,
-						    res->vmem_addr,
+			/* Disable OCMEM/VMEM unconditionally until support is added */
+			venus_hfi_core_set_resource(core, VIDC_RESOURCE_NONE,
+						    0,
+						    0,
 						    hdev);
 			break;
 		case HFI_MSG_SYS_RELEASE_RESOURCE:

-- 
2.44.0

Re: [PATCH v3 09/19] media: venus: core: Drop cache properties in resource struct

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> Currently VMEM/OCMEM/LLCC is disabled on all platforms.
> 
> Make it unconditional to save on space.
> 
> These caches will not be enabled until the Venus driver can reference
> them as chunks of SRAM (they're modelled as separate devices) to avoid
> hardcoding magic addresses and rougely accessing the hardware,
> bypassing the normal accessors.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c      | 24 ------------------------
>  drivers/media/platform/qcom/venus/core.h      |  3 ---
>  drivers/media/platform/qcom/venus/hfi_venus.c | 10 ++++------
>  3 files changed, 4 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 26a0c264685a..51ac9eff244c 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -562,9 +562,6 @@ static const struct venus_resources msm8916_res = {
>  	.clks_num = 3,
>  	.max_load = 352800, /* 720p@30 + 1080p@30 */
>  	.hfi_version = HFI_VERSION_1XX,
> -	.vmem_id = VIDC_RESOURCE_NONE,
> -	.vmem_size = 0,
> -	.vmem_addr = 0,
>  	.dma_mask = 0xddc00000 - 1,
>  	.fwname = "qcom/venus-1.8/venus.mbn",
>  };
> @@ -595,9 +592,6 @@ static const struct venus_resources msm8996_res = {
>  	.vcodec_clks_num = 1,
>  	.max_load = 2563200,
>  	.hfi_version = HFI_VERSION_3XX,
> -	.vmem_id = VIDC_RESOURCE_NONE,
> -	.vmem_size = 0,
> -	.vmem_addr = 0,
>  	.dma_mask = 0xddc00000 - 1,
>  	.fwname = "qcom/venus-4.2/venus.mbn",
>  };
> @@ -653,9 +647,6 @@ static const struct venus_resources sdm660_res = {
>  	.vcodec_clks_num = 1,
>  	.max_load = 1036800,
>  	.hfi_version = HFI_VERSION_3XX,
> -	.vmem_id = VIDC_RESOURCE_NONE,
> -	.vmem_size = 0,
> -	.vmem_addr = 0,
>  	.cp_start = 0,
>  	.cp_size = 0x79000000,
>  	.cp_nonpixel_start = 0x1000000,
> @@ -702,9 +693,6 @@ static const struct venus_resources sdm845_res = {
>  	.max_load = 3110400,	/* 4096x2160@90 */
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> -	.vmem_id = VIDC_RESOURCE_NONE,
> -	.vmem_size = 0,
> -	.vmem_addr = 0,
>  	.dma_mask = 0xe0000000 - 1,
>  	.fwname = "qcom/venus-5.2/venus.mbn",
>  };
> @@ -727,9 +715,6 @@ static const struct venus_resources sdm845_res_v2 = {
>  	.max_load = 3110400,	/* 4096x2160@90 */
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> -	.vmem_id = VIDC_RESOURCE_NONE,
> -	.vmem_size = 0,
> -	.vmem_addr = 0,
>  	.dma_mask = 0xe0000000 - 1,
>  	.cp_start = 0,
>  	.cp_size = 0x70800000,
> @@ -774,9 +759,6 @@ static const struct venus_resources sc7180_res = {
>  	.opp_pmdomain = pd_names_cx,
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> -	.vmem_id = VIDC_RESOURCE_NONE,
> -	.vmem_size = 0,
> -	.vmem_addr = 0,
>  	.dma_mask = 0xe0000000 - 1,
>  	.cp_start = 0,
>  	.cp_size = 0x70800000,
> @@ -832,9 +814,6 @@ static const struct venus_resources sm8250_res = {
>  	.hfi_version = HFI_VERSION_6XX,
>  	.vpu_version = VPU_VERSION_IRIS2,
>  	.num_vpp_pipes = 4,
> -	.vmem_id = VIDC_RESOURCE_NONE,
> -	.vmem_size = 0,
> -	.vmem_addr = 0,
>  	.dma_mask = 0xe0000000 - 1,
>  	.fwname = "qcom/vpu-1.0/venus.mbn",
>  };
> @@ -889,9 +868,6 @@ static const struct venus_resources sc7280_res = {
>  	.hfi_version = HFI_VERSION_6XX,
>  	.vpu_version = VPU_VERSION_IRIS2_1,
>  	.num_vpp_pipes = 1,
> -	.vmem_id = VIDC_RESOURCE_NONE,
> -	.vmem_size = 0,
> -	.vmem_addr = 0,
>  	.dma_mask = 0xe0000000 - 1,
>  	.cp_start = 0,
>  	.cp_size = 0x25800000,
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 376de1161114..e083ebb3ab4b 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -80,9 +80,6 @@ struct venus_resources {
>  	enum vpu_version vpu_version;
>  	u8 num_vpp_pipes;
>  	u32 max_load;
> -	unsigned int vmem_id;
> -	u32 vmem_size;
> -	u32 vmem_addr;
>  	u32 cp_start;
>  	u32 cp_size;
>  	u32 cp_nonpixel_start;
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index f9437b6412b9..42ff96f71235 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -1067,17 +1067,14 @@ static void venus_process_msg_sys_error(struct venus_hfi_device *hdev,
>  static irqreturn_t venus_isr_thread(struct venus_core *core)
>  {
>  	struct venus_hfi_device *hdev = to_hfi_priv(core);
> -	const struct venus_resources *res;
>  	void *pkt;
>  	u32 msg_ret;
>  
>  	if (!hdev)
>  		return IRQ_NONE;
>  
> -	res = hdev->core->res;
>  	pkt = hdev->pkt_buf;
>  
> -
>  	while (!venus_iface_msgq_read(hdev, pkt)) {
>  		msg_ret = hfi_process_msg_packet(core, pkt);
>  		switch (msg_ret) {
> @@ -1085,9 +1082,10 @@ static irqreturn_t venus_isr_thread(struct venus_core *core)
>  			venus_process_msg_sys_error(hdev, pkt);
>  			break;
>  		case HFI_MSG_SYS_INIT:
> -			venus_hfi_core_set_resource(core, res->vmem_id,
> -						    res->vmem_size,
> -						    res->vmem_addr,
> +			/* Disable OCMEM/VMEM unconditionally until support is added */
> +			venus_hfi_core_set_resource(core, VIDC_RESOURCE_NONE,
> +						    0,
> +						    0,
>  						    hdev);
>  			break;
>  		case HFI_MSG_SYS_RELEASE_RESOURCE:
> 
If no issues reported by firmware on any SOC, then
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 10/19] media: venus: core: Use GENMASK for dma_mask

[Konrad Dybcio]

The raw literals mean very little. Substitute it with more telling
bitops macros.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 51ac9eff244c..5d41ecddcef6 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -562,7 +562,7 @@ static const struct venus_resources msm8916_res = {
 	.clks_num = 3,
 	.max_load = 352800, /* 720p@30 + 1080p@30 */
 	.hfi_version = HFI_VERSION_1XX,
-	.dma_mask = 0xddc00000 - 1,
+	.dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
 	.fwname = "qcom/venus-1.8/venus.mbn",
 };
 
@@ -592,7 +592,7 @@ static const struct venus_resources msm8996_res = {
 	.vcodec_clks_num = 1,
 	.max_load = 2563200,
 	.hfi_version = HFI_VERSION_3XX,
-	.dma_mask = 0xddc00000 - 1,
+	.dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
 	.fwname = "qcom/venus-4.2/venus.mbn",
 };
 
@@ -693,7 +693,7 @@ static const struct venus_resources sdm845_res = {
 	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
-	.dma_mask = 0xe0000000 - 1,
+	.dma_mask = GENMASK(31, 29) - 1,
 	.fwname = "qcom/venus-5.2/venus.mbn",
 };
 
@@ -715,7 +715,7 @@ static const struct venus_resources sdm845_res_v2 = {
 	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
-	.dma_mask = 0xe0000000 - 1,
+	.dma_mask = GENMASK(31, 29) - 1,
 	.cp_start = 0,
 	.cp_size = 0x70800000,
 	.cp_nonpixel_start = 0x1000000,
@@ -759,7 +759,7 @@ static const struct venus_resources sc7180_res = {
 	.opp_pmdomain = pd_names_cx,
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
-	.dma_mask = 0xe0000000 - 1,
+	.dma_mask = GENMASK(31, 29) - 1,
 	.cp_start = 0,
 	.cp_size = 0x70800000,
 	.cp_nonpixel_start = 0x1000000,
@@ -814,7 +814,7 @@ static const struct venus_resources sm8250_res = {
 	.hfi_version = HFI_VERSION_6XX,
 	.vpu_version = VPU_VERSION_IRIS2,
 	.num_vpp_pipes = 4,
-	.dma_mask = 0xe0000000 - 1,
+	.dma_mask = GENMASK(31, 29) - 1,
 	.fwname = "qcom/vpu-1.0/venus.mbn",
 };
 
@@ -868,7 +868,7 @@ static const struct venus_resources sc7280_res = {
 	.hfi_version = HFI_VERSION_6XX,
 	.vpu_version = VPU_VERSION_IRIS2_1,
 	.num_vpp_pipes = 1,
-	.dma_mask = 0xe0000000 - 1,
+	.dma_mask = GENMASK(31, 29) - 1,
 	.cp_start = 0,
 	.cp_size = 0x25800000,
 	.cp_nonpixel_start = 0x1000000,

-- 
2.44.0

Re: [PATCH v3 10/19] media: venus: core: Use GENMASK for dma_mask

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> The raw literals mean very little. Substitute it with more telling
> bitops macros.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 51ac9eff244c..5d41ecddcef6 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -562,7 +562,7 @@ static const struct venus_resources msm8916_res = {
>  	.clks_num = 3,
>  	.max_load = 352800, /* 720p@30 + 1080p@30 */
>  	.hfi_version = HFI_VERSION_1XX,
> -	.dma_mask = 0xddc00000 - 1,
> +	.dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
>  	.fwname = "qcom/venus-1.8/venus.mbn",
>  };
>  
> @@ -592,7 +592,7 @@ static const struct venus_resources msm8996_res = {
>  	.vcodec_clks_num = 1,
>  	.max_load = 2563200,
>  	.hfi_version = HFI_VERSION_3XX,
> -	.dma_mask = 0xddc00000 - 1,
> +	.dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
>  	.fwname = "qcom/venus-4.2/venus.mbn",
>  };
>  
> @@ -693,7 +693,7 @@ static const struct venus_resources sdm845_res = {
>  	.max_load = 3110400,	/* 4096x2160@90 */
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> -	.dma_mask = 0xe0000000 - 1,
> +	.dma_mask = GENMASK(31, 29) - 1,
>  	.fwname = "qcom/venus-5.2/venus.mbn",
>  };
>  
> @@ -715,7 +715,7 @@ static const struct venus_resources sdm845_res_v2 = {
>  	.max_load = 3110400,	/* 4096x2160@90 */
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> -	.dma_mask = 0xe0000000 - 1,
> +	.dma_mask = GENMASK(31, 29) - 1,
>  	.cp_start = 0,
>  	.cp_size = 0x70800000,
>  	.cp_nonpixel_start = 0x1000000,
> @@ -759,7 +759,7 @@ static const struct venus_resources sc7180_res = {
>  	.opp_pmdomain = pd_names_cx,
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> -	.dma_mask = 0xe0000000 - 1,
> +	.dma_mask = GENMASK(31, 29) - 1,
>  	.cp_start = 0,
>  	.cp_size = 0x70800000,
>  	.cp_nonpixel_start = 0x1000000,
> @@ -814,7 +814,7 @@ static const struct venus_resources sm8250_res = {
>  	.hfi_version = HFI_VERSION_6XX,
>  	.vpu_version = VPU_VERSION_IRIS2,
>  	.num_vpp_pipes = 4,
> -	.dma_mask = 0xe0000000 - 1,
> +	.dma_mask = GENMASK(31, 29) - 1,
>  	.fwname = "qcom/vpu-1.0/venus.mbn",
>  };
>  
> @@ -868,7 +868,7 @@ static const struct venus_resources sc7280_res = {
>  	.hfi_version = HFI_VERSION_6XX,
>  	.vpu_version = VPU_VERSION_IRIS2_1,
>  	.num_vpp_pipes = 1,
> -	.dma_mask = 0xe0000000 - 1,
> +	.dma_mask = GENMASK(31, 29) - 1,
>  	.cp_start = 0,
>  	.cp_size = 0x25800000,
>  	.cp_nonpixel_start = 0x1000000,
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 11/19] media: venus: core: Remove cp_start

[Konrad Dybcio]

It's hardcoded to be zero. Always. Ever since msm-3.10. Or maybe
even before. Remove it!

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c     | 4 ----
 drivers/media/platform/qcom/venus/core.h     | 1 -
 drivers/media/platform/qcom/venus/firmware.c | 3 +--
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 5d41ecddcef6..b10d083b8b17 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -647,7 +647,6 @@ static const struct venus_resources sdm660_res = {
 	.vcodec_clks_num = 1,
 	.max_load = 1036800,
 	.hfi_version = HFI_VERSION_3XX,
-	.cp_start = 0,
 	.cp_size = 0x79000000,
 	.cp_nonpixel_start = 0x1000000,
 	.cp_nonpixel_size = 0x28000000,
@@ -716,7 +715,6 @@ static const struct venus_resources sdm845_res_v2 = {
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
 	.dma_mask = GENMASK(31, 29) - 1,
-	.cp_start = 0,
 	.cp_size = 0x70800000,
 	.cp_nonpixel_start = 0x1000000,
 	.cp_nonpixel_size = 0x24800000,
@@ -760,7 +758,6 @@ static const struct venus_resources sc7180_res = {
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
 	.dma_mask = GENMASK(31, 29) - 1,
-	.cp_start = 0,
 	.cp_size = 0x70800000,
 	.cp_nonpixel_start = 0x1000000,
 	.cp_nonpixel_size = 0x24800000,
@@ -869,7 +866,6 @@ static const struct venus_resources sc7280_res = {
 	.vpu_version = VPU_VERSION_IRIS2_1,
 	.num_vpp_pipes = 1,
 	.dma_mask = GENMASK(31, 29) - 1,
-	.cp_start = 0,
 	.cp_size = 0x25800000,
 	.cp_nonpixel_start = 0x1000000,
 	.cp_nonpixel_size = 0x24800000,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index e083ebb3ab4b..19908f028441 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -80,7 +80,6 @@ struct venus_resources {
 	enum vpu_version vpu_version;
 	u8 num_vpp_pipes;
 	u32 max_load;
-	u32 cp_start;
 	u32 cp_size;
 	u32 cp_nonpixel_start;
 	u32 cp_nonpixel_size;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index fe7da2b30482..16e578780be7 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -245,7 +245,6 @@ int venus_boot(struct venus_core *core)
 	if (core->use_tz && res->cp_size) {
 		/*
 		 * Clues for porting using downstream data:
-		 * cp_start = 0
 		 * cp_size = venus_ns/virtual-addr-pool[0] - yes, address and not size!
 		 *   This works, as the non-secure context bank is placed
 		 *   contiguously right after the Content Protection region.
@@ -253,7 +252,7 @@ int venus_boot(struct venus_core *core)
 		 * cp_nonpixel_start = venus_sec_non_pixel/virtual-addr-pool[0]
 		 * cp_nonpixel_size = venus_sec_non_pixel/virtual-addr-pool[1]
 		 */
-		ret = qcom_scm_mem_protect_video_var(res->cp_start,
+		ret = qcom_scm_mem_protect_video_var(0,
 						     res->cp_size,
 						     res->cp_nonpixel_start,
 						     res->cp_nonpixel_size);

-- 
2.44.0

Re: [PATCH v3 11/19] media: venus: core: Remove cp_start

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> It's hardcoded to be zero. Always. Ever since msm-3.10. Or maybe
> even before. Remove it!
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c     | 4 ----
>  drivers/media/platform/qcom/venus/core.h     | 1 -
>  drivers/media/platform/qcom/venus/firmware.c | 3 +--
>  3 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 5d41ecddcef6..b10d083b8b17 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -647,7 +647,6 @@ static const struct venus_resources sdm660_res = {
>  	.vcodec_clks_num = 1,
>  	.max_load = 1036800,
>  	.hfi_version = HFI_VERSION_3XX,
> -	.cp_start = 0,
>  	.cp_size = 0x79000000,
>  	.cp_nonpixel_start = 0x1000000,
>  	.cp_nonpixel_size = 0x28000000,
> @@ -716,7 +715,6 @@ static const struct venus_resources sdm845_res_v2 = {
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
>  	.dma_mask = GENMASK(31, 29) - 1,
> -	.cp_start = 0,
>  	.cp_size = 0x70800000,
>  	.cp_nonpixel_start = 0x1000000,
>  	.cp_nonpixel_size = 0x24800000,
> @@ -760,7 +758,6 @@ static const struct venus_resources sc7180_res = {
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
>  	.dma_mask = GENMASK(31, 29) - 1,
> -	.cp_start = 0,
>  	.cp_size = 0x70800000,
>  	.cp_nonpixel_start = 0x1000000,
>  	.cp_nonpixel_size = 0x24800000,
> @@ -869,7 +866,6 @@ static const struct venus_resources sc7280_res = {
>  	.vpu_version = VPU_VERSION_IRIS2_1,
>  	.num_vpp_pipes = 1,
>  	.dma_mask = GENMASK(31, 29) - 1,
> -	.cp_start = 0,
>  	.cp_size = 0x25800000,
>  	.cp_nonpixel_start = 0x1000000,
>  	.cp_nonpixel_size = 0x24800000,
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index e083ebb3ab4b..19908f028441 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -80,7 +80,6 @@ struct venus_resources {
>  	enum vpu_version vpu_version;
>  	u8 num_vpp_pipes;
>  	u32 max_load;
> -	u32 cp_start;
>  	u32 cp_size;
>  	u32 cp_nonpixel_start;
>  	u32 cp_nonpixel_size;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index fe7da2b30482..16e578780be7 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -245,7 +245,6 @@ int venus_boot(struct venus_core *core)
>  	if (core->use_tz && res->cp_size) {
>  		/*
>  		 * Clues for porting using downstream data:
> -		 * cp_start = 0
>  		 * cp_size = venus_ns/virtual-addr-pool[0] - yes, address and not size!
>  		 *   This works, as the non-secure context bank is placed
>  		 *   contiguously right after the Content Protection region.
> @@ -253,7 +252,7 @@ int venus_boot(struct venus_core *core)
>  		 * cp_nonpixel_start = venus_sec_non_pixel/virtual-addr-pool[0]
>  		 * cp_nonpixel_size = venus_sec_non_pixel/virtual-addr-pool[1]
>  		 */
> -		ret = qcom_scm_mem_protect_video_var(res->cp_start,
> +		ret = qcom_scm_mem_protect_video_var(0,
>  						     res->cp_size,
>  						     res->cp_nonpixel_start,
>  						     res->cp_nonpixel_size);
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 12/19] media: venus: pm_helpers: Commonize core_power

[Konrad Dybcio]

core_power_v4 called with num_resets = 0 and core->pmdomains[0] == NULL
does exactly the same thing as core_power_v1. Unify them!

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       | 21 +++++++--------------
 drivers/media/platform/qcom/venus/pm_helpers.c | 17 +----------------
 drivers/media/platform/qcom/venus/pm_helpers.h |  2 +-
 3 files changed, 9 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index b10d083b8b17..6bbb8153797c 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -477,18 +477,15 @@ static void venus_core_shutdown(struct platform_device *pdev)
 static __maybe_unused int venus_runtime_suspend(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
-	const struct venus_pm_ops *pm_ops = core->pm_ops;
 	int ret;
 
 	ret = hfi_core_suspend(core);
 	if (ret)
 		return ret;
 
-	if (pm_ops->core_power) {
-		ret = pm_ops->core_power(core, POWER_OFF);
-		if (ret)
-			return ret;
-	}
+	ret = venus_core_power(core, POWER_OFF);
+	if (ret)
+		return ret;
 
 	ret = icc_set_bw(core->cpucfg_path, 0, 0);
 	if (ret)
@@ -503,8 +500,7 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
 err_video_path:
 	icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
 err_cpucfg_path:
-	if (pm_ops->core_power)
-		pm_ops->core_power(core, POWER_ON);
+	venus_core_power(core, POWER_ON);
 
 	return ret;
 }
@@ -512,7 +508,6 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
 static __maybe_unused int venus_runtime_resume(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
-	const struct venus_pm_ops *pm_ops = core->pm_ops;
 	int ret;
 
 	ret = icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
@@ -523,11 +518,9 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (pm_ops->core_power) {
-		ret = pm_ops->core_power(core, POWER_ON);
-		if (ret)
-			return ret;
-	}
+	ret = venus_core_power(core, POWER_ON);
+	if (ret)
+		return ret;
 
 	return hfi_core_resume(core, false);
 }
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ba63e6427eb9..3410039bb641 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -322,22 +322,9 @@ static void core_put_v1(struct venus_core *core)
 {
 }
 
-static int core_power_v1(struct venus_core *core, int on)
-{
-	int ret = 0;
-
-	if (on == POWER_ON)
-		ret = core_clks_enable(core);
-	else
-		core_clks_disable(core);
-
-	return ret;
-}
-
 static const struct venus_pm_ops pm_ops_v1 = {
 	.core_get = venus_clks_get,
 	.core_put = core_put_v1,
-	.core_power = core_power_v1,
 	.load_scale = load_scale_v1,
 };
 
@@ -410,7 +397,6 @@ static int venc_power_v3(struct device *dev, int on)
 static const struct venus_pm_ops pm_ops_v3 = {
 	.core_get = venus_clks_get,
 	.core_put = core_put_v1,
-	.core_power = core_power_v1,
 	.vdec_get = vdec_get_v3,
 	.vdec_power = vdec_power_v3,
 	.venc_get = venc_get_v3,
@@ -990,7 +976,7 @@ static void core_put_v4(struct venus_core *core)
 	vcodec_domains_put(core);
 }
 
-static int core_power_v4(struct venus_core *core, int on)
+int venus_core_power(struct venus_core *core, int on)
 {
 	struct device *dev = core->dev;
 	struct device *pmctrl = core->pmdomains ?
@@ -1138,7 +1124,6 @@ static int load_scale_v4(struct venus_inst *inst)
 static const struct venus_pm_ops pm_ops_v4 = {
 	.core_get = core_get_v4,
 	.core_put = core_put_v4,
-	.core_power = core_power_v4,
 	.vdec_get = vdec_get_v4,
 	.vdec_put = vdec_put_v4,
 	.vdec_power = vdec_power_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index a492c50c5543..77db940a265c 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -12,7 +12,6 @@ struct venus_core;
 struct venus_pm_ops {
 	int (*core_get)(struct venus_core *core);
 	void (*core_put)(struct venus_core *core);
-	int (*core_power)(struct venus_core *core, int on);
 
 	int (*vdec_get)(struct device *dev);
 	void (*vdec_put)(struct device *dev);
@@ -28,6 +27,7 @@ struct venus_pm_ops {
 };
 
 const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
+int venus_core_power(struct venus_core *core, int on);
 
 static inline int venus_pm_load_scale(struct venus_inst *inst)
 {

-- 
2.44.0

Re: [PATCH v3 12/19] media: venus: pm_helpers: Commonize core_power

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> core_power_v4 called with num_resets = 0 and core->pmdomains[0] == NULL
> does exactly the same thing as core_power_v1. Unify them!
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c       | 21 +++++++--------------
>  drivers/media/platform/qcom/venus/pm_helpers.c | 17 +----------------
>  drivers/media/platform/qcom/venus/pm_helpers.h |  2 +-
>  3 files changed, 9 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index b10d083b8b17..6bbb8153797c 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -477,18 +477,15 @@ static void venus_core_shutdown(struct platform_device *pdev)
>  static __maybe_unused int venus_runtime_suspend(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> -	const struct venus_pm_ops *pm_ops = core->pm_ops;
>  	int ret;
>  
>  	ret = hfi_core_suspend(core);
>  	if (ret)
>  		return ret;
>  
> -	if (pm_ops->core_power) {
> -		ret = pm_ops->core_power(core, POWER_OFF);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = venus_core_power(core, POWER_OFF);
> +	if (ret)
> +		return ret;
>  
>  	ret = icc_set_bw(core->cpucfg_path, 0, 0);
>  	if (ret)
> @@ -503,8 +500,7 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
>  err_video_path:
>  	icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
>  err_cpucfg_path:
> -	if (pm_ops->core_power)
> -		pm_ops->core_power(core, POWER_ON);
> +	venus_core_power(core, POWER_ON);
>  
>  	return ret;
>  }
> @@ -512,7 +508,6 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
>  static __maybe_unused int venus_runtime_resume(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> -	const struct venus_pm_ops *pm_ops = core->pm_ops;
>  	int ret;
>  
>  	ret = icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
> @@ -523,11 +518,9 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	if (pm_ops->core_power) {
> -		ret = pm_ops->core_power(core, POWER_ON);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = venus_core_power(core, POWER_ON);
> +	if (ret)
> +		return ret;
>  
>  	return hfi_core_resume(core, false);
>  }
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index ba63e6427eb9..3410039bb641 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -322,22 +322,9 @@ static void core_put_v1(struct venus_core *core)
>  {
>  }
>  
> -static int core_power_v1(struct venus_core *core, int on)
> -{
> -	int ret = 0;
> -
> -	if (on == POWER_ON)
> -		ret = core_clks_enable(core);
> -	else
> -		core_clks_disable(core);
> -
> -	return ret;
> -}
> -
>  static const struct venus_pm_ops pm_ops_v1 = {
>  	.core_get = venus_clks_get,
>  	.core_put = core_put_v1,
> -	.core_power = core_power_v1,
>  	.load_scale = load_scale_v1,
>  };
>  
> @@ -410,7 +397,6 @@ static int venc_power_v3(struct device *dev, int on)
>  static const struct venus_pm_ops pm_ops_v3 = {
>  	.core_get = venus_clks_get,
>  	.core_put = core_put_v1,
> -	.core_power = core_power_v1,
>  	.vdec_get = vdec_get_v3,
>  	.vdec_power = vdec_power_v3,
>  	.venc_get = venc_get_v3,
> @@ -990,7 +976,7 @@ static void core_put_v4(struct venus_core *core)
>  	vcodec_domains_put(core);
>  }
>  
> -static int core_power_v4(struct venus_core *core, int on)
> +int venus_core_power(struct venus_core *core, int on)
>  {
>  	struct device *dev = core->dev;
>  	struct device *pmctrl = core->pmdomains ?
> @@ -1138,7 +1124,6 @@ static int load_scale_v4(struct venus_inst *inst)
>  static const struct venus_pm_ops pm_ops_v4 = {
>  	.core_get = core_get_v4,
>  	.core_put = core_put_v4,
> -	.core_power = core_power_v4,
>  	.vdec_get = vdec_get_v4,
>  	.vdec_put = vdec_put_v4,
>  	.vdec_power = vdec_power_v4,
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
> index a492c50c5543..77db940a265c 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.h
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.h
> @@ -12,7 +12,6 @@ struct venus_core;
>  struct venus_pm_ops {
>  	int (*core_get)(struct venus_core *core);
>  	void (*core_put)(struct venus_core *core);
> -	int (*core_power)(struct venus_core *core, int on);
>  
>  	int (*vdec_get)(struct device *dev);
>  	void (*vdec_put)(struct device *dev);
> @@ -28,6 +27,7 @@ struct venus_pm_ops {
>  };
>  
>  const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
> +int venus_core_power(struct venus_core *core, int on);
>  
>  static inline int venus_pm_load_scale(struct venus_inst *inst)
>  {
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 13/19] media: venus: pm_helpers: Remove pm_ops->core_put

[Konrad Dybcio]

Without an OPP table and with vcodec_pmdomains_num (so, v1, v3 and
sdm845_legacy targets), core_put_v4 is a NOP, jut like core_put_v1.
Unify them!

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       |  8 +++-----
 drivers/media/platform/qcom/venus/pm_helpers.c | 17 +----------------
 drivers/media/platform/qcom/venus/pm_helpers.h |  2 +-
 3 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 6bbb8153797c..5b18b1f41267 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -426,15 +426,14 @@ static int venus_probe(struct platform_device *pdev)
 err_core_deinit:
 	hfi_core_deinit(core, false);
 err_core_put:
-	if (core->pm_ops->core_put)
-		core->pm_ops->core_put(core);
+	vcodec_domains_put(core);
+
 	return ret;
 }
 
 static void venus_remove(struct platform_device *pdev)
 {
 	struct venus_core *core = platform_get_drvdata(pdev);
-	const struct venus_pm_ops *pm_ops = core->pm_ops;
 	struct device *dev = core->dev;
 	int ret;
 
@@ -452,8 +451,7 @@ static void venus_remove(struct platform_device *pdev)
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 
-	if (pm_ops->core_put)
-		pm_ops->core_put(core);
+	vcodec_domains_put(core);
 
 	v4l2_device_unregister(&core->v4l2_dev);
 
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 3410039bb641..d717e150b34f 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -318,13 +318,8 @@ static int load_scale_v1(struct venus_inst *inst)
 	return ret;
 }
 
-static void core_put_v1(struct venus_core *core)
-{
-}
-
 static const struct venus_pm_ops pm_ops_v1 = {
 	.core_get = venus_clks_get,
-	.core_put = core_put_v1,
 	.load_scale = load_scale_v1,
 };
 
@@ -396,7 +391,6 @@ static int venc_power_v3(struct device *dev, int on)
 
 static const struct venus_pm_ops pm_ops_v3 = {
 	.core_get = venus_clks_get,
-	.core_put = core_put_v1,
 	.vdec_get = vdec_get_v3,
 	.vdec_power = vdec_power_v3,
 	.venc_get = venc_get_v3,
@@ -893,7 +887,7 @@ static int vcodec_domains_get(struct venus_core *core)
 	return ret;
 }
 
-static void vcodec_domains_put(struct venus_core *core)
+void vcodec_domains_put(struct venus_core *core)
 {
 	dev_pm_domain_detach_list(core->pmdomains);
 
@@ -968,14 +962,6 @@ static int core_get_v4(struct venus_core *core)
 	return 0;
 }
 
-static void core_put_v4(struct venus_core *core)
-{
-	if (legacy_binding)
-		return;
-
-	vcodec_domains_put(core);
-}
-
 int venus_core_power(struct venus_core *core, int on)
 {
 	struct device *dev = core->dev;
@@ -1123,7 +1109,6 @@ static int load_scale_v4(struct venus_inst *inst)
 
 static const struct venus_pm_ops pm_ops_v4 = {
 	.core_get = core_get_v4,
-	.core_put = core_put_v4,
 	.vdec_get = vdec_get_v4,
 	.vdec_put = vdec_put_v4,
 	.vdec_power = vdec_power_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index 77db940a265c..3014b39aa6e3 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -11,7 +11,6 @@ struct venus_core;
 
 struct venus_pm_ops {
 	int (*core_get)(struct venus_core *core);
-	void (*core_put)(struct venus_core *core);
 
 	int (*vdec_get)(struct device *dev);
 	void (*vdec_put)(struct device *dev);
@@ -28,6 +27,7 @@ struct venus_pm_ops {
 
 const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
 int venus_core_power(struct venus_core *core, int on);
+void vcodec_domains_put(struct venus_core *core);
 
 static inline int venus_pm_load_scale(struct venus_inst *inst)
 {

-- 
2.44.0

Re: [PATCH v3 13/19] media: venus: pm_helpers: Remove pm_ops->core_put

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> Without an OPP table and with vcodec_pmdomains_num (so, v1, v3 and
> sdm845_legacy targets), core_put_v4 is a NOP, jut like core_put_v1.
> Unify them!
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c       |  8 +++-----
>  drivers/media/platform/qcom/venus/pm_helpers.c | 17 +----------------
>  drivers/media/platform/qcom/venus/pm_helpers.h |  2 +-
>  3 files changed, 5 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 6bbb8153797c..5b18b1f41267 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -426,15 +426,14 @@ static int venus_probe(struct platform_device *pdev)
>  err_core_deinit:
>  	hfi_core_deinit(core, false);
>  err_core_put:
> -	if (core->pm_ops->core_put)
> -		core->pm_ops->core_put(core);
> +	vcodec_domains_put(core);
> +
>  	return ret;
>  }
>  
>  static void venus_remove(struct platform_device *pdev)
>  {
>  	struct venus_core *core = platform_get_drvdata(pdev);
> -	const struct venus_pm_ops *pm_ops = core->pm_ops;
>  	struct device *dev = core->dev;
>  	int ret;
>  
> @@ -452,8 +451,7 @@ static void venus_remove(struct platform_device *pdev)
>  	pm_runtime_put_sync(dev);
>  	pm_runtime_disable(dev);
>  
> -	if (pm_ops->core_put)
> -		pm_ops->core_put(core);
> +	vcodec_domains_put(core);
>  
>  	v4l2_device_unregister(&core->v4l2_dev);
>  
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 3410039bb641..d717e150b34f 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -318,13 +318,8 @@ static int load_scale_v1(struct venus_inst *inst)
>  	return ret;
>  }
>  
> -static void core_put_v1(struct venus_core *core)
> -{
> -}
> -
>  static const struct venus_pm_ops pm_ops_v1 = {
>  	.core_get = venus_clks_get,
> -	.core_put = core_put_v1,
>  	.load_scale = load_scale_v1,
>  };
>  
> @@ -396,7 +391,6 @@ static int venc_power_v3(struct device *dev, int on)
>  
>  static const struct venus_pm_ops pm_ops_v3 = {
>  	.core_get = venus_clks_get,
> -	.core_put = core_put_v1,
>  	.vdec_get = vdec_get_v3,
>  	.vdec_power = vdec_power_v3,
>  	.venc_get = venc_get_v3,
> @@ -893,7 +887,7 @@ static int vcodec_domains_get(struct venus_core *core)
>  	return ret;
>  }
>  
> -static void vcodec_domains_put(struct venus_core *core)
> +void vcodec_domains_put(struct venus_core *core)
>  {
>  	dev_pm_domain_detach_list(core->pmdomains);
>  
> @@ -968,14 +962,6 @@ static int core_get_v4(struct venus_core *core)
>  	return 0;
>  }
>  
> -static void core_put_v4(struct venus_core *core)
> -{
> -	if (legacy_binding)
> -		return;
> -
> -	vcodec_domains_put(core);
> -}
> -
>  int venus_core_power(struct venus_core *core, int on)
>  {
>  	struct device *dev = core->dev;
> @@ -1123,7 +1109,6 @@ static int load_scale_v4(struct venus_inst *inst)
>  
>  static const struct venus_pm_ops pm_ops_v4 = {
>  	.core_get = core_get_v4,
> -	.core_put = core_put_v4,
>  	.vdec_get = vdec_get_v4,
>  	.vdec_put = vdec_put_v4,
>  	.vdec_power = vdec_power_v4,
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
> index 77db940a265c..3014b39aa6e3 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.h
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.h
> @@ -11,7 +11,6 @@ struct venus_core;
>  
>  struct venus_pm_ops {
>  	int (*core_get)(struct venus_core *core);
> -	void (*core_put)(struct venus_core *core);
>  
>  	int (*vdec_get)(struct device *dev);
>  	void (*vdec_put)(struct device *dev);
> @@ -28,6 +27,7 @@ struct venus_pm_ops {
>  
>  const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
>  int venus_core_power(struct venus_core *core, int on);
> +void vcodec_domains_put(struct venus_core *core);
>  
>  static inline int venus_pm_load_scale(struct venus_inst *inst)
>  {
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

[PATCH v3 14/19] media: venus: core: Define a pointer to core->res

[Konrad Dybcio]

To make the code more concise, define a new variable 'res' pointing to
the abundantly referenced core->res.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 5b18b1f41267..e61aa863b7f7 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -285,6 +285,7 @@ static irqreturn_t venus_isr_thread(int irq, void *dev_id)
 
 static int venus_probe(struct platform_device *pdev)
 {
+	const struct venus_resources *res;
 	struct device *dev = &pdev->dev;
 	struct venus_core *core;
 	int i, ret;
@@ -315,9 +316,11 @@ static int venus_probe(struct platform_device *pdev)
 	if (!core->res)
 		return -ENODEV;
 
+	res = core->res;
+
 	mutex_init(&core->pm_lock);
 
-	core->pm_ops = venus_pm_get(core->res->hfi_version);
+	core->pm_ops = venus_pm_get(res->hfi_version);
 	if (!core->pm_ops)
 		return -ENODEV;
 
@@ -325,8 +328,8 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	for (i = 0; i < core->res->resets_num; i++) {
-		core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
+	for (i = 0; i < res->resets_num; i++) {
+		core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
 		if (IS_ERR(core->resets[i]))
 			return PTR_ERR(core->resets[i]);
 	}
@@ -337,7 +340,7 @@ static int venus_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	ret = dma_set_mask_and_coherent(dev, core->res->dma_mask);
+	ret = dma_set_mask_and_coherent(dev, res->dma_mask);
 	if (ret)
 		goto err_core_put;
 

-- 
2.44.0

Re: [PATCH v3 14/19] media: venus: core: Define a pointer to core->res

[Bryan O'Donoghue]

On 27/03/2024 18:08, Konrad Dybcio wrote:
> To make the code more concise, define a new variable 'res' pointing to
> the abundantly referenced core->res.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v3 14/19] media: venus: core: Define a pointer to core->res

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> To make the code more concise, define a new variable 'res' pointing to
> the abundantly referenced core->res.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 5b18b1f41267..e61aa863b7f7 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -285,6 +285,7 @@ static irqreturn_t venus_isr_thread(int irq, void *dev_id)
>  
>  static int venus_probe(struct platform_device *pdev)
>  {
> +	const struct venus_resources *res;
>  	struct device *dev = &pdev->dev;
>  	struct venus_core *core;
>  	int i, ret;
> @@ -315,9 +316,11 @@ static int venus_probe(struct platform_device *pdev)
>  	if (!core->res)
>  		return -ENODEV;
>  
> +	res = core->res;
> +
>  	mutex_init(&core->pm_lock);
>  
> -	core->pm_ops = venus_pm_get(core->res->hfi_version);
> +	core->pm_ops = venus_pm_get(res->hfi_version);
>  	if (!core->pm_ops)
>  		return -ENODEV;
>  
> @@ -325,8 +328,8 @@ static int venus_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < core->res->resets_num; i++) {
> -		core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
> +	for (i = 0; i < res->resets_num; i++) {
> +		core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
>  		if (IS_ERR(core->resets[i]))
>  			return PTR_ERR(core->resets[i]);
>  	}
> @@ -337,7 +340,7 @@ static int venus_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>  
> -	ret = dma_set_mask_and_coherent(dev, core->res->dma_mask);
> +	ret = dma_set_mask_and_coherent(dev, res->dma_mask);
>  	if (ret)
>  		goto err_core_put;
>  
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>	

[PATCH v3 15/19] media: venus: pm_helpers: Simplify vcodec clock handling

[Konrad Dybcio]

Currently the infrastructure is set up for vast expandability, but
it's far too complex for what is just 0-2 clocks. Categorize the
clocks and simplify their getting.

One notable change is that vcodec clocks are switched to use
devm_clk_get_optional, which will let us commonize the code further
while leaving the burden of figuring out which SoCs need codec-specific
clocks and which don't to the bindings checker.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       |  18 ----
 drivers/media/platform/qcom/venus/core.h       |  13 ++-
 drivers/media/platform/qcom/venus/pm_helpers.c | 129 +++++++++++++------------
 3 files changed, 71 insertions(+), 89 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index e61aa863b7f7..1f4a86b1bd73 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -581,9 +581,6 @@ static const struct venus_resources msm8996_res = {
 	.reg_tbl_size = ARRAY_SIZE(msm8996_reg_preset),
 	.clks = {"core", "iface", "bus", "mbus" },
 	.clks_num = 4,
-	.vcodec0_clks = { "core" },
-	.vcodec1_clks = { "core" },
-	.vcodec_clks_num = 1,
 	.max_load = 2563200,
 	.hfi_version = HFI_VERSION_3XX,
 	.dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
@@ -636,9 +633,6 @@ static const struct venus_resources sdm660_res = {
 	.bw_tbl_dec_size = ARRAY_SIZE(sdm660_bw_table_dec),
 	.clks = {"core", "iface", "bus", "bus_throttle" },
 	.clks_num = 4,
-	.vcodec0_clks = { "vcodec0_core" },
-	.vcodec1_clks = { "vcodec0_core" },
-	.vcodec_clks_num = 1,
 	.max_load = 1036800,
 	.hfi_version = HFI_VERSION_3XX,
 	.cp_size = 0x79000000,
@@ -680,9 +674,6 @@ static const struct venus_resources sdm845_res = {
 	.bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
 	.clks = {"core", "iface", "bus" },
 	.clks_num = 3,
-	.vcodec0_clks = { "core", "bus" },
-	.vcodec1_clks = { "core", "bus" },
-	.vcodec_clks_num = 2,
 	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_4XX,
 	.vpu_version = VPU_VERSION_AR50,
@@ -699,9 +690,6 @@ static const struct venus_resources sdm845_res_v2 = {
 	.bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
 	.clks = {"core", "iface", "bus" },
 	.clks_num = 3,
-	.vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
-	.vcodec1_clks = { "vcodec1_core", "vcodec1_bus" },
-	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
 	.vcodec_pmdomains_num = 3,
 	.opp_pmdomain = pd_names_cx,
@@ -744,8 +732,6 @@ static const struct venus_resources sc7180_res = {
 	.bw_tbl_dec_size = ARRAY_SIZE(sc7180_bw_table_dec),
 	.clks = {"core", "iface", "bus" },
 	.clks_num = 3,
-	.vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
-	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = pd_names_cx,
@@ -796,8 +782,6 @@ static const struct venus_resources sm8250_res = {
 	.clks_num = 2,
 	.resets = { "bus", "core" },
 	.resets_num = 2,
-	.vcodec0_clks = { "vcodec0_core" },
-	.vcodec_clks_num = 1,
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = pd_names_mx,
@@ -851,8 +835,6 @@ static const struct venus_resources sc7280_res = {
 	.ubwc_conf = &sc7280_ubwc_config,
 	.clks = {"core", "bus", "iface"},
 	.clks_num = 3,
-	.vcodec0_clks = {"vcodec_core", "vcodec_bus"},
-	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
 	.opp_pmdomain = pd_names_cx,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 19908f028441..b4c41dc0f8c7 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -24,9 +24,10 @@
 #define VDBGFW	"VenusFW  : "
 
 #define VIDC_CLKS_NUM_MAX		4
-#define VIDC_VCODEC_CLKS_NUM_MAX	2
 #define VIDC_RESETS_NUM_MAX		2
 
+#define MAX_NUM_VCODECS			2
+
 extern int venus_fw_debug;
 
 struct freq_tbl {
@@ -68,8 +69,6 @@ struct venus_resources {
 	const struct hfi_ubwc_config *ubwc_conf;
 	const char * const clks[VIDC_CLKS_NUM_MAX];
 	unsigned int clks_num;
-	const char * const vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
-	const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
 	unsigned int vcodec_clks_num;
 	const char **vcodec_pmdomains;
 	unsigned int vcodec_pmdomains_num;
@@ -123,8 +122,8 @@ struct venus_format {
  * @aon_base:	AON base address
  * @irq:		Venus irq
  * @clks:	an array of struct clk pointers
- * @vcodec0_clks: an array of vcodec0 struct clk pointers
- * @vcodec1_clks: an array of vcodec1 struct clk pointers
+ * @vcodec_core_clks: an array of codec core clk pointers
+ * @vcodec_bus_clks: an array of codec bus clk pointers
  * @video_path: an interconnect handle to video to/from memory path
  * @cpucfg_path: an interconnect handle to cpu configuration path
  * @has_opp_table: does OPP table exist
@@ -176,8 +175,8 @@ struct venus_core {
 	void __iomem *aon_base;
 	int irq;
 	struct clk *clks[VIDC_CLKS_NUM_MAX];
-	struct clk *vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
-	struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
+	struct clk *vcodec_core_clks[MAX_NUM_VCODECS];
+	struct clk *vcodec_bus_clks[MAX_NUM_VCODECS];
 	struct icc_path *video_path;
 	struct icc_path *cpucfg_path;
 	bool has_opp_table;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index d717e150b34f..583153bbb74e 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -110,67 +110,74 @@ static void core_clks_disable(struct venus_core *core)
 
 static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
 {
-	int ret;
+	int i, ret;
 
 	ret = dev_pm_opp_set_rate(core->dev, freq);
 	if (ret)
 		return ret;
 
-	ret = clk_set_rate(core->vcodec0_clks[0], freq);
-	if (ret)
-		return ret;
-
-	ret = clk_set_rate(core->vcodec1_clks[0], freq);
-	if (ret)
-		return ret;
+	for (i = 0; i < MAX_NUM_VCODECS; i++) {
+		ret = clk_set_rate(core->vcodec_core_clks[i], freq);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
 
-static int vcodec_clks_get(struct venus_core *core, struct device *dev,
-			   struct clk **clks, const char * const *id)
+static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
 {
-	const struct venus_resources *res = core->res;
-	unsigned int i;
+	char buf[13] = { 0 }; /* vcodecX_core\0 */
 
-	for (i = 0; i < res->vcodec_clks_num; i++) {
-		if (!id[i])
-			continue;
-		clks[i] = devm_clk_get(dev, id[i]);
-		if (IS_ERR(clks[i]))
-			return PTR_ERR(clks[i]);
+	/* Best we can do is 2 cores */
+	if (id > MAX_NUM_VCODECS - 1) {
+		dev_err(dev, "Got impossible vcodec id %u\n", id);
+		return -EINVAL;
+	};
+
+	snprintf(buf, sizeof(buf), "vcodec%u_core", id);
+
+	/* First try the non-legacy name */
+	core->vcodec_core_clks[id] = devm_clk_get_optional(dev, buf);
+	if (IS_ERR(core->vcodec_core_clks[id])) {
+		/* Try again, with the legacy name */
+		core->vcodec_core_clks[id] = devm_clk_get_optional(dev, "core");
+		if (IS_ERR(core->vcodec_core_clks[id]))
+			return PTR_ERR(core->vcodec_core_clks[id]);
+	}
+
+	memset(buf, 0, sizeof(buf));
+	snprintf(buf, sizeof(buf), "vcodec%u_bus", id);
+
+	core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, buf);
+	if (IS_ERR(core->vcodec_bus_clks[id])) {
+		core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, "bus");
+		if (IS_ERR(core->vcodec_bus_clks[id]))
+			return PTR_ERR(core->vcodec_bus_clks[id]);
 	}
 
 	return 0;
 }
 
-static int vcodec_clks_enable(struct venus_core *core, struct clk **clks)
+static int vcodec_clks_enable(struct venus_core *core, u8 id)
 {
-	const struct venus_resources *res = core->res;
-	unsigned int i;
 	int ret;
 
-	for (i = 0; i < res->vcodec_clks_num; i++) {
-		ret = clk_prepare_enable(clks[i]);
-		if (ret)
-			goto err;
-	}
+	ret = clk_prepare_enable(core->vcodec_core_clks[id]);
+	if (ret)
+		return ret;
 
-	return 0;
-err:
-	while (i--)
-		clk_disable_unprepare(clks[i]);
+	ret = clk_prepare_enable(core->vcodec_bus_clks[id]);
+	if (ret)
+		clk_disable_unprepare(core->vcodec_core_clks[id]);
 
 	return ret;
 }
 
-static void vcodec_clks_disable(struct venus_core *core, struct clk **clks)
+static void vcodec_clks_disable(struct venus_core *core, u8 id)
 {
-	const struct venus_resources *res = core->res;
-	unsigned int i = res->vcodec_clks_num;
-
-	while (i--)
-		clk_disable_unprepare(clks[i]);
+	clk_disable_unprepare(core->vcodec_bus_clks[id]);
+	clk_disable_unprepare(core->vcodec_core_clks[id]);
 }
 
 static u32 load_per_instance(struct venus_inst *inst)
@@ -343,8 +350,7 @@ static int vdec_get_v3(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
 
-	return vcodec_clks_get(core, dev, core->vcodec0_clks,
-			       core->res->vcodec0_clks);
+	return vcodec_clks_get(core, dev, 0);
 }
 
 static int vdec_power_v3(struct device *dev, int on)
@@ -355,9 +361,9 @@ static int vdec_power_v3(struct device *dev, int on)
 	vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, true);
 
 	if (on == POWER_ON)
-		ret = vcodec_clks_enable(core, core->vcodec0_clks);
+		ret = vcodec_clks_enable(core, 0);
 	else
-		vcodec_clks_disable(core, core->vcodec0_clks);
+		vcodec_clks_disable(core, 0);
 
 	vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, false);
 
@@ -368,8 +374,7 @@ static int venc_get_v3(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
 
-	return vcodec_clks_get(core, dev, core->vcodec1_clks,
-			       core->res->vcodec1_clks);
+	return vcodec_clks_get(core, dev, 1);
 }
 
 static int venc_power_v3(struct device *dev, int on)
@@ -380,9 +385,9 @@ static int venc_power_v3(struct device *dev, int on)
 	vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, true);
 
 	if (on == POWER_ON)
-		ret = vcodec_clks_enable(core, core->vcodec1_clks);
+		ret = vcodec_clks_enable(core, 1);
 	else
-		vcodec_clks_disable(core, core->vcodec1_clks);
+		vcodec_clks_disable(core, 1);
 
 	vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, false);
 
@@ -441,7 +446,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
 		if (ret)
 			return ret;
 
-		vcodec_clks_disable(core, core->vcodec0_clks);
+		vcodec_clks_disable(core, 0);
 
 		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
 		if (ret)
@@ -457,7 +462,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
 		if (ret)
 			return ret;
 
-		vcodec_clks_disable(core, core->vcodec1_clks);
+		vcodec_clks_disable(core, 1);
 
 		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
 		if (ret)
@@ -484,7 +489,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
 		if (ret)
 			return ret;
 
-		ret = vcodec_clks_enable(core, core->vcodec0_clks);
+		ret = vcodec_clks_enable(core, 0);
 		if (ret)
 			return ret;
 
@@ -502,7 +507,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
 		if (ret)
 			return ret;
 
-		ret = vcodec_clks_enable(core, core->vcodec1_clks);
+		ret = vcodec_clks_enable(core, 1);
 		if (ret)
 			return ret;
 
@@ -763,20 +768,18 @@ static int vdec_get_v4(struct device *dev)
 	if (!legacy_binding)
 		return 0;
 
-	return vcodec_clks_get(core, dev, core->vcodec0_clks,
-			       core->res->vcodec0_clks);
+	return vcodec_clks_get(core, dev, 0);
 }
 
 static void vdec_put_v4(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
-	unsigned int i;
 
 	if (!legacy_binding)
 		return;
 
-	for (i = 0; i < core->res->vcodec_clks_num; i++)
-		core->vcodec0_clks[i] = NULL;
+	core->vcodec_core_clks[0] = NULL;
+	core->vcodec_bus_clks[0] = NULL;
 }
 
 static int vdec_power_v4(struct device *dev, int on)
@@ -792,9 +795,9 @@ static int vdec_power_v4(struct device *dev, int on)
 		return ret;
 
 	if (on == POWER_ON)
-		ret = vcodec_clks_enable(core, core->vcodec0_clks);
+		ret = vcodec_clks_enable(core, 0);
 	else
-		vcodec_clks_disable(core, core->vcodec0_clks);
+		vcodec_clks_disable(core, 0);
 
 	vcodec_control_v4(core, VIDC_CORE_ID_1, false);
 
@@ -808,20 +811,18 @@ static int venc_get_v4(struct device *dev)
 	if (!legacy_binding)
 		return 0;
 
-	return vcodec_clks_get(core, dev, core->vcodec1_clks,
-			       core->res->vcodec1_clks);
+	return vcodec_clks_get(core, dev, 1);
 }
 
 static void venc_put_v4(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
-	unsigned int i;
 
 	if (!legacy_binding)
 		return;
 
-	for (i = 0; i < core->res->vcodec_clks_num; i++)
-		core->vcodec1_clks[i] = NULL;
+	core->vcodec_core_clks[1] = NULL;
+	core->vcodec_bus_clks[1] = NULL;
 }
 
 static int venc_power_v4(struct device *dev, int on)
@@ -837,9 +838,9 @@ static int venc_power_v4(struct device *dev, int on)
 		return ret;
 
 	if (on == POWER_ON)
-		ret = vcodec_clks_enable(core, core->vcodec1_clks);
+		ret = vcodec_clks_enable(core, 1);
 	else
-		vcodec_clks_disable(core, core->vcodec1_clks);
+		vcodec_clks_disable(core, 1);
 
 	vcodec_control_v4(core, VIDC_CORE_ID_2, false);
 
@@ -934,11 +935,11 @@ static int core_get_v4(struct venus_core *core)
 
 	dev_info(dev, "%s legacy binding\n", legacy_binding ? "" : "non");
 
-	ret = vcodec_clks_get(core, dev, core->vcodec0_clks, res->vcodec0_clks);
+	ret = vcodec_clks_get(core, dev, 0);
 	if (ret)
 		return ret;
 
-	ret = vcodec_clks_get(core, dev, core->vcodec1_clks, res->vcodec1_clks);
+	ret = vcodec_clks_get(core, dev, 1);
 	if (ret)
 		return ret;
 

-- 
2.44.0

Re: [PATCH v3 15/19] media: venus: pm_helpers: Simplify vcodec clock handling

[Dikshita Agarwal]

Hi Konrad,

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> Currently the infrastructure is set up for vast expandability, but
> it's far too complex for what is just 0-2 clocks. Categorize the
> clocks and simplify their getting.
> 
> One notable change is that vcodec clocks are switched to use
> devm_clk_get_optional, which will let us commonize the code further
> while leaving the burden of figuring out which SoCs need codec-specific
> clocks and which don't to the bindings checker.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c       |  18 ----
>  drivers/media/platform/qcom/venus/core.h       |  13 ++-
>  drivers/media/platform/qcom/venus/pm_helpers.c | 129 +++++++++++++------------
>  3 files changed, 71 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index e61aa863b7f7..1f4a86b1bd73 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -581,9 +581,6 @@ static const struct venus_resources msm8996_res = {
>  	.reg_tbl_size = ARRAY_SIZE(msm8996_reg_preset),
>  	.clks = {"core", "iface", "bus", "mbus" },
>  	.clks_num = 4,
> -	.vcodec0_clks = { "core" },
> -	.vcodec1_clks = { "core" },
> -	.vcodec_clks_num = 1,
>  	.max_load = 2563200,
>  	.hfi_version = HFI_VERSION_3XX,
>  	.dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
> @@ -636,9 +633,6 @@ static const struct venus_resources sdm660_res = {
>  	.bw_tbl_dec_size = ARRAY_SIZE(sdm660_bw_table_dec),
>  	.clks = {"core", "iface", "bus", "bus_throttle" },
>  	.clks_num = 4,
> -	.vcodec0_clks = { "vcodec0_core" },
> -	.vcodec1_clks = { "vcodec0_core" },
> -	.vcodec_clks_num = 1,
>  	.max_load = 1036800,
>  	.hfi_version = HFI_VERSION_3XX,
>  	.cp_size = 0x79000000,
> @@ -680,9 +674,6 @@ static const struct venus_resources sdm845_res = {
>  	.bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
>  	.clks = {"core", "iface", "bus" },
>  	.clks_num = 3,
> -	.vcodec0_clks = { "core", "bus" },
> -	.vcodec1_clks = { "core", "bus" },
> -	.vcodec_clks_num = 2,
>  	.max_load = 3110400,	/* 4096x2160@90 */
>  	.hfi_version = HFI_VERSION_4XX,
>  	.vpu_version = VPU_VERSION_AR50,
> @@ -699,9 +690,6 @@ static const struct venus_resources sdm845_res_v2 = {
>  	.bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
>  	.clks = {"core", "iface", "bus" },
>  	.clks_num = 3,
> -	.vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
> -	.vcodec1_clks = { "vcodec1_core", "vcodec1_bus" },
> -	.vcodec_clks_num = 2,
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
>  	.vcodec_pmdomains_num = 3,
>  	.opp_pmdomain = pd_names_cx,
> @@ -744,8 +732,6 @@ static const struct venus_resources sc7180_res = {
>  	.bw_tbl_dec_size = ARRAY_SIZE(sc7180_bw_table_dec),
>  	.clks = {"core", "iface", "bus" },
>  	.clks_num = 3,
> -	.vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
> -	.vcodec_clks_num = 2,
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
>  	.opp_pmdomain = pd_names_cx,
> @@ -796,8 +782,6 @@ static const struct venus_resources sm8250_res = {
>  	.clks_num = 2,
>  	.resets = { "bus", "core" },
>  	.resets_num = 2,
> -	.vcodec0_clks = { "vcodec0_core" },
> -	.vcodec_clks_num = 1,
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
>  	.opp_pmdomain = pd_names_mx,
> @@ -851,8 +835,6 @@ static const struct venus_resources sc7280_res = {
>  	.ubwc_conf = &sc7280_ubwc_config,
>  	.clks = {"core", "bus", "iface"},
>  	.clks_num = 3,
> -	.vcodec0_clks = {"vcodec_core", "vcodec_bus"},
> -	.vcodec_clks_num = 2,
>  	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
>  	.vcodec_pmdomains_num = 2,
>  	.opp_pmdomain = pd_names_cx,
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 19908f028441..b4c41dc0f8c7 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -24,9 +24,10 @@
>  #define VDBGFW	"VenusFW  : "
>  
>  #define VIDC_CLKS_NUM_MAX		4
> -#define VIDC_VCODEC_CLKS_NUM_MAX	2
>  #define VIDC_RESETS_NUM_MAX		2
>  
> +#define MAX_NUM_VCODECS			2
> +
>  extern int venus_fw_debug;
>  
>  struct freq_tbl {
> @@ -68,8 +69,6 @@ struct venus_resources {
>  	const struct hfi_ubwc_config *ubwc_conf;
>  	const char * const clks[VIDC_CLKS_NUM_MAX];
>  	unsigned int clks_num;
> -	const char * const vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> -	const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
>  	unsigned int vcodec_clks_num;
>  	const char **vcodec_pmdomains;
>  	unsigned int vcodec_pmdomains_num;
> @@ -123,8 +122,8 @@ struct venus_format {
>   * @aon_base:	AON base address
>   * @irq:		Venus irq
>   * @clks:	an array of struct clk pointers
> - * @vcodec0_clks: an array of vcodec0 struct clk pointers
> - * @vcodec1_clks: an array of vcodec1 struct clk pointers
> + * @vcodec_core_clks: an array of codec core clk pointers
> + * @vcodec_bus_clks: an array of codec bus clk pointers
>   * @video_path: an interconnect handle to video to/from memory path
>   * @cpucfg_path: an interconnect handle to cpu configuration path
>   * @has_opp_table: does OPP table exist
> @@ -176,8 +175,8 @@ struct venus_core {
>  	void __iomem *aon_base;
>  	int irq;
>  	struct clk *clks[VIDC_CLKS_NUM_MAX];
> -	struct clk *vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> -	struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> +	struct clk *vcodec_core_clks[MAX_NUM_VCODECS];
> +	struct clk *vcodec_bus_clks[MAX_NUM_VCODECS];
>  	struct icc_path *video_path;
>  	struct icc_path *cpucfg_path;
>  	bool has_opp_table;
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index d717e150b34f..583153bbb74e 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -110,67 +110,74 @@ static void core_clks_disable(struct venus_core *core)
>  
>  static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
>  {
> -	int ret;
> +	int i, ret;
>  
>  	ret = dev_pm_opp_set_rate(core->dev, freq);
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_set_rate(core->vcodec0_clks[0], freq);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_set_rate(core->vcodec1_clks[0], freq);
> -	if (ret)
> -		return ret;
> +	for (i = 0; i < MAX_NUM_VCODECS; i++) {
> +		ret = clk_set_rate(core->vcodec_core_clks[i], freq);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
>  
> -static int vcodec_clks_get(struct venus_core *core, struct device *dev,
> -			   struct clk **clks, const char * const *id)
> +static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
>  {
> -	const struct venus_resources *res = core->res;
> -	unsigned int i;
> +	char buf[13] = { 0 }; /* vcodecX_core\0 */
>  
> -	for (i = 0; i < res->vcodec_clks_num; i++) {
> -		if (!id[i])
> -			continue;
> -		clks[i] = devm_clk_get(dev, id[i]);
> -		if (IS_ERR(clks[i]))
> -			return PTR_ERR(clks[i]);
> +	/* Best we can do is 2 cores */
> +	if (id > MAX_NUM_VCODECS - 1) {
> +		dev_err(dev, "Got impossible vcodec id %u\n", id);
> +		return -EINVAL;
> +	};
> +
> +	snprintf(buf, sizeof(buf), "vcodec%u_core", id);
> +
> +	/* First try the non-legacy name */
> +	core->vcodec_core_clks[id] = devm_clk_get_optional(dev, buf);
> +	if (IS_ERR(core->vcodec_core_clks[id])) {
> +		/* Try again, with the legacy name */
> +		core->vcodec_core_clks[id] = devm_clk_get_optional(dev, "core");
> +		if (IS_ERR(core->vcodec_core_clks[id]))
> +			return PTR_ERR(core->vcodec_core_clks[id]);
> +	}
> +
> +	memset(buf, 0, sizeof(buf));
> +	snprintf(buf, sizeof(buf), "vcodec%u_bus", id);
> +
> +	core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, buf);
> +	if (IS_ERR(core->vcodec_bus_clks[id])) {
> +		core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, "bus");
> +		if (IS_ERR(core->vcodec_bus_clks[id]))
> +			return PTR_ERR(core->vcodec_bus_clks[id]);
>  	}
>  
>  	return 0;
>  }
>  
> -static int vcodec_clks_enable(struct venus_core *core, struct clk **clks)
> +static int vcodec_clks_enable(struct venus_core *core, u8 id)
>  {
> -	const struct venus_resources *res = core->res;
> -	unsigned int i;
>  	int ret;
>  
> -	for (i = 0; i < res->vcodec_clks_num; i++) {
> -		ret = clk_prepare_enable(clks[i]);
> -		if (ret)
> -			goto err;
> -	}
> +	ret = clk_prepare_enable(core->vcodec_core_clks[id]);
> +	if (ret)
> +		return ret;
>  
> -	return 0;
> -err:
> -	while (i--)
> -		clk_disable_unprepare(clks[i]);
> +	ret = clk_prepare_enable(core->vcodec_bus_clks[id]);
> +	if (ret)
> +		clk_disable_unprepare(core->vcodec_core_clks[id]);
>  
>  	return ret;
>  }
>  
> -static void vcodec_clks_disable(struct venus_core *core, struct clk **clks)
> +static void vcodec_clks_disable(struct venus_core *core, u8 id)
>  {
> -	const struct venus_resources *res = core->res;
> -	unsigned int i = res->vcodec_clks_num;
> -
> -	while (i--)
> -		clk_disable_unprepare(clks[i]);
> +	clk_disable_unprepare(core->vcodec_bus_clks[id]);
> +	clk_disable_unprepare(core->vcodec_core_clks[id]);
>  }
>  
>  static u32 load_per_instance(struct venus_inst *inst)
> @@ -343,8 +350,7 @@ static int vdec_get_v3(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
>  
> -	return vcodec_clks_get(core, dev, core->vcodec0_clks,
> -			       core->res->vcodec0_clks);
> +	return vcodec_clks_get(core, dev, 0);
>  }
>  
>  static int vdec_power_v3(struct device *dev, int on)
> @@ -355,9 +361,9 @@ static int vdec_power_v3(struct device *dev, int on)
>  	vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, true);
>  
>  	if (on == POWER_ON)
> -		ret = vcodec_clks_enable(core, core->vcodec0_clks);
> +		ret = vcodec_clks_enable(core, 0);
>  	else
> -		vcodec_clks_disable(core, core->vcodec0_clks);
> +		vcodec_clks_disable(core, 0);
>  
>  	vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, false);
>  
> @@ -368,8 +374,7 @@ static int venc_get_v3(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
>  
> -	return vcodec_clks_get(core, dev, core->vcodec1_clks,
> -			       core->res->vcodec1_clks);
> +	return vcodec_clks_get(core, dev, 1);
>  }
>  
>  static int venc_power_v3(struct device *dev, int on)
> @@ -380,9 +385,9 @@ static int venc_power_v3(struct device *dev, int on)
>  	vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, true);
>  
>  	if (on == POWER_ON)
> -		ret = vcodec_clks_enable(core, core->vcodec1_clks);
> +		ret = vcodec_clks_enable(core, 1);
>  	else
> -		vcodec_clks_disable(core, core->vcodec1_clks);
> +		vcodec_clks_disable(core, 1);
>  
>  	vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, false);
>  
> @@ -441,7 +446,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>  		if (ret)
>  			return ret;
>  
> -		vcodec_clks_disable(core, core->vcodec0_clks);
> +		vcodec_clks_disable(core, 0);
>  
>  		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
>  		if (ret)
> @@ -457,7 +462,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>  		if (ret)
>  			return ret;
>  
> -		vcodec_clks_disable(core, core->vcodec1_clks);
> +		vcodec_clks_disable(core, 1);
>  
>  		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
>  		if (ret)
> @@ -484,7 +489,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
>  		if (ret)
>  			return ret;
>  
> -		ret = vcodec_clks_enable(core, core->vcodec0_clks);
> +		ret = vcodec_clks_enable(core, 0);
>  		if (ret)
>  			return ret;
>  
> @@ -502,7 +507,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
>  		if (ret)
>  			return ret;
>  
> -		ret = vcodec_clks_enable(core, core->vcodec1_clks);
> +		ret = vcodec_clks_enable(core, 1);
>  		if (ret)
>  			return ret;
>  
> @@ -763,20 +768,18 @@ static int vdec_get_v4(struct device *dev)
>  	if (!legacy_binding)
>  		return 0;
>  
> -	return vcodec_clks_get(core, dev, core->vcodec0_clks,
> -			       core->res->vcodec0_clks);
> +	return vcodec_clks_get(core, dev, 0);
>  }
>  
>  static void vdec_put_v4(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> -	unsigned int i;
>  
>  	if (!legacy_binding)
>  		return;
>  
> -	for (i = 0; i < core->res->vcodec_clks_num; i++)
> -		core->vcodec0_clks[i] = NULL;
> +	core->vcodec_core_clks[0] = NULL;
> +	core->vcodec_bus_clks[0] = NULL;
>  }
>  
>  static int vdec_power_v4(struct device *dev, int on)
> @@ -792,9 +795,9 @@ static int vdec_power_v4(struct device *dev, int on)
>  		return ret;
>  
>  	if (on == POWER_ON)
> -		ret = vcodec_clks_enable(core, core->vcodec0_clks);
> +		ret = vcodec_clks_enable(core, 0);
>  	else
> -		vcodec_clks_disable(core, core->vcodec0_clks);
> +		vcodec_clks_disable(core, 0);
>  
>  	vcodec_control_v4(core, VIDC_CORE_ID_1, false);
>  
> @@ -808,20 +811,18 @@ static int venc_get_v4(struct device *dev)
>  	if (!legacy_binding)
>  		return 0;
>  
> -	return vcodec_clks_get(core, dev, core->vcodec1_clks,
> -			       core->res->vcodec1_clks);
> +	return vcodec_clks_get(core, dev, 1);
>  }
>  
>  static void venc_put_v4(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> -	unsigned int i;
>  
>  	if (!legacy_binding)
>  		return;
>  
> -	for (i = 0; i < core->res->vcodec_clks_num; i++)
> -		core->vcodec1_clks[i] = NULL;
> +	core->vcodec_core_clks[1] = NULL;
> +	core->vcodec_bus_clks[1] = NULL;
>  }
>  
>  static int venc_power_v4(struct device *dev, int on)
> @@ -837,9 +838,9 @@ static int venc_power_v4(struct device *dev, int on)
>  		return ret;
>  
>  	if (on == POWER_ON)
> -		ret = vcodec_clks_enable(core, core->vcodec1_clks);
> +		ret = vcodec_clks_enable(core, 1);
>  	else
> -		vcodec_clks_disable(core, core->vcodec1_clks);
> +		vcodec_clks_disable(core, 1);
>  
>  	vcodec_control_v4(core, VIDC_CORE_ID_2, false);
>  
> @@ -934,11 +935,11 @@ static int core_get_v4(struct venus_core *core)
>  
>  	dev_info(dev, "%s legacy binding\n", legacy_binding ? "" : "non");
>  
> -	ret = vcodec_clks_get(core, dev, core->vcodec0_clks, res->vcodec0_clks);
> +	ret = vcodec_clks_get(core, dev, 0);
>  	if (ret)
>  		return ret;
>  
> -	ret = vcodec_clks_get(core, dev, core->vcodec1_clks, res->vcodec1_clks);
> +	ret = vcodec_clks_get(core, dev, 1);
>  	if (ret)
>  		return ret;
>  
> 
It is better to keep the information about all clocks being used for a
particular SOC in one place, like its currently captured in resource
structure. Leaving core clocks in resources and moving vcodec clocks to
other places makes it less readable.
Also let's say, if in future, a new clock is introcued, we will need to
introduce a new array additional to vcodec_core, vcodec_bus.

Thanks,
Dikshita

[PATCH v3 16/19] media: venus: pm_helpers: Commonize getting clocks and GenPDs

[Konrad Dybcio]

As has been the story with the past few commits, much of the resource
acquisition logic is totally identical between different generations
and there's no good reason to invent a new function for each one.

Commonize core_get() and rename it to venus_get_resources() to be more
meaningful.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       | 8 +++-----
 drivers/media/platform/qcom/venus/pm_helpers.c | 5 +----
 drivers/media/platform/qcom/venus/pm_helpers.h | 3 +--
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 1f4a86b1bd73..6914fa991efb 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -334,11 +334,9 @@ static int venus_probe(struct platform_device *pdev)
 			return PTR_ERR(core->resets[i]);
 	}
 
-	if (core->pm_ops->core_get) {
-		ret = core->pm_ops->core_get(core);
-		if (ret)
-			return ret;
-	}
+	ret = venus_get_resources(core);
+	if (ret)
+		return ret;
 
 	ret = dma_set_mask_and_coherent(dev, res->dma_mask);
 	if (ret)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 583153bbb74e..ba5199d9e5c9 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -326,7 +326,6 @@ static int load_scale_v1(struct venus_inst *inst)
 }
 
 static const struct venus_pm_ops pm_ops_v1 = {
-	.core_get = venus_clks_get,
 	.load_scale = load_scale_v1,
 };
 
@@ -395,7 +394,6 @@ static int venc_power_v3(struct device *dev, int on)
 }
 
 static const struct venus_pm_ops pm_ops_v3 = {
-	.core_get = venus_clks_get,
 	.vdec_get = vdec_get_v3,
 	.vdec_power = vdec_power_v3,
 	.venc_get = venc_get_v3,
@@ -920,7 +918,7 @@ static int core_resets_reset(struct venus_core *core)
 	return ret;
 }
 
-static int core_get_v4(struct venus_core *core)
+int venus_get_resources(struct venus_core *core)
 {
 	struct device *dev = core->dev;
 	const struct venus_resources *res = core->res;
@@ -1109,7 +1107,6 @@ static int load_scale_v4(struct venus_inst *inst)
 }
 
 static const struct venus_pm_ops pm_ops_v4 = {
-	.core_get = core_get_v4,
 	.vdec_get = vdec_get_v4,
 	.vdec_put = vdec_put_v4,
 	.vdec_power = vdec_power_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index 3014b39aa6e3..7a55a55029f3 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -10,8 +10,6 @@ struct venus_core;
 #define POWER_OFF	0
 
 struct venus_pm_ops {
-	int (*core_get)(struct venus_core *core);
-
 	int (*vdec_get)(struct device *dev);
 	void (*vdec_put)(struct device *dev);
 	int (*vdec_power)(struct device *dev, int on);
@@ -28,6 +26,7 @@ struct venus_pm_ops {
 const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
 int venus_core_power(struct venus_core *core, int on);
 void vcodec_domains_put(struct venus_core *core);
+int venus_get_resources(struct venus_core *core);
 
 static inline int venus_pm_load_scale(struct venus_inst *inst)
 {

-- 
2.44.0

Re: [PATCH v3 16/19] media: venus: pm_helpers: Commonize getting clocks and GenPDs

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> As has been the story with the past few commits, much of the resource
> acquisition logic is totally identical between different generations
> and there's no good reason to invent a new function for each one.
> 
> Commonize core_get() and rename it to venus_get_resources() to be more
> meaningful.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c       | 8 +++-----
>  drivers/media/platform/qcom/venus/pm_helpers.c | 5 +----
>  drivers/media/platform/qcom/venus/pm_helpers.h | 3 +--
>  3 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 1f4a86b1bd73..6914fa991efb 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -334,11 +334,9 @@ static int venus_probe(struct platform_device *pdev)
>  			return PTR_ERR(core->resets[i]);
>  	}
>  
> -	if (core->pm_ops->core_get) {
> -		ret = core->pm_ops->core_get(core);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = venus_get_resources(core);
> +	if (ret)
> +		return ret;
>  
>  	ret = dma_set_mask_and_coherent(dev, res->dma_mask);
>  	if (ret)
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 583153bbb74e..ba5199d9e5c9 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -326,7 +326,6 @@ static int load_scale_v1(struct venus_inst *inst)
>  }
>  
>  static const struct venus_pm_ops pm_ops_v1 = {
> -	.core_get = venus_clks_get,
>  	.load_scale = load_scale_v1,
>  };
>  
> @@ -395,7 +394,6 @@ static int venc_power_v3(struct device *dev, int on)
>  }
>  
>  static const struct venus_pm_ops pm_ops_v3 = {
> -	.core_get = venus_clks_get,
>  	.vdec_get = vdec_get_v3,
>  	.vdec_power = vdec_power_v3,
>  	.venc_get = venc_get_v3,
> @@ -920,7 +918,7 @@ static int core_resets_reset(struct venus_core *core)
>  	return ret;
>  }
>  
> -static int core_get_v4(struct venus_core *core)
> +int venus_get_resources(struct venus_core *core)
>  {
>  	struct device *dev = core->dev;
>  	const struct venus_resources *res = core->res;
> @@ -1109,7 +1107,6 @@ static int load_scale_v4(struct venus_inst *inst)

With this change vcodec_clks_get will be called for legacy targets as well
in venus probe itself, which is currently being called in vdec/venc_probe
for v1 and v3 targets.
This needs to be validated on legacy v1 and v3 devices.

Thanks,
Dikshita
>  }
>  
>  static const struct venus_pm_ops pm_ops_v4 = {
> -	.core_get = core_get_v4,
>  	.vdec_get = vdec_get_v4,
>  	.vdec_put = vdec_put_v4,
>  	.vdec_power = vdec_power_v4,
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
> index 3014b39aa6e3..7a55a55029f3 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.h
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.h
> @@ -10,8 +10,6 @@ struct venus_core;
>  #define POWER_OFF	0
>  
>  struct venus_pm_ops {
> -	int (*core_get)(struct venus_core *core);
> -
>  	int (*vdec_get)(struct device *dev);
>  	void (*vdec_put)(struct device *dev);
>  	int (*vdec_power)(struct device *dev, int on);
> @@ -28,6 +26,7 @@ struct venus_pm_ops {
>  const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
>  int venus_core_power(struct venus_core *core, int on);
>  void vcodec_domains_put(struct venus_core *core);
> +int venus_get_resources(struct venus_core *core);
>  
>  static inline int venus_pm_load_scale(struct venus_inst *inst)
>  {
> 

[PATCH v3 17/19] media: venus: pm_helpers: Commonize vdec_get()

[Konrad Dybcio]

This function can be very easily commonized between the supported gens.
Do so!

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 22 ++--------------------
 drivers/media/platform/qcom/venus/pm_helpers.h |  2 +-
 drivers/media/platform/qcom/venus/vdec.c       |  9 +++++++--
 3 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ba5199d9e5c9..3818384bae10 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -125,7 +125,7 @@ static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
 	return 0;
 }
 
-static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
+int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
 {
 	char buf[13] = { 0 }; /* vcodecX_core\0 */
 
@@ -158,6 +158,7 @@ static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(vcodec_clks_get);
 
 static int vcodec_clks_enable(struct venus_core *core, u8 id)
 {
@@ -345,13 +346,6 @@ vcodec_control_v3(struct venus_core *core, u32 session_type, bool enable)
 		writel(1, ctrl);
 }
 
-static int vdec_get_v3(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-
-	return vcodec_clks_get(core, dev, 0);
-}
-
 static int vdec_power_v3(struct device *dev, int on)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
@@ -394,7 +388,6 @@ static int venc_power_v3(struct device *dev, int on)
 }
 
 static const struct venus_pm_ops pm_ops_v3 = {
-	.vdec_get = vdec_get_v3,
 	.vdec_power = vdec_power_v3,
 	.venc_get = venc_get_v3,
 	.venc_power = venc_power_v3,
@@ -759,16 +752,6 @@ static int coreid_power_v4(struct venus_inst *inst, int on)
 	return ret;
 }
 
-static int vdec_get_v4(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-
-	if (!legacy_binding)
-		return 0;
-
-	return vcodec_clks_get(core, dev, 0);
-}
-
 static void vdec_put_v4(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
@@ -1107,7 +1090,6 @@ static int load_scale_v4(struct venus_inst *inst)
 }
 
 static const struct venus_pm_ops pm_ops_v4 = {
-	.vdec_get = vdec_get_v4,
 	.vdec_put = vdec_put_v4,
 	.vdec_power = vdec_power_v4,
 	.venc_get = venc_get_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index 7a55a55029f3..4afc57dac865 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -10,7 +10,6 @@ struct venus_core;
 #define POWER_OFF	0
 
 struct venus_pm_ops {
-	int (*vdec_get)(struct device *dev);
 	void (*vdec_put)(struct device *dev);
 	int (*vdec_power)(struct device *dev, int on);
 
@@ -27,6 +26,7 @@ const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
 int venus_core_power(struct venus_core *core, int on);
 void vcodec_domains_put(struct venus_core *core);
 int venus_get_resources(struct venus_core *core);
+int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id);
 
 static inline int venus_pm_load_scale(struct venus_inst *inst)
 {
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 29130a9441e7..d127311100cd 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1788,8 +1788,13 @@ static int vdec_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, core);
 
-	if (core->pm_ops->vdec_get) {
-		ret = core->pm_ops->vdec_get(dev);
+	/*
+	 * If the vcodec core clock is missing by now, it either doesn't exist
+	 * (8916) or deprecated bindings with pre-assigned core functions and
+	 * resources under the decoder node are in use.
+	 */
+	if (!core->vcodec_core_clks[0]) {
+		ret = vcodec_clks_get(core, dev, 0);
 		if (ret)
 			return ret;
 	}

-- 
2.44.0

Re: [PATCH v3 17/19] media: venus: pm_helpers: Commonize vdec_get()

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> This function can be very easily commonized between the supported gens.
> Do so!
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 22 ++--------------------
>  drivers/media/platform/qcom/venus/pm_helpers.h |  2 +-
>  drivers/media/platform/qcom/venus/vdec.c       |  9 +++++++--
>  3 files changed, 10 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index ba5199d9e5c9..3818384bae10 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -125,7 +125,7 @@ static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
>  	return 0;
>  }
>  
> -static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
> +int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
>  {
>  	char buf[13] = { 0 }; /* vcodecX_core\0 */
>  
> @@ -158,6 +158,7 @@ static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(vcodec_clks_get);
>  
>  static int vcodec_clks_enable(struct venus_core *core, u8 id)
>  {
> @@ -345,13 +346,6 @@ vcodec_control_v3(struct venus_core *core, u32 session_type, bool enable)
>  		writel(1, ctrl);
>  }
>  
> -static int vdec_get_v3(struct device *dev)
> -{
> -	struct venus_core *core = dev_get_drvdata(dev);
> -
> -	return vcodec_clks_get(core, dev, 0);
> -}
> -
>  static int vdec_power_v3(struct device *dev, int on)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> @@ -394,7 +388,6 @@ static int venc_power_v3(struct device *dev, int on)
>  }
>  
>  static const struct venus_pm_ops pm_ops_v3 = {
> -	.vdec_get = vdec_get_v3,
>  	.vdec_power = vdec_power_v3,
>  	.venc_get = venc_get_v3,
>  	.venc_power = venc_power_v3,
> @@ -759,16 +752,6 @@ static int coreid_power_v4(struct venus_inst *inst, int on)
>  	return ret;
>  }
>  
> -static int vdec_get_v4(struct device *dev)
> -{
> -	struct venus_core *core = dev_get_drvdata(dev);
> -
> -	if (!legacy_binding)
> -		return 0;
> -
> -	return vcodec_clks_get(core, dev, 0);
> -}
> -
>  static void vdec_put_v4(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> @@ -1107,7 +1090,6 @@ static int load_scale_v4(struct venus_inst *inst)
>  }
>  
>  static const struct venus_pm_ops pm_ops_v4 = {
> -	.vdec_get = vdec_get_v4,
>  	.vdec_put = vdec_put_v4,
>  	.vdec_power = vdec_power_v4,
>  	.venc_get = venc_get_v4,
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
> index 7a55a55029f3..4afc57dac865 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.h
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.h
> @@ -10,7 +10,6 @@ struct venus_core;
>  #define POWER_OFF	0
>  
>  struct venus_pm_ops {
> -	int (*vdec_get)(struct device *dev);
>  	void (*vdec_put)(struct device *dev);
>  	int (*vdec_power)(struct device *dev, int on);
>  
> @@ -27,6 +26,7 @@ const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
>  int venus_core_power(struct venus_core *core, int on);
>  void vcodec_domains_put(struct venus_core *core);
>  int venus_get_resources(struct venus_core *core);
> +int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id);
>  
>  static inline int venus_pm_load_scale(struct venus_inst *inst)
>  {
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 29130a9441e7..d127311100cd 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -1788,8 +1788,13 @@ static int vdec_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, core);
>  
> -	if (core->pm_ops->vdec_get) {
> -		ret = core->pm_ops->vdec_get(dev);
> +	/*
> +	 * If the vcodec core clock is missing by now, it either doesn't exist
> +	 * (8916) or deprecated bindings with pre-assigned core functions and
> +	 * resources under the decoder node are in use.
> +	 */
This comment is not very clear to me, could you please elaborate more on
"deprecated bindings with pre-assigned core functions and
 resources under the decoder node are in use"
> +	if (!core->vcodec_core_clks[0]) {
> +		ret = vcodec_clks_get(core, dev, 0);
>  		if (ret)
>  			return ret;
>  	}
> 
Calling vcodec_clks_get directly instead of vdec_get op looks fine to me,
but this depends on the previous patch, so will need some changes.

Thanks,
Dikshita

[PATCH v3 18/19] media: venus: pm_helpers: Commonize venc_get()

[Konrad Dybcio]

This function can be very easily commonized between the supported gens.
Do so!

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 19 -------------------
 drivers/media/platform/qcom/venus/pm_helpers.h |  1 -
 drivers/media/platform/qcom/venus/venc.c       |  9 +++++++--
 3 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 3818384bae10..5d174b83926b 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -363,13 +363,6 @@ static int vdec_power_v3(struct device *dev, int on)
 	return ret;
 }
 
-static int venc_get_v3(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-
-	return vcodec_clks_get(core, dev, 1);
-}
-
 static int venc_power_v3(struct device *dev, int on)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
@@ -389,7 +382,6 @@ static int venc_power_v3(struct device *dev, int on)
 
 static const struct venus_pm_ops pm_ops_v3 = {
 	.vdec_power = vdec_power_v3,
-	.venc_get = venc_get_v3,
 	.venc_power = venc_power_v3,
 	.load_scale = load_scale_v1,
 };
@@ -785,16 +777,6 @@ static int vdec_power_v4(struct device *dev, int on)
 	return ret;
 }
 
-static int venc_get_v4(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-
-	if (!legacy_binding)
-		return 0;
-
-	return vcodec_clks_get(core, dev, 1);
-}
-
 static void venc_put_v4(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
@@ -1092,7 +1074,6 @@ static int load_scale_v4(struct venus_inst *inst)
 static const struct venus_pm_ops pm_ops_v4 = {
 	.vdec_put = vdec_put_v4,
 	.vdec_power = vdec_power_v4,
-	.venc_get = venc_get_v4,
 	.venc_put = venc_put_v4,
 	.venc_power = venc_power_v4,
 	.coreid_power = coreid_power_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index 4afc57dac865..cbf54e6c6eab 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -13,7 +13,6 @@ struct venus_pm_ops {
 	void (*vdec_put)(struct device *dev);
 	int (*vdec_power)(struct device *dev, int on);
 
-	int (*venc_get)(struct device *dev);
 	void (*venc_put)(struct device *dev);
 	int (*venc_power)(struct device *dev, int on);
 
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 3ec2fb8d9fab..d17aeba74b49 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1557,8 +1557,13 @@ static int venc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, core);
 
-	if (core->pm_ops->venc_get) {
-		ret = core->pm_ops->venc_get(dev);
+	/*
+	 * If the vcodec core clock is missing by now, it either doesn't exist
+	 * (8916) or deprecated bindings with pre-assigned core functions and
+	 * resources under the decoder node are in use.
+	 */
+	if (!core->vcodec_core_clks[1]) {
+		ret = vcodec_clks_get(core, dev, 1);
 		if (ret)
 			return ret;
 	}

-- 
2.44.0

Re: [PATCH v3 18/19] media: venus: pm_helpers: Commonize venc_get()

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> This function can be very easily commonized between the supported gens.
> Do so!
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 19 -------------------
>  drivers/media/platform/qcom/venus/pm_helpers.h |  1 -
>  drivers/media/platform/qcom/venus/venc.c       |  9 +++++++--
>  3 files changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 3818384bae10..5d174b83926b 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -363,13 +363,6 @@ static int vdec_power_v3(struct device *dev, int on)
>  	return ret;
>  }
>  
> -static int venc_get_v3(struct device *dev)
> -{
> -	struct venus_core *core = dev_get_drvdata(dev);
> -
> -	return vcodec_clks_get(core, dev, 1);
> -}
> -
>  static int venc_power_v3(struct device *dev, int on)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> @@ -389,7 +382,6 @@ static int venc_power_v3(struct device *dev, int on)
>  
>  static const struct venus_pm_ops pm_ops_v3 = {
>  	.vdec_power = vdec_power_v3,
> -	.venc_get = venc_get_v3,
>  	.venc_power = venc_power_v3,
>  	.load_scale = load_scale_v1,
>  };
> @@ -785,16 +777,6 @@ static int vdec_power_v4(struct device *dev, int on)
>  	return ret;
>  }
>  
> -static int venc_get_v4(struct device *dev)
> -{
> -	struct venus_core *core = dev_get_drvdata(dev);
> -
> -	if (!legacy_binding)
> -		return 0;
> -
> -	return vcodec_clks_get(core, dev, 1);
> -}
> -
>  static void venc_put_v4(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> @@ -1092,7 +1074,6 @@ static int load_scale_v4(struct venus_inst *inst)
>  static const struct venus_pm_ops pm_ops_v4 = {
>  	.vdec_put = vdec_put_v4,
>  	.vdec_power = vdec_power_v4,
> -	.venc_get = venc_get_v4,
>  	.venc_put = venc_put_v4,
>  	.venc_power = venc_power_v4,
>  	.coreid_power = coreid_power_v4,
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
> index 4afc57dac865..cbf54e6c6eab 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.h
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.h
> @@ -13,7 +13,6 @@ struct venus_pm_ops {
>  	void (*vdec_put)(struct device *dev);
>  	int (*vdec_power)(struct device *dev, int on);
>  
> -	int (*venc_get)(struct device *dev);
>  	void (*venc_put)(struct device *dev);
>  	int (*venc_power)(struct device *dev, int on);
>  
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 3ec2fb8d9fab..d17aeba74b49 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -1557,8 +1557,13 @@ static int venc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, core);
>  
> -	if (core->pm_ops->venc_get) {
> -		ret = core->pm_ops->venc_get(dev);
> +	/*
> +	 * If the vcodec core clock is missing by now, it either doesn't exist
> +	 * (8916) or deprecated bindings with pre-assigned core functions and
> +	 * resources under the decoder node are in use.
> +	 */
> +	if (!core->vcodec_core_clks[1]) {
> +		ret = vcodec_clks_get(core, dev, 1);
>  		if (ret)
>  			return ret;
>  	}
> 
same comment as previous patch.

Thanks,
Dikshita

[PATCH v3 19/19] media: venus: pm_helpers: Use reset_bulk API

[Konrad Dybcio]

All of the resets are toggled together. Use the bulk api to save on some
code complexity.

The delay between resets is now correctly determined by the reset
framework.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       | 15 ++++++++++-----
 drivers/media/platform/qcom/venus/core.h       |  4 ++--
 drivers/media/platform/qcom/venus/pm_helpers.c | 15 +++------------
 3 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 6914fa991efb..f1762c725a11 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -328,11 +328,16 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	for (i = 0; i < res->resets_num; i++) {
-		core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
-		if (IS_ERR(core->resets[i]))
-			return PTR_ERR(core->resets[i]);
-	}
+	core->resets = devm_kcalloc(dev, res->resets_num, sizeof(*core->resets), GFP_KERNEL);
+	if (res->resets_num && !core->resets)
+		return -ENOMEM;
+
+	for (i = 0; i < res->resets_num; i++)
+		core->resets[i].id = res->resets[i];
+
+	ret = devm_reset_control_bulk_get_exclusive(dev, res->resets_num, core->resets);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get resets\n");
 
 	ret = venus_get_resources(core);
 	if (ret)
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index b4c41dc0f8c7..287bcf905057 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -130,7 +130,7 @@ struct venus_format {
  * @pmdomains:	a pointer to a list of pmdomains
  * @opp_dl_venus: an device-link for device OPP
  * @opp_pmdomain: an OPP power-domain
- * @resets: an array of reset signals
+ * @resets: a reset_control_bulk_data array of hardware reset signals
  * @vdev_dec:	a reference to video device structure for decoder instances
  * @vdev_enc:	a reference to video device structure for encoder instances
  * @v4l2_dev:	a holder for v4l2 device structure
@@ -183,7 +183,7 @@ struct venus_core {
 	struct dev_pm_domain_list *pmdomains;
 	struct device_link *opp_dl_venus;
 	struct device *opp_pmdomain;
-	struct reset_control *resets[VIDC_RESETS_NUM_MAX];
+	struct reset_control_bulk_data *resets;
 	struct video_device *vdev_dec;
 	struct video_device *vdev_enc;
 	struct v4l2_device v4l2_dev;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 5d174b83926b..7690f66d1184 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -865,21 +865,12 @@ void vcodec_domains_put(struct venus_core *core)
 static int core_resets_reset(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;
-	unsigned int i;
 	int ret;
 
-	for (i = 0; i < res->resets_num; i++) {
-		ret = reset_control_assert(core->resets[i]);
-		if (ret)
-			goto err;
-
-		usleep_range(150, 250);
-		ret = reset_control_deassert(core->resets[i]);
-		if (ret)
-			goto err;
-	}
+	ret = reset_control_bulk_reset(res->resets_num, core->resets);
+	if (ret)
+		dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
 
-err:
 	return ret;
 }
 

-- 
2.44.0

Re: [PATCH v3 19/19] media: venus: pm_helpers: Use reset_bulk API

[Dikshita Agarwal]

On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> All of the resets are toggled together. Use the bulk api to save on some
> code complexity.
> 
> The delay between resets is now correctly determined by the reset
> framework.
> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.c       | 15 ++++++++++-----
>  drivers/media/platform/qcom/venus/core.h       |  4 ++--
>  drivers/media/platform/qcom/venus/pm_helpers.c | 15 +++------------
>  3 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 6914fa991efb..f1762c725a11 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -328,11 +328,16 @@ static int venus_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < res->resets_num; i++) {
> -		core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
> -		if (IS_ERR(core->resets[i]))
> -			return PTR_ERR(core->resets[i]);
> -	}
> +	core->resets = devm_kcalloc(dev, res->resets_num, sizeof(*core->resets), GFP_KERNEL);
> +	if (res->resets_num && !core->resets)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < res->resets_num; i++)
> +		core->resets[i].id = res->resets[i];
> +
> +	ret = devm_reset_control_bulk_get_exclusive(dev, res->resets_num, core->resets);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get resets\n");
>  
>  	ret = venus_get_resources(core);
>  	if (ret)
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index b4c41dc0f8c7..287bcf905057 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -130,7 +130,7 @@ struct venus_format {
>   * @pmdomains:	a pointer to a list of pmdomains
>   * @opp_dl_venus: an device-link for device OPP
>   * @opp_pmdomain: an OPP power-domain
> - * @resets: an array of reset signals
> + * @resets: a reset_control_bulk_data array of hardware reset signals
>   * @vdev_dec:	a reference to video device structure for decoder instances
>   * @vdev_enc:	a reference to video device structure for encoder instances
>   * @v4l2_dev:	a holder for v4l2 device structure
> @@ -183,7 +183,7 @@ struct venus_core {
>  	struct dev_pm_domain_list *pmdomains;
>  	struct device_link *opp_dl_venus;
>  	struct device *opp_pmdomain;
> -	struct reset_control *resets[VIDC_RESETS_NUM_MAX];
> +	struct reset_control_bulk_data *resets;
>  	struct video_device *vdev_dec;
>  	struct video_device *vdev_enc;
>  	struct v4l2_device v4l2_dev;
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 5d174b83926b..7690f66d1184 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -865,21 +865,12 @@ void vcodec_domains_put(struct venus_core *core)
>  static int core_resets_reset(struct venus_core *core)
>  {
>  	const struct venus_resources *res = core->res;
> -	unsigned int i;
>  	int ret;
>  
> -	for (i = 0; i < res->resets_num; i++) {
> -		ret = reset_control_assert(core->resets[i]);
> -		if (ret)
> -			goto err;
> -
> -		usleep_range(150, 250);
> -		ret = reset_control_deassert(core->resets[i]);
> -		if (ret)
> -			goto err;
> -	}
> +	ret = reset_control_bulk_reset(res->resets_num, core->resets);
> +	if (ret)
> +		dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
>  
> -err:
>  	return ret;
>  }
>  
> 
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>