Message-ID: <VE1PR04MB6688F1C65295C1A0D689C5A289469@VE1PR04MB6688.eurprd04.prod.outlook.com>
Date:   Thu, 22 Apr 2021 03:33:32 +0000
From:   Robin Gong <yibin.gong@....com>
To:     Lucas Stach <l.stach@...gutronix.de>,
        Shengjiu Wang <shengjiu.wang@...il.com>
CC:     Nicolin Chen <nicoleotsuka@...il.com>,
        Linux-ALSA <alsa-devel@...a-project.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        Timur Tabi <timur@...nel.org>, Xiubo Li <Xiubo.Lee@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "S.j. Wang" <shengjiu.wang@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Takashi Iwai <tiwai@...e.com>,
        "linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
        Mark Brown <broonie@...nel.org>,
        dl-linux-imx <linux-imx@....com>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        "perex@...ex.cz" <perex@...ex.cz>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>
Subject: RE: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in
 probe

On 2021/04/22 Lucas Stach <l.stach@...gutronix.de> wrote:
> > But the timer of runtime_auto_suspend decide when enter runtime
> > suspend rather than hardware, while transfer data size and transfer
> > rate on IP bus decide when the dma interrupt happen.
> >
> But it isn't the hardware that decides to drop the rpm refcount to 0 and
> starting the autosuspend timer, it's the driver.
Yes, driver should keep rpm refcount never to 0 in such case. But I think that case
Is a common case in dma cyclic with runtime_auto_suspend, so some dma driver
also add pm_runtime_get_sync/ pm_runtime_put_autosuspend in interrupt handler
like qcom/bam_dma.c for safe rather than only pm_runtime_mark_last_busy().

> 
> >  Generally, we can call pm_runtime_get_sync(fsl_chan->dev)/
> > pm_runtime_mark_last_busy in interrupt handler to hope the
> > runtime_auto_suspend timer expiry later than interrupt coming, but if
> > the transfer data size is larger in cyclic and transfer rate is very
> > slow like 115200 or lower on uart, the fix autosuspend timer
> > 100ms/200ms maybe not enough, hence, runtime suspend may execute
> > meanwhile the dma interrupt maybe triggered and caught by GIC(but
> > interrupt handler prevent by spin_lock_irqsave in pm_suspend_timer_fn() ),
> and then interrupt handler start to run after runtime suspend.
> 
> If your driver code drops the rpm refcount to 0 and starts the autosuspend
> timer while a cyclic transfer is still in flight this is clearly a bug. Autosuspend is
> not there to paper over driver bugs, but to amortize cost of actually
> suspending and resuming the hardware. Your driver code must still work even
> if the timeout is 0, i.e. the hardware is immediately suspended after you drop
> the rpm refcount to 0.
> 
> If you still have transfers queued/in-flight the driver code must keep a rpm
> reference.
Yes, that's what I said for fix before as below.
'I have a simple workaround that disable runtime suspend in issue_pending worker by
calling pm_runtime_forbid() and then enable runtime auto suspend in dmaengine_terminate_all
so that we could easily regard that edma channel is always in runtime resume between
issue_pending and channel terminated and ignore the above interrupt handler/scu-pd limitation' 

Powered by blists - more mailing lists