[PATCH 0/6] Exynos video fixes from ChromeOS

[PATCH 0/6] Exynos video fixes from ChromeOS

[John Sheu]

These are various video processing driver fixes for the s5p-mfc and gsc-m2m
hardware blocks on the Samsung Exynos (ARM) platform, that have been carried
in the ChromeOS kernel tree for a while and should be pushed upstream.

John Sheu (6):
  [media] s5p-mfc: fix DISPLAY_DELAY
  [media] s5p-mfc: fix encoder crash after VIDIOC_STREAMOFF
  [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder
  [media] s5p-mfc: support dynamic encoding parameter changes
  [media] gsc-m2m: report correct format bytesperline and sizeimage
  [media] v4l2-mem2mem: allow reqbufs(0) with "in use" MMAP buffers

 drivers/media/platform/exynos-gsc/gsc-core.c    | 153 +++++++++++++-----------
 drivers/media/platform/exynos-gsc/gsc-core.h    |  16 +--
 drivers/media/platform/exynos-gsc/gsc-regs.c    |  40 ++++---
 drivers/media/platform/exynos-gsc/gsc-regs.h    |   4 +-
 drivers/media/platform/s5p-mfc/regs-mfc-v6.h    |   4 +
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  38 ++++--
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |   7 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 126 ++++++++++++++++---
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  29 +++--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 119 +++++++++---------
 drivers/media/v4l2-core/videobuf2-core.c        |  26 +---
 11 files changed, 339 insertions(+), 223 deletions(-)

-- 
1.8.4

[PATCH 1/6] [media] s5p-mfc: fix DISPLAY_DELAY

[John Sheu]

V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY_ENABLE is being
ignored, and the display delay is always being applied.  Fix this.

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 461358c..5bf6efd 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -1271,11 +1271,8 @@ static int s5p_mfc_init_decode_v6(struct s5p_mfc_ctx *ctx)
 	/* FMO_ASO_CTRL - 0: Enable, 1: Disable */
 	reg |= (fmo_aso_ctrl << S5P_FIMV_D_OPT_FMO_ASO_CTRL_MASK_V6);
 
-	/* When user sets desplay_delay to 0,
-	 * It works as "display_delay enable" and delay set to 0.
-	 * If user wants display_delay disable, It should be
-	 * set to negative value. */
-	if (ctx->display_delay >= 0) {
+	/* Setup display delay, only if enabled. */
+	if (ctx->display_delay_enable) {
 		reg |= (0x1 << S5P_FIMV_D_OPT_DDELAY_EN_SHIFT_V6);
 		WRITEL(ctx->display_delay, S5P_FIMV_D_DISPLAY_DELAY_V6);
 	}
-- 
1.8.4

[PATCH 2/6] [media] s5p-mfc: fix encoder crash after VIDIOC_STREAMOFF

[John Sheu]

VIDIOC_STREAMOFF clears the encoder's destination queue -- routines run
from the interrupt handler cannot assume that the queue is non-empty.

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 41f5a3c..8b24829 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -714,13 +714,16 @@ static int enc_post_seq_start(struct s5p_mfc_ctx *ctx)
 
 	if (p->seq_hdr_mode == V4L2_MPEG_VIDEO_HEADER_MODE_SEPARATE) {
 		spin_lock_irqsave(&dev->irqlock, flags);
-		dst_mb = list_entry(ctx->dst_queue.next,
-				struct s5p_mfc_buf, list);
-		list_del(&dst_mb->list);
-		ctx->dst_queue_cnt--;
-		vb2_set_plane_payload(dst_mb->b, 0,
-			s5p_mfc_hw_call(dev->mfc_ops, get_enc_strm_size, dev));
-		vb2_buffer_done(dst_mb->b, VB2_BUF_STATE_DONE);
+		if (!list_empty(&ctx->dst_queue)) {
+			dst_mb = list_entry(ctx->dst_queue.next,
+					struct s5p_mfc_buf, list);
+			list_del(&dst_mb->list);
+			ctx->dst_queue_cnt--;
+			vb2_set_plane_payload(dst_mb->b, 0,
+				s5p_mfc_hw_call(dev->mfc_ops, get_enc_strm_size,
+						dev));
+			vb2_buffer_done(dst_mb->b, VB2_BUF_STATE_DONE);
+		}
 		spin_unlock_irqrestore(&dev->irqlock, flags);
 	}
 
@@ -825,8 +828,7 @@ static int enc_post_frame_start(struct s5p_mfc_ctx *ctx)
 		mfc_debug(2, "enc src count: %d, enc ref count: %d\n",
 			  ctx->src_queue_cnt, ctx->ref_queue_cnt);
 	}
-	if (strm_size > 0) {
-		/* at least one more dest. buffers exist always  */
+	if ((ctx->dst_queue_cnt > 0) && (strm_size > 0)) {
 		mb_entry = list_entry(ctx->dst_queue.next, struct s5p_mfc_buf,
 									list);
 		list_del(&mb_entry->list);
-- 
1.8.4

[PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[John Sheu]

Allow userspace to set the crop rect of the input image buffer to
encode.

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  6 ++-
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |  7 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 54 ++++++++++++++++++++++++-
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 +++++---
 4 files changed, 70 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 6920b54..48f706f 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -428,8 +428,10 @@ struct s5p_mfc_vp8_enc_params {
  * struct s5p_mfc_enc_params - general encoding parameters
  */
 struct s5p_mfc_enc_params {
-	u16 width;
-	u16 height;
+	u16 crop_left_offset;
+	u16 crop_right_offset;
+	u16 crop_top_offset;
+	u16 crop_bottom_offset;
 
 	u16 gop_size;
 	enum v4l2_mpeg_video_multi_slice_mode slice_mode;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 8faf969..e99bcb8 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -334,10 +334,9 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	    ctx->state >= MFCINST_HEAD_PARSED &&
 	    ctx->state < MFCINST_ABORT) {
 		/* This is run on CAPTURE (decode output) */
-		/* Width and height are set to the dimensions
-		   of the movie, the buffer is bigger and
-		   further processing stages should crop to this
-		   rectangle. */
+		/* Width and height are set to the dimensions of the buffer,
+		   The movie's dimensions may be smaller; the cropping rectangle
+		   required should be queried with VIDIOC_G_CROP. */
 		pix_mp->width = ctx->buf_width;
 		pix_mp->height = ctx->buf_height;
 		pix_mp->field = V4L2_FIELD_NONE;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 8b24829..4ad9349 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1599,7 +1599,57 @@ static int vidioc_g_parm(struct file *file, void *priv,
 		a->parm.output.timeperframe.numerator =
 					ctx->enc_params.rc_framerate_denom;
 	} else {
-		mfc_err("Setting FPS is only possible for the output queue\n");
+		mfc_err("Getting FPS is only possible for the output queue\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int vidioc_g_crop(struct file *file, void *priv, struct v4l2_crop *a)
+{
+	struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
+	struct s5p_mfc_enc_params *p = &ctx->enc_params;
+
+	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+		a->c.left = p->crop_left_offset;
+		a->c.top = p->crop_top_offset;
+		a->c.width = ctx->img_width -
+			(p->crop_left_offset + p->crop_right_offset);
+		a->c.height = ctx->img_height -
+			(p->crop_top_offset + p->crop_bottom_offset);
+	} else {
+		mfc_err("Getting crop is only possible for the output queue\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int vidioc_s_crop(struct file *file, void *priv,
+			 const struct v4l2_crop *a)
+{
+	struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
+	struct s5p_mfc_enc_params *p = &ctx->enc_params;
+
+	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+		int left, right, top, bottom;
+		left = round_down(a->c.left, 16);
+		right = ctx->img_width - (left + a->c.width);
+		top = round_down(a->c.top, 16);
+		bottom = ctx->img_height - (top + a->c.height);
+		if (left > ctx->img_width)
+			left = ctx->img_width;
+		if (right < 0)
+			right = 0;
+		if (top > ctx->img_height)
+			top = ctx->img_height;
+		if (bottom < 0)
+			bottom = 0;
+		p->crop_left_offset = left;
+		p->crop_right_offset = right;
+		p->crop_top_offset = top;
+		p->crop_bottom_offset = bottom;
+	} else {
+		mfc_err("Setting crop is only possible for the output queue\n");
 		return -EINVAL;
 	}
 	return 0;
@@ -1679,6 +1729,8 @@ static const struct v4l2_ioctl_ops s5p_mfc_enc_ioctl_ops = {
 	.vidioc_streamoff = vidioc_streamoff,
 	.vidioc_s_parm = vidioc_s_parm,
 	.vidioc_g_parm = vidioc_g_parm,
+	.vidioc_g_crop = vidioc_g_crop,
+	.vidioc_s_crop = vidioc_s_crop,
 	.vidioc_encoder_cmd = vidioc_encoder_cmd,
 	.vidioc_subscribe_event = vidioc_subscribe_event,
 	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 5bf6efd..1bb487c 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -600,12 +600,16 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx)
 	/* height */
 	WRITEL(ctx->img_height, S5P_FIMV_E_FRAME_HEIGHT_V6); /* 16 align */
 
-	/* cropped width */
-	WRITEL(ctx->img_width, S5P_FIMV_E_CROPPED_FRAME_WIDTH_V6);
-	/* cropped height */
-	WRITEL(ctx->img_height, S5P_FIMV_E_CROPPED_FRAME_HEIGHT_V6);
-	/* cropped offset */
-	WRITEL(0x0, S5P_FIMV_E_FRAME_CROP_OFFSET_V6);
+	/* cropped width, pixels */
+	WRITEL(ctx->img_width - (p->crop_left_offset + p->crop_right_offset),
+		S5P_FIMV_E_CROPPED_FRAME_WIDTH_V6);
+	/* cropped height, pixels */
+	WRITEL(ctx->img_height - (p->crop_top_offset + p->crop_bottom_offset),
+		S5P_FIMV_E_CROPPED_FRAME_HEIGHT_V6);
+	/* cropped offset, macroblocks */
+	WRITEL(((p->crop_left_offset / 16) & 0x2FF) |
+		(((p->crop_top_offset / 16) & 0x2FF) << 10),
+		S5P_FIMV_E_FRAME_CROP_OFFSET_V6);
 
 	/* pictype : IDR period */
 	reg = 0;
-- 
1.8.4

Re: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[Hans Verkuil]

Hi John,

Thanks for the patches! It's nice to see fixes/improvements like this being upstreamed.

Unfortunately I have to NACK this patch, but fortunately it is not difficult to fix.

The main problem is that you use the wrong API: you need to use G/S_SELECTION instead
of G/S_CROP. S_CROP on an output video node doesn't crop, it composes. And if your
reaction is 'Huh?', then you're not alone. Which is why the selection API was added.

The selection API can crop and compose for both capture and output nodes, and it
does what you expect.

You need to implement TGT_CROP, TGT_CROP_DEFAULT and TGT_CROP_BOUNDS. The latter
two are read-only and just return the current format's width and height.

Applications that today use S_CROP to set the crop rectangle for this driver's output
need to be adapted to using the S_SELECTION API since S_CROP really doesn't do what
they think it does.

Regards,

	Hans

On 10/10/2013 01:49 AM, John Sheu wrote:
> Allow userspace to set the crop rect of the input image buffer to
> encode.
> 
> Signed-off-by: John Sheu <sheu@google.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  6 ++-
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |  7 ++--
>  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 54 ++++++++++++++++++++++++-
>  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 +++++---
>  4 files changed, 70 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index 6920b54..48f706f 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -428,8 +428,10 @@ struct s5p_mfc_vp8_enc_params {
>   * struct s5p_mfc_enc_params - general encoding parameters
>   */
>  struct s5p_mfc_enc_params {
> -	u16 width;
> -	u16 height;
> +	u16 crop_left_offset;
> +	u16 crop_right_offset;
> +	u16 crop_top_offset;
> +	u16 crop_bottom_offset;
>  
>  	u16 gop_size;
>  	enum v4l2_mpeg_video_multi_slice_mode slice_mode;
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 8faf969..e99bcb8 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -334,10 +334,9 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	    ctx->state >= MFCINST_HEAD_PARSED &&
>  	    ctx->state < MFCINST_ABORT) {
>  		/* This is run on CAPTURE (decode output) */
> -		/* Width and height are set to the dimensions
> -		   of the movie, the buffer is bigger and
> -		   further processing stages should crop to this
> -		   rectangle. */
> +		/* Width and height are set to the dimensions of the buffer,
> +		   The movie's dimensions may be smaller; the cropping rectangle
> +		   required should be queried with VIDIOC_G_CROP. */
>  		pix_mp->width = ctx->buf_width;
>  		pix_mp->height = ctx->buf_height;
>  		pix_mp->field = V4L2_FIELD_NONE;
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> index 8b24829..4ad9349 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> @@ -1599,7 +1599,57 @@ static int vidioc_g_parm(struct file *file, void *priv,
>  		a->parm.output.timeperframe.numerator =
>  					ctx->enc_params.rc_framerate_denom;
>  	} else {
> -		mfc_err("Setting FPS is only possible for the output queue\n");
> +		mfc_err("Getting FPS is only possible for the output queue\n");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int vidioc_g_crop(struct file *file, void *priv, struct v4l2_crop *a)
> +{
> +	struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
> +	struct s5p_mfc_enc_params *p = &ctx->enc_params;
> +
> +	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +		a->c.left = p->crop_left_offset;
> +		a->c.top = p->crop_top_offset;
> +		a->c.width = ctx->img_width -
> +			(p->crop_left_offset + p->crop_right_offset);
> +		a->c.height = ctx->img_height -
> +			(p->crop_top_offset + p->crop_bottom_offset);
> +	} else {
> +		mfc_err("Getting crop is only possible for the output queue\n");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int vidioc_s_crop(struct file *file, void *priv,
> +			 const struct v4l2_crop *a)
> +{
> +	struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
> +	struct s5p_mfc_enc_params *p = &ctx->enc_params;
> +
> +	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +		int left, right, top, bottom;
> +		left = round_down(a->c.left, 16);
> +		right = ctx->img_width - (left + a->c.width);
> +		top = round_down(a->c.top, 16);
> +		bottom = ctx->img_height - (top + a->c.height);
> +		if (left > ctx->img_width)
> +			left = ctx->img_width;
> +		if (right < 0)
> +			right = 0;
> +		if (top > ctx->img_height)
> +			top = ctx->img_height;
> +		if (bottom < 0)
> +			bottom = 0;
> +		p->crop_left_offset = left;
> +		p->crop_right_offset = right;
> +		p->crop_top_offset = top;
> +		p->crop_bottom_offset = bottom;
> +	} else {
> +		mfc_err("Setting crop is only possible for the output queue\n");
>  		return -EINVAL;
>  	}
>  	return 0;
> @@ -1679,6 +1729,8 @@ static const struct v4l2_ioctl_ops s5p_mfc_enc_ioctl_ops = {
>  	.vidioc_streamoff = vidioc_streamoff,
>  	.vidioc_s_parm = vidioc_s_parm,
>  	.vidioc_g_parm = vidioc_g_parm,
> +	.vidioc_g_crop = vidioc_g_crop,
> +	.vidioc_s_crop = vidioc_s_crop,
>  	.vidioc_encoder_cmd = vidioc_encoder_cmd,
>  	.vidioc_subscribe_event = vidioc_subscribe_event,
>  	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> index 5bf6efd..1bb487c 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -600,12 +600,16 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx)
>  	/* height */
>  	WRITEL(ctx->img_height, S5P_FIMV_E_FRAME_HEIGHT_V6); /* 16 align */
>  
> -	/* cropped width */
> -	WRITEL(ctx->img_width, S5P_FIMV_E_CROPPED_FRAME_WIDTH_V6);
> -	/* cropped height */
> -	WRITEL(ctx->img_height, S5P_FIMV_E_CROPPED_FRAME_HEIGHT_V6);
> -	/* cropped offset */
> -	WRITEL(0x0, S5P_FIMV_E_FRAME_CROP_OFFSET_V6);
> +	/* cropped width, pixels */
> +	WRITEL(ctx->img_width - (p->crop_left_offset + p->crop_right_offset),
> +		S5P_FIMV_E_CROPPED_FRAME_WIDTH_V6);
> +	/* cropped height, pixels */
> +	WRITEL(ctx->img_height - (p->crop_top_offset + p->crop_bottom_offset),
> +		S5P_FIMV_E_CROPPED_FRAME_HEIGHT_V6);
> +	/* cropped offset, macroblocks */
> +	WRITEL(((p->crop_left_offset / 16) & 0x2FF) |
> +		(((p->crop_top_offset / 16) & 0x2FF) << 10),
> +		S5P_FIMV_E_FRAME_CROP_OFFSET_V6);
>  
>  	/* pictype : IDR period */
>  	reg = 0;
> 

Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[John Sheu]

On Wed, Oct 9, 2013 at 11:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> The main problem is that you use the wrong API: you need to use G/S_SELECTION instead
> of G/S_CROP. S_CROP on an output video node doesn't crop, it composes. And if your
> reaction is 'Huh?', then you're not alone. Which is why the selection API was added.
>
> The selection API can crop and compose for both capture and output nodes, and it
> does what you expect.


Happy to fix up the patch.  I'll just need some clarification on the
terminology here.  So, as I understand it:

(I'll use "source"/"sink" to refer to the device's inputs/outputs,
since "output" collides with the V4L2 concept of an OUTPUT device or
OUTPUT queue).

In all cases, the crop boundary refers to the area in the source
image; for a CAPTURE device, this is the (presumably analog) sensor,
and for an OUTPUT device, this is the memory buffer.  My particular
case is a memory-to-memory device, with both CAPTURE and OUTPUT
queues.  In this case, {G,S}_CROP on either the CAPTURE or OUTPUT
queues should effect exactly the same operation: cropping on the
source image, i.e. whatever image buffer I'm providing to the OUTPUT
queue.

The addition of {G,S}_SELECTION is to allow this same operation,
except on the sink side this time.  So, {G,S}_SELECTION setting the
compose bounds on either the CAPTURE or OUTPUT queues should also
effect exactly the same operation; cropping on the sink image, i.e.
whatever memory buffer I'm providing to the CAPTURE queue.

Not sure what you mean by "S_CROP on an output video node doesn't
crop, it composes", though.

Thanks,
-John Sheu

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[Hans Verkuil]

On 10/12/2013 01:48 AM, John Sheu wrote:
> On Wed, Oct 9, 2013 at 11:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> The main problem is that you use the wrong API: you need to use G/S_SELECTION instead
>> of G/S_CROP. S_CROP on an output video node doesn't crop, it composes. And if your
>> reaction is 'Huh?', then you're not alone. Which is why the selection API was added.
>>
>> The selection API can crop and compose for both capture and output nodes, and it
>> does what you expect.
> 
> 
> Happy to fix up the patch.  I'll just need some clarification on the
> terminology here.  So, as I understand it:
> 
> (I'll use "source"/"sink" to refer to the device's inputs/outputs,
> since "output" collides with the V4L2 concept of an OUTPUT device or
> OUTPUT queue).
> 
> In all cases, the crop boundary refers to the area in the source
> image; for a CAPTURE device, this is the (presumably analog) sensor,

Correct.

> and for an OUTPUT device, this is the memory buffer.

Correct.

> My particular
> case is a memory-to-memory device, with both CAPTURE and OUTPUT
> queues.  In this case, {G,S}_CROP on either the CAPTURE or OUTPUT
> queues should effect exactly the same operation: cropping on the
> source image, i.e. whatever image buffer I'm providing to the OUTPUT
> queue.

Incorrect.

S_CROP on an OUTPUT queue does the inverse: it refers to the area in
the sink image.

When the S_CROP API was designed a long time ago output devices were
very rare. Those that existed would output video to a S-Video or Composite
connector and often could scale the source image down to a particular
rectangle on the screen with the area outside of that rectangle either
having a fixed color or being a graphical picture.

The rectangle for the video on the output image was set by S_CROP, thus
effectively composing instead of cropping.

As the spec says (http://hverkuil.home.xs4all.nl/spec/media.html#crop):

"On a video output device the source are the images passed in by the
 application, and their size is again negotiated with the VIDIOC_G/S_FMT ioctls,
 or may be encoded in a compressed video stream. The target is the video signal,
 and the cropping ioctls determine the area where the images are inserted."

This was always confusing, but since there were so few output drivers it
was never that important. But when we started seeing many more output and
m2m drivers this became a real issue, in particular since S_CROP did only
half the operation (cropping for capture, composing for output) and was
missing composing for capture and "real" cropping for output.

> 
> The addition of {G,S}_SELECTION is to allow this same operation,
> except on the sink side this time.

No, it adds the compose operation for capture and the crop operation for
output, and it uses the terms 'cropping' and 'composing' correctly
without the inversion that S_CROP introduced on the output side.

> So, {G,S}_SELECTION setting the
> compose bounds on either the CAPTURE or OUTPUT queues should also
> effect exactly the same operation; cropping on the sink image, i.e.
> whatever memory buffer I'm providing to the CAPTURE queue.
> 
> Not sure what you mean by "S_CROP on an output video node doesn't
> crop, it composes", though.

I hope I explained it better this time.

Bottom line: S_CROP for capture is equivalent to S_SELECTION(V4L2_SEL_TGT_CROP).
S_CROP for output is equivalent to S_SELECTION(V4L2_SEL_TGT_COMPOSE).

Highly counter-intuitive, blame the original designers of the V4L2 API.

Regards,

	Hans

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[John Sheu]

I thought you were not making sense for a bit.  Then I walked away,
came back, and I think you're making sense now.  So:

* Crop always refers to the source image
* Compose always refers to the destination image

On Sat, Oct 12, 2013 at 1:00 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 10/12/2013 01:48 AM, John Sheu wrote:
>> On Wed, Oct 9, 2013 at 11:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> In all cases, the crop boundary refers to the area in the source
>> image; for a CAPTURE device, this is the (presumably analog) sensor,
>
> Correct.
>
>> and for an OUTPUT device, this is the memory buffer.
>
> Correct.

Here you are referring to the crop boundary, which is _not_ always
what {G,S}_CROP refers to.  (Confusing part).  {G,S}_CROP refers to
the crop boundary only for a CAPTURE queue.

>> My particular
>> case is a memory-to-memory device, with both CAPTURE and OUTPUT
>> queues.  In this case, {G,S}_CROP on either the CAPTURE or OUTPUT
>> queues should effect exactly the same operation: cropping on the
>> source image, i.e. whatever image buffer I'm providing to the OUTPUT
>> queue.
>
> Incorrect.
>
> S_CROP on an OUTPUT queue does the inverse: it refers to the area in
> the sink image.

This confused me for a bit (seeming contradiction with the above),
until I realized that you're referring to the S_CROP ioctl here, which
is _not_ the "crop boundary"; on an OUTPUT queue it refers to the
compose boundary.

> No, it adds the compose operation for capture and the crop operation for
> output, and it uses the terms 'cropping' and 'composing' correctly
> without the inversion that S_CROP introduced on the output side.
>
> Bottom line: S_CROP for capture is equivalent to S_SELECTION(V4L2_SEL_TGT_CROP).
> S_CROP for output is equivalent to S_SELECTION(V4L2_SEL_TGT_COMPOSE).

So yes.  By adding the {G,S}_SELECTION ioctls we can now refer to the
compose boundary for CAPTURE, and crop boundary for OUTPUT.


Now, here's a question.  It seems that for a mem2mem device, since
{G,S}_CROP on the CAPTURE queue covers the crop boundary, and
{G,S}_CROP on the OUTPUT queue capture the compose boundary, is there
any missing functionality that {G,S}_SELECTION is covering here.  In
other words: for a mem2mem device, the crop and compose boundaries
should be identical for the CAPTURE and OUTPUT queues?

-John Sheu

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[Tomasz Stanislawski]

Hello John,

I am a designer of original selection API. Maybe I could clarify what
does what in VIDIOC_S_CROP ioctl.

> I thought you were not making sense for a bit.  Then I walked away,
> came back, and I think you're making sense now.  So:
>
> * Crop always refers to the source image
> * Compose always refers to the destination image
>

Not exactly. Look below.

There are three basic cases in V4L2 that deal with crop/compose features.



CASE 1. Capture devices like sensors



There is only queue, the capture queue.

The only valid buffer types are V4L2_BUF_TYPE_VIDEO_CAPTURE
or V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE.

OLD API:

The operation
	ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE } )

selects an area on a sensor array that is processed by a device.
The memory buffers is filled totally with data produced by the device.

It is NOT possible to partially fill a buffer.


In selection API the old API call:
	ioctl(capture, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE })
becomes
	ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
		.target = V4L2_SEL_TGT_CROP })


Setting a area in the memory buffer (not supported in old API) is done by:
	ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
		.target = V4L2_SEL_TGT_COMPOSE })



CASE 2. Output devices like TV encoders.




There is only output queue.

The only valid buffer types are V4L2_BUF_TYPE_VIDEO_OUTPUT
or V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE.

OLD API:

The operation
	ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT } )

selects an area on a display where buffer's content is inserted.
So technically VIDIOC_S_CROP is used to configure composing.
It was a quick and effective hack to utilize good old VIDIOC_S_CROP
for a new type of device in V4L2, the output device.

It is NOT possible to chose which part of a memory buffer is going to be
displayed. One has to insert the whole buffer.

With selection API old API call becomes
	ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT,
		.target = V4L2_SEL_TGT_COMPOSE })


Setting a area in the memory buffer (not supported in old API) is done by:
	ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT,
		.target = V4L2_SEL_TGT_CROP })




CASE 3. The memory-to-memory devices




There are two queues capture and output.
The only valid buffer types are V4L2_BUF_TYPE_VIDEO_OUTPUT
or V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE or V4L2_BUF_TYPE_VIDEO_CAPTURE
or V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE.

OLD API:

The operation
	ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE } )
selects an area in a destination buffer that is filled by a device.
Notice that it is something completely different from definition
of VIDIOC_S_CROP on only-capture devices.
In case of M2M, crop means composing actually.

The operation
	ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT } )
selects an area in source buffer that is processed by hardware. So it is
actually cropping contradictory to what VIDIOC_S_CROP does on only-output devices.

All describe non-consistencies seams to a result of some
misunderstanding that happened in early days of m2m API.
Currently, there are applications use M2M devices and
assume the mentioned behavior. I am afraid that it is now
an unintentional part V4L2 API :(.

That is why I strongly recommend to migrate to selection API.
In selection API
	ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT } )
becomes
	ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT,
		.target = V4L2_SEL_TGT_CROP })

Moreover, the old API call
	ioctl(fd, VIDIOC_S_CROP, struct v4l2_crop { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE } )
becomes
	ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
		.target = V4L2_SEL_TGT_COMPOSE })

So in all cases cropping is cropping, composing is composing.
The other advantage of selection API are constraint flags that can be used co control
for policy of rounding rectangle's coordinates.
Moreover, the selection API provides additional methods to extract
bounds for rectangles, default areas, and padding areas.

It is important to know that currently the behavior of two operation:
	ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
		.target = V4L2_SEL_TGT_CROP })
	ioctl(capture, VIDIOC_S_SELECTION, struct v4l2_selection { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT,
		.target = V4L2_SEL_TGT_COMPOSE })
