On Wed, Sep 17, 2014 at 3:27 PM, Kamil Debski <k.debski@xxxxxxxxxxx> wrote: > Hi, > >> From: Kiran Avnd [mailto:kiran@xxxxxxxxxxxx] >> Sent: Wednesday, September 17, 2014 11:26 AM >> >> Hi Kamil, >> >> On Tue, Sep 16, 2014 at 7:50 PM, Kamil Debski <k.debski@xxxxxxxxxxx> >> wrote: >> > >> > Hi Kiran, >> > >> > > From: Kiran AVND [mailto:avnd.kiran@xxxxxxxxxxx] >> > > Sent: Monday, September 15, 2014 8:43 AM >> > > >> > > From: Arun Mankuzhi <arun.m@xxxxxxxxxxx> >> > > >> > > 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@xxxxxxxxxxx> >> > > Signed-off-by: Arun Mankuzhi <arun.m@xxxxxxxxxxx> >> > > Signed-off-by: Kiran AVND <avnd.kiran@xxxxxxxxxxx> >> > > --- >> > > 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@xxxxxxxxxxxxxxx More majordomo >> > info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
