dmaengine Archive on lore.kernel.org
 help / color / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: Vinod Koul <vkoul@kernel.org>,
	dmaengine@vger.kernel.org, Michal Simek <michal.simek@xilinx.com>,
	Hyun Kwon <hyun.kwon@xilinx.com>,
	Tejas Upadhyay <tejasu@xilinx.com>,
	Satish Kumar Nagireddy <SATISHNA@xilinx.com>
Subject: Re: [PATCH v3 2/6] dmaengine: Add interleaved cyclic transaction type
Date: Fri, 24 Jan 2020 10:56:54 +0200
Message-ID: <20200124085654.GB4842@pendragon.ideasonboard.com> (raw)
In-Reply-To: <ded9c051-11f3-e61a-e0de-1cd54a8c85d5@ti.com>

Hi Peter,

On Fri, Jan 24, 2020 at 09:20:15AM +0200, Peter Ujfalusi wrote:
> On 23/01/2020 14.23, Laurent Pinchart wrote:
> >>>> I think capture (camera) is another potential beneficiary of this.
> > 
> > Possibly, although in the camera case I'd rather have the hardware stop
> > if there's no more buffer. Requiring a buffer to always be present is
> > annoying from a userspace point of view. For display it's different, if
> > userspace doesn't submit a new frame, the same frame should keep being
> > displayed on the screen.
> > 
> >>>> So you don't need to terminate the running interleaved_cyclic and start
> >>>> a new one, but prepare and issue a new one, which would
> >>>> terminate/replace the currently running cyclic interleaved DMA?
> > 
> > Correct.
> > 
> >>> Why not explicitly terminate the transfer and start when a new one is
> >>> issued. That can be common usage for audio and display..
> >>
> >> Yes, this is what I'm asking. The cyclic transfer is running and in
> >> order to start the new transfer, the previous should stop. But in cyclic
> >> case it is not going to happen unless it is terminated.
> >>
> >> When one would want to have different interleaved transfer the display
> >> (or capture )IP needs to be reconfigured as well. The the would need to
> >> be terminated anyways to avoid interpreting data in a wrong way.
> > 
> > The use case here is not to switch to a new configuration, but to switch
> > to a new buffer. If the transfer had to be terminated manually first,
> > the DMA engine would potentially miss a frame, which is not acceptable.
> > We need an atomic way to switch to the next transfer.
> 
> You have a special hardware in hand, most DMAs can not just replace a
> cyclic transfer in-flight and it also kind of violates the DMAengine
> principles.

That's why cyclic support is optional :-)

> If cyclic transfer is started then it is expected to run forever until
> it is terminated. Preparing and issuing a new transfer will not get
> executed when there is already a cyclic transfer in flight as your only
> option is to terminate_all, which will kill the running cyclic _and_
> will discard the issued and pending transfers.

