[PATCH v2 00/14] Fixes from Chrome OS tree for MFC driver

[PATCH v2 00/14] Fixes from Chrome OS tree for MFC driver

[Kiran AVND]

Upstreaming the fixes which have gone in to Chrome OS tree for MFC driver.
Tested on MFCV8, MFCV7 and MFCV6 based Exynos5 based boards, peach-pi
(5800), peach-pit (5420) and snow (5250).

These are all independent fixes and hence posting them as a patchset.

Changes from v1:
1) Addressed all review comments from Kamil.
2) Dropped patches
   [media] s5p-mfc: set B-frames as 2 while encoding
   [media] s5p-mfc: remove reduntant clock on & clock off
   [media] s5p-mfc: don't disable clock when next ctx is pending
3) Rebased on media-tree

Arun Mankuzhi (2):
  [media] s5p-mfc: modify mfc wakeup sequence for V8
  [media] s5p-mfc: De-init MFC when watchdog kicks in

Ilja Friedel (1):
  [media] s5p-mfc: Only set timestamp/timecode for new frames.

Kiran AVND (4):
  [media] s5p-mfc: support MIN_BUFFERS query for encoder
  [media] s5p-mfc: keep RISC ON during reset for V7/V8
  [media] s5p-mfc: check mfc bus ctrl before reset
  [media] s5p-mfc: flush dpbs when resolution changes

Pawel Osciak (5):
  [media] s5p-mfc: Fix REQBUFS(0) for encoder.
  [media] s5p-mfc: Don't crash the kernel if the watchdog kicks in.
  [media] s5p-mfc: Remove unused alloc field from private buffer
    struct.
  [media] s5p-mfc: fix V4L2_CID_MIN_BUFFERS_FOR_CAPTURE on resolution
    change.
  [media] s5p-mfc: fix a race in interrupt flags handling

Prathyush K (1):
  [media] s5p-mfc: clear 'enter_suspend' flag if suspend fails

Wu-Cheng Li (1):
  [media] s5p-mfc: Don't change the image size to smaller than the
    request.

 drivers/media/platform/s5p-mfc/regs-mfc-v6.h    |    1 +
 drivers/media/platform/s5p-mfc/s5p_mfc.c        |   49 +++++----
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |    4 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   |  122 ++++++++++++++++++-----
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |    6 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    |   67 ++++++++++++-
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |   13 +--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |   32 +-----
 8 files changed, 199 insertions(+), 95 deletions(-)

-- 
1.7.9.5

[PATCH v2 01/14] [media] s5p-mfc: support MIN_BUFFERS query for encoder

[Kiran AVND]

Add V4L2_CID_MIN_BUFFERS_FOR_OUTPUT query for encoder.
Once mfc encoder state is HEAD_PARSED, which is sequence
header produced, dpb_count is avaialable. Let user space
query this value.

Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c |   42 ++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 41f3b7f..b45cccc 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -690,6 +690,16 @@ static struct mfc_control controls[] = {
 		.step = 1,
 		.default_value = 0,
 	},
+	{
+		.id = V4L2_CID_MIN_BUFFERS_FOR_OUTPUT,
+		.type = V4L2_CTRL_TYPE_INTEGER,
+		.name = "Minimum number of output bufs",
+		.minimum = 1,
+		.maximum = 32,
+		.step = 1,
+		.default_value = 1,
+		.is_volatile = 1,
+	},
 };
 
 #define NUM_CTRLS ARRAY_SIZE(controls)
@@ -1640,8 +1650,40 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl)
 	return ret;
 }
 
+static int s5p_mfc_enc_g_v_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct s5p_mfc_ctx *ctx = ctrl_to_ctx(ctrl);
+	struct s5p_mfc_dev *dev = ctx->dev;
+
+	switch (ctrl->id) {
+	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
+		if (ctx->state >= MFCINST_HEAD_PARSED &&
+		    ctx->state < MFCINST_ABORT) {
+			ctrl->val = ctx->pb_count;
+			break;
+		} else if (ctx->state != MFCINST_INIT) {
+			v4l2_err(&dev->v4l2_dev, "Encoding not initialised\n");
+			return -EINVAL;
+		}
+		/* Should wait for the header to be produced */
+		s5p_mfc_clean_ctx_int_flags(ctx);
+		s5p_mfc_wait_for_done_ctx(ctx,
+				S5P_MFC_R2H_CMD_SEQ_DONE_RET, 0);
+		if (ctx->state >= MFCINST_HEAD_PARSED &&
+		    ctx->state < MFCINST_ABORT) {
+			ctrl->val = ctx->pb_count;
+		} else {
+			v4l2_err(&dev->v4l2_dev, "Encoding not initialised\n");
+			return -EINVAL;
+		}
+		break;
+	}
+	return 0;
+}
+
 static const struct v4l2_ctrl_ops s5p_mfc_enc_ctrl_ops = {
 	.s_ctrl = s5p_mfc_enc_s_ctrl,
+	.g_volatile_ctrl = s5p_mfc_enc_g_v_ctrl,
 };
 
 static int vidioc_s_parm(struct file *file, void *priv,
-- 
1.7.9.5

[PATCH v2 02/14] [media] s5p-mfc: Fix REQBUFS(0) for encoder.

[Kiran AVND]

From: Pawel Osciak <posciak@chromium.org>

Handle REQBUFS(0) for CAPTURE queue as well. Also use the proper queue to call
it on for OUTPUT.

Signed-off-by: Pawel Osciak <posciak@chromium.org>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index b45cccc..653f28f 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1163,6 +1163,11 @@ static int vidioc_reqbufs(struct file *file, void *priv,
 		(reqbufs->memory != V4L2_MEMORY_USERPTR))
 		return -EINVAL;
 	if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+		if (reqbufs->count == 0) {
+			ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
+			ctx->capture_state = QUEUE_FREE;
+			return ret;
+		}
 		if (ctx->capture_state != QUEUE_FREE) {
 			mfc_err("invalid capture state: %d\n",
 							ctx->capture_state);
@@ -1184,6 +1189,14 @@ static int vidioc_reqbufs(struct file *file, void *priv,
 			return -ENOMEM;
 		}
 	} else if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
+		if (reqbufs->count == 0) {
+			mfc_debug(2, "Freeing buffers\n");
+			ret = vb2_reqbufs(&ctx->vq_src, reqbufs);
+			s5p_mfc_hw_call(dev->mfc_ops, release_codec_buffers,
+					ctx);
+			ctx->output_state = QUEUE_FREE;
+			return ret;
+		}
 		if (ctx->output_state != QUEUE_FREE) {
 			mfc_err("invalid output state: %d\n",
 							ctx->output_state);
-- 
1.7.9.5

[PATCH v2 03/14] [media] s5p-mfc: clear 'enter_suspend' flag if suspend fails

[Kiran AVND]

From: Prathyush K <prathyush.k@samsung.com>

The enter_suspend flag is set after we enter mfc suspend function but if
suspend fails after that due to any reason (like hardware timeout etc),
this flag must be cleared before returning an error. Otherwise, this
flag never gets cleared and the MFC suspend will always return an error
on subsequent tries. If clock off fails, disable hw_lock also.

Signed-off-by: Prathyush K <prathyush.k@samsung.com>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index d180440..e876839 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1279,11 +1279,17 @@ static int s5p_mfc_suspend(struct device *dev)
 			m_dev->int_cond, msecs_to_jiffies(MFC_INT_TIMEOUT));
 		if (ret == 0) {
 			mfc_err("Waiting for hardware to finish timed out\n");
+			clear_bit(0, &m_dev->enter_suspend);
 			return -EIO;
 		}
 	}
 
-	return s5p_mfc_sleep(m_dev);
+	ret = s5p_mfc_sleep(m_dev);
+	if (ret) {
+		clear_bit(0, &m_dev->enter_suspend);
+		clear_bit(0, &m_dev->hw_lock);
+	}
+	return ret;
 }
 
 static int s5p_mfc_resume(struct device *dev)
-- 
1.7.9.5

[PATCH v2 04/14] [media] s5p-mfc: Only set timestamp/timecode for new frames.

[Kiran AVND]

From: Ilja Friedel <ihf@chromium.org>

Timestamp i of a previously decoded buffer was overwritten for some
H.264 streams with timestamp i+1 of the next buffer. This happened when
encountering frame_type S5P_FIMV_DECODE_FRAME_SKIPPED, indicating no
new frame.

In most cases this wrong indexing might not have been noticed except
for a one frame delay in frame presentation. For H.264 streams though
that require reordering of frames for presentation, it caused a slightly
erratic presentation time lookup and consequently dropped frames in the
Pepper Flash plugin.

Signed-off-by: Ilja H. Friedel <ihf@google.com>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index e876839..3fc2f8a 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -220,11 +220,14 @@ static void s5p_mfc_handle_frame_copy_time(struct s5p_mfc_ctx *ctx)
 	size_t dec_y_addr;
 	unsigned int frame_type;
 
-	dec_y_addr = s5p_mfc_hw_call(dev->mfc_ops, get_dec_y_adr, dev);
+	/* Make sure we actually have a new frame before continuing. */
 	frame_type = s5p_mfc_hw_call(dev->mfc_ops, get_dec_frame_type, dev);
+	if (frame_type == S5P_FIMV_DECODE_FRAME_SKIPPED)
+		return;
+	dec_y_addr = s5p_mfc_hw_call(dev->mfc_ops, get_dec_y_adr, dev);
 
 	/* Copy timestamp / timecode from decoded src to dst and set
-	   appropriate flags */
+	   appropriate flags. */
 	src_buf = list_entry(ctx->src_queue.next, struct s5p_mfc_buf, list);
 	list_for_each_entry(dst_buf, &ctx->dst_queue, list) {
 		if (vb2_dma_contig_plane_dma_addr(dst_buf->b, 0) == dec_y_addr) {
@@ -250,6 +253,11 @@ static void s5p_mfc_handle_frame_copy_time(struct s5p_mfc_ctx *ctx)
 				dst_buf->b->v4l2_buf.flags |=
 						V4L2_BUF_FLAG_BFRAME;
 				break;
+			default:
+				/* Don't know how to handle
+				   S5P_FIMV_DECODE_FRAME_OTHER_FRAME. */
+				mfc_debug(2, "Unexpected frame type: %d\n",
+						frame_type);
 			}
 			break;
 		}
