Re: [PATCH] dmaengine: cppi41: Fix cppi41_dma_prep_slave_sg() when idle

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

Tony,

On 10/23/19 10:18 PM, Tony Lindgren wrote:
>> the autosuspend_delay is set 100 ms, so if you put a udelay(101) between
>> prep_sg and issue_pending in the usb driver this trickery will be for
>> nothing, right?
>> If the usb driver is preempted for longer than 100ms between the two
>> calls, same issue.
>> Not sure, but if for some reason the transfer would take longer than
>> 100ms than pm_runtime will bring down the dma, no?
> 
> No, the dma will happen just fine no matter what the delay is.
> 
> Part of the problem here is the musb driver. It friggin
> continues before checking the completion of a dma transfer!

nice. Well I assume it is from an era when pm_runtime did not even
existed and assumes that things are always available and react without
delay.

> Fixing that currently is not trivial.

I had the pleasure to lurk in for the tusb6010_omap and I fully agree.

>>> But yeah, some dmaengine API that can sleep to tell
>>> a request is about to come would simplify things.
>>
>> any of the prep callbacks kind of indicates that a client is preparing a
>> transfer so in a perfect world it is going to want to execute it..
>>
>>> I don't think we have anything like that available
>>> right now?
>>
>> Well, it would have the same issues. If the time between
>> dmaengine_be_warned_i_m_going_to_call_issue_pending_soon and
>> issue_pending is more than the autosuspend_delay then it is not going to
>> help.
> 
> We'd have to allow dma consumer driver call pm_runtime_get_sync()
> on the dma device. Something similar maybe to what we have
> for phy_pm_runtime_get_sync(). Or just get the device handle for
> dma so the consumer can call pm_runtime_get_sync() on it.

How much a pm_runtime_get_sync(dmadev) is different when it is issued by
the client driver compared to when the dma driver issues it for it's own
device?

>> On the other hand: if the usb driver assumes that the dma transfer is
>> already finished when issue_pending returned and carry on with
>> subsequent request, that is also a problematic assumption. One can only
>> consider a transfer to be done if the completion callback is called or
>> you have polled for the completion and it tells you the same.
>> This is problematic if you are in atomic context as the DMA completion
>> interrupt might not come while you are there.
> 
> Yeah the musb driver has a history of 14 years of issues.
> I guess the logic there has been, it's usb, it can disconnect
> at any time..
> 
>> imho, this fix is working by lucky constellation of the stars ;)
>> Or we can assume that there will never be more than 100ms delay between
>> prepare_sg and issue_pending...
> 
> Not true. The cpp41 dma is doing the right thing and is not
> affected by the PM runtime autosuspend delay.

Yes, what the cppi41 does is (was) correct. If the rpm's work is not
executed between the runtime_get and when the is_suspended flag is
checked in issue_pending, then the resume callback will start the
transfer. That's kind of fine(ish). If cppi41 would be used for audio
then it would not be as the DMA must start when we tell it to start.

> Otherwise USB
> hard drives would not work, they can take seconds to spin up :)

Hrm, true.

> The missing part here seems to be the fact that musb continues
> before the dma completion is done.

But I still fail to see the difference between the events before this
patch and with the case when there is a 100ms delay between prep_sg and
issue_pending.

Before this patch:

prep_sg()
issue_pending() <- runtime_get() /  put_autosuspend()
		   _not_ starting transfer
runtime_resume() <- starts the transfer

With this patch and than 100ms delay between prep_sg and issue_pending:

prep_sg() <- runtime_get() /  put_autosuspend()
runtime_resume() <- not starting transfer
issue_pending() <- runtime_get() /  put_autosuspend()
		   starts the transfer

With this patch, but more than 100ms delay in between:

prep_sg() <- runtime_get() /  put_autosuspend()
runtime_resume() <- not starting transfer
> 100ms delay
runtime_suspend()
issue_pending() <- runtime_get() /  put_autosuspend()
		   _not_ starting transfer
runtime_resume() <- starts the transfer

pm_runtime_get_sync() in issue_pending would be the solution to avoid
delayed execution, but the usb driver should not assume that DMA is
completed as soon as issue_pending returned.

- Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki