[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(-)

[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 <***@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,

[Kiran AVND]

From: Pawel Osciak <***@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 <***@chromium.org>
Signed-off-by: Kiran AVND <***@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);

[Kiran AVND]

From: 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 <***@samsung.com>
Signed-off-by: Kiran AVND <***@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)

[Kiran AVND]

From: Ilja Friedel <***@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 <***@google.com>
Signed-off-by: Kiran AVND <***@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;
}

[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 <***@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");

[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 <***@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);

[Kiran AVND]

From: Pawel Osciak <***@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 <***@chromium.org>
Signed-off-by: Kiran AVND <***@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 {

[Kamil Debski]

Hi,

This patch does not apply to the current media tree.

commit cf3167cf1e969b17671a4d3d956d22718a8ceb85)
Author: Antti Palosaari <***@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-----
Sent: Friday, September 26, 2014 6:52 AM
Subject: [PATCH v2 07/14] [media] s5p-mfc: Don't crash the kernel if
the watchdog kicks in.
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.
---
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;
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

[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

Post by Kamil Debski
Hi,
This patch does not apply to the current media tree.
commit cf3167cf1e969b17671a4d3d956d22718a8ceb85)
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-----
Sent: Friday, September 26, 2014 6:52 AM
Subject: [PATCH v2 07/14] [media] s5p-mfc: Don't crash the kernel if
the watchdog kicks in.
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.
---
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;
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
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Kiran AVND]

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 <***@samsung.com>
Signed-off-by: Kiran AVND <***@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) {

[Kiran AVND]

From: Arun Mankuzhi <***@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 <***@samsung.com>
Signed-off-by: Kiran AVND <***@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) {

[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 <***@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)

[Kiran AVND]

From: Pawel Osciak <***@chromium.org>

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

Signed-off-by: Pawel Osciak <***@chromium.org>
Signed-off-by: Kiran AVND <***@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;

[Kiran AVND]

From: Pawel Osciak <***@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 <***@chromium.org>
Signed-off-by: Kiran AVND <***@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;
}

[Kiran AVND]

From: Pawel Osciak <***@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 <***@chromium.org>
Signed-off-by: Kiran AVND <***@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;

[Kamil Debski]

Hi,

This patch does not apply to the current media tree.

commit cf3167cf1e969b17671a4d3d956d22718a8ceb85)
Author: Antti Palosaari <***@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-----
Sent: Friday, September 26, 2014 6:52 AM
Subject: [PATCH v2 13/14] [media] s5p-mfc: fix a race in interrupt
flags handling
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.
---
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,
@@ 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);
@@ 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-

Post by Kiran AVND
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;
*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) {
@@ 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;
- s5p_mfc_clean_ctx_int_flags(ctx);
ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
ctx);
break;
- 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;
- s5p_mfc_clean_ctx_int_flags(ctx);
ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
ctx);
break;
- 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;
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) {
@@ 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) {
@@ 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) {
@@ -1769,12 +1763,10 @@ static void s5p_mfc_try_run_v6(struct
s5p_mfc_dev *dev)
ret = s5p_mfc_run_dec_frame(ctx);
break;
- s5p_mfc_clean_ctx_int_flags(ctx);
ret = s5p_mfc_hw_call(dev->mfc_cmds, open_inst_cmd,
ctx);
break;
- s5p_mfc_clean_ctx_int_flags(ctx);
ret = s5p_mfc_hw_call(dev->mfc_cmds, close_inst_cmd,
ctx);
break;
--
1.7.9.5

[Kiran AVND]

From: Wu-Cheng Li <***@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 <***@chromium.org>
Signed-off-by: Kiran AVND <***@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;

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

Sent: Friday, September 26, 2014 6:52 AM
Subject: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size
to smaller than the request.
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.

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

[Nicolas Dufresne]

Post by 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 th=

e

Post by Kamil Debski
values?
Is this a hardware problem? Which MFC version does it affect?
Is it a software problem? If so, maybe the user space application sho=

uld

Post by Kamil Debski
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=20
manage to get the time. In an Odroid Integration we have started using=20
the following simple patch to work around this:

https://github.com/dsd/linux-odroid/commit/c76b38c1d682b9870ea3b00093ad=
6500a9c5f5f6

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

In real life, whenever the resulting format is smaller then request,=20
there is little we can do other then fail or try again blindly other=20
sizes. But with bigger raw buffers, we can use zero-copy cropping=20
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=20
image_generator to use buffers from the hw_converter. For the scenario,=
=20
it is likely that a fixed video size is wanted, but this size is also=20
likely not to match HW requirement. If hw_converter decide to give back=
=20
something smaller, there is nothing image_generator can do. It would=20
have to try again with random size to find out that best match. It's a=20
bit silly to force that on application, as the hw_converter know the=20
closest best match, which is simply the next valid bigger size if that=20
exist.

hope that helps,
Nicolas

[Kamil Debski]

Hi,

Sent: Wednesday, October 08, 2014 4:35 PM
=20
=20

Post by 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

Post by Kamil Debski
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

Post by Kamil Debski
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 usin=

g

=20
https://github.com/dsd/linux-
odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6
=20
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 throug=

h

a
lot, and so far I have found no rational that explains this choice
other
that this felt right.
=20
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
=20
image_generator -> hw_converter -> display
=20
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 scenari=

o,

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 ba=

ck

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 tha=

t

exist.
=20
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 dese=
rvers
an RFC and some discussion as this would change the behaviour of quite
a few drivers.=20

I think the documentation does not specify how TRY_FMT/S_FMT should adj=
ust
the parameters. Maybe it would a good idea to add some flagS that deter=
mine
the behaviour?

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

[Nicolas Dufresne]

I think the documentation does not specify how TRY_FMT/S_FMT should a=

djust

the parameters. Maybe it would a good idea to add some flagS that det=

ermine

the behaviour?

A flag could be a good option, maybe we should take a minute and discus=
s=20
this next week.

cheers,
Nicolas

[Hans Verkuil]

Hi,

Let me chime in as well, based on the discussions I had last week in

Hi,

Sent: Wednesday, October 08, 2014 4:35 PM

Post by 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

Post by Kamil Debski
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

Post by Kamil Debski
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 usi=

ng

https://github.com/dsd/linux-
odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6
The context is that right now we have decided that alignment in s_fm=

t

shall be done with a closest rounding. So the format returned may be
bigger, or smaller, that's basically random. I've been digging throu=

gh

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
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 scenar=

io,

it is likely that a fixed video size is wanted, but this size is als=

o

likely not to match HW requirement. If hw_converter decide to give b=

ack

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 th=

at

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 Video4Linu=

x2

devices that use v4l_bound_align_image. I agree with you that this de=

servers

an RFC and some discussion as this would change the behaviour of quit=

e

a few drivers.
I think the documentation does not specify how TRY_FMT/S_FMT should a=

djust

the parameters. Maybe it would a good idea to add some flagS that det=

ermine

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 t=
o be
able to select what you want to do.

Regards,

Hans

[Nicolas Dufresne]

I think we should add flags here as well. NEAREST (the default),=20
ROUND_DOWN and
ROUND_UP. Existing calls will use NEAREST. I can think of use-cases=20
for all
three of these, and I think the caller should just have to specify=20
what is
needed.
Just replacing the algorithm used seems asking for problems, you want=

=20

to be
able to select what you want to do.=20

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

Nicolas

[Kamil Debski]

Hi,

Sent: Tuesday, October 21, 2014 1:35 PM
=20
Hi,
=20
Let me chime in as well, based on the discussions I had last week in
=20

Hi,

Sent: Wednesday, October 08, 2014 4:35 PM

Hi,
This patch seems complicated and I do not understand your motives=

=2E

Could you explain what is the problem with the current aligning o=

f

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 nev=

er

manage to get the time. In an Odroid Integration we have started

using

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 reques=

t,

there is little we can do other then fail or try again blindly oth=

er

sizes. But with bigger raw buffers, we can use zero-copy cropping
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 wou=

ld

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 t=

he

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 i=

t

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?

=20
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 f=

or

all
three of these, and I think the caller should just have to specify wh=

at

is
needed.
=20
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 fla=
gs
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 behav=
iour,
and when flags are set it would either round up, down or return an erro=
r
if the exact value cannot be used.

What do you think?

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