-- 
1.7.9.5

[PATCH v2 05/14] [media] s5p-mfc: keep RISC ON during reset for V7/V8

[Kiran AVND]

Reset sequence for MFC V7 and V8 do not need RISC_ON
to be set to 0, while for MFC V6 it is still needed.

Also, remove a couple of register settings during Reset
which are not needed from V6 onwards.

Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |    1 +
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   |   25 ++++++++++++++---------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 01816ff..c0de03d 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -340,6 +340,7 @@ struct s5p_mfc_dev {
 	struct s5p_mfc_hw_cmds *mfc_cmds;
 	const struct s5p_mfc_regs *mfc_regs;
 	enum s5p_mfc_fw_ver fw_ver;
+	bool risc_on; /* indicates if RISC is on or off */
 };
 
 /**
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
index 23d247d..4b5cb02 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -139,12 +139,6 @@ int s5p_mfc_reset(struct s5p_mfc_dev *dev)
 	mfc_debug_enter();
 
 	if (IS_MFCV6_PLUS(dev)) {
-		/* Reset IP */
-		/*  except RISC, reset */
-		mfc_write(dev, 0xFEE, S5P_FIMV_MFC_RESET_V6);
-		/*  reset release */
-		mfc_write(dev, 0x0, S5P_FIMV_MFC_RESET_V6);
-
 		/* Zero Initialization of MFC registers */
 		mfc_write(dev, 0, S5P_FIMV_RISC2HOST_CMD_V6);
 		mfc_write(dev, 0, S5P_FIMV_HOST2RISC_CMD_V6);
@@ -153,8 +147,13 @@ int s5p_mfc_reset(struct s5p_mfc_dev *dev)
 		for (i = 0; i < S5P_FIMV_REG_CLEAR_COUNT_V6; i++)
 			mfc_write(dev, 0, S5P_FIMV_REG_CLEAR_BEGIN_V6 + (i*4));
 
-		/* Reset */
-		mfc_write(dev, 0, S5P_FIMV_RISC_ON_V6);
+		/* Reset
+		 * set RISC_ON to 0 during power_on & wake_up.
+		 * V6 needs RISC_ON set to 0 during reset also.
+		 */
+		if ((!dev->risc_on) || (!IS_MFCV7(dev)))
+			mfc_write(dev, 0, S5P_FIMV_RISC_ON_V6);
+
 		mfc_write(dev, 0x1FFF, S5P_FIMV_MFC_RESET_V6);
 		mfc_write(dev, 0, S5P_FIMV_MFC_RESET_V6);
 	} else {
@@ -226,6 +225,7 @@ int s5p_mfc_init_hw(struct s5p_mfc_dev *dev)
 	/* 0. MFC reset */
 	mfc_debug(2, "MFC reset..\n");
 	s5p_mfc_clock_on();
+	dev->risc_on = 0;
 	ret = s5p_mfc_reset(dev);
 	if (ret) {
 		mfc_err("Failed to reset MFC - timeout\n");
@@ -238,8 +238,10 @@ int s5p_mfc_init_hw(struct s5p_mfc_dev *dev)
 	s5p_mfc_clear_cmds(dev);
 	/* 3. Release reset signal to the RISC */
 	s5p_mfc_clean_dev_int_flags(dev);
-	if (IS_MFCV6_PLUS(dev))
+	if (IS_MFCV6_PLUS(dev)) {
+		dev->risc_on = 1;
 		mfc_write(dev, 0x1, S5P_FIMV_RISC_ON_V6);
+	}
 	else
 		mfc_write(dev, 0x3ff, S5P_FIMV_SW_RESET);
 	mfc_debug(2, "Will now wait for completion of firmware transfer\n");
@@ -336,6 +338,7 @@ int s5p_mfc_wakeup(struct s5p_mfc_dev *dev)
 	/* 0. MFC reset */
 	mfc_debug(2, "MFC reset..\n");
 	s5p_mfc_clock_on();
+	dev->risc_on = 0;
 	ret = s5p_mfc_reset(dev);
 	if (ret) {
 		mfc_err("Failed to reset MFC - timeout\n");
@@ -354,8 +357,10 @@ int s5p_mfc_wakeup(struct s5p_mfc_dev *dev)
 		return ret;
 	}
 	/* 4. Release reset signal to the RISC */
-	if (IS_MFCV6_PLUS(dev))
+	if (IS_MFCV6_PLUS(dev)) {
+		dev->risc_on = 1;
 		mfc_write(dev, 0x1, S5P_FIMV_RISC_ON_V6);
+	}
 	else
 		mfc_write(dev, 0x3ff, S5P_FIMV_SW_RESET);
 	mfc_debug(2, "Ok, now will write a command to wakeup the system\n");
-- 
1.7.9.5

[PATCH v2 06/14] [media] s5p-mfc: check mfc bus ctrl before reset

[Kiran AVND]

during reset sequence, it is advisable to follow the below
sequence, in order to avoid unexpected behavior from MFC
. set SFR 0x7110 MFC_BUS_RESET_CTRL 0x1
  // wait for REQ_STATUS to be 1
. get SFR 0x7110 MFC_BUS_RESET_CTRL 0x3
  // reset now

Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/regs-mfc-v6.h  |    1 +
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c |   25 ++++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/regs-mfc-v6.h b/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
index 51cb2dd..83e01f3 100644
--- a/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
+++ b/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
@@ -71,6 +71,7 @@
 #define S5P_FIMV_R2H_CMD_ENC_BUFFER_FUL_RET_V6	16
 #define S5P_FIMV_R2H_CMD_ERR_RET_V6		32
 
+#define S5P_FIMV_MFC_BUS_RESET_CTRL            0x7110
 #define S5P_FIMV_FW_VERSION_V6			0xf000
 
 #define S5P_FIMV_INSTANCE_ID_V6			0xf008
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
index 4b5cb02..605f11e 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -129,6 +129,25 @@ int s5p_mfc_release_firmware(struct s5p_mfc_dev *dev)
 	return 0;
 }
 
+int s5p_mfc_bus_reset(struct s5p_mfc_dev *dev)
+{
+	unsigned int status;
+	unsigned long timeout;
+
+	/* Reset */
+	mfc_write(dev, 0x1, S5P_FIMV_MFC_BUS_RESET_CTRL);
+	timeout = jiffies + msecs_to_jiffies(MFC_BW_TIMEOUT);
+	/* Check bus status */
+	do {
+		if (time_after(jiffies, timeout)) {
+			mfc_err("Timeout while resetting MFC.\n");
+			return -EIO;
+		}
+		status = mfc_read(dev, S5P_FIMV_MFC_BUS_RESET_CTRL);
+	} while ((status & 0x2) == 0);
+	return 0;
+}
+
 /* Reset the device */
 int s5p_mfc_reset(struct s5p_mfc_dev *dev)
 {
@@ -147,11 +166,15 @@ int s5p_mfc_reset(struct s5p_mfc_dev *dev)
 		for (i = 0; i < S5P_FIMV_REG_CLEAR_COUNT_V6; i++)
 			mfc_write(dev, 0, S5P_FIMV_REG_CLEAR_BEGIN_V6 + (i*4));
 
+		/* check bus reset control before reset */
+		if (dev->risc_on)
+			if (s5p_mfc_bus_reset(dev))
+				return -EIO;
 		/* Reset
 		 * set RISC_ON to 0 during power_on & wake_up.
 		 * V6 needs RISC_ON set to 0 during reset also.
 		 */
-		if ((!dev->risc_on) || (!IS_MFCV7(dev)))
+		if ((!dev->risc_on) || (!IS_MFCV7_PLUS(dev)))
 			mfc_write(dev, 0, S5P_FIMV_RISC_ON_V6);
 
 		mfc_write(dev, 0x1FFF, S5P_FIMV_MFC_RESET_V6);
-- 
1.7.9.5

[PATCH v2 07/14] [media] s5p-mfc: Don't crash the kernel if the watchdog kicks in.

[Kiran AVND]

From: Pawel Osciak <posciak@chromium.org>

If the software watchdog kicks in, the watchdog worker is not synchronized
with hardware interrupts and does not block other instances. It's possible
for it to clear the hw_lock, making other instances trigger a BUG() on
hw_lock checks. Since it's not fatal to clear the hw_lock to zero twice,
just WARN in those cases for now. We should not explode, as firmware will
return errors as needed for other instances after it's reloaded, or they
will time out.

A clean fix should involve killing other instances when watchdog kicks in,
but requires a major redesign of locking in the driver.

Signed-off-by: Pawel Osciak <posciak@chromium.org>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c |   25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 3fc2f8a..8d5da0c 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -337,8 +337,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx *ctx,
 		ctx->state = MFCINST_RES_CHANGE_INIT;
 		s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
 		wake_up_ctx(ctx, reason, err);
-		if (test_and_clear_bit(0, &dev->hw_lock) == 0)
-			BUG();
+		WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
 		s5p_mfc_clock_off();
 		s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
 		return;
@@ -410,8 +409,7 @@ leave_handle_frame:
 		clear_work_bit(ctx);
 	s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
 	wake_up_ctx(ctx, reason, err);
-	if (test_and_clear_bit(0, &dev->hw_lock) == 0)
-		BUG();
+	WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
 	s5p_mfc_clock_off();
 	/* if suspending, wake up device and do not try_run again*/
 	if (test_bit(0, &dev->enter_suspend))
@@ -458,8 +456,7 @@ static void s5p_mfc_handle_error(struct s5p_mfc_dev *dev,
 			break;
 		}
 	}
-	if (test_and_clear_bit(0, &dev->hw_lock) == 0)
-		BUG();
+	WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
 	s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
 	s5p_mfc_clock_off();
 	wake_up_dev(dev, reason, err);
@@ -513,8 +510,7 @@ static void s5p_mfc_handle_seq_done(struct s5p_mfc_ctx *ctx,
 	}
 	s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
 	clear_work_bit(ctx);
-	if (test_and_clear_bit(0, &dev->hw_lock) == 0)
-		BUG();
+	WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
 	s5p_mfc_clock_off();
 	s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
 	wake_up_ctx(ctx, reason, err);
@@ -552,19 +548,13 @@ static void s5p_mfc_handle_init_buffers(struct s5p_mfc_ctx *ctx,
 		} else {
 			ctx->dpb_flush_flag = 0;
 		}
-		if (test_and_clear_bit(0, &dev->hw_lock) == 0)
-			BUG();
-
+		WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
 		s5p_mfc_clock_off();
-
 		wake_up(&ctx->queue);
 		s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
 	} else {
-		if (test_and_clear_bit(0, &dev->hw_lock) == 0)
-			BUG();
-
+		WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
 		s5p_mfc_clock_off();
-
 		wake_up(&ctx->queue);
 	}
 }
