[PATCH 0/2] Venus fixes

[PATCH 0/2] Venus fixes

[Vikash Garodia]

The series consist of following fixes
1. Internal buffers handling during reset sequence.
2. Venus power on sequence during stream off.

Please review and share your comments.

Vikash Garodia (2):
  media: venus: do not queue internal buffers from previous sequence
  media: venus: vdec: ensure venus is powered on during stream off

 drivers/media/platform/qcom/venus/helpers.c | 37 ++++++++++++++++++++++-------
 drivers/media/platform/qcom/venus/vdec.c    |  2 ++
 2 files changed, 30 insertions(+), 9 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/2] media: venus: do not queue internal buffers from previous sequence

[Vikash Garodia]

During reconfig (DRC) event from firmware, it is not guaranteed that
all the DPB(internal) buffers would be released by the firmware. Some
buffers might be released gradually while processing frames from the
new sequence. These buffers now stay idle in the dpblist.
In subsequent call to queue the DPBs to firmware, these idle buffers
should not be queued. The fix identifies those buffers and free them.
---
 drivers/media/platform/qcom/venus/helpers.c | 37 ++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 0bca95d..0602f03 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -90,12 +90,31 @@ bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
 }
 EXPORT_SYMBOL_GPL(venus_helper_check_codec);
 
+static int venus_helper_free_dpb_buf(struct venus_inst *inst, struct intbuf *buf)
+{
+	ida_free(&inst->dpb_ids, buf->dpb_out_tag);
+
+	list_del_init(&buf->list);
+	dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
+		       buf->attrs);
+	kfree(buf);
+
+	return 0;
+}
+
 int venus_helper_queue_dpb_bufs(struct venus_inst *inst)
 {
-	struct intbuf *buf;
+	struct intbuf *buf, *next;
+	unsigned int dpb_size = 0;
 	int ret = 0;
 
-	list_for_each_entry(buf, &inst->dpbbufs, list) {
+	if (inst->dpb_buftype == HFI_BUFFER_OUTPUT)
+		dpb_size = inst->output_buf_size;
+	else if (inst->dpb_buftype == HFI_BUFFER_OUTPUT2)
+		dpb_size = inst->output2_buf_size;
+
+
+	list_for_each_entry_safe(buf, next, &inst->dpbbufs, list) {
 		struct hfi_frame_data fdata;
 
 		memset(&fdata, 0, sizeof(fdata));
@@ -106,6 +125,12 @@ int venus_helper_queue_dpb_bufs(struct venus_inst *inst)
 		if (buf->owned_by == FIRMWARE)
 			continue;
 
+		/* free buffer from previous sequence which was released later */
+		if (dpb_size > buf->size) {
+			venus_helper_free_dpb_buf(inst, buf);
+			continue;
+		}
+
 		fdata.clnt_data = buf->dpb_out_tag;
 
 		ret = hfi_session_process_buf(inst, &fdata);
@@ -127,13 +152,7 @@ int venus_helper_free_dpb_bufs(struct venus_inst *inst)
 	list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) {
 		if (buf->owned_by == FIRMWARE)
 			continue;
-
-		ida_free(&inst->dpb_ids, buf->dpb_out_tag);
-
-		list_del_init(&buf->list);
-		dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
-			       buf->attrs);
-		kfree(buf);
+		venus_helper_free_dpb_buf(inst, buf);
 	}
 
 	if (list_empty(&inst->dpbbufs))
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] media: venus: vdec: ensure venus is powered on during stream off

[Vikash Garodia]

Video decoder driver auto-suspends the hardware if there is no
exchange of command or response for certain amount of time.
In auto suspended state, it becomes mandatory to power on the
hardware before requesting it to process a command. The fix
ensures the hardware is powered on during stop streaming.
---
 drivers/media/platform/qcom/venus/vdec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 91da3f5..4ac1132 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1200,6 +1200,8 @@ static void vdec_stop_streaming(struct vb2_queue *q)
 	struct venus_inst *inst = vb2_get_drv_priv(q);
 	int ret = -EINVAL;
 
