dmaengine Archive on lore.kernel.org
 help / color / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Vinod Koul <vkoul@kernel.org>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>,
	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: Wed, 4 Mar 2020 10:01:28 +0200
Message-ID: <20200304080128.GA4712@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20200304051301.GS4148@vkoul-mobl>

Hi Vinod,

On Wed, Mar 04, 2020 at 10:43:01AM +0530, Vinod Koul wrote:
> On 03-03-20, 21:22, Laurent Pinchart wrote:
> > On Tue, Mar 03, 2020 at 10:02:54AM +0530, Vinod Koul wrote:
> > > On 02-03-20, 09:37, Laurent Pinchart wrote:
> > > > > I would be more comfortable in calling an API to do so :)
> > > > > The flow I am thinking is:
> > > > > 
> > > > > - prep cyclic1 txn
> > > > > - submit cyclic1 txn
> > > > > - call issue_pending() (cyclic one starts)
> > > > > 
> > > > > - prep cyclic2 txn
> > > > > - submit cyclic2 txn
> > > > > - signal_cyclic1_txn aka terminate_cookie()
> > > > > - cyclic1 completes, switch to cyclic2 (dmaengine driver)
> > > > > - get callback for cyclic1 (optional)
> > > > > 
> > > > > To check if hw supports terminate_cookie() or not we can check if the
> > > > > callback support is implemented
> > > > 
> > > > Two questions though:
> > > > 
> > > > - Where is .issue_pending() called for cyclic2 in your above sequence ?
> > > >   Surely it should be called somewhere, as the DMA engine API requires
> > > >   .issue_pending() to be called for a transfer to be executed, otherwise
> > > >   it stays in the submitted but not pending queue.
> > > 
> > > Sorry missed that one, I would do that after submit cyclic2 txn step and
> > > then signal signal_cyclic1_txn termination
> > 
> > OK, that matches my understanding, good :-)
> > 
> > > > - With the introduction of a new .terminate_cookie() operation, we need
> > > >   to specify that operation for all transfer types. What's its
> > > 
> > > Correct
> > > 
> > > >   envisioned semantics for non-cyclic transfers ? And how do DMA engine
> > > >   drivers report that they support .terminate_cookie() for cyclic
> > > >   transfers but not for other transfer types (the counterpart of
> > > >   reporting, in my proposition, that .issue_pending() isn't supported
> > > >   replace the current cyclic transfer) ?
> > > 
> > > Typically for dmaengine controller cyclic is *not* a special mode, only
> > > change is that a list provided to controller is circular.
> > 
> > I don't agree with this. For cyclic transfers to be replaceable in a
> > clean way, the feature must be specifically implemented at the hardware
> > level. A DMA engine that supports chaining transfers with an explicit
> > way to override that chaining, and without the logic to report if the
> > inherent race was lost or not, really can't support this API.
> 
> Well chaining is a typical feature in dmaengine and making last chain
> point to first makes it circular. I have seen couple of engines and this
> was the implementation in the hardware.
> 
> There can exist special hardware for this purposes as well, but the
> point is that the cyclic can be treated as circular list.
> 
> > Furthemore, for non-cyclic transfers, what would .terminate_cookie() do
> > ? I need it to be defined as terminating the current transfer when it
> > ends for the cyclic case, not terminating it immediately. All non-cyclic
> > transfers terminate by themselves when they end, so what would this new
> > operation do ?
> 
> I would use it for two purposes, cancelling txn but at the end of
> current txn. I have couple of usages where this would be helpful.

I fail to see how that would help. Non-cyclic transfers always stop at
the end of the transfer. "Cancelling txn but at the end of current txn"
is what DMA engine drivers already do if you call .terminate_cookie() on
the ongoing transfer. It would thus be a no-op.

> Second in error handling where some engines do not support
> aborting (unless we reset the whole controller)

Could you explain that one ? I'm not sure to understand it.

> But yes the .terminate_cookie() semantics should indicate if the
> termination should be immediate or end of current txn. I see people
> using it for both.

Immediate termination is *not* something I'll implement as I have no
good way to test that semantics. I assume you would be fine with leaving
that for later, when someone will need it ?

> And with this I think it would make sense to also add this to
> capabilities :)

I'll repeat the comment I made to Peter: you want me to implement a
feature that you think would be useful, but is completely unrelated to
my use case, while there's a more natural way to handle my issue with
the current API, without precluding in any way the addition of your new
feature in the future. Not fair.

> > > So, the .terminate_cookie() should be a feature for all type of txn's.
> > > If for some reason (dont discount what hw designers can do) a controller
> > > supports this for some specific type(s), then they should return
> > > -ENOTSUPP for cookies that do not support and let the caller know.
> > 
> > But then the caller can't know ahead of time, it will only find out when
> > it's too late, and can't decide not to use the DMA engine if it doesn't
> > support the feature. I don't think that's a very good option.
> 
> Agreed so lets go with adding these in caps.

So if there's a need for caps anyway, why not a cap that marks
.issue_pending() as moving from the current cyclic transfer to the next
one ? 

-- 
Regards,

Laurent Pinchart

  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 [this message]
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
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=20200304080128.GA4712@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