@@ -638,8 +628,7 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
 				mfc_err("post_frame_start() failed\n");
 			s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
 			wake_up_ctx(ctx, reason, err);
-			if (test_and_clear_bit(0, &dev->hw_lock) == 0)
-				BUG();
+			WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
 			s5p_mfc_clock_off();
 			s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
 		} else {
-- 
1.7.9.5

RE: [PATCH v2 07/14] [media] s5p-mfc: Don't crash the kernel if the watchdog kicks in.

[Kamil Debski]

Hi,

This patch does not apply to the current media tree.

commit cf3167cf1e969b17671a4d3d956d22718a8ceb85)
Author: Antti Palosaari <crope@iki.fi>
Date:   Fri Sep 26 22:45:36 2014 -0300

    [media] pt3: fix DTV FE I2C driver load error paths
    
Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland


> -----Original Message-----
> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> Sent: Friday, September 26, 2014 6:52 AM
> To: linux-media@vger.kernel.org
> Cc: k.debski@samsung.com; wuchengli@chromium.org; posciak@chromium.org;
> arun.m@samsung.com; ihf@chromium.org; prathyush.k@samsung.com;
> arun.kk@samsung.com; kiran@chromium.org
> Subject: [PATCH v2 07/14] [media] s5p-mfc: Don't crash the kernel if
> the watchdog kicks in.
> 
> From: Pawel Osciak <posciak@chromium.org>
> 
> If the software watchdog kicks in, the watchdog worker is not
> synchronized with hardware interrupts and does not block other
> instances. It's possible for it to clear the hw_lock, making other
> instances trigger a BUG() on hw_lock checks. Since it's not fatal to
> clear the hw_lock to zero twice, just WARN in those cases for now. We
> should not explode, as firmware will return errors as needed for other
> instances after it's reloaded, or they will time out.
> 
> A clean fix should involve killing other instances when watchdog kicks
> in, but requires a major redesign of locking in the driver.
> 
> Signed-off-by: Pawel Osciak <posciak@chromium.org>
> Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c |   25 +++++++---------------
> ---
>  1 file changed, 7 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 3fc2f8a..8d5da0c 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -337,8 +337,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
> *ctx,
>  		ctx->state = MFCINST_RES_CHANGE_INIT;
>  		s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>  		wake_up_ctx(ctx, reason, err);
> -		if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> -			BUG();
> +		WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>  		s5p_mfc_clock_off();
>  		s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>  		return;
> @@ -410,8 +409,7 @@ leave_handle_frame:
>  		clear_work_bit(ctx);
>  	s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>  	wake_up_ctx(ctx, reason, err);
> -	if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> -		BUG();
> +	WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>  	s5p_mfc_clock_off();
>  	/* if suspending, wake up device and do not try_run again*/
>  	if (test_bit(0, &dev->enter_suspend))
> @@ -458,8 +456,7 @@ static void s5p_mfc_handle_error(struct s5p_mfc_dev
> *dev,
>  			break;
>  		}
>  	}
> -	if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> -		BUG();
> +	WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>  	s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>  	s5p_mfc_clock_off();
>  	wake_up_dev(dev, reason, err);
> @@ -513,8 +510,7 @@ static void s5p_mfc_handle_seq_done(struct
> s5p_mfc_ctx *ctx,
>  	}
>  	s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>  	clear_work_bit(ctx);
> -	if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> -		BUG();
> +	WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>  	s5p_mfc_clock_off();
>  	s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>  	wake_up_ctx(ctx, reason, err);
> @@ -552,19 +548,13 @@ static void s5p_mfc_handle_init_buffers(struct
> s5p_mfc_ctx *ctx,
>  		} else {
>  			ctx->dpb_flush_flag = 0;
>  		}
> -		if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> -			BUG();
> -
> +		WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>  		s5p_mfc_clock_off();
> -
>  		wake_up(&ctx->queue);
>  		s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>  	} else {
> -		if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> -			BUG();
> -
> +		WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>  		s5p_mfc_clock_off();
> -
>  		wake_up(&ctx->queue);
>  	}
>  }
> @@ -638,8 +628,7 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
>  				mfc_err("post_frame_start() failed\n");
>  			s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>  			wake_up_ctx(ctx, reason, err);
> -			if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> -				BUG();
> +			WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>  			s5p_mfc_clock_off();
>  			s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>  		} else {
> --
> 1.7.9.5

Re: [PATCH v2 07/14] [media] s5p-mfc: Don't crash the kernel if the watchdog kicks in.

[Arun Kumar K]

Hi Kamil,

Kiran will not be available to handle review comments of these patches.
So I will be pushing the updated patchset rebased on media-tree.

I hope all patches are good to merge except
[media] s5p-mfc: Don't change the image size to smaller than the request.

I will drop this one patch and send all others in v3 version.

Regards
Arun

On Wed, Oct 8, 2014 at 3:56 PM, Kamil Debski <k.debski@samsung.com> wrote:
> Hi,
>
> This patch does not apply to the current media tree.
>
> commit cf3167cf1e969b17671a4d3d956d22718a8ceb85)
> Author: Antti Palosaari <crope@iki.fi>
> Date:   Fri Sep 26 22:45:36 2014 -0300
>
>     [media] pt3: fix DTV FE I2C driver load error paths
>
> Best wishes,
> --
> Kamil Debski
> Samsung R&D Institute Poland
>
>
>> -----Original Message-----
>> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
>> Sent: Friday, September 26, 2014 6:52 AM
>> To: linux-media@vger.kernel.org
>> Cc: k.debski@samsung.com; wuchengli@chromium.org; posciak@chromium.org;
>> arun.m@samsung.com; ihf@chromium.org; prathyush.k@samsung.com;
>> arun.kk@samsung.com; kiran@chromium.org
>> Subject: [PATCH v2 07/14] [media] s5p-mfc: Don't crash the kernel if
>> the watchdog kicks in.
>>
>> From: Pawel Osciak <posciak@chromium.org>
>>
>> If the software watchdog kicks in, the watchdog worker is not
>> synchronized with hardware interrupts and does not block other
>> instances. It's possible for it to clear the hw_lock, making other
>> instances trigger a BUG() on hw_lock checks. Since it's not fatal to
>> clear the hw_lock to zero twice, just WARN in those cases for now. We
>> should not explode, as firmware will return errors as needed for other
>> instances after it's reloaded, or they will time out.
>>
>> A clean fix should involve killing other instances when watchdog kicks
>> in, but requires a major redesign of locking in the driver.
>>
>> Signed-off-by: Pawel Osciak <posciak@chromium.org>
>> Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
>> ---
>>  drivers/media/platform/s5p-mfc/s5p_mfc.c |   25 +++++++---------------
>> ---
>>  1 file changed, 7 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> index 3fc2f8a..8d5da0c 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> @@ -337,8 +337,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
>> *ctx,
>>               ctx->state = MFCINST_RES_CHANGE_INIT;
>>               s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>>               wake_up_ctx(ctx, reason, err);
>> -             if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> -                     BUG();
>> +             WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>>               s5p_mfc_clock_off();
>>               s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>>               return;
>> @@ -410,8 +409,7 @@ leave_handle_frame:
>>               clear_work_bit(ctx);
>>       s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>>       wake_up_ctx(ctx, reason, err);
>> -     if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> -             BUG();
>> +     WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>>       s5p_mfc_clock_off();
>>       /* if suspending, wake up device and do not try_run again*/
>>       if (test_bit(0, &dev->enter_suspend))
>> @@ -458,8 +456,7 @@ static void s5p_mfc_handle_error(struct s5p_mfc_dev
>> *dev,
>>                       break;
>>               }
>>       }
>> -     if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> -             BUG();
>> +     WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>>       s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>>       s5p_mfc_clock_off();
>>       wake_up_dev(dev, reason, err);
>> @@ -513,8 +510,7 @@ static void s5p_mfc_handle_seq_done(struct
>> s5p_mfc_ctx *ctx,
>>       }
>>       s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>>       clear_work_bit(ctx);
>> -     if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> -             BUG();
>> +     WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>>       s5p_mfc_clock_off();
>>       s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>>       wake_up_ctx(ctx, reason, err);
>> @@ -552,19 +548,13 @@ static void s5p_mfc_handle_init_buffers(struct
>> s5p_mfc_ctx *ctx,
>>               } else {
>>                       ctx->dpb_flush_flag = 0;
>>               }
>> -             if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> -                     BUG();
>> -
>> +             WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>>               s5p_mfc_clock_off();
>> -
>>               wake_up(&ctx->queue);
>>               s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>>       } else {
>> -             if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> -                     BUG();
>> -
>> +             WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>>               s5p_mfc_clock_off();
>> -
>>               wake_up(&ctx->queue);
>>       }
>>  }
>> @@ -638,8 +628,7 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
>>                               mfc_err("post_frame_start() failed\n");
>>                       s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>>                       wake_up_ctx(ctx, reason, err);
>> -                     if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> -                             BUG();
>> +                     WARN_ON(test_and_clear_bit(0, &dev->hw_lock) == 0);
>>                       s5p_mfc_clock_off();
>>                       s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>>               } else {
>> --
>> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 08/14] [media] s5p-mfc: modify mfc wakeup sequence for V8

