[V2] venus: vdec: decoded picture buffer handling during reconfig sequence

[V2] venus: vdec: decoded picture buffer handling during reconfig sequence

[Mansur Alisha Shaik]

In existing implementation, driver is freeing and un-mapping all the
decoded picture buffers(DPB) as part of dynamic resolution change(DRC)
handling. As a result, when firmware try to access the DPB buffer, from
previous sequence, SMMU context fault is seen due to the buffer being
already unmapped.

With this change, driver defines ownership of each DPB buffer. If a buffer
is owned by firmware, driver would skip from un-mapping the same.

Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>

Changes in V2:
- Fixed proto type warnings reported by kernel test robot
---
 drivers/media/platform/qcom/venus/core.h    |  3 ++
 drivers/media/platform/qcom/venus/helpers.c | 38 ++++++++++++++++-----
 drivers/media/platform/qcom/venus/helpers.h | 18 ++++++++++
 drivers/media/platform/qcom/venus/vdec.c    | 25 +++++++++++++-
 4 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 8df2d497d706..7ecbf9ed1acb 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -450,6 +450,7 @@ struct venus_inst {
 	bool next_buf_last;
 	bool drain_active;
 	enum venus_inst_modes flags;
+	u32 dpb_out_tag[VB2_MAX_FRAME];
 };
 
 #define IS_V1(core)	((core)->res->hfi_version == HFI_VERSION_1XX)
@@ -484,4 +485,6 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
 	return NULL;
 }
 
+void dpb_out_tag_init(struct venus_inst *inst);
+
 #endif
diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 1fe6d463dc99..4d308be712d7 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -21,14 +21,6 @@
 #define NUM_MBS_720P	(((1280 + 15) >> 4) * ((720 + 15) >> 4))
 #define NUM_MBS_4K	(((4096 + 15) >> 4) * ((2304 + 15) >> 4))
 
-struct intbuf {
-	struct list_head list;
-	u32 type;
-	size_t size;
-	void *va;
-	dma_addr_t da;
-	unsigned long attrs;
-};
 
 bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
 {
@@ -95,9 +87,16 @@ int venus_helper_queue_dpb_bufs(struct venus_inst *inst)
 		fdata.device_addr = buf->da;
 		fdata.buffer_type = buf->type;
 
+		if (buf->owned_by == FIRMWARE)
+			continue;
+
+		fdata.clnt_data = buf->dpb_out_tag;
+
 		ret = hfi_session_process_buf(inst, &fdata);
 		if (ret)
 			goto fail;
+
+		buf->owned_by = FIRMWARE;
 	}
 
 fail:
@@ -110,18 +109,37 @@ int venus_helper_free_dpb_bufs(struct venus_inst *inst)
 	struct intbuf *buf, *n;
 
 	list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) {
+		if (buf->owned_by == FIRMWARE)
+			continue;
+
+		inst->dpb_out_tag[buf->dpb_out_tag - VB2_MAX_FRAME] = 0;
+
 		list_del_init(&buf->list);
 		dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
 			       buf->attrs);
 		kfree(buf);
 	}
 
-	INIT_LIST_HEAD(&inst->dpbbufs);
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(venus_helper_free_dpb_bufs);
 
