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