is still undefined and application should not use them.


I hope you find this information useful.


Regards,
Tomasz Stanislawski


On 10/12/2013 11:08 AM, John Sheu wrote:

> On Sat, Oct 12, 2013 at 1:00 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 10/12/2013 01:48 AM, John Sheu wrote:
>>> On Wed, Oct 9, 2013 at 11:49 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> In all cases, the crop boundary refers to the area in the source
>>> image; for a CAPTURE device, this is the (presumably analog) sensor,
>>
>> Correct.
>>
>>> and for an OUTPUT device, this is the memory buffer.
>>
>> Correct.
> 
> Here you are referring to the crop boundary, which is _not_ always
> what {G,S}_CROP refers to.  (Confusing part).  {G,S}_CROP refers to
> the crop boundary only for a CAPTURE queue.
> 
>>> My particular
>>> case is a memory-to-memory device, with both CAPTURE and OUTPUT
>>> queues.  In this case, {G,S}_CROP on either the CAPTURE or OUTPUT
>>> queues should effect exactly the same operation: cropping on the
>>> source image, i.e. whatever image buffer I'm providing to the OUTPUT
>>> queue.
>>
>> Incorrect.
>>
>> S_CROP on an OUTPUT queue does the inverse: it refers to the area in
>> the sink image.
> 
> This confused me for a bit (seeming contradiction with the above),
> until I realized that you're referring to the S_CROP ioctl here, which
> is _not_ the "crop boundary"; on an OUTPUT queue it refers to the
> compose boundary.
> 
>> No, it adds the compose operation for capture and the crop operation for
>> output, and it uses the terms 'cropping' and 'composing' correctly
>> without the inversion that S_CROP introduced on the output side.
>>
>> Bottom line: S_CROP for capture is equivalent to S_SELECTION(V4L2_SEL_TGT_CROP).
>> S_CROP for output is equivalent to S_SELECTION(V4L2_SEL_TGT_COMPOSE).
> 
> So yes.  By adding the {G,S}_SELECTION ioctls we can now refer to the
> compose boundary for CAPTURE, and crop boundary for OUTPUT.
> 
> 
> Now, here's a question.  It seems that for a mem2mem device, since
> {G,S}_CROP on the CAPTURE queue covers the crop boundary, and
> {G,S}_CROP on the OUTPUT queue capture the compose boundary, is there
> any missing functionality that {G,S}_SELECTION is covering here.  In
> other words: for a mem2mem device, the crop and compose boundaries
> should be identical for the CAPTURE and OUTPUT queues?
> 
> -John Sheu
> 

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[John Sheu]

On Thu, Oct 17, 2013 at 8:27 AM, Tomasz Stanislawski
<t.stanislaws@samsung.com> wrote:
>
> Hello John,
>
> I am a designer of original selection API. Maybe I could clarify what
> does what in VIDIOC_S_CROP ioctl.
>
> <snip>
>

Sweet.  Thanks for spelling things out explicitly like this.  The fact
that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness"
when used in a m2m device is definitely all sorts of confusing.

I'll have an updated patch shortly.

-John Sheu

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[John Sheu]

On Thu, Oct 17, 2013 at 2:46 PM, John Sheu <sheu@google.com> wrote:
> Sweet.  Thanks for spelling things out explicitly like this.  The fact
> that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness"
> when used in a m2m device is definitely all sorts of confusing.

Just to double-check: this means that we have another bug.

In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop,
we "simulate" a G_CROP or S_CROP, if the entry point is not defined
for that device, by doing the appropriate S_SELECTION or G_SELECTION.
Unfortunately then, for M2M this is incorrect then.

Am I reading this right?

-John Sheu

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[Sylwester Nawrocki]

On 10/18/2013 12:25 AM, John Sheu wrote:
> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com>  wrote:
>> >  Sweet.  Thanks for spelling things out explicitly like this.  The fact
>> >  that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness"
>> >  when used in a m2m device is definitely all sorts of confusing.
>
> Just to double-check: this means that we have another bug.
>
> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop,
> we "simulate" a G_CROP or S_CROP, if the entry point is not defined
> for that device, by doing the appropriate S_SELECTION or G_SELECTION.
> Unfortunately then, for M2M this is incorrect then.
>
> Am I reading this right?

You are right, John. Firstly a clear specification needs to be written,
something along the lines of Tomasz's explanation in this thread, once
all agree to that the ioctl code should be corrected if needed.

It seems this [1] RFC is an answer exactly to your question.

Exact meaning of the selection ioctl is only part of the problem, also
interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec.

[1] http://www.spinics.net/lists/linux-media/msg56078.html

Regards,
Sylwester

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[John Sheu]

On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 10/18/2013 12:25 AM, John Sheu wrote:
>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com>  wrote:
>>> >  Sweet.  Thanks for spelling things out explicitly like this.  The fact
>>> >  that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness"
>>> >  when used in a m2m device is definitely all sorts of confusing.
>>
>> Just to double-check: this means that we have another bug.
>>
>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop,
>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined
>> for that device, by doing the appropriate S_SELECTION or G_SELECTION.
>> Unfortunately then, for M2M this is incorrect then.
>>
>> Am I reading this right?
>
> You are right, John. Firstly a clear specification needs to be written,
> something along the lines of Tomasz's explanation in this thread, once
> all agree to that the ioctl code should be corrected if needed.
>
> It seems this [1] RFC is an answer exactly to your question.
>
> Exact meaning of the selection ioctl is only part of the problem, also
> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec.
>
> [1] http://www.spinics.net/lists/linux-media/msg56078.html

I think the "inversion" behavior is confusing and we should remove it
if at all possible.

I took a look through all the drivers in linux-media which implement
S_CROP.  Most of them are either OUTPUT or CAPTURE/OVERLAY-only.  Of
those that aren't:

* drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both
  OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state.
  No functional difference.
* drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify
  the queue, but is a CAPTURE-only device.  Probably an (unrelated) bug.
* drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver
  with both OUTPUT and CAPTURE queues.  It has uninverted behavior:
  S_CROP(CAPTURE) -> source
  S_CROP(OUTPUT) -> destination
* drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both
  OUTPUT and CAPTURE queues.  It has inverted behavior:
  S_CROP(CAPTURE) -> destination
  S_CROP(OUTPUT) -> source

The last two points above are the most relevant.  So we already have
at least one broken driver, regardless of whether we allow inversion
or not; I'd think this grants us a certain freedom to redefine the
specification to be more logical.  Can we do this please?

-John Sheu

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[Hans Verkuil]

Hi John,

On 10/18/2013 02:03 AM, John Sheu wrote:
> On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki
> <sylvester.nawrocki@gmail.com> wrote:
>> On 10/18/2013 12:25 AM, John Sheu wrote:
>>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com>  wrote:
>>>>>  Sweet.  Thanks for spelling things out explicitly like this.  The fact
>>>>>  that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness"
>>>>>  when used in a m2m device is definitely all sorts of confusing.
>>>
>>> Just to double-check: this means that we have another bug.
>>>
>>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop,
>>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined
>>> for that device, by doing the appropriate S_SELECTION or G_SELECTION.
>>> Unfortunately then, for M2M this is incorrect then.
>>>
>>> Am I reading this right?
>>
>> You are right, John. Firstly a clear specification needs to be written,
>> something along the lines of Tomasz's explanation in this thread, once
>> all agree to that the ioctl code should be corrected if needed.

I don't understand the problem here. The specification has always been clear:
s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE).

Drivers should only implement the selection API and the v4l2 core will do the
correct translation of s_crop.

Yes, I know it's weird, but that's the way the crop API was defined way back
and that's what should be used.

My advise: forget about s_crop and just implement s_selection.

>>
>> It seems this [1] RFC is an answer exactly to your question.
>>
>> Exact meaning of the selection ioctl is only part of the problem, also
>> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec.
>>
>> [1] http://www.spinics.net/lists/linux-media/msg56078.html
> 
> I think the "inversion" behavior is confusing and we should remove it
> if at all possible.
> 
> I took a look through all the drivers in linux-media which implement
> S_CROP.  Most of them are either OUTPUT or CAPTURE/OVERLAY-only.  Of
> those that aren't:
> 
> * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both
>   OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state.
>   No functional difference.

Yeah, I guess that's a driver bug. This is a very old driver that originally
used a custom API for these things, and since no selection API existed at the
time it was just mapped to the crop API. Eventually it should use the selection
API as well and do it correctly. But to be honest, nobody cares about this driver :-)

It is however on my TODO list of drivers that need to be converted to the latest
frameworks, so I might fix this eventually.

> * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify
>   the queue, but is a CAPTURE-only device.  Probably an (unrelated) bug.

Yes, that's a driver bug. It should check the buffer type.

> * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver
>   with both OUTPUT and CAPTURE queues.  It has uninverted behavior:
>   S_CROP(CAPTURE) -> source
>   S_CROP(OUTPUT) -> destination

This is the wrong behavior.

> * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both
>   OUTPUT and CAPTURE queues.  It has inverted behavior:
>   S_CROP(CAPTURE) -> destination
>   S_CROP(OUTPUT) -> source

This is the correct behavior.

> 
> The last two points above are the most relevant.  So we already have
> at least one broken driver, regardless of whether we allow inversion
> or not; I'd think this grants us a certain freedom to redefine the
> specification to be more logical.  Can we do this please?

