Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1232291 > unrolled thread

Re: [PATCH 1/3] dmaengine: virt-dma: don't always free descriptor upon completion

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] dmaengine: virt-dma: don't always free descriptor  upon completion Vinod Koul <vinod.koul@intel.com> - 2015-09-24 19:30 +0200
    Re: [PATCH 1/3] dmaengine: virt-dma: don't always free descriptor upon completion Robert Jarzmik <robert.jarzmik@free.fr> - 2015-09-24 22:00 +0200

#1232291 — Re: [PATCH 1/3] dmaengine: virt-dma: don't always free descriptor upon completion

On Sun, Sep 06, 2015 at 01:40:52PM +0200, Robert Jarzmik wrote:
> @@ -29,7 +29,7 @@ dma_cookie_t vchan_tx_submit(struct dma_async_tx_descriptor *tx)
>  	spin_lock_irqsave(&vc->lock, flags);
>  	cookie = dma_cookie_assign(tx);
>  
> -	list_add_tail(&vd->node, &vc->desc_submitted);
> +	list_move_tail(&vd->node, &vc->desc_submitted);

I am not sure I follow this, why move ?

> +int vchan_tx_desc_free(struct dma_async_tx_descriptor *tx)
> +{
> +	struct virt_dma_chan *vc = to_virt_chan(tx->chan);
> +	struct virt_dma_desc *vd = to_virt_desc(tx);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vc->lock, flags);
> +	list_del(&vd->node);
> +	spin_unlock_irqrestore(&vc->lock, flags);
> +
> +	dev_dbg(vc->chan.device->dev, "vchan %p: txd %p[%x]: freeing\n",
> +		vc, vd, vd->tx.cookie);
> +	vc->desc_free(vd);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vchan_tx_desc_free);
this one seems okay, can you add comments here and also update documentation
for this

> @@ -96,9 +115,13 @@ void vchan_dma_desc_free_list(struct virt_dma_chan *vc, struct list_head *head)
>  	while (!list_empty(head)) {
>  		struct virt_dma_desc *vd = list_first_entry(head,
>  			struct virt_dma_desc, node);
> -		list_del(&vd->node);
> -		dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd);
> -		vc->desc_free(vd);
> +		if (dmaengine_desc_test_reuse(&vd->tx)) {
> +			list_move_tail(&vd->node, &vc->desc_allocated);

should we check this if the list is to be freed? Why would anyone call free
except when cleaning up ?

>  static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
>  {
> +	struct virt_dma_desc *vd;
>  	unsigned long flags;
>  	LIST_HEAD(head);
>  
>  	spin_lock_irqsave(&vc->lock, flags);
>  	vchan_get_all_descriptors(vc, &head);
> +	list_for_each_entry(vd, &head, node)
> +		dmaengine_desc_clear_reuse(&vd->tx);

why do we want to do this, if we are freeing them

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1232404 — Re: [PATCH 1/3] dmaengine: virt-dma: don't always free descriptor upon completion

Vinod Koul <vinod.koul@intel.com> writes:

> On Sun, Sep 06, 2015 at 01:40:52PM +0200, Robert Jarzmik wrote:
>> @@ -29,7 +29,7 @@ dma_cookie_t vchan_tx_submit(struct dma_async_tx_descriptor *tx)
>>  	spin_lock_irqsave(&vc->lock, flags);
>>  	cookie = dma_cookie_assign(tx);
>>  
>> -	list_add_tail(&vd->node, &vc->desc_submitted);
>> +	list_move_tail(&vd->node, &vc->desc_submitted);
>
> I am not sure I follow this, why move ?
Because it is already on the allocated list (see vchan_tx_prep()).

>> +int vchan_tx_desc_free(struct dma_async_tx_descriptor *tx)
>> +{
>> +	struct virt_dma_chan *vc = to_virt_chan(tx->chan);
>> +	struct virt_dma_desc *vd = to_virt_desc(tx);
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&vc->lock, flags);
>> +	list_del(&vd->node);
>> +	spin_unlock_irqrestore(&vc->lock, flags);
>> +
>> +	dev_dbg(vc->chan.device->dev, "vchan %p: txd %p[%x]: freeing\n",
>> +		vc, vd, vd->tx.cookie);
>> +	vc->desc_free(vd);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(vchan_tx_desc_free);
> this one seems okay, can you add comments here and also update documentation
> for this
Ah yes, very good point, doxygen like documentation, got it.

>> @@ -96,9 +115,13 @@ void vchan_dma_desc_free_list(struct virt_dma_chan *vc, struct list_head *head)
>>  	while (!list_empty(head)) {
>>  		struct virt_dma_desc *vd = list_first_entry(head,
>>  			struct virt_dma_desc, node);
>> -		list_del(&vd->node);
>> -		dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd);
>> -		vc->desc_free(vd);
>> +		if (dmaengine_desc_test_reuse(&vd->tx)) {
>> +			list_move_tail(&vd->node, &vc->desc_allocated);
>
> should we check this if the list is to be freed? Why would anyone call free
> except when cleaning up ?
Yes, there is a corner case : a driver does a dmaengine_terminate_all(), to stop
the dma transfers (a missed video frame for example). Then upon the sync signal,
it resubmits the reusable transfers it had.

Or said differently, reusable transfers should continue to be reusable through a
dmaengine_terminate_all(), hence this test. And there is a link with your
comment below.

>>  static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
>>  {
>> +	struct virt_dma_desc *vd;
>>  	unsigned long flags;
>>  	LIST_HEAD(head);
>>  
>>  	spin_lock_irqsave(&vc->lock, flags);
>>  	vchan_get_all_descriptors(vc, &head);
>> +	list_for_each_entry(vd, &head, node)
>> +		dmaengine_desc_clear_reuse(&vd->tx);
>
> why do we want to do this, if we are freeing them
Because if we don't clear reuse flag, vchan_dma_desc_free_list() won't really
free the descriptor, but move it to allocated list.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web