+	vdec_pm_get_put(inst);
+
 	mutex_lock(&inst->lock);
 
 	if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] media: venus: vdec: ensure venus is powered on during stream off

[Stanimir Varbanov]

Hi Vikash,

On 4/12/22 10:09, Vikash Garodia wrote:
> Video decoder driver auto-suspends the hardware if there is no
> exchange of command or response for certain amount of time.
> In auto suspended state, it becomes mandatory to power on the
> hardware before requesting it to process a command. The fix
> ensures the hardware is powered on during stop streaming.
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 91da3f5..4ac1132 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -1200,6 +1200,8 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  	struct venus_inst *inst = vb2_get_drv_priv(q);
>  	int ret = -EINVAL;
>  
> +	vdec_pm_get_put(inst);
> +
>  	mutex_lock(&inst->lock);
>  
>  	if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)

-- 
regards,
Stan

Re: [PATCH 1/2] media: venus: do not queue internal buffers from previous sequence

[Stanimir Varbanov]

Hi Vikash,

Thanks for the patch!

On 4/12/22 10:09, Vikash Garodia wrote:
> During reconfig (DRC) event from firmware, it is not guaranteed that
> all the DPB(internal) buffers would be released by the firmware. Some
> buffers might be released gradually while processing frames from the
> new sequence. These buffers now stay idle in the dpblist.
> In subsequent call to queue the DPBs to firmware, these idle buffers
> should not be queued. The fix identifies those buffers and free them.
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 37 ++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 0bca95d..0602f03 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -90,12 +90,31 @@ bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
>  }
>  EXPORT_SYMBOL_GPL(venus_helper_check_codec);
>  
> +static int venus_helper_free_dpb_buf(struct venus_inst *inst, struct intbuf *buf)

This function should return void. Also, venus_helper_ prefix is not
needed since if you follow the helpers.c style the prefix is only used
for exported helper functions.

I guess it could be:

static void free_dpb_buf(struct venus_inst *inst, struct intbuf *buf)

With this fixed:

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> +{
> +	ida_free(&inst->dpb_ids, buf->dpb_out_tag);
> +
> +	list_del_init(&buf->list);
> +	dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
> +		       buf->attrs);
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
>  int venus_helper_queue_dpb_bufs(struct venus_inst *inst)
>  {
> -	struct intbuf *buf;
> +	struct intbuf *buf, *next;
> +	unsigned int dpb_size = 0;
>  	int ret = 0;
>  
> -	list_for_each_entry(buf, &inst->dpbbufs, list) {
> +	if (inst->dpb_buftype == HFI_BUFFER_OUTPUT)
> +		dpb_size = inst->output_buf_size;
> +	else if (inst->dpb_buftype == HFI_BUFFER_OUTPUT2)
> +		dpb_size = inst->output2_buf_size;
> +
> +
> +	list_for_each_entry_safe(buf, next, &inst->dpbbufs, list) {
>  		struct hfi_frame_data fdata;
>  
>  		memset(&fdata, 0, sizeof(fdata));
> @@ -106,6 +125,12 @@ int venus_helper_queue_dpb_bufs(struct venus_inst *inst)
>  		if (buf->owned_by == FIRMWARE)
>  			continue;
>  
> +		/* free buffer from previous sequence which was released later */
> +		if (dpb_size > buf->size) {
> +			venus_helper_free_dpb_buf(inst, buf);
> +			continue;
> +		}
> +
>  		fdata.clnt_data = buf->dpb_out_tag;
>  
>  		ret = hfi_session_process_buf(inst, &fdata);
> @@ -127,13 +152,7 @@ int venus_helper_free_dpb_bufs(struct venus_inst *inst)
>  	list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) {
>  		if (buf->owned_by == FIRMWARE)
>  			continue;
> -
> -		ida_free(&inst->dpb_ids, buf->dpb_out_tag);
> -
> -		list_del_init(&buf->list);
> -		dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
> -			       buf->attrs);
> -		kfree(buf);
> +		venus_helper_free_dpb_buf(inst, buf);
>  	}
>  
>  	if (list_empty(&inst->dpbbufs))

-- 
regards,
Stan