No. The fimc-m2m.c driver needs to be fixed. That's the broken one.

Regards,

	Hans

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[Sylwester Nawrocki]

Sorry, I missed to reply to this e-mail.

On 04/11/13 11:57, Hans Verkuil wrote:
> Hi John,
> 
> On 10/18/2013 02:03 AM, John Sheu wrote:
>> On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki
>> <sylvester.nawrocki@gmail.com> wrote:
>>> On 10/18/2013 12:25 AM, John Sheu wrote:
>>>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com>  wrote:
>>>>>>  Sweet.  Thanks for spelling things out explicitly like this.  The fact
>>>>>>  that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness"
>>>>>>  when used in a m2m device is definitely all sorts of confusing.
>>>>
>>>> Just to double-check: this means that we have another bug.
>>>>
>>>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop,
>>>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined
>>>> for that device, by doing the appropriate S_SELECTION or G_SELECTION.
>>>> Unfortunately then, for M2M this is incorrect then.
>>>>
>>>> Am I reading this right?
>>>
>>> You are right, John. Firstly a clear specification needs to be written,
>>> something along the lines of Tomasz's explanation in this thread, once
>>> all agree to that the ioctl code should be corrected if needed.
> 
> I don't understand the problem here. The specification has always been clear:
> s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE).
> 
> Drivers should only implement the selection API and the v4l2 core will do the
> correct translation of s_crop.
> 
> Yes, I know it's weird, but that's the way the crop API was defined way back
> and that's what should be used.
> 
> My advise: forget about s_crop and just implement s_selection.
> 
>>>
>>> It seems this [1] RFC is an answer exactly to your question.
>>>
>>> Exact meaning of the selection ioctl is only part of the problem, also
>>> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec.
>>>
>>> [1] http://www.spinics.net/lists/linux-media/msg56078.html
>>
>> I think the "inversion" behavior is confusing and we should remove it
>> if at all possible.
>>
>> I took a look through all the drivers in linux-media which implement
>> S_CROP.  Most of them are either OUTPUT or CAPTURE/OVERLAY-only.  Of
>> those that aren't:
>>
>> * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both
>>   OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state.
>>   No functional difference.
> 
> Yeah, I guess that's a driver bug. This is a very old driver that originally
> used a custom API for these things, and since no selection API existed at the
> time it was just mapped to the crop API. Eventually it should use the selection
> API as well and do it correctly. But to be honest, nobody cares about this driver :-)
> 
> It is however on my TODO list of drivers that need to be converted to the latest
> frameworks, so I might fix this eventually.
> 
>> * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify
>>   the queue, but is a CAPTURE-only device.  Probably an (unrelated) bug.
> 
> Yes, that's a driver bug. It should check the buffer type.
> 
>> * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver
>>   with both OUTPUT and CAPTURE queues.  It has uninverted behavior:
>>   S_CROP(CAPTURE) -> source
>>   S_CROP(OUTPUT) -> destination

No, that's not true. It seems you got it wrong, cropping in case of this m2m
driver works like this:

S_CROP(OUTPUT) -> source (from HW POV)
S_CROP(CAPTURE) -> destination

I.e. exactly same way as for s5p-g2d, for which it somehow was a reference
implementation.

> This is the wrong behavior.
> 
>> * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both
>>   OUTPUT and CAPTURE queues.  It has inverted behavior:
>>   S_CROP(CAPTURE) -> destination
>>   S_CROP(OUTPUT) -> source
> 
> This is the correct behavior.
> 
>>
>> The last two points above are the most relevant.  So we already have
>> at least one broken driver, regardless of whether we allow inversion
>> or not; I'd think this grants us a certain freedom to redefine the
>> specification to be more logical.  Can we do this please?
> 
> No. The fimc-m2m.c driver needs to be fixed. That's the broken one.

It's not broken, it's easy to figure out if you actually look at the
code, e.g. fimc_m2m_try_crop() function.

--
Regards,
Sylwester

-- 
Sylwester Nawrocki
Samsung R&D Institute Poland

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[Hans Verkuil]

On 11/04/2013 12:29 PM, Sylwester Nawrocki wrote:
> Sorry, I missed to reply to this e-mail.
> 
> On 04/11/13 11:57, Hans Verkuil wrote:
>> Hi John,
>>
>> On 10/18/2013 02:03 AM, John Sheu wrote:
>>> On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki
>>> <sylvester.nawrocki@gmail.com> wrote:
>>>> On 10/18/2013 12:25 AM, John Sheu wrote:
>>>>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@google.com>  wrote:
>>>>>>>  Sweet.  Thanks for spelling things out explicitly like this.  The fact
>>>>>>>  that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness"
>>>>>>>  when used in a m2m device is definitely all sorts of confusing.
>>>>>
>>>>> Just to double-check: this means that we have another bug.
>>>>>
>>>>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop,
>>>>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined
>>>>> for that device, by doing the appropriate S_SELECTION or G_SELECTION.
>>>>> Unfortunately then, for M2M this is incorrect then.
>>>>>
>>>>> Am I reading this right?
>>>>
>>>> You are right, John. Firstly a clear specification needs to be written,
>>>> something along the lines of Tomasz's explanation in this thread, once
>>>> all agree to that the ioctl code should be corrected if needed.
>>
>> I don't understand the problem here. The specification has always been clear:
>> s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE).
>>
>> Drivers should only implement the selection API and the v4l2 core will do the
>> correct translation of s_crop.
>>
>> Yes, I know it's weird, but that's the way the crop API was defined way back
>> and that's what should be used.
>>
>> My advise: forget about s_crop and just implement s_selection.
>>
>>>>
>>>> It seems this [1] RFC is an answer exactly to your question.
>>>>
>>>> Exact meaning of the selection ioctl is only part of the problem, also
>>>> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec.
>>>>
>>>> [1] http://www.spinics.net/lists/linux-media/msg56078.html
>>>
>>> I think the "inversion" behavior is confusing and we should remove it
>>> if at all possible.
>>>
>>> I took a look through all the drivers in linux-media which implement
>>> S_CROP.  Most of them are either OUTPUT or CAPTURE/OVERLAY-only.  Of
>>> those that aren't:
>>>
>>> * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both
>>>   OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state.
>>>   No functional difference.
>>
>> Yeah, I guess that's a driver bug. This is a very old driver that originally
>> used a custom API for these things, and since no selection API existed at the
>> time it was just mapped to the crop API. Eventually it should use the selection
>> API as well and do it correctly. But to be honest, nobody cares about this driver :-)
>>
>> It is however on my TODO list of drivers that need to be converted to the latest
>> frameworks, so I might fix this eventually.
>>
>>> * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify
>>>   the queue, but is a CAPTURE-only device.  Probably an (unrelated) bug.
>>
>> Yes, that's a driver bug. It should check the buffer type.
>>
>>> * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver
>>>   with both OUTPUT and CAPTURE queues.  It has uninverted behavior:
>>>   S_CROP(CAPTURE) -> source
>>>   S_CROP(OUTPUT) -> destination
> 
> No, that's not true. It seems you got it wrong, cropping in case of this m2m
> driver works like this:
> 
> S_CROP(OUTPUT) -> source (from HW POV)
> S_CROP(CAPTURE) -> destination
> 
> I.e. exactly same way as for s5p-g2d, for which it somehow was a reference
> implementation.
> 
>> This is the wrong behavior.
>>
>>> * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both
>>>   OUTPUT and CAPTURE queues.  It has inverted behavior:
>>>   S_CROP(CAPTURE) -> destination
>>>   S_CROP(OUTPUT) -> source
>>
>> This is the correct behavior.
>>
>>>
>>> The last two points above are the most relevant.  So we already have
>>> at least one broken driver, regardless of whether we allow inversion
>>> or not; I'd think this grants us a certain freedom to redefine the
>>> specification to be more logical.  Can we do this please?
>>
>> No. The fimc-m2m.c driver needs to be fixed. That's the broken one.
> 
> It's not broken, it's easy to figure out if you actually look at the
> code, e.g. fimc_m2m_try_crop() function.

I did look at the code, but it is quite possible I got confused.

Let me be precise as to what should happen, and you can check whether that's
what is actually done in the fimc and g2d drivers.

For V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:

Say that the mem2mem hardware creates a 640x480 picture. If VIDIOC_S_CROP was
called for V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE with a rectangle of 320x240@160x120,
then the DMA engine will only transfer the center 320x240 rectangle to memory.
This means that S_FMT needs to provide a buffer size large enough to accomodate
a 320x240 image.

So: VIDIOC_S_CROP(CAPTURE) == S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP).


For V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:

Say that the image in memory is a 640x480 picture. If VIDIOC_S_CROP was called
for V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE with a rectangle of 320x240@160x120 then
this would mean that the full 640x480 image is DMAed to the hardware, is scaled
down to 320x240 and composed at position (160x120) in a canvas of at least 480x360.

In other words, S_CROP behaves as composition for output devices:
VIDIOC_S_CROP(OUTPUT) == S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE).

The last operation in particular is almost certainly not what you want for
m2m devices. Instead, you want to select (crop) part of the image in memory and
DMA that to the device. This is S_SELECTION(OUTPUT, V4L2_SEL_TGT_CROP) and cannot
be translated to an S_CROP ioctl.

What's more: in order to implement S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE) you
would need some way of setting the 'canvas' size of the m2m device, and there is
no API today to do this (this was discussed during the v4l/dvb mini-summit).

Regarding the capture side of an m2m device: it is not clear to me what these
drivers implement: S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP) or S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE).

If it is the latter, then again S_CROP cannot be used and you have to use S_SELECTION.

Regards,

	Hans

Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder

[Sylwester Nawrocki]

Hi Hans,

On 11/04/2013 01:07 PM, Hans Verkuil wrote:
> Let me be precise as to what should happen, and you can check whether that's
> what is actually done in the fimc and g2d drivers.
>
> For V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>
> Say that the mem2mem hardware creates a 640x480 picture. If VIDIOC_S_CROP was
> called for V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE with a rectangle of 320x240@160x120,
> then the DMA engine will only transfer the center 320x240 rectangle to memory.
> This means that S_FMT needs to provide a buffer size large enough to accomodate
> a 320x240 image.
>
> So: VIDIOC_S_CROP(CAPTURE) == S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP).

Unfortunately it's not how it currently works at these drivers. For 
VIDIOC_S_CROP
called with V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE and a rectangle of 
320x240@160x120
the hardware would scale and compose full 640x480 image onto 320x240 
rectangle
in the output memory buffer at position 160x120.
IIRC the g2d device cannot scale so it would not allow to select DMA output
rectangle smaller than 640x480. But looking at the code it doesn't do 
any crop
parameters validation...

So VIDIOC_S_CROP(CAPTURE) is actually being abused on m2m as 
S_SELECTION(CAPTURE,
V4L2_SEL_TGT_COMPOSE).

> For V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>
> Say that the image in memory is a 640x480 picture. If VIDIOC_S_CROP was called
> for V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE with a rectangle of 320x240@160x120 then
> this would mean that the full 640x480 image is DMAed to the hardware, is scaled
> down to 320x240 and composed at position (160x120) in a canvas of at least 480x360.
>
> In other words, S_CROP behaves as composition for output devices:
> VIDIOC_S_CROP(OUTPUT) == S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE).

No, in case of these devices VIDIOC_S_CROP(OUTPUT) does what it actually 
means -
the DMA would read only 320x240 rectangle at position 160x120.

> The last operation in particular is almost certainly not what you want for
> m2m devices. Instead, you want to select (crop) part of the image in memory and
> DMA that to the device. This is S_SELECTION(OUTPUT, V4L2_SEL_TGT_CROP) and cannot
> be translated to an S_CROP ioctl.

Yeah, I didn't come yet across a video mem-to-mem hardware that would 
support steps
as in your first description of crop on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE.
S_SELECTION(OUTPUT, V4L2_SEL_TGT_CROP) seems to have been redefined on 
mem-to-mem
devices to do what it actually says. But it's not written anywhere in 
the spec
yet, so I guess we could keep the crop ioctls in those drivers, in order 
to not
break existing user space, and implement the selection API ioctls after 
documenting
its semantics for mem-to-mem devices.

> What's more: in order to implement S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE) you
> would need some way of setting the 'canvas' size of the m2m device, and there is
> no API today to do this (this was discussed during the v4l/dvb mini-summit).
>
> Regarding the capture side of an m2m device: it is not clear to me what these
> drivers implement: S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP) or
>S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE).
>
> If it is the latter, then again S_CROP cannot be used and you have to use
>S_SELECTION.

It's equivalent of S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE). Note that 
the
fimc and mfc drivers were written long before the selection API was 
introduced.

Presumably the crop ioctls should just be deprecated (however handlers 
left for
backward compatibility) in those drivers, once there is complete 
definition of
the selections API for the m2m video devices.
It's probably worth to avoid adding any translation layer of such behaviour
(which doesn't match your definitions above) to the v4l2-core.

--
Regards,
Sylwester

[PATCH 4/6] [media] s5p-mfc: support dynamic encoding parameter changes

[John Sheu]

Add support for dynamic encoding parameter changes in MFCv6.  Parameters
set are applied with the next OUTPUT buffer queued to the device with
VIDIOC_QBUF.

Supported parameters are:

* GOP size (V4L2_CID_MPEG_VIDEO_GOP_SIZE)
* framerate (from VIDIOC_S_PARM)
* VBR target bitrate (V4L2_CID_MPEG_VIDEO_BITRATE)
* (h264) frame type (V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE)

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/media/platform/s5p-mfc/regs-mfc-v6.h    |  4 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 32 +++++++--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 53 +++++++++++---
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c | 29 ++++----
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 94 ++++++++++++++-----------
 5 files changed, 140 insertions(+), 72 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/regs-mfc-v6.h b/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
index 2398cdf..495ed21 100644
--- a/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
+++ b/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
@@ -330,6 +330,10 @@
 #define S5P_FIMV_E_MVC_RC_RPARA_VIEW1_V6		0xfd50
 #define S5P_FIMV_E_MVC_INTER_VIEW_PREDICTION_ON_V6	0xfd80
 
+#define S5P_FIMV_E_GOP_CONFIG_CHANGE_SHIFT_V6		0
+#define S5P_FIMV_E_FRAME_RATE_CHANGE_SHIFT_V6		1
+#define S5P_FIMV_E_BIT_RATE_CHANGE_SHIFT_V6		2
+
 /* Codec numbers  */
 #define S5P_FIMV_CODEC_NONE_V6		-1
 
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 48f706f..af134fd 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -173,12 +173,35 @@ enum s5p_mfc_decode_arg {
 	MFC_DEC_RES_CHANGE,
 };
 
+
+/**
+ * enum s5p_mfc_encoder_param_change - indicates runtime parameter change
+ */
+enum s5p_mfc_encode_param_change {
+	MFC_ENC_GOP_CONFIG_CHANGE,
+	MFC_ENC_FRAME_RATE_CHANGE,
+	MFC_ENC_BIT_RATE_CHANGE,
+	MFC_ENC_FRAME_INSERTION,
+};
+
 #define MFC_BUF_FLAG_USED	(1 << 0)
 #define MFC_BUF_FLAG_EOS	(1 << 1)
 
 struct s5p_mfc_ctx;
 
 /**
+ * struct s5p_mfc_enc_params - runtime modifiable encoding parameters
+ */
+struct s5p_mfc_runtime_enc_params {
+	u32 params_changed;
+	u16 gop_size;
+	u32 rc_framerate_num;
+	u32 rc_framerate_denom;
+	u32 rc_bitrate;
+	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
+};
+
+/**
  * struct s5p_mfc_buf - MFC buffer
  */
 struct s5p_mfc_buf {
@@ -192,6 +215,7 @@ struct s5p_mfc_buf {
 		size_t stream;
 	} cookie;
 	int flags;
+	struct s5p_mfc_runtime_enc_params runtime_enc_params;
 };
 
 /**
@@ -433,7 +457,6 @@ struct s5p_mfc_enc_params {
 	u16 crop_top_offset;
 	u16 crop_bottom_offset;
 
-	u16 gop_size;
 	enum v4l2_mpeg_video_multi_slice_mode slice_mode;
 	u16 slice_mb;
 	u32 slice_bit;
@@ -444,7 +467,6 @@ struct s5p_mfc_enc_params {
 	u8 pad_cr;
 	int rc_frame;
 	int rc_mb;
-	u32 rc_bitrate;
 	u16 rc_reaction_coeff;
 	u16 vbv_size;
 	u32 vbv_delay;
@@ -454,15 +476,13 @@ struct s5p_mfc_enc_params {
 	int fixed_target_bit;
 
 	u8 num_b_frame;
-	u32 rc_framerate_num;
-	u32 rc_framerate_denom;
 
 	struct {
+		struct s5p_mfc_runtime_enc_params runtime;
 		struct s5p_mfc_h264_enc_params h264;
 		struct s5p_mfc_mpeg4_enc_params mpeg4;
 		struct s5p_mfc_vp8_enc_params vp8;
 	} codec;
-
 };
 
 /**
@@ -638,8 +658,6 @@ struct s5p_mfc_ctx {
 	size_t me_buffer_size;
 	size_t tmv_buffer_size;
 
-	enum v4l2_mpeg_mfc51_video_force_frame_type force_frame_type;
-
 	struct list_head ref_queue;
 	unsigned int ref_queue_cnt;
 
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 4ad9349..0898dee 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1342,7 +1342,9 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
-		p->gop_size = ctrl->val;
+		p->codec.runtime.params_changed |=
+			(1 << MFC_ENC_GOP_CONFIG_CHANGE);
+		p->codec.runtime.gop_size = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
 		p->slice_mode = ctrl->val;
@@ -1368,13 +1370,17 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl)
 		p->rc_frame = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_VIDEO_BITRATE:
-		p->rc_bitrate = ctrl->val;
+		p->codec.runtime.params_changed |=
+			(1 << MFC_ENC_BIT_RATE_CHANGE);
+		p->codec.runtime.rc_bitrate = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_MFC51_VIDEO_RC_REACTION_COEFF:
 		p->rc_reaction_coeff = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE:
-		ctx->force_frame_type = ctrl->val;
+		p->codec.runtime.params_changed |=
+			(1 << MFC_ENC_FRAME_INSERTION);
+		p->codec.runtime.force_frame_type = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_VIDEO_VBV_SIZE:
 		p->vbv_size = ctrl->val;
@@ -1575,12 +1581,29 @@ static int vidioc_s_parm(struct file *file, void *priv,
 			 struct v4l2_streamparm *a)
 {
 	struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
+	struct s5p_mfc_runtime_enc_params *runtime_params =
+		&ctx->enc_params.codec.runtime;
 
+	/*
+	 * Note that MFC hardware specifies framerate but the V4L2 API specifies
+	 * timeperframe. Take the reciprocal of one to get the other.
+	 */
 	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
-		ctx->enc_params.rc_framerate_num =
-					a->parm.output.timeperframe.denominator;
-		ctx->enc_params.rc_framerate_denom =
-					a->parm.output.timeperframe.numerator;
+		if ((runtime_params->rc_framerate_num !=
+			a->parm.output.timeperframe.denominator) ||
+			(runtime_params->rc_framerate_denom !=
+			a->parm.output.timeperframe.numerator)) {
+			if (a->parm.output.timeperframe.numerator == 0) {
+				mfc_err("Cannot set an infinite FPS\n");
+				return -EINVAL;
+			}
+			runtime_params->params_changed |=
+				(1 << MFC_ENC_FRAME_RATE_CHANGE);
+			runtime_params->rc_framerate_num =
+				a->parm.output.timeperframe.denominator;
+			runtime_params->rc_framerate_denom =
+				a->parm.output.timeperframe.numerator;
+		}
 	} else {
 		mfc_err("Setting FPS is only possible for the output queue\n");
 		return -EINVAL;
@@ -1595,9 +1618,9 @@ static int vidioc_g_parm(struct file *file, void *priv,
 
 	if (a->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
 		a->parm.output.timeperframe.denominator =
-					ctx->enc_params.rc_framerate_num;
+			ctx->enc_params.codec.runtime.rc_framerate_num;
 		a->parm.output.timeperframe.numerator =
-					ctx->enc_params.rc_framerate_denom;
+			ctx->enc_params.codec.runtime.rc_framerate_denom;
 	} else {
 		mfc_err("Getting FPS is only possible for the output queue\n");
 		return -EINVAL;
@@ -1983,7 +2006,19 @@ static void s5p_mfc_buf_queue(struct vb2_buffer *vb)
 		ctx->dst_queue_cnt++;
 		spin_unlock_irqrestore(&dev->irqlock, flags);
 	} else if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
+		struct s5p_mfc_runtime_enc_params *runtime_params =
+			&ctx->enc_params.codec.runtime;
 		mfc_buf = &ctx->src_bufs[vb->v4l2_buf.index];
+		/* Tag buffer with runtime encoding parameters */
+		memcpy(&mfc_buf->runtime_enc_params, runtime_params,
+			sizeof(mfc_buf->runtime_enc_params));
+		runtime_params->params_changed = 0;
+		/* force_frame_type needs to revert to 0 after being sent. */
+		if (runtime_params->force_frame_type != 0) {
+			v4l2_ctrl_s_ctrl(v4l2_ctrl_find(&ctx->ctrl_handler,
+				V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE),
+				V4L2_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE_DISABLED);
+		}
 		mfc_buf->flags &= ~MFC_BUF_FLAG_USED;
 		spin_lock_irqsave(&dev->irqlock, flags);
 		list_add_tail(&mfc_buf->list, &ctx->src_queue);
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
index 368582b..428cbb8 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
@@ -688,7 +688,7 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx)
 	reg = mfc_read(dev, S5P_FIMV_ENC_PIC_TYPE_CTRL);
 	reg |= (1 << 18);
 	reg &= ~(0xFFFF);
-	reg |= p->gop_size;
+	reg |= p->codec.runtime.gop_size;
 	mfc_write(dev, reg, S5P_FIMV_ENC_PIC_TYPE_CTRL);
 	mfc_write(dev, 0, S5P_FIMV_ENC_B_RECON_WRITE_ON);
 	/* multi-slice control */
@@ -736,7 +736,7 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx)
 	mfc_write(dev, reg, S5P_FIMV_ENC_RC_CONFIG);
 	/* bit rate */
 	if (p->rc_frame)
-		mfc_write(dev, p->rc_bitrate,
+		mfc_write(dev, p->codec.runtime.rc_bitrate,
 			S5P_FIMV_ENC_RC_BIT_RATE);
 	else
 		mfc_write(dev, 0, S5P_FIMV_ENC_RC_BIT_RATE);
@@ -831,9 +831,10 @@ static int s5p_mfc_set_enc_params_h264(struct s5p_mfc_ctx *ctx)
 	reg |= p_264->rc_frame_qp;
 	mfc_write(dev, reg, S5P_FIMV_ENC_RC_CONFIG);
 	/* frame rate */
-	if (p->rc_frame && p->rc_framerate_denom)
-		mfc_write(dev, p->rc_framerate_num * 1000
-			/ p->rc_framerate_denom, S5P_FIMV_ENC_RC_FRAME_RATE);
+	if (p->rc_frame && p->codec.runtime.rc_framerate_denom)
+		mfc_write(dev, p->codec.runtime.rc_framerate_num * 1000
+			/ p->codec.runtime.rc_framerate_denom,
+			S5P_FIMV_ENC_RC_FRAME_RATE);
 	else
 		mfc_write(dev, 0, S5P_FIMV_ENC_RC_FRAME_RATE);
 	/* max & min value of QP */
@@ -950,16 +951,17 @@ static int s5p_mfc_set_enc_params_mpeg4(struct s5p_mfc_ctx *ctx)
 	}
 	/* frame rate */
 	if (p->rc_frame) {
-		if (p->rc_framerate_denom > 0) {
-			framerate = p->rc_framerate_num * 1000 /
-						p->rc_framerate_denom;
+		if (p->codec.runtime.rc_framerate_denom > 0) {
+			framerate = p->codec.runtime.rc_framerate_num * 1000 /
+				p->codec.runtime.rc_framerate_denom;
 			mfc_write(dev, framerate,
 				S5P_FIMV_ENC_RC_FRAME_RATE);
 			shm = s5p_mfc_read_info_v5(ctx, RC_VOP_TIMING);
 			shm &= ~(0xFFFFFFFF);
 			shm |= (1 << 31);
-			shm |= ((p->rc_framerate_num & 0x7FFF) << 16);
-			shm |= (p->rc_framerate_denom & 0xFFFF);
+			shm |= ((p->codec.runtime.rc_framerate_num & 0x7FFF)
+				<< 16);
+			shm |= (p->codec.runtime.rc_framerate_denom & 0xFFFF);
 			s5p_mfc_write_info_v5(ctx, shm, RC_VOP_TIMING);
 		}
 	} else {
@@ -1009,9 +1011,10 @@ static int s5p_mfc_set_enc_params_h263(struct s5p_mfc_ctx *ctx)
 		s5p_mfc_write_info_v5(ctx, shm, P_B_FRAME_QP);
 	}
 	/* frame rate */
-	if (p->rc_frame && p->rc_framerate_denom)
-		mfc_write(dev, p->rc_framerate_num * 1000
-			/ p->rc_framerate_denom, S5P_FIMV_ENC_RC_FRAME_RATE);
+	if (p->rc_frame && p->codec.runtime.rc_framerate_denom)
+		mfc_write(dev, p->codec.runtime.rc_framerate_num * 1000
+		/ p->codec.runtime.rc_framerate_denom,
+		S5P_FIMV_ENC_RC_FRAME_RATE);
 	else
 		mfc_write(dev, 0, S5P_FIMV_ENC_RC_FRAME_RATE);
 	/* rate control config. */
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 1bb487c..4b82338 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -587,6 +587,52 @@ static int s5p_mfc_set_slice_mode(struct s5p_mfc_ctx *ctx)
 	return 0;
 }
 