[Kiran AVND]

From: Arun Mankuzhi <arun.m@samsung.com>

>From MFC V8, the MFC wakeup sequence has changed.
MFC wakeup command has to be sent after the host receives
firmware load complete status from risc.

Signed-off-by: Arun Mankuzhi <arun.m@samsung.com>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c |   78 +++++++++++++++++++------
 1 file changed, 61 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
index 605f11e..565a6ed 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -353,6 +353,58 @@ int s5p_mfc_sleep(struct s5p_mfc_dev *dev)
 	return ret;
 }
 
+static int s5p_mfc_v8_wait_wakeup(struct s5p_mfc_dev *dev)
+{
+	int ret;
+
+	/* Release reset signal to the RISC */
+	dev->risc_on = 1;
+	mfc_write(dev, 0x1, S5P_FIMV_RISC_ON_V6);
+
+	if (s5p_mfc_wait_for_done_dev(dev, S5P_MFC_R2H_CMD_FW_STATUS_RET)) {
+		mfc_err("Failed to reset MFCV8\n");
+		return -EIO;
+	}
+	mfc_debug(2, "Write command to wakeup MFCV8\n");
+	ret = s5p_mfc_hw_call(dev->mfc_cmds, wakeup_cmd, dev);
+	if (ret) {
+		mfc_err("Failed to send command to MFCV8 - timeout\n");
+		return ret;
+	}
+
+	if (s5p_mfc_wait_for_done_dev(dev, S5P_MFC_R2H_CMD_WAKEUP_RET)) {
+		mfc_err("Failed to wakeup MFC\n");
+		return -EIO;
+	}
+	return ret;
+}
+
+static int s5p_mfc_wait_wakeup(struct s5p_mfc_dev *dev)
+{
+	int ret;
+
+	/* Send MFC wakeup command */
+	ret = s5p_mfc_hw_call(dev->mfc_cmds, wakeup_cmd, dev);
+	if (ret) {
+		mfc_err("Failed to send command to MFC - timeout\n");
+		return ret;
+	}
+
+	/* Release reset signal to the RISC */
+	if (IS_MFCV6_PLUS(dev)) {
+		dev->risc_on = 1;
+		mfc_write(dev, 0x1, S5P_FIMV_RISC_ON_V6);
+	} else {
+		mfc_write(dev, 0x3ff, S5P_FIMV_SW_RESET);
+	}
+
+	if (s5p_mfc_wait_for_done_dev(dev, S5P_MFC_R2H_CMD_WAKEUP_RET)) {
+		mfc_err("Failed to wakeup MFC\n");
+		return -EIO;
+	}
+	return ret;
+}
+
 int s5p_mfc_wakeup(struct s5p_mfc_dev *dev)
 {
 	int ret;
@@ -365,6 +417,7 @@ int s5p_mfc_wakeup(struct s5p_mfc_dev *dev)
 	ret = s5p_mfc_reset(dev);
 	if (ret) {
 		mfc_err("Failed to reset MFC - timeout\n");
+		s5p_mfc_clock_off();
 		return ret;
 	}
 	mfc_debug(2, "Done MFC reset..\n");
@@ -373,25 +426,16 @@ int s5p_mfc_wakeup(struct s5p_mfc_dev *dev)
 	/* 2. Initialize registers of channel I/F */
 	s5p_mfc_clear_cmds(dev);
 	s5p_mfc_clean_dev_int_flags(dev);
-	/* 3. Initialize firmware */
-	ret = s5p_mfc_hw_call(dev->mfc_cmds, wakeup_cmd, dev);
-	if (ret) {
-		mfc_err("Failed to send command to MFC - timeout\n");
-		return ret;
-	}
-	/* 4. Release reset signal to the RISC */
-	if (IS_MFCV6_PLUS(dev)) {
-		dev->risc_on = 1;
-		mfc_write(dev, 0x1, S5P_FIMV_RISC_ON_V6);
-	}
+	/* 3. Send MFC wakeup command and wait for completion*/
+	if (IS_MFCV8(dev))
+		ret = s5p_mfc_v8_wait_wakeup(dev);
 	else
-		mfc_write(dev, 0x3ff, S5P_FIMV_SW_RESET);
-	mfc_debug(2, "Ok, now will write a command to wakeup the system\n");
-	if (s5p_mfc_wait_for_done_dev(dev, S5P_MFC_R2H_CMD_WAKEUP_RET)) {
-		mfc_err("Failed to load firmware\n");
-		return -EIO;
-	}
+		ret = s5p_mfc_wait_wakeup(dev);
+
 	s5p_mfc_clock_off();
+	if (ret)
+		return ret;
+
 	dev->int_cond = 0;
 	if (dev->int_err != 0 || dev->int_type !=
 						S5P_MFC_R2H_CMD_WAKEUP_RET) {
-- 
1.7.9.5

[PATCH v2 09/14] [media] s5p-mfc: De-init MFC when watchdog kicks in

[Kiran AVND]

From: Arun Mankuzhi <arun.m@samsung.com>

If the software watchdog kicks in, we need to de-init MFC
before reloading firmware and re-intializing it again.

Signed-off-by: Arun Mankuzhi <arun.m@samsung.com>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 8d5da0c..21b6006 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -159,6 +159,10 @@ static void s5p_mfc_watchdog_worker(struct work_struct *work)
 	}
 	clear_bit(0, &dev->hw_lock);
 	spin_unlock_irqrestore(&dev->irqlock, flags);
+
+	/* De-init MFC */
+	s5p_mfc_deinit_hw(dev);
+
 	/* Double check if there is at least one instance running.
 	 * If no instance is in memory than no firmware should be present */
 	if (dev->num_inst > 0) {
-- 
1.7.9.5

[PATCH v2 10/14] [media] s5p-mfc: flush dpbs when resolution changes

[Kiran AVND]

While resolution change is detected by MFC, we flush out
older dpbs, send the resolution change event to application,
and then accept further queuing of new src buffers.

Sometimes, we error out during dpb flush because of lack of src
buffers. Since we have not started decoding new resolution yet,
it is simpler to push zero-size buffer until we flush out all dpbs.

This is already been done while handling EOS command, and this patch
extends the same logic to resolution change as well.

Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |   20 ++------------------
 1 file changed, 2 insertions(+), 18 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 c1c12f8..991008a 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -1537,27 +1537,11 @@ static inline int s5p_mfc_get_new_ctx(struct s5p_mfc_dev *dev)
 static inline void s5p_mfc_run_dec_last_frames(struct s5p_mfc_ctx *ctx)
 {
 	struct s5p_mfc_dev *dev = ctx->dev;
-	struct s5p_mfc_buf *temp_vb;
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->irqlock, flags);
-
-	/* Frames are being decoded */
-	if (list_empty(&ctx->src_queue)) {
-		mfc_debug(2, "No src buffers.\n");
-		spin_unlock_irqrestore(&dev->irqlock, flags);
-		return;
-	}
-	/* Get the next source buffer */
-	temp_vb = list_entry(ctx->src_queue.next, struct s5p_mfc_buf, list);
-	temp_vb->flags |= MFC_BUF_FLAG_USED;
-	s5p_mfc_set_dec_stream_buffer_v6(ctx,
-			vb2_dma_contig_plane_dma_addr(temp_vb->b, 0), 0, 0);
-	spin_unlock_irqrestore(&dev->irqlock, flags);
 
+	s5p_mfc_set_dec_stream_buffer_v6(ctx, 0, 0, 0);
 	dev->curr_ctx = ctx->num;
 	s5p_mfc_clean_ctx_int_flags(ctx);
-	s5p_mfc_decode_one_frame_v6(ctx, 1);
+	s5p_mfc_decode_one_frame_v6(ctx, MFC_DEC_LAST_FRAME);
 }
 
 static inline int s5p_mfc_run_dec_frame(struct s5p_mfc_ctx *ctx)
-- 
1.7.9.5

[PATCH v2 11/14] [media] s5p-mfc: Remove unused alloc field from private buffer struct.

[Kiran AVND]

From: Pawel Osciak <posciak@chromium.org>

This field is no longer used as MFC driver doesn't use vb2 alloc contexts
anymore.

Signed-off-by: Pawel Osciak <posciak@chromium.org>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index c0de03d..7aaa203 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -237,8 +237,6 @@ struct s5p_mfc_variant {
 
 /**
  * struct s5p_mfc_priv_buf - represents internal used buffer
- * @alloc:		allocation-specific context for each buffer
- *			(videobuf2 allocator)
  * @ofs:		offset of each buffer, will be used for MFC
  * @virt:		kernel virtual address, only valid when the
  *			buffer accessed by driver
@@ -246,7 +244,6 @@ struct s5p_mfc_variant {
  * @size:		size of the buffer
  */
 struct s5p_mfc_priv_buf {
-	void		*alloc;
 	unsigned long	ofs;
 	void		*virt;
 	dma_addr_t	dma;
-- 
1.7.9.5

[PATCH v2 12/14] [media] s5p-mfc: fix V4L2_CID_MIN_BUFFERS_FOR_CAPTURE on resolution change.

[Kiran AVND]

From: Pawel Osciak <posciak@chromium.org>

G_CTRL on V4L2_CID_MIN_BUFFERS_FOR_CAPTURE will fail if userspace happens to
query it after getting a resolution change event and before the codec has
a chance to parse the header and switch to an initialized state.

Signed-off-by: Pawel Osciak <posciak@chromium.org>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index fe4d21c..301d74f 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -756,7 +756,8 @@ static int s5p_mfc_dec_g_v_ctrl(struct v4l2_ctrl *ctrl)
 		    ctx->state < MFCINST_ABORT) {
 			ctrl->val = ctx->pb_count;
 			break;
-		} else if (ctx->state != MFCINST_INIT) {
+		} else if (ctx->state != MFCINST_INIT &&
+				ctx->state != MFCINST_RES_CHANGE_END) {
 			v4l2_err(&dev->v4l2_dev, "Decoding not initialised\n");
 			return -EINVAL;
 		}
-- 
1.7.9.5

[PATCH v2 13/14] [media] s5p-mfc: fix a race in interrupt flags handling

[Kiran AVND]

From: Pawel Osciak <posciak@chromium.org>

Interrupt result flags have to be cleared before a hardware job is run.
Otherwise, if they are cleared asynchronously, we may end up clearing them
after the interrupt for which we wanted to wait has already arrived, thus
overwriting the job results that we intended to wait for.

To prevent this, clear the flags only under hw_lock and before running
a hardware job.

Signed-off-by: Pawel Osciak <posciak@chromium.org>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   |    2 --
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |    3 ---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    |    1 -
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |   13 ++-----------
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |   12 ++----------
 5 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
index 565a6ed..ca0a5cd 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -468,7 +468,6 @@ int s5p_mfc_open_mfc_inst(struct s5p_mfc_dev *dev, struct s5p_mfc_ctx *ctx)
 	}
 
 	set_work_bit_irqsave(ctx);
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
 	if (s5p_mfc_wait_for_done_ctx(ctx,
 		S5P_MFC_R2H_CMD_OPEN_INSTANCE_RET, 0)) {
@@ -494,7 +493,6 @@ void s5p_mfc_close_mfc_inst(struct s5p_mfc_dev *dev, struct s5p_mfc_ctx *ctx)
 {
 	ctx->state = MFCINST_RETURN_INST;
 	set_work_bit_irqsave(ctx);
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
 	/* Wait until instance is returned or timeout occurred */
 	if (s5p_mfc_wait_for_done_ctx(ctx,
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 301d74f..7eef03a 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -350,7 +350,6 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 						MFCINST_RES_CHANGE_END)) {
 		/* If the MFC is parsing the header,
 		 * so wait until it is finished */
-		s5p_mfc_clean_ctx_int_flags(ctx);
 		s5p_mfc_wait_for_done_ctx(ctx, S5P_MFC_R2H_CMD_SEQ_DONE_RET,
 									0);
 	}
@@ -762,7 +761,6 @@ static int s5p_mfc_dec_g_v_ctrl(struct v4l2_ctrl *ctrl)
 			return -EINVAL;
 		}
 		/* Should wait for the header to be parsed */
-		s5p_mfc_clean_ctx_int_flags(ctx);
 		s5p_mfc_wait_for_done_ctx(ctx,
 				S5P_MFC_R2H_CMD_SEQ_DONE_RET, 0);
 		if (ctx->state >= MFCINST_HEAD_PARSED &&
@@ -1076,7 +1074,6 @@ static void s5p_mfc_stop_streaming(struct vb2_queue *q)
 		if (IS_MFCV6_PLUS(dev) && (ctx->state == MFCINST_RUNNING)) {
 			ctx->state = MFCINST_FLUSH;
 			set_work_bit_irqsave(ctx);
-			s5p_mfc_clean_ctx_int_flags(ctx);
 			s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
 			if (s5p_mfc_wait_for_done_ctx(ctx,
 				S5P_MFC_R2H_CMD_DPB_FLUSH_RET, 0))
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 653f28f..407dc63 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1679,7 +1679,6 @@ static int s5p_mfc_enc_g_v_ctrl(struct v4l2_ctrl *ctrl)
 			return -EINVAL;
 		}
 		/* Should wait for the header to be produced */
-		s5p_mfc_clean_ctx_int_flags(ctx);
 		s5p_mfc_wait_for_done_ctx(ctx,
 				S5P_MFC_R2H_CMD_SEQ_DONE_RET, 0);
 		if (ctx->state >= MFCINST_HEAD_PARSED &&
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 f882905..40a98ad 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
@@ -1178,7 +1178,6 @@ static void s5p_mfc_run_res_change(struct s5p_mfc_ctx *ctx)
 
 	s5p_mfc_set_dec_stream_buffer_v5(ctx, 0, 0, 0);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_decode_one_frame_v5(ctx, MFC_DEC_RES_CHANGE);
 }
 
@@ -1192,7 +1191,6 @@ static int s5p_mfc_run_dec_frame(struct s5p_mfc_ctx *ctx, int last_frame)
 		last_frame = MFC_DEC_LAST_FRAME;
 		s5p_mfc_set_dec_stream_buffer_v5(ctx, 0, 0, 0);
 		dev->curr_ctx = ctx->num;
-		s5p_mfc_clean_ctx_int_flags(ctx);
 		s5p_mfc_decode_one_frame_v5(ctx, last_frame);
 		return 0;
 	}
@@ -1212,7 +1210,6 @@ static int s5p_mfc_run_dec_frame(struct s5p_mfc_ctx *ctx, int last_frame)
 		ctx->consumed_stream, temp_vb->b->v4l2_planes[0].bytesused);
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	if (temp_vb->b->v4l2_planes[0].bytesused == 0) {
 		last_frame = MFC_DEC_LAST_FRAME;
 		mfc_debug(2, "Setting ctx->state to FINISHING\n");
@@ -1273,7 +1270,6 @@ static int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
 	s5p_mfc_set_enc_stream_buffer_v5(ctx, dst_addr, dst_size);
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	mfc_debug(2, "encoding buffer with index=%d state=%d\n",
 		  src_mb ? src_mb->b->v4l2_buf.index : -1, ctx->state);
 	s5p_mfc_encode_one_frame_v5(ctx);
@@ -1297,7 +1293,6 @@ static void s5p_mfc_run_init_dec(struct s5p_mfc_ctx *ctx)
 				0, temp_vb->b->v4l2_planes[0].bytesused);
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_init_decode_v5(ctx);
 }
 
