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

[Stanimir Varbanov <stanimir.varbanov@linaro.org>]

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@linaro.org>
>> ---
>>  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