On 23/10/2019 23:18, Tony Lindgren wrote:
* Peter Ujfalusi <peter.ujfalusi@xxxxxx> [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
