[PATCH v2 0/8] hantro: set of small cleanups and fixes

[PATCH v2 0/8] hantro: set of small cleanups and fixes

[Ezequiel Garcia]

Hi all,

Cleanups and fixes, second iteration.

The main idea here is to address two issues, and while
at it, clean the driver a bit.

The first issue can be found in Patch 1, when the Request
API is used, the CAPTURE buffer should be returned _before_
the OUTPUT buffer, to avoid waking up userspace prematurely.

I noticed this issue while working on the rkvdec driver,
but this time I've decided to tackle it at the core,
in v4l2_m2m_buf_done_and_job_finish().

The second issue is a simple compliance issue, which is solved
by refactoring the driver, dealing with internal set format
properly.

Changes v2:

* Fix compile warning introduced by patch 6.

* I'm adding two additional patches this time.
  Patch 7 converts the binding to json-schema,
  and patch 8 puts linux-rockchip mailing list in MAINTAINERS. 

Thanks,
Ezequiel

Ezequiel Garcia (8):
  v4l2-mem2mem: return CAPTURE buffer first
  hantro: Set buffers' zeroth plane payload in .buf_prepare
  hantro: Use v4l2_m2m_buf_done_and_job_finish
  hantro: Remove unneeded hantro_dec_buf_finish
  hantro: Move H264 motion vector calculation to a helper
  hantro: Refactor for V4L2 API spec compliancy
  dt-bindings: rockchip-vpu: Convert bindings to json-schema
  hantro: Add linux-rockchip mailing list to MAINTAINERS

 .../bindings/media/rockchip-vpu.txt           |  43 -------
 .../bindings/media/rockchip-vpu.yaml          |  82 +++++++++++++
 MAINTAINERS                                   |   3 +-
 drivers/media/v4l2-core/v4l2-mem2mem.c        |  11 +-
 drivers/staging/media/hantro/hantro.h         |   7 +-
 drivers/staging/media/hantro/hantro_drv.c     |  37 ++----
 drivers/staging/media/hantro/hantro_hw.h      |  31 +++++
 drivers/staging/media/hantro/hantro_v4l2.c    | 111 +++++++++---------
 8 files changed, 194 insertions(+), 131 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/rockchip-vpu.txt
 create mode 100644 Documentation/devicetree/bindings/media/rockchip-vpu.yaml

-- 
2.25.0

[PATCH v2 1/8] v4l2-mem2mem: return CAPTURE buffer first

[Ezequiel Garcia]

When the request API is used, typically an OUTPUT (src) buffer
will be part of a request. A userspace process will be typically
blocked, waiting on the request file descriptor.

Returning the OUTPUT (src) buffer will wake-up such processes,
who will immediately attempt to dequeue the CAPTURE buffer,
only to find it's still unavailable.

Therefore, change v4l2_m2m_buf_done_and_job_finish returning
the CAPTURE (dst) buffer first, to avoid signalling the request
file descriptor prematurely, i.e. before the CAPTURE buffer is done.

