[PATCH v1 0/5] Venus - Decode reconfig sequence

[PATCH v1 0/5] Venus - Decode reconfig sequence

[Srinu Gorle]

Hello,

The patch set mainly adds logic to handle multiresolution clips
for video decode. And also added few miscellaneous fixes.

The patch set is based on top of recent Venus updates - PIL v9 patches
posted by Vikash Garodia.
This patch series to align with https://patchwork.linuxtv.org/patch/52153/ 

Comments are welcome!

Regards,
Srinu Gorle

Srinu Gorle (5):
  media: venus: handle video decoder resolution change
  media: venus: dynamically configure codec type
  media: venus: do not destroy video session during queue setup
  media: venus: video decoder drop frames handling
  media: venus: update number of bytes used field properly for EOS
    frames

 drivers/media/platform/qcom/venus/helpers.c | 210 ++++++++++++++++++++--------
 drivers/media/platform/qcom/venus/helpers.h |   4 +
 drivers/media/platform/qcom/venus/hfi.c     |   8 +-
 drivers/media/platform/qcom/venus/hfi.h     |   2 +-
 drivers/media/platform/qcom/venus/vdec.c    | 114 +++++++++++++--
 drivers/media/platform/qcom/venus/venc.c    |  20 ++-
 6 files changed, 285 insertions(+), 73 deletions(-)

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

[PATCH v1 1/5] media: venus: handle video decoder resolution change

[Srinu Gorle]

Add logic for below to handle resolution change during video decode.
- stream off support for video decoder OUTPUT plane and
  flush old resolution OUTPUT plane buffers.
- De-allocate and allocate video firmware internal buffers.
  And also ensures g_fmt for output plane populated
  only after SPS and PPS has parsed.

Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
---
 drivers/media/platform/qcom/venus/helpers.c | 159 +++++++++++++++++++++++-----
 drivers/media/platform/qcom/venus/helpers.h |   3 +
 drivers/media/platform/qcom/venus/hfi.c     |   5 +-
 drivers/media/platform/qcom/venus/hfi.h     |   2 +-
 drivers/media/platform/qcom/venus/vdec.c    | 102 +++++++++++++++---
 drivers/media/platform/qcom/venus/venc.c    |  20 +++-
 6 files changed, 246 insertions(+), 45 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index e436385..822a853 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -180,6 +180,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
 		list_add_tail(&buf->list, &inst->dpbbufs);
 	}
 
+	venus_helper_queue_dpb_bufs(inst);
 	return 0;
 
 fail:
@@ -319,6 +320,65 @@ static int intbufs_free(struct venus_inst *inst)
 	return intbufs_unset_buffers(inst);
 }
 
+static int alloc_reconfig_buffers(struct venus_inst *inst)
+{
+	size_t arr_sz;
+	size_t i;
+	int ret;
+	unsigned int buf_type;
+
+	if (IS_V4(inst->core))
+		arr_sz = ARRAY_SIZE(intbuf_types_4xx);
+	else
+		arr_sz = ARRAY_SIZE(intbuf_types_1xx);
+
+	for (i = 0; i < arr_sz; i++) {
+		buf_type = IS_V4(inst->core) ? intbuf_types_4xx[i] :
+					intbuf_types_1xx[i];
+		if (buf_type == HFI_BUFFER_INTERNAL_PERSIST ||
+		    buf_type == HFI_BUFFER_INTERNAL_PERSIST_1)
+			continue;
+
+		ret = intbufs_set_buffer(inst, buf_type);
+		if (ret)
+			goto error;
+	}
+
+	return 0;
+
+error:
+	intbufs_unset_buffers(inst);
+	return ret;
+}
+
+static int unset_reconfig_buffers(struct venus_inst *inst)
+{
+	struct hfi_buffer_desc bd = {0};
+	struct intbuf *buf, *n;
+	int ret = 0;
+
+	list_for_each_entry_safe(buf, n, &inst->internalbufs, list) {
+		if (buf->type == HFI_BUFFER_INTERNAL_PERSIST ||
+		    buf->type == HFI_BUFFER_INTERNAL_PERSIST_1)
+			continue;
+
+		bd.buffer_size = buf->size;
+		bd.buffer_type = buf->type;
+		bd.num_buffers = 1;
+		bd.device_addr = buf->da;
+		bd.response_required = true;
+
+		ret = hfi_session_unset_buffers(inst, &bd);
+
+		list_del_init(&buf->list);
+		dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
+			       buf->attrs);
+		kfree(buf);
+	}
+
+	return ret;
+}
+
 static u32 load_per_instance(struct venus_inst *inst)
 {
 	u32 mbs;
@@ -969,14 +1029,26 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
 	struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
+	struct venus_core *core = inst->core;
+	struct device *dev = core->dev;
 	int ret;
+	bool is_plane_enabled;
 
 	mutex_lock(&inst->lock);
 
 	v4l2_m2m_buf_queue(m2m_ctx, vbuf);
 
-	if (!(inst->streamon_out & inst->streamon_cap))
+	is_plane_enabled = inst->streamon_out &&
+		vb->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+	is_plane_enabled |= inst->streamon_cap &&
+		vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+
+	if (!is_plane_enabled) {
+		dev_info(dev, "%s: Yet to start_stream the Q",
+			 vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ?
+			 "FTB" : "ETB");
 		goto unlock;
+	}
 
 	ret = is_buf_refed(inst, vbuf);
 	if (ret)
@@ -1009,37 +1081,72 @@ void venus_helper_vb2_stop_streaming(struct vb2_queue *q)
 	struct venus_core *core = inst->core;
 	int ret;
 
-	mutex_lock(&inst->lock);
-
-	if (inst->streamon_out & inst->streamon_cap) {
-		ret = hfi_session_stop(inst);
-		ret |= hfi_session_unload_res(inst);
+	hfi_session_stop(inst);
+	ret = hfi_session_unload_res(inst);
+	if (inst->hfi_codec == HFI_VIDEO_CODEC_H264)
 		ret |= session_unregister_bufs(inst);
-		ret |= intbufs_free(inst);
-		ret |= hfi_session_deinit(inst);
-
-		if (inst->session_error || core->sys_error)
-			ret = -EIO;
+	ret |= intbufs_free(inst);
+	ret |= hfi_session_deinit(inst);
 
-		if (ret)
-			hfi_session_abort(inst);
+	if (inst->session_error || core->sys_error)
+		ret = -EIO;
 
-		venus_helper_free_dpb_bufs(inst);
+	if (IS_V3(core) && ret)
+		hfi_session_abort(inst);
 
-		load_scale_clocks(core);
-		INIT_LIST_HEAD(&inst->registeredbufs);
-	}
+	venus_helper_free_dpb_bufs(inst);
+	load_scale_clocks(core);
+	INIT_LIST_HEAD(&inst->registeredbufs);
 
 	venus_helper_buffers_done(inst, VB2_BUF_STATE_ERROR);
+}
+EXPORT_SYMBOL_GPL(venus_helper_vb2_stop_streaming);
 