@@ -1317,7 +1312,6 @@ static void s5p_mfc_run_init_enc(struct s5p_mfc_ctx *ctx)
 	s5p_mfc_set_enc_stream_buffer_v5(ctx, dst_addr, dst_size);
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_init_encode_v5(ctx);
 }
 
@@ -1352,7 +1346,6 @@ static int s5p_mfc_run_init_dec_buffers(struct s5p_mfc_ctx *ctx)
 				0, temp_vb->b->v4l2_planes[0].bytesused);
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	ret = s5p_mfc_set_dec_frame_buffer_v5(ctx);
 	if (ret) {
 		mfc_err("Failed to alloc frame mem\n");
@@ -1396,6 +1389,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
 	 * Now obtaining frames from MFC buffer
 	 */
 	s5p_mfc_clock_on();
+	s5p_mfc_clean_ctx_int_flags(ctx);
+
 	if (ctx->type == MFCINST_DECODER) {
 		s5p_mfc_set_dec_desc_buffer(ctx);
 		switch (ctx->state) {
@@ -1406,12 +1401,10 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
 			ret = s5p_mfc_run_dec_frame(ctx, MFC_DEC_FRAME);
 			break;
 		case MFCINST_INIT:
-			s5p_mfc_clean_ctx_int_flags(ctx);
 			ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
 					ctx);
 			break;
 		case MFCINST_RETURN_INST:
-			s5p_mfc_clean_ctx_int_flags(ctx);
 			ret = s5p_mfc_hw_call(dev->mfc_cmds, close_inst_cmd,
 					ctx);
 			break;
@@ -1444,12 +1437,10 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
 			ret = s5p_mfc_run_enc_frame(ctx);
 			break;
 		case MFCINST_INIT:
-			s5p_mfc_clean_ctx_int_flags(ctx);
 			ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
 					ctx);
 			break;
 		case MFCINST_RETURN_INST:
-			s5p_mfc_clean_ctx_int_flags(ctx);
 			ret = s5p_mfc_hw_call(dev->mfc_cmds, close_inst_cmd,
 					ctx);
 			break;
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 991008a..9104a75 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -1399,7 +1399,6 @@ static inline void s5p_mfc_set_flush(struct s5p_mfc_ctx *ctx, int flush)
 
 	if (flush) {
 		dev->curr_ctx = ctx->num;
-		s5p_mfc_clean_ctx_int_flags(ctx);
 		WRITEL(ctx->inst_no, mfc_regs->instance_id);
 		s5p_mfc_hw_call(dev->mfc_cmds, cmd_host2risc, dev,
 				S5P_FIMV_H2R_CMD_FLUSH_V6, NULL);
@@ -1540,7 +1539,6 @@ static inline void s5p_mfc_run_dec_last_frames(struct s5p_mfc_ctx *ctx)
 
 	s5p_mfc_set_dec_stream_buffer_v6(ctx, 0, 0, 0);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_decode_one_frame_v6(ctx, MFC_DEC_LAST_FRAME);
 }
 
@@ -1577,7 +1575,6 @@ static inline int s5p_mfc_run_dec_frame(struct s5p_mfc_ctx *ctx)
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	if (temp_vb->b->v4l2_planes[0].bytesused == 0) {
 		last_frame = 1;
 		mfc_debug(2, "Setting ctx->state to FINISHING\n");
@@ -1634,7 +1631,6 @@ static inline int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_encode_one_frame_v6(ctx);
 
 	return 0;
@@ -1656,7 +1652,6 @@ static inline void s5p_mfc_run_init_dec(struct s5p_mfc_ctx *ctx)
 			temp_vb->b->v4l2_planes[0].bytesused);
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_init_decode_v6(ctx);
 }
 
@@ -1676,7 +1671,6 @@ static inline void s5p_mfc_run_init_enc(struct s5p_mfc_ctx *ctx)
 	s5p_mfc_set_enc_stream_buffer_v6(ctx, dst_addr, dst_size);
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	s5p_mfc_init_encode_v6(ctx);
 }
 