When the request API is not used, this change should have
no impact.

Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 8986c31176e9..62ac9424c92a 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -504,12 +504,21 @@ void v4l2_m2m_buf_done_and_job_finish(struct v4l2_m2m_dev *m2m_dev,
 
 	if (WARN_ON(!src_buf || !dst_buf))
 		goto unlock;
-	v4l2_m2m_buf_done(src_buf, state);
 	dst_buf->is_held = src_buf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
 	if (!dst_buf->is_held) {
 		v4l2_m2m_dst_buf_remove(m2m_ctx);
 		v4l2_m2m_buf_done(dst_buf, state);
 	}
+	/*
+	 * If the request API is being used, returning the OUTPUT
+	 * (src) buffer will wake-up any process waiting on the
+	 * request file descriptor.
+	 *
+	 * Therefore, return the CAPTURE (dst) buffer first,
+	 * to avoid signalling the request file descriptor
+	 * before the CAPTURE buffer is done.
+	 */
+	v4l2_m2m_buf_done(src_buf, state);
 	schedule_next = _v4l2_m2m_job_finish(m2m_dev, m2m_ctx);
 unlock:
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
-- 
2.25.0

[PATCH v2 2/8] hantro: Set buffers' zeroth plane payload in .buf_prepare

[Ezequiel Garcia]

Buffers' zeroth plane payload size is calculated at format
negotiation time, and so it can be set in .buf_prepare.

Keep in mind that, to make this change easier, hantro_buf_prepare
is refactored, using the cedrus driver as reference. This results
in cleaner code as byproduct.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_v4l2.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index f4ae2cee0f18..3142ab6697d5 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -608,7 +608,7 @@ hantro_queue_setup(struct vb2_queue *vq, unsigned int *num_buffers,
 }
 
 static int
-hantro_buf_plane_check(struct vb2_buffer *vb, const struct hantro_fmt *vpu_fmt,
+hantro_buf_plane_check(struct vb2_buffer *vb,
 		       struct v4l2_pix_format_mplane *pixfmt)
 {
 	unsigned int sz;
@@ -630,12 +630,18 @@ static int hantro_buf_prepare(struct vb2_buffer *vb)
 {
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct hantro_ctx *ctx = vb2_get_drv_priv(vq);
+	struct v4l2_pix_format_mplane *pix_fmt;
+	int ret;
 
 	if (V4L2_TYPE_IS_OUTPUT(vq->type))
-		return hantro_buf_plane_check(vb, ctx->vpu_src_fmt,
-						  &ctx->src_fmt);
-
-	return hantro_buf_plane_check(vb, ctx->vpu_dst_fmt, &ctx->dst_fmt);
+		pix_fmt = &ctx->src_fmt;
+	else
+		pix_fmt = &ctx->dst_fmt;
+	ret = hantro_buf_plane_check(vb, pix_fmt);
+	if (ret)
+		return ret;
+	vb2_set_plane_payload(vb, 0, pix_fmt->plane_fmt[0].sizeimage);
+	return 0;
 }
 
 static void hantro_buf_queue(struct vb2_buffer *vb)
-- 
2.25.0

[PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish

[Ezequiel Garcia]

Let the core sort out the nuances of returning buffers
to userspace, by using the v4l2_m2m_buf_done_and_job_finish
helper.

This change also removes usage of buffer sequence fields,
which shouldn't have any meaning for stateless decoders.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++---------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 0b1200fc0e1a..ec889d755cd6 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu,
 			      unsigned int bytesused,
 			      enum vb2_buffer_state result)
 {
-	struct vb2_v4l2_buffer *src, *dst;
 	int ret;
 
 	pm_runtime_mark_last_busy(vpu->dev);
 	pm_runtime_put_autosuspend(vpu->dev);
 	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
 
-	src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
-	dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
-
-	if (WARN_ON(!src))
-		return;
-	if (WARN_ON(!dst))
-		return;
-
-	src->sequence = ctx->sequence_out++;
-	dst->sequence = ctx->sequence_cap++;
-
-	ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
-	if (ret)
-		result = VB2_BUF_STATE_ERROR;
+	if (ctx->buf_finish) {
+		struct vb2_v4l2_buffer *dst;
 
-	v4l2_m2m_buf_done(src, result);
-	v4l2_m2m_buf_done(dst, result);
+		dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+		ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
+		if (ret)
+			result = VB2_BUF_STATE_ERROR;
+	}
 
-	v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx);
+	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
+					 result);
 }
 
 void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused,
-- 
2.25.0

[PATCH v2 4/8] hantro: Remove unneeded hantro_dec_buf_finish

[Ezequiel Garcia]

Since now .buf_prepare takes care of setting the
buffer payload size, we can get rid of this,
at least for decoders.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_drv.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index ec889d755cd6..bd204da6c669 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -80,15 +80,6 @@ hantro_enc_buf_finish(struct hantro_ctx *ctx, struct vb2_buffer *buf,
 	return 0;
 }
 
-static int
-hantro_dec_buf_finish(struct hantro_ctx *ctx, struct vb2_buffer *buf,
-		      unsigned int bytesused)
-{
-	/* For decoders set bytesused as per the output picture. */
-	buf->planes[0].bytesused = ctx->dst_fmt.plane_fmt[0].sizeimage;
-	return 0;
-}
-
 static void hantro_job_finish(struct hantro_dev *vpu,
 			      struct hantro_ctx *ctx,
 			      unsigned int bytesused,
@@ -422,7 +413,6 @@ static int hantro_open(struct file *filp)
 		ctx->buf_finish = hantro_enc_buf_finish;
 	} else if (func->id == MEDIA_ENT_F_PROC_VIDEO_DECODER) {
 		allowed_codecs = vpu->variant->codec & HANTRO_DECODERS;
-		ctx->buf_finish = hantro_dec_buf_finish;
 	} else {
 		ret = -ENODEV;
 		goto err_ctx_free;
-- 
2.25.0

[PATCH v2 5/8] hantro: Move H264 motion vector calculation to a helper

[Ezequiel Garcia]

Move the extra bytes calculation that are needed for H264
motion vector to a helper. This is just a cosmetic cleanup.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro.h      |  4 ---
 drivers/staging/media/hantro/hantro_hw.h   | 31 ++++++++++++++++++++++
 drivers/staging/media/hantro/hantro_v4l2.c | 25 ++---------------
 3 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index 327ddef45345..2089f88a44a2 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -26,10 +26,6 @@
 
 #include "hantro_hw.h"
 
-#define MB_DIM			16
-#define MB_WIDTH(w)		DIV_ROUND_UP(w, MB_DIM)
-#define MB_HEIGHT(h)		DIV_ROUND_UP(h, MB_DIM)
-
 struct hantro_ctx;
 struct hantro_codec_ops;
 
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 2398d4c1f207..435f30ae89fd 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -18,6 +18,10 @@
 
 #define DEC_8190_ALIGN_MASK	0x07U
 
+#define MB_DIM			16
+#define MB_WIDTH(w)		DIV_ROUND_UP(w, MB_DIM)
+#define MB_HEIGHT(h)		DIV_ROUND_UP(h, MB_DIM)
+
 struct hantro_dev;
 struct hantro_ctx;
 struct hantro_buf;
@@ -175,6 +179,33 @@ void hantro_g1_h264_dec_run(struct hantro_ctx *ctx);
 int hantro_h264_dec_init(struct hantro_ctx *ctx);
 void hantro_h264_dec_exit(struct hantro_ctx *ctx);
 
+static inline size_t
+hantro_h264_mv_size(unsigned int width, unsigned int height)
+{
+	/*
+	 * A decoded 8-bit 4:2:0 NV12 frame may need memory for up to
+	 * 448 bytes per macroblock with additional 32 bytes on
+	 * multi-core variants.
+	 *
+	 * The H264 decoder needs extra space on the output buffers
+	 * to store motion vectors. This is needed for reference
+	 * frames and only if the format is non-post-processed NV12.
+	 *
+	 * Memory layout is as follow:
+	 *
+	 * +---------------------------+
+	 * | Y-plane   256 bytes x MBs |
+	 * +---------------------------+
+	 * | UV-plane  128 bytes x MBs |
+	 * +---------------------------+
+	 * | MV buffer  64 bytes x MBs |
+	 * +---------------------------+
+	 * | MC sync          32 bytes |
+	 * +---------------------------+
+	 */
+	return 64 * MB_WIDTH(width) * MB_WIDTH(height) + 32;
+}
+
 void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx);
 void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx);
 void hantro_mpeg2_dec_copy_qtable(u8 *qtable,
diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 3142ab6697d5..458b502ff01b 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -273,32 +273,11 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f,
 		/* Fill remaining fields */
 		v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width,
 				    pix_mp->height);