+static int s5p_mfc_set_runtime_enc_params(struct s5p_mfc_ctx *ctx,
+		struct s5p_mfc_runtime_enc_params *runtime_p)
+{
+	struct s5p_mfc_dev *dev = ctx->dev;
+	struct s5p_mfc_enc_params *p = &ctx->enc_params;
+	unsigned int params_changed = 0;
+
+	if (runtime_p->params_changed & (1 << MFC_ENC_GOP_CONFIG_CHANGE)) {
+		params_changed |= (1 << S5P_FIMV_E_GOP_CONFIG_CHANGE_SHIFT_V6);
+		/* pictype: IDR period */
+		WRITEL((runtime_p->gop_size & 0xFFFF),
+			S5P_FIMV_E_GOP_CONFIG_V6);
+	}
+	if (runtime_p->params_changed & (1 << MFC_ENC_FRAME_RATE_CHANGE)) {
+		/* frame rate */
+		if (p->rc_frame && runtime_p->rc_framerate_num &&
+			runtime_p->rc_framerate_denom) {
+			params_changed |=
+				(1 << S5P_FIMV_E_FRAME_RATE_CHANGE_SHIFT_V6);
+			WRITEL((((runtime_p->rc_framerate_num & 0xFFFF) << 16) |
+				(runtime_p->rc_framerate_denom & 0xFFFF)),
+				S5P_FIMV_E_RC_FRAME_RATE_V6);
+		}
+	}
+	if (runtime_p->params_changed & (1 << MFC_ENC_BIT_RATE_CHANGE)) {
+		/* bit rate */
+		if (p->rc_frame) {
+			params_changed |=
+				(1 << S5P_FIMV_E_BIT_RATE_CHANGE_SHIFT_V6);
+			WRITEL(runtime_p->rc_bitrate,
+				S5P_FIMV_E_RC_BIT_RATE_V6);
+		}
+	}
+	if (runtime_p->params_changed & (1 << MFC_ENC_FRAME_INSERTION)) {
+		unsigned int reg = READL(S5P_FIMV_E_FRAME_INSERTION_V6);
+		reg &= ~0x3;
+		reg |= runtime_p->force_frame_type & 0x3;
+		WRITEL(reg, S5P_FIMV_E_FRAME_INSERTION_V6);
+	}
+
+	if (params_changed)
+		WRITEL(params_changed, S5P_FIMV_E_PARAM_CHANGE_V6);
+
+	return 0;
+}
+
 static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx)
 {
 	struct s5p_mfc_dev *dev = ctx->dev;
@@ -611,10 +657,10 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx)
 		(((p->crop_top_offset / 16) & 0x2FF) << 10),
 		S5P_FIMV_E_FRAME_CROP_OFFSET_V6);
 
-	/* pictype : IDR period */
-	reg = 0;
-	reg |= p->gop_size & 0xFFFF;
-	WRITEL(reg, S5P_FIMV_E_GOP_CONFIG_V6);
+	/* send all runtime encoder parameters. */
+	p->codec.runtime.params_changed = ~0;
+	s5p_mfc_set_runtime_enc_params(ctx, &p->codec.runtime);
+	p->codec.runtime.params_changed = 0;
 
 	/* multi-slice control */
 	/* multi-slice MB number or bit size */
@@ -700,13 +746,6 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx)
 	reg |= ((p->rc_frame & 0x1) << 9);
 	WRITEL(reg, S5P_FIMV_E_RC_CONFIG_V6);
 
-	/* bit rate */
-	if (p->rc_frame)
-		WRITEL(p->rc_bitrate,
-			S5P_FIMV_E_RC_BIT_RATE_V6);
-	else
-		WRITEL(1, S5P_FIMV_E_RC_BIT_RATE_V6);
-
 	/* reaction coefficient */
 	if (p->rc_frame) {
 		if (p->rc_reaction_coeff < TIGHT_CBR_MAX) /* tight CBR */
@@ -814,14 +853,6 @@ static int s5p_mfc_set_enc_params_h264(struct s5p_mfc_ctx *ctx)
 		WRITEL(reg, S5P_FIMV_E_FIXED_PICTURE_QP_V6);
 	}
 
-	/* frame rate */
-	if (p->rc_frame && p->rc_framerate_num && p->rc_framerate_denom) {
-		reg = 0;
-		reg |= ((p->rc_framerate_num & 0xFFFF) << 16);
-		reg |= p->rc_framerate_denom & 0xFFFF;
-		WRITEL(reg, S5P_FIMV_E_RC_FRAME_RATE_V6);
-	}
-
 	/* vbv buffer size */
 	if (p->frame_skip_mode ==
 			V4L2_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT) {
@@ -1089,14 +1120,6 @@ static int s5p_mfc_set_enc_params_mpeg4(struct s5p_mfc_ctx *ctx)
 		WRITEL(reg, S5P_FIMV_E_FIXED_PICTURE_QP_V6);
 	}
 
-	/* frame rate */
-	if (p->rc_frame && p->rc_framerate_num && p->rc_framerate_denom) {
-		reg = 0;
-		reg |= ((p->rc_framerate_num & 0xFFFF) << 16);
-		reg |= p->rc_framerate_denom & 0xFFFF;
-		WRITEL(reg, S5P_FIMV_E_RC_FRAME_RATE_V6);
-	}
-
 	/* vbv buffer size */
 	if (p->frame_skip_mode ==
 			V4L2_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT) {
@@ -1161,14 +1184,6 @@ static int s5p_mfc_set_enc_params_h263(struct s5p_mfc_ctx *ctx)
 		WRITEL(reg, S5P_FIMV_E_FIXED_PICTURE_QP_V6);
 	}
 