+int venus_helper_get_free_dpb_tag(struct venus_inst *inst)
+{
+	u32 i;
+
+	for (i = 0; i < VB2_MAX_FRAME; i++) {
+		if (inst->dpb_out_tag[i] == 0) {
+			inst->dpb_out_tag[i] = i + VB2_MAX_FRAME;
+			return inst->dpb_out_tag[i];
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(venus_helper_get_free_dpb_tag);
+
 int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
 {
 	struct venus_core *core = inst->core;
@@ -171,6 +189,8 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
 			ret = -ENOMEM;
 			goto fail;
 		}
+		buf->owned_by = DRIVER;
+		buf->dpb_out_tag = venus_helper_get_free_dpb_tag(inst);
 
 		list_add_tail(&buf->list, &inst->dpbbufs);
 	}
diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h
index e6269b4be3af..ea87a552c3ca 100644
--- a/drivers/media/platform/qcom/venus/helpers.h
+++ b/drivers/media/platform/qcom/venus/helpers.h
@@ -8,6 +8,22 @@
 
 #include <media/videobuf2-v4l2.h>
 
+enum dpb_buf_owner {
+	DRIVER,
+	FIRMWARE,
+};
+
+struct intbuf {
+	struct list_head list;
+	u32 type;
+	size_t size;
+	void *va;
+	dma_addr_t da;
+	unsigned long attrs;
+	enum dpb_buf_owner owned_by;
+	u32 dpb_out_tag;
+};
+
 struct venus_inst;
 struct venus_core;
 
@@ -66,4 +82,6 @@ int venus_helper_get_profile_level(struct venus_inst *inst, u32 *profile, u32 *l
 int venus_helper_set_profile_level(struct venus_inst *inst, u32 profile, u32 level);
 int venus_helper_set_stride(struct venus_inst *inst, unsigned int aligned_width,
 			    unsigned int aligned_height);
+int venus_helper_get_free_dpb_tag(struct venus_inst *inst);
+
 #endif
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 198e47eb63f4..ba46085301ee 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1297,6 +1297,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
 	struct vb2_v4l2_buffer *vbuf;
 	struct vb2_buffer *vb;
 	unsigned int type;
+	struct intbuf *dpb_buf;
 
 	vdec_pm_touch(inst);
 
@@ -1306,8 +1307,18 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
 		type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 
 	vbuf = venus_helper_find_buf(inst, type, tag);
-	if (!vbuf)
+	if (!vbuf) {
+		if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
+		    buf_type == HFI_BUFFER_OUTPUT) {
+			list_for_each_entry(dpb_buf, &inst->dpbbufs, list) {
+				if (dpb_buf->dpb_out_tag == tag) {
+					dpb_buf->owned_by = DRIVER;
+					break;
+				}
+			}
+		}
 		return;
+	}
 
 	vbuf->flags = flags;
 	vbuf->field = V4L2_FIELD_NONE;
@@ -1542,6 +1553,14 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq,
 	return vb2_queue_init(dst_vq);
 }
 
+void dpb_out_tag_init(struct venus_inst *inst)
+{
+	u32 i;
+
+	for (i = 0; i < VB2_MAX_FRAME; i++)
+		inst->dpb_out_tag[i] = 0;
+}
+
 static int vdec_open(struct file *file)
 {
 	struct venus_core *core = video_drvdata(file);
@@ -1580,6 +1599,8 @@ static int vdec_open(struct file *file)
 
 	vdec_inst_init(inst);
 
+	dpb_out_tag_init(inst);
+
 	/*
 	 * create m2m device for every instance, the m2m context scheduling
 	 * is made by firmware side so we do not need to care about.
@@ -1622,6 +1643,8 @@ static int vdec_close(struct file *file)
 
 	vdec_pm_get(inst);
 
+	venus_helper_free_dpb_bufs(inst);
+	INIT_LIST_HEAD(&inst->dpbbufs);
 	v4l2_m2m_ctx_release(inst->m2m_ctx);
 	v4l2_m2m_release(inst->m2m_dev);
 	vdec_ctrl_deinit(inst);

base-commit: c1a6d08348fc729e59776fe22878d4ce8fb97cde
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [V2] venus: vdec: decoded picture buffer handling during reconfig sequence

[Stanimir Varbanov]

On 7/15/21 3:35 PM, Mansur Alisha Shaik wrote:
> In existing implementation, driver is freeing and un-mapping all the
> decoded picture buffers(DPB) as part of dynamic resolution change(DRC)
> handling. As a result, when firmware try to access the DPB buffer, from
> previous sequence, SMMU context fault is seen due to the buffer being
> already unmapped.
> 
> With this change, driver defines ownership of each DPB buffer. If a buffer
> is owned by firmware, driver would skip from un-mapping the same.
> 
> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
> 
> Changes in V2:
> - Fixed proto type warnings reported by kernel test robot
> ---
>  drivers/media/platform/qcom/venus/core.h    |  3 ++
>  drivers/media/platform/qcom/venus/helpers.c | 38 ++++++++++++++++-----
>  drivers/media/platform/qcom/venus/helpers.h | 18 ++++++++++
>  drivers/media/platform/qcom/venus/vdec.c    | 25 +++++++++++++-
>  4 files changed, 74 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 8df2d497d706..7ecbf9ed1acb 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -450,6 +450,7 @@ struct venus_inst {
>  	bool next_buf_last;
>  	bool drain_active;
>  	enum venus_inst_modes flags;
> +	u32 dpb_out_tag[VB2_MAX_FRAME];
>  };
>  
>  #define IS_V1(core)	((core)->res->hfi_version == HFI_VERSION_1XX)
> @@ -484,4 +485,6 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
>  	return NULL;
>  }
>  
> +void dpb_out_tag_init(struct venus_inst *inst);
> +
>  #endif
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 1fe6d463dc99..4d308be712d7 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -21,14 +21,6 @@
>  #define NUM_MBS_720P	(((1280 + 15) >> 4) * ((720 + 15) >> 4))
>  #define NUM_MBS_4K	(((4096 + 15) >> 4) * ((2304 + 15) >> 4))
>  
> -struct intbuf {
> -	struct list_head list;
> -	u32 type;
> -	size_t size;
> -	void *va;
> -	dma_addr_t da;
> -	unsigned long attrs;
> -};
>  
>  bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
>  {
> @@ -95,9 +87,16 @@ int venus_helper_queue_dpb_bufs(struct venus_inst *inst)
>  		fdata.device_addr = buf->da;
>  		fdata.buffer_type = buf->type;
>  
> +		if (buf->owned_by == FIRMWARE)
> +			continue;
> +
> +		fdata.clnt_data = buf->dpb_out_tag;
> +
>  		ret = hfi_session_process_buf(inst, &fdata);
>  		if (ret)
>  			goto fail;
> +
> +		buf->owned_by = FIRMWARE;
>  	}
>  
>  fail:
> @@ -110,18 +109,37 @@ int venus_helper_free_dpb_bufs(struct venus_inst *inst)
>  	struct intbuf *buf, *n;
>  
>  	list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) {
> +		if (buf->owned_by == FIRMWARE)
> +			continue;
> +
> +		inst->dpb_out_tag[buf->dpb_out_tag - VB2_MAX_FRAME] = 0;

This looks wrong. The dpb_out_tag is in range of 0 .. 31, then for
dpb_out_tag = 0 you will have negative array index. Could you revisit that.

> +
>  		list_del_init(&buf->list);
>  		dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
>  			       buf->attrs);
>  		kfree(buf);
>  	}
>  
> -	INIT_LIST_HEAD(&inst->dpbbufs);

Instead of delete INIT_LIST_HEAD you can do:

if (list_empty(&inst->dpbbufs)
	INIT_LIST_HEAD(&inst->dpbbufs);

>  
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(venus_helper_free_dpb_bufs);
>  
> +int venus_helper_get_free_dpb_tag(struct venus_inst *inst)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < VB2_MAX_FRAME; i++) {
> +		if (inst->dpb_out_tag[i] == 0) {
> +			inst->dpb_out_tag[i] = i + VB2_MAX_FRAME;
> +			return inst->dpb_out_tag[i];
> +		}
> +	}

Can we use kernel API for that? I think ida_alloc|free|destroy would
work nicely and also will save a lot of code lines.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(venus_helper_get_free_dpb_tag);
> +
>  int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
>  {
>  	struct venus_core *core = inst->core;
> @@ -171,6 +189,8 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
>  			ret = -ENOMEM;
>  			goto fail;
>  		}
> +		buf->owned_by = DRIVER;
> +		buf->dpb_out_tag = venus_helper_get_free_dpb_tag(inst);
>  
>  		list_add_tail(&buf->list, &inst->dpbbufs);
>  	}
> diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h
> index e6269b4be3af..ea87a552c3ca 100644
> --- a/drivers/media/platform/qcom/venus/helpers.h
> +++ b/drivers/media/platform/qcom/venus/helpers.h
> @@ -8,6 +8,22 @@
>  
>  #include <media/videobuf2-v4l2.h>
>  
> +enum dpb_buf_owner {
> +	DRIVER,
> +	FIRMWARE,
> +};
> +
> +struct intbuf {
> +	struct list_head list;
> +	u32 type;
> +	size_t size;
> +	void *va;
> +	dma_addr_t da;
> +	unsigned long attrs;
> +	enum dpb_buf_owner owned_by;
> +	u32 dpb_out_tag;
> +};
> +

If you make dpb buffer manipulations as venus_helpers you don't need to
move the prototype of the structure outside of helpers.c.

>  struct venus_inst;
>  struct venus_core;
>  
> @@ -66,4 +82,6 @@ int venus_helper_get_profile_level(struct venus_inst *inst, u32 *profile, u32 *l
>  int venus_helper_set_profile_level(struct venus_inst *inst, u32 profile, u32 level);
>  int venus_helper_set_stride(struct venus_inst *inst, unsigned int aligned_width,
>  			    unsigned int aligned_height);
> +int venus_helper_get_free_dpb_tag(struct venus_inst *inst);
> +
>  #endif
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 198e47eb63f4..ba46085301ee 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -1297,6 +1297,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  	struct vb2_v4l2_buffer *vbuf;
>  	struct vb2_buffer *vb;
>  	unsigned int type;
> +	struct intbuf *dpb_buf;
>  
>  	vdec_pm_touch(inst);
>  
> @@ -1306,8 +1307,18 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  		type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>  
>  	vbuf = venus_helper_find_buf(inst, type, tag);
> -	if (!vbuf)
> +	if (!vbuf) {
> +		if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
> +		    buf_type == HFI_BUFFER_OUTPUT) {
> +			list_for_each_entry(dpb_buf, &inst->dpbbufs, list) {
> +				if (dpb_buf->dpb_out_tag == tag) {
> +					dpb_buf->owned_by = DRIVER;
> +					break;
> +				}
> +			}
> +		}

Please make this chuck of code a new venus_helper_find_dpb_buf()

>  		return;
> +	}
>  
>  	vbuf->flags = flags;
>  	vbuf->field = V4L2_FIELD_NONE;
> @@ -1542,6 +1553,14 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq,
>  	return vb2_queue_init(dst_vq);
>  }
>  
> +void dpb_out_tag_init(struct venus_inst *inst)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < VB2_MAX_FRAME; i++)
> +		inst->dpb_out_tag[i] = 0;

This initialization cycle can be moved in vdec_inst_init(), but if you
migrate to ida_xxx APIs this will be part of venus_helper_alloc_dpb_bufs()

> +}
> +
>  static int vdec_open(struct file *file)
>  {
>  	struct venus_core *core = video_drvdata(file);
> @@ -1580,6 +1599,8 @@ static int vdec_open(struct file *file)
>  
>  	vdec_inst_init(inst);
>  
> +	dpb_out_tag_init(inst);
> +
>  	/*
>  	 * create m2m device for every instance, the m2m context scheduling
>  	 * is made by firmware side so we do not need to care about.
> @@ -1622,6 +1643,8 @@ static int vdec_close(struct file *file)
>  
>  	vdec_pm_get(inst);
>  
> +	venus_helper_free_dpb_bufs(inst);
> +	INIT_LIST_HEAD(&inst->dpbbufs);
>  	v4l2_m2m_ctx_release(inst->m2m_ctx);
>  	v4l2_m2m_release(inst->m2m_dev);
>  	vdec_ctrl_deinit(inst);
> 
> base-commit: c1a6d08348fc729e59776fe22878d4ce8fb97cde
> 

-- 
regards,
Stan