-	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
-		inst->streamon_out = 0;
-	else
-		inst->streamon_cap = 0;
+int venus_helper_alloc_intbufs(struct venus_inst *inst)
+{
+	int ret = 0;
 
-	mutex_unlock(&inst->lock);
+	ret = intbufs_free(inst);
+	ret |= intbufs_alloc(inst);
+
+	return ret;
 }
-EXPORT_SYMBOL_GPL(venus_helper_vb2_stop_streaming);
+EXPORT_SYMBOL_GPL(venus_helper_alloc_intbufs);
+
+int venus_helper_alloc_reconfig_bufs(struct venus_inst *inst)
+{
+	int ret = 0;
+
+	ret = unset_reconfig_buffers(inst);
+	ret |= alloc_reconfig_buffers(inst);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(venus_helper_alloc_reconfig_bufs);
+
+int venus_helper_queue_initial_bufs(struct venus_inst *inst, unsigned int type)
+{
+	struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
+	struct v4l2_m2m_buffer *buf, *n;
+	int ret;
+
+	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)	{
+		v4l2_m2m_for_each_dst_buf_safe(m2m_ctx, buf, n)	{
+			ret = session_process_buf(inst, &buf->vb);
+			if (ret)
+				return_buf_error(inst, &buf->vb);
+		}
+	}
+	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
+		v4l2_m2m_for_each_src_buf_safe(m2m_ctx, buf, n) {
+			ret = session_process_buf(inst, &buf->vb);
+			if (ret)
+				return_buf_error(inst, &buf->vb);
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL(venus_helper_queue_initial_bufs);
 
 int venus_helper_vb2_start_streaming(struct venus_inst *inst)
 {
@@ -1064,14 +1171,8 @@ int venus_helper_vb2_start_streaming(struct venus_inst *inst)
 	if (ret)
 		goto err_unload_res;
 
-	ret = venus_helper_queue_dpb_bufs(inst);
-	if (ret)
-		goto err_session_stop;
-
 	return 0;
 
-err_session_stop:
-	hfi_session_stop(inst);
 err_unload_res:
 	hfi_session_unload_res(inst);
 err_unreg_bufs:
diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h
index 2475f284..3de0c44 100644
--- a/drivers/media/platform/qcom/venus/helpers.h
+++ b/drivers/media/platform/qcom/venus/helpers.h
@@ -31,6 +31,8 @@ void venus_helper_buffers_done(struct venus_inst *inst,
 int venus_helper_vb2_start_streaming(struct venus_inst *inst);
 void venus_helper_m2m_device_run(void *priv);
 void venus_helper_m2m_job_abort(void *priv);
+int venus_helper_queue_initial_bufs(struct venus_inst *inst, unsigned int type);
+int venus_helper_alloc_intbufs(struct venus_inst *inst);
 int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
 			    struct hfi_buffer_requirements *req);
 u32 venus_helper_get_framesz_raw(u32 hfi_fmt, u32 width, u32 height);
@@ -62,4 +64,5 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 fmt, u32 *out_fmt,
 int venus_helper_free_dpb_bufs(struct venus_inst *inst);
 int venus_helper_power_enable(struct venus_core *core, u32 session_type,
 			      bool enable);
+int venus_helper_alloc_reconfig_bufs(struct venus_inst *inst);
 #endif
diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
index 2420782..36a4784 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -308,6 +308,7 @@ int hfi_session_stop(struct venus_inst *inst)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(hfi_session_stop);
 
 int hfi_session_continue(struct venus_inst *inst)
 {
@@ -384,14 +385,14 @@ int hfi_session_unload_res(struct venus_inst *inst)
 	return 0;
 }
 
-int hfi_session_flush(struct venus_inst *inst)
+int hfi_session_flush(struct venus_inst *inst, u32 mode)
 {
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
 	reinit_completion(&inst->done);
 
-	ret = ops->session_flush(inst, HFI_FLUSH_ALL);
+	ret = ops->session_flush(inst, mode);
 	if (ret)
 		return ret;
 
diff --git a/drivers/media/platform/qcom/venus/hfi.h b/drivers/media/platform/qcom/venus/hfi.h
index 6038d8e..5e883a1 100644
--- a/drivers/media/platform/qcom/venus/hfi.h
+++ b/drivers/media/platform/qcom/venus/hfi.h
@@ -170,7 +170,7 @@ struct hfi_ops {
 int hfi_session_abort(struct venus_inst *inst);
 int hfi_session_load_res(struct venus_inst *inst);
 int hfi_session_unload_res(struct venus_inst *inst);
-int hfi_session_flush(struct venus_inst *inst);
+int hfi_session_flush(struct venus_inst *inst, u32 mode);
 int hfi_session_set_buffers(struct venus_inst *inst,
 			    struct hfi_buffer_desc *bd);
 int hfi_session_unset_buffers(struct venus_inst *inst,
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 991e158..98675db 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -207,7 +207,6 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
 
 		inst->out_width = inst->reconfig_width;
 		inst->out_height = inst->reconfig_height;
-		inst->reconfig = false;
 
 		format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 		format.fmt.pix_mp.pixelformat = inst->fmt_cap->pixfmt;
@@ -223,6 +222,9 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
 	pixmp->pixelformat = fmt->pixfmt;
 
 	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+		if (!(inst->reconfig))
+			return -EINVAL;
+
 		pixmp->width = inst->width;
 		pixmp->height = inst->height;
 		pixmp->colorspace = inst->colorspace;
@@ -451,6 +453,8 @@ static int vdec_subscribe_event(struct v4l2_fh *fh,
 		if (cmd->flags & V4L2_DEC_CMD_STOP_TO_BLACK)
 			return -EINVAL;
 		break;
+	case V4L2_DEC_CMD_START:
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -465,6 +469,9 @@ static int vdec_subscribe_event(struct v4l2_fh *fh,
 	struct hfi_frame_data fdata = {0};
 	int ret;
 
+	if (cmd->cmd != V4L2_DEC_CMD_STOP)
+		return 0;
+
 	ret = vdec_try_decoder_cmd(file, fh, cmd);
 	if (ret)
 		return ret;
@@ -790,22 +797,60 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct venus_inst *inst = vb2_get_drv_priv(q);
 	int ret;
+	bool is_mplane_enabled;
 
 	mutex_lock(&inst->lock);
+	is_mplane_enabled = q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
+				inst->streamon_cap;
+	is_mplane_enabled |= q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
+				inst->streamon_out;
 
-	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
-		inst->streamon_out = 1;
-	else
-		inst->streamon_cap = 1;
+	if (is_mplane_enabled) {
+		mutex_unlock(&inst->lock);
+		return 0;
+	}
 
-	if (!(inst->streamon_out & inst->streamon_cap)) {
+	if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
+	    !inst->streamon_out){
 		mutex_unlock(&inst->lock);
 		return 0;
 	}
 
+	if (inst->streamon_out && !inst->streamon_cap &&
+	    q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+		ret = vdec_output_conf(inst);
+		if (ret)
+			goto deinit_sess;
+
+		ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
+						inst->num_output_bufs,
+						inst->num_output_bufs);
+
+		if (ret)
+			goto deinit_sess;
+
+		ret = vdec_verify_conf(inst);
+		if (ret)
+			goto deinit_sess;
+
+		if (inst->reconfig)
+			ret = venus_helper_alloc_reconfig_bufs(inst);
+
+		if (ret)
+			goto deinit_sess;
+
+		ret = venus_helper_alloc_dpb_bufs(inst);
+		if (ret)
+			goto deinit_sess;
+
+		if (inst->reconfig) {
+			hfi_session_continue(inst);
+			inst->reconfig = false;
+		}
+		goto enable_mplane;
+	}
 	venus_helper_init_instance(inst);
 
-	inst->reconfig = false;
 	inst->sequence_cap = 0;
 	inst->sequence_out = 0;
 
@@ -830,14 +875,17 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 	if (ret)
 		goto deinit_sess;
 
-	ret = venus_helper_alloc_dpb_bufs(inst);
-	if (ret)
-		goto deinit_sess;
-
 	ret = venus_helper_vb2_start_streaming(inst);
 	if (ret)
 		goto deinit_sess;
 
+enable_mplane:
+	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		inst->streamon_out = 1;
+	else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+		inst->streamon_cap = 1;
+
+	ret = venus_helper_queue_initial_bufs(inst, q->type);
 	mutex_unlock(&inst->lock);
 
 	return 0;
@@ -854,12 +902,42 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
 	return ret;
 }
 
+static void vdec_stop_streaming(struct vb2_queue *q)
+{
+	struct venus_inst *inst = vb2_get_drv_priv(q);
+	int ret;
+
+	mutex_lock(&inst->lock);
+
+	if (!inst->streamon_cap && !inst->streamon_out)
+		goto unlock;
+
+	if (inst->streamon_cap &&
+	    q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+		ret = hfi_session_stop(inst);
+		inst->streamon_cap = 0;
+	}
+
+	if (inst->streamon_out && !inst->streamon_cap) {
+		inst->streamon_out = 0;
+		venus_helper_vb2_stop_streaming(q);
+	}
+
+	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		inst->streamon_out = 0;
+	else
+		inst->streamon_cap = 0;
+unlock:
+	venus_helper_buffers_done(inst, VB2_BUF_STATE_ERROR);
+	mutex_unlock(&inst->lock);
+}
+
 static const struct vb2_ops vdec_vb2_ops = {
 	.queue_setup = vdec_queue_setup,
 	.buf_init = venus_helper_vb2_buf_init,
 	.buf_prepare = venus_helper_vb2_buf_prepare,
 	.start_streaming = vdec_start_streaming,
-	.stop_streaming = venus_helper_vb2_stop_streaming,
+	.stop_streaming = vdec_stop_streaming,
 	.buf_queue = venus_helper_vb2_buf_queue,
 };
 
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index ce85962..3ce0f7a 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1005,12 +1005,30 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
 	return ret;
 }
 
+void venc_stop_streaming(struct vb2_queue *q)
+{
+	struct venus_inst *inst = vb2_get_drv_priv(q);
+
+	mutex_lock(&inst->lock);
+
+	if (inst->streamon_out & inst->streamon_cap)
+		venus_helper_vb2_stop_streaming(q);
+
+	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		inst->streamon_out = 0;
+	else
+		inst->streamon_cap = 0;
+
+	mutex_unlock(&inst->lock);
+}
+EXPORT_SYMBOL_GPL(venc_stop_streaming);
+
 static const struct vb2_ops venc_vb2_ops = {
 	.queue_setup = venc_queue_setup,
 	.buf_init = venus_helper_vb2_buf_init,
 	.buf_prepare = venus_helper_vb2_buf_prepare,
 	.start_streaming = venc_start_streaming,
-	.stop_streaming = venus_helper_vb2_stop_streaming,
+	.stop_streaming = venc_stop_streaming,
 	.buf_queue = venus_helper_vb2_buf_queue,
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 2/5] media: venus: dynamically configure codec type

[Srinu Gorle]

- currently video decoder instance is hardcoded to H.264 video format.
- this change enables video decoder dynamically configure to
  any supported video format.

Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
---
 drivers/media/platform/qcom/venus/helpers.c | 51 ++++++++++++++---------------
 drivers/media/platform/qcom/venus/helpers.h |  1 +
 drivers/media/platform/qcom/venus/vdec.c    |  2 ++
 3 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 822a853..c82dbac 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -36,47 +36,44 @@ struct intbuf {
 	unsigned long attrs;
 };
 
-bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
+u32 v4l2_venus_fmt(u32 pixfmt)
 {
-	struct venus_core *core = inst->core;
-	u32 session_type = inst->session_type;
-	u32 codec;
-
-	switch (v4l2_pixfmt) {
+	switch (pixfmt) {
 	case V4L2_PIX_FMT_H264:
-		codec = HFI_VIDEO_CODEC_H264;
-		break;
+	case V4L2_PIX_FMT_H264_NO_SC:
+		return HFI_VIDEO_CODEC_H264;
 	case V4L2_PIX_FMT_H263:
-		codec = HFI_VIDEO_CODEC_H263;
-		break;
+		return HFI_VIDEO_CODEC_H263;
 	case V4L2_PIX_FMT_MPEG1:
-		codec = HFI_VIDEO_CODEC_MPEG1;
-		break;
+		return HFI_VIDEO_CODEC_MPEG1;
 	case V4L2_PIX_FMT_MPEG2:
-		codec = HFI_VIDEO_CODEC_MPEG2;
-		break;
+		return HFI_VIDEO_CODEC_MPEG2;
 	case V4L2_PIX_FMT_MPEG4:
-		codec = HFI_VIDEO_CODEC_MPEG4;
-		break;
+		return HFI_VIDEO_CODEC_MPEG4;
 	case V4L2_PIX_FMT_VC1_ANNEX_G:
 	case V4L2_PIX_FMT_VC1_ANNEX_L:
-		codec = HFI_VIDEO_CODEC_VC1;
-		break;
+		return HFI_VIDEO_CODEC_VC1;
 	case V4L2_PIX_FMT_VP8:
-		codec = HFI_VIDEO_CODEC_VP8;
-		break;
+		return HFI_VIDEO_CODEC_VP8;
 	case V4L2_PIX_FMT_VP9:
-		codec = HFI_VIDEO_CODEC_VP9;
-		break;
+		return HFI_VIDEO_CODEC_VP9;
 	case V4L2_PIX_FMT_XVID:
-		codec = HFI_VIDEO_CODEC_DIVX;
-		break;
+		return HFI_VIDEO_CODEC_DIVX;
 	case V4L2_PIX_FMT_HEVC:
-		codec = HFI_VIDEO_CODEC_HEVC;
-		break;
+		return HFI_VIDEO_CODEC_HEVC;
 	default:
-		return false;
+		return 0;
 	}
+}
+EXPORT_SYMBOL_GPL(v4l2_venus_fmt);
+
+bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
+{
+	struct venus_core *core = inst->core;
+	u32 session_type = inst->session_type;
+	u32 codec;
+
+	codec = v4l2_venus_fmt(v4l2_pixfmt);
 
 	if (session_type == VIDC_SESSION_TYPE_ENC && core->enc_codecs & codec)
 		return true;
diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h
index 3de0c44..725831d 100644
--- a/drivers/media/platform/qcom/venus/helpers.h
+++ b/drivers/media/platform/qcom/venus/helpers.h
@@ -19,6 +19,7 @@
 
 struct venus_inst;
 
+u32 v4l2_venus_fmt(u32 pixfmt);
 bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt);
 struct vb2_v4l2_buffer *venus_helper_find_buf(struct venus_inst *inst,
 					      unsigned int type, u32 idx);
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 98675db..afe3b36 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -413,6 +413,8 @@ static int vdec_enum_framesizes(struct file *file, void *fh,
 				  V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
 		if (!fmt)
 			return -EINVAL;
+		inst->fmt_out = fmt;
+		inst->hfi_codec = v4l2_venus_fmt(fmt->pixfmt);
 	}
 
 	if (fsize->index)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 3/5] media: venus: do not destroy video session during queue setup

[Srinu Gorle]

- open and close video sessions for plane properties is incorrect.
- add check to ensure, same instance persist from driver open to close.

Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
---
 drivers/media/platform/qcom/venus/hfi.c  | 3 +++
 drivers/media/platform/qcom/venus/vdec.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
index 36a4784..59c34ba 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -207,6 +207,9 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
 	const struct hfi_ops *ops = core->ops;
 	int ret;
 
+	if (inst->state >= INST_INIT && inst->state < INST_STOP)
+		return 0;
+
 	inst->hfi_codec = to_codec_type(pixfmt);
 	reinit_completion(&inst->done);
 
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index afe3b36..0035cf2 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -700,6 +700,8 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num,
 
 	*out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
 
+	return 0;
+
 deinit:
 	hfi_session_deinit(inst);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 4/5] media: venus: video decoder drop frames handling

[Srinu Gorle]

- when drop frame flag received from venus h/w, reset buffer
  parameters and update v4l2 buffer flags as error buffer.

Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 0035cf2..311f209 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -991,6 +991,12 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
 	if (hfi_flags & HFI_BUFFERFLAG_DATACORRUPT)
 		state = VB2_BUF_STATE_ERROR;
 
+	if (hfi_flags & HFI_BUFFERFLAG_DROP_FRAME) {
+		vb->planes[0].bytesused = 0;
+		vb->timestamp = 0;
+		state = VB2_BUF_STATE_ERROR;
+	}
+
 	v4l2_m2m_buf_done(vbuf, state);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames

[Srinu Gorle]

- In video decoder session, update number of bytes used for
  yuv buffers appropriately for EOS buffers.

Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 311f209..a48eed1 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -978,7 +978,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
 
 		if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
 			const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
-
+			vb->planes[0].bytesused = bytesused;
 			v4l2_event_queue_fh(&inst->fh, &ev);
 		}
 	} else {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames

[Stanimir Varbanov]

Hi,

On 9/29/18 3:00 PM, Srinu Gorle wrote:
> - In video decoder session, update number of bytes used for
>   yuv buffers appropriately for EOS buffers.
> 
> Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

NACK, that was already discussed see:

https://patchwork.kernel.org/patch/10630411/

> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 311f209..a48eed1 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -978,7 +978,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  
>  		if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
>  			const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
> -
> +			vb->planes[0].bytesused = bytesused;
>  			v4l2_event_queue_fh(&inst->fh, &ev);
>  		}
>  	} else {
> 

-- 
regards,
Stan

Re: [PATCH v1 3/5] media: venus: do not destroy video session during queue setup

[Stanimir Varbanov]

Hi Srinu,

On 9/29/18 3:00 PM, Srinu Gorle wrote:
> - open and close video sessions for plane properties is incorrect.

Could you rephrase this statement? I really don't understand what you mean.

> - add check to ensure, same instance persist from driver open to close.

This assumption is wrong. The v4l client can change the codec by SFMT
without close the device node, in that case we have to destroy and
create a new session with new codec.

> 
> Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/hfi.c  | 3 +++
>  drivers/media/platform/qcom/venus/vdec.c | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
> index 36a4784..59c34ba 100644
> --- a/drivers/media/platform/qcom/venus/hfi.c
> +++ b/drivers/media/platform/qcom/venus/hfi.c
> @@ -207,6 +207,9 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
>  	const struct hfi_ops *ops = core->ops;
>  	int ret;
>  
> +	if (inst->state >= INST_INIT && inst->state < INST_STOP)
> +		return 0;

In fact you want to be able to call session_init multiple times but
deinit the session only once? The hfi.c layer is designed to follow the
states as they are expected by the firmware side, if you want to call
session_init multiple times just make a wrapper in the vdec.c with
reference counting.

> +
>  	inst->hfi_codec = to_codec_type(pixfmt);
>  	reinit_completion(&inst->done);
>  
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index afe3b36..0035cf2 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -700,6 +700,8 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num,
>  
>  	*out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
>  
> +	return 0;

OK in present implementation I decided that the codec is settled when
streamon on both queues is called (i.e. the final session_init is made
in streamon). IMO the correct one is to init the session in
reqbuf(output) and deinit session in reqbuf(output, count=0)?

The problem I see when you skip session_deinit is that the codec cannot
be changed without closing the video node.

> +
>  deinit:
>  	hfi_session_deinit(inst);
>  
> 

-- 
regards,
Stan

Re: [PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames

[Alexandre Courbot]

Hi Stan,

On Thu, Nov 8, 2018 at 7:16 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi,
>
> On 9/29/18 3:00 PM, Srinu Gorle wrote:
> > - In video decoder session, update number of bytes used for
> >   yuv buffers appropriately for EOS buffers.
> >
> > Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
> > ---
> >  drivers/media/platform/qcom/venus/vdec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> NACK, that was already discussed see:
>
> https://patchwork.kernel.org/patch/10630411/

I believe you are referring to this discussion?

https://lkml.org/lkml/2018/10/2/302

In this case, with https://patchwork.kernel.org/patch/10630411/
applied, I am seeing the troublesome case of having the last (empty)
buffer being returned with a payload of obs_sz, which I believe is
incorrect. The present patch seems to restore the correct behavior.

An alternative would be to set the payload as follows:

vb2_set_plane_payload(vb, 0, bytesused);

This works for SDM845, but IIRC we weren't sure that this would
display the correct behavior with all firmware versions?

>
> >
> > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> > index 311f209..a48eed1 100644
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -978,7 +978,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
> >
> >               if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
> >                       const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
> > -
> > +                     vb->planes[0].bytesused = bytesused;
> >                       v4l2_event_queue_fh(&inst->fh, &ev);
> >               }
> >       } else {
> >
>
> --
> regards,
> Stan

Re: [PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames

[Stanimir Varbanov]

Hi Alex,

On 11/12/18 10:12 AM, Alexandre Courbot wrote:
> Hi Stan,
> 
> On Thu, Nov 8, 2018 at 7:16 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Hi,
>>
>> On 9/29/18 3:00 PM, Srinu Gorle wrote:
>>> - In video decoder session, update number of bytes used for
>>>   yuv buffers appropriately for EOS buffers.
>>>
>>> Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
>>> ---
>>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> NACK, that was already discussed see:
>>
>> https://patchwork.kernel.org/patch/10630411/
> 
> I believe you are referring to this discussion?
> 
> https://lkml.org/lkml/2018/10/2/302
> 
> In this case, with https://patchwork.kernel.org/patch/10630411/
> applied, I am seeing the troublesome case of having the last (empty)
> buffer being returned with a payload of obs_sz, which I believe is
> incorrect. The present patch seems to restore the correct behavior.

Sorry, I thought that this solution was suggested (and tested on Venus
v4) by you, right?

> 
> An alternative would be to set the payload as follows:
> 
> vb2_set_plane_payload(vb, 0, bytesused);
> 
> This works for SDM845, but IIRC we weren't sure that this would
> display the correct behavior with all firmware versions?

OK if you are still seeing issues I think we can switch to
vb2_set_plane_payload(vb, 0, bytesused); for all buffers? I.e. not only
for buffers with flag V4L2_BUF_FLAG_LAST set.

> 
>>
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>>> index 311f209..a48eed1 100644
>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>> @@ -978,7 +978,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>>>
>>>               if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
>>>                       const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
>>> -
>>> +                     vb->planes[0].bytesused = bytesused;

Is 'bytesused' != 0 in case of EoS ever?

i.e. shouldn't this be vb->planes[0].bytesused = 0 ?

>>>                       v4l2_event_queue_fh(&inst->fh, &ev);
>>>               }
>>>       } else {
>>>
>>
>> --
>> regards,
>> Stan

-- 
regards,
Stan

Re: [PATCH v1 5/5] media: venus: update number of bytes used field properly for EOS frames

[Alexandre Courbot]

On Mon, Nov 12, 2018 at 9:20 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Alex,
>
> On 11/12/18 10:12 AM, Alexandre Courbot wrote:
> > Hi Stan,
> >
> > On Thu, Nov 8, 2018 at 7:16 PM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Hi,
> >>
> >> On 9/29/18 3:00 PM, Srinu Gorle wrote:
> >>> - In video decoder session, update number of bytes used for
> >>>   yuv buffers appropriately for EOS buffers.
> >>>
> >>> Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
> >>> ---
> >>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> NACK, that was already discussed see:
> >>
> >> https://patchwork.kernel.org/patch/10630411/
> >
> > I believe you are referring to this discussion?
> >
> > https://lkml.org/lkml/2018/10/2/302
> >
> > In this case, with https://patchwork.kernel.org/patch/10630411/
> > applied, I am seeing the troublesome case of having the last (empty)
> > buffer being returned with a payload of obs_sz, which I believe is
> > incorrect. The present patch seems to restore the correct behavior.
>
> Sorry, I thought that this solution was suggested (and tested on Venus
> v4) by you, right?

That's correct. >_< Looks like I overlooked this case.

>
> >
> > An alternative would be to set the payload as follows:
> >
> > vb2_set_plane_payload(vb, 0, bytesused);
> >
> > This works for SDM845, but IIRC we weren't sure that this would
> > display the correct behavior with all firmware versions?
>
> OK if you are still seeing issues I think we can switch to
> vb2_set_plane_payload(vb, 0, bytesused); for all buffers? I.e. not only
> for buffers with flag V4L2_BUF_FLAG_LAST set.

That's the fix I am currently using in my source tree and it indeed
seems to be ok. I also agree it is better than special-casing EOS
buffers. I have sent a patch for this.

Thanks and sorry for the confusion.

Re: [PATCH v1 1/5] media: venus: handle video decoder resolution change

[Tomasz Figa]

Hi Srinu,

On Sat, Sep 29, 2018 at 9:01 PM Srinu Gorle <sgorle@codeaurora.org> wrote:
>
> Add logic for below to handle resolution change during video decode.
> - stream off support for video decoder OUTPUT plane and
>   flush old resolution OUTPUT plane buffers.

I think you mean CAPTURE (the decoded buffers)?

> - De-allocate and allocate video firmware internal buffers.
>   And also ensures g_fmt for output plane populated
>   only after SPS and PPS has parsed.

A resolution change would be triggered by new resolution parsed from a
SPS/PPS, so the format should be ready at that time right? Generally
the spec requires G_FMT to return the new resolution after the source
change event is signaled.

[snip]
> @@ -969,14 +1029,26 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
>         struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>         struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
>         struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
> +       struct venus_core *core = inst->core;
> +       struct device *dev = core->dev;
>         int ret;
> +       bool is_plane_enabled;
>
>         mutex_lock(&inst->lock);
>
>         v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>
> -       if (!(inst->streamon_out & inst->streamon_cap))
> +       is_plane_enabled = inst->streamon_out &&
> +               vb->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +       is_plane_enabled |= inst->streamon_cap &&
> +               vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;

Why does the condition change?

Putting aside this change, why does this driver do anything here
actually? I think it should just call v4l2_m2m_qbuf() from vidioc_qbuf
and then the M2M framework would fire driver's .device_run() callback,
taking into account driver specific conditions. It would also take
care of any necessary streamon checks, so they don't need to be
explicitly repeated in the driver. Unrelated to the patch, but perhaps
should be considered while at it?

> +
> +       if (!is_plane_enabled) {
> +               dev_info(dev, "%s: Yet to start_stream the Q",
> +                        vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ?
> +                        "FTB" : "ETB");
>                 goto unlock;
> +       }
>
>         ret = is_buf_refed(inst, vbuf);
>         if (ret)
> @@ -1009,37 +1081,72 @@ void venus_helper_vb2_stop_streaming(struct vb2_queue *q)
>         struct venus_core *core = inst->core;
>         int ret;
>
> -       mutex_lock(&inst->lock);
> -
> -       if (inst->streamon_out & inst->streamon_cap) {
> -               ret = hfi_session_stop(inst);
> -               ret |= hfi_session_unload_res(inst);
> +       hfi_session_stop(inst);
> +       ret = hfi_session_unload_res(inst);

Wouldn't this end up stopping and unloading two times, once for
CAPTURE and once for OUTPUT?

Also, stopping streaming on CAPTURE shouldn't have any side effects,
other than dropping any in flight CAPTURE buffers, so this change
sounds wrong.

> +       if (inst->hfi_codec == HFI_VIDEO_CODEC_H264)
>                 ret |= session_unregister_bufs(inst);
> -               ret |= intbufs_free(inst);
> -               ret |= hfi_session_deinit(inst);
> -
> -               if (inst->session_error || core->sys_error)
> -                       ret = -EIO;
> +       ret |= intbufs_free(inst);
> +       ret |= hfi_session_deinit(inst);
>
> -               if (ret)
> -                       hfi_session_abort(inst);
> +       if (inst->session_error || core->sys_error)
> +               ret = -EIO;
>
> -               venus_helper_free_dpb_bufs(inst);
> +       if (IS_V3(core) && ret)
> +               hfi_session_abort(inst);
>
> -               load_scale_clocks(core);
> -               INIT_LIST_HEAD(&inst->registeredbufs);
> -       }
> +       venus_helper_free_dpb_bufs(inst);
> +       load_scale_clocks(core);
> +       INIT_LIST_HEAD(&inst->registeredbufs);
>
>         venus_helper_buffers_done(inst, VB2_BUF_STATE_ERROR);
> +}
> +EXPORT_SYMBOL_GPL(venus_helper_vb2_stop_streaming);
>
> -       if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> -               inst->streamon_out = 0;
> -       else
> -               inst->streamon_cap = 0;
> +int venus_helper_alloc_intbufs(struct venus_inst *inst)
> +{
> +       int ret = 0;
>
> -       mutex_unlock(&inst->lock);
> +       ret = intbufs_free(inst);
> +       ret |= intbufs_alloc(inst);

Please don't OR return values. If these two functions return two
different error codes, you get useless garbage in the variable...

> +
> +       return ret;
>  }
> -EXPORT_SYMBOL_GPL(venus_helper_vb2_stop_streaming);
> +EXPORT_SYMBOL_GPL(venus_helper_alloc_intbufs);
> +
> +int venus_helper_alloc_reconfig_bufs(struct venus_inst *inst)
> +{
> +       int ret = 0;
> +
> +       ret = unset_reconfig_buffers(inst);
> +       ret |= alloc_reconfig_buffers(inst);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(venus_helper_alloc_reconfig_bufs);
> +
> +int venus_helper_queue_initial_bufs(struct venus_inst *inst, unsigned int type)
> +{
> +       struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
> +       struct v4l2_m2m_buffer *buf, *n;
> +       int ret;
> +
> +       if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> +               v4l2_m2m_for_each_dst_buf_safe(m2m_ctx, buf, n) {
> +                       ret = session_process_buf(inst, &buf->vb);
> +                       if (ret)
> +                               return_buf_error(inst, &buf->vb);
> +               }
> +       }
> +       if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +               v4l2_m2m_for_each_src_buf_safe(m2m_ctx, buf, n) {
> +                       ret = session_process_buf(inst, &buf->vb);
> +                       if (ret)
> +                               return_buf_error(inst, &buf->vb);
> +               }
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL(venus_helper_queue_initial_bufs);
>
>  int venus_helper_vb2_start_streaming(struct venus_inst *inst)
>  {
> @@ -1064,14 +1171,8 @@ int venus_helper_vb2_start_streaming(struct venus_inst *inst)
>         if (ret)
>                 goto err_unload_res;
>
> -       ret = venus_helper_queue_dpb_bufs(inst);
> -       if (ret)
> -               goto err_session_stop;
> -
>         return 0;
>
> -err_session_stop:
> -       hfi_session_stop(inst);
>  err_unload_res:
>         hfi_session_unload_res(inst);
>  err_unreg_bufs:
> diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h
> index 2475f284..3de0c44 100644
> --- a/drivers/media/platform/qcom/venus/helpers.h
> +++ b/drivers/media/platform/qcom/venus/helpers.h
> @@ -31,6 +31,8 @@ void venus_helper_buffers_done(struct venus_inst *inst,
>  int venus_helper_vb2_start_streaming(struct venus_inst *inst);
>  void venus_helper_m2m_device_run(void *priv);
>  void venus_helper_m2m_job_abort(void *priv);
> +int venus_helper_queue_initial_bufs(struct venus_inst *inst, unsigned int type);
> +int venus_helper_alloc_intbufs(struct venus_inst *inst);
>  int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
>                             struct hfi_buffer_requirements *req);
>  u32 venus_helper_get_framesz_raw(u32 hfi_fmt, u32 width, u32 height);
> @@ -62,4 +64,5 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 fmt, u32 *out_fmt,
>  int venus_helper_free_dpb_bufs(struct venus_inst *inst);
>  int venus_helper_power_enable(struct venus_core *core, u32 session_type,
>                               bool enable);
> +int venus_helper_alloc_reconfig_bufs(struct venus_inst *inst);
>  #endif
> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
> index 2420782..36a4784 100644
> --- a/drivers/media/platform/qcom/venus/hfi.c
> +++ b/drivers/media/platform/qcom/venus/hfi.c
> @@ -308,6 +308,7 @@ int hfi_session_stop(struct venus_inst *inst)
>
>         return 0;
>  }
> +EXPORT_SYMBOL_GPL(hfi_session_stop);
>
>  int hfi_session_continue(struct venus_inst *inst)
>  {
> @@ -384,14 +385,14 @@ int hfi_session_unload_res(struct venus_inst *inst)
>         return 0;
>  }
>
> -int hfi_session_flush(struct venus_inst *inst)
> +int hfi_session_flush(struct venus_inst *inst, u32 mode)
>  {
>         const struct hfi_ops *ops = inst->core->ops;
>         int ret;
>
>         reinit_completion(&inst->done);
>
> -       ret = ops->session_flush(inst, HFI_FLUSH_ALL);
> +       ret = ops->session_flush(inst, mode);
>         if (ret)
>                 return ret;
>
> diff --git a/drivers/media/platform/qcom/venus/hfi.h b/drivers/media/platform/qcom/venus/hfi.h
> index 6038d8e..5e883a1 100644
> --- a/drivers/media/platform/qcom/venus/hfi.h
> +++ b/drivers/media/platform/qcom/venus/hfi.h
> @@ -170,7 +170,7 @@ struct hfi_ops {
>  int hfi_session_abort(struct venus_inst *inst);
>  int hfi_session_load_res(struct venus_inst *inst);
>  int hfi_session_unload_res(struct venus_inst *inst);
> -int hfi_session_flush(struct venus_inst *inst);
> +int hfi_session_flush(struct venus_inst *inst, u32 mode);
>  int hfi_session_set_buffers(struct venus_inst *inst,
>                             struct hfi_buffer_desc *bd);
>  int hfi_session_unset_buffers(struct venus_inst *inst,
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 991e158..98675db 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -207,7 +207,6 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
>
>                 inst->out_width = inst->reconfig_width;
>                 inst->out_height = inst->reconfig_height;
> -               inst->reconfig = false;
>
>                 format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>                 format.fmt.pix_mp.pixelformat = inst->fmt_cap->pixfmt;
> @@ -223,6 +222,9 @@ static int vdec_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
>         pixmp->pixelformat = fmt->pixfmt;
>
>         if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> +               if (!(inst->reconfig))
> +                       return -EINVAL;
> +
>                 pixmp->width = inst->width;
>                 pixmp->height = inst->height;
>                 pixmp->colorspace = inst->colorspace;
> @@ -451,6 +453,8 @@ static int vdec_subscribe_event(struct v4l2_fh *fh,
>                 if (cmd->flags & V4L2_DEC_CMD_STOP_TO_BLACK)
>                         return -EINVAL;
>                 break;
> +       case V4L2_DEC_CMD_START:
> +               return 0;
>         default:
>                 return -EINVAL;
>         }
> @@ -465,6 +469,9 @@ static int vdec_subscribe_event(struct v4l2_fh *fh,
>         struct hfi_frame_data fdata = {0};
>         int ret;
>
> +       if (cmd->cmd != V4L2_DEC_CMD_STOP)
> +               return 0;
> +

Huh? This callback should return an error in case of an unsupported command.

Also, the START command can't be just ignored. The STOP command must
pause the decoding until it's reset by restarting the streaming or
resumed by the START command.

>         ret = vdec_try_decoder_cmd(file, fh, cmd);
>         if (ret)
>                 return ret;
> @@ -790,22 +797,60 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>         struct venus_inst *inst = vb2_get_drv_priv(q);
>         int ret;
> +       bool is_mplane_enabled;
>
>         mutex_lock(&inst->lock);
> +       is_mplane_enabled = q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
> +                               inst->streamon_cap;
> +       is_mplane_enabled |= q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
> +                               inst->streamon_out;
>
> -       if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> -               inst->streamon_out = 1;
> -       else
> -               inst->streamon_cap = 1;
> +       if (is_mplane_enabled) {
> +               mutex_unlock(&inst->lock);
> +               return 0;
> +       }
>
> -       if (!(inst->streamon_out & inst->streamon_cap)) {
> +       if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
> +           !inst->streamon_out){
>                 mutex_unlock(&inst->lock);
>                 return 0;
>         }
>
> +       if (inst->streamon_out && !inst->streamon_cap &&
> +           q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> +               ret = vdec_output_conf(inst);
> +               if (ret)
> +                       goto deinit_sess;
> +
> +               ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
> +                                               inst->num_output_bufs,
> +                                               inst->num_output_bufs);
> +
> +               if (ret)
> +                       goto deinit_sess;
> +
> +               ret = vdec_verify_conf(inst);
> +               if (ret)
> +                       goto deinit_sess;
> +
> +               if (inst->reconfig)
> +                       ret = venus_helper_alloc_reconfig_bufs(inst);
> +
> +               if (ret)
> +                       goto deinit_sess;
> +
> +               ret = venus_helper_alloc_dpb_bufs(inst);
> +               if (ret)
> +                       goto deinit_sess;
> +
> +               if (inst->reconfig) {
> +                       hfi_session_continue(inst);
> +                       inst->reconfig = false;
> +               }
> +               goto enable_mplane;
> +       }
>         venus_helper_init_instance(inst);
>
> -       inst->reconfig = false;
>         inst->sequence_cap = 0;
>         inst->sequence_out = 0;
>
> @@ -830,14 +875,17 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>         if (ret)
>                 goto deinit_sess;
>
> -       ret = venus_helper_alloc_dpb_bufs(inst);
> -       if (ret)
> -               goto deinit_sess;
> -
>         ret = venus_helper_vb2_start_streaming(inst);
>         if (ret)
>                 goto deinit_sess;
>
> +enable_mplane:
> +       if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +               inst->streamon_out = 1;
> +       else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +               inst->streamon_cap = 1;
> +
> +       ret = venus_helper_queue_initial_bufs(inst, q->type);
>         mutex_unlock(&inst->lock);
>
>         return 0;
> @@ -854,12 +902,42 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>         return ret;
>  }
>
> +static void vdec_stop_streaming(struct vb2_queue *q)
> +{
> +       struct venus_inst *inst = vb2_get_drv_priv(q);
> +       int ret;
> +
> +       mutex_lock(&inst->lock);
> +
> +       if (!inst->streamon_cap && !inst->streamon_out)
> +               goto unlock;
> +
> +       if (inst->streamon_cap &&
> +           q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> +               ret = hfi_session_stop(inst);
> +               inst->streamon_cap = 0;
> +       }
> +
> +       if (inst->streamon_out && !inst->streamon_cap) {

This looks wrong. If the application stops OUT first and then CAP
later, this code won't run.

> +               inst->streamon_out = 0;
> +               venus_helper_vb2_stop_streaming(q);
> +       }

Stopping streaming on CAPTURE shouldn't have any significant side
effects - just any queued or not-dequeued-yet buffers would be
dropped. Stopping OUTPUT begins a seek. To completely reset the
decoder, one needs to stop OUTPUT and REQBUFS(0) on it.

Best regards,
Tomasz

Re: [PATCH v1 2/5] media: venus: dynamically configure codec type

[Tomasz Figa]

On Sat, Sep 29, 2018 at 9:01 PM Srinu Gorle <sgorle@codeaurora.org> wrote:
>
> - currently video decoder instance is hardcoded to H.264 video format.
> - this change enables video decoder dynamically configure to
>   any supported video format.
>
> Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 51 ++++++++++++++---------------
>  drivers/media/platform/qcom/venus/helpers.h |  1 +
>  drivers/media/platform/qcom/venus/vdec.c    |  2 ++
>  3 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 822a853..c82dbac 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -36,47 +36,44 @@ struct intbuf {
>         unsigned long attrs;
>  };
>
> -bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
> +u32 v4l2_venus_fmt(u32 pixfmt)
>  {
> -       struct venus_core *core = inst->core;
> -       u32 session_type = inst->session_type;
> -       u32 codec;
> -
> -       switch (v4l2_pixfmt) {
> +       switch (pixfmt) {
>         case V4L2_PIX_FMT_H264:
> -               codec = HFI_VIDEO_CODEC_H264;
> -               break;
> +       case V4L2_PIX_FMT_H264_NO_SC:
> +               return HFI_VIDEO_CODEC_H264;
>         case V4L2_PIX_FMT_H263:
> -               codec = HFI_VIDEO_CODEC_H263;
> -               break;
> +               return HFI_VIDEO_CODEC_H263;
>         case V4L2_PIX_FMT_MPEG1:
> -               codec = HFI_VIDEO_CODEC_MPEG1;
> -               break;
> +               return HFI_VIDEO_CODEC_MPEG1;
>         case V4L2_PIX_FMT_MPEG2:
> -               codec = HFI_VIDEO_CODEC_MPEG2;
> -               break;
> +               return HFI_VIDEO_CODEC_MPEG2;
>         case V4L2_PIX_FMT_MPEG4:
> -               codec = HFI_VIDEO_CODEC_MPEG4;
> -               break;
> +               return HFI_VIDEO_CODEC_MPEG4;
>         case V4L2_PIX_FMT_VC1_ANNEX_G:
>         case V4L2_PIX_FMT_VC1_ANNEX_L:
> -               codec = HFI_VIDEO_CODEC_VC1;
> -               break;
> +               return HFI_VIDEO_CODEC_VC1;
>         case V4L2_PIX_FMT_VP8:
> -               codec = HFI_VIDEO_CODEC_VP8;
> -               break;
> +               return HFI_VIDEO_CODEC_VP8;
>         case V4L2_PIX_FMT_VP9:
> -               codec = HFI_VIDEO_CODEC_VP9;
> -               break;
> +               return HFI_VIDEO_CODEC_VP9;
>         case V4L2_PIX_FMT_XVID:
> -               codec = HFI_VIDEO_CODEC_DIVX;
> -               break;
> +               return HFI_VIDEO_CODEC_DIVX;
>         case V4L2_PIX_FMT_HEVC:
> -               codec = HFI_VIDEO_CODEC_HEVC;
> -               break;
> +               return HFI_VIDEO_CODEC_HEVC;
>         default:
> -               return false;
> +               return 0;
>         }
> +}
> +EXPORT_SYMBOL_GPL(v4l2_venus_fmt);
> +
> +bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
> +{
> +       struct venus_core *core = inst->core;
> +       u32 session_type = inst->session_type;
> +       u32 codec;
> +
> +       codec = v4l2_venus_fmt(v4l2_pixfmt);
>
>         if (session_type == VIDC_SESSION_TYPE_ENC && core->enc_codecs & codec)
>                 return true;
> diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h
> index 3de0c44..725831d 100644
> --- a/drivers/media/platform/qcom/venus/helpers.h
> +++ b/drivers/media/platform/qcom/venus/helpers.h
> @@ -19,6 +19,7 @@
>
>  struct venus_inst;
>
> +u32 v4l2_venus_fmt(u32 pixfmt);
>  bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt);
>  struct vb2_v4l2_buffer *venus_helper_find_buf(struct venus_inst *inst,
>                                               unsigned int type, u32 idx);
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 98675db..afe3b36 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -413,6 +413,8 @@ static int vdec_enum_framesizes(struct file *file, void *fh,
>                                   V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
>                 if (!fmt)
>                         return -EINVAL;
> +               inst->fmt_out = fmt;
> +               inst->hfi_codec = v4l2_venus_fmt(fmt->pixfmt);

That's wrong. ENUM_FRAMESIZES (and any other ENUM_* or G_* ioctl) must
not affect driver state or result in any other side effects.

Best regards,
Tomasz

Re: [PATCH v1 3/5] media: venus: do not destroy video session during queue setup

[Tomasz Figa]

On Fri, Nov 9, 2018 at 7:00 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Srinu,
>
> On 9/29/18 3:00 PM, Srinu Gorle wrote:
> > - open and close video sessions for plane properties is incorrect.
>
> Could you rephrase this statement? I really don't understand what you mean.
>
> > - add check to ensure, same instance persist from driver open to close.
>
> This assumption is wrong. The v4l client can change the codec by SFMT
> without close the device node, in that case we have to destroy and
> create a new session with new codec.

Right.

[snip]
> > +
> >       inst->hfi_codec = to_codec_type(pixfmt);
> >       reinit_completion(&inst->done);
> >
> > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> > index afe3b36..0035cf2 100644
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -700,6 +700,8 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num,
> >
> >       *out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
> >
> > +     return 0;
>
> OK in present implementation I decided that the codec is settled when
> streamon on both queues is called (i.e. the final session_init is made
> in streamon). IMO the correct one is to init the session in
> reqbuf(output) and deinit session in reqbuf(output, count=0)?
>

Depending on what you mean by "settled" that could be fine.

Generally for initialization, one would typically use REQBUFS(OUTPUT,
>0) (since it's not possible to change the format after the
allocation) or STREAMON(OUTPUT) (to defer the hardware operations
until really necessary).

Since STREAMOFF(OUTPUT) is expected to just trigger a seek,
termination should be done in REQBUFS(OUTPUT, 0) indeed and after that
one should be able to switch to another codec (OUTPUT format).

Best regards,
Tomasz

Re: [PATCH v1 4/5] media: venus: video decoder drop frames handling

[Tomasz Figa]

On Sat, Sep 29, 2018 at 9:01 PM Srinu Gorle <sgorle@codeaurora.org> wrote:
>
> - when drop frame flag received from venus h/w, reset buffer
>   parameters and update v4l2 buffer flags as error buffer.
>
> Signed-off-by: Srinu Gorle <sgorle@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 0035cf2..311f209 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -991,6 +991,12 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>         if (hfi_flags & HFI_BUFFERFLAG_DATACORRUPT)
>                 state = VB2_BUF_STATE_ERROR;
>
> +       if (hfi_flags & HFI_BUFFERFLAG_DROP_FRAME) {
> +               vb->planes[0].bytesused = 0;
> +               vb->timestamp = 0;
> +               state = VB2_BUF_STATE_ERROR;
> +       }

What does this HFI_BUFFERFLAG_DROP_FRAME flag mean? When would it
happen? I assume it applies only to CAPTURE buffers, since for OUTPUT
buffers you must not set the bytesuses/timestamp yourself, right? Is
the buffer guaranteed to have no decoded frame inside or the frame
could be there, but incomplete/corrupted?

Best regards,
Tomasz