@@ -1696,7 +1690,6 @@ static inline int s5p_mfc_run_init_dec_buffers(struct s5p_mfc_ctx *ctx)
 	}
 
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	ret = s5p_mfc_set_dec_frame_buffer_v6(ctx);
 	if (ret) {
 		mfc_err("Failed to alloc frame mem.\n");
@@ -1711,7 +1704,6 @@ static inline int s5p_mfc_run_init_enc_buffers(struct s5p_mfc_ctx *ctx)
 	int ret;
 
 	dev->curr_ctx = ctx->num;
-	s5p_mfc_clean_ctx_int_flags(ctx);
 	ret = s5p_mfc_set_enc_ref_buffer_v6(ctx);
 	if (ret) {
 		mfc_err("Failed to alloc frame mem.\n");
@@ -1760,6 +1752,8 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
 	 * Now obtaining frames from MFC buffer */
 
 	s5p_mfc_clock_on();
+	s5p_mfc_clean_ctx_int_flags(ctx);
+
 	if (ctx->type == MFCINST_DECODER) {
 		switch (ctx->state) {
 		case MFCINST_FINISHING:
@@ -1769,12 +1763,10 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
 			ret = s5p_mfc_run_dec_frame(ctx);
 			break;
 		case MFCINST_INIT:
-			s5p_mfc_clean_ctx_int_flags(ctx);
 			ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
 					ctx);
 			break;
 		case MFCINST_RETURN_INST:
-			s5p_mfc_clean_ctx_int_flags(ctx);
 			ret = s5p_mfc_hw_call(dev->mfc_cmds, close_inst_cmd,
 					ctx);
 			break;
-- 
1.7.9.5

RE: [PATCH v2 13/14] [media] s5p-mfc: fix a race in interrupt flags handling

[Kamil Debski]

Hi,

This patch does not apply to the current media tree.

commit cf3167cf1e969b17671a4d3d956d22718a8ceb85)
Author: Antti Palosaari <crope@iki.fi>
Date:   Fri Sep 26 22:45:36 2014 -0300

    [media] pt3: fix DTV FE I2C driver load error paths
    
Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

> -----Original Message-----
> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> Sent: Friday, September 26, 2014 6:52 AM
> To: linux-media@vger.kernel.org
> Cc: k.debski@samsung.com; wuchengli@chromium.org; posciak@chromium.org;
> arun.m@samsung.com; ihf@chromium.org; prathyush.k@samsung.com;
> arun.kk@samsung.com; kiran@chromium.org
> Subject: [PATCH v2 13/14] [media] s5p-mfc: fix a race in interrupt
> flags handling
> 
> From: Pawel Osciak <posciak@chromium.org>
> 
> Interrupt result flags have to be cleared before a hardware job is run.
> Otherwise, if they are cleared asynchronously, we may end up clearing
> them after the interrupt for which we wanted to wait has already
> arrived, thus overwriting the job results that we intended to wait for.
> 
> To prevent this, clear the flags only under hw_lock and before running
> a hardware job.
> 
> Signed-off-by: Pawel Osciak <posciak@chromium.org>
> Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   |    2 --
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |    3 ---
>  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    |    1 -
>  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |   13 ++-----------
>  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |   12 ++----------
>  5 files changed, 4 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
> index 565a6ed..ca0a5cd 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
> @@ -468,7 +468,6 @@ int s5p_mfc_open_mfc_inst(struct s5p_mfc_dev *dev,
> struct s5p_mfc_ctx *ctx)
>  	}
> 
>  	set_work_bit_irqsave(ctx);
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>  	if (s5p_mfc_wait_for_done_ctx(ctx,
>  		S5P_MFC_R2H_CMD_OPEN_INSTANCE_RET, 0)) { @@ -494,7 +493,6
> @@ void s5p_mfc_close_mfc_inst(struct s5p_mfc_dev *dev, struct
> s5p_mfc_ctx *ctx)  {
>  	ctx->state = MFCINST_RETURN_INST;
>  	set_work_bit_irqsave(ctx);
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>  	/* Wait until instance is returned or timeout occurred */
>  	if (s5p_mfc_wait_for_done_ctx(ctx,
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 301d74f..7eef03a 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -350,7 +350,6 @@ static int vidioc_g_fmt(struct file *file, void
> *priv, struct v4l2_format *f)
>  						MFCINST_RES_CHANGE_END)) {
>  		/* If the MFC is parsing the header,
>  		 * so wait until it is finished */
> -		s5p_mfc_clean_ctx_int_flags(ctx);
>  		s5p_mfc_wait_for_done_ctx(ctx,
> S5P_MFC_R2H_CMD_SEQ_DONE_RET,
>  									0);
>  	}
> @@ -762,7 +761,6 @@ static int s5p_mfc_dec_g_v_ctrl(struct v4l2_ctrl
> *ctrl)
>  			return -EINVAL;
>  		}
>  		/* Should wait for the header to be parsed */
> -		s5p_mfc_clean_ctx_int_flags(ctx);
>  		s5p_mfc_wait_for_done_ctx(ctx,
>  				S5P_MFC_R2H_CMD_SEQ_DONE_RET, 0);
>  		if (ctx->state >= MFCINST_HEAD_PARSED && @@ -1076,7 +1074,6
> @@ static void s5p_mfc_stop_streaming(struct vb2_queue *q)
>  		if (IS_MFCV6_PLUS(dev) && (ctx->state == MFCINST_RUNNING))
> {
>  			ctx->state = MFCINST_FLUSH;
>  			set_work_bit_irqsave(ctx);
> -			s5p_mfc_clean_ctx_int_flags(ctx);
>  			s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>  			if (s5p_mfc_wait_for_done_ctx(ctx,
>  				S5P_MFC_R2H_CMD_DPB_FLUSH_RET, 0))
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> index 653f28f..407dc63 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> @@ -1679,7 +1679,6 @@ static int s5p_mfc_enc_g_v_ctrl(struct v4l2_ctrl
> *ctrl)
>  			return -EINVAL;
>  		}
>  		/* Should wait for the header to be produced */
> -		s5p_mfc_clean_ctx_int_flags(ctx);
>  		s5p_mfc_wait_for_done_ctx(ctx,
>  				S5P_MFC_R2H_CMD_SEQ_DONE_RET, 0);
>  		if (ctx->state >= MFCINST_HEAD_PARSED && 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 f882905..40a98ad 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> @@ -1178,7 +1178,6 @@ static void s5p_mfc_run_res_change(struct
> s5p_mfc_ctx *ctx)
> 
>  	s5p_mfc_set_dec_stream_buffer_v5(ctx, 0, 0, 0);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_decode_one_frame_v5(ctx, MFC_DEC_RES_CHANGE);  }
> 
> @@ -1192,7 +1191,6 @@ static int s5p_mfc_run_dec_frame(struct
> s5p_mfc_ctx *ctx, int last_frame)
>  		last_frame = MFC_DEC_LAST_FRAME;
>  		s5p_mfc_set_dec_stream_buffer_v5(ctx, 0, 0, 0);
>  		dev->curr_ctx = ctx->num;
> -		s5p_mfc_clean_ctx_int_flags(ctx);
>  		s5p_mfc_decode_one_frame_v5(ctx, last_frame);
>  		return 0;
>  	}
> @@ -1212,7 +1210,6 @@ static int s5p_mfc_run_dec_frame(struct
> s5p_mfc_ctx *ctx, int last_frame)
>  		ctx->consumed_stream, temp_vb->b-
> >v4l2_planes[0].bytesused);
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	if (temp_vb->b->v4l2_planes[0].bytesused == 0) {
>  		last_frame = MFC_DEC_LAST_FRAME;
>  		mfc_debug(2, "Setting ctx->state to FINISHING\n"); @@ -
> 1273,7 +1270,6 @@ static int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx
> *ctx)
>  	s5p_mfc_set_enc_stream_buffer_v5(ctx, dst_addr, dst_size);
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	mfc_debug(2, "encoding buffer with index=%d state=%d\n",
>  		  src_mb ? src_mb->b->v4l2_buf.index : -1, ctx->state);
>  	s5p_mfc_encode_one_frame_v5(ctx);
> @@ -1297,7 +1293,6 @@ static void s5p_mfc_run_init_dec(struct
> s5p_mfc_ctx *ctx)
>  				0, temp_vb->b->v4l2_planes[0].bytesused);
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_init_decode_v5(ctx);
>  }
> 
> @@ -1317,7 +1312,6 @@ static void s5p_mfc_run_init_enc(struct
> s5p_mfc_ctx *ctx)
>  	s5p_mfc_set_enc_stream_buffer_v5(ctx, dst_addr, dst_size);
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_init_encode_v5(ctx);
>  }
> 
> @@ -1352,7 +1346,6 @@ static int s5p_mfc_run_init_dec_buffers(struct
> s5p_mfc_ctx *ctx)
>  				0, temp_vb->b->v4l2_planes[0].bytesused);
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	ret = s5p_mfc_set_dec_frame_buffer_v5(ctx);
>  	if (ret) {
>  		mfc_err("Failed to alloc frame mem\n"); @@ -1396,6 +1389,8
> @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
>  	 * Now obtaining frames from MFC buffer
>  	 */
>  	s5p_mfc_clock_on();
> +	s5p_mfc_clean_ctx_int_flags(ctx);
> +
>  	if (ctx->type == MFCINST_DECODER) {
>  		s5p_mfc_set_dec_desc_buffer(ctx);
>  		switch (ctx->state) {
> @@ -1406,12 +1401,10 @@ static void s5p_mfc_try_run_v5(struct
> s5p_mfc_dev *dev)
>  			ret = s5p_mfc_run_dec_frame(ctx, MFC_DEC_FRAME);
>  			break;
>  		case MFCINST_INIT:
> -			s5p_mfc_clean_ctx_int_flags(ctx);
>  			ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
>  					ctx);
>  			break;
>  		case MFCINST_RETURN_INST:
> -			s5p_mfc_clean_ctx_int_flags(ctx);
>  			ret = s5p_mfc_hw_call(dev->mfc_cmds, close_inst_cmd,
>  					ctx);
>  			break;
> @@ -1444,12 +1437,10 @@ static void s5p_mfc_try_run_v5(struct
> s5p_mfc_dev *dev)
>  			ret = s5p_mfc_run_enc_frame(ctx);
>  			break;
>  		case MFCINST_INIT:
> -			s5p_mfc_clean_ctx_int_flags(ctx);
>  			ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
>  					ctx);
>  			break;
>  		case MFCINST_RETURN_INST:
> -			s5p_mfc_clean_ctx_int_flags(ctx);
>  			ret = s5p_mfc_hw_call(dev->mfc_cmds, close_inst_cmd,
>  					ctx);
>  			break;
> 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 991008a..9104a75 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -1399,7 +1399,6 @@ static inline void s5p_mfc_set_flush(struct
> s5p_mfc_ctx *ctx, int flush)
> 
>  	if (flush) {
>  		dev->curr_ctx = ctx->num;
> -		s5p_mfc_clean_ctx_int_flags(ctx);
>  		WRITEL(ctx->inst_no, mfc_regs->instance_id);
>  		s5p_mfc_hw_call(dev->mfc_cmds, cmd_host2risc, dev,
>  				S5P_FIMV_H2R_CMD_FLUSH_V6, NULL);
> @@ -1540,7 +1539,6 @@ static inline void
> s5p_mfc_run_dec_last_frames(struct s5p_mfc_ctx *ctx)
> 
>  	s5p_mfc_set_dec_stream_buffer_v6(ctx, 0, 0, 0);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_decode_one_frame_v6(ctx, MFC_DEC_LAST_FRAME);  }
> 
> @@ -1577,7 +1575,6 @@ static inline int s5p_mfc_run_dec_frame(struct
> s5p_mfc_ctx *ctx)
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
> 
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	if (temp_vb->b->v4l2_planes[0].bytesused == 0) {
>  		last_frame = 1;
>  		mfc_debug(2, "Setting ctx->state to FINISHING\n"); @@ -
> 1634,7 +1631,6 @@ static inline int s5p_mfc_run_enc_frame(struct
> s5p_mfc_ctx *ctx)
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
> 
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_encode_one_frame_v6(ctx);
> 
>  	return 0;
> @@ -1656,7 +1652,6 @@ static inline void s5p_mfc_run_init_dec(struct
> s5p_mfc_ctx *ctx)
>  			temp_vb->b->v4l2_planes[0].bytesused);
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_init_decode_v6(ctx);
>  }
> 
> @@ -1676,7 +1671,6 @@ static inline void s5p_mfc_run_init_enc(struct
> s5p_mfc_ctx *ctx)
>  	s5p_mfc_set_enc_stream_buffer_v6(ctx, dst_addr, dst_size);
>  	spin_unlock_irqrestore(&dev->irqlock, flags);
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	s5p_mfc_init_encode_v6(ctx);
>  }
> 
> @@ -1696,7 +1690,6 @@ static inline int
> s5p_mfc_run_init_dec_buffers(struct s5p_mfc_ctx *ctx)
>  	}
> 
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	ret = s5p_mfc_set_dec_frame_buffer_v6(ctx);
>  	if (ret) {
>  		mfc_err("Failed to alloc frame mem.\n"); @@ -1711,7 +1704,6
> @@ static inline int s5p_mfc_run_init_enc_buffers(struct s5p_mfc_ctx
> *ctx)
>  	int ret;
> 
>  	dev->curr_ctx = ctx->num;
> -	s5p_mfc_clean_ctx_int_flags(ctx);
>  	ret = s5p_mfc_set_enc_ref_buffer_v6(ctx);
>  	if (ret) {
>  		mfc_err("Failed to alloc frame mem.\n"); @@ -1760,6 +1752,8
> @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
>  	 * Now obtaining frames from MFC buffer */
> 
>  	s5p_mfc_clock_on();
> +	s5p_mfc_clean_ctx_int_flags(ctx);
> +
>  	if (ctx->type == MFCINST_DECODER) {
>  		switch (ctx->state) {
>  		case MFCINST_FINISHING:
> @@ -1769,12 +1763,10 @@ static void s5p_mfc_try_run_v6(struct
> s5p_mfc_dev *dev)
>  			ret = s5p_mfc_run_dec_frame(ctx);
>  			break;
>  		case MFCINST_INIT:
> -			s5p_mfc_clean_ctx_int_flags(ctx);
>  			ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
>  					ctx);
>  			break;
>  		case MFCINST_RETURN_INST:
> -			s5p_mfc_clean_ctx_int_flags(ctx);
>  			ret = s5p_mfc_hw_call(dev->mfc_cmds, close_inst_cmd,
>  					ctx);
>  			break;
> --
> 1.7.9.5

[PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Kiran AVND]

From: Wu-Cheng Li <wuchengli@chromium.org>

Use the requested size as the minimum bound, unless it's less than the
required hardware minimum. The bound align function will align to the
closest value but we do not want to adjust below the requested size.

Signed-off-by: Wu-Cheng Li <wuchengli@chromium.org>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 407dc63..7b48180 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1056,6 +1056,7 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	struct s5p_mfc_dev *dev = video_drvdata(file);
 	struct s5p_mfc_fmt *fmt;
 	struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp;
+	u32 min_w, min_h;
 
 	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
 		fmt = find_format(f, MFC_FMT_ENC);
@@ -1090,8 +1091,16 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 			return -EINVAL;
 		}
 
-		v4l_bound_align_image(&pix_fmt_mp->width, 8, 1920, 1,
-			&pix_fmt_mp->height, 4, 1080, 1, 0);
+		/*
+		 * Use the requested size as the minimum bound, unless it's less
+		 * than the required hardware minimum. The bound align function
+		 * will align to the closest value but we do not want to adjust
+		 * below the requested size.
+		 */
+		min_w = min(max(16u, pix_fmt_mp->width), 1920u);
+		min_h = min(max(16u, pix_fmt_mp->height), 1088u);
+		v4l_bound_align_image(&pix_fmt_mp->width, min_w, 1920, 4,
+			&pix_fmt_mp->height, min_h, 1088, 4, 0);
 	} else {
 		mfc_err("invalid buf type\n");
 		return -EINVAL;
-- 
1.7.9.5

RE: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Kamil Debski]

Hi,

This patch seems complicated and I do not understand your motives.

Could you explain what is the problem with the current aligning of the
values?
Is this a hardware problem? Which MFC version does it affect?
Is it a software problem? If so, maybe the user space application should
take extra care on what value it passes/receives to try_fmt?

> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> Sent: Friday, September 26, 2014 6:52 AM
> To: linux-media@vger.kernel.org
> Cc: k.debski@samsung.com; wuchengli@chromium.org; posciak@chromium.org;
> arun.m@samsung.com; ihf@chromium.org; prathyush.k@samsung.com;
> arun.kk@samsung.com; kiran@chromium.org
> Subject: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size
> to smaller than the request.
> 
> From: Wu-Cheng Li <wuchengli@chromium.org>
> 
> Use the requested size as the minimum bound, unless it's less than the
> required hardware minimum. The bound align function will align to the
> closest value but we do not want to adjust below the requested size.

This patch does also change the alignment. This is not mentioned in the
commit
message (!). It was 2, now it enforces 16. Could you justify this?
If I remember correctly having even number was enough for MFC v5 encoder
to work properly.

> Signed-off-by: Wu-Cheng Li <wuchengli@chromium.org>
> Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> index 407dc63..7b48180 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> @@ -1056,6 +1056,7 @@ static int vidioc_try_fmt(struct file *file, void
> *priv, struct v4l2_format *f)
>  	struct s5p_mfc_dev *dev = video_drvdata(file);
>  	struct s5p_mfc_fmt *fmt;
>  	struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp;
> +	u32 min_w, min_h;
> 
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>  		fmt = find_format(f, MFC_FMT_ENC);
> @@ -1090,8 +1091,16 @@ static int vidioc_try_fmt(struct file *file,
> void *priv, struct v4l2_format *f)
>  			return -EINVAL;
>  		}
> 
> -		v4l_bound_align_image(&pix_fmt_mp->width, 8, 1920, 1,
> -			&pix_fmt_mp->height, 4, 1080, 1, 0);
> +		/*
> +		 * Use the requested size as the minimum bound, unless it's
> less
> +		 * than the required hardware minimum. The bound align
> function
> +		 * will align to the closest value but we do not want to
> adjust
> +		 * below the requested size.

Other drivers use v4l2_bound_align and user space apps can cope with
the driver returning a value that is below the requested value.

> +		 */
> +		min_w = min(max(16u, pix_fmt_mp->width), 1920u);
> +		min_h = min(max(16u, pix_fmt_mp->height), 1088u);
> +		v4l_bound_align_image(&pix_fmt_mp->width, min_w, 1920, 4,
> +			&pix_fmt_mp->height, min_h, 1088, 4, 0);
>  	} else {
>  		mfc_err("invalid buf type\n");
>  		return -EINVAL;
> --
> 1.7.9.5


Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Nicolas Dufresne]

