On 10/12/2019 16.19, Sascha Hauer wrote:
> On Tue, Dec 10, 2019 at 03:12:47PM +0200, Peter Ujfalusi wrote:
>>> static inline void vchan_synchronize(struct virt_dma_chan *vc)
>>> {
>>> + LIST_HEAD(head);
>>> unsigned long flags;
>>>
>>> tasklet_kill(&vc->task);
>>>
>>> spin_lock_irqsave(&vc->lock, flags);
>>> - if (vc->vd_terminated) {
>>> - vchan_vdesc_fini(vc->vd_terminated);
>>> - vc->vd_terminated = NULL;
>>> - }
>>> +
>>> + list_splice_tail_init(&vc->desc_terminated, &head);
>>> +
>>> spin_unlock_irqrestore(&vc->lock, flags);
>>> +
>>> + vchan_dma_desc_free_list(vc, &head);
>>
>> My only issue with the vchan_dma_desc_free_list() is that it prints with
>> dev_dbg() for each descriptor it is freeing up.
>> The 'stuck' descriptor happens quite frequently if you start/stop audio
>> for example.
>
> if we consider the message useful then I would say it's equally useful
> both for the 'stuck' descriptor and for the regular case.
The case with the 'stuck' descriptor is not really a stuck descriptor.
It is the cyclic descriptor which is by principle never completes so the
vchan_complete() would be never called for it -> we leaked descriptors
in audio start/stop/start/...
Printing about it up is like printing before each vc->desc_free() call
at each completion.
> IMO the debug message only makes sense if we make sure it is printed
> each time a descriptor is freed. Currently it's printed when the
> descriptor is freed from vchan_dma_desc_free_list(), but not when it's
> freed from vchan_vdesc_fini(). This is confusing as looking at the dmesg
> suggests that we lose descriptors.
The only case I can think when it is usable is when client prepared
several transfers, but decides to terminate the channel, or if several
descriptors are in the issued list waiting and the client terminates the
channel.
Enabling the debug for all free operation would easily make the device
overwhelmed with prints during boot (MMC rootfs w/ system DMA?).
I'm questioning the sole usefulness of the print. I don't think it adds
any value or information.
>> This is why I proposed a local
>>
>> list_for_each_entry_safe(vd, _vd, &head, node) {
>> list_del(&vd->node);
>> vchan_vdesc_fini(vd);
>> }
>>
>> On the other hand what vchan_dma_desc_free_list() is doing is exactly
>> the same thing as this loop is doing with the addition of the debug print.
>>
>> I'm not sure how useful that debug print is, not sure if anyone would
>> miss if it is gone?
>>
>> If not, than see my comment on patch 2.
>
> We could add the dev_dbg into vchan_vdesc_fini() as well and still
> implement your suggestion on patch 2...
That would be a bad idea. imho.
> Anyway, I don't care much if the dev_dbg is there or not. I'll happily
> add a patch removing it for the next round if that's what we agree upon.
I would just drop the debug print ;)
Vinod?
>
> Sascha
>
>
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
