Re: [PATCH v7 5/9] media: venus: vdec: add video decoder files

From: Stanimir Varbanov
Date: Sat Mar 25 2017 - 18:48:17 EST


Thanks for the comments!

On 03/24/2017 04:41 PM, Hans Verkuil wrote:
Some comments and questions below:

On 03/13/17 17:37, Stanimir Varbanov wrote:
This consists of video decoder implementation plus decoder
controls.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
---
drivers/media/platform/qcom/venus/vdec.c | 1091 ++++++++++++++++++++++++
drivers/media/platform/qcom/venus/vdec.h | 23 +
drivers/media/platform/qcom/venus/vdec_ctrls.c | 149 ++++
3 files changed, 1263 insertions(+)
create mode 100644 drivers/media/platform/qcom/venus/vdec.c
create mode 100644 drivers/media/platform/qcom/venus/vdec.h
create mode 100644 drivers/media/platform/qcom/venus/vdec_ctrls.c

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
new file mode 100644
index 000000000000..ec5203f2ba81
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -0,0 +1,1091 @@
+/*
+ * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-mem2mem.h>
+#include <media/videobuf2-dma-sg.h>
+
+#include "hfi_venus_io.h"
+#include "core.h"
+#include "helpers.h"
+#include "vdec.h"
+
+static u32 get_framesize_uncompressed(unsigned int plane, u32 width, u32 height)
+{
+ u32 y_stride, uv_stride, y_plane;
+ u32 y_sclines, uv_sclines, uv_plane;
+ u32 size;
+
+ y_stride = ALIGN(width, 128);
+ uv_stride = ALIGN(width, 128);
+ y_sclines = ALIGN(height, 32);
+ uv_sclines = ALIGN(((height + 1) >> 1), 16);
+
+ y_plane = y_stride * y_sclines;
+ uv_plane = uv_stride * uv_sclines + SZ_4K;
+ size = y_plane + uv_plane + SZ_8K;
+
+ return ALIGN(size, SZ_4K);
+}
+
+static u32 get_framesize_compressed(unsigned int width, unsigned int height)
+{
+ return ((width * height * 3 / 2) / 2) + 128;
+}
+
+static const struct venus_format vdec_formats[] = {
+ {
+ .pixfmt = V4L2_PIX_FMT_NV12,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,

Just curious: is NV12 the only uncompressed format supported by the hardware?
Or just the only one that is implemented here?

+ }, {
+ .pixfmt = V4L2_PIX_FMT_MPEG4,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+ }, {
+ .pixfmt = V4L2_PIX_FMT_MPEG2,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+ }, {
+ .pixfmt = V4L2_PIX_FMT_H263,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+ }, {
+ .pixfmt = V4L2_PIX_FMT_VC1_ANNEX_G,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+ }, {
+ .pixfmt = V4L2_PIX_FMT_VC1_ANNEX_L,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+ }, {
+ .pixfmt = V4L2_PIX_FMT_H264,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+ }, {
+ .pixfmt = V4L2_PIX_FMT_VP8,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+ }, {
+ .pixfmt = V4L2_PIX_FMT_XVID,
+ .num_planes = 1,
+ .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+ },

num_planes is always 1, do you need it at all? And if it is always one,
why use _MPLANE at all? Is this for future additions?

+};
+

<snip> three reasons:
- _MPLAIN allows one plane only
- downstream qualcomm driver use _MPLAIN (the second plain is used for extaradata, I ignored the extaradata support for now until v4l2 metadata api is merged)
- I still believe that qualcomm firmware guys will add support the second or even third plain at some point.

+
+static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
+ u32 tag, u32 bytesused, u32 data_offset, u32 flags,
+ u64 timestamp_us)
+{
+ struct vb2_v4l2_buffer *vbuf;
+ struct vb2_buffer *vb;
+ unsigned int type;
+
+ if (buf_type == HFI_BUFFER_INPUT)
+ type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+ else
+ type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+
+ vbuf = helper_find_buf(inst, type, tag);
+ if (!vbuf)
+ return;
+
+ vbuf->flags = flags;
+
+ if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+ vb = &vbuf->vb2_buf;
+ vb->planes[0].bytesused =
+ max_t(unsigned int, inst->output_buf_size, bytesused);
+ vb->planes[0].data_offset = data_offset;
+ vb->timestamp = timestamp_us * NSEC_PER_USEC;
+ vbuf->sequence = inst->sequence++;

timestamp and sequence are only set for CAPTURE, not OUTPUT. Is that correct?

Correct. I can add sequence for the OUTPUT queue too, but I have no idea how that sequence is used by userspace.


+
+ if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
+ const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
+
+ v4l2_event_queue_fh(&inst->fh, &ev);
+ }
+ }
+
+ v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
+}

<snip>

--
regards,
Stan