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

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 23/10/2019 23:18, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [191023 19:55]:
>> On 10/23/19 10:18 PM, Tony Lindgren wrote:
>>> 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?
> 
> Well the consumer device could call pm_runtime_get_sync(dmadev)
> when the USB cable is connected for example, and then call
> pm_runtime_pu(dmadev) when let's say the USB cable is disconnected.
> 
> Without using pm_runtime_irq_safe() we currently don't have a
> clear path for doing this where the pm_runtime_get_sync(dmadev)
> may sleep.
> 
>> 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.
> 
> Oh I see. Yes the consumer driver would need to check for
> the completed dma transfer in all cases. The delay issues
> should not currently happen in the musb_ep_program() problem
> case as it gets called from IRQ context.
> 
> And no, adding pm_runtime_get_sync() to issue_pending is not
> a solution. There may be clocks and regulators that need to
> be powered up, and we don't want to use pm_runtime_irq_safe()
> because of the permanent use count on the parent.

5 cents.

I think the right thing might be to get rid of pm_runtime_xxx()
in cppi41_dma_issue_pending(). So overall approach will be:

- new job -> cppi41_dma_prep_slave_sg() -> pm_runtime_get()
- issue_pending: fill backlog if suspended or run_queue if active (pm_runtime_active())
- job done: dmaengine_desc_get_callback_invoke() ->

	dmaengine_desc_get_callback_invoke();
	pm_runtime_mark_last_busy(cdd->ddev.dev);
	pm_runtime_put_autosuspend(cdd->ddev.dev);
   in all places.

It even might allow to get rid of cdd->lock.


-- 
Best regards,
grygorii