diff mbox series

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

Message ID 20230228-topic-venus-v1-5-58c2c88384e9@linaro.org
State New
Headers
Series Venus QoL / maintainability fixes |

Commit Message

Konrad Dybcio Feb. 28, 2023, 3:24 p.m. UTC 
  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(-)

Comments

Dikshita Agarwal March 2, 2023, 11 a.m. UTC | #1 
On 2/28/2023 8:54 PM, Konrad Dybcio wrote:
> 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 4ccf31147c2a..772e5e9cf127 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -448,20 +448,21 @@ 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;
>   
>   	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
> -	if (IS_V6(hdev->core)) {
> +	if (IS_IRIS1(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {

I think the IRIS1 check can be removed from here as we are not handling 
IRIS1 related things at other places.

we can add the required checks for IRIS1 when we add support for any 
IRIS1 based chipset.

Thanks,

Dikshita

>   		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);
>   
> @@ -480,10 +481,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;
>   }
>
Konrad Dybcio March 2, 2023, 11:10 a.m. UTC | #2 
On 2.03.2023 12:00, Dikshita Agarwal wrote:
> 
> On 2/28/2023 8:54 PM, Konrad Dybcio wrote:
>> 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 4ccf31147c2a..772e5e9cf127 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -448,20 +448,21 @@ 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;
>>         writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>> -    if (IS_V6(hdev->core)) {
>> +    if (IS_IRIS1(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
> 
> I think the IRIS1 check can be removed from here as we are not handling IRIS1 related things at other places.
> 
> we can add the required checks for IRIS1 when we add support for any IRIS1 based chipset.
Up to you really, I plan on getting IRIS1 (SM8150) supported and have
some mumbling going on for that on my local branch. FWIW these checks
are logically correct and I would personally prefer not to have to go
through each one of them and remove them just to bring them back soon.

Konrad
> 
> Thanks,
> 
> Dikshita
> 
>>           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);
>>   @@ -480,10 +481,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;
>>   }
>>
Dikshita Agarwal March 2, 2023, 11:58 a.m. UTC | #3 
On 3/2/2023 4:40 PM, Konrad Dybcio wrote:
>
> On 2.03.2023 12:00, Dikshita Agarwal wrote:
>> On 2/28/2023 8:54 PM, Konrad Dybcio wrote:
>>> 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 4ccf31147c2a..772e5e9cf127 100644
>>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>>> @@ -448,20 +448,21 @@ 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;
>>>          writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
>>> -    if (IS_V6(hdev->core)) {
>>> +    if (IS_IRIS1(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
>> I think the IRIS1 check can be removed from here as we are not handling IRIS1 related things at other places.
>>
>> we can add the required checks for IRIS1 when we add support for any IRIS1 based chipset.
> Up to you really, I plan on getting IRIS1 (SM8150) supported and have
> some mumbling going on for that on my local branch. FWIW these checks
> are logically correct and I would personally prefer not to have to go
> through each one of them and remove them just to bring them back soon.
>
> Konrad

oh, I see. I wasn't aware about the plan for IRIS1.

if you plan to add these checks on all relevant places then it should be 
fine.

Thanks,

Dikshita

>> Thanks,
>>
>> Dikshita
>>
>>>            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);
>>>    @@ -480,10 +481,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;
>>>    }
>>>
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 4ccf31147c2a..772e5e9cf127 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -448,20 +448,21 @@  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;
 
 	writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
-	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);
 
@@ -480,10 +481,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;
 }