Le 2014-10-08 06:24, Kamil Debski a écrit :
> Hi,
>
> This patch seems complicated and I do not understand your motives.
>
> Could you explain what is the problem with the current aligning of the
> values?
> Is this a hardware problem? Which MFC version does it affect?
> Is it a software problem? If so, maybe the user space application should
> take extra care on what value it passes/receives to try_fmt?
This looks like something I wanted to bring here as an RFC but never 
manage to get the time. In an Odroid Integration we have started using 
the following simple patch to work around this:

https://github.com/dsd/linux-odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6

The context is that right now we have decided that alignment in s_fmt 
shall be done with a closest rounding. So the format returned may be 
bigger, or smaller, that's basically random. I've been digging through a 
lot, and so far I have found no rational that explains this choice other 
that this felt right.

In real life, whenever the resulting format is smaller then request, 
there is little we can do other then fail or try again blindly other 
sizes. But with bigger raw buffers, we can use zero-copy  cropping 
techniques to keep going. Here's a example:

image_generator -> hw_converter -> display

As hw_converter is a V4L2 M2M, an ideal use case here would be for 
image_generator to use buffers from the hw_converter. For the scenario, 
it is likely that a fixed video size is wanted, but this size is also 
likely not to match HW requirement. If hw_converter decide to give back 
something smaller, there is nothing image_generator can do. It would 
have to try again with random size to find out that best match. It's a 
bit silly to force that on application, as the hw_converter know the 
closest best match, which is simply the next valid bigger size if that 
exist.