-		/*
-		 * A decoded 8-bit 4:2:0 NV12 frame may need memory for up to
-		 * 448 bytes per macroblock with additional 32 bytes on
-		 * multi-core variants.
-		 *
-		 * The H264 decoder needs extra space on the output buffers
-		 * to store motion vectors. This is needed for reference
-		 * frames and only if the format is non-post-processed NV12.
-		 *
-		 * Memory layout is as follow:
-		 *
-		 * +---------------------------+
-		 * | Y-plane   256 bytes x MBs |
-		 * +---------------------------+
-		 * | UV-plane  128 bytes x MBs |
-		 * +---------------------------+
-		 * | MV buffer  64 bytes x MBs |
-		 * +---------------------------+
-		 * | MC sync          32 bytes |
-		 * +---------------------------+
-		 */
 		if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE &&
 		    !hantro_needs_postproc(ctx, fmt))
 			pix_mp->plane_fmt[0].sizeimage +=
-				64 * MB_WIDTH(pix_mp->width) *
-				     MB_WIDTH(pix_mp->height) + 32;
+				hantro_h264_mv_size(pix_mp->width,
+						    pix_mp->height);
 	} else if (!pix_mp->plane_fmt[0].sizeimage) {
 		/*
 		 * For coded formats the application can specify
-- 
2.25.0

[PATCH v2 6/8] hantro: Refactor for V4L2 API spec compliancy

[Ezequiel Garcia]

Refactor how S_FMT and TRY_FMT are handled, and also make sure
internal initial format and format reset are done properly.

The latter is achieved by making sure the same hantro_{set,try}_fmt
helpers are called on all paths that set the format (which is
part of the driver state).

This commit removes the following v4l2-compliance warnings:

test VIDIOC_G_FMT: OK
	fail: v4l2-test-formats.cpp(711): Video Capture Multiplanar: TRY_FMT(G_FMT) != G_FMT
test VIDIOC_TRY_FMT: FAIL
	fail: v4l2-test-formats.cpp(1116): Video Capture Multiplanar: S_FMT(G_FMT) != G_FMT
test VIDIOC_S_FMT: FAIL

Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
v2:
* Fix warning by using a proper const qualifier
  in hantro_needs_postproc() args.

 drivers/staging/media/hantro/hantro.h      |  3 +-
 drivers/staging/media/hantro/hantro_v4l2.c | 70 ++++++++++++++--------
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index 2089f88a44a2..3005207fc6fb 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -417,7 +417,8 @@ hantro_get_dst_buf(struct hantro_ctx *ctx)
 }
 
 static inline bool
-hantro_needs_postproc(struct hantro_ctx *ctx, const struct hantro_fmt *fmt)
+hantro_needs_postproc(const struct hantro_ctx *ctx,
+		      const struct hantro_fmt *fmt)
 {
 	return !hantro_is_encoder_ctx(ctx) && fmt->fourcc != V4L2_PIX_FMT_NV12;
 }
diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 458b502ff01b..f28a94e2fa93 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -30,6 +30,11 @@
 #include "hantro_hw.h"
 #include "hantro_v4l2.h"
 
+static int hantro_set_fmt_out(struct hantro_ctx *ctx,
+			      struct v4l2_pix_format_mplane *pix_mp);
+static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
+			      struct v4l2_pix_format_mplane *pix_mp);
+
 static const struct hantro_fmt *
 hantro_get_formats(const struct hantro_ctx *ctx, unsigned int *num_fmts)
 {
@@ -227,12 +232,12 @@ static int vidioc_g_fmt_cap_mplane(struct file *file, void *priv,
 	return 0;
 }
 
-static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f,
-			  bool capture)
+static int hantro_try_fmt(const struct hantro_ctx *ctx,
+			  struct v4l2_pix_format_mplane *pix_mp,
+			  enum v4l2_buf_type type)
 {
-	struct hantro_ctx *ctx = fh_to_ctx(priv);
-	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
 	const struct hantro_fmt *fmt, *vpu_fmt;
+	bool capture = !V4L2_TYPE_IS_OUTPUT(type);
 	bool coded;
 
 	coded = capture == hantro_is_encoder_ctx(ctx);
@@ -246,7 +251,7 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f,
 	fmt = hantro_find_format(ctx, pix_mp->pixelformat);
 	if (!fmt) {
 		fmt = hantro_get_default_fmt(ctx, coded);
-		f->fmt.pix_mp.pixelformat = fmt->fourcc;
+		pix_mp->pixelformat = fmt->fourcc;
 	}
 
 	if (coded) {
@@ -294,13 +299,13 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f,
 static int vidioc_try_fmt_cap_mplane(struct file *file, void *priv,
 				     struct v4l2_format *f)
 {
-	return vidioc_try_fmt(file, priv, f, true);
+	return hantro_try_fmt(fh_to_ctx(priv), &f->fmt.pix_mp, f->type);
 }
 
 static int vidioc_try_fmt_out_mplane(struct file *file, void *priv,
 				     struct v4l2_format *f)
 {
-	return vidioc_try_fmt(file, priv, f, false);
+	return hantro_try_fmt(fh_to_ctx(priv), &f->fmt.pix_mp, f->type);
 }
 
 static void
@@ -334,11 +339,12 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
 	}
 
 	hantro_reset_fmt(fmt, vpu_fmt);
-	fmt->num_planes = 1;
 	fmt->width = vpu_fmt->frmsize.min_width;
 	fmt->height = vpu_fmt->frmsize.min_height;
-	fmt->plane_fmt[0].sizeimage = vpu_fmt->header_size +
-				fmt->width * fmt->height * vpu_fmt->max_depth;
+	if (hantro_is_encoder_ctx(ctx))
+		hantro_set_fmt_cap(ctx, fmt);
+	else
+		hantro_set_fmt_out(ctx, fmt);
 }
 
 static void
@@ -360,9 +366,12 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
 	}
 
 	hantro_reset_fmt(raw_fmt, raw_vpu_fmt);
-	v4l2_fill_pixfmt_mp(raw_fmt, raw_vpu_fmt->fourcc,
-			    encoded_fmt->width,
-			    encoded_fmt->height);
+	raw_fmt->width = encoded_fmt->width;
+	raw_fmt->width = encoded_fmt->width;
+	if (hantro_is_encoder_ctx(ctx))
+		hantro_set_fmt_out(ctx, raw_fmt);
+	else
+		hantro_set_fmt_cap(ctx, raw_fmt);
 }
 
 void hantro_reset_fmts(struct hantro_ctx *ctx)
@@ -388,15 +397,15 @@ hantro_update_requires_request(struct hantro_ctx *ctx, u32 fourcc)
 	}
 }
 
-static int
-vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
+static int hantro_set_fmt_out(struct hantro_ctx *ctx,
+			      struct v4l2_pix_format_mplane *pix_mp)
 {
-	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
-	struct hantro_ctx *ctx = fh_to_ctx(priv);
-	struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
+	struct vb2_queue *vq;
 	int ret;
 
-	ret = vidioc_try_fmt_out_mplane(file, priv, f);
+	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
+			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
+	ret = hantro_try_fmt(ctx, pix_mp, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
 	if (ret)
 		return ret;
 
@@ -458,16 +467,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
 	return 0;
 }
 
-static int vidioc_s_fmt_cap_mplane(struct file *file, void *priv,
-				   struct v4l2_format *f)
+static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
+			      struct v4l2_pix_format_mplane *pix_mp)
 {
-	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
-	struct hantro_ctx *ctx = fh_to_ctx(priv);
 	struct vb2_queue *vq;
 	int ret;
 
 	/* Change not allowed if queue is busy. */
-	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
+	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
+			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
 	if (vb2_is_busy(vq))
 		return -EBUSY;
 
@@ -488,7 +496,7 @@ static int vidioc_s_fmt_cap_mplane(struct file *file, void *priv,
 			return -EBUSY;
 	}
 
-	ret = vidioc_try_fmt_cap_mplane(file, priv, f);
+	ret = hantro_try_fmt(ctx, pix_mp, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
 	if (ret)
 		return ret;
 
@@ -522,6 +530,18 @@ static int vidioc_s_fmt_cap_mplane(struct file *file, void *priv,
 	return 0;
 }
 
+static int
+vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
+{
+	return hantro_set_fmt_out(fh_to_ctx(priv), &f->fmt.pix_mp);
+}
+
+static int
+vidioc_s_fmt_cap_mplane(struct file *file, void *priv, struct v4l2_format *f)
+{
+	return hantro_set_fmt_cap(fh_to_ctx(priv), &f->fmt.pix_mp);
+}
+
 const struct v4l2_ioctl_ops hantro_ioctl_ops = {
 	.vidioc_querycap = vidioc_querycap,
 	.vidioc_enum_framesizes = vidioc_enum_framesizes,
-- 
2.25.0

[PATCH v2 7/8] dt-bindings: rockchip-vpu: Convert bindings to json-schema

[Ezequiel Garcia]

Convert Rockchip VPU (Hantro IP block) codec driver documentation to
json-schema.

Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../bindings/media/rockchip-vpu.txt           | 43 ----------
 .../bindings/media/rockchip-vpu.yaml          | 82 +++++++++++++++++++
 MAINTAINERS                                   |  2 +-
 3 files changed, 83 insertions(+), 44 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/rockchip-vpu.txt
 create mode 100644 Documentation/devicetree/bindings/media/rockchip-vpu.yaml

diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.txt b/Documentation/devicetree/bindings/media/rockchip-vpu.txt
deleted file mode 100644
index 339252d9c515..000000000000
--- a/Documentation/devicetree/bindings/media/rockchip-vpu.txt
+++ /dev/null
@@ -1,43 +0,0 @@
-device-tree bindings for rockchip VPU codec
-
-Rockchip (Video Processing Unit) present in various Rockchip platforms,
-such as RK3288, RK3328 and RK3399.
-
-Required properties:
-- compatible: value should be one of the following
-		"rockchip,rk3288-vpu";
-		"rockchip,rk3328-vpu";
-		"rockchip,rk3399-vpu";
-- interrupts: encoding and decoding interrupt specifiers
-- interrupt-names: should be
-		"vepu", "vdpu" on RK3288 and RK3399,
-		"vdpu" on RK3328.
-- clocks: phandle to VPU aclk, hclk clocks
-- clock-names: should be "aclk" and "hclk"
-- power-domains: phandle to power domain node
-- iommus: phandle to a iommu node
-
-Example:
-SoC-specific DT entry:
-	vpu: video-codec@ff9a0000 {
-		compatible = "rockchip,rk3288-vpu";
-		reg = <0x0 0xff9a0000 0x0 0x800>;
-		interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
-		interrupt-names = "vepu", "vdpu";
-		clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
-		clock-names = "aclk", "hclk";
-		power-domains = <&power RK3288_PD_VIDEO>;
-		iommus = <&vpu_mmu>;
-	};
-
-	vpu: video-codec@ff350000 {
-		compatible = "rockchip,rk3328-vpu";
-		reg = <0x0 0xff350000 0x0 0x800>;
-		interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
-		interrupt-names = "vdpu";
-		clocks = <&cru ACLK_VPU>, <&cru HCLK_VPU>;
-		clock-names = "aclk", "hclk";
-		power-domains = <&power RK3328_PD_VPU>;
-		iommus = <&vpu_mmu>;
-	};
diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
new file mode 100644
index 000000000000..a0c45e05cf03
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/media/rockchip-vpu.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Hantro G1 VPU codecs implemented on Rockchip SoCs
+
+maintainers:
+  - Ezequiel Garcia <ezequiel@collabora.com>
+
+description:
+  Hantro G1 video encode and decode accelerators present on Rockchip SoCs.
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3288-vpu
+      - rockchip,rk3328-vpu
+      - rockchip,rk3399-vpu
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 2
+
+  interrupt-names:
+    items:
+      - const: vepu
+      - const: vdpu
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: aclk
+      - const: hclk
+
+  power-domains:
+    maxItems: 1
+
+  iommus:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+
+examples:
+  - |
+        #include <dt-bindings/clock/rk3288-cru.h>
+        #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+        vpu: video-codec@ff9a0000 {
+                compatible = "rockchip,rk3288-vpu";
+                reg = <0x0 0xff9a0000 0x0 0x800>;
+                interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
+                             <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
+                interrupt-names = "vepu", "vdpu";
+                clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
+                clock-names = "aclk", "hclk";
+                power-domains = <&power RK3288_PD_VIDEO>;
+                iommus = <&vpu_mmu>;
+        };
+
+        vpu: video-codec@ff350000 {
+                compatible = "rockchip,rk3328-vpu";
+                reg = <0x0 0xff350000 0x0 0x800>;
+                interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+                interrupt-names = "vdpu";
+                clocks = <&cru ACLK_VPU>, <&cru HCLK_VPU>;
+                clock-names = "aclk", "hclk";
+                power-domains = <&power RK3328_PD_VPU>;
+                iommus = <&vpu_mmu>;
+        };
diff --git a/MAINTAINERS b/MAINTAINERS
index a0fc9dae4622..28bbbb6c73ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14315,7 +14315,7 @@ M:	Ezequiel Garcia <ezequiel@collabora.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/staging/media/hantro/
-F:	Documentation/devicetree/bindings/media/rockchip-vpu.txt
+F:	Documentation/devicetree/bindings/media/rockchip-vpu.yaml
 
 ROCKER DRIVER
 M:	Jiri Pirko <jiri@resnulli.us>
-- 
2.25.0

[PATCH v2 8/8] hantro: Add linux-rockchip mailing list to MAINTAINERS

[Ezequiel Garcia]

The linux-rockchip mailing list is relevant for the
Hantro driver, given this support the VPU present
in Rockchip SoCs.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 28bbbb6c73ef..dc56b9bfc3b3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14313,6 +14313,7 @@ F:	Documentation/devicetree/bindings/media/rockchip-rga.txt
 HANTRO VPU CODEC DRIVER
 M:	Ezequiel Garcia <ezequiel@collabora.com>
 L:	linux-media@vger.kernel.org
+L:	linux-rockchip@lists.infradead.org
 S:	Maintained
 F:	drivers/staging/media/hantro/
 F:	Documentation/devicetree/bindings/media/rockchip-vpu.yaml
-- 
2.25.0

Re: [PATCH v2 8/8] hantro: Add linux-rockchip mailing list to MAINTAINERS

[Heiko Stübner]

Am Mittwoch, 18. März 2020, 14:21:08 CET schrieb Ezequiel Garcia:
> The linux-rockchip mailing list is relevant for the
> Hantro driver, given this support the VPU present
> in Rockchip SoCs.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 28bbbb6c73ef..dc56b9bfc3b3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14313,6 +14313,7 @@ F:	Documentation/devicetree/bindings/media/rockchip-rga.txt
>  HANTRO VPU CODEC DRIVER
>  M:	Ezequiel Garcia <ezequiel@collabora.com>
>  L:	linux-media@vger.kernel.org
> +L:	linux-rockchip@lists.infradead.org
>  S:	Maintained
>  F:	drivers/staging/media/hantro/
>  F:	Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> 

Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish

[Hans Verkuil]

On 3/18/20 2:21 PM, Ezequiel Garcia wrote:
> Let the core sort out the nuances of returning buffers
> to userspace, by using the v4l2_m2m_buf_done_and_job_finish
> helper.
> 
> This change also removes usage of buffer sequence fields,
> which shouldn't have any meaning for stateless decoders.

Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance?
Also, while I agree that it is not terribly useful, it doesn't hurt, does it?

And the V4L2 spec makes no exception for stateless codecs with respect to the
sequence field.

Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++---------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 0b1200fc0e1a..ec889d755cd6 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu,
>  			      unsigned int bytesused,
>  			      enum vb2_buffer_state result)
>  {
> -	struct vb2_v4l2_buffer *src, *dst;
>  	int ret;
>  
>  	pm_runtime_mark_last_busy(vpu->dev);
>  	pm_runtime_put_autosuspend(vpu->dev);
>  	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
>  
> -	src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> -	dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> -
> -	if (WARN_ON(!src))
> -		return;
> -	if (WARN_ON(!dst))
> -		return;
> -
> -	src->sequence = ctx->sequence_out++;
> -	dst->sequence = ctx->sequence_cap++;
> -
> -	ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
> -	if (ret)
> -		result = VB2_BUF_STATE_ERROR;
> +	if (ctx->buf_finish) {
> +		struct vb2_v4l2_buffer *dst;
>  
> -	v4l2_m2m_buf_done(src, result);
> -	v4l2_m2m_buf_done(dst, result);
> +		dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> +		ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
> +		if (ret)
> +			result = VB2_BUF_STATE_ERROR;
> +	}
>  
> -	v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx);
> +	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
> +					 result);
>  }
>  
>  void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused,
> 

Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish

