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, 15 Apr 2020 18:12:41 +0300 [thread overview]
Message-ID: <20200415151241.GG4758@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20200408170047.GA21714@pendragon.ideasonboard.com>
Hi Vinod,
Ping. We need a solution to this problem, it's been way too long
already. If you don't want to accept my proposal, please provide me with
an implementation or a very detailed spec I can implement.
On Wed, Apr 08, 2020 at 08:00:49PM +0300, Laurent Pinchart wrote:
> On Thu, Mar 26, 2020 at 12:32:34PM +0530, Vinod Koul wrote:
> > On 11-03-20, 17:52, Laurent Pinchart wrote:
> >> On Wed, Mar 04, 2020 at 09:54:26PM +0530, Vinod Koul wrote:
> >>>>>>> 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.
> >>>>>
> >>>>> So I have dma to a slow peripheral and it is stuck for some reason. I
> >>>>> want to abort the cookie and let subsequent ones runs (btw this is for
> >>>>> non cyclic case), so I would use that here. Today we terminate_all and
> >>>>> then resubmit...
> >>>>
> >>>> That's also for immediate abort, right ?
> >>>
> >>> Right
> >>>
> >>>> For this to work properly we need very accurate residue reporting, as
> >>>> the client will usually need to know exactly what has been transferred.
> >>>> The device would need to support DMA_RESIDUE_GRANULARITY_BURST when
> >>>> aborting an ongoing transfer. What hardware supports this ?
> >>>
> >>> git grep DMA_RESIDUE_GRANULARITY_BURST drivers/dma/ |wc -l
> >>> 27
> >>>
> >>> So it seems many do support the burst reporting.
> >>
> >> Yes, but not all of those may support aborting a transfer *and*
> >> reporting the exact residue of cancelled transfers. We need both to
> >> implement your proposal.
> >
> > Reporting residue is already implemented, please see struct
> > dmaengine_result. This can be passed by a callback
> > dma_async_tx_callback_result() in struct dma_async_tx_descriptor.
>
> I mean that I don't know if the driver that support
> DMA_RESIDUE_GRANULARITY_BURST only support reporting the residue when
> the transfer is active, or also support reporting it when cancelling a
> transfer. Maybe all of them do, maybe a subset of them do, so I can't
> tell if this would be a feature that could be widely supported.
>
> >>>>>>> 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 ?
> >>>>>
> >>>>> Sure, if you have hw to support please test. If not, you will not
> >>>>> implement that.
> >>>>>
> >>>>> The point is that API should support it and people can add support in
> >>>>> the controllers and test :)
> >>>>
> >>>> I still think this is a different API. We'll have
> >>>>
> >>>> 1. Existing .issue_pending(), queueing the next transfer for non-cyclic
> >>>> cases, and being a no-op for cyclic cases.
> >>>> 2. New .terminate_cookie(AT_END_OF_TRANSFER), being a no-op for
> >>>> non-cyclic cases, and moving to the next transfer for cyclic cases.
> >>>> 3. New .terminate_cookie(ABORT_IMMEDIATELY), applicable to both cyclic
> >>>> and non-cyclic cases.
> >>>>
> >>>> 3. is an API I don't need, and can't easily test. I agree that it can
> >>>> have use cases (provided the DMA device can abort an ongoing transfer
> >>>> *and* still support DMA_RESIDUE_GRANULARITY_BURST in that case).
> >>>>
> >>>> I'm troubled by my inability to convince you that 1. and 2. are really
> >>>> the same, with 1. addressing the non-cyclic case and 2. addressing the
> >>>> cyclic case :-) This is why I think they should both be implemeted using
> >>>> .issue_pending() (no other option for 1., that's what it uses today).
> >>>> This wouldn't prevent implementing 3. with a new .terminate_cookie()
> >>>> operation, that wouldn't need to take a flag as it would always operate
> >>>> in ABORT_IMMEDIATELY mode. There would also be no need to report a new
> >>>> capability for 3., as the presence of the .terminate_cookie() handler
> >>>> would be enough to tell clients that the API is supported. Only a new
> >>>> capability for 2. would be needed.
> >>>
> >>> Well I agree 1 & 2 seem similar but I would like to define the behaviour
> >>> not dependent on the txn being cyclic or not. That is my concern and
> >>> hence the idea that:
> >>>
> >>> 1. .issue_pending() will push txn to pending_queue, you may have a case
> >>> where that is done only once (due to nature of txn), but no other
> >>> implication
> >>>
> >>> 2. .terminate_cookie(EOT) will abort the transfer at the end. Maybe not
> >>> used for cyclic but irrespective of that, the behaviour would be abort
> >>> at end of cyclic
> >>
> >> Did you mean "maybe not used for non-cyclic" ?
> >
> > Yes I think so..
> >
> >>> 3. .terminate_cookie(IMMEDIATE) will abort immediately. If there is
> >>> anything in pending_queue that will get pushed to hardware.
> >>>
> >>> 4. Cyclic by nature never completes
> >>> - as a consequence needs to be stopped by terminate_all/terminate_cookie
> >>>
> >>> Does these rules make sense :)
> >>
> >> It's a set of rules that I think can handle my use case, but I still
> >> believe my proposal based on just .issue_pending() would be simpler, in
> >> line with the existing API concepts, and wouldn't preclude the addition
> >> of .terminate_cookie(IMMEDIATE) at a later point. It's your call though,
> >> especially if you provide the implementation :-) When do you think you
> >> will be able to do so ?
> >
> > I will try to take a stab at it once merge window opens.. will let you
> > and Peter for sneak preview once I start on it :)
>
> I started giving it a try as this has been blocked for two months and a
> half now.
>
> I very quickly ran into issues as the interface is ill-defined as it
> stands.
>
> - What should happen when .terminate_cookie(EOT) is called with no other
> transfer issued, and a new transfer is issued before the current
> transfer terminates ?
>
> - I expect .terminate_cookie() to be asynchronous, as .terminate_all().
> This means that actual termination of cyclic transfers will actually
> be handled at end of transfer, in the interrupt handler. This creates
> race conditions with other operations. It would also make it much more
> difficult to support this feature for devices that require sleeping
> when stopping the DMA engine at the end of a cyclic transfer.
>
> If we have to go forward with this new API, I need a detailed
> explanation of how all this should be handled. I still truly believe
> this is a case of yak shaving that introduces additional complexity for
> absolutely no valid reason, when a solution that is aligned with the
> existing API and its concepts exists already. It's your decision as the
> subsystem maintainer, but if you want something more complex, please
> provide it soon. I don't want to wait another three months to see
> progress on this issue.
>
> >>>>>>> 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 from API design pov, I would like this to support both the features.
> >>>>> This helps us to not rework the API again for the immediate abort.
> >>>>>
> >>>>> I am not expecting this to be implemented by you if your hw doesn't
> >>>>> support it. The core changes are pretty minimal and callback in the
> >>>>> driver is the one which does the job and yours wont do this
> >>>>
> >>>> Xilinx DMA drivers don't support DMA_RESIDUE_GRANULARITY_BURST so I
> >>>> can't test this indeed.
> >>>
> >>> Sure I understand that! Am sure folks will respond to CFT and I guess
> >>> Peter will also be interested in testing.
> >>
> >> s/testing/implementing it/ :-)
> >
> > Even better :)
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2020-04-15 15:13 UTC|newest]
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 [this message]
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=20200415151241.GG4758@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).