-	/* frame rate */
-	if (p->rc_frame && p->rc_framerate_num && p->rc_framerate_denom) {
-		reg = 0;
-		reg |= ((p->rc_framerate_num & 0xFFFF) << 16);
-		reg |= p->rc_framerate_denom & 0xFFFF;
-		WRITEL(reg, S5P_FIMV_E_RC_FRAME_RATE_V6);
-	}
-
 	/* vbv buffer size */
 	if (p->frame_skip_mode ==
 			V4L2_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT) {
@@ -1214,14 +1229,6 @@ static int s5p_mfc_set_enc_params_vp8(struct s5p_mfc_ctx *ctx)
 	reg |= ((p->rc_mb & 0x1) << 8);
 	WRITEL(reg, S5P_FIMV_E_RC_CONFIG_V6);
 
-	/* frame rate */
-	if (p->rc_frame && p->rc_framerate_num && p->rc_framerate_denom) {
-		reg = 0;
-		reg |= ((p->rc_framerate_num & 0xFFFF) << 16);
-		reg |= p->rc_framerate_denom & 0xFFFF;
-		WRITEL(reg, S5P_FIMV_E_RC_FRAME_RATE_V6);
-	}
-
 	/* vbv buffer size */
 	if (p->frame_skip_mode ==
 			V4L2_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT) {
@@ -1560,6 +1567,7 @@ static inline int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
 	mfc_debug(2, "enc src c addr: 0x%08lx\n", src_c_addr);
 
 	s5p_mfc_set_enc_frame_buffer_v6(ctx, src_y_addr, src_c_addr);
+	s5p_mfc_set_runtime_enc_params(ctx, &src_mb->runtime_enc_params);
 
 	dst_mb = list_entry(ctx->dst_queue.next, struct s5p_mfc_buf, list);
 	dst_mb->flags |= MFC_BUF_FLAG_USED;
-- 
1.8.4

[PATCH 5/6] [media] gsc-m2m: report correct format bytesperline and sizeimage

[John Sheu]

Explicitly specify sampling period for subsampled chroma formats, so
stride and image size are properly reported through VIDIOC_{S,G}_FMT.

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/media/platform/exynos-gsc/gsc-core.c | 154 +++++++++++++++------------
 drivers/media/platform/exynos-gsc/gsc-core.h |  16 +--
 drivers/media/platform/exynos-gsc/gsc-regs.c |  40 +++----
 drivers/media/platform/exynos-gsc/gsc-regs.h |   4 +-
 4 files changed, 116 insertions(+), 98 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 9d0cc04..c02adde 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -34,167 +34,185 @@ static const struct gsc_fmt gsc_formats[] = {
 	{
 		.name		= "RGB565",
 		.pixelformat	= V4L2_PIX_FMT_RGB565X,
-		.depth		= { 16 },
 		.color		= GSC_RGB,
 		.num_planes	= 1,
 		.num_comp	= 1,
+		.depth		= { 16 },
+		.sampling	= { { 1, 1 } },
 	}, {
 		.name		= "XRGB-8-8-8-8, 32 bpp",
 		.pixelformat	= V4L2_PIX_FMT_RGB32,
-		.depth		= { 32 },
 		.color		= GSC_RGB,
 		.num_planes	= 1,
 		.num_comp	= 1,
+		.depth		= { 32 },
+		.sampling	= { { 1, 1 } },
 	}, {
 		.name		= "YUV 4:2:2 packed, YCbYCr",
 		.pixelformat	= V4L2_PIX_FMT_YUYV,
-		.depth		= { 16 },
 		.color		= GSC_YUV422,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 1,
 		.num_comp	= 1,
+		.depth		= { 16 },
+		.sampling	= { { 1, 1 } },
 		.mbus_code	= V4L2_MBUS_FMT_YUYV8_2X8,
 	}, {
 		.name		= "YUV 4:2:2 packed, CbYCrY",
 		.pixelformat	= V4L2_PIX_FMT_UYVY,
-		.depth		= { 16 },
 		.color		= GSC_YUV422,
 		.yorder		= GSC_LSB_C,
 		.corder		= GSC_CBCR,
 		.num_planes	= 1,
 		.num_comp	= 1,
+		.depth		= { 16 },
+		.sampling	= { { 1, 1 } },
 		.mbus_code	= V4L2_MBUS_FMT_UYVY8_2X8,
 	}, {
 		.name		= "YUV 4:2:2 packed, CrYCbY",
 		.pixelformat	= V4L2_PIX_FMT_VYUY,
-		.depth		= { 16 },
 		.color		= GSC_YUV422,
 		.yorder		= GSC_LSB_C,
 		.corder		= GSC_CRCB,
 		.num_planes	= 1,
 		.num_comp	= 1,
+		.depth		= { 16 },
+		.sampling	= { { 1, 1 } },
 		.mbus_code	= V4L2_MBUS_FMT_VYUY8_2X8,
 	}, {
 		.name		= "YUV 4:2:2 packed, YCrYCb",
 		.pixelformat	= V4L2_PIX_FMT_YVYU,
-		.depth		= { 16 },
 		.color		= GSC_YUV422,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CRCB,
 		.num_planes	= 1,
 		.num_comp	= 1,
+		.depth		= { 16 },
+		.sampling	= { { 1, 1 } },
 		.mbus_code	= V4L2_MBUS_FMT_YVYU8_2X8,
 	}, {
 		.name		= "YUV 4:4:4 planar, YCbYCr",
 		.pixelformat	= V4L2_PIX_FMT_YUV32,
-		.depth		= { 32 },
 		.color		= GSC_YUV444,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 1,
 		.num_comp	= 1,
+		.depth		= { 32 },
+		.sampling	= { { 1, 1 } },
 	}, {
 		.name		= "YUV 4:2:2 planar, Y/Cb/Cr",
 		.pixelformat	= V4L2_PIX_FMT_YUV422P,
-		.depth		= { 16 },
 		.color		= GSC_YUV422,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 1,
 		.num_comp	= 3,
+		.depth		= { 8, 8, 8 },
+		.sampling	= { { 1, 1 }, { 2, 1 }, { 2, 1 } },
 	}, {
 		.name		= "YUV 4:2:2 planar, Y/CbCr",
 		.pixelformat	= V4L2_PIX_FMT_NV16,
-		.depth		= { 16 },
 		.color		= GSC_YUV422,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 1,
 		.num_comp	= 2,
+		.depth		= { 8, 16 },
+		.sampling	= { { 1, 1 }, { 2, 1 } },
 	}, {
 		.name		= "YUV 4:2:2 planar, Y/CrCb",
 		.pixelformat	= V4L2_PIX_FMT_NV61,
-		.depth		= { 16 },
 		.color		= GSC_YUV422,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CRCB,
 		.num_planes	= 1,
 		.num_comp	= 2,
+		.depth		= { 8, 16 },
+		.sampling	= { { 1, 1 }, { 2, 1 } },
 	}, {
 		.name		= "YUV 4:2:0 planar, YCbCr",
 		.pixelformat	= V4L2_PIX_FMT_YUV420,
-		.depth		= { 12 },
 		.color		= GSC_YUV420,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 1,
 		.num_comp	= 3,
+		.depth		= { 8, 8, 8 },
+		.sampling	= { { 1, 1 }, { 2, 2 }, { 2, 2 } },
 	}, {
 		.name		= "YUV 4:2:0 planar, YCrCb",
 		.pixelformat	= V4L2_PIX_FMT_YVU420,
-		.depth		= { 12 },
 		.color		= GSC_YUV420,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CRCB,
 		.num_planes	= 1,
 		.num_comp	= 3,
-
+		.depth		= { 8, 8, 8 },
+		.sampling	= { { 1, 1 }, { 2, 2 }, { 2, 2 } },
 	}, {
 		.name		= "YUV 4:2:0 planar, Y/CbCr",
 		.pixelformat	= V4L2_PIX_FMT_NV12,
-		.depth		= { 12 },
 		.color		= GSC_YUV420,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 1,
 		.num_comp	= 2,
+		.depth		= { 8, 16 },
+		.sampling	= { { 1, 1 }, { 2, 2 } },
 	}, {
 		.name		= "YUV 4:2:0 planar, Y/CrCb",
 		.pixelformat	= V4L2_PIX_FMT_NV21,
-		.depth		= { 12 },
 		.color		= GSC_YUV420,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CRCB,
 		.num_planes	= 1,
 		.num_comp	= 2,
+		.depth		= { 8, 16 },
+		.sampling	= { { 1, 1 }, { 2, 2 } },
 	}, {
-		.name		= "YUV 4:2:0 non-contig. 2p, Y/CbCr",
+		.name		= "YUV 4:2:0 non-contiguous 2-planar, Y/CbCr",
 		.pixelformat	= V4L2_PIX_FMT_NV12M,
-		.depth		= { 8, 4 },
 		.color		= GSC_YUV420,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 2,
 		.num_comp	= 2,
+		.depth		= { 8, 16 },
+		.sampling	= { { 1, 1 }, { 2, 2 } },
 	}, {
-		.name		= "YUV 4:2:0 non-contig. 3p, Y/Cb/Cr",
+		.name		= "YUV 4:2:0 non-contiguous 3-planar, Y/Cb/Cr",
 		.pixelformat	= V4L2_PIX_FMT_YUV420M,
-		.depth		= { 8, 2, 2 },
 		.color		= GSC_YUV420,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 3,
 		.num_comp	= 3,
+		.depth		= { 8, 8, 8 },
+		.sampling	= { { 1, 1 }, { 2, 2 }, { 2, 2 } },
 	}, {
-		.name		= "YUV 4:2:0 non-contig. 3p, Y/Cr/Cb",
+		.name		= "YUV 4:2:0 non-contiguous 3-planar, Y/Cr/Cb",
 		.pixelformat	= V4L2_PIX_FMT_YVU420M,
-		.depth		= { 8, 2, 2 },
 		.color		= GSC_YUV420,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CRCB,
 		.num_planes	= 3,
 		.num_comp	= 3,
+		.depth		= { 8, 8, 8 },
+		.sampling	= { { 1, 1 }, { 2, 2 }, { 2, 2 } },
 	}, {
-		.name		= "YUV 4:2:0 n.c. 2p, Y/CbCr tiled",
+		.name		=
+			"YUV 4:2:0 non-contiguous 2-planar, Y/CbCr, tiled",
 		.pixelformat	= V4L2_PIX_FMT_NV12MT_16X16,
-		.depth		= { 8, 4 },
 		.color		= GSC_YUV420,
 		.yorder		= GSC_LSB_Y,
 		.corder		= GSC_CBCR,
 		.num_planes	= 2,
 		.num_comp	= 2,
-	}
+		.depth		= { 8, 16 },
+		.sampling	= { { 1, 1 }, { 2, 2 } },
+	},
 };
 
 const struct gsc_fmt *get_format(int index)
@@ -384,6 +402,14 @@ void gsc_set_prefbuf(struct gsc_dev *gsc, struct gsc_frame *frm)
 			f_chk_addr, f_chk_len, s_chk_addr, s_chk_len);
 }
 
+static void get_format_size(__u32 width, __u32 height,
+			    const struct gsc_fmt *fmt, int plane,
+			    __u16 *bytesperline, __u32 *sizeimage) {
+	__u16 bpl = ((width * fmt->depth[plane]) / fmt->sampling[plane][0]) / 8;
+	*bytesperline = bpl;
+	*sizeimage = (height * bpl) / fmt->sampling[plane][0];
+}
+
 int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 {
 	struct gsc_dev *gsc = ctx->gsc_dev;
@@ -448,14 +474,16 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 	else /* SD */
 		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
 
-
+	/* V4L2 specifies for contiguous planar formats that bytesperline and
+	   sizeimage are set to values appropriate for the first plane. */
 	for (i = 0; i < pix_mp->num_planes; ++i) {
-		int bpl = (pix_mp->width * fmt->depth[i]) >> 3;
-		pix_mp->plane_fmt[i].bytesperline = bpl;
-		pix_mp->plane_fmt[i].sizeimage = bpl * pix_mp->height;
+		get_format_size(pix_mp->width, pix_mp->height, fmt, i,
+				&pix_mp->plane_fmt[i].bytesperline,
+				&pix_mp->plane_fmt[i].sizeimage);
 
 		pr_debug("[%d]: bpl: %d, sizeimage: %d",
-				i, bpl, pix_mp->plane_fmt[i].sizeimage);
+				i, pix_mp->plane_fmt[i].bytesperline,
+				pix_mp->plane_fmt[i].sizeimage);
 	}
 
 	return 0;
@@ -465,26 +493,27 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 {
 	struct gsc_frame *frame;
 	struct v4l2_pix_format_mplane *pix_mp;
+	const struct gsc_fmt *fmt;
 	int i;
 
 	frame = ctx_get_frame(ctx, f->type);
 	if (IS_ERR(frame))
 		return PTR_ERR(frame);
+	fmt = frame->fmt;
 
 	pix_mp = &f->fmt.pix_mp;
 
 	pix_mp->width		= frame->f_width;
 	pix_mp->height		= frame->f_height;
 	pix_mp->field		= V4L2_FIELD_NONE;
-	pix_mp->pixelformat	= frame->fmt->pixelformat;
+	pix_mp->pixelformat	= fmt->pixelformat;
 	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
-	pix_mp->num_planes	= frame->fmt->num_planes;
+	pix_mp->num_planes	= fmt->num_planes;
 
 	for (i = 0; i < pix_mp->num_planes; ++i) {
-		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
-			frame->fmt->depth[i]) / 8;
-		pix_mp->plane_fmt[i].sizeimage =
-			 pix_mp->plane_fmt[i].bytesperline * frame->f_height;
+		get_format_size(pix_mp->width, pix_mp->height, fmt, i,
+				&pix_mp->plane_fmt[i].bytesperline,
+				&pix_mp->plane_fmt[i].sizeimage);
 	}
 
 	return 0;
@@ -794,11 +823,12 @@ void gsc_ctrls_delete(struct gsc_ctx *ctx)
 	}
 }
 