[Nicolas Dufresne]

Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit :
> On 3/18/20 2:21 PM, Ezequiel Garcia wrote:
> > Let the core sort out the nuances of returning buffers
> > to userspace, by using the v4l2_m2m_buf_done_and_job_finish
> > helper.
> > 
> > This change also removes usage of buffer sequence fields,
> > which shouldn't have any meaning for stateless decoders.
> 
> Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance?
> Also, while I agree that it is not terribly useful, it doesn't hurt, does it?
> 
> And the V4L2 spec makes no exception for stateless codecs with respect to the
> sequence field.
> 
> Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

The spec also does not say what it means either. As an example, you
have spec for ALTERNATE interlacing mode that changes the meaning of
the sequence, but not for alternate H264 fields (which cannot be part
of the format, since this changes often). We also don't have spec for
the the sequence behaviour while using HOLD features.

I'm worried we are falling into the test driven trap, were people
implement features to satisfy a test, while the added complexity don't
really make sense. Shouldn't we change our approach and opt-out
features for new type of HW, so that we can keep the drivers code
saner?

> 
> Regards,
> 
> 	Hans
> 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++---------------
> >  1 file changed, 9 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index 0b1200fc0e1a..ec889d755cd6 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu,
> >  			      unsigned int bytesused,
> >  			      enum vb2_buffer_state result)
> >  {
> > -	struct vb2_v4l2_buffer *src, *dst;
> >  	int ret;
> >  
> >  	pm_runtime_mark_last_busy(vpu->dev);
> >  	pm_runtime_put_autosuspend(vpu->dev);
> >  	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> >  
> > -	src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > -	dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > -
> > -	if (WARN_ON(!src))
> > -		return;
> > -	if (WARN_ON(!dst))
> > -		return;
> > -
> > -	src->sequence = ctx->sequence_out++;
> > -	dst->sequence = ctx->sequence_cap++;
> > -
> > -	ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
> > -	if (ret)
> > -		result = VB2_BUF_STATE_ERROR;
> > +	if (ctx->buf_finish) {
> > +		struct vb2_v4l2_buffer *dst;
> >  
> > -	v4l2_m2m_buf_done(src, result);
> > -	v4l2_m2m_buf_done(dst, result);
> > +		dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > +		ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
> > +		if (ret)
> > +			result = VB2_BUF_STATE_ERROR;
> > +	}
> >  
> > -	v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx);
> > +	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
> > +					 result);
> >  }
> >  
> >  void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused,
> > 

Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish

[Hans Verkuil]

On 3/25/20 3:02 PM, Nicolas Dufresne wrote:
> Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit :
>> On 3/18/20 2:21 PM, Ezequiel Garcia wrote:
>>> Let the core sort out the nuances of returning buffers
>>> to userspace, by using the v4l2_m2m_buf_done_and_job_finish
>>> helper.
>>>
>>> This change also removes usage of buffer sequence fields,
>>> which shouldn't have any meaning for stateless decoders.
>>
>> Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance?
>> Also, while I agree that it is not terribly useful, it doesn't hurt, does it?
>>
>> And the V4L2 spec makes no exception for stateless codecs with respect to the
>> sequence field.
>>
>> Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> The spec also does not say what it means either. As an example, you
> have spec for ALTERNATE interlacing mode that changes the meaning of
> the sequence, but not for alternate H264 fields (which cannot be part
> of the format, since this changes often). We also don't have spec for
> the the sequence behaviour while using HOLD features.

I hate it that the spec changes the sequence meaning for FIELD_ALTERNATE,
I always thought that that made drivers unnecessarily complicated. Unfortunately,
this is something we inherited.

Currently the spec says for sequence:

"Set by the driver, counting the frames (not fields!) in sequence. This field is set
 for both input and output devices."

The only thing missing here is that it should say that for compressed formats this
counts the buffers, since one buffer with compressed data may not have a one-to-one
mapping with frames.

This description for 'sequence' was never updated when compressed data formats were
added, so it is a bit out of date.

> 
> I'm worried we are falling into the test driven trap, were people
> implement features to satisfy a test, while the added complexity don't
> really make sense. Shouldn't we change our approach and opt-out
> features for new type of HW, so that we can keep the drivers code
> saner?

Why wasn't the existing code in this patch sane? Sure, we can change the spec, but
then 1) all existing drivers need to be updated as well, and 2) v4l2-compliance needs
to be changed to test specifically for this class of drivers and ensure that for those
the sequence field is set to 0. Not to mention introducing an exception in the uAPI
where the sequence field suddenly isn't used anymore.

Frankly, I would prefer that the whole sequence handling is moved to videobuf2-v4l2.c.
It really doesn't belong in drivers, with the exception of incrementing the sequence
counter in case of dropped frames.

I think I suggested it when vb2 was being designed, but at the time the preference
was to keep it in the driver. Long time ago, though.

And another reason why I want to keep it: I find it actually useful to see a running
counter: it helps keeping track of how many buffers you've processed since you started
streaming.

Finally, the removal of the sequence counter simply does not belong in this patch.

Regards,

	Hans

> 
>>
>> Regards,
>>
>> 	Hans
>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>  drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++---------------
>>>  1 file changed, 9 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
>>> index 0b1200fc0e1a..ec889d755cd6 100644
>>> --- a/drivers/staging/media/hantro/hantro_drv.c
>>> +++ b/drivers/staging/media/hantro/hantro_drv.c
>>> @@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu,
>>>  			      unsigned int bytesused,
>>>  			      enum vb2_buffer_state result)
>>>  {
>>> -	struct vb2_v4l2_buffer *src, *dst;
>>>  	int ret;
>>>  
>>>  	pm_runtime_mark_last_busy(vpu->dev);
>>>  	pm_runtime_put_autosuspend(vpu->dev);
>>>  	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
>>>  
>>> -	src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
>>> -	dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>>> -
>>> -	if (WARN_ON(!src))
>>> -		return;
>>> -	if (WARN_ON(!dst))
>>> -		return;
>>> -
>>> -	src->sequence = ctx->sequence_out++;
>>> -	dst->sequence = ctx->sequence_cap++;
>>> -
>>> -	ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
>>> -	if (ret)
>>> -		result = VB2_BUF_STATE_ERROR;
>>> +	if (ctx->buf_finish) {
>>> +		struct vb2_v4l2_buffer *dst;
>>>  
>>> -	v4l2_m2m_buf_done(src, result);
>>> -	v4l2_m2m_buf_done(dst, result);
>>> +		dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>>> +		ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
>>> +		if (ret)
>>> +			result = VB2_BUF_STATE_ERROR;
>>> +	}
>>>  
>>> -	v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx);
>>> +	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
>>> +					 result);
>>>  }
>>>  
>>>  void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused,
>>>
> 

Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish

[Ezequiel Garcia]

   1. On Wed, 2020-03-25 at 16:28 +0100, Hans Verkuil wrote:
> On 3/25/20 3:02 PM, Nicolas Dufresne wrote:
> > Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit :
> > > On 3/18/20 2:21 PM, Ezequiel Garcia wrote:
> > > > Let the core sort out the nuances of returning buffers
> > > > to userspace, by using the v4l2_m2m_buf_done_and_job_finish
> > > > helper.
> > > > 
> > > > This change also removes usage of buffer sequence fields,
> > > > which shouldn't have any meaning for stateless decoders.
> > > 
> > > Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance?
> > > Also, while I agree that it is not terribly useful, it doesn't hurt, does it?
> > > 
> > > And the V4L2 spec makes no exception for stateless codecs with respect to the
> > > sequence field.
> > > 
> > > Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > 
> > The spec also does not say what it means either. As an example, you
> > have spec for ALTERNATE interlacing mode that changes the meaning of
> > the sequence, but not for alternate H264 fields (which cannot be part
> > of the format, since this changes often). We also don't have spec for
> > the the sequence behaviour while using HOLD features.
> 
> I hate it that the spec changes the sequence meaning for FIELD_ALTERNATE,
> I always thought that that made drivers unnecessarily complicated. Unfortunately,
> this is something we inherited.
> 
> Currently the spec says for sequence:
> 
> "Set by the driver, counting the frames (not fields!) in sequence. This field is set
>  for both input and output devices."
> 
> The only thing missing here is that it should say that for compressed formats this
> counts the buffers, since one buffer with compressed data may not have a one-to-one
> mapping with frames.
> 
> This description for 'sequence' was never updated when compressed data formats were
> added, so it is a bit out of date.
> 
> > I'm worried we are falling into the test driven trap, were people
> > implement features to satisfy a test, while the added complexity don't
> > really make sense. Shouldn't we change our approach and opt-out
> > features for new type of HW, so that we can keep the drivers code
> > saner?
> 
> Why wasn't the existing code in this patch sane? Sure, we can change the spec, but
> then 1) all existing drivers need to be updated as well, and 2) v4l2-compliance needs
> to be changed to test specifically for this class of drivers and ensure that for those
> the sequence field is set to 0. Not to mention introducing an exception in the uAPI
> where the sequence field suddenly isn't used anymore.
> 
> Frankly, I would prefer that the whole sequence handling is moved to videobuf2-v4l2.c.
> It really doesn't belong in drivers, with the exception of incrementing the sequence
> counter in case of dropped frames.
> 
> I think I suggested it when vb2 was being designed, but at the time the preference
> was to keep it in the driver. Long time ago, though.
> 

