* Re: [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers
2025-07-02 13:42 [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers Krzysztof Kozlowski
@ 2025-07-02 14:02 ` Konrad Dybcio
2025-07-02 14:06 ` Krzysztof Kozlowski
2025-07-02 15:07 ` Bryan O'Donoghue
2025-07-02 17:20 ` Vikash Garodia
2 siblings, 1 reply; 8+ messages in thread
From: Konrad Dybcio @ 2025-07-02 14:02 UTC (permalink / raw)
To: Krzysztof Kozlowski, Vikash Garodia, Dikshita Agarwal,
Abhinav Kumar, Bryan O'Donoghue, Mauro Carvalho Chehab,
linux-media, linux-arm-msm, linux-kernel
On 7/2/25 3:42 PM, Krzysztof Kozlowski wrote:
> Driver implements different callbacks for power off hardware
> (.power_off_hw) and power off controller (.power_off_controller):
>
> - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
> - iris_vpu3_power_off_hardware,
> - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,
>
> The first group (iris_vpu_power_off_hw() and
> iris_vpu_power_off_controller()) is used on older VPU2 designs but also
> called from newer ones: iris_vpu3_power_off_hardware() calls
> iris_vpu_power_off_controller().
>
> In the same time there is wrapper iris_vpu_power_off() which calls
> respective callbacks (the VPU2, VPU3 etc).
>
> Let's make it more obvious which function is a generic wrapper over
> specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
> the callback by adding "2" to callbacks used on VPU2. No functional
> changes.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
[...]
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 9b7c9a1495ee..a2c8a1650153 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> @@ -104,7 +104,7 @@ static void iris_vpu3_power_off_hardware(struct iris_core *core)
> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>
> disable_power:
> - iris_vpu_power_off_hw(core);
> + iris_vpu2_power_off_hw(core);
> }
>
> static void iris_vpu33_power_off_hardware(struct iris_core *core)
> @@ -142,7 +142,7 @@ static void iris_vpu33_power_off_hardware(struct iris_core *core)
> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>
> disable_power:
> - iris_vpu_power_off_hw(core);
> + iris_vpu2_power_off_hw(core);
> }
I don't really like how v3 calls v2 ops internally.. and there's
nothing really vpu2-specific about what the function does.
Maybe something along the lines of "iris_disable_resources"?
Konrad
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers
2025-07-02 14:02 ` Konrad Dybcio
@ 2025-07-02 14:06 ` Krzysztof Kozlowski
0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-02 14:06 UTC (permalink / raw)
To: Konrad Dybcio, Vikash Garodia, Dikshita Agarwal, Abhinav Kumar,
Bryan O'Donoghue, Mauro Carvalho Chehab, linux-media,
linux-arm-msm, linux-kernel
On 02/07/2025 16:02, Konrad Dybcio wrote:
> On 7/2/25 3:42 PM, Krzysztof Kozlowski wrote:
>> Driver implements different callbacks for power off hardware
>> (.power_off_hw) and power off controller (.power_off_controller):
>>
>> - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
>> - iris_vpu3_power_off_hardware,
>> - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,
>>
>> The first group (iris_vpu_power_off_hw() and
>> iris_vpu_power_off_controller()) is used on older VPU2 designs but also
>> called from newer ones: iris_vpu3_power_off_hardware() calls
>> iris_vpu_power_off_controller().
>>
>> In the same time there is wrapper iris_vpu_power_off() which calls
>> respective callbacks (the VPU2, VPU3 etc).
>>
>> Let's make it more obvious which function is a generic wrapper over
>> specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
>> the callback by adding "2" to callbacks used on VPU2. No functional
>> changes.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>
> [...]
>
>> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> index 9b7c9a1495ee..a2c8a1650153 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> @@ -104,7 +104,7 @@ static void iris_vpu3_power_off_hardware(struct iris_core *core)
>> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>
>> disable_power:
>> - iris_vpu_power_off_hw(core);
>> + iris_vpu2_power_off_hw(core);
>> }
>>
>> static void iris_vpu33_power_off_hardware(struct iris_core *core)
>> @@ -142,7 +142,7 @@ static void iris_vpu33_power_off_hardware(struct iris_core *core)
>> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>
>> disable_power:
>> - iris_vpu_power_off_hw(core);
>> + iris_vpu2_power_off_hw(core);
>> }
>
> I don't really like how v3 calls v2 ops internally.. and there's
> nothing really vpu2-specific about what the function does.
> Maybe something along the lines of "iris_disable_resources"?
Context: sm8750 comes with more resources, so it will come with vpu35
doing this differently.
That's why any generic name (non platform specific) is misleading IMO.
This is really disabling/power off for the VPU2, 3 and 3.3. Not newer,
at least after initial look at SM8750.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers
2025-07-02 13:42 [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers Krzysztof Kozlowski
2025-07-02 14:02 ` Konrad Dybcio
@ 2025-07-02 15:07 ` Bryan O'Donoghue
2025-07-02 15:31 ` Krzysztof Kozlowski
2025-07-02 17:20 ` Vikash Garodia
2 siblings, 1 reply; 8+ messages in thread
From: Bryan O'Donoghue @ 2025-07-02 15:07 UTC (permalink / raw)
To: Krzysztof Kozlowski, Vikash Garodia, Dikshita Agarwal,
Abhinav Kumar, Mauro Carvalho Chehab, linux-media, linux-arm-msm,
linux-kernel
On 02/07/2025 14:42, Krzysztof Kozlowski wrote:
> Driver implements different callbacks for power off hardware
> (.power_off_hw) and power off controller (.power_off_controller):
>
> - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
> - iris_vpu3_power_off_hardware,
> - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,
>
> The first group (iris_vpu_power_off_hw() and
> iris_vpu_power_off_controller()) is used on older VPU2 designs but also
> called from newer ones: iris_vpu3_power_off_hardware() calls
> iris_vpu_power_off_controller().
>
> In the same time there is wrapper iris_vpu_power_off() which calls
> respective callbacks (the VPU2, VPU3 etc).
>
> Let's make it more obvious which function is a generic wrapper over
> specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
> the callback by adding "2" to callbacks used on VPU2. No functional
> changes.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> drivers/media/platform/qcom/iris/iris_vpu2.c | 4 ++--
> drivers/media/platform/qcom/iris/iris_vpu3x.c | 6 +++---
> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 ++--
> drivers/media/platform/qcom/iris/iris_vpu_common.h | 4 ++--
> 4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu2.c b/drivers/media/platform/qcom/iris/iris_vpu2.c
> index 7cf1bfc352d3..2570e65816f6 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu2.c
> @@ -33,7 +33,7 @@ static u64 iris_vpu2_calc_freq(struct iris_inst *inst, size_t data_size)
> }
>
> const struct vpu_ops iris_vpu2_ops = {
> - .power_off_hw = iris_vpu_power_off_hw,
> - .power_off_controller = iris_vpu_power_off_controller,
> + .power_off_hw = iris_vpu2_power_off_hw,
> + .power_off_controller = iris_vpu2_power_off_controller,
> .calc_freq = iris_vpu2_calc_freq,
> };
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 9b7c9a1495ee..a2c8a1650153 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> @@ -104,7 +104,7 @@ static void iris_vpu3_power_off_hardware(struct iris_core *core)
> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>
> disable_power:
> - iris_vpu_power_off_hw(core);
> + iris_vpu2_power_off_hw(core);
> }
>
> static void iris_vpu33_power_off_hardware(struct iris_core *core)
> @@ -142,7 +142,7 @@ static void iris_vpu33_power_off_hardware(struct iris_core *core)
> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>
> disable_power:
> - iris_vpu_power_off_hw(core);
> + iris_vpu2_power_off_hw(core);
> }
>
> static int iris_vpu33_power_off_controller(struct iris_core *core)
> @@ -264,7 +264,7 @@ static u64 iris_vpu3x_calculate_frequency(struct iris_inst *inst, size_t data_si
>
> const struct vpu_ops iris_vpu3_ops = {
> .power_off_hw = iris_vpu3_power_off_hardware,
> - .power_off_controller = iris_vpu_power_off_controller,
> + .power_off_controller = iris_vpu2_power_off_controller,
> .calc_freq = iris_vpu3x_calculate_frequency,
> };
>
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 42a7c53ce48e..22f190e0c7c6 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -211,7 +211,7 @@ int iris_vpu_prepare_pc(struct iris_core *core)
> return -EAGAIN;
> }
>
> -int iris_vpu_power_off_controller(struct iris_core *core)
> +int iris_vpu2_power_off_controller(struct iris_core *core)
> {
> u32 val = 0;
> int ret;
> @@ -253,7 +253,7 @@ int iris_vpu_power_off_controller(struct iris_core *core)
> return 0;
> }
>
> -void iris_vpu_power_off_hw(struct iris_core *core)
> +void iris_vpu2_power_off_hw(struct iris_core *core)
> {
> dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
> iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> index 93b7fa27be3b..8f63f243dd0d 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> @@ -24,8 +24,8 @@ void iris_vpu_clear_interrupt(struct iris_core *core);
> int iris_vpu_watchdog(struct iris_core *core, u32 intr_status);
> int iris_vpu_prepare_pc(struct iris_core *core);
> int iris_vpu_power_on(struct iris_core *core);
> -int iris_vpu_power_off_controller(struct iris_core *core);
> -void iris_vpu_power_off_hw(struct iris_core *core);
> +int iris_vpu2_power_off_controller(struct iris_core *core);
> +void iris_vpu2_power_off_hw(struct iris_core *core);
> void iris_vpu_power_off(struct iris_core *core);
>
> #endif
I prefer these names with an explicit v2 more logical/consistent with
the hw names.
Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers
2025-07-02 15:07 ` Bryan O'Donoghue
@ 2025-07-02 15:31 ` Krzysztof Kozlowski
0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-02 15:31 UTC (permalink / raw)
To: Bryan O'Donoghue, Vikash Garodia, Dikshita Agarwal,
Abhinav Kumar, Mauro Carvalho Chehab, linux-media, linux-arm-msm,
linux-kernel
On 02/07/2025 17:07, Bryan O'Donoghue wrote:
>> {
>> dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
>> iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
>> index 93b7fa27be3b..8f63f243dd0d 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
>> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
>> @@ -24,8 +24,8 @@ void iris_vpu_clear_interrupt(struct iris_core *core);
>> int iris_vpu_watchdog(struct iris_core *core, u32 intr_status);
>> int iris_vpu_prepare_pc(struct iris_core *core);
>> int iris_vpu_power_on(struct iris_core *core);
>> -int iris_vpu_power_off_controller(struct iris_core *core);
>> -void iris_vpu_power_off_hw(struct iris_core *core);
>> +int iris_vpu2_power_off_controller(struct iris_core *core);
>> +void iris_vpu2_power_off_hw(struct iris_core *core);
>> void iris_vpu_power_off(struct iris_core *core);
>>
>> #endif
>
> I prefer these names with an explicit v2 more logical/consistent with
> the hw names.
>
> Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Thanks.
I got some patchwork complains, so it seems I missed something which
clang did not catch. There will be a v2.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers
2025-07-02 13:42 [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers Krzysztof Kozlowski
2025-07-02 14:02 ` Konrad Dybcio
2025-07-02 15:07 ` Bryan O'Donoghue
@ 2025-07-02 17:20 ` Vikash Garodia
2025-07-02 17:29 ` Krzysztof Kozlowski
2 siblings, 1 reply; 8+ messages in thread
From: Vikash Garodia @ 2025-07-02 17:20 UTC (permalink / raw)
To: Krzysztof Kozlowski, Dikshita Agarwal, Abhinav Kumar,
Bryan O'Donoghue, Mauro Carvalho Chehab, linux-media,
linux-arm-msm, linux-kernel
On 7/2/2025 7:12 PM, Krzysztof Kozlowski wrote:
> Driver implements different callbacks for power off hardware
> (.power_off_hw) and power off controller (.power_off_controller):
>
> - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
> - iris_vpu3_power_off_hardware,
> - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,
>
> The first group (iris_vpu_power_off_hw() and
> iris_vpu_power_off_controller()) is used on older VPU2 designs but also
> called from newer ones: iris_vpu3_power_off_hardware() calls
> iris_vpu_power_off_controller().
>
> In the same time there is wrapper iris_vpu_power_off() which calls
> respective callbacks (the VPU2, VPU3 etc).
>
> Let's make it more obvious which function is a generic wrapper over
> specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
> the callback by adding "2" to callbacks used on VPU2. No functional
> changes.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> drivers/media/platform/qcom/iris/iris_vpu2.c | 4 ++--
> drivers/media/platform/qcom/iris/iris_vpu3x.c | 6 +++---
> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 ++--
> drivers/media/platform/qcom/iris/iris_vpu_common.h | 4 ++--
> 4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu2.c b/drivers/media/platform/qcom/iris/iris_vpu2.c
> index 7cf1bfc352d3..2570e65816f6 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu2.c
> @@ -33,7 +33,7 @@ static u64 iris_vpu2_calc_freq(struct iris_inst *inst, size_t data_size)
> }
>
> const struct vpu_ops iris_vpu2_ops = {
> - .power_off_hw = iris_vpu_power_off_hw,
> - .power_off_controller = iris_vpu_power_off_controller,
> + .power_off_hw = iris_vpu2_power_off_hw,
> + .power_off_controller = iris_vpu2_power_off_controller,
There was a reason to name it as VPU* independent, as it can be used for
multiple VPUs. There isn't any VPU specific code within iris_vpu_power_off_hw
that it needs to be associated to any VPU.
> .calc_freq = iris_vpu2_calc_freq,
> };
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 9b7c9a1495ee..a2c8a1650153 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> @@ -104,7 +104,7 @@ static void iris_vpu3_power_off_hardware(struct iris_core *core)
> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>
> disable_power:
> - iris_vpu_power_off_hw(core);
> + iris_vpu2_power_off_hw(core);
Again, its like VPU3 does something specific and then reuses the common handling.
I do not see a point in making this change.
Regards,
Vikash
> }
>
> static void iris_vpu33_power_off_hardware(struct iris_core *core)
> @@ -142,7 +142,7 @@ static void iris_vpu33_power_off_hardware(struct iris_core *core)
> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>
> disable_power:
> - iris_vpu_power_off_hw(core);
> + iris_vpu2_power_off_hw(core);
> }
>
> static int iris_vpu33_power_off_controller(struct iris_core *core)
> @@ -264,7 +264,7 @@ static u64 iris_vpu3x_calculate_frequency(struct iris_inst *inst, size_t data_si
>
> const struct vpu_ops iris_vpu3_ops = {
> .power_off_hw = iris_vpu3_power_off_hardware,
> - .power_off_controller = iris_vpu_power_off_controller,
> + .power_off_controller = iris_vpu2_power_off_controller,
> .calc_freq = iris_vpu3x_calculate_frequency,
> };
>
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 42a7c53ce48e..22f190e0c7c6 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -211,7 +211,7 @@ int iris_vpu_prepare_pc(struct iris_core *core)
> return -EAGAIN;
> }
>
> -int iris_vpu_power_off_controller(struct iris_core *core)
> +int iris_vpu2_power_off_controller(struct iris_core *core)
> {
> u32 val = 0;
> int ret;
> @@ -253,7 +253,7 @@ int iris_vpu_power_off_controller(struct iris_core *core)
> return 0;
> }
>
> -void iris_vpu_power_off_hw(struct iris_core *core)
> +void iris_vpu2_power_off_hw(struct iris_core *core)
> {
> dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
> iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> index 93b7fa27be3b..8f63f243dd0d 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> @@ -24,8 +24,8 @@ void iris_vpu_clear_interrupt(struct iris_core *core);
> int iris_vpu_watchdog(struct iris_core *core, u32 intr_status);
> int iris_vpu_prepare_pc(struct iris_core *core);
> int iris_vpu_power_on(struct iris_core *core);
> -int iris_vpu_power_off_controller(struct iris_core *core);
> -void iris_vpu_power_off_hw(struct iris_core *core);
> +int iris_vpu2_power_off_controller(struct iris_core *core);
> +void iris_vpu2_power_off_hw(struct iris_core *core);
> void iris_vpu_power_off(struct iris_core *core);
>
> #endif
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers
2025-07-02 17:20 ` Vikash Garodia
@ 2025-07-02 17:29 ` Krzysztof Kozlowski
2025-07-02 17:46 ` Vikash Garodia
0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-02 17:29 UTC (permalink / raw)
To: Vikash Garodia, Dikshita Agarwal, Abhinav Kumar,
Bryan O'Donoghue, Mauro Carvalho Chehab, linux-media,
linux-arm-msm, linux-kernel
On 02/07/2025 19:20, Vikash Garodia wrote:
>>
>> const struct vpu_ops iris_vpu2_ops = {
>> - .power_off_hw = iris_vpu_power_off_hw,
>> - .power_off_controller = iris_vpu_power_off_controller,
>> + .power_off_hw = iris_vpu2_power_off_hw,
>> + .power_off_controller = iris_vpu2_power_off_controller,
> There was a reason to name it as VPU* independent, as it can be used for
> multiple VPUs. There isn't any VPU specific code within iris_vpu_power_off_hw
> that it needs to be associated to any VPU.
>
>> .calc_freq = iris_vpu2_calc_freq,
>> };
>> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> index 9b7c9a1495ee..a2c8a1650153 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> @@ -104,7 +104,7 @@ static void iris_vpu3_power_off_hardware(struct iris_core *core)
>> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>
>> disable_power:
>> - iris_vpu_power_off_hw(core);
>> + iris_vpu2_power_off_hw(core);
> Again, its like VPU3 does something specific and then reuses the common handling.
>
> I do not see a point in making this change.
The point is expressed in commit msg so address that. Also, this will
not be even correct for SM8750.
>
> Regards,
> Vikash
>> }
>>
>> static void iris_vpu33_power_off_hardware(struct iris_core *core)
>> @@ -142,7 +142,7 @@ static void iris_vpu33_power_off_hardware(struct iris_core *core)
>> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>
Please kindly trim the replies from unnecessary context. It makes it
much easier to find new content.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers
2025-07-02 17:29 ` Krzysztof Kozlowski
@ 2025-07-02 17:46 ` Vikash Garodia
0 siblings, 0 replies; 8+ messages in thread
From: Vikash Garodia @ 2025-07-02 17:46 UTC (permalink / raw)
To: Krzysztof Kozlowski, Dikshita Agarwal, Abhinav Kumar,
Bryan O'Donoghue, Mauro Carvalho Chehab, linux-media,
linux-arm-msm, linux-kernel
On 7/2/2025 10:59 PM, Krzysztof Kozlowski wrote:
> On 02/07/2025 19:20, Vikash Garodia wrote:
>>>
>>> const struct vpu_ops iris_vpu2_ops = {
>>> - .power_off_hw = iris_vpu_power_off_hw,
>>> - .power_off_controller = iris_vpu_power_off_controller,
>>> + .power_off_hw = iris_vpu2_power_off_hw,
>>> + .power_off_controller = iris_vpu2_power_off_controller,
>> There was a reason to name it as VPU* independent, as it can be used for
>> multiple VPUs. There isn't any VPU specific code within iris_vpu_power_off_hw
>> that it needs to be associated to any VPU.>>
>>> .calc_freq = iris_vpu2_calc_freq,
>>> };
>>> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> index 9b7c9a1495ee..a2c8a1650153 100644
>>> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> @@ -104,7 +104,7 @@ static void iris_vpu3_power_off_hardware(struct iris_core *core)
>>> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>>
>>> disable_power:
>>> - iris_vpu_power_off_hw(core);
>>> + iris_vpu2_power_off_hw(core);
>> Again, its like VPU3 does something specific and then reuses the common handling.
>>
>> I do not see a point in making this change.
>
> The point is expressed in commit msg so address that. Also, this will
> not be even correct for SM8750.
When changes are raised for SM8750, the need of it can be reviewed then. Raise
the patch for power off for SM8750 to review the incorrectness better.
Regards,
Vikash
>
>>
>> Regards,
>> Vikash
>>> }
>>>
>>> static void iris_vpu33_power_off_hardware(struct iris_core *core)
>>> @@ -142,7 +142,7 @@ static void iris_vpu33_power_off_hardware(struct iris_core *core)
>>> writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>>
>
> Please kindly trim the replies from unnecessary context. It makes it
> much easier to find new content.
>
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread