* [PATCH 01/17] [media] s5p-mfc: support MIN_BUFFERS query for encoder
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
@ 2014-09-15 6:42 ` Kiran AVND
2014-09-15 6:42 ` [PATCH 02/17] [media] s5p-mfc: Fix REQBUFS(0) " Kiran AVND
` (15 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:42 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index d26b248..ecd2bd1 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)
@@ -1643,8 +1653,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 02/17] [media] s5p-mfc: Fix REQBUFS(0) for encoder.
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
2014-09-15 6:42 ` [PATCH 01/17] [media] s5p-mfc: support MIN_BUFFERS query for encoder Kiran AVND
@ 2014-09-15 6:42 ` Kiran AVND
2014-09-15 6:42 ` [PATCH 03/17] [media] s5p-mfc: set B-frames as 2 while encoding Kiran AVND
` (14 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:42 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index ecd2bd1..cd1b2a2 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1166,6 +1166,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);
@@ -1187,6 +1192,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 03/17] [media] s5p-mfc: set B-frames as 2 while encoding
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
2014-09-15 6:42 ` [PATCH 01/17] [media] s5p-mfc: support MIN_BUFFERS query for encoder Kiran AVND
2014-09-15 6:42 ` [PATCH 02/17] [media] s5p-mfc: Fix REQBUFS(0) " Kiran AVND
@ 2014-09-15 6:42 ` Kiran AVND
2014-09-15 14:42 ` Kamil Debski
2014-09-15 6:42 ` [PATCH 04/17] [media] s5p-mfc: clear 'enter_suspend' flag if suspend fails Kiran AVND
` (13 subsequent siblings)
16 siblings, 1 reply; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:42 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
set default number of B-frames as 2 while encoding
for better compression. This User can however change
this setting using V4L2_CID_MPEG_VIDEO_B_FRAMES.
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index cd1b2a2..f7a6f68 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -280,7 +280,7 @@ static struct mfc_control controls[] = {
.minimum = 0,
.maximum = 2,
.step = 1,
- .default_value = 0,
+ .default_value = 2,
},
{
.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
--
1.7.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* RE: [PATCH 03/17] [media] s5p-mfc: set B-frames as 2 while encoding
2014-09-15 6:42 ` [PATCH 03/17] [media] s5p-mfc: set B-frames as 2 while encoding Kiran AVND
@ 2014-09-15 14:42 ` Kamil Debski
[not found] ` <CAFHkUjjjK4TOSfYj=xS12LPcUAusKrMaDG1WYiVegG+B9K6w3A@mail.gmail.com>
0 siblings, 1 reply; 27+ messages in thread
From: Kamil Debski @ 2014-09-15 14:42 UTC (permalink / raw)
To: 'Kiran AVND', linux-media
Cc: wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
Hi,
> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> Sent: Monday, September 15, 2014 8:43 AM
>
> set default number of B-frames as 2 while encoding for better
> compression. This User can however change this setting using
> V4L2_CID_MPEG_VIDEO_B_FRAMES.
The last sentence should be rephrased, as it is not clear.
The tougher question is what should be the default?
In my opinion the default should produce a stream that is as
simple as it gets. It should be playable on most hardware,
some of which may not support B frames.
Anyway - this setting can be changed by the user to 2 using
V4L2_CID_MPEG_VIDEO_B_FRAMES ;)
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
>
> Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> index cd1b2a2..f7a6f68 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> @@ -280,7 +280,7 @@ static struct mfc_control controls[] = {
> .minimum = 0,
> .maximum = 2,
> .step = 1,
> - .default_value = 0,
> + .default_value = 2,
> },
> {
> .id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> --
> 1.7.3.rc2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 04/17] [media] s5p-mfc: clear 'enter_suspend' flag if suspend fails
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (2 preceding siblings ...)
2014-09-15 6:42 ` [PATCH 03/17] [media] s5p-mfc: set B-frames as 2 while encoding Kiran AVND
@ 2014-09-15 6:42 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 05/17] [media] s5p-mfc: don't disable clock when next ctx is pending Kiran AVND
` (12 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:42 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index ebfe381..e37fb99 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1312,11 +1312,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 05/17] [media] s5p-mfc: don't disable clock when next ctx is pending
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (3 preceding siblings ...)
2014-09-15 6:42 ` [PATCH 04/17] [media] s5p-mfc: clear 'enter_suspend' flag if suspend fails Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-16 14:20 ` Kamil Debski
2014-09-15 6:43 ` [PATCH 06/17] [media] s5p-mfc: Only set timestamp/timecode for new frames Kiran AVND
` (11 subsequent siblings)
16 siblings, 1 reply; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
From: Arun Mankuzhi <arun.m@samsung.com>
In MFC's dynamic clock gating, we turn on the clock in try_run and
turn off the clock upon receiving an interrupt. But this
leads to excessive gating/ungating of the clocks in the case of
multi-instance video playback. The clock gets disabled in ctx1's irq
and then immediately turned on for ctx2's try_run.
A better solution is to turn off the clocks only when there are no new
frames to be processed in any context. This is done by conditionally
clocking on/off calls in try-run. clock-off is done outside try-run
only for suspend and error scenarios.
Signed-off-by: Prathyush K <prathyush.k@samsung.com>
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 | 26 ++++++++--------------
drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 +
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c | 11 ++++++++-
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 ++++++++++++-
4 files changed, 35 insertions(+), 20 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index e37fb99..9df130b 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -150,7 +150,8 @@ static void s5p_mfc_watchdog_worker(struct work_struct *work)
mfc_err("Error: some instance may be closing/opening\n");
spin_lock_irqsave(&dev->irqlock, flags);
- s5p_mfc_clock_off();
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
for (i = 0; i < MFC_NUM_CONTEXTS; i++) {
ctx = dev->ctx[i];
@@ -174,7 +175,6 @@ static void s5p_mfc_watchdog_worker(struct work_struct *work)
mfc_err("Failed to reload FW\n");
goto unlock;
}
- s5p_mfc_clock_on();
ret = s5p_mfc_init_hw(dev);
if (ret)
mfc_err("Failed to reinit FW\n");
@@ -338,7 +338,6 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx *ctx,
wake_up_ctx(ctx, reason, err);
if (test_and_clear_bit(0, &dev->hw_lock) == 0)
BUG();
- s5p_mfc_clock_off();
s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
return;
}
@@ -411,12 +410,11 @@ leave_handle_frame:
wake_up_ctx(ctx, reason, err);
if (test_and_clear_bit(0, &dev->hw_lock) == 0)
BUG();
- s5p_mfc_clock_off();
- /* if suspending, wake up device and do not try_run again*/
+ /* if suspending, wake up device*/
if (test_bit(0, &dev->enter_suspend))
wake_up_dev(dev, reason, err);
- else
- s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
+
+ s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
}
/* Error handling for interrupt */
@@ -460,7 +458,8 @@ static void s5p_mfc_handle_error(struct s5p_mfc_dev *dev,
if (test_and_clear_bit(0, &dev->hw_lock) == 0)
BUG();
s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
- s5p_mfc_clock_off();
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
wake_up_dev(dev, reason, err);
return;
}
@@ -514,7 +513,6 @@ static void s5p_mfc_handle_seq_done(struct s5p_mfc_ctx *ctx,
clear_work_bit(ctx);
if (test_and_clear_bit(0, &dev->hw_lock) == 0)
BUG();
- s5p_mfc_clock_off();
s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
wake_up_ctx(ctx, reason, err);
}
@@ -554,15 +552,14 @@ static void s5p_mfc_handle_init_buffers(struct s5p_mfc_ctx *ctx,
if (test_and_clear_bit(0, &dev->hw_lock) == 0)
BUG();
- 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();
- s5p_mfc_clock_off();
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
wake_up(&ctx->queue);
}
@@ -596,7 +593,6 @@ static void s5p_mfc_handle_stream_complete(struct s5p_mfc_ctx *ctx,
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);
}
@@ -639,7 +635,6 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
wake_up_ctx(ctx, reason, err);
if (test_and_clear_bit(0, &dev->hw_lock) == 0)
BUG();
- s5p_mfc_clock_off();
s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
} else {
s5p_mfc_handle_frame(ctx, reason, err);
@@ -704,8 +699,6 @@ irq_cleanup_hw:
if (test_and_clear_bit(0, &dev->hw_lock) == 0)
mfc_err("Failed to unlock hw\n");
- s5p_mfc_clock_off();
-
s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
mfc_debug(2, "Exit via irq_cleanup_hw\n");
return IRQ_HANDLED;
@@ -1216,6 +1209,7 @@ static int s5p_mfc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, dev);
dev->hw_lock = 0;
+ dev->clk_flag = 0;
dev->watchdog_workqueue = create_singlethread_workqueue(S5P_MFC_NAME);
INIT_WORK(&dev->watchdog_work, s5p_mfc_watchdog_worker);
atomic_set(&dev->watchdog_cnt, 0);
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 01816ff..92f596e 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -289,6 +289,7 @@ struct s5p_mfc_priv_buf {
* @watchdog_workqueue: workqueue for the watchdog
* @watchdog_work: worker for the watchdog
* @alloc_ctx: videobuf2 allocator contexts for two memory banks
+ * @clk_flag: flag used for dynamic control of mfc clock
* @enter_suspend: flag set when entering suspend
* @ctx_buf: common context memory (MFCv6)
* @warn_start: hardware error code from which warnings start
@@ -332,6 +333,7 @@ struct s5p_mfc_dev {
struct workqueue_struct *watchdog_workqueue;
struct work_struct watchdog_work;
void *alloc_ctx[2];
+ unsigned long clk_flag;
unsigned long enter_suspend;
struct s5p_mfc_priv_buf ctx_buf;
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 58ec7bb..e2b2f31 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
@@ -1371,6 +1371,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
if (test_bit(0, &dev->enter_suspend)) {
mfc_debug(1, "Entering suspend so do not schedule any jobs\n");
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
return;
}
/* Check whether hardware is not running */
@@ -1383,6 +1385,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
new_ctx = s5p_mfc_get_new_ctx(dev);
if (new_ctx < 0) {
/* No contexts to run */
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
mfc_err("Failed to unlock hardware\n");
return;
@@ -1396,7 +1400,9 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
* Last frame has already been sent to MFC.
* Now obtaining frames from MFC buffer
*/
- s5p_mfc_clock_on();
+ if (test_and_set_bit(0, &dev->clk_flag) == 0)
+ s5p_mfc_clock_on();
+
if (ctx->type == MFCINST_DECODER) {
s5p_mfc_set_dec_desc_buffer(ctx);
switch (ctx->state) {
@@ -1474,7 +1480,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
* scheduled, reduce the clock count as no one will
* ever do this, because no interrupt related to this try_run
* will ever come from hardware. */
- s5p_mfc_clock_off();
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
}
}
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 85600f2..8cf1c6f 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -1745,6 +1745,13 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
mfc_debug(1, "Try run dev: %p\n", dev);
+ if (test_bit(0, &dev->enter_suspend)) {
+ mfc_debug(1, "Entering suspend so do not schedule any jobs\n");
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
+ return;
+ }
+
/* Check whether hardware is not running */
if (test_and_set_bit(0, &dev->hw_lock) != 0) {
/* This is perfectly ok, the scheduled ctx should wait */
@@ -1756,6 +1763,8 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
new_ctx = s5p_mfc_get_new_ctx(dev);
if (new_ctx < 0) {
/* No contexts to run */
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
mfc_err("Failed to unlock hardware.\n");
return;
@@ -1775,7 +1784,9 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
/* Last frame has already been sent to MFC
* Now obtaining frames from MFC buffer */
- s5p_mfc_clock_on();
+ if (test_and_set_bit(0, &dev->clk_flag) == 0)
+ s5p_mfc_clock_on();
+
if (ctx->type == MFCINST_DECODER) {
switch (ctx->state) {
case MFCINST_FINISHING:
@@ -1855,7 +1866,8 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
* scheduled, reduce the clock count as no one will
* ever do this, because no interrupt related to this try_run
* will ever come from hardware. */
- s5p_mfc_clock_off();
+ if (test_and_clear_bit(0, &dev->clk_flag))
+ s5p_mfc_clock_off();
}
}
--
1.7.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* RE: [PATCH 05/17] [media] s5p-mfc: don't disable clock when next ctx is pending
2014-09-15 6:43 ` [PATCH 05/17] [media] s5p-mfc: don't disable clock when next ctx is pending Kiran AVND
@ 2014-09-16 14:20 ` Kamil Debski
2014-09-17 9:25 ` Kiran Avnd
0 siblings, 1 reply; 27+ messages in thread
From: Kamil Debski @ 2014-09-16 14:20 UTC (permalink / raw)
To: 'Kiran AVND', linux-media
Cc: wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
Hi Kiran,
> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> Sent: Monday, September 15, 2014 8:43 AM
>
> From: Arun Mankuzhi <arun.m@samsung.com>
>
> In MFC's dynamic clock gating, we turn on the clock in try_run and turn
> off the clock upon receiving an interrupt. But this leads to excessive
> gating/ungating of the clocks in the case of multi-instance video
> playback. The clock gets disabled in ctx1's irq and then immediately
> turned on for ctx2's try_run.
>
> A better solution is to turn off the clocks only when there are no new
> frames to be processed in any context. This is done by conditionally
> clocking on/off calls in try-run. clock-off is done outside try-run
> only for suspend and error scenarios.
Why is this solution better? According to my best knowledge clock gating
is a simple register write that controls whether the clock is passed.
Adding an if statement and a new flag in my opinion adds overhead.
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
>
> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> 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 | 26 ++++++++-------
> -------
> drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 +
> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c | 11 ++++++++-
> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 ++++++++++++-
> 4 files changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index e37fb99..9df130b 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -150,7 +150,8 @@ static void s5p_mfc_watchdog_worker(struct
> work_struct *work)
> mfc_err("Error: some instance may be closing/opening\n");
> spin_lock_irqsave(&dev->irqlock, flags);
>
> - s5p_mfc_clock_off();
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
>
> for (i = 0; i < MFC_NUM_CONTEXTS; i++) {
> ctx = dev->ctx[i];
> @@ -174,7 +175,6 @@ static void s5p_mfc_watchdog_worker(struct
> work_struct *work)
> mfc_err("Failed to reload FW\n");
> goto unlock;
> }
> - s5p_mfc_clock_on();
> ret = s5p_mfc_init_hw(dev);
> if (ret)
> mfc_err("Failed to reinit FW\n");
> @@ -338,7 +338,6 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
> *ctx,
> wake_up_ctx(ctx, reason, err);
> if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> BUG();
> - s5p_mfc_clock_off();
> s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> return;
> }
> @@ -411,12 +410,11 @@ leave_handle_frame:
> wake_up_ctx(ctx, reason, err);
> if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> BUG();
> - s5p_mfc_clock_off();
> - /* if suspending, wake up device and do not try_run again*/
> + /* if suspending, wake up device*/
> if (test_bit(0, &dev->enter_suspend))
> wake_up_dev(dev, reason, err);
> - else
> - s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> +
> + s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> }
>
> /* Error handling for interrupt */
> @@ -460,7 +458,8 @@ static void s5p_mfc_handle_error(struct s5p_mfc_dev
> *dev,
> if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> BUG();
> s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
> - s5p_mfc_clock_off();
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
> wake_up_dev(dev, reason, err);
> return;
> }
> @@ -514,7 +513,6 @@ static void s5p_mfc_handle_seq_done(struct
> s5p_mfc_ctx *ctx,
> clear_work_bit(ctx);
> if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> BUG();
> - s5p_mfc_clock_off();
> s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> wake_up_ctx(ctx, reason, err);
> }
> @@ -554,15 +552,14 @@ static void s5p_mfc_handle_init_buffers(struct
> s5p_mfc_ctx *ctx,
> if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> BUG();
>
> - 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();
>
> - s5p_mfc_clock_off();
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
>
> wake_up(&ctx->queue);
> }
> @@ -596,7 +593,6 @@ static void s5p_mfc_handle_stream_complete(struct
> s5p_mfc_ctx *ctx,
>
> 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); } @@ -639,7 +635,6
> @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
> wake_up_ctx(ctx, reason, err);
> if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> BUG();
> - s5p_mfc_clock_off();
> s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> } else {
> s5p_mfc_handle_frame(ctx, reason, err); @@ -704,8
> +699,6 @@ irq_cleanup_hw:
> if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> mfc_err("Failed to unlock hw\n");
>
> - s5p_mfc_clock_off();
> -
> s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> mfc_debug(2, "Exit via irq_cleanup_hw\n");
> return IRQ_HANDLED;
> @@ -1216,6 +1209,7 @@ static int s5p_mfc_probe(struct platform_device
> *pdev)
> platform_set_drvdata(pdev, dev);
>
> dev->hw_lock = 0;
> + dev->clk_flag = 0;
> dev->watchdog_workqueue =
> create_singlethread_workqueue(S5P_MFC_NAME);
> INIT_WORK(&dev->watchdog_work, s5p_mfc_watchdog_worker);
> atomic_set(&dev->watchdog_cnt, 0);
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index 01816ff..92f596e 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -289,6 +289,7 @@ struct s5p_mfc_priv_buf {
> * @watchdog_workqueue: workqueue for the watchdog
> * @watchdog_work: worker for the watchdog
> * @alloc_ctx: videobuf2 allocator contexts for two memory
> banks
> + * @clk_flag: flag used for dynamic control of mfc clock
> * @enter_suspend: flag set when entering suspend
> * @ctx_buf: common context memory (MFCv6)
> * @warn_start: hardware error code from which warnings
start
> @@ -332,6 +333,7 @@ struct s5p_mfc_dev {
> struct workqueue_struct *watchdog_workqueue;
> struct work_struct watchdog_work;
> void *alloc_ctx[2];
> + unsigned long clk_flag;
> unsigned long enter_suspend;
>
> struct s5p_mfc_priv_buf ctx_buf;
> 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 58ec7bb..e2b2f31 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> @@ -1371,6 +1371,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev
> *dev)
>
> if (test_bit(0, &dev->enter_suspend)) {
> mfc_debug(1, "Entering suspend so do not schedule any
> jobs\n");
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
> return;
> }
> /* Check whether hardware is not running */ @@ -1383,6 +1385,8 @@
> static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
> new_ctx = s5p_mfc_get_new_ctx(dev);
> if (new_ctx < 0) {
> /* No contexts to run */
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
> if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
> mfc_err("Failed to unlock hardware\n");
> return;
> @@ -1396,7 +1400,9 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev
> *dev)
> * Last frame has already been sent to MFC.
> * Now obtaining frames from MFC buffer
> */
> - s5p_mfc_clock_on();
> + if (test_and_set_bit(0, &dev->clk_flag) == 0)
> + s5p_mfc_clock_on();
> +
> if (ctx->type == MFCINST_DECODER) {
> s5p_mfc_set_dec_desc_buffer(ctx);
> switch (ctx->state) {
> @@ -1474,7 +1480,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev
> *dev)
> * scheduled, reduce the clock count as no one will
> * ever do this, because no interrupt related to this
> try_run
> * will ever come from hardware. */
> - s5p_mfc_clock_off();
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
> }
> }
>
> 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 85600f2..8cf1c6f 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -1745,6 +1745,13 @@ static void s5p_mfc_try_run_v6(struct
> s5p_mfc_dev *dev)
>
> mfc_debug(1, "Try run dev: %p\n", dev);
>
> + if (test_bit(0, &dev->enter_suspend)) {
> + mfc_debug(1, "Entering suspend so do not schedule any
> jobs\n");
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
> + return;
> + }
> +
> /* Check whether hardware is not running */
> if (test_and_set_bit(0, &dev->hw_lock) != 0) {
> /* This is perfectly ok, the scheduled ctx should wait */
> @@ -1756,6 +1763,8 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev
> *dev)
> new_ctx = s5p_mfc_get_new_ctx(dev);
> if (new_ctx < 0) {
> /* No contexts to run */
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
> if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
> mfc_err("Failed to unlock hardware.\n");
> return;
> @@ -1775,7 +1784,9 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev
> *dev)
> /* Last frame has already been sent to MFC
> * Now obtaining frames from MFC buffer */
>
> - s5p_mfc_clock_on();
> + if (test_and_set_bit(0, &dev->clk_flag) == 0)
> + s5p_mfc_clock_on();
> +
> if (ctx->type == MFCINST_DECODER) {
> switch (ctx->state) {
> case MFCINST_FINISHING:
> @@ -1855,7 +1866,8 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev
> *dev)
> * scheduled, reduce the clock count as no one will
> * ever do this, because no interrupt related to this
> try_run
> * will ever come from hardware. */
> - s5p_mfc_clock_off();
> + if (test_and_clear_bit(0, &dev->clk_flag))
> + s5p_mfc_clock_off();
> }
> }
>
> --
> 1.7.3.rc2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 05/17] [media] s5p-mfc: don't disable clock when next ctx is pending
2014-09-16 14:20 ` Kamil Debski
@ 2014-09-17 9:25 ` Kiran Avnd
2014-09-17 9:57 ` Kamil Debski
0 siblings, 1 reply; 27+ messages in thread
From: Kiran Avnd @ 2014-09-17 9:25 UTC (permalink / raw)
To: linux-media, Kamil Debski
Cc: Kiran AVND, wuchengli, posciak, Arun Mankuzhi, ihf, Prathyush,
Arun Kumar, Kiran Avnd
Hi Kamil,
On Tue, Sep 16, 2014 at 7:50 PM, Kamil Debski <k.debski@samsung.com> wrote:
>
> Hi Kiran,
>
> > From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> > Sent: Monday, September 15, 2014 8:43 AM
> >
> > From: Arun Mankuzhi <arun.m@samsung.com>
> >
> > In MFC's dynamic clock gating, we turn on the clock in try_run and turn
> > off the clock upon receiving an interrupt. But this leads to excessive
> > gating/ungating of the clocks in the case of multi-instance video
> > playback. The clock gets disabled in ctx1's irq and then immediately
> > turned on for ctx2's try_run.
> >
> > A better solution is to turn off the clocks only when there are no new
> > frames to be processed in any context. This is done by conditionally
> > clocking on/off calls in try-run. clock-off is done outside try-run
> > only for suspend and error scenarios.
>
> Why is this solution better? According to my best knowledge clock gating
> is a simple register write that controls whether the clock is passed.
>
In newer versions of MFC we cannot turn off the clock asynchronously.
There is a bus reset procedure which is added in patch 08/17.
Since this procedure involves an overhead of waiting for the bus to
complete data transaction,
this patch removes clock off at different places and optimizes the
number of clock switching .
Regards
Kiran
> Adding an if statement and a new flag in my opinion adds overhead.
>
> Best wishes,
> --
> Kamil Debski
> Samsung R&D Institute Poland
>
> >
> > Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> > 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 | 26 ++++++++-------
> > -------
> > drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 +
> > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c | 11 ++++++++-
> > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 ++++++++++++-
> > 4 files changed, 35 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > index e37fb99..9df130b 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > @@ -150,7 +150,8 @@ static void s5p_mfc_watchdog_worker(struct
> > work_struct *work)
> > mfc_err("Error: some instance may be closing/opening\n");
> > spin_lock_irqsave(&dev->irqlock, flags);
> >
> > - s5p_mfc_clock_off();
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> >
> > for (i = 0; i < MFC_NUM_CONTEXTS; i++) {
> > ctx = dev->ctx[i];
> > @@ -174,7 +175,6 @@ static void s5p_mfc_watchdog_worker(struct
> > work_struct *work)
> > mfc_err("Failed to reload FW\n");
> > goto unlock;
> > }
> > - s5p_mfc_clock_on();
> > ret = s5p_mfc_init_hw(dev);
> > if (ret)
> > mfc_err("Failed to reinit FW\n");
> > @@ -338,7 +338,6 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
> > *ctx,
> > wake_up_ctx(ctx, reason, err);
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > BUG();
> > - s5p_mfc_clock_off();
> > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > return;
> > }
> > @@ -411,12 +410,11 @@ leave_handle_frame:
> > wake_up_ctx(ctx, reason, err);
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > BUG();
> > - s5p_mfc_clock_off();
> > - /* if suspending, wake up device and do not try_run again*/
> > + /* if suspending, wake up device*/
> > if (test_bit(0, &dev->enter_suspend))
> > wake_up_dev(dev, reason, err);
> > - else
> > - s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > +
> > + s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > }
> >
> > /* Error handling for interrupt */
> > @@ -460,7 +458,8 @@ static void s5p_mfc_handle_error(struct s5p_mfc_dev
> > *dev,
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > BUG();
> > s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
> > - s5p_mfc_clock_off();
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> > wake_up_dev(dev, reason, err);
> > return;
> > }
> > @@ -514,7 +513,6 @@ static void s5p_mfc_handle_seq_done(struct
> > s5p_mfc_ctx *ctx,
> > clear_work_bit(ctx);
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > BUG();
> > - s5p_mfc_clock_off();
> > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > wake_up_ctx(ctx, reason, err);
> > }
> > @@ -554,15 +552,14 @@ static void s5p_mfc_handle_init_buffers(struct
> > s5p_mfc_ctx *ctx,
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > BUG();
> >
> > - 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();
> >
> > - s5p_mfc_clock_off();
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> >
> > wake_up(&ctx->queue);
> > }
> > @@ -596,7 +593,6 @@ static void s5p_mfc_handle_stream_complete(struct
> > s5p_mfc_ctx *ctx,
> >
> > 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); } @@ -639,7 +635,6
> > @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
> > wake_up_ctx(ctx, reason, err);
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > BUG();
> > - s5p_mfc_clock_off();
> > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > } else {
> > s5p_mfc_handle_frame(ctx, reason, err); @@ -704,8
> > +699,6 @@ irq_cleanup_hw:
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > mfc_err("Failed to unlock hw\n");
> >
> > - s5p_mfc_clock_off();
> > -
> > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > mfc_debug(2, "Exit via irq_cleanup_hw\n");
> > return IRQ_HANDLED;
> > @@ -1216,6 +1209,7 @@ static int s5p_mfc_probe(struct platform_device
> > *pdev)
> > platform_set_drvdata(pdev, dev);
> >
> > dev->hw_lock = 0;
> > + dev->clk_flag = 0;
> > dev->watchdog_workqueue =
> > create_singlethread_workqueue(S5P_MFC_NAME);
> > INIT_WORK(&dev->watchdog_work, s5p_mfc_watchdog_worker);
> > atomic_set(&dev->watchdog_cnt, 0);
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > index 01816ff..92f596e 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > @@ -289,6 +289,7 @@ struct s5p_mfc_priv_buf {
> > * @watchdog_workqueue: workqueue for the watchdog
> > * @watchdog_work: worker for the watchdog
> > * @alloc_ctx: videobuf2 allocator contexts for two memory
> > banks
> > + * @clk_flag: flag used for dynamic control of mfc clock
> > * @enter_suspend: flag set when entering suspend
> > * @ctx_buf: common context memory (MFCv6)
> > * @warn_start: hardware error code from which warnings
> start
> > @@ -332,6 +333,7 @@ struct s5p_mfc_dev {
> > struct workqueue_struct *watchdog_workqueue;
> > struct work_struct watchdog_work;
> > void *alloc_ctx[2];
> > + unsigned long clk_flag;
> > unsigned long enter_suspend;
> >
> > struct s5p_mfc_priv_buf ctx_buf;
> > 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 58ec7bb..e2b2f31 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> > @@ -1371,6 +1371,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev
> > *dev)
> >
> > if (test_bit(0, &dev->enter_suspend)) {
> > mfc_debug(1, "Entering suspend so do not schedule any
> > jobs\n");
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> > return;
> > }
> > /* Check whether hardware is not running */ @@ -1383,6 +1385,8 @@
> > static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
> > new_ctx = s5p_mfc_get_new_ctx(dev);
> > if (new_ctx < 0) {
> > /* No contexts to run */
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
> > mfc_err("Failed to unlock hardware\n");
> > return;
> > @@ -1396,7 +1400,9 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev
> > *dev)
> > * Last frame has already been sent to MFC.
> > * Now obtaining frames from MFC buffer
> > */
> > - s5p_mfc_clock_on();
> > + if (test_and_set_bit(0, &dev->clk_flag) == 0)
> > + s5p_mfc_clock_on();
> > +
> > if (ctx->type == MFCINST_DECODER) {
> > s5p_mfc_set_dec_desc_buffer(ctx);
> > switch (ctx->state) {
> > @@ -1474,7 +1480,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev
> > *dev)
> > * scheduled, reduce the clock count as no one will
> > * ever do this, because no interrupt related to this
> > try_run
> > * will ever come from hardware. */
> > - s5p_mfc_clock_off();
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> > }
> > }
> >
> > 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 85600f2..8cf1c6f 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> > @@ -1745,6 +1745,13 @@ static void s5p_mfc_try_run_v6(struct
> > s5p_mfc_dev *dev)
> >
> > mfc_debug(1, "Try run dev: %p\n", dev);
> >
> > + if (test_bit(0, &dev->enter_suspend)) {
> > + mfc_debug(1, "Entering suspend so do not schedule any
> > jobs\n");
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> > + return;
> > + }
> > +
> > /* Check whether hardware is not running */
> > if (test_and_set_bit(0, &dev->hw_lock) != 0) {
> > /* This is perfectly ok, the scheduled ctx should wait */
> > @@ -1756,6 +1763,8 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev
> > *dev)
> > new_ctx = s5p_mfc_get_new_ctx(dev);
> > if (new_ctx < 0) {
> > /* No contexts to run */
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> > if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
> > mfc_err("Failed to unlock hardware.\n");
> > return;
> > @@ -1775,7 +1784,9 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev
> > *dev)
> > /* Last frame has already been sent to MFC
> > * Now obtaining frames from MFC buffer */
> >
> > - s5p_mfc_clock_on();
> > + if (test_and_set_bit(0, &dev->clk_flag) == 0)
> > + s5p_mfc_clock_on();
> > +
> > if (ctx->type == MFCINST_DECODER) {
> > switch (ctx->state) {
> > case MFCINST_FINISHING:
> > @@ -1855,7 +1866,8 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev
> > *dev)
> > * scheduled, reduce the clock count as no one will
> > * ever do this, because no interrupt related to this
> > try_run
> > * will ever come from hardware. */
> > - s5p_mfc_clock_off();
> > + if (test_and_clear_bit(0, &dev->clk_flag))
> > + s5p_mfc_clock_off();
> > }
> > }
> >
> > --
> > 1.7.3.rc2
>
> --
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* RE: [PATCH 05/17] [media] s5p-mfc: don't disable clock when next ctx is pending
2014-09-17 9:25 ` Kiran Avnd
@ 2014-09-17 9:57 ` Kamil Debski
2014-09-17 10:55 ` Kiran Avnd
0 siblings, 1 reply; 27+ messages in thread
From: Kamil Debski @ 2014-09-17 9:57 UTC (permalink / raw)
To: 'Kiran Avnd', linux-media
Cc: 'Kiran AVND', wuchengli, posciak, 'Arun Mankuzhi',
ihf, 'Prathyush', 'Arun Kumar'
Hi,
> From: Kiran Avnd [mailto:kiran@chromium.org]
> Sent: Wednesday, September 17, 2014 11:26 AM
>
> Hi Kamil,
>
> On Tue, Sep 16, 2014 at 7:50 PM, Kamil Debski <k.debski@samsung.com>
> wrote:
> >
> > Hi Kiran,
> >
> > > From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> > > Sent: Monday, September 15, 2014 8:43 AM
> > >
> > > From: Arun Mankuzhi <arun.m@samsung.com>
> > >
> > > In MFC's dynamic clock gating, we turn on the clock in try_run and
> > > turn off the clock upon receiving an interrupt. But this leads to
> > > excessive gating/ungating of the clocks in the case of
> > > multi-instance video playback. The clock gets disabled in ctx1's
> irq
> > > and then immediately turned on for ctx2's try_run.
> > >
> > > A better solution is to turn off the clocks only when there are no
> > > new frames to be processed in any context. This is done by
> > > conditionally clocking on/off calls in try-run. clock-off is done
> > > outside try-run only for suspend and error scenarios.
> >
> > Why is this solution better? According to my best knowledge clock
> > gating is a simple register write that controls whether the clock is
> passed.
> >
>
> In newer versions of MFC we cannot turn off the clock asynchronously.
> There is a bus reset procedure which is added in patch 08/17.
>
> Since this procedure involves an overhead of waiting for the bus to
> complete data transaction, this patch removes clock off at different
> places and optimizes the number of clock switching .
The patch adds a new reset function - yes, but how this is connected with
the clock? In s5p_mfc_try_run_* there are s5p_mfc_clock_on/off calls and
no s5p_mfc_reset in that function. This contradict what you wrote above.
>
> Regards
> Kiran
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
>
> > Adding an if statement and a new flag in my opinion adds overhead.
> >
> > Best wishes,
> > --
> > Kamil Debski
> > Samsung R&D Institute Poland
> >
> > >
> > > Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> > > 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 | 26 ++++++++---
> ----
> > > -------
> > > drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 +
> > > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c | 11 ++++++++-
> > > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16
> ++++++++++++-
> > > 4 files changed, 35 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > > b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > > index e37fb99..9df130b 100644
> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > > @@ -150,7 +150,8 @@ static void s5p_mfc_watchdog_worker(struct
> > > work_struct *work)
> > > mfc_err("Error: some instance may be
> closing/opening\n");
> > > spin_lock_irqsave(&dev->irqlock, flags);
> > >
> > > - s5p_mfc_clock_off();
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > >
> > > for (i = 0; i < MFC_NUM_CONTEXTS; i++) {
> > > ctx = dev->ctx[i];
> > > @@ -174,7 +175,6 @@ static void s5p_mfc_watchdog_worker(struct
> > > work_struct *work)
> > > mfc_err("Failed to reload FW\n");
> > > goto unlock;
> > > }
> > > - s5p_mfc_clock_on();
> > > ret = s5p_mfc_init_hw(dev);
> > > if (ret)
> > > mfc_err("Failed to reinit FW\n"); @@ -338,7
> > > +338,6 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx *ctx,
> > > wake_up_ctx(ctx, reason, err);
> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > > BUG();
> > > - s5p_mfc_clock_off();
> > > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > > return;
> > > }
> > > @@ -411,12 +410,11 @@ leave_handle_frame:
> > > wake_up_ctx(ctx, reason, err);
> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > > BUG();
> > > - s5p_mfc_clock_off();
> > > - /* if suspending, wake up device and do not try_run again*/
> > > + /* if suspending, wake up device*/
> > > if (test_bit(0, &dev->enter_suspend))
> > > wake_up_dev(dev, reason, err);
> > > - else
> > > - s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > > +
> > > + s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > > }
> > >
> > > /* Error handling for interrupt */
> > > @@ -460,7 +458,8 @@ static void s5p_mfc_handle_error(struct
> > > s5p_mfc_dev *dev,
> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > > BUG();
> > > s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
> > > - s5p_mfc_clock_off();
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > > wake_up_dev(dev, reason, err);
> > > return;
> > > }
> > > @@ -514,7 +513,6 @@ static void s5p_mfc_handle_seq_done(struct
> > > s5p_mfc_ctx *ctx,
> > > clear_work_bit(ctx);
> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > > BUG();
> > > - s5p_mfc_clock_off();
> > > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > > wake_up_ctx(ctx, reason, err); } @@ -554,15 +552,14 @@
> static
> > > void s5p_mfc_handle_init_buffers(struct
> > > s5p_mfc_ctx *ctx,
> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > > BUG();
> > >
> > > - 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();
> > >
> > > - s5p_mfc_clock_off();
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > >
> > > wake_up(&ctx->queue);
> > > }
> > > @@ -596,7 +593,6 @@ static void
> > > s5p_mfc_handle_stream_complete(struct
> > > s5p_mfc_ctx *ctx,
> > >
> > > 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); } @@ -639,7
> > > +635,6 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
> > > wake_up_ctx(ctx, reason, err);
> > > if (test_and_clear_bit(0, &dev->hw_lock) ==
> 0)
> > > BUG();
> > > - s5p_mfc_clock_off();
> > > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > > } else {
> > > s5p_mfc_handle_frame(ctx, reason, err); @@
> > > -704,8
> > > +699,6 @@ irq_cleanup_hw:
> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
> > > mfc_err("Failed to unlock hw\n");
> > >
> > > - s5p_mfc_clock_off();
> > > -
> > > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
> > > mfc_debug(2, "Exit via irq_cleanup_hw\n");
> > > return IRQ_HANDLED;
> > > @@ -1216,6 +1209,7 @@ static int s5p_mfc_probe(struct
> > > platform_device
> > > *pdev)
> > > platform_set_drvdata(pdev, dev);
> > >
> > > dev->hw_lock = 0;
> > > + dev->clk_flag = 0;
> > > dev->watchdog_workqueue =
> > > create_singlethread_workqueue(S5P_MFC_NAME);
> > > INIT_WORK(&dev->watchdog_work, s5p_mfc_watchdog_worker);
> > > atomic_set(&dev->watchdog_cnt, 0); diff --git
> > > a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > > index 01816ff..92f596e 100644
> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> > > @@ -289,6 +289,7 @@ struct s5p_mfc_priv_buf {
> > > * @watchdog_workqueue: workqueue for the watchdog
> > > * @watchdog_work: worker for the watchdog
> > > * @alloc_ctx: videobuf2 allocator contexts for two
> memory
> > > banks
> > > + * @clk_flag: flag used for dynamic control of mfc
> clock
> > > * @enter_suspend: flag set when entering suspend
> > > * @ctx_buf: common context memory (MFCv6)
> > > * @warn_start: hardware error code from which
> warnings
> > start
> > > @@ -332,6 +333,7 @@ struct s5p_mfc_dev {
> > > struct workqueue_struct *watchdog_workqueue;
> > > struct work_struct watchdog_work;
> > > void *alloc_ctx[2];
> > > + unsigned long clk_flag;
> > > unsigned long enter_suspend;
> > >
> > > struct s5p_mfc_priv_buf ctx_buf; 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 58ec7bb..e2b2f31 100644
> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> > > @@ -1371,6 +1371,8 @@ static void s5p_mfc_try_run_v5(struct
> > > s5p_mfc_dev
> > > *dev)
> > >
> > > if (test_bit(0, &dev->enter_suspend)) {
> > > mfc_debug(1, "Entering suspend so do not schedule any
> > > jobs\n");
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > > return;
> > > }
> > > /* Check whether hardware is not running */ @@ -1383,6
> +1385,8
> > > @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
> > > new_ctx = s5p_mfc_get_new_ctx(dev);
> > > if (new_ctx < 0) {
> > > /* No contexts to run */
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
> > > mfc_err("Failed to unlock hardware\n");
> > > return;
> > > @@ -1396,7 +1400,9 @@ static void s5p_mfc_try_run_v5(struct
> > > s5p_mfc_dev
> > > *dev)
> > > * Last frame has already been sent to MFC.
> > > * Now obtaining frames from MFC buffer
> > > */
> > > - s5p_mfc_clock_on();
> > > + if (test_and_set_bit(0, &dev->clk_flag) == 0)
> > > + s5p_mfc_clock_on();
> > > +
> > > if (ctx->type == MFCINST_DECODER) {
> > > s5p_mfc_set_dec_desc_buffer(ctx);
> > > switch (ctx->state) {
> > > @@ -1474,7 +1480,8 @@ static void s5p_mfc_try_run_v5(struct
> > > s5p_mfc_dev
> > > *dev)
> > > * scheduled, reduce the clock count as no one will
> > > * ever do this, because no interrupt related to this
> > > try_run
> > > * will ever come from hardware. */
> > > - s5p_mfc_clock_off();
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > > }
> > > }
> > >
> > > 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 85600f2..8cf1c6f 100644
> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> > > @@ -1745,6 +1745,13 @@ static void s5p_mfc_try_run_v6(struct
> > > s5p_mfc_dev *dev)
> > >
> > > mfc_debug(1, "Try run dev: %p\n", dev);
> > >
> > > + if (test_bit(0, &dev->enter_suspend)) {
> > > + mfc_debug(1, "Entering suspend so do not schedule any
> > > jobs\n");
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > > + return;
> > > + }
> > > +
> > > /* Check whether hardware is not running */
> > > if (test_and_set_bit(0, &dev->hw_lock) != 0) {
> > > /* This is perfectly ok, the scheduled ctx should
> wait
> > > */ @@ -1756,6 +1763,8 @@ static void s5p_mfc_try_run_v6(struct
> > > s5p_mfc_dev
> > > *dev)
> > > new_ctx = s5p_mfc_get_new_ctx(dev);
> > > if (new_ctx < 0) {
> > > /* No contexts to run */
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
> > > mfc_err("Failed to unlock hardware.\n");
> > > return;
> > > @@ -1775,7 +1784,9 @@ static void s5p_mfc_try_run_v6(struct
> > > s5p_mfc_dev
> > > *dev)
> > > /* Last frame has already been sent to MFC
> > > * Now obtaining frames from MFC buffer */
> > >
> > > - s5p_mfc_clock_on();
> > > + if (test_and_set_bit(0, &dev->clk_flag) == 0)
> > > + s5p_mfc_clock_on();
> > > +
> > > if (ctx->type == MFCINST_DECODER) {
> > > switch (ctx->state) {
> > > case MFCINST_FINISHING:
> > > @@ -1855,7 +1866,8 @@ static void s5p_mfc_try_run_v6(struct
> > > s5p_mfc_dev
> > > *dev)
> > > * scheduled, reduce the clock count as no one will
> > > * ever do this, because no interrupt related to this
> > > try_run
> > > * will ever come from hardware. */
> > > - s5p_mfc_clock_off();
> > > + if (test_and_clear_bit(0, &dev->clk_flag))
> > > + s5p_mfc_clock_off();
> > > }
> > > }
> > >
> > > --
> > > 1.7.3.rc2
> >
> > --
> > 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 05/17] [media] s5p-mfc: don't disable clock when next ctx is pending
2014-09-17 9:57 ` Kamil Debski
@ 2014-09-17 10:55 ` Kiran Avnd
0 siblings, 0 replies; 27+ messages in thread
From: Kiran Avnd @ 2014-09-17 10:55 UTC (permalink / raw)
To: Kamil Debski
Cc: linux-media, Kiran AVND, Wu-cheng Li, Pawel Osciak, Arun Mankuzhi,
Ilja Friedel, Prathyush, Arun Kumar
On Wed, Sep 17, 2014 at 3:27 PM, Kamil Debski <k.debski@samsung.com> wrote:
> Hi,
>
>> From: Kiran Avnd [mailto:kiran@chromium.org]
>> Sent: Wednesday, September 17, 2014 11:26 AM
>>
>> Hi Kamil,
>>
>> On Tue, Sep 16, 2014 at 7:50 PM, Kamil Debski <k.debski@samsung.com>
>> wrote:
>> >
>> > Hi Kiran,
>> >
>> > > From: Kiran AVND [mailto:avnd.kiran@samsung.com]
>> > > Sent: Monday, September 15, 2014 8:43 AM
>> > >
>> > > From: Arun Mankuzhi <arun.m@samsung.com>
>> > >
>> > > In MFC's dynamic clock gating, we turn on the clock in try_run and
>> > > turn off the clock upon receiving an interrupt. But this leads to
>> > > excessive gating/ungating of the clocks in the case of
>> > > multi-instance video playback. The clock gets disabled in ctx1's
>> irq
>> > > and then immediately turned on for ctx2's try_run.
>> > >
>> > > A better solution is to turn off the clocks only when there are no
>> > > new frames to be processed in any context. This is done by
>> > > conditionally clocking on/off calls in try-run. clock-off is done
>> > > outside try-run only for suspend and error scenarios.
>> >
>> > Why is this solution better? According to my best knowledge clock
>> > gating is a simple register write that controls whether the clock is
>> passed.
>> >
>>
>> In newer versions of MFC we cannot turn off the clock asynchronously.
>> There is a bus reset procedure which is added in patch 08/17.
>>
>> Since this procedure involves an overhead of waiting for the bus to
>> complete data transaction, this patch removes clock off at different
>> places and optimizes the number of clock switching .
>
> The patch adds a new reset function - yes, but how this is connected with
> the clock? In s5p_mfc_try_run_* there are s5p_mfc_clock_on/off calls and
> no s5p_mfc_reset in that function. This contradict what you wrote above.
>
Thats right, its a mistake. The changes done in the clock_on and
clock_off functions
is part of another patchset that I will be posting soon, after this.
I shall postpone this change until then,and drop it now,
Thanks for the review,
Regards
Kiran
>>
>> Regards
>> Kiran
>
> Best wishes,
> --
> Kamil Debski
> Samsung R&D Institute Poland
>
>>
>> > Adding an if statement and a new flag in my opinion adds overhead.
>> >
>> > Best wishes,
>> > --
>> > Kamil Debski
>> > Samsung R&D Institute Poland
>> >
>> > >
>> > > Signed-off-by: Prathyush K <prathyush.k@samsung.com>
>> > > 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 | 26 ++++++++---
>> ----
>> > > -------
>> > > drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 +
>> > > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c | 11 ++++++++-
>> > > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16
>> ++++++++++++-
>> > > 4 files changed, 35 insertions(+), 20 deletions(-)
>> > >
>> > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> > > b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> > > index e37fb99..9df130b 100644
>> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> > > @@ -150,7 +150,8 @@ static void s5p_mfc_watchdog_worker(struct
>> > > work_struct *work)
>> > > mfc_err("Error: some instance may be
>> closing/opening\n");
>> > > spin_lock_irqsave(&dev->irqlock, flags);
>> > >
>> > > - s5p_mfc_clock_off();
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > >
>> > > for (i = 0; i < MFC_NUM_CONTEXTS; i++) {
>> > > ctx = dev->ctx[i];
>> > > @@ -174,7 +175,6 @@ static void s5p_mfc_watchdog_worker(struct
>> > > work_struct *work)
>> > > mfc_err("Failed to reload FW\n");
>> > > goto unlock;
>> > > }
>> > > - s5p_mfc_clock_on();
>> > > ret = s5p_mfc_init_hw(dev);
>> > > if (ret)
>> > > mfc_err("Failed to reinit FW\n"); @@ -338,7
>> > > +338,6 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx *ctx,
>> > > wake_up_ctx(ctx, reason, err);
>> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> > > BUG();
>> > > - s5p_mfc_clock_off();
>> > > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>> > > return;
>> > > }
>> > > @@ -411,12 +410,11 @@ leave_handle_frame:
>> > > wake_up_ctx(ctx, reason, err);
>> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> > > BUG();
>> > > - s5p_mfc_clock_off();
>> > > - /* if suspending, wake up device and do not try_run again*/
>> > > + /* if suspending, wake up device*/
>> > > if (test_bit(0, &dev->enter_suspend))
>> > > wake_up_dev(dev, reason, err);
>> > > - else
>> > > - s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>> > > +
>> > > + s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>> > > }
>> > >
>> > > /* Error handling for interrupt */
>> > > @@ -460,7 +458,8 @@ static void s5p_mfc_handle_error(struct
>> > > s5p_mfc_dev *dev,
>> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> > > BUG();
>> > > s5p_mfc_hw_call(dev->mfc_ops, clear_int_flags, dev);
>> > > - s5p_mfc_clock_off();
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > > wake_up_dev(dev, reason, err);
>> > > return;
>> > > }
>> > > @@ -514,7 +513,6 @@ static void s5p_mfc_handle_seq_done(struct
>> > > s5p_mfc_ctx *ctx,
>> > > clear_work_bit(ctx);
>> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> > > BUG();
>> > > - s5p_mfc_clock_off();
>> > > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>> > > wake_up_ctx(ctx, reason, err); } @@ -554,15 +552,14 @@
>> static
>> > > void s5p_mfc_handle_init_buffers(struct
>> > > s5p_mfc_ctx *ctx,
>> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> > > BUG();
>> > >
>> > > - 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();
>> > >
>> > > - s5p_mfc_clock_off();
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > >
>> > > wake_up(&ctx->queue);
>> > > }
>> > > @@ -596,7 +593,6 @@ static void
>> > > s5p_mfc_handle_stream_complete(struct
>> > > s5p_mfc_ctx *ctx,
>> > >
>> > > 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); } @@ -639,7
>> > > +635,6 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
>> > > wake_up_ctx(ctx, reason, err);
>> > > if (test_and_clear_bit(0, &dev->hw_lock) ==
>> 0)
>> > > BUG();
>> > > - s5p_mfc_clock_off();
>> > > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>> > > } else {
>> > > s5p_mfc_handle_frame(ctx, reason, err); @@
>> > > -704,8
>> > > +699,6 @@ irq_cleanup_hw:
>> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0)
>> > > mfc_err("Failed to unlock hw\n");
>> > >
>> > > - s5p_mfc_clock_off();
>> > > -
>> > > s5p_mfc_hw_call(dev->mfc_ops, try_run, dev);
>> > > mfc_debug(2, "Exit via irq_cleanup_hw\n");
>> > > return IRQ_HANDLED;
>> > > @@ -1216,6 +1209,7 @@ static int s5p_mfc_probe(struct
>> > > platform_device
>> > > *pdev)
>> > > platform_set_drvdata(pdev, dev);
>> > >
>> > > dev->hw_lock = 0;
>> > > + dev->clk_flag = 0;
>> > > dev->watchdog_workqueue =
>> > > create_singlethread_workqueue(S5P_MFC_NAME);
>> > > INIT_WORK(&dev->watchdog_work, s5p_mfc_watchdog_worker);
>> > > atomic_set(&dev->watchdog_cnt, 0); diff --git
>> > > a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> > > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> > > index 01816ff..92f596e 100644
>> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
>> > > @@ -289,6 +289,7 @@ struct s5p_mfc_priv_buf {
>> > > * @watchdog_workqueue: workqueue for the watchdog
>> > > * @watchdog_work: worker for the watchdog
>> > > * @alloc_ctx: videobuf2 allocator contexts for two
>> memory
>> > > banks
>> > > + * @clk_flag: flag used for dynamic control of mfc
>> clock
>> > > * @enter_suspend: flag set when entering suspend
>> > > * @ctx_buf: common context memory (MFCv6)
>> > > * @warn_start: hardware error code from which
>> warnings
>> > start
>> > > @@ -332,6 +333,7 @@ struct s5p_mfc_dev {
>> > > struct workqueue_struct *watchdog_workqueue;
>> > > struct work_struct watchdog_work;
>> > > void *alloc_ctx[2];
>> > > + unsigned long clk_flag;
>> > > unsigned long enter_suspend;
>> > >
>> > > struct s5p_mfc_priv_buf ctx_buf; 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 58ec7bb..e2b2f31 100644
>> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
>> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
>> > > @@ -1371,6 +1371,8 @@ static void s5p_mfc_try_run_v5(struct
>> > > s5p_mfc_dev
>> > > *dev)
>> > >
>> > > if (test_bit(0, &dev->enter_suspend)) {
>> > > mfc_debug(1, "Entering suspend so do not schedule any
>> > > jobs\n");
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > > return;
>> > > }
>> > > /* Check whether hardware is not running */ @@ -1383,6
>> +1385,8
>> > > @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
>> > > new_ctx = s5p_mfc_get_new_ctx(dev);
>> > > if (new_ctx < 0) {
>> > > /* No contexts to run */
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
>> > > mfc_err("Failed to unlock hardware\n");
>> > > return;
>> > > @@ -1396,7 +1400,9 @@ static void s5p_mfc_try_run_v5(struct
>> > > s5p_mfc_dev
>> > > *dev)
>> > > * Last frame has already been sent to MFC.
>> > > * Now obtaining frames from MFC buffer
>> > > */
>> > > - s5p_mfc_clock_on();
>> > > + if (test_and_set_bit(0, &dev->clk_flag) == 0)
>> > > + s5p_mfc_clock_on();
>> > > +
>> > > if (ctx->type == MFCINST_DECODER) {
>> > > s5p_mfc_set_dec_desc_buffer(ctx);
>> > > switch (ctx->state) {
>> > > @@ -1474,7 +1480,8 @@ static void s5p_mfc_try_run_v5(struct
>> > > s5p_mfc_dev
>> > > *dev)
>> > > * scheduled, reduce the clock count as no one will
>> > > * ever do this, because no interrupt related to this
>> > > try_run
>> > > * will ever come from hardware. */
>> > > - s5p_mfc_clock_off();
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > > }
>> > > }
>> > >
>> > > 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 85600f2..8cf1c6f 100644
>> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
>> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
>> > > @@ -1745,6 +1745,13 @@ static void s5p_mfc_try_run_v6(struct
>> > > s5p_mfc_dev *dev)
>> > >
>> > > mfc_debug(1, "Try run dev: %p\n", dev);
>> > >
>> > > + if (test_bit(0, &dev->enter_suspend)) {
>> > > + mfc_debug(1, "Entering suspend so do not schedule any
>> > > jobs\n");
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > > + return;
>> > > + }
>> > > +
>> > > /* Check whether hardware is not running */
>> > > if (test_and_set_bit(0, &dev->hw_lock) != 0) {
>> > > /* This is perfectly ok, the scheduled ctx should
>> wait
>> > > */ @@ -1756,6 +1763,8 @@ static void s5p_mfc_try_run_v6(struct
>> > > s5p_mfc_dev
>> > > *dev)
>> > > new_ctx = s5p_mfc_get_new_ctx(dev);
>> > > if (new_ctx < 0) {
>> > > /* No contexts to run */
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > > if (test_and_clear_bit(0, &dev->hw_lock) == 0) {
>> > > mfc_err("Failed to unlock hardware.\n");
>> > > return;
>> > > @@ -1775,7 +1784,9 @@ static void s5p_mfc_try_run_v6(struct
>> > > s5p_mfc_dev
>> > > *dev)
>> > > /* Last frame has already been sent to MFC
>> > > * Now obtaining frames from MFC buffer */
>> > >
>> > > - s5p_mfc_clock_on();
>> > > + if (test_and_set_bit(0, &dev->clk_flag) == 0)
>> > > + s5p_mfc_clock_on();
>> > > +
>> > > if (ctx->type == MFCINST_DECODER) {
>> > > switch (ctx->state) {
>> > > case MFCINST_FINISHING:
>> > > @@ -1855,7 +1866,8 @@ static void s5p_mfc_try_run_v6(struct
>> > > s5p_mfc_dev
>> > > *dev)
>> > > * scheduled, reduce the clock count as no one will
>> > > * ever do this, because no interrupt related to this
>> > > try_run
>> > > * will ever come from hardware. */
>> > > - s5p_mfc_clock_off();
>> > > + if (test_and_clear_bit(0, &dev->clk_flag))
>> > > + s5p_mfc_clock_off();
>> > > }
>> > > }
>> > >
>> > > --
>> > > 1.7.3.rc2
>> >
>> > --
>> > 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
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 06/17] [media] s5p-mfc: Only set timestamp/timecode for new frames.
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (4 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 05/17] [media] s5p-mfc: don't disable clock when next ctx is pending Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 07/17] [media] s5p-mfc: keep RISC ON during reset for V7/V8 Kiran AVND
` (10 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files 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 9df130b..4ab3b53 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -227,11 +227,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) {
@@ -257,6 +260,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 07/17] [media] s5p-mfc: keep RISC ON during reset for V7/V8
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (5 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 06/17] [media] s5p-mfc: Only set timestamp/timecode for new frames Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 08/17] [media] s5p-mfc: check mfc bus ctrl before reset Kiran AVND
` (9 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 92f596e..c72a338 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -342,6 +342,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 ca9f789..39258f3 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -138,12 +138,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);
@@ -152,8 +146,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 {
@@ -225,6 +224,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");
@@ -237,8 +237,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");
@@ -335,6 +337,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");
@@ -353,8 +356,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 08/17] [media] s5p-mfc: check mfc bus ctrl before reset
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (6 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 07/17] [media] s5p-mfc: keep RISC ON during reset for V7/V8 Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 09/17] [media] s5p-mfc: Don't crash the kernel if the watchdog kicks in Kiran AVND
` (8 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 deletions(-)
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 39258f3..24d5252 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -128,6 +128,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)
{
@@ -146,11 +165,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 09/17] [media] s5p-mfc: Don't crash the kernel if the watchdog kicks in.
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (7 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 08/17] [media] s5p-mfc: check mfc bus ctrl before reset Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 10/17] [media] s5p-mfc: modify mfc wakeup sequence for V8 Kiran AVND
` (7 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 | 21 +++++++--------------
1 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 4ab3b53..cc9fd0c 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -344,8 +344,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_hw_call(dev->mfc_ops, try_run, dev);
return;
}
@@ -416,8 +415,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);
/* if suspending, wake up device*/
if (test_bit(0, &dev->enter_suspend))
wake_up_dev(dev, reason, err);
@@ -463,8 +461,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);
if (test_and_clear_bit(0, &dev->clk_flag))
s5p_mfc_clock_off();
@@ -519,8 +516,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_hw_call(dev->mfc_ops, try_run, dev);
wake_up_ctx(ctx, reason, err);
}
@@ -557,14 +553,12 @@ 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);
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);
if (test_and_clear_bit(0, &dev->clk_flag))
s5p_mfc_clock_off();
@@ -641,8 +635,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_hw_call(dev->mfc_ops, try_run, dev);
} else {
s5p_mfc_handle_frame(ctx, reason, err);
--
1.7.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 10/17] [media] s5p-mfc: modify mfc wakeup sequence for V8
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (8 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 09/17] [media] s5p-mfc: Don't crash the kernel if the watchdog kicks in Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 14:42 ` Kamil Debski
2014-09-15 6:43 ` [PATCH 11/17] [media] s5p-mfc: De-init MFC when watchdog kicks in Kiran AVND
` (6 subsequent siblings)
16 siblings, 1 reply; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files 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 24d5252..8531c72 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -352,6 +352,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;
@@ -364,6 +416,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");
@@ -372,25 +425,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* RE: [PATCH 10/17] [media] s5p-mfc: modify mfc wakeup sequence for V8
2014-09-15 6:43 ` [PATCH 10/17] [media] s5p-mfc: modify mfc wakeup sequence for V8 Kiran AVND
@ 2014-09-15 14:42 ` Kamil Debski
0 siblings, 0 replies; 27+ messages in thread
From: Kamil Debski @ 2014-09-15 14:42 UTC (permalink / raw)
To: 'Kiran AVND', linux-media
Cc: wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
Hi Kiran,
> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> Sent: Monday, September 15, 2014 8:43 AM
>
> 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 files 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 24d5252..8531c72 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
> @@ -352,6 +352,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;
> @@ -364,6 +416,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");
> @@ -372,25 +425,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);
I think that this solution is messy. There are two functions - one
for v8, another for other versions. In the latter there are if conditions
that further change its behaviour accordingly to the version.
I think there are two possible solutions:
- introduce one function per version
- use one function with if statements to change its behaviour for various
MFC versions
The former will introduce repeated code, hence I suggest the latter
solution.
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
> - 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.3.rc2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 11/17] [media] s5p-mfc: De-init MFC when watchdog kicks in
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (9 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 10/17] [media] s5p-mfc: modify mfc wakeup sequence for V8 Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 12/17] [media] s5p-mfc: flush dpbs when resolution changes Kiran AVND
` (5 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index cc9fd0c..f4cb7f2 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -167,6 +167,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 12/17] [media] s5p-mfc: flush dpbs when resolution changes
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (10 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 11/17] [media] s5p-mfc: De-init MFC when watchdog kicks in Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 13/17] [media] s5p-mfc: Remove unused alloc field from private buffer struct Kiran AVND
` (4 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files 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 8cf1c6f..4cdea67 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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 13/17] [media] s5p-mfc: Remove unused alloc field from private buffer struct.
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (11 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 12/17] [media] s5p-mfc: flush dpbs when resolution changes Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 14/17] [media] s5p-mfc: fix V4L2_CID_MIN_BUFFERS_FOR_CAPTURE on resolution change Kiran AVND
` (3 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files changed, 0 insertions(+), 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 c72a338..2dda27d 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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 14/17] [media] s5p-mfc: fix V4L2_CID_MIN_BUFFERS_FOR_CAPTURE on resolution change.
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (12 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 13/17] [media] s5p-mfc: Remove unused alloc field from private buffer struct Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 6:43 ` [PATCH 15/17] [media] s5p-mfc: remove reduntant clock on & clock off Kiran AVND
` (2 subsequent siblings)
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 9103258..ca4b69f9 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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 15/17] [media] s5p-mfc: remove reduntant clock on & clock off
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (13 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 14/17] [media] s5p-mfc: fix V4L2_CID_MIN_BUFFERS_FOR_CAPTURE on resolution change Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 14:42 ` Kamil Debski
2014-09-15 6:43 ` [PATCH 16/17] [media] s5p-mfc: fix a race in interrupt flags handling Kiran AVND
2014-09-15 6:43 ` [PATCH 17/17] [media] s5p-mfc: Don't change the image size to smaller than the request Kiran AVND
16 siblings, 1 reply; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
sysmmu will control mfc device clock on/off wherever
needed. Explicit clock on/off in the driver is not needed
anymore. Remove such reduntant clock on/off in the driver.
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 --
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 6 ------
2 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index f4cb7f2..3a1f97e 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -883,7 +883,6 @@ static int s5p_mfc_release(struct file *file)
mfc_debug_enter();
mutex_lock(&dev->mfc_mutex);
- s5p_mfc_clock_on();
vb2_queue_release(&ctx->vq_src);
vb2_queue_release(&ctx->vq_dst);
/* Mark context as idle */
@@ -906,7 +905,6 @@ static int s5p_mfc_release(struct file *file)
mfc_err("Power off failed\n");
}
mfc_debug(2, "Shutting down clock\n");
- s5p_mfc_clock_off();
dev->ctx[ctx->num] = NULL;
s5p_mfc_dec_ctrls_delete(ctx);
v4l2_fh_del(&ctx->fh);
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index ca4b69f9..d0bdbfb 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -487,8 +487,6 @@ static int reqbufs_output(struct s5p_mfc_dev *dev, struct s5p_mfc_ctx *ctx,
{
int ret = 0;
- s5p_mfc_clock_on();
-
if (reqbufs->count == 0) {
mfc_debug(2, "Freeing buffers\n");
ret = vb2_reqbufs(&ctx->vq_src, reqbufs);
@@ -525,7 +523,6 @@ static int reqbufs_output(struct s5p_mfc_dev *dev, struct s5p_mfc_ctx *ctx,
ret = -EINVAL;
}
out:
- s5p_mfc_clock_off();
if (ret)
mfc_err("Failed allocating buffers for OUTPUT queue\n");
return ret;
@@ -536,8 +533,6 @@ static int reqbufs_capture(struct s5p_mfc_dev *dev, struct s5p_mfc_ctx *ctx,
{
int ret = 0;
- s5p_mfc_clock_on();
-
if (reqbufs->count == 0) {
mfc_debug(2, "Freeing buffers\n");
ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
@@ -579,7 +574,6 @@ static int reqbufs_capture(struct s5p_mfc_dev *dev, struct s5p_mfc_ctx *ctx,
ret = -EINVAL;
}
out:
- s5p_mfc_clock_off();
if (ret)
mfc_err("Failed allocating buffers for CAPTURE queue\n");
return ret;
--
1.7.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* RE: [PATCH 15/17] [media] s5p-mfc: remove reduntant clock on & clock off
2014-09-15 6:43 ` [PATCH 15/17] [media] s5p-mfc: remove reduntant clock on & clock off Kiran AVND
@ 2014-09-15 14:42 ` Kamil Debski
0 siblings, 0 replies; 27+ messages in thread
From: Kamil Debski @ 2014-09-15 14:42 UTC (permalink / raw)
To: 'Kiran AVND', linux-media
Cc: wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
Hi Kiran,
> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> Sent: Monday, September 15, 2014 8:43 AM
>
> sysmmu will control mfc device clock on/off wherever
> needed. Explicit clock on/off in the driver is not needed
> anymore. Remove such reduntant clock on/off in the driver.
What if... iommu is not used? MFC can work with either iommu or CMA.
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
>
> Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 --
> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 6 ------
> 2 files changed, 0 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index f4cb7f2..3a1f97e 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -883,7 +883,6 @@ static int s5p_mfc_release(struct file *file)
>
> mfc_debug_enter();
> mutex_lock(&dev->mfc_mutex);
> - s5p_mfc_clock_on();
> vb2_queue_release(&ctx->vq_src);
> vb2_queue_release(&ctx->vq_dst);
> /* Mark context as idle */
> @@ -906,7 +905,6 @@ static int s5p_mfc_release(struct file *file)
> mfc_err("Power off failed\n");
> }
> mfc_debug(2, "Shutting down clock\n");
> - s5p_mfc_clock_off();
> dev->ctx[ctx->num] = NULL;
> s5p_mfc_dec_ctrls_delete(ctx);
> v4l2_fh_del(&ctx->fh);
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index ca4b69f9..d0bdbfb 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -487,8 +487,6 @@ static int reqbufs_output(struct s5p_mfc_dev *dev,
> struct s5p_mfc_ctx *ctx,
> {
> int ret = 0;
>
> - s5p_mfc_clock_on();
> -
> if (reqbufs->count == 0) {
> mfc_debug(2, "Freeing buffers\n");
> ret = vb2_reqbufs(&ctx->vq_src, reqbufs);
> @@ -525,7 +523,6 @@ static int reqbufs_output(struct s5p_mfc_dev *dev,
> struct s5p_mfc_ctx *ctx,
> ret = -EINVAL;
> }
> out:
> - s5p_mfc_clock_off();
> if (ret)
> mfc_err("Failed allocating buffers for OUTPUT queue\n");
> return ret;
> @@ -536,8 +533,6 @@ static int reqbufs_capture(struct s5p_mfc_dev *dev,
> struct s5p_mfc_ctx *ctx,
> {
> int ret = 0;
>
> - s5p_mfc_clock_on();
> -
> if (reqbufs->count == 0) {
> mfc_debug(2, "Freeing buffers\n");
> ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
> @@ -579,7 +574,6 @@ static int reqbufs_capture(struct s5p_mfc_dev *dev,
> struct s5p_mfc_ctx *ctx,
> ret = -EINVAL;
> }
> out:
> - s5p_mfc_clock_off();
> if (ret)
> mfc_err("Failed allocating buffers for CAPTURE queue\n");
> return ret;
> --
> 1.7.3.rc2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 16/17] [media] s5p-mfc: fix a race in interrupt flags handling
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (14 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 15/17] [media] s5p-mfc: remove reduntant clock on & clock off Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
2014-09-15 14:42 ` Kamil Debski
2014-09-15 6:43 ` [PATCH 17/17] [media] s5p-mfc: Don't change the image size to smaller than the request Kiran AVND
16 siblings, 1 reply; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 8531c72..7aae7f4 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -467,7 +467,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)) {
@@ -493,7 +492,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 d0bdbfb..fb8069a 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);
}
@@ -756,7 +755,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 &&
@@ -1070,7 +1068,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 f7a6f68..04b0467 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1682,7 +1682,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 e2b2f31..38e79e0 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
@@ -1177,7 +1177,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;
}
@@ -1213,7 +1211,6 @@ static int s5p_mfc_run_dec_frame(struct s5p_mfc_ctx *ctx, int last_frame)
spin_unlock_irqrestore(&dev->irqlock, flags);
index = temp_vb->b->v4l2_buf.index;
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");
@@ -1274,7 +1271,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);
@@ -1298,7 +1294,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);
}
@@ -1318,7 +1313,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);
}
@@ -1353,7 +1347,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");
@@ -1403,6 +1396,8 @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
if (test_and_set_bit(0, &dev->clk_flag) == 0)
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) {
@@ -1413,12 +1408,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;
@@ -1451,12 +1444,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 4cdea67..92e7f3e 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");
@@ -1771,6 +1763,8 @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
if (test_and_set_bit(0, &dev->clk_flag) == 0)
s5p_mfc_clock_on();
+ s5p_mfc_clean_ctx_int_flags(ctx);
+
if (ctx->type == MFCINST_DECODER) {
switch (ctx->state) {
case MFCINST_FINISHING:
@@ -1780,12 +1774,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread* RE: [PATCH 16/17] [media] s5p-mfc: fix a race in interrupt flags handling
2014-09-15 6:43 ` [PATCH 16/17] [media] s5p-mfc: fix a race in interrupt flags handling Kiran AVND
@ 2014-09-15 14:42 ` Kamil Debski
0 siblings, 0 replies; 27+ messages in thread
From: Kamil Debski @ 2014-09-15 14:42 UTC (permalink / raw)
To: 'Kiran AVND', linux-media
Cc: wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
Hi,
The patch does not apply to the newest media_tree branch.
http://git.linuxtv.org/cgit.cgi/media_tree.git/
Please send a rebased version.
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
> -----Original Message-----
> From: Kiran AVND [mailto:avnd.kiran@samsung.com]
> Sent: Monday, September 15, 2014 8:43 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
> Subject: [PATCH 16/17] [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 8531c72..7aae7f4 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
> @@ -467,7 +467,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)) { @@ -493,7 +492,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 d0bdbfb..fb8069a 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);
> }
> @@ -756,7 +755,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 && @@ -1070,7 +1068,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 f7a6f68..04b0467 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> @@ -1682,7 +1682,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 e2b2f31..38e79e0 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> @@ -1177,7 +1177,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;
> }
> @@ -1213,7 +1211,6 @@ static int s5p_mfc_run_dec_frame(struct
> s5p_mfc_ctx *ctx, int last_frame)
> spin_unlock_irqrestore(&dev->irqlock, flags);
> index = temp_vb->b->v4l2_buf.index;
> 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"); @@ -
> 1274,7 +1271,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);
> @@ -1298,7 +1294,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);
> }
>
> @@ -1318,7 +1313,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);
> }
>
> @@ -1353,7 +1347,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"); @@ -1403,6 +1396,8
> @@ static void s5p_mfc_try_run_v5(struct s5p_mfc_dev *dev)
> if (test_and_set_bit(0, &dev->clk_flag) == 0)
> 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) {
> @@ -1413,12 +1408,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;
> @@ -1451,12 +1444,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 4cdea67..92e7f3e 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"); @@ -1771,6 +1763,8
> @@ static void s5p_mfc_try_run_v6(struct s5p_mfc_dev *dev)
> if (test_and_set_bit(0, &dev->clk_flag) == 0)
> s5p_mfc_clock_on();
>
> + s5p_mfc_clean_ctx_int_flags(ctx);
> +
> if (ctx->type == MFCINST_DECODER) {
> switch (ctx->state) {
> case MFCINST_FINISHING:
> @@ -1780,12 +1774,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.3.rc2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 17/17] [media] s5p-mfc: Don't change the image size to smaller than the request.
2014-09-15 6:42 [PATCH 00/17] Fixes from Chrome OS tree for MFC driver Kiran AVND
` (15 preceding siblings ...)
2014-09-15 6:43 ` [PATCH 16/17] [media] s5p-mfc: fix a race in interrupt flags handling Kiran AVND
@ 2014-09-15 6:43 ` Kiran AVND
16 siblings, 0 replies; 27+ messages in thread
From: Kiran AVND @ 2014-09-15 6:43 UTC (permalink / raw)
To: linux-media
Cc: k.debski, wuchengli, posciak, arun.m, ihf, prathyush.k, arun.kk
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 files 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 04b0467..aa99742 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1059,6 +1059,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);
@@ -1093,8 +1094,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.3.rc2
^ permalink raw reply related [flat|nested] 27+ messages in thread