Hi Kiran, > From: Kiran AVND [mailto:[email protected]] > Sent: Monday, September 15, 2014 8:43 AM > > From: Arun Mankuzhi <[email protected]> > > 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 <[email protected]> > Signed-off-by: Arun Mankuzhi <[email protected]> > Signed-off-by: Kiran AVND <[email protected]> > --- > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
