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

2017-03-27 Thread Nicolas Dufresne 
Le lundi 27 mars 2017 à 10:45 +0200, Hans Verkuil a écrit :
> > > 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.
> 
> You set V4L2_BUF_FLAG_TIMESTAMP_COPY, so you have to copy the
> timestamp from the output buffer
> to the capture buffer, if that makes sense for this codec. If not,
> then you shouldn't use that
> V4L2_BUF_FLAG and just generate new timestamps whenever a capture
> buffer is ready.
> 
> For sequence numbering just give the output queue its own sequence
> counter.

Btw, GStreamer and Chromium only supports TIMESTAMP_COPY, and will most
likely leak frames if you craft timestamp.

Nicolas

signature.asc
Description: This is a digitally signed message part

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

2017-03-27 Thread Stanimir Varbanov 
Hi Hans,

On 03/27/2017 11:45 AM, Hans Verkuil wrote:
> On 25/03/17 23:30, Stanimir Varbanov wrote:
>> Thanks for the comments!



 +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 = >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.
> 
> You set V4L2_BUF_FLAG_TIMESTAMP_COPY, so you have to copy the timestamp from 
> the output buffer
> to the capture buffer, if that makes sense for this codec. If not, then you 
> shouldn't use that

The timestamp_us is filled by firmware and it is the timestamp of the
output buffer which is used to produce the uncompressed capture buffer.
So I think V4L2_BUF_FLAG_TIMESTAMP_COPY is correctly used here.

> V4L2_BUF_FLAG and just generate new timestamps whenever a capture buffer is 
> ready.
> 
> For sequence numbering just give the output queue its own sequence counter.

OK will do.

-- 
regards,
Stan

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

2017-03-27 Thread Hans Verkuil 
On 27/03/17 04:18, Nicolas Dufresne wrote:
> Le dimanche 26 mars 2017 à 00:30 +0200, Stanimir Varbanov a écrit :
 +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.
> 
> Neither GStreamer or Chromium seems to use it. What does that number
> means for a m2m driver ? Does it really means something ?

It can be used to detect dropped frame (the sequence counter will skip in that
case).

Unlikely to happen for m2m devices, and most apps ignore it as well. But you
still need to fill it in, it's a V4L2 requirement.

Regards,

Hans

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

2017-03-27 Thread 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 
>>> ---
>>>  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 ..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 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#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?
>>
>>> +};
>>> +
> 
>  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

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

2017-03-27 Thread 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 
>>> ---
>>>  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 ..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 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#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?
>>
>>> +};
>>> +
> 
>  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;

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

2017-03-26 Thread Nicolas Dufresne 
Le dimanche 26 mars 2017 à 00:30 +0200, Stanimir Varbanov a écrit :
> > > +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.

Neither GStreamer or Chromium seems to use it. What does that number
means for a m2m driver ? Does it really means something ?

Nicolas

signature.asc
Description: This is a digitally signed message part

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

2017-03-25 Thread Stanimir Varbanov 

Hi,

On 24.03.2017 20:21, Nicolas Dufresne wrote:

Le vendredi 24 mars 2017 à 15:41 +0100, Hans Verkuil a écrit :

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


yes, at least to my knowledge (except below UBWC).



The downstream kernel[0], from Qualcomm have:

{
.name = "UBWC YCbCr Semiplanar 4:2:0",
.description = "UBWC Y/CbCr 4:2:0",
.fourcc = V4L2_PIX_FMT_NV12_UBWC,
.num_planes = 2,
.get_frame_size = get_frame_size_nv12_ubwc,
.type = CAPTURE_PORT,
},
{
.name = "UBWC YCbCr Semiplanar 4:2:0 10bit",
.description = "UBWC Y/CbCr 4:2:0 10bit",
.fourcc = V4L2_PIX_FMT_NV12_TP10_UBWC,
.num_planes = 2,
.get_frame_size = get_frame_size_nv12_ubwc_10bit,
.type = CAPTURE_PORT,
},

I have no idea what UBWC stands for. The performance in NV12 is more
then decent from my testing. Though, there is no 10bit variant.


UBWC is some kind of compressed format for NV12 [1]. This format is 
applicable for the newer venus hardware revisions and I planed to add it 
later on (when Adreno GPU driver starts handle it).


regards,
Stan

[1] 
https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/include/media/msm_media_info.h#151

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

2017-03-25 Thread Stanimir Varbanov 

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 
---
 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 ..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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?


+};
+


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

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

2017-03-24 Thread Nicolas Dufresne 
Le vendredi 24 mars 2017 à 15:41 +0100, Hans Verkuil a écrit :
> > +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?

The downstream kernel[0], from Qualcomm have:

{
.name = "UBWC YCbCr Semiplanar 4:2:0",
.description = "UBWC Y/CbCr 4:2:0",
.fourcc = V4L2_PIX_FMT_NV12_UBWC,
.num_planes = 2,
.get_frame_size = get_frame_size_nv12_ubwc,
.type = CAPTURE_PORT,
},
{
.name = "UBWC YCbCr Semiplanar 4:2:0 10bit",
.description = "UBWC Y/CbCr 4:2:0 10bit",
.fourcc = V4L2_PIX_FMT_NV12_TP10_UBWC,
.num_planes = 2,
.get_frame_size = get_frame_size_nv12_ubwc_10bit,
.type = CAPTURE_PORT,
},

I have no idea what UBWC stands for. The performance in NV12 is more
then decent from my testing. Though, there is no 10bit variant.

regards,
Nicolas

[0] https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/dr
ivers/media/platform/msm/vidc/msm_vdec.c#695

signature.asc
Description: This is a digitally signed message part

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

2017-03-24 Thread Hans Verkuil 
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 
> ---
>  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 ..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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#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?

> +};
> +
> +static const struct venus_format *find_format(u32 pixfmt, u32 type)
> +{
> + const struct venus_format *fmt = vdec_formats;
> + unsigned int size = ARRAY_SIZE(vdec_formats);
> + unsigned int i;
> +
> + for (i = 0; i < size; i++) {
> + if (fmt[i].pixfmt == pixfmt)
> + break;
> + }
> +
> + if (i == size || fmt[i].type != type)
> + return NULL;
> +
> + return [i];
> +}
> +
> +static const struct venus_format *find_format_by_index(int index, u32 type)
> +{
> + const struct venus_format *fmt = vdec_formats;
> + unsigned int size = ARRAY_SIZE(vdec_formats);
> + int i, k = 0;
> +
> + if (index < 0 || index > size)
> + return NULL;
> +
> + for (i = 0; i < size; i++) {
> + if (fmt[i].type != type)
> +

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

2017-03-13 Thread Stanimir Varbanov 
This consists of video decoder implementation plus decoder
controls.

Signed-off-by: Stanimir Varbanov 
---
 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 ..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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,
+   }, {
+   .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,
+   },
+};
+
+static const struct venus_format *find_format(u32 pixfmt, u32 type)
+{
+   const struct venus_format *fmt = vdec_formats;
+   unsigned int size = ARRAY_SIZE(vdec_formats);
+   unsigned int i;
+
+   for (i = 0; i < size; i++) {
+   if (fmt[i].pixfmt == pixfmt)
+   break;
+   }
+
+   if (i == size || fmt[i].type != type)
+   return NULL;
+
+   return [i];
+}
+
+static const struct venus_format *find_format_by_index(int index, u32 type)
+{
+   const struct venus_format *fmt = vdec_formats;
+   unsigned int size = ARRAY_SIZE(vdec_formats);
+   int i, k = 0;
+
+   if (index < 0 || index > size)
+   return NULL;
+
+   for (i = 0; i < size; i++) {
+   if (fmt[i].type != type)
+   continue;
+   if (k == index)
+   break;
+   k++;
+   }
+
+   if (i == size)
+   return NULL;
+
+   return [i];
+}
+
+static const struct venus_format *
+vdec_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
+{
+   struct v4l2_pix_format_mplane *pixmp = >fmt.pix_mp;
+   struct v4l2_plane_pix_format *pfmt = pixmp->plane_fmt;
+   const struct venus_format *fmt;
+   unsigned int p;
+
+