-/* The color format (num_comp, num_planes) must be already configured. */
+/* The color format (num_planes, num_comp) must be already configured. */
 int gsc_prepare_addr(struct gsc_ctx *ctx, struct vb2_buffer *vb,
 			struct gsc_frame *frame, struct gsc_addr *addr)
 {
 	int ret = 0;
+	const struct gsc_fmt *fmt = frame->fmt;
 	u32 pix_size;
 
 	if ((vb == NULL) || (frame == NULL))
@@ -810,46 +840,30 @@ int gsc_prepare_addr(struct gsc_ctx *ctx, struct vb2_buffer *vb,
 		frame->fmt->num_planes, frame->fmt->num_comp, pix_size);
 
 	addr->y = vb2_dma_contig_plane_dma_addr(vb, 0);
-
-	if (frame->fmt->num_planes == 1) {
-		switch (frame->fmt->num_comp) {
-		case 1:
-			addr->cb = 0;
-			addr->cr = 0;
-			break;
-		case 2:
-			/* decompose Y into Y/Cb */
-			addr->cb = (dma_addr_t)(addr->y + pix_size);
-			addr->cr = 0;
-			break;
-		case 3:
-			/* decompose Y into Y/Cb/Cr */
-			addr->cb = (dma_addr_t)(addr->y + pix_size);
-			if (GSC_YUV420 == frame->fmt->color)
-				addr->cr = (dma_addr_t)(addr->cb
-						+ (pix_size >> 2));
-			else /* 422 */
-				addr->cr = (dma_addr_t)(addr->cb
-						+ (pix_size >> 1));
-			break;
-		default:
-			pr_err("Invalid the number of color planes");
-			return -EINVAL;
+	addr->cb = 0;
+	addr->cr = 0;
+
+	if (fmt->num_planes == 1) {
+		if (fmt->num_comp >= 2) {
+			addr->cb = (dma_addr_t)(addr->y +
+				((pix_size * fmt->depth[0]) /
+				(fmt->sampling[0][0] *
+				 fmt->sampling[0][1]) / 8));
+		}
+		if (fmt->num_comp >= 3) {
+			addr->cr = (dma_addr_t)(addr->cb +
+				((pix_size * fmt->depth[1]) /
+				(fmt->sampling[1][0] *
+				 fmt->sampling[1][1]) / 8));
 		}
 	} else {
-		if (frame->fmt->num_planes >= 2)
+		if (fmt->num_comp >= 2)
 			addr->cb = vb2_dma_contig_plane_dma_addr(vb, 1);
-
-		if (frame->fmt->num_planes == 3)
+		if (fmt->num_comp == 3)
 			addr->cr = vb2_dma_contig_plane_dma_addr(vb, 2);
 	}
 
-	if ((frame->fmt->pixelformat == V4L2_PIX_FMT_VYUY) ||
-		(frame->fmt->pixelformat == V4L2_PIX_FMT_YVYU) ||
-		(frame->fmt->pixelformat == V4L2_PIX_FMT_NV61) ||
-		(frame->fmt->pixelformat == V4L2_PIX_FMT_YVU420) ||
-		(frame->fmt->pixelformat == V4L2_PIX_FMT_NV21) ||
-		(frame->fmt->pixelformat == V4L2_PIX_FMT_YVU420M))
+	if (fmt->corder == GSC_CRCB)
 		swap(addr->cb, addr->cr);
 
 	pr_debug("ADDR: y= 0x%X  cb= 0x%X cr= 0x%X ret= %d",
diff --git a/drivers/media/platform/exynos-gsc/gsc-core.h b/drivers/media/platform/exynos-gsc/gsc-core.h
index 76435d3..c393678 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.h
+++ b/drivers/media/platform/exynos-gsc/gsc-core.h
@@ -105,27 +105,27 @@ enum gsc_yuv_fmt {
 	container_of((__ctrl)->handler, struct gsc_ctx, ctrl_handler)
 /**
  * struct gsc_fmt - the driver's internal color format data
- * @mbus_code: Media Bus pixel code, -1 if not applicable
  * @name: format description
  * @pixelformat: the fourcc code for this format, 0 if not applicable
  * @yorder: Y/C order
  * @corder: Chrominance order control
  * @num_planes: number of physically non-contiguous data planes
- * @nr_comp: number of physically contiguous data planes
- * @depth: per plane driver's private 'number of bits per pixel'
- * @flags: flags indicating which operation mode format applies to
+ * @num_comp: number of physically contiguous data planes
+ * @depth: bit depth of each component
+ * @sampling: sampling frequency of each components, X and Y
+ * @mbus_code: Media Bus pixel code, -1 if not applicable
  */
 struct gsc_fmt {
-	enum v4l2_mbus_pixelcode mbus_code;
 	char	*name;
 	u32	pixelformat;
 	u32	color;
 	u32	yorder;
 	u32	corder;
-	u16	num_planes;
-	u16	num_comp;
+	u8	num_planes;
+	u8	num_comp;
 	u8	depth[VIDEO_MAX_PLANES];
-	u32	flags;
+	u8	sampling[VIDEO_MAX_PLANES][2];
+	enum v4l2_mbus_pixelcode mbus_code;
 };
 
 /**
diff --git a/drivers/media/platform/exynos-gsc/gsc-regs.c b/drivers/media/platform/exynos-gsc/gsc-regs.c
index e22d147..a8d6c90 100644
--- a/drivers/media/platform/exynos-gsc/gsc-regs.c
+++ b/drivers/media/platform/exynos-gsc/gsc-regs.c
@@ -167,6 +167,7 @@ void gsc_hw_set_in_image_format(struct gsc_ctx *ctx)
 {
 	struct gsc_dev *dev = ctx->gsc_dev;
 	struct gsc_frame *frame = &ctx->s_frame;
+	const struct gsc_fmt *fmt = frame->fmt;
 	u32 i, depth = 0;
 	u32 cfg;
 
@@ -176,21 +177,22 @@ void gsc_hw_set_in_image_format(struct gsc_ctx *ctx)
 		 GSC_IN_TILE_TYPE_MASK | GSC_IN_TILE_MODE);
 	writel(cfg, dev->regs + GSC_IN_CON);
 
-	if (is_rgb(frame->fmt->color)) {
+	if (is_rgb(fmt->color)) {
 		gsc_hw_set_in_image_rgb(ctx);
 		return;
 	}
-	for (i = 0; i < frame->fmt->num_planes; i++)
-		depth += frame->fmt->depth[i];
+	for (i = 0; i < fmt->num_comp; i++)
+		depth += fmt->depth[i] /
+			(fmt->sampling[i][0] * fmt->sampling[i][1]);
 
-	switch (frame->fmt->num_comp) {
+	switch (fmt->num_comp) {
 	case 1:
 		cfg |= GSC_IN_YUV422_1P;
-		if (frame->fmt->yorder == GSC_LSB_Y)
+		if (fmt->yorder == GSC_LSB_Y)
 			cfg |= GSC_IN_YUV422_1P_ORDER_LSB_Y;
 		else
-			cfg |= GSC_IN_YUV422_1P_OEDER_LSB_C;
-		if (frame->fmt->corder == GSC_CBCR)
+			cfg |= GSC_IN_YUV422_1P_ORDER_LSB_C;
+		if (fmt->corder == GSC_CBCR)
 			cfg |= GSC_IN_CHROMA_ORDER_CBCR;
 		else
 			cfg |= GSC_IN_CHROMA_ORDER_CRCB;
@@ -200,7 +202,7 @@ void gsc_hw_set_in_image_format(struct gsc_ctx *ctx)
 			cfg |= GSC_IN_YUV420_2P;
 		else
 			cfg |= GSC_IN_YUV422_2P;
-		if (frame->fmt->corder == GSC_CBCR)
+		if (fmt->corder == GSC_CBCR)
 			cfg |= GSC_IN_CHROMA_ORDER_CBCR;
 		else
 			cfg |= GSC_IN_CHROMA_ORDER_CRCB;
@@ -213,7 +215,7 @@ void gsc_hw_set_in_image_format(struct gsc_ctx *ctx)
 		break;
 	}
 
-	if (is_tiled(frame->fmt))
+	if (is_tiled(fmt))
 		cfg |= GSC_IN_TILE_C_16x8 | GSC_IN_TILE_MODE;
 
 	writel(cfg, dev->regs + GSC_IN_CON);
@@ -287,6 +289,7 @@ void gsc_hw_set_out_image_format(struct gsc_ctx *ctx)
 {
 	struct gsc_dev *dev = ctx->gsc_dev;
 	struct gsc_frame *frame = &ctx->d_frame;
+	const struct gsc_fmt *fmt = frame->fmt;
 	u32 i, depth = 0;
 	u32 cfg;
 
@@ -296,7 +299,7 @@ void gsc_hw_set_out_image_format(struct gsc_ctx *ctx)
 		 GSC_OUT_TILE_TYPE_MASK | GSC_OUT_TILE_MODE);
 	writel(cfg, dev->regs + GSC_OUT_CON);
 
-	if (is_rgb(frame->fmt->color)) {
+	if (is_rgb(fmt->color)) {
 		gsc_hw_set_out_image_rgb(ctx);
 		return;
 	}
@@ -306,17 +309,18 @@ void gsc_hw_set_out_image_format(struct gsc_ctx *ctx)
 		goto end_set;
 	}
 
-	for (i = 0; i < frame->fmt->num_planes; i++)
-		depth += frame->fmt->depth[i];
+	for (i = 0; i < fmt->num_comp; i++)
+		depth += fmt->depth[i] /
+			(fmt->sampling[i][0] * fmt->sampling[i][1]);
 
-	switch (frame->fmt->num_comp) {
+	switch (fmt->num_comp) {
 	case 1:
 		cfg |= GSC_OUT_YUV422_1P;
-		if (frame->fmt->yorder == GSC_LSB_Y)
+		if (fmt->yorder == GSC_LSB_Y)
 			cfg |= GSC_OUT_YUV422_1P_ORDER_LSB_Y;
 		else
-			cfg |= GSC_OUT_YUV422_1P_OEDER_LSB_C;
-		if (frame->fmt->corder == GSC_CBCR)
+			cfg |= GSC_OUT_YUV422_1P_ORDER_LSB_C;
+		if (fmt->corder == GSC_CBCR)
 			cfg |= GSC_OUT_CHROMA_ORDER_CBCR;
 		else
 			cfg |= GSC_OUT_CHROMA_ORDER_CRCB;
@@ -326,7 +330,7 @@ void gsc_hw_set_out_image_format(struct gsc_ctx *ctx)
 			cfg |= GSC_OUT_YUV420_2P;
 		else
 			cfg |= GSC_OUT_YUV422_2P;
-		if (frame->fmt->corder == GSC_CBCR)
+		if (fmt->corder == GSC_CBCR)
 			cfg |= GSC_OUT_CHROMA_ORDER_CBCR;
 		else
 			cfg |= GSC_OUT_CHROMA_ORDER_CRCB;
@@ -336,7 +340,7 @@ void gsc_hw_set_out_image_format(struct gsc_ctx *ctx)
 		break;
 	}
 
-	if (is_tiled(frame->fmt))
+	if (is_tiled(fmt))
 		cfg |= GSC_OUT_TILE_C_16x8 | GSC_OUT_TILE_MODE;
 
 end_set:
diff --git a/drivers/media/platform/exynos-gsc/gsc-regs.h b/drivers/media/platform/exynos-gsc/gsc-regs.h
index 4678f9a..b03401d 100644
--- a/drivers/media/platform/exynos-gsc/gsc-regs.h
+++ b/drivers/media/platform/exynos-gsc/gsc-regs.h
@@ -46,7 +46,7 @@
 #define GSC_IN_RGB_SD_WIDE		(0 << 14)
 #define GSC_IN_YUV422_1P_ORDER_MASK	(1 << 13)
 #define GSC_IN_YUV422_1P_ORDER_LSB_Y	(0 << 13)
-#define GSC_IN_YUV422_1P_OEDER_LSB_C	(1 << 13)
+#define GSC_IN_YUV422_1P_ORDER_LSB_C	(1 << 13)
 #define GSC_IN_CHROMA_ORDER_MASK	(1 << 12)
 #define GSC_IN_CHROMA_ORDER_CBCR	(0 << 12)
 #define GSC_IN_CHROMA_ORDER_CRCB	(1 << 12)
@@ -91,7 +91,7 @@
 #define GSC_OUT_RGB_SD_NARROW		(0 << 10)
 #define GSC_OUT_YUV422_1P_ORDER_MASK	(1 << 9)
 #define GSC_OUT_YUV422_1P_ORDER_LSB_Y	(0 << 9)
-#define GSC_OUT_YUV422_1P_OEDER_LSB_C	(1 << 9)
+#define GSC_OUT_YUV422_1P_ORDER_LSB_C	(1 << 9)
 #define GSC_OUT_CHROMA_ORDER_MASK	(1 << 8)
 #define GSC_OUT_CHROMA_ORDER_CBCR	(0 << 8)
 #define GSC_OUT_CHROMA_ORDER_CRCB	(1 << 8)
-- 
1.8.4

[PATCH 6/6] [media] v4l2-mem2mem: allow reqbufs(0) with "in use" MMAP buffers

[John Sheu]

v4l2-mem2mem presently does not allow VIDIOC_REQBUFS to destroy
outstanding buffers if the queue is of type V4L2_MEMORY_MMAP, and if the
buffers are considered "in use".  This is different behavior than for
other memory types, and prevents us for deallocating buffers in a few
cases:

* In the case that there are outstanding mmap()ed views on the buffer,
  refcounting on the videobuf2 buffer backing the vm_area will track
  lifetime appropriately,
* In the case that the buffer has been exported as a DMABUF, refcounting
  on the videobuf2 bufer backing the DMABUF will track lifetime
  appropriately.

Remove the specific check for type V4L2_MEMOMRY_MMAP when freeing all
buffers through VIDIOC_REQBUFS.

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index fc8af50..3c31efb 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -369,8 +369,7 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 }
 
 /**
- * __buffer_in_use() - return true if the buffer is in use and
- * the queue cannot be freed (by the means of REQBUFS(0)) call
+ * __buffer_in_use() - return true if the buffer is in use.
  */
 static bool __buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
 {
@@ -390,20 +389,6 @@ static bool __buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
 }
 
 /**
- * __buffers_in_use() - return true if any buffers on the queue are in use and
- * the queue cannot be freed (by the means of REQBUFS(0)) call
- */
-static bool __buffers_in_use(struct vb2_queue *q)
-{
-	unsigned int buffer;
-	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
-		if (__buffer_in_use(q, q->bufs[buffer]))
-			return true;
-	}
-	return false;
-}
-
-/**
  * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
  * returned to userspace
  */
@@ -626,15 +611,6 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	}
 
 	if (req->count == 0 || q->num_buffers != 0 || q->memory != req->memory) {
-		/*
-		 * We already have buffers allocated, so first check if they
-		 * are not in use and can be freed.
-		 */
-		if (q->memory == V4L2_MEMORY_MMAP && __buffers_in_use(q)) {
-			dprintk(1, "reqbufs: memory in use, cannot free\n");
-			return -EBUSY;
-		}
-
 		__vb2_queue_free(q, q->num_buffers);
 
 		/*
-- 
1.8.4