diff mbox series

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

Message ID 20230228-topic-venus-v2-1-d95d14949c79@linaro.org (mailing list archive)
State Superseded
Delegated to: Stanimir Varbanov
Headers
Series Venus QoL / maintainability fixes |

Commit Message

Konrad Dybcio May 4, 2023, 8 a.m. UTC 
  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(-)

Comments

Vikash Garodia May 5, 2023, 12:28 p.m. UTC | #1 
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)
>
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 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)