For the existing cyclic API, I could agree with that, although there's
very little documentation in the dmaengine subsystem to be used as an
authoritative source of information :-(

> So the use case is page flip when you have multiple framebuffers and you
> switch them to show the updated one, right?

Correct.

> There are things missing in DMAengine in API level for sure to do this,
> imho.
> The issue is that cyclic transfers will never complete, they run until
> terminated, but you want to replace the currently executing one with a
> another cyclic transfer without actually terminating the other.

Correct.

> It is like pause the 1st cyclic and continue with the 2nd one. Then at
> some point you pause the 2nd one and restart the 1st one.

No, after the 2nd one comes the 3rd one. It's not a double-buffering
case, it's really about replacing the buffer with another one,
regardless of where it comes from. Userspace may double-buffer, or
triple, or more.

> It is also crucial that the pause /switch happens when the executing one
> finished the interleaved round and not in the middle somewhere, right?

Yes. But that's not specific to this use case, with all non-cyclic
transfers submitting a new transfer request doesn't stop the ongoing
transfer (if any) immediately, it just queues the new transfer for
processing.

> If you:
> desc_1 = dmaengine_prep_interleaved_cyclic(chan, );
> cookie_1 = dmaengine_submit(desc_1);
> desc_2 = dmaengine_prep_interleaved_cyclic(chan, );
> cookie_2 = dmaengine_submit(desc_1);
> 
> /* cookie_1/desc_1 is started */
> dma_async_issue_pending(chan);
> 
> /* When need to switch to cookie_2 */
> dmaengine_cyclic_set_active_cookie(chan, cookie_2);
> /*
>  * cookie_1 execution is suspended after it finished the running
>  * dma_interleaved_template or buffer in normal cyclic and cookie_2
>  * is replacing it.
>  */
> 
> /* Switch back to cookie_1 */
> dmaengine_cyclic_set_active_cookie(chan, cookie_1);
> /*
>  * cookie_2 execution is suspended after it finished the running
>  * dma_interleaved_template or buffer in normal cyclic and cookie_1
>  * is replacing it.
>  */

As explained above, I don't want to switch back to a previous transfer,
I always want a new one. I don't see why we would need this kind of API
when we can just define that any queued interleaved transfer, whether
cyclic or not, is just queued and replaces the ongoing transfer at the
next frame boundary. Drivers don't have to implement the new API if the
hardware doesn't possess this capability.

> There should be a (yet another) capabilities flag got
> cyclic_set_active_cookie and the documentation should be strict on what
> is the expected behavior.
> 
> You can kill everything with terminate_all.
> There is another thing which is missing imho from DMAengine: to
> terminate a specific cookie, not the entire channel, which might be a
> good addition as you might spawn framebuffers and then delete them and
> you might want to release the corresponding cookie/descriptor as well.
> 
> What do you think?

I think it's overcomplicated for this use case :-)

-- 
Regards,

Laurent Pinchart

  parent reply index

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-23  2:29 [PATCH v3 0/6] dma: Add Xilinx ZynqMP DPDMA driver Laurent Pinchart
2020-01-23  2:29 ` [PATCH v3 1/6] dt: bindings: dma: xilinx: dpdma: DT bindings for Xilinx DPDMA Laurent Pinchart
2020-01-23  2:29 ` [PATCH v3 2/6] dmaengine: Add interleaved cyclic transaction type Laurent Pinchart
2020-01-23  8:03   ` Peter Ujfalusi
2020-01-23  8:43     ` Vinod Koul
2020-01-23  8:51       ` Peter Ujfalusi
2020-01-23 12:23         ` Laurent Pinchart
2020-01-24  6:10           ` Vinod Koul
2020-01-24  8:50             ` Laurent Pinchart
2020-02-10 14:06               ` Laurent Pinchart
2020-02-13 13:29                 ` Vinod Koul
2020-02-13 13:48                   ` Laurent Pinchart
2020-02-13 14:07                     ` Vinod Koul
2020-02-13 14:15                       ` Peter Ujfalusi
2020-02-13 16:52                         ` Laurent Pinchart
2020-02-14  4:23                           ` Vinod Koul
2020-02-14 16:22                             ` Laurent Pinchart
2020-02-17 10:00                               ` Peter Ujfalusi
2020-02-19  9:25                                 ` Vinod Koul
2020-02-26 16:30                                   ` Laurent Pinchart
2020-03-02  3:47                                     ` Vinod Koul
2020-03-02  7:37                                       ` Laurent Pinchart
2020-03-03  4:32                                         ` Vinod Koul
2020-03-03 19:22                                           ` Laurent Pinchart
2020-03-04  5:13                                             ` Vinod Koul
2020-03-04  8:01                                               ` Laurent Pinchart
2020-03-04 15:37                                                 ` Vinod Koul
2020-03-04 16:00                                                   ` Laurent Pinchart
2020-03-04 16:24                                                     ` Vinod Koul
     [not found]                                                       ` <20200311155248.GA4772@pendragon.ideasonboard.com>
2020-03-18 15:14                                                         ` Laurent Pinchart
2020-03-25 16:00                                                           ` Laurent Pinchart
2020-03-26  7:02                                                         ` Vinod Koul
2020-04-08 17:00                                                           ` Laurent Pinchart
2020-04-15 15:12                                                             ` Laurent Pinchart
2020-03-06 14:49                                                     ` Peter Ujfalusi
2020-03-11 23:15                                                       ` Laurent Pinchart
2020-02-26 16:24                                 ` Laurent Pinchart
2020-03-02  3:42                                   ` Vinod Koul
2020-01-24  7:20           ` Peter Ujfalusi
2020-01-24  7:38             ` Peter Ujfalusi
2020-01-24  8:58               ` Laurent Pinchart
2020-01-24  8:56             ` Laurent Pinchart [this message]
2020-01-23  2:29 ` [PATCH v3 3/6] dmaengine: virt-dma: Use lockdep to check locking requirements Laurent Pinchart
2020-01-23  2:29 ` [PATCH v3 4/6] dmaengine: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver Laurent Pinchart
2020-01-23  2:29 ` [PATCH v3 5/6] dmaengine: xilinx: dpdma: Add debugfs support Laurent Pinchart
2020-01-23  2:29 ` [PATCH v3 6/6] arm64: dts: zynqmp: Add DPDMA node Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200124085654.GB4842@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=SATISHNA@xilinx.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=hyun.kwon@xilinx.com \
    --cc=michal.simek@xilinx.com \
    --cc=peter.ujfalusi@ti.com \
    --cc=tejasu@xilinx.com \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git