Do you think we could try to move this to the core?

I might be able find some time to try that.

> And another reason why I want to keep it: I find it actually useful to see a running
> counter: it helps keeping track of how many buffers you've processed since you started
> streaming.
> 

+1

> Finally, the removal of the sequence counter simply does not belong in this patch.
> 

Agreed, no complaints on my side.

I am actually happy about this feedback.

Thanks,
Ezequiel

Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish

[Nicolas Dufresne]

Le mercredi 25 mars 2020 à 17:30 -0300, Ezequiel Garcia a écrit :
>    1. On Wed, 2020-03-25 at 16:28 +0100, Hans Verkuil wrote:
> > On 3/25/20 3:02 PM, Nicolas Dufresne wrote:
> > > Le mercredi 25 mars 2020 à 09:22 +0100, Hans Verkuil a écrit :
> > > > On 3/18/20 2:21 PM, Ezequiel Garcia wrote:
> > > > > Let the core sort out the nuances of returning buffers
> > > > > to userspace, by using the v4l2_m2m_buf_done_and_job_finish
> > > > > helper.
> > > > > 
> > > > > This change also removes usage of buffer sequence fields,
> > > > > which shouldn't have any meaning for stateless decoders.
> > > > 
> > > > Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance?
> > > > Also, while I agree that it is not terribly useful, it doesn't hurt, does it?
> > > > 
> > > > And the V4L2 spec makes no exception for stateless codecs with respect to the
> > > > sequence field.
> > > > 
> > > > Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > 
> > > The spec also does not say what it means either. As an example, you
> > > have spec for ALTERNATE interlacing mode that changes the meaning of
> > > the sequence, but not for alternate H264 fields (which cannot be part
> > > of the format, since this changes often). We also don't have spec for
> > > the the sequence behaviour while using HOLD features.
> > 
> > I hate it that the spec changes the sequence meaning for FIELD_ALTERNATE,
> > I always thought that that made drivers unnecessarily complicated. Unfortunately,
> > this is something we inherited.
> > 
> > Currently the spec says for sequence:
> > 
> > "Set by the driver, counting the frames (not fields!) in sequence. This field is set
> >  for both input and output devices."
> > 
> > The only thing missing here is that it should say that for compressed formats this
> > counts the buffers, since one buffer with compressed data may not have a one-to-one
> > mapping with frames.

That's also why I think it's programatically useless in that case, there is no
logic for which input/output can be related unless you know what the framing is.

> > 
> > This description for 'sequence' was never updated when compressed data formats were
> > added, so it is a bit out of date.
> > 
> > > I'm worried we are falling into the test driven trap, were people
> > > implement features to satisfy a test, while the added complexity don't
> > > really make sense. Shouldn't we change our approach and opt-out
> > > features for new type of HW, so that we can keep the drivers code
> > > saner?
> > 
> > Why wasn't the existing code in this patch sane? Sure, we can change the spec, but
> > then 1) all existing drivers need to be updated as well, and 2) v4l2-compliance needs
> > to be changed to test specifically for this class of drivers and ensure that for those
> > the sequence field is set to 0. Not to mention introducing an exception in the uAPI
> > where the sequence field suddenly isn't used anymore.
> > 
> > Frankly, I would prefer that the whole sequence handling is moved to videobuf2-v4l2.c.
> > It really doesn't belong in drivers, with the exception of incrementing the sequence
> > counter in case of dropped frames.
> > 
> > I think I suggested it when vb2 was being designed, but at the time the preference
> > was to keep it in the driver. Long time ago, though.
> > 
> 
> Do you think we could try to move this to the core?

I'm also happy as long as drivers stop having to implement this generic
statistic. Note, that only applies to existing m2m, we still need that counter
to detect driver side frame drops in CAPTURE only devices (like UVC cameras).

> 
> I might be able find some time to try that.
> 
> > And another reason why I want to keep it: I find it actually useful to see a running
> > counter: it helps keeping track of how many buffers you've processed since you started
> > streaming.
> > 
> 
> +1
> 
> > Finally, the removal of the sequence counter simply does not belong in this patch.
> > 
> 
> Agreed, no complaints on my side.
> 
> I am actually happy about this feedback.
> 
> Thanks,
> Ezequiel
> 
>