Message-ID: <VE1PR04MB6638EE85485768351755557B89499@VE1PR04MB6638.eurprd04.prod.outlook.com>
Date:   Mon, 19 Apr 2021 07:17:12 +0000
From:   Robin Gong <yibin.gong@....com>
To:     Lucas Stach <l.stach@...gutronix.de>,
        Shengjiu Wang <shengjiu.wang@...il.com>
CC:     "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
        "linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
        Linux-ALSA <alsa-devel@...a-project.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Timur Tabi <timur@...nel.org>, Xiubo Li <Xiubo.Lee@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "S.j. Wang" <shengjiu.wang@....com>, Takashi Iwai <tiwai@...e.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "perex@...ex.cz" <perex@...ex.cz>,
        Nicolin Chen <nicoleotsuka@...il.com>,
        Mark Brown <broonie@...nel.org>,
        dl-linux-imx <linux-imx@....com>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "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

Hi Lucas,

On 2021/04/14 Lucas Stach <l.stach@...gutronix.de> wrote:
> Hi Robin,
> 
> Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong:
> > On 2020/05/20 17:43 Lucas Stach <l.stach@...gutronix.de> wrote:
> > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang:
> > > > Hi
> > > >
> > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach
> > > > <l.stach@...gutronix.de>
> > > wrote:
> > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang:
> > > > > > There are two requirements that we need to move the request of
> > > > > > dma channel from probe to open.
> > > > >
> > > > > How do you handle -EPROBE_DEFER return code from the channel
> > > > > request if you don't do it in probe?
> > > >
> > > > I use the dma_request_slave_channel or dma_request_channel instead
> > > > of dmaengine_pcm_request_chan_of. so there should be not
> > > > -EPROBE_DEFER return code.
> > >
> > > This is a pretty weak argument. The dmaengine device might probe
> > > after you try to get the channel. Using a function to request the
> > > channel that doesn't allow you to handle probe deferral is IMHO a
> > > bug and should be fixed, instead of building even more assumptions on top
> of it.
> > >
> > > > > > - When dma device binds with power-domains, the power will be
> > > > > > enabled when we request dma channel. If the request of dma
> > > > > > channel happen on probe, then the power-domains will be always
> > > > > > enabled after kernel boot up,  which is not good for power
> > > > > > saving,  so we need to move the request of dma channel to
> > > > > > .open();
> > > > >
> > > > > This is certainly something which could be fixed in the
> > > > > dmaengine driver.
> > > >
> > > > Dma driver always call the pm_runtime_get_sync in
> > > > device_alloc_chan_resources, the device_alloc_chan_resources is
> > > > called when channel is requested. so power is enabled on channel
> request.
> > >
> > > So why can't you fix the dmaengine driver to do that RPM call at a
> > > later time when the channel is actually going to be used? This will
> > > allow further power savings with other slave devices than the audio PCM.
> > Hi Lucas,
> >   Thanks for your suggestion. I have tried to implement runtime
> > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2 sec)
> > for this feature as below (or you can refer to
> > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/
> > pm_runtime_put_autosuspend in all dmaengine driver interface like
> > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_cycli
> > c/
> > device_tx_status...
> >
> >
> >                 pm_runtime_use_autosuspend(fsl_chan->dev);
> >                 pm_runtime_set_autosuspend_delay(fsl_chan->dev,
> 2000);
> >
> > That could resolve this audio case since the autosuspend could suspend
> > runtime after
> > 2 seconds if there is no further dma transfer but only channel
> request(device_alloc_chan_resources).
> > But unfortunately, it cause another issue. As you know, on our
> > i.mx8qm/qxp, power domain done by scfw (drivers/firmware/imx/scu-pd.c)
> over mailbox:
> >  imx_sc_pd_power()->imx_scu_call_rpc()->
> > imx_scu_ipc_write()->mbox_send_message()
> > which means have to 'waits for completion', meanwhile, some driver
> > like tty will call dmaengine interfaces in non-atomic case as below,
> >
> > static int uart_write(struct tty_struct *tty, const unsigned char
> > *buf, int count) {
> >    .......
> > 	    port = uart_port_lock(state, flags);
> >    ......
> >         __uart_start(tty);  //call start_tx()->dmaengine_prep_slave_sg...
> >         uart_port_unlock(port, flags);
> >         return ret;
> > }
> >
> > Thus dma runtime resume may happen in that timing window and cause
> kernel alarm.
> > I'm not sure whether there are similar limitations on other driver
> > subsystem. But for me, It looks like the only way to resolve the
> > contradiction between tty and scu-pd (hardware limitation on
> > i.mx8qm/qxp) is to give up autosuspend and keep pm_runtime_get_sync
> only in device_alloc_chan_resources because request channel is a safe
> non-atomic phase.
> > Do you have any idea? Thanks in advance.
> 
> If you look closely at the driver you used as an example (hidma.c) it looks like
> there is already something in there, which looks very much like what you need
> here:
> 
> In hidma_issue_pending() the driver tries to get the device to runtime resume.
> If this doesn't work, maybe due to the power domain code not being able to
> be called in atomic context, the actual work of waking up the dma hardware
> and issuing the descriptor is shunted to a tasklet.
> 
> If I'm reading this right, this is exactly what you need here to be able to call the
> dmaengine code from atomic context: try the rpm get and issue immediately
> when possible, otherwise shunt the work to a non- atomic context where you
> can deal with the requirements of scu-pd.
Yes, I can schedule_work to worker to runtime resume edma channel by calling scu-pd.
But that means all dmaengine interfaces should be taken care, not only issue_pending()
but also dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/
dmaengine_tx_status(). Not sure why hidma only take care issue_pending. Maybe
their user case is just for memcpy/memset so that no further complicate case as ALSA
or TTY.
Besides, for autosuspend in cyclic, we have to add pm_runtime_get_sync into interrupt
handler as qcom/bam_dma.c. but how could resolve the scu-pd's non-atmoic limitation
in interrupt handler?  

> 
> Also you don't need the runtime resume in all of the functions you mentioned.
> From a quick look into the edma driver it looks like for example the
> prep_slave_dma() function only touches data structures in memory, so you
> don't actually need the device to be awake at that point. Only later in the flow
> when you write registers in the dma hardware and actually issue the transfer
> you need to wake the device from sleep.
Yes, don't need add pm_runtime_get_sync into prep_slave_dma, only care where
HW indeed touched like issue_pending()/terminated_all()/pause()/resume..etc.
 

Powered by blists - more mailing lists