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

[Hans Verkuil]

On 25/03/17 23:30, Stanimir Varbanov wrote:
> 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.

Please add a comment in the code explaining the reason. Just before this format list would be
a good place for that.

	Hans

> 
>>> +
>>> +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>
>