mbox series

[v4,00/17] Venus QoL / maintainability fixes

Message ID 20230228-topic-venus-v4-0-feebb2f6e9b8@linaro.org
Headers show
Series Venus QoL / maintainability fixes | expand

Message

Konrad Dybcio May 30, 2023, 12:30 p.m. UTC 
v3 -> v4:
- Rebase on Stanimir's venus-for-next-v6.5
- Collapse 2 identical if-statements in "Sanitize venus_boot_core()
  per-VPU-version"
- Reword "Assign registers based on VPU version"
- Check for IS_IRIS2_1() instead of wrongly checking for core->use_tz,
  update commit msg in "media: venus: firmware: Correct IS_V6() checks"
- Access correct struct fields in "Use newly-introduced
  hfi_buffer_requirements accessors", drop Bryan's r-b

v3: https://lore.kernel.org/r/20230228-topic-venus-v3-0-6092ae43b58f@linaro.org

v2 -> v3:
- Rephrase "Write to VIDC_CTRL_INIT after unmasking interrupts" commit msg
- Drop "Remap bufreq fields on HFI6XX"
- Rephrase "Introduce VPU version distinction" commit msg
- Better explain "Leave a clue for homegrown porters"
- Drop incorrect fixes tags/rephrase version check alternations
- Drop AR50L/IRIS1 from if-conditions, they'll be introduced separately
- pick up tags
- rebase on next-20230517 (no effective changes)

v2: https://lore.kernel.org/r/20230228-topic-venus-v2-0-d95d14949c79@linaro.org

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 (17):
      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: Introduce VPU version distinction
      media: venus: Add vpu_version to most SoCs
      media: venus: firmware: Leave a clue about obtaining CP VARs
      media: venus: hfi_venus: Sanitize venus_boot_core() per-VPU-version
      media: venus: core: Assign registers based on VPU version
      media: venus: hfi_venus: Sanitize venus_halt_axi() per-VPU-version
      media: venus: hfi_venus: Sanitize venus_isr() per-VPU-version
      media: venus: hfi_venus: Sanitize venus_cpu_and_video_core_idle() per-VPU-version
      media: venus: hfi_venus: Sanitize venus_cpu_idle_and_pc_ready() per-VPU-version
      media: venus: firmware: Sanitize per-VPU-version
      media: venus: hfi_platform: Check vpu_version instead of device compatible
      media: venus: vdec: Sanitize vdec_set_work_route() per-VPU-version
      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           |  7 ++-
 drivers/media/platform/qcom/venus/core.h           | 15 ++++++
 drivers/media/platform/qcom/venus/firmware.c       | 18 +++++--
 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      | 42 +++++++--------
 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, 133 insertions(+), 61 deletions(-)
---
base-commit: 9f9f8ca6f012d25428f8605cb36369a449db8508
change-id: 20230228-topic-venus-70ea3bc76688

Best regards,

Comments

Vikash Garodia June 1, 2023, 9:08 a.m. UTC | #1 
On 5/30/2023 6:00 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>

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

> ---
>  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 f0b46389e8d5..918a283bd890 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;
> @@ -927,17 +926,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)
>
Vikash Garodia June 1, 2023, 9:17 a.m. UTC | #2 
On 5/30/2023 6:00 PM, Konrad Dybcio wrote:
> 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>

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

> ---
>  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 82854553f99e..19fc6575a489 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) {
>
Dikshita Agarwal June 1, 2023, 9:37 a.m. UTC | #3 
On 5/30/2023 6:00 PM, Konrad Dybcio wrote:
> 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>

Reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

> ---
>  drivers/media/platform/qcom/venus/helpers.c    |  2 +-
>  drivers/media/platform/qcom/venus/hfi_helper.h | 61 ++++++++++++++++++++++----
>  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(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 1822e85ab6bf..b70bd3dac4df 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 0abbc50c5864..e4c05d62cfc7 100644
> --- a/drivers/media/platform/qcom/venus/hfi_helper.h
> +++ b/drivers/media/platform/qcom/venus/hfi_helper.h
> @@ -1170,14 +1170,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 ? 0 : (bufreq)->hold_count)
> -#define HFI_BUFREQ_COUNT_MIN(bufreq, ver)	\
> -	((ver) == HFI_VERSION_4XX ? (bufreq)->hold_count : (bufreq)->count_min)
> -#define HFI_BUFREQ_COUNT_MIN_HOST(bufreq, ver)	\
> -	((ver) == HFI_VERSION_4XX ? (bufreq)->count_min : 0)
> -
>  struct hfi_buffer_requirements {
>  	u32 type;
>  	u32 size;
> @@ -1189,6 +1181,59 @@ struct hfi_buffer_requirements {
>  	u32 alignment;
>  };
>  
> +/* On HFI 4XX, some of the struct members have been swapped. */
> +static inline u32 hfi_bufreq_get_hold_count(struct hfi_buffer_requirements *req,
> +					    u32 ver)
> +{
> +	if (ver == HFI_VERSION_4XX)
> +		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)
> +		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)
> +		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)
> +		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)
> +		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)
> +		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 3d5dadfa1900..7cab685a2ec8 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 063a8b0d357b..2a1e038f92cf 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -899,13 +899,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;
>  }
> @@ -1019,14 +1019,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 b60772cc2cdc..d2e2d3108752 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -1207,7 +1207,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);
> @@ -1215,7 +1215,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;
>