hope that helps,
Nicolas

RE: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Kamil Debski]

Hi,

> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Wednesday, October 08, 2014 4:35 PM
> 
> 
> Le 2014-10-08 06:24, Kamil Debski a écrit :
> > Hi,
> >
> > This patch seems complicated and I do not understand your motives.
> >
> > Could you explain what is the problem with the current aligning of
> the
> > values?
> > Is this a hardware problem? Which MFC version does it affect?
> > Is it a software problem? If so, maybe the user space application
> should
> > take extra care on what value it passes/receives to try_fmt?
> This looks like something I wanted to bring here as an RFC but never
> manage to get the time. In an Odroid Integration we have started using
> the following simple patch to work around this:
> 
> https://github.com/dsd/linux-
> odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6
> 
> The context is that right now we have decided that alignment in s_fmt
> shall be done with a closest rounding. So the format returned may be
> bigger, or smaller, that's basically random. I've been digging through
> a
> lot, and so far I have found no rational that explains this choice
> other
> that this felt right.
> 
> In real life, whenever the resulting format is smaller then request,
> there is little we can do other then fail or try again blindly other
> sizes. But with bigger raw buffers, we can use zero-copy  cropping
> techniques to keep going. Here's a example:
> 
> image_generator -> hw_converter -> display
> 
> As hw_converter is a V4L2 M2M, an ideal use case here would be for
> image_generator to use buffers from the hw_converter. For the scenario,
> it is likely that a fixed video size is wanted, but this size is also
> likely not to match HW requirement. If hw_converter decide to give back
> something smaller, there is nothing image_generator can do. It would
> have to try again with random size to find out that best match. It's a
> bit silly to force that on application, as the hw_converter know the
> closest best match, which is simply the next valid bigger size if that
> exist.
> 
> hope that helps,
> Nicolas

Nicolas, thank you for shedding light on this problem. I see that it is
not MFC specific. It seems that the problem applies to all Video4Linux2
devices that use v4l_bound_align_image. I agree with you that this deservers
an RFC and some discussion as this would change the behaviour of quite
a few drivers. 

I think the documentation does not specify how TRY_FMT/S_FMT should adjust
the parameters. Maybe it would a good idea to add some flagS that determine
the behaviour?

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Nicolas Dufresne]

Le 2014-10-09 06:06, Kamil Debski a écrit :
> I think the documentation does not specify how TRY_FMT/S_FMT should adjust
> the parameters. Maybe it would a good idea to add some flagS that determine
> the behaviour?
A flag could be a good option, maybe we should take a minute and discuss 
this next week.

cheers,
Nicolas

Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Hans Verkuil]

Hi,

Let me chime in as well, based on the discussions I had last week in
Düsseldorf:

On 10/09/2014 12:06 PM, Kamil Debski wrote:
> Hi,
>
>> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
>> Sent: Wednesday, October 08, 2014 4:35 PM
>>
>>
>> Le 2014-10-08 06:24, Kamil Debski a écrit :
>>> Hi,
>>>
>>> This patch seems complicated and I do not understand your motives.
>>>
>>> Could you explain what is the problem with the current aligning of
>> the
>>> values?
>>> Is this a hardware problem? Which MFC version does it affect?
>>> Is it a software problem? If so, maybe the user space application
>> should
>>> take extra care on what value it passes/receives to try_fmt?
>> This looks like something I wanted to bring here as an RFC but never
>> manage to get the time. In an Odroid Integration we have started using
>> the following simple patch to work around this:
>>
>> https://github.com/dsd/linux-
>> odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6
>>
>> The context is that right now we have decided that alignment in s_fmt
>> shall be done with a closest rounding. So the format returned may be
>> bigger, or smaller, that's basically random. I've been digging through
>> a
>> lot, and so far I have found no rational that explains this choice
>> other
>> that this felt right.
>>
>> In real life, whenever the resulting format is smaller then request,
>> there is little we can do other then fail or try again blindly other
>> sizes. But with bigger raw buffers, we can use zero-copy  cropping
>> techniques to keep going. Here's a example:
>>
>> image_generator -> hw_converter -> display
>>
>> As hw_converter is a V4L2 M2M, an ideal use case here would be for
>> image_generator to use buffers from the hw_converter. For the scenario,
>> it is likely that a fixed video size is wanted, but this size is also
>> likely not to match HW requirement. If hw_converter decide to give back
>> something smaller, there is nothing image_generator can do. It would
>> have to try again with random size to find out that best match. It's a
>> bit silly to force that on application, as the hw_converter know the
>> closest best match, which is simply the next valid bigger size if that
>> exist.
>>
>> hope that helps,
>> Nicolas
>
> Nicolas, thank you for shedding light on this problem. I see that it is
> not MFC specific. It seems that the problem applies to all Video4Linux2
> devices that use v4l_bound_align_image. I agree with you that this deservers
> an RFC and some discussion as this would change the behaviour of quite
> a few drivers.
>
> I think the documentation does not specify how TRY_FMT/S_FMT should adjust
> the parameters. Maybe it would a good idea to add some flagS that determine
> the behaviour?

I think we should add flags here as well. NEAREST (the default), ROUND_DOWN and
ROUND_UP. Existing calls will use NEAREST. I can think of use-cases for all
three of these, and I think the caller should just have to specify what is
needed.

Just replacing the algorithm used seems asking for problems, you want to be
able to select what you want to do.

Regards,

	Hans

Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Nicolas Dufresne]

Le 2014-10-21 07:34, Hans Verkuil a écrit :
>
> I think we should add flags here as well. NEAREST (the default), 
> ROUND_DOWN and
> ROUND_UP. Existing calls will use NEAREST. I can think of use-cases 
> for all
> three of these, and I think the caller should just have to specify 
> what is
> needed.
>
> Just replacing the algorithm used seems asking for problems, you want 
> to be
> able to select what you want to do. 

One more thing, we realize that in selection scenario, we do want 
nearest or lowest, so indeed a flag that let user space choose is the best.

Nicolas

RE: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Kamil Debski]

Hi,

> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Tuesday, October 21, 2014 1:35 PM
> 
> Hi,
> 
> Let me chime in as well, based on the discussions I had last week in
> Düsseldorf:
> 
> On 10/09/2014 12:06 PM, Kamil Debski wrote:
> > Hi,
> >
> >> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> >> Sent: Wednesday, October 08, 2014 4:35 PM
> >>
> >>
> >> Le 2014-10-08 06:24, Kamil Debski a écrit :
> >>> Hi,
> >>>
> >>> This patch seems complicated and I do not understand your motives.
> >>>
> >>> Could you explain what is the problem with the current aligning of
> >> the
> >>> values?
> >>> Is this a hardware problem? Which MFC version does it affect?
> >>> Is it a software problem? If so, maybe the user space application
> >> should
> >>> take extra care on what value it passes/receives to try_fmt?
> >> This looks like something I wanted to bring here as an RFC but never
> >> manage to get the time. In an Odroid Integration we have started
> using
> >> the following simple patch to work around this:
> >>
> >> https://github.com/dsd/linux-
> >> odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6
> >>
> >> The context is that right now we have decided that alignment in
> s_fmt
> >> shall be done with a closest rounding. So the format returned may be
> >> bigger, or smaller, that's basically random. I've been digging
> through
> >> a
> >> lot, and so far I have found no rational that explains this choice
> >> other
> >> that this felt right.
> >>
> >> In real life, whenever the resulting format is smaller then request,
> >> there is little we can do other then fail or try again blindly other
> >> sizes. But with bigger raw buffers, we can use zero-copy  cropping
> >> techniques to keep going. Here's a example:
> >>
> >> image_generator -> hw_converter -> display
> >>
> >> As hw_converter is a V4L2 M2M, an ideal use case here would be for
> >> image_generator to use buffers from the hw_converter. For the
> scenario,
> >> it is likely that a fixed video size is wanted, but this size is
> also
> >> likely not to match HW requirement. If hw_converter decide to give
> back
> >> something smaller, there is nothing image_generator can do. It would
> >> have to try again with random size to find out that best match. It's
> a
> >> bit silly to force that on application, as the hw_converter know the
> >> closest best match, which is simply the next valid bigger size if
> that
> >> exist.
> >>
> >> hope that helps,
> >> Nicolas
> >
> > Nicolas, thank you for shedding light on this problem. I see that it
> is
> > not MFC specific. It seems that the problem applies to all
> Video4Linux2
> > devices that use v4l_bound_align_image. I agree with you that this
> deservers
> > an RFC and some discussion as this would change the behaviour of
> quite
> > a few drivers.
> >
> > I think the documentation does not specify how TRY_FMT/S_FMT should
> adjust
> > the parameters. Maybe it would a good idea to add some flagS that
> determine
> > the behaviour?
> 
> I think we should add flags here as well. NEAREST (the default),
> ROUND_DOWN and
> ROUND_UP. Existing calls will use NEAREST. I can think of use-cases for
> all
> three of these, and I think the caller should just have to specify what
> is
> needed.
> 
> Just replacing the algorithm used seems asking for problems, you want
> to be
> able to select what you want to do.

I agree with Hans here. Nicolas, Pawel, I understand your problem with
the nearest value behaviour and I think this should be resolved right.

I think the flags could be added to the flags field of v4l2_pix_format (and
its multiplane counterpart). Something along the lines:
V4L2_PIX_FMT_FLAG_ALIGN_GE, V4L2_PIX_FMT_FLAG_ALIGN_LE (akin to the flags
used in the selection API).

The function v4l2_bound_align could be then modified to take one more
argument and act accordingly. No flags set would mean the current behaviour,
and when flags are set it would either round up, down or return an error
if the exact value cannot be used.

What do you think?

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland