[PATCH v2 00/18] Venus QoL / maintainability fixes

[PATCH v2 00/18] Venus QoL / maintainability fixes

[Konrad Dybcio]

v1 -> v2:
- Move "Write to VIDC_CTRL_INIT after unmasking interrupts" up and add
  a Fixes tag & Cc stable
- Reword the comment in "Correct IS_V6() checks"
- Move up "media: venus: Remap bufreq fields on HFI6XX", add Fixes and
  Cc stable
- Use better English in "Use newly-introduced hfi_buffer_requirements
  accessors" commit message
- Mention "Restrict writing SCIACMDARG3 to Venus V1/V2" doesn't seem to
  regress SM8250 in the commit message
- Pick up tags (note: I capitalized the R in Dikshita's 'reviewed-by'
  and removed one occurrence of random '**' to make sure review tools
  like b4 don't go crazy)
- Handle AR50_LITE in "Assign registers based on VPU version"
- Drop /* VPUn */ comments, they're invalid as explained by Vikash
- Take a different approach to the sys_idle problem in patch 1

v1: https://lore.kernel.org/r/20230228-topic-venus-v1-0-58c2c88384e9@linaro.org

Currently upstream assumes all (well, almost all - see 7280 or CrOS
specific checks) Venus implementations using the same version of the
Hardware Firmware Interface can be treated the same way. This is
however not the case.

This series tries to introduce the groundwork to start differentiating
them based on the VPU (Video Processing Unit) hardware type, fixes a
couple of issues that were an effect of that generalized assumption
and lays the foundation for supporting 8150 (IRIS1) and SM6115/QCM2290
(AR50 Lite), which will hopefully come soon.

Tested on 8250, but pretty please test it on your boards too!

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (18):
      media: venus: hfi_venus: Only consider sys_idle_indicator on V1
      media: venus: hfi_venus: Write to VIDC_CTRL_INIT after unmasking interrupts
      media: venus: Remap bufreq fields on HFI6XX
      media: venus: Introduce VPU version distinction
      media: venus: Add vpu_version to most SoCs
      media: venus: firmware: Leave a clue for homegrown porters
      media: venus: hfi_venus: Sanitize venus_boot_core() per-VPU-version
      media: venus: core: Assign registers based on VPU version
      media: venus: hfi_venus: Fix version checks in venus_halt_axi()
      media: venus: hfi_venus: Fix version checks in venus_isr()
      media: venus: hfi_venus: Fix version check in venus_cpu_and_video_core_idle()
      media: venus: hfi_venus: Fix version check in venus_cpu_idle_and_pc_ready()
      media: venus: firmware: Correct IS_V6() checks
      media: venus: hfi_platform: Check vpu_version instead of device compatible
      media: venus: vdec: Fix version check in vdec_set_work_route()
      media: venus: Introduce accessors for remapped hfi_buffer_reqs members
      media: venus: Use newly-introduced hfi_buffer_requirements accessors
      media: venus: hfi_venus: Restrict writing SCIACMDARG3 to Venus V1/V2

 drivers/media/platform/qcom/venus/core.c           | 11 ++--
 drivers/media/platform/qcom/venus/core.h           | 15 ++++++
 drivers/media/platform/qcom/venus/firmware.c       | 19 +++++--
 drivers/media/platform/qcom/venus/helpers.c        |  7 +--
 drivers/media/platform/qcom/venus/hfi_helper.h     | 61 +++++++++++++++++++---
 drivers/media/platform/qcom/venus/hfi_msgs.c       |  2 +-
 .../media/platform/qcom/venus/hfi_plat_bufs_v6.c   | 22 ++++----
 drivers/media/platform/qcom/venus/hfi_platform.c   |  2 +-
 drivers/media/platform/qcom/venus/hfi_venus.c      | 45 ++++++++--------
 drivers/media/platform/qcom/venus/vdec.c           | 10 ++--
 drivers/media/platform/qcom/venus/vdec_ctrls.c     |  2 +-
 drivers/media/platform/qcom/venus/venc.c           |  4 +-
 drivers/media/platform/qcom/venus/venc_ctrls.c     |  2 +-
 13 files changed, 138 insertions(+), 64 deletions(-)
---
base-commit: 92e815cf07ed24ee1c51b122f24ffcf2964b4b13
change-id: 20230228-topic-venus-70ea3bc76688

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

[PATCH v2 01/18] media: venus: hfi_venus: Only consider sys_idle_indicator on V1

[Konrad Dybcio]

As per information from Qualcomm [1], this property is not really
supported beyond msm8916 (HFI V1) and some newer HFI versions really
dislike receiving it, going as far as crashing the device.

Only consider toggling it (via the module option) on HFIV1.
While at it, get rid of the global static variable (which defaulted
to zero) which was never explicitly assigned to for V1.

Note: [1] is a reply to the actual message in question, as lore did not
properly receive some of the emails..

[1] https://lore.kernel.org/lkml/955cd520-3881-0c22-d818-13fe9a47e124@linaro.org/
Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 2ad40b3945b0..bff435abd59b 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -131,7 +131,6 @@ struct venus_hfi_device {
 
 static bool venus_pkt_debug;
 int venus_fw_debug = HFI_DEBUG_MSG_ERROR | HFI_DEBUG_MSG_FATAL;
-static bool venus_sys_idle_indicator;
 static bool venus_fw_low_power_mode = true;
 static int venus_hw_rsp_timeout = 1000;
 static bool venus_fw_coverage;
@@ -947,17 +946,12 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
 	if (ret)
 		dev_warn(dev, "setting fw debug msg ON failed (%d)\n", ret);
 
-	/*
-	 * Idle indicator is disabled by default on some 4xx firmware versions,
-	 * enable it explicitly in order to make suspend functional by checking
-	 * WFI (wait-for-interrupt) bit.
-	 */
-	if (IS_V4(hdev->core) || IS_V6(hdev->core))
-		venus_sys_idle_indicator = true;
-
-	ret = venus_sys_set_idle_message(hdev, venus_sys_idle_indicator);
-	if (ret)
-		dev_warn(dev, "setting idle response ON failed (%d)\n", ret);
+	/* HFI_PROPERTY_SYS_IDLE_INDICATOR is not supported beyond 8916 (HFI V1) */
+	if (IS_V1(hdev->core)) {
+		ret = venus_sys_set_idle_message(hdev, false);
+		if (ret)
+			dev_warn(dev, "setting idle response ON failed (%d)\n", ret);
+	}
 
 	ret = venus_sys_set_power_control(hdev, venus_fw_low_power_mode);
 	if (ret)

-- 
2.40.1

Re: [PATCH v2 01/18] media: venus: hfi_venus: Only consider sys_idle_indicator on V1

[Vikash Garodia]

On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
> As per information from Qualcomm [1], this property is not really
> supported beyond msm8916 (HFI V1) and some newer HFI versions really
> dislike receiving it, going as far as crashing the device.
>
> Only consider toggling it (via the module option) on HFIV1.
> While at it, get rid of the global static variable (which defaulted
> to zero) which was never explicitly assigned to for V1.
>
> Note: [1] is a reply to the actual message in question, as lore did not
> properly receive some of the emails..
>
> [1] https://lore.kernel.org/lkml/955cd520-3881-0c22-d818-13fe9a47e124@linaro.org/
> Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 2ad40b3945b0..bff435abd59b 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -131,7 +131,6 @@ struct venus_hfi_device {
>   
>   static bool venus_pkt_debug;
>   int venus_fw_debug = HFI_DEBUG_MSG_ERROR | HFI_DEBUG_MSG_FATAL;
> -static bool venus_sys_idle_indicator;
>   static bool venus_fw_low_power_mode = true;
>   static int venus_hw_rsp_timeout = 1000;
>   static bool venus_fw_coverage;
> @@ -947,17 +946,12 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
>   	if (ret)
>   		dev_warn(dev, "setting fw debug msg ON failed (%d)\n", ret);
>   
> -	/*
> -	 * Idle indicator is disabled by default on some 4xx firmware versions,
> -	 * enable it explicitly in order to make suspend functional by checking
> -	 * WFI (wait-for-interrupt) bit.
> -	 */
> -	if (IS_V4(hdev->core) || IS_V6(hdev->core))
> -		venus_sys_idle_indicator = true;
> -
> -	ret = venus_sys_set_idle_message(hdev, venus_sys_idle_indicator);
> -	if (ret)
> -		dev_warn(dev, "setting idle response ON failed (%d)\n", ret);
> +	/* HFI_PROPERTY_SYS_IDLE_INDICATOR is not supported beyond 8916 (HFI V1) */
> +	if (IS_V1(hdev->core)) {
> +		ret = venus_sys_set_idle_message(hdev, false);
> +		if (ret)
> +			dev_warn(dev, "setting idle response ON failed (%d)\n", ret);
> +	}

This property was enabled in video firmware for SDM845 by default from 
version #7. Need to confirm if patch[1]

was added before the default enablement in firmware. This patch need to 
be verified on SDM845 for suspend

functionality, before removing it for V4 to avoid power regression.

[1] 17cd3d1d2e52: media: venus: hfi_venus: add suspend functionality for 
Venus 4xx

-Vikash

>   
>   	ret = venus_sys_set_power_control(hdev, venus_fw_low_power_mode);
>   	if (ret)
>

[PATCH v2 02/18] media: venus: hfi_venus: Write to VIDC_CTRL_INIT after unmasking interrupts

[Konrad Dybcio]

The downstream driver signals the hardware to be enabled only after the
interrupts are unmasked, which... makes sense. Follow suit.

Cc: stable@vger.kernel.org # v4.12+
Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index bff435abd59b..8fc8f46dc390 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -453,7 +453,6 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
 	void __iomem *wrapper_base = hdev->core->wrapper_base;
 	int ret = 0;
 
-	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
 	if (IS_V6(hdev->core)) {
 		mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
 		mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
@@ -464,6 +463,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
 	writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
 	writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
 
+	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
 	while (!ctrl_status && count < max_tries) {
 		ctrl_status = readl(cpu_cs_base + CPU_CS_SCIACMDARG0);
 		if ((ctrl_status & CPU_CS_SCIACMDARG0_ERROR_STATUS_MASK) == 4) {

-- 
2.40.1

Re: [PATCH v2 02/18] media: venus: hfi_venus: Write to VIDC_CTRL_INIT after unmasking interrupts

[Vikash Garodia]

On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
> The downstream driver signals the hardware to be enabled only after the
> interrupts are unmasked, which... makes sense. Follow suit.

Rephrase the commit text,

1. No need to mention downstream driver, if something is buggy, fix it.

2. Avoid "..." and lets make it more formal.

> Cc: stable@vger.kernel.org # v4.12+
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index bff435abd59b..8fc8f46dc390 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -453,7 +453,6 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>   	void __iomem *wrapper_base = hdev->core->wrapper_base;
>   	int ret = 0;
>   
> -	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>   	if (IS_V6(hdev->core)) {
>   		mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
>   		mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
> @@ -464,6 +463,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>   	writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
>   	writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
>   
> +	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>   	while (!ctrl_status && count < max_tries) {
>   		ctrl_status = readl(cpu_cs_base + CPU_CS_SCIACMDARG0);
>   		if ((ctrl_status & CPU_CS_SCIACMDARG0_ERROR_STATUS_MASK) == 4) {

Above code looks good.

-Vikash

Re: [PATCH v2 02/18] media: venus: hfi_venus: Write to VIDC_CTRL_INIT after unmasking interrupts

[Konrad Dybcio]

On 5.05.2023 14:33, Vikash Garodia wrote:
> 
> On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
>> The downstream driver signals the hardware to be enabled only after the
>> interrupts are unmasked, which... makes sense. Follow suit.
> 
> Rephrase the commit text,
> 
> 1. No need to mention downstream driver, if something is buggy, fix it.
Generally I'd agree, however in this specific case the downstream
driver is the only available source of knowledge about what the correct
(or at least working) initialization sequence of this hw block is.

> 
> 2. Avoid "..." and lets make it more formal.
Ack

Konrad
> 
>> Cc: stable@vger.kernel.org # v4.12+
>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index bff435abd59b..8fc8f46dc390 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -453,7 +453,6 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>>       void __iomem *wrapper_base = hdev->core->wrapper_base;
>>       int ret = 0;
>>   -    writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>       if (IS_V6(hdev->core)) {
>>           mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
>>           mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
>> @@ -464,6 +463,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>>       writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
>>       writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
>>   +    writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>       while (!ctrl_status && count < max_tries) {
>>           ctrl_status = readl(cpu_cs_base + CPU_CS_SCIACMDARG0);
>>           if ((ctrl_status & CPU_CS_SCIACMDARG0_ERROR_STATUS_MASK) == 4) {
> 
> Above code looks good.
> 
> -Vikash
> 

Re: [PATCH v2 02/18] media: venus: hfi_venus: Write to VIDC_CTRL_INIT after unmasking interrupts

[Vikash Garodia]

On 5/6/2023 12:18 AM, Konrad Dybcio wrote:
> 
> 
> On 5.05.2023 14:33, Vikash Garodia wrote:
>>
>> On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
>>> The downstream driver signals the hardware to be enabled only after the
>>> interrupts are unmasked, which... makes sense. Follow suit.
>>
>> Rephrase the commit text,
>>
>> 1. No need to mention downstream driver, if something is buggy, fix it.
> Generally I'd agree, however in this specific case the downstream
> driver is the only available source of knowledge about what the correct
> (or at least working) initialization sequence of this hw block is.
Ideal way would be to refer the sequence of register programming from hardware
programming guide (HPG). In this case, the commit can describe the change which
is being done and reviewer/maintainer can certify the sequence from the HPG.
Also downstream driver can be buggy too, so IMO, it is not the right reference.

-Vikash

>>
>> 2. Avoid "..." and lets make it more formal.
> Ack
> 
> Konrad
>>
>>> Cc: stable@vger.kernel.org # v4.12+
>>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>>> index bff435abd59b..8fc8f46dc390 100644
>>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>>> @@ -453,7 +453,6 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>>>       void __iomem *wrapper_base = hdev->core->wrapper_base;
>>>       int ret = 0;
>>>   -    writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>>       if (IS_V6(hdev->core)) {
>>>           mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
>>>           mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
>>> @@ -464,6 +463,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
>>>       writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
>>>       writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
>>>   +    writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>>       while (!ctrl_status && count < max_tries) {
>>>           ctrl_status = readl(cpu_cs_base + CPU_CS_SCIACMDARG0);
>>>           if ((ctrl_status & CPU_CS_SCIACMDARG0_ERROR_STATUS_MASK) == 4) {
>>
>> Above code looks good.
>>
>> -Vikash
>>

[PATCH v2 03/18] media: venus: Remap bufreq fields on HFI6XX

[Konrad Dybcio]

Similarly to HFI4XX, the fields are remapped on 6XX as well. Fix it.

Cc: stable@vger.kernel.org # v5.12+
Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_helper.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
index 105792a68060..e0c8f15644df 100644
--- a/drivers/media/platform/qcom/venus/hfi_helper.h
+++ b/drivers/media/platform/qcom/venus/hfi_helper.h
@@ -1170,11 +1170,14 @@ struct hfi_buffer_display_hold_count_actual {
 
 /* HFI 4XX reorder the fields, use these macros */
 #define HFI_BUFREQ_HOLD_COUNT(bufreq, ver)	\
-	((ver) == HFI_VERSION_4XX ? 0 : (bufreq)->hold_count)
+	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
+	? 0 : (bufreq)->hold_count)
 #define HFI_BUFREQ_COUNT_MIN(bufreq, ver)	\
-	((ver) == HFI_VERSION_4XX ? (bufreq)->hold_count : (bufreq)->count_min)
+	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
+	? (bufreq)->hold_count : (bufreq)->count_min)
 #define HFI_BUFREQ_COUNT_MIN_HOST(bufreq, ver)	\
-	((ver) == HFI_VERSION_4XX ? (bufreq)->count_min : 0)
+	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
+	? (bufreq)->count_min : 0)
 
 struct hfi_buffer_requirements {
 	u32 type;

-- 
2.40.1

Re: [PATCH v2 03/18] media: venus: Remap bufreq fields on HFI6XX

[Vikash Garodia]

On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
> Similarly to HFI4XX, the fields are remapped on 6XX as well. Fix it.
>
> Cc: stable@vger.kernel.org # v5.12+
> Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/hfi_helper.h | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
> index 105792a68060..e0c8f15644df 100644
> --- a/drivers/media/platform/qcom/venus/hfi_helper.h
> +++ b/drivers/media/platform/qcom/venus/hfi_helper.h
> @@ -1170,11 +1170,14 @@ struct hfi_buffer_display_hold_count_actual {
>   
>   /* HFI 4XX reorder the fields, use these macros */
>   #define HFI_BUFREQ_HOLD_COUNT(bufreq, ver)	\
> -	((ver) == HFI_VERSION_4XX ? 0 : (bufreq)->hold_count)
> +	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
> +	? 0 : (bufreq)->hold_count)
>   #define HFI_BUFREQ_COUNT_MIN(bufreq, ver)	\
> -	((ver) == HFI_VERSION_4XX ? (bufreq)->hold_count : (bufreq)->count_min)
> +	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
> +	? (bufreq)->hold_count : (bufreq)->count_min)
>   #define HFI_BUFREQ_COUNT_MIN_HOST(bufreq, ver)	\
> -	((ver) == HFI_VERSION_4XX ? (bufreq)->count_min : 0)
> +	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
> +	? (bufreq)->count_min : 0)

This patch is not correct. The existing code handles the disparity of 
buffer requirement payload received from firmware.

Its applicable only for V4.

For V6, driver does not rely on firmware to get the buffer requirement. 
Refer the buffer platform code for more details.

-Vikash

>   struct hfi_buffer_requirements {
>   	u32 type;
>

Re: [PATCH v2 03/18] media: venus: Remap bufreq fields on HFI6XX

[Konrad Dybcio]

On 5.05.2023 14:38, Vikash Garodia wrote:
> On 5/4/2023 1:30 PM, Konrad Dybcio wrote:
>> Similarly to HFI4XX, the fields are remapped on 6XX as well. Fix it.
>>
>> Cc: stable@vger.kernel.org # v5.12+
>> Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_helper.h | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
>> index 105792a68060..e0c8f15644df 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_helper.h
>> +++ b/drivers/media/platform/qcom/venus/hfi_helper.h
>> @@ -1170,11 +1170,14 @@ struct hfi_buffer_display_hold_count_actual {
>>     /* HFI 4XX reorder the fields, use these macros */
>>   #define HFI_BUFREQ_HOLD_COUNT(bufreq, ver)    \
>> -    ((ver) == HFI_VERSION_4XX ? 0 : (bufreq)->hold_count)
>> +    ((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
>> +    ? 0 : (bufreq)->hold_count)
>>   #define HFI_BUFREQ_COUNT_MIN(bufreq, ver)    \
>> -    ((ver) == HFI_VERSION_4XX ? (bufreq)->hold_count : (bufreq)->count_min)
>> +    ((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
>> +    ? (bufreq)->hold_count : (bufreq)->count_min)
>>   #define HFI_BUFREQ_COUNT_MIN_HOST(bufreq, ver)    \
>> -    ((ver) == HFI_VERSION_4XX ? (bufreq)->count_min : 0)
>> +    ((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
>> +    ? (bufreq)->count_min : 0)
> 
> This patch is not correct. The existing code handles the disparity of buffer requirement payload received from firmware.
> 
> Its applicable only for V4.
> 
> For V6, driver does not rely on firmware to get the buffer requirement. Refer the buffer platform code for more details.
OK right I can see, downstream has a condition for IRIS2/IRIS2_1,
thanks for pointing this out!

Konrad
> 
> -Vikash
> 
>>   struct hfi_buffer_requirements {
>>       u32 type;
>>

[PATCH v2 04/18] media: venus: Introduce VPU version distinction

[Konrad Dybcio]

The Video Processing Unit hardware version is the differentiator,
based on which we should decide which code paths to take in hw
init. Up until now, we've relied on HFI versions, but that was
just a happy accident between recent SoCs. Add a field in the
res struct and add correlated definitions that will be used to
account for the aforementioned differences.

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

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 4f81669986ba..62c310b7dee6 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -48,6 +48,14 @@ struct bw_tbl {
 	u32 peak_10bit;
 };
 
+enum vpu_version {
+	VPU_VERSION_AR50,
+	VPU_VERSION_AR50_LITE,
+	VPU_VERSION_IRIS1,
+	VPU_VERSION_IRIS2,
+	VPU_VERSION_IRIS2_1,
+};
+
 struct venus_resources {
 	u64 dma_mask;
 	const struct freq_tbl *freq_tbl;
@@ -71,6 +79,7 @@ struct venus_resources {
 	const char * const resets[VIDC_RESETS_NUM_MAX];
 	unsigned int resets_num;
 	enum hfi_version hfi_version;
+	enum vpu_version vpu_version;
 	u8 num_vpp_pipes;
 	u32 max_load;
 	unsigned int vmem_id;
@@ -481,6 +490,12 @@ struct venus_inst {
 #define IS_V4(core)	((core)->res->hfi_version == HFI_VERSION_4XX)
 #define IS_V6(core)	((core)->res->hfi_version == HFI_VERSION_6XX)
 
+#define IS_AR50(core)		((core)->res->vpu_version == VPU_VERSION_AR50)
+#define IS_AR50_LITE(core)	((core)->res->vpu_version == VPU_VERSION_AR50_LITE)
+#define IS_IRIS1(core)		((core)->res->vpu_version == VPU_VERSION_IRIS1)
+#define IS_IRIS2(core)		((core)->res->vpu_version == VPU_VERSION_IRIS2)
+#define IS_IRIS2_1(core)	((core)->res->vpu_version == VPU_VERSION_IRIS2_1)
+
 #define ctrl_to_inst(ctrl)	\
 	container_of((ctrl)->handler, struct venus_inst, ctrl_handler)
 

-- 
2.40.1

Re: [PATCH v2 04/18] media: venus: Introduce VPU version distinction

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> The Video Processing Unit hardware version is the differentiator,
> based on which we should decide which code paths to take in hw
we -> video driver
> init. Up until now, we've relied on HFI versions, but that was

not just hw init, aspects like power sequence, buffer calculations, etc 
would

rely on hardware version.

Once the above comments are addressed, you can put

Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>

> just a happy accident between recent SoCs. Add a field in the
> res struct and add correlated definitions that will be used to
> account for the aforementioned differences.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/core.h | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 4f81669986ba..62c310b7dee6 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -48,6 +48,14 @@ struct bw_tbl {
>   	u32 peak_10bit;
>   };
>   
> +enum vpu_version {
> +	VPU_VERSION_AR50,
> +	VPU_VERSION_AR50_LITE,
> +	VPU_VERSION_IRIS1,
> +	VPU_VERSION_IRIS2,
> +	VPU_VERSION_IRIS2_1,
> +};
> +
>   struct venus_resources {
>   	u64 dma_mask;
>   	const struct freq_tbl *freq_tbl;
> @@ -71,6 +79,7 @@ struct venus_resources {
>   	const char * const resets[VIDC_RESETS_NUM_MAX];
>   	unsigned int resets_num;
>   	enum hfi_version hfi_version;
> +	enum vpu_version vpu_version;
>   	u8 num_vpp_pipes;
>   	u32 max_load;
>   	unsigned int vmem_id;
> @@ -481,6 +490,12 @@ struct venus_inst {
>   #define IS_V4(core)	((core)->res->hfi_version == HFI_VERSION_4XX)
>   #define IS_V6(core)	((core)->res->hfi_version == HFI_VERSION_6XX)
>   
> +#define IS_AR50(core)		((core)->res->vpu_version == VPU_VERSION_AR50)
> +#define IS_AR50_LITE(core)	((core)->res->vpu_version == VPU_VERSION_AR50_LITE)
> +#define IS_IRIS1(core)		((core)->res->vpu_version == VPU_VERSION_IRIS1)
> +#define IS_IRIS2(core)		((core)->res->vpu_version == VPU_VERSION_IRIS2)
> +#define IS_IRIS2_1(core)	((core)->res->vpu_version == VPU_VERSION_IRIS2_1)
> +
>   #define ctrl_to_inst(ctrl)	\
>   	container_of((ctrl)->handler, struct venus_inst, ctrl_handler)
>   
>

[PATCH v2 05/18] media: venus: Add vpu_version to most SoCs

[Konrad Dybcio]

Add vpu_version where I was able to retrieve the information to
allow for more precise hardware-specific code path matching.

Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 2ae867cb4c48..01671dd23888 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -684,6 +684,7 @@ static const struct venus_resources sdm845_res = {
 	.vcodec_clks_num = 2,
 	.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,
@@ -709,6 +710,7 @@ static const struct venus_resources sdm845_res_v2 = {
 	.vcodec_num = 2,
 	.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,
@@ -756,6 +758,7 @@ static const struct venus_resources sc7180_res = {
 	.opp_pmdomain = (const char *[]) { "cx", NULL },
 	.vcodec_num = 1,
 	.hfi_version = HFI_VERSION_4XX,
+	.vpu_version = VPU_VERSION_AR50,
 	.vmem_id = VIDC_RESOURCE_NONE,
 	.vmem_size = 0,
 	.vmem_addr = 0,
@@ -809,6 +812,7 @@ static const struct venus_resources sm8250_res = {
 	.vcodec_num = 1,
 	.max_load = 7833600,
 	.hfi_version = HFI_VERSION_6XX,
+	.vpu_version = VPU_VERSION_IRIS2,
 	.num_vpp_pipes = 4,
 	.vmem_id = VIDC_RESOURCE_NONE,
 	.vmem_size = 0,
@@ -866,6 +870,7 @@ static const struct venus_resources sc7280_res = {
 	.opp_pmdomain = (const char *[]) { "cx", NULL },
 	.vcodec_num = 1,
 	.hfi_version = HFI_VERSION_6XX,
+	.vpu_version = VPU_VERSION_IRIS2_1,
 	.num_vpp_pipes = 1,
 	.vmem_id = VIDC_RESOURCE_NONE,
 	.vmem_size = 0,

-- 
2.40.1

Re: [PATCH v2 05/18] media: venus: Add vpu_version to most SoCs

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> Add vpu_version where I was able to retrieve the information to
> allow for more precise hardware-specific code path matching.
>
> Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/core.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 2ae867cb4c48..01671dd23888 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -684,6 +684,7 @@ static const struct venus_resources sdm845_res = {
>   	.vcodec_clks_num = 2,
>   	.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,
> @@ -709,6 +710,7 @@ static const struct venus_resources sdm845_res_v2 = {
>   	.vcodec_num = 2,
>   	.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,
> @@ -756,6 +758,7 @@ static const struct venus_resources sc7180_res = {
>   	.opp_pmdomain = (const char *[]) { "cx", NULL },
>   	.vcodec_num = 1,
>   	.hfi_version = HFI_VERSION_4XX,
> +	.vpu_version = VPU_VERSION_AR50,
>   	.vmem_id = VIDC_RESOURCE_NONE,
>   	.vmem_size = 0,
>   	.vmem_addr = 0,
> @@ -809,6 +812,7 @@ static const struct venus_resources sm8250_res = {
>   	.vcodec_num = 1,
>   	.max_load = 7833600,
>   	.hfi_version = HFI_VERSION_6XX,
> +	.vpu_version = VPU_VERSION_IRIS2,
>   	.num_vpp_pipes = 4,
>   	.vmem_id = VIDC_RESOURCE_NONE,
>   	.vmem_size = 0,
> @@ -866,6 +870,7 @@ static const struct venus_resources sc7280_res = {
>   	.opp_pmdomain = (const char *[]) { "cx", NULL },
>   	.vcodec_num = 1,
>   	.hfi_version = HFI_VERSION_6XX,
> +	.vpu_version = VPU_VERSION_IRIS2_1,
>   	.num_vpp_pipes = 1,
>   	.vmem_id = VIDC_RESOURCE_NONE,
>   	.vmem_size = 0,
>

[PATCH v2 06/18] media: venus: firmware: Leave a clue for homegrown porters

[Konrad Dybcio]

Leave a clue about where the seemingly magic values come from, as it
is not obvious and requires some digging downstream..

Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/firmware.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index cfb11c551167..a4cd919e1dbe 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -241,6 +241,13 @@ int venus_boot(struct venus_core *core)
 		return ret;
 
 	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, addr not size)
+		 * 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,
 						     res->cp_size,
 						     res->cp_nonpixel_start,

-- 
2.40.1

Re: [PATCH v2 06/18] media: venus: firmware: Leave a clue for homegrown porters

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> Leave a clue about where the seemingly magic values come from, as it
> is not obvious and requires some digging downstream..
>
> Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/firmware.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index cfb11c551167..a4cd919e1dbe 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -241,6 +241,13 @@ int venus_boot(struct venus_core *core)
>   		return ret;
>   
>   	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, addr not size)

The field is the start address of ns context bank. Since the cp_start is 
0, the start address for (next) non-secure context bank

is interpreted as size of the (previous) content protection region.

> +		 * 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,
>   						     res->cp_size,
>   						     res->cp_nonpixel_start,
>

Re: [PATCH v2 06/18] media: venus: firmware: Leave a clue for homegrown porters

[Vikash Garodia]

On 5/5/2023 6:27 PM, Vikash Garodia wrote:
>
> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>> Leave a clue about where the seemingly magic values come from, as it
>> is not obvious and requires some digging downstream..
Rephrase the commit text.
>> Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/firmware.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/firmware.c 
>> b/drivers/media/platform/qcom/venus/firmware.c
>> index cfb11c551167..a4cd919e1dbe 100644
>> --- a/drivers/media/platform/qcom/venus/firmware.c
>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>> @@ -241,6 +241,13 @@ int venus_boot(struct venus_core *core)
>>           return ret;
>>         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, addr not size)
>
> The field is the start address of ns context bank. Since the cp_start 
> is 0, the start address for (next) non-secure context bank
>
> is interpreted as size of the (previous) content protection region.
>
>> +         * 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,
>>                                res->cp_size,
>>                                res->cp_nonpixel_start,
>>

Re: [PATCH v2 06/18] media: venus: firmware: Leave a clue for homegrown porters

[Konrad Dybcio]

On 5.05.2023 15:00, Vikash Garodia wrote:
> 
> On 5/5/2023 6:27 PM, Vikash Garodia wrote:
>>
>> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>>> Leave a clue about where the seemingly magic values come from, as it
>>> is not obvious and requires some digging downstream..
> Rephrase the commit text.
Please be more specific, do you want me to use the
explanations you gave in the previous reply?

Konrad
>>> Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>>> ---
>>>   drivers/media/platform/qcom/venus/firmware.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
>>> index cfb11c551167..a4cd919e1dbe 100644
>>> --- a/drivers/media/platform/qcom/venus/firmware.c
>>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>>> @@ -241,6 +241,13 @@ int venus_boot(struct venus_core *core)
>>>           return ret;
>>>         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, addr not size)
>>
>> The field is the start address of ns context bank. Since the cp_start is 0, the start address for (next) non-secure context bank
>>
>> is interpreted as size of the (previous) content protection region.
>>
>>> +         * 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,
>>>                                res->cp_size,
>>>                                res->cp_nonpixel_start,
>>>

Re: [PATCH v2 06/18] media: venus: firmware: Leave a clue for homegrown porters

[Vikash Garodia]

On 5/6/2023 12:54 AM, Konrad Dybcio wrote:
> 
> 
> On 5.05.2023 15:00, Vikash Garodia wrote:
>>
>> On 5/5/2023 6:27 PM, Vikash Garodia wrote:
>>>
>>> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>>>> Leave a clue about where the seemingly magic values come from, as it
>>>> is not obvious and requires some digging downstream..
>> Rephrase the commit text.
> Please be more specific, do you want me to use the
> explanations you gave in the previous reply?

Something which describes the patch like - Add comments to describe the
arguments of the qcom_scm_mem_protect_video_var TZ call. The TZ call programs
the video hardware with CP and non-CP VARs, and the comments describes these VARs.

> Konrad
>>>> Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>>>> ---
>>>>   drivers/media/platform/qcom/venus/firmware.c | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
>>>> index cfb11c551167..a4cd919e1dbe 100644
>>>> --- a/drivers/media/platform/qcom/venus/firmware.c
>>>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>>>> @@ -241,6 +241,13 @@ int venus_boot(struct venus_core *core)
>>>>           return ret;
>>>>         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, addr not size)
>>>
>>> The field is the start address of ns context bank. Since the cp_start is 0, the start address for (next) non-secure context bank
>>>
>>> is interpreted as size of the (previous) content protection region.
>>>
>>>> +         * 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,
>>>>                                res->cp_size,
>>>>                                res->cp_nonpixel_start,
>>>>

[PATCH v2 07/18] media: venus: hfi_venus: Sanitize venus_boot_core() per-VPU-version

[Konrad Dybcio]

The current assumption of IS_V6 is overgeneralized. Adjust the logic
to take the VPU hardware version into account.

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

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 8fc8f46dc390..9b840440a115 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -447,19 +447,20 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
 {
 	struct device *dev = hdev->core->dev;
 	static const unsigned int max_tries = 100;
-	u32 ctrl_status = 0, mask_val;
+	u32 ctrl_status = 0, mask_val = 0;
 	unsigned int count = 0;
 	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
 	void __iomem *wrapper_base = hdev->core->wrapper_base;
 	int ret = 0;
 
-	if (IS_V6(hdev->core)) {
+	if (IS_IRIS1(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
 		mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
 		mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
 			      WRAPPER_INTR_MASK_A2HCPU_MASK);
 	} else {
 		mask_val = WRAPPER_INTR_MASK_A2HVCODEC_MASK;
 	}
+
 	writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
 	writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
 
@@ -479,10 +480,11 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
 	if (count >= max_tries)
 		ret = -ETIMEDOUT;
 
-	if (IS_V6(hdev->core)) {
+	if (IS_AR50_LITE(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
 		writel(0x1, cpu_cs_base + CPU_CS_H2XSOFTINTEN_V6);
+
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
 		writel(0x0, cpu_cs_base + CPU_CS_X2RPMH_V6);
-	}
 
 	return ret;
 }

-- 
2.40.1

[PATCH v2 08/18] media: venus: core: Assign registers based on VPU version

[Konrad Dybcio]

IRIS2(_1) has a different register map compared to other HFI6XX-
using VPUs. AR50L uses the same offsets, but doesn't feature vbif_base
and aon_base. Take care of it.

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

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 01671dd23888..51cead91571d 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -246,14 +246,14 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type)
 
 static void venus_assign_register_offsets(struct venus_core *core)
 {
-	if (IS_V6(core)) {
-		core->vbif_base = core->base + VBIF_BASE;
+	if (IS_AR50_LITE(core) || IS_IRIS2(core) || IS_IRIS2_1(core)) {
+		core->vbif_base = IS_AR50_LITE(core) ? NULL : core->base + VBIF_BASE;
 		core->cpu_base = core->base + CPU_BASE_V6;
 		core->cpu_cs_base = core->base + CPU_CS_BASE_V6;
 		core->cpu_ic_base = core->base + CPU_IC_BASE_V6;
 		core->wrapper_base = core->base + WRAPPER_BASE_V6;
 		core->wrapper_tz_base = core->base + WRAPPER_TZ_BASE_V6;
-		core->aon_base = core->base + AON_BASE_V6;
+		core->aon_base = IS_AR50_LITE(core) ? NULL : core->base + AON_BASE_V6;
 	} else {
 		core->vbif_base = core->base + VBIF_BASE;
 		core->cpu_base = core->base + CPU_BASE;

-- 
2.40.1

[PATCH v2 09/18] media: venus: hfi_venus: Fix version checks in venus_halt_axi()

[Konrad Dybcio]

Only IRIS2(_1) should enter the until-now-IS_V6() path and the
condition for skipping part of it should be IS_IRIS2_1 and not the
number of VPP pipes. Fix that.

Fixes: 4b0b6e147dc9 ("media: venus: hfi: Add 6xx AXI halt logic")
Fixes: 78d434ba8659 ("media: venus: hfi: Skip AON register programming for V6 1pipe")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 9b840440a115..ca56b1a8eb71 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -549,10 +549,10 @@ static int venus_halt_axi(struct venus_hfi_device *hdev)
 	u32 mask_val;
 	int ret;
 
-	if (IS_V6(hdev->core)) {
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
 		writel(0x3, cpu_cs_base + CPU_CS_X2RPMH_V6);
 
-		if (hdev->core->res->num_vpp_pipes == 1)
+		if (IS_IRIS2_1(hdev->core))
 			goto skip_aon_mvp_noc;
 
 		writel(0x1, aon_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);

-- 
2.40.1

Re: [PATCH v2 09/18] media: venus: hfi_venus: Fix version checks in venus_halt_axi()

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> Only IRIS2(_1) should enter the until-now-IS_V6() path and the
> condition for skipping part of it should be IS_IRIS2_1 and not the
> number of VPP pipes. Fix that.

Do not see any issue with existing code. IRIS2 with single pipe is 
IRIS2_1. This does not

quality as a fix to earlier implementation. Since this series introduces 
VPU versions,

IRIS2 with 1 pipe is being replaced with IRIS2_1.

-Vikash

>
> Fixes: 4b0b6e147dc9 ("media: venus: hfi: Add 6xx AXI halt logic")
> Fixes: 78d434ba8659 ("media: venus: hfi: Skip AON register programming for V6 1pipe")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 9b840440a115..ca56b1a8eb71 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -549,10 +549,10 @@ static int venus_halt_axi(struct venus_hfi_device *hdev)
>   	u32 mask_val;
>   	int ret;
>   
> -	if (IS_V6(hdev->core)) {
> +	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
>   		writel(0x3, cpu_cs_base + CPU_CS_X2RPMH_V6);
>   
> -		if (hdev->core->res->num_vpp_pipes == 1)
> +		if (IS_IRIS2_1(hdev->core))
>   			goto skip_aon_mvp_noc;
>   
>   		writel(0x1, aon_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
>

Re: [PATCH v2 09/18] media: venus: hfi_venus: Fix version checks in venus_halt_axi()

[Dmitry Baryshkov]

On Fri, 5 May 2023 at 16:22, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>
>
> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> > Only IRIS2(_1) should enter the until-now-IS_V6() path and the
> > condition for skipping part of it should be IS_IRIS2_1 and not the
> > number of VPP pipes. Fix that.
>
> Do not see any issue with existing code. IRIS2 with single pipe is
> IRIS2_1. This does not
>
> quality as a fix to earlier implementation. Since this series introduces
> VPU versions,
>
> IRIS2 with 1 pipe is being replaced with IRIS2_1.

Could you please fix the line wrapping of your emails. It becomes hard
to read them otherwise.


-- 
With best wishes
Dmitry

Re: [PATCH v2 09/18] media: venus: hfi_venus: Fix version checks in venus_halt_axi()

[Vikash Garodia]

On 5/5/2023 8:13 PM, Dmitry Baryshkov wrote:
> On Fri, 5 May 2023 at 16:22, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>>
>> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>>> Only IRIS2(_1) should enter the until-now-IS_V6() path and the
>>> condition for skipping part of it should be IS_IRIS2_1 and not the
>>> number of VPP pipes. Fix that.
>> Do not see any issue with existing code. IRIS2 with single pipe is
>> IRIS2_1. This does not
>>
>> quality as a fix to earlier implementation. Since this series introduces
>> VPU versions,
>>
>> IRIS2 with 1 pipe is being replaced with IRIS2_1.
> Could you please fix the line wrapping of your emails. It becomes hard
> to read them otherwise.

My apologies for the inconvenience. Tried to set the email wrap config, but it 
does not take

an effect on my email client. Will try playing around a bit with it and fix it.

Meanwhile go through the comments and if you can align them with minimal effort, we

can continue to discuss on the same.

-Vikash

>
>

Re: [PATCH v2 09/18] media: venus: hfi_venus: Fix version checks in venus_halt_axi()

[Konrad Dybcio]

On 5.05.2023 15:21, Vikash Garodia wrote:
> 
> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>> Only IRIS2(_1) should enter the until-now-IS_V6() path and the
>> condition for skipping part of it should be IS_IRIS2_1 and not the
>> number of VPP pipes. Fix that.
> 
> Do not see any issue with existing code. IRIS2 with single pipe is IRIS2_1. This does not
> 
> quality as a fix to earlier implementation. Since this series introduces VPU versions,
> 
> IRIS2 with 1 pipe is being replaced with IRIS2_1.
> 
> -Vikash
Right, I'll drop the fixes tags.

Also, your email client seems to be inserting 2 newlines instead of 1.
If you're using thunderbird, you may want to edit:

mail.identity.(default or your mail identity idx).default.compose_html

to `false`

or you can use shift+enter as a half-measure

Konrad
> 
>>
>> Fixes: 4b0b6e147dc9 ("media: venus: hfi: Add 6xx AXI halt logic")
>> Fixes: 78d434ba8659 ("media: venus: hfi: Skip AON register programming for V6 1pipe")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 9b840440a115..ca56b1a8eb71 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -549,10 +549,10 @@ static int venus_halt_axi(struct venus_hfi_device *hdev)
>>       u32 mask_val;
>>       int ret;
>>   -    if (IS_V6(hdev->core)) {
>> +    if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
>>           writel(0x3, cpu_cs_base + CPU_CS_X2RPMH_V6);
>>   -        if (hdev->core->res->num_vpp_pipes == 1)
>> +        if (IS_IRIS2_1(hdev->core))
>>               goto skip_aon_mvp_noc;
>>             writel(0x1, aon_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
>>

[PATCH v2 10/18] media: venus: hfi_venus: Fix version checks in venus_isr()

[Konrad Dybcio]

IS_V6 was used there IS_IRIS2(_1) should have been and the !IS_V6
condition was only correct by luck and for now. Replace them both
with VPU version checks.

Fixes: 24fcc0522d87 ("media: venus: hfi: Add 6xx interrupt support")
Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index ca56b1a8eb71..6d5906fab800 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1130,7 +1130,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
 	wrapper_base = hdev->core->wrapper_base;
 
 	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
-	if (IS_V6(core)) {
+	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
 		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
 		    status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 ||
 		    status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK)
@@ -1142,7 +1142,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
 			hdev->irq_status = status;
 	}
 	writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR);
-	if (!IS_V6(core))
+	if (!(IS_AR50_LITE(core) || IS_IRIS2(core) || IS_IRIS2_1(core)))
 		writel(status, wrapper_base + WRAPPER_INTR_CLEAR);
 
 	return IRQ_WAKE_THREAD;

-- 
2.40.1

Re: [PATCH v2 10/18] media: venus: hfi_venus: Fix version checks in venus_isr()

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> IS_V6 was used there IS_IRIS2(_1) should have been and the !IS_V6
> condition was only correct by luck and for now. Replace them both
> with VPU version checks.

Existing video driver supports IRIS2(_1) under V6 category. What is meant by

!IS_V6 condition was correct by luck ?

-Vikash

> Fixes: 24fcc0522d87 ("media: venus: hfi: Add 6xx interrupt support")
> Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index ca56b1a8eb71..6d5906fab800 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -1130,7 +1130,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
>   	wrapper_base = hdev->core->wrapper_base;
>   
>   	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
> -	if (IS_V6(core)) {
> +	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
>   		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
>   		    status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 ||
>   		    status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK)
> @@ -1142,7 +1142,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
>   			hdev->irq_status = status;
>   	}
>   	writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR);
> -	if (!IS_V6(core))
> +	if (!(IS_AR50_LITE(core) || IS_IRIS2(core) || IS_IRIS2_1(core)))
>   		writel(status, wrapper_base + WRAPPER_INTR_CLEAR);
>   
>   	return IRQ_WAKE_THREAD;
>

Re: [PATCH v2 10/18] media: venus: hfi_venus: Fix version checks in venus_isr()

[Konrad Dybcio]

On 5.05.2023 15:29, Vikash Garodia wrote:
> 
> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>> IS_V6 was used there IS_IRIS2(_1) should have been and the !IS_V6
>> condition was only correct by luck and for now. Replace them both
>> with VPU version checks.
> 
> Existing video driver supports IRIS2(_1) under V6 category. What is meant by
> 
> !IS_V6 condition was correct by luck ?
Right, I didn't quite think of it this way. I probably meant "it works
just because there are no other HFIv6 cores supported", but that might
have just as well been a design choice. I'll reword and drop fixes.

Konrad
> 
> -Vikash
> 
>> Fixes: 24fcc0522d87 ("media: venus: hfi: Add 6xx interrupt support")
>> Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index ca56b1a8eb71..6d5906fab800 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -1130,7 +1130,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
>>       wrapper_base = hdev->core->wrapper_base;
>>         status = readl(wrapper_base + WRAPPER_INTR_STATUS);
>> -    if (IS_V6(core)) {
>> +    if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
>>           if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
>>               status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 ||
>>               status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK)
>> @@ -1142,7 +1142,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
>>               hdev->irq_status = status;
>>       }
>>       writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR);
>> -    if (!IS_V6(core))
>> +    if (!(IS_AR50_LITE(core) || IS_IRIS2(core) || IS_IRIS2_1(core)))
>>           writel(status, wrapper_base + WRAPPER_INTR_CLEAR);
>>         return IRQ_WAKE_THREAD;
>>

[PATCH v2 11/18] media: venus: hfi_venus: Fix version check in venus_cpu_and_video_core_idle()

[Konrad Dybcio]

IS_V6() should have instead checked for specific VPU versions. Fix it.

Fixes: e396e75fc254 ("media: venus: hfi: Read WRAPPER_TZ_CPU_STATUS_V6 on 6xx")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 6d5906fab800..82aa7deeafa1 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1537,7 +1537,7 @@ static bool venus_cpu_and_video_core_idle(struct venus_hfi_device *hdev)
 	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
 	u32 ctrl_status, cpu_status;
 
-	if (IS_V6(hdev->core))
+	if (IS_AR50_LITE(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
 		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
 	else
 		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

-- 
2.40.1

Re: [PATCH v2 11/18] media: venus: hfi_venus: Fix version check in venus_cpu_and_video_core_idle()

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> IS_V6() should have instead checked for specific VPU versions. Fix it.

This is again not a fix. The patch just adds a video hardware AR50_LITE, 
which is

not supported on existing driver yet. With existing code, IS_V6 covers 
the video

hardwares which are enabled by the driver.

-Vikash

>
> Fixes: e396e75fc254 ("media: venus: hfi: Read WRAPPER_TZ_CPU_STATUS_V6 on 6xx")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 6d5906fab800..82aa7deeafa1 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -1537,7 +1537,7 @@ static bool venus_cpu_and_video_core_idle(struct venus_hfi_device *hdev)
>   	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
>   	u32 ctrl_status, cpu_status;
>   
> -	if (IS_V6(hdev->core))
> +	if (IS_AR50_LITE(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
>   		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
>   	else
>   		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);
>

Re: [PATCH v2 11/18] media: venus: hfi_venus: Fix version check in venus_cpu_and_video_core_idle()

[Konrad Dybcio]

On 5.05.2023 15:36, Vikash Garodia wrote:
> 
> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>> IS_V6() should have instead checked for specific VPU versions. Fix it.
> 
> This is again not a fix. The patch just adds a video hardware AR50_LITE, which is
> 
> not supported on existing driver yet. With existing code, IS_V6 covers the video
> 
> hardwares which are enabled by the driver.
ack

Konrad
> 
> -Vikash
> 
>>
>> Fixes: e396e75fc254 ("media: venus: hfi: Read WRAPPER_TZ_CPU_STATUS_V6 on 6xx")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 6d5906fab800..82aa7deeafa1 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -1537,7 +1537,7 @@ static bool venus_cpu_and_video_core_idle(struct venus_hfi_device *hdev)
>>       void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
>>       u32 ctrl_status, cpu_status;
>>   -    if (IS_V6(hdev->core))
>> +    if (IS_AR50_LITE(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
>>           cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
>>       else
>>           cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);
>>

[PATCH v2 12/18] media: venus: hfi_venus: Fix version check in venus_cpu_idle_and_pc_ready()

[Konrad Dybcio]

IS_V6() should have instead checked for specific VPU versions. Fix it.

Fixes: e396e75fc254 ("media: venus: hfi: Read WRAPPER_TZ_CPU_STATUS_V6 on 6xx")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 82aa7deeafa1..d6df99a921bb 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1557,7 +1557,7 @@ static bool venus_cpu_idle_and_pc_ready(struct venus_hfi_device *hdev)
 	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
 	u32 ctrl_status, cpu_status;
 
-	if (IS_V6(hdev->core))
+	if (IS_AR50_LITE(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
 		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
 	else
 		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

-- 
2.40.1

Re: [PATCH v2 12/18] media: venus: hfi_venus: Fix version check in venus_cpu_idle_and_pc_ready()

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> IS_V6() should have instead checked for specific VPU versions. Fix it.
This is again not a fix. The patch just adds a video hardware AR50_LITE, 
which is

not supported on existing driver yet. With existing code, IS_V6 covers 
the video

hardwares which are enabled by the driver.

-Vikash
>
> Fixes: e396e75fc254 ("media: venus: hfi: Read WRAPPER_TZ_CPU_STATUS_V6 on 6xx")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 82aa7deeafa1..d6df99a921bb 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -1557,7 +1557,7 @@ static bool venus_cpu_idle_and_pc_ready(struct venus_hfi_device *hdev)
>   	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
>   	u32 ctrl_status, cpu_status;
>   
> -	if (IS_V6(hdev->core))
> +	if (IS_AR50_LITE(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
>   		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
>   	else
>   		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);
>

Re: [PATCH v2 12/18] media: venus: hfi_venus: Fix version check in venus_cpu_idle_and_pc_ready()

[Konrad Dybcio]

On 5.05.2023 15:40, Vikash Garodia wrote:
> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>> IS_V6() should have instead checked for specific VPU versions. Fix it.
> This is again not a fix. The patch just adds a video hardware AR50_LITE, which is
> 
> not supported on existing driver yet. With existing code, IS_V6 covers the video
> 
> hardwares which are enabled by the driver.
> 
> -Vikash
Ack

Konrad
>>
>> Fixes: e396e75fc254 ("media: venus: hfi: Read WRAPPER_TZ_CPU_STATUS_V6 on 6xx")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 82aa7deeafa1..d6df99a921bb 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -1557,7 +1557,7 @@ static bool venus_cpu_idle_and_pc_ready(struct venus_hfi_device *hdev)
>>       void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
>>       u32 ctrl_status, cpu_status;
>>   -    if (IS_V6(hdev->core))
>> +    if (IS_AR50_LITE(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
>>           cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
>>       else
>>           cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);
>>

[PATCH v2 13/18] media: venus: firmware: Correct IS_V6() checks

[Konrad Dybcio]

Most of these checks should have checked for TZ presence (or well,
absence), as we shouldn't really be doing things that the black box
does for us on non-CrOS platforms.

The IS_V6() check in venus_shutdown_no_tz() should have checked
whether the core version is IRIS2_1 (so, SC7280). Fix that.

Fixes: afeae6ef0780 ("media: venus: firmware: enable no tz fw loading for sc7280")
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/firmware.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index a4cd919e1dbe..be59f2017155 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -29,7 +29,11 @@ static void venus_reset_cpu(struct venus_core *core)
 	u32 fw_size = core->fw.mapped_mem_size;
 	void __iomem *wrapper_base;
 
-	if (IS_V6(core))
+	/*
+	 * When there's no Qualcomm TZ (like on Chromebooks), the OS is
+	 * responsible for bringing up the hardware instead.
+	 */
+	if (!core->use_tz)
 		wrapper_base = core->wrapper_tz_base;
 	else
 		wrapper_base = core->wrapper_base;
@@ -41,7 +45,7 @@ static void venus_reset_cpu(struct venus_core *core)
 	writel(fw_size, wrapper_base + WRAPPER_NONPIX_START_ADDR);
 	writel(fw_size, wrapper_base + WRAPPER_NONPIX_END_ADDR);
 
-	if (IS_V6(core)) {
+	if (!core->use_tz) {
 		/* Bring XTSS out of reset */
 		writel(0, wrapper_base + WRAPPER_TZ_XTSS_SW_RESET);
 	} else {
@@ -67,7 +71,7 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
 	if (resume) {
 		venus_reset_cpu(core);
 	} else {
-		if (IS_V6(core))
+		if (!core->use_tz)
 			writel(WRAPPER_XTSS_SW_RESET_BIT,
 			       core->wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
 		else
@@ -179,7 +183,7 @@ static int venus_shutdown_no_tz(struct venus_core *core)
 	void __iomem *wrapper_base = core->wrapper_base;
 	void __iomem *wrapper_tz_base = core->wrapper_tz_base;
 
-	if (IS_V6(core)) {
+	if (IS_IRIS2_1(core)) {
 		/* Assert the reset to XTSS */
 		reg = readl(wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
 		reg |= WRAPPER_XTSS_SW_RESET_BIT;

-- 
2.40.1

[PATCH v2 14/18] media: venus: hfi_platform: Check vpu_version instead of device compatible

[Konrad Dybcio]

This is not a matter of the host SoC, but the VPU chip in Venus. Fix it.

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

diff --git a/drivers/media/platform/qcom/venus/hfi_platform.c b/drivers/media/platform/qcom/venus/hfi_platform.c
index f07f554bc5fe..d163d5b0e6b7 100644
--- a/drivers/media/platform/qcom/venus/hfi_platform.c
+++ b/drivers/media/platform/qcom/venus/hfi_platform.c
@@ -80,7 +80,7 @@ hfi_platform_get_codecs(struct venus_core *core, u32 *enc_codecs, u32 *dec_codec
 	if (plat->codecs)
 		plat->codecs(enc_codecs, dec_codecs, count);
 
-	if (of_device_is_compatible(core->dev->of_node, "qcom,sc7280-venus")) {
+	if (IS_IRIS2_1(core)) {
 		*enc_codecs &= ~HFI_VIDEO_CODEC_VP8;
 		*dec_codecs &= ~HFI_VIDEO_CODEC_VP8;
 	}

-- 
2.40.1

Re: [PATCH v2 14/18] media: venus: hfi_platform: Check vpu_version instead of device compatible

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> This is not a matter of the host SoC, but the VPU chip in Venus. Fix it.
>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/hfi_platform.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_platform.c b/drivers/media/platform/qcom/venus/hfi_platform.c
> index f07f554bc5fe..d163d5b0e6b7 100644
> --- a/drivers/media/platform/qcom/venus/hfi_platform.c
> +++ b/drivers/media/platform/qcom/venus/hfi_platform.c
> @@ -80,7 +80,7 @@ hfi_platform_get_codecs(struct venus_core *core, u32 *enc_codecs, u32 *dec_codec
>   	if (plat->codecs)
>   		plat->codecs(enc_codecs, dec_codecs, count);
>   
> -	if (of_device_is_compatible(core->dev->of_node, "qcom,sc7280-venus")) {
> +	if (IS_IRIS2_1(core)) {
>   		*enc_codecs &= ~HFI_VIDEO_CODEC_VP8;
>   		*dec_codecs &= ~HFI_VIDEO_CODEC_VP8;
>   	}
>

[PATCH v2 15/18] media: venus: vdec: Fix version check in vdec_set_work_route()

[Konrad Dybcio]

This is not so much V6-dependent as it's IRIS(1|2|2_1). Fix it.

Fixes: 6483a8cbea54 ("media: venus: vdec: set work route to fw")
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 51a53bf82bd3..33e3f7208b1a 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -701,7 +701,7 @@ static int vdec_set_work_route(struct venus_inst *inst)
 	u32 ptype = HFI_PROPERTY_PARAM_WORK_ROUTE;
 	struct hfi_video_work_route wr;
 
-	if (!IS_V6(inst->core))
+	if (!(IS_IRIS1(inst->core) || IS_IRIS2(inst->core) || IS_IRIS2_1(inst->core)))
 		return 0;
 
 	wr.video_work_route = inst->core->res->num_vpp_pipes;

-- 
2.40.1

Re: [PATCH v2 15/18] media: venus: vdec: Fix version check in vdec_set_work_route()

[Vikash Garodia]

On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> This is not so much V6-dependent as it's IRIS(1|2|2_1). Fix it.
Again, why is it marked as fix ?
>
> Fixes: 6483a8cbea54 ("media: venus: vdec: set work route to fw")
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/vdec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 51a53bf82bd3..33e3f7208b1a 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -701,7 +701,7 @@ static int vdec_set_work_route(struct venus_inst *inst)
>   	u32 ptype = HFI_PROPERTY_PARAM_WORK_ROUTE;
>   	struct hfi_video_work_route wr;
>   
> -	if (!IS_V6(inst->core))
> +	if (!(IS_IRIS1(inst->core) || IS_IRIS2(inst->core) || IS_IRIS2_1(inst->core)))

Not a good idea to add IRIS1 just for deciding work route and not at 
other places in driver. Add IRIS1 relevant

code in other aspects as well, if the patch needs to handle anything 
w.r.t IRIS1.

>   		return 0;
>   
>   	wr.video_work_route = inst->core->res->num_vpp_pipes;
>

Re: [PATCH v2 15/18] media: venus: vdec: Fix version check in vdec_set_work_route()

[Konrad Dybcio]

On 5.05.2023 16:02, Vikash Garodia wrote:
> 
> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>> This is not so much V6-dependent as it's IRIS(1|2|2_1). Fix it.
> Again, why is it marked as fix ?
It corrects the logic but does not manifest on currently
supported hardware. I'll reword it and drop the fixes tag.

>>
>> Fixes: 6483a8cbea54 ("media: venus: vdec: set work route to fw")
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>> index 51a53bf82bd3..33e3f7208b1a 100644
>> --- a/drivers/media/platform/qcom/venus/vdec.c
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>> @@ -701,7 +701,7 @@ static int vdec_set_work_route(struct venus_inst *inst)
>>       u32 ptype = HFI_PROPERTY_PARAM_WORK_ROUTE;
>>       struct hfi_video_work_route wr;
>>   -    if (!IS_V6(inst->core))
>> +    if (!(IS_IRIS1(inst->core) || IS_IRIS2(inst->core) || IS_IRIS2_1(inst->core)))
> 
> Not a good idea to add IRIS1 just for deciding work route and not at other places in driver. Add IRIS1 relevant
> 
> code in other aspects as well, if the patch needs to handle anything w.r.t IRIS1.
I'd say that correcting this condition is fair regardless. I can
however delay this patch until IRIS1 enablement if you'd prefer
that.

Konrad
> 
>>           return 0;
>>         wr.video_work_route = inst->core->res->num_vpp_pipes;
>>

Re: [PATCH v2 15/18] media: venus: vdec: Fix version check in vdec_set_work_route()

[Vikash Garodia]

On 5/6/2023 12:45 AM, Konrad Dybcio wrote:
> 
> 
> On 5.05.2023 16:02, Vikash Garodia wrote:
>>
>> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>>> This is not so much V6-dependent as it's IRIS(1|2|2_1). Fix it.
>> Again, why is it marked as fix ?
> It corrects the logic but does not manifest on currently
> supported hardware. I'll reword it and drop the fixes tag.
> 
>>>
>>> Fixes: 6483a8cbea54 ("media: venus: vdec: set work route to fw")
>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>   drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>>> index 51a53bf82bd3..33e3f7208b1a 100644
>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>> @@ -701,7 +701,7 @@ static int vdec_set_work_route(struct venus_inst *inst)
>>>       u32 ptype = HFI_PROPERTY_PARAM_WORK_ROUTE;
>>>       struct hfi_video_work_route wr;
>>>   -    if (!IS_V6(inst->core))
>>> +    if (!(IS_IRIS1(inst->core) || IS_IRIS2(inst->core) || IS_IRIS2_1(inst->core)))
>>
>> Not a good idea to add IRIS1 just for deciding work route and not at other places in driver. Add IRIS1 relevant
>>
>> code in other aspects as well, if the patch needs to handle anything w.r.t IRIS1.
> I'd say that correcting this condition is fair regardless. I can
> however delay this patch until IRIS1 enablement if you'd prefer
> that.

Lets add IRIS1 along with other IRIS1 changes.

> Konrad
>>
>>>           return 0;
>>>         wr.video_work_route = inst->core->res->num_vpp_pipes;
>>>

[PATCH v2 16/18] media: venus: Introduce accessors for remapped hfi_buffer_reqs members

[Konrad Dybcio]

Currently we have macros to access these, but they don't provide a
way to override the remapped fields. Replace the macros with actual
get/set pairs to fix that.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/helpers.c    |  2 +-
 drivers/media/platform/qcom/venus/hfi_helper.h | 64 +++++++++++++++++++++-----
 drivers/media/platform/qcom/venus/hfi_msgs.c   |  2 +-
 drivers/media/platform/qcom/venus/vdec.c       |  8 ++--
 drivers/media/platform/qcom/venus/vdec_ctrls.c |  2 +-
 drivers/media/platform/qcom/venus/venc.c       |  4 +-
 drivers/media/platform/qcom/venus/venc_ctrls.c |  2 +-
 7 files changed, 63 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index a2ceab7f9ddb..1ce2624abc12 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -189,7 +189,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
 	if (ret)
 		return ret;
 
-	count = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
+	count = hfi_bufreq_get_count_min(&bufreq, ver);
 
 	for (i = 0; i < count; i++) {
 		buf = kzalloc(sizeof(*buf), GFP_KERNEL);
diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
index e0c8f15644df..1e06227ad240 100644
--- a/drivers/media/platform/qcom/venus/hfi_helper.h
+++ b/drivers/media/platform/qcom/venus/hfi_helper.h
@@ -1168,17 +1168,6 @@ struct hfi_buffer_display_hold_count_actual {
 	u32 hold_count;
 };
 
-/* HFI 4XX reorder the fields, use these macros */
-#define HFI_BUFREQ_HOLD_COUNT(bufreq, ver)	\
-	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
-	? 0 : (bufreq)->hold_count)
-#define HFI_BUFREQ_COUNT_MIN(bufreq, ver)	\
-	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
-	? (bufreq)->hold_count : (bufreq)->count_min)
-#define HFI_BUFREQ_COUNT_MIN_HOST(bufreq, ver)	\
-	((ver) == HFI_VERSION_4XX || (ver) == HFI_VERSION_6XX \
-	? (bufreq)->count_min : 0)
-
 struct hfi_buffer_requirements {
 	u32 type;
 	u32 size;
@@ -1190,6 +1179,59 @@ struct hfi_buffer_requirements {
 	u32 alignment;
 };
 
+/* Starting with HFI 4XX, some properties were swapped.. */
+static inline u32 hfi_bufreq_get_hold_count(struct hfi_buffer_requirements *req,
+					    u32 ver)
+{
+	if (ver == HFI_VERSION_4XX || ver == HFI_VERSION_6XX)
+		return 0;
+
+	return req->hold_count;
+};
+
+static inline u32 hfi_bufreq_get_count_min(struct hfi_buffer_requirements *req,
+					   u32 ver)
+{
+	if (ver == HFI_VERSION_4XX || ver == HFI_VERSION_6XX)
+		return req->hold_count;
+
+	return req->count_min;
+};
+
+static inline u32 hfi_bufreq_get_count_min_host(struct hfi_buffer_requirements *req,
+						u32 ver)
+{
+	if (ver == HFI_VERSION_4XX || ver == HFI_VERSION_6XX)
+		return req->count_min;
+
+	return 0;
+};
+
+static inline void hfi_bufreq_set_hold_count(struct hfi_buffer_requirements *req,
+					     u32 ver, u32 val)
+{
+	if (ver == HFI_VERSION_4XX || ver == HFI_VERSION_6XX)
+		return;
+
+	req->hold_count = val;
+};
+
+static inline void hfi_bufreq_set_count_min(struct hfi_buffer_requirements *req,
+					    u32 ver, u32 val)
+{
+	if (ver == HFI_VERSION_4XX || ver == HFI_VERSION_6XX)
+		req->hold_count = val;
+
+	req->count_min = val;
+};
+
+static inline void hfi_bufreq_set_count_min_host(struct hfi_buffer_requirements *req,
+						 u32 ver, u32 val)
+{
+	if (ver == HFI_VERSION_4XX || ver == HFI_VERSION_6XX)
+		req->count_min = val;
+};
+
 struct hfi_data_payload {
 	u32 size;
 	u8 data[1];
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index df96db3761a7..c320ebbdb24e 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -99,7 +99,7 @@ static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
 		case HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS:
 			data_ptr += sizeof(u32);
 			bufreq = (struct hfi_buffer_requirements *)data_ptr;
-			event.buf_count = HFI_BUFREQ_COUNT_MIN(bufreq, ver);
+			event.buf_count = hfi_bufreq_get_count_min(bufreq, ver);
 			data_ptr += sizeof(*bufreq);
 			break;
 		case HFI_INDEX_EXTRADATA_INPUT_CROP:
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 33e3f7208b1a..2f0a95860c94 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -870,13 +870,13 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num,
 	if (ret)
 		return ret;
 
-	*in_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
+	*in_num = hfi_bufreq_get_count_min(&bufreq, ver);
 
 	ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq);
 	if (ret)
 		return ret;
 
-	*out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
+	*out_num = hfi_bufreq_get_count_min(&bufreq, ver);
 
 	return 0;
 }
@@ -990,14 +990,14 @@ static int vdec_verify_conf(struct venus_inst *inst)
 		return ret;
 
 	if (inst->num_output_bufs < bufreq.count_actual ||
-	    inst->num_output_bufs < HFI_BUFREQ_COUNT_MIN(&bufreq, ver))
+	    inst->num_output_bufs < hfi_bufreq_get_count_min(&bufreq, ver))
 		return -EINVAL;
 
 	ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
 	if (ret)
 		return ret;
 
-	if (inst->num_input_bufs < HFI_BUFREQ_COUNT_MIN(&bufreq, ver))
+	if (inst->num_input_bufs < hfi_bufreq_get_count_min(&bufreq, ver))
 		return -EINVAL;
 
 	return 0;
diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c
index fbe12a608b21..7e0f29bf7fae 100644
--- a/drivers/media/platform/qcom/venus/vdec_ctrls.c
+++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c
@@ -79,7 +79,7 @@ static int vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 		ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq);
 		if (!ret)
-			ctrl->val = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
+			ctrl->val = hfi_bufreq_get_count_min(&bufreq, ver);
 		break;
 	default:
 		return -EINVAL;
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 4666f42feea3..42cbb1619463 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1202,7 +1202,7 @@ static int venc_verify_conf(struct venus_inst *inst)
 		return ret;
 
 	if (inst->num_output_bufs < bufreq.count_actual ||
-	    inst->num_output_bufs < HFI_BUFREQ_COUNT_MIN(&bufreq, ver))
+	    inst->num_output_bufs < hfi_bufreq_get_count_min(&bufreq, ver))
 		return -EINVAL;
 
 	ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
@@ -1210,7 +1210,7 @@ static int venc_verify_conf(struct venus_inst *inst)
 		return ret;
 
 	if (inst->num_input_bufs < bufreq.count_actual ||
-	    inst->num_input_bufs < HFI_BUFREQ_COUNT_MIN(&bufreq, ver))
+	    inst->num_input_bufs < hfi_bufreq_get_count_min(&bufreq, ver))
 		return -EINVAL;
 
 	return 0;
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 7468e43800a9..d9d2a293f3ef 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -358,7 +358,7 @@ static int venc_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
 		ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq);
 		if (!ret)
-			ctrl->val = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
+			ctrl->val = hfi_bufreq_get_count_min(&bufreq, ver);
 		break;
 	default:
 		return -EINVAL;

-- 
2.40.1

[PATCH v2 17/18] media: venus: Use newly-introduced hfi_buffer_requirements accessors

[Konrad Dybcio]

Now that we have a way which is independent of the HFI version to set
the correct fields in hfi_buffer_requirements, use it!

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/helpers.c        |  5 +++--
 .../media/platform/qcom/venus/hfi_plat_bufs_v6.c   | 22 +++++++++++-----------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 1ce2624abc12..0268129ab9ac 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -667,6 +667,7 @@ int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
 			    struct hfi_buffer_requirements *req)
 {
 	u32 ptype = HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS;
+	enum hfi_version ver = inst->core->res->hfi_version;
 	union hfi_get_property hprop;
 	unsigned int i;
 	int ret;
@@ -674,12 +675,12 @@ int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
 	memset(req, 0, sizeof(*req));
 
 	if (type == HFI_BUFFER_OUTPUT || type == HFI_BUFFER_OUTPUT2)
-		req->count_min = inst->fw_min_cnt;
+		hfi_bufreq_set_count_min(req, ver, inst->fw_min_cnt);
 
 	ret = platform_get_bufreq(inst, type, req);
 	if (!ret) {
 		if (type == HFI_BUFFER_OUTPUT || type == HFI_BUFFER_OUTPUT2)
-			inst->fw_min_cnt = req->count_min;
+			inst->fw_min_cnt = hfi_bufreq_get_count_min(req, ver);
 		return 0;
 	}
 
diff --git a/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c b/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
index a9be31ec6927..5eb4032bc551 100644
--- a/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
+++ b/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
@@ -1214,25 +1214,25 @@ static int bufreq_dec(struct hfi_plat_buffers_params *params, u32 buftype,
 
 	out_min_count = output_buffer_count(VIDC_SESSION_TYPE_DEC, codec);
 	/* Max of driver and FW count */
-	out_min_count = max(out_min_count, bufreq->count_min);
+	out_min_count = max(out_min_count, hfi_bufreq_get_count_min(bufreq, version));
 
 	bufreq->type = buftype;
 	bufreq->region_size = 0;
-	bufreq->count_min = 1;
 	bufreq->count_actual = 1;
-	bufreq->hold_count = 1;
+	hfi_bufreq_set_count_min(bufreq, version, 1);
+	hfi_bufreq_set_count_min_host(bufreq, version, 1);
 	bufreq->contiguous = 1;
 	bufreq->alignment = 256;
 
 	if (buftype == HFI_BUFFER_INPUT) {
-		bufreq->count_min = MIN_INPUT_BUFFERS;
+		hfi_bufreq_set_count_min(bufreq, version, MIN_INPUT_BUFFERS);
 		bufreq->size =
 			calculate_dec_input_frame_size(width, height, codec,
 						       max_mbs_per_frame,
 						       buffer_size_limit);
 	} else if (buftype == HFI_BUFFER_OUTPUT ||
 		   buftype == HFI_BUFFER_OUTPUT2) {
-		bufreq->count_min = out_min_count;
+		hfi_bufreq_set_count_min(bufreq, version, out_min_count);
 		bufreq->size =
 			venus_helper_get_framesz_raw(params->hfi_color_fmt,
 						     width, height);
@@ -1264,7 +1264,7 @@ static int bufreq_enc(struct hfi_plat_buffers_params *params, u32 buftype,
 	u32 work_mode = params->enc.work_mode;
 	u32 rc_type = params->enc.rc_type;
 	u32 num_vpp_pipes = params->num_vpp_pipes;
-	u32 num_ref;
+	u32 num_ref, count_min;
 
 	switch (codec) {
 	case V4L2_PIX_FMT_H264:
@@ -1284,21 +1284,21 @@ static int bufreq_enc(struct hfi_plat_buffers_params *params, u32 buftype,
 
 	bufreq->type = buftype;
 	bufreq->region_size = 0;
-	bufreq->count_min = 1;
 	bufreq->count_actual = 1;
-	bufreq->hold_count = 1;
+	hfi_bufreq_set_count_min(bufreq, version, 1);
+	hfi_bufreq_set_count_min_host(bufreq, version, 1);
 	bufreq->contiguous = 1;
 	bufreq->alignment = 256;
 
 	if (buftype == HFI_BUFFER_INPUT) {
-		bufreq->count_min = MIN_INPUT_BUFFERS;
+		hfi_bufreq_set_count_min(bufreq, version, MIN_INPUT_BUFFERS);
 		bufreq->size =
 			venus_helper_get_framesz_raw(params->hfi_color_fmt,
 						     width, height);
 	} else if (buftype == HFI_BUFFER_OUTPUT ||
 		   buftype == HFI_BUFFER_OUTPUT2) {
-		bufreq->count_min =
-			output_buffer_count(VIDC_SESSION_TYPE_ENC, codec);
+		count_min = output_buffer_count(VIDC_SESSION_TYPE_ENC, codec);
+		hfi_bufreq_set_count_min(bufreq, version, count_min);
 		bufreq->size = calculate_enc_output_frame_size(width, height,
 							       rc_type);
 	} else if (buftype == HFI_BUFFER_INTERNAL_SCRATCH(version)) {

-- 
2.40.1

[PATCH v2 18/18] media: venus: hfi_venus: Restrict writing SCIACMDARG3 to Venus V1/V2

[Konrad Dybcio]

This write was last present on msm-3.10, which means before HFI3XX
platforms were introduced. Guard it with an appropriate if condition.

Does not seem to have any adverse effects on at least SM8250.

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

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index d6df99a921bb..6405771568d1 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -462,7 +462,8 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
 	}
 
 	writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
-	writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
+	if (IS_V1(hdev->core))
+		writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
 
 	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
 	while (!ctrl_status && count < max_tries) {

-- 
2.40.1

Re: [PATCH v2 00/18] Venus QoL / maintainability fixes

[Bryan O'Donoghue]

On 04/05/2023 09:00, Konrad Dybcio wrote:
> Tested on 8250, but pretty please test it on your boards too!

What's the definition of test here ?

I ran this

ffplay -codec:video h264_v4l2m2m FantasticFour-ROTSS.mp4

and this

ffplay -codec:video vp8_v4l2m2m /mnt/big-buck-bunny_trailer.webm

on db410c with no errors. Then again I applied and disapplied the 8x8 
264 fix to that branch and saw no discernable difference so I'm not very 
confident we have good coverage.

@Stan @Vikash could you give some suggested tests for coverage here ?

@Konrad - get a db410c !

My superficial first-pass on this series looks good but, before giving a 
Tested-by here, I think we should define a set of coverage tests, run 
them - the upper end on sm8250 and lower end msm8916 "makes sense to me"

20? different gstreamer tests at different formats and different sizes 
on our selected platforms db410c, rb5, rb3 I have - also an 820 I 
haven't booted and an enforce sdm660.

Which tests will we use to validate this series and subsequent series to 
ensure we don't have more regressions ?

---
bod

Re: [PATCH v2 00/18] Venus QoL / maintainability fixes

[Vikash Garodia]

On 5/12/2023 8:31 AM, Bryan O'Donoghue wrote:
> On 04/05/2023 09:00, Konrad Dybcio wrote:
>> Tested on 8250, but pretty please test it on your boards too!
> 
> What's the definition of test here ?
> 
> I ran this
> 
> ffplay -codec:video h264_v4l2m2m FantasticFour-ROTSS.mp4
> 
> and this
> 
> ffplay -codec:video vp8_v4l2m2m /mnt/big-buck-bunny_trailer.webm
> 
> on db410c with no errors. Then again I applied and disapplied the 8x8 264 fix to
> that branch and saw no discernable difference so I'm not very confident we have
> good coverage.
> 
> @Stan @Vikash could you give some suggested tests for coverage here ?

I could think of below test aspects for this series
1. Suspend Resume
2. Concurrency test
3. Module load -> video usecase -> module unload -> module load -> video
usecase. This would ensure video firmware is reloaded and functional.
4. Video playback and encode for all supported resolution and codecs.
5. In general, video playback with more test content.

I would be testing the series with stability test suite on CrOS. That would be
either on sc7180 or sc7280 setup.

Konrad, you can post the new version as one patch needs to be dropped. Test can
be done on the new version. There are few patches in the series pending review,
which can be done in parallel.

-Vikash

> 
> @Konrad - get a db410c !
> 
> My superficial first-pass on this series looks good but, before giving a
> Tested-by here, I think we should define a set of coverage tests, run them - the
> upper end on sm8250 and lower end msm8916 "makes sense to me"
> 
> 20? different gstreamer tests at different formats and different sizes on our
> selected platforms db410c, rb5, rb3 I have - also an 820 I haven't booted and an
> enforce sdm660.
> 
> Which tests will we use to validate this series and subsequent series to ensure
> we don't have more regressions ?
> 
> ---
> bod

Re: [PATCH v2 00/18] Venus QoL / maintainability fixes

[Konrad Dybcio]

On 16.05.2023 14:57, Vikash Garodia wrote:
> 
> On 5/12/2023 8:31 AM, Bryan O'Donoghue wrote:
>> On 04/05/2023 09:00, Konrad Dybcio wrote:
>>> Tested on 8250, but pretty please test it on your boards too!
>>
>> What's the definition of test here ?
>>
>> I ran this
>>
>> ffplay -codec:video h264_v4l2m2m FantasticFour-ROTSS.mp4
>>
>> and this
>>
>> ffplay -codec:video vp8_v4l2m2m /mnt/big-buck-bunny_trailer.webm
>>
>> on db410c with no errors. Then again I applied and disapplied the 8x8 264 fix to
>> that branch and saw no discernable difference so I'm not very confident we have
>> good coverage.
>>
>> @Stan @Vikash could you give some suggested tests for coverage here ?
> 
> I could think of below test aspects for this series
> 1. Suspend Resume
> 2. Concurrency test
> 3. Module load -> video usecase -> module unload -> module load -> video
> usecase. This would ensure video firmware is reloaded and functional.
> 4. Video playback and encode for all supported resolution and codecs.
> 5. In general, video playback with more test content.
> 
> I would be testing the series with stability test suite on CrOS. That would be
> either on sc7180 or sc7280 setup.
> 
> Konrad, you can post the new version as one patch needs to be dropped. Test can
> be done on the new version. There are few patches in the series pending review,
> which can be done in parallel.
Sure, working on it!

Konrad
> 
> -Vikash
> 
>>
>> @Konrad - get a db410c !
>>
>> My superficial first-pass on this series looks good but, before giving a
>> Tested-by here, I think we should define a set of coverage tests, run them - the
>> upper end on sm8250 and lower end msm8916 "makes sense to me"
>>
>> 20? different gstreamer tests at different formats and different sizes on our
>> selected platforms db410c, rb5, rb3 I have - also an 820 I haven't booted and an
>> enforce sdm660.
>>
>> Which tests will we use to validate this series and subsequent series to ensure
>> we don't have more regressions ?
>>
>> ---
>> bod

Re: [PATCH v2 00/18] Venus QoL / maintainability fixes

[Konrad Dybcio]

On 12.05.2023 05:01, Bryan O'Donoghue wrote:
> On 04/05/2023 09:00, Konrad Dybcio wrote:
>> Tested on 8250, but pretty please test it on your boards too!
> 
> What's the definition of test here ?
> 
> I ran this
> 
> ffplay -codec:video h264_v4l2m2m FantasticFour-ROTSS.mp4
> 
> and this
> 
> ffplay -codec:video vp8_v4l2m2m /mnt/big-buck-bunny_trailer.webm
> 
> on db410c with no errors. Then again I applied and disapplied the 8x8 264 fix to that branch and saw no discernable difference so I'm not very confident we have good coverage.
I don't think we have any coverage at all, especially considering
there were 1 or 2 patches fixing vdec not working at all in the past
few months.. Maybe CrOS has some internal pipelines for this.

> 
> @Stan @Vikash could you give some suggested tests for coverage here ?
> 
> @Konrad - get a db410c !
Don't think they're even made anymore!

> 
> My superficial first-pass on this series looks good but, before giving a Tested-by here, I think we should define a set of coverage tests, run them - the upper end on sm8250 and lower end msm8916 "makes sense to me"
> 
> 20? different gstreamer tests at different formats and different sizes on our selected platforms db410c, rb5, rb3 I have - also an 820 I haven't booted and an enforce sdm660.
> 
> Which tests will we use to validate this series and subsequent series to ensure we don't have more regressions ?
Personally I've done:

- boot and check if venus still probes and doesn't shout in dmesg
- decode and re-encode a H264 file

which is far from perfect..

Konrad
> 
> ---
> bod