Re: [PATCH v9 03/11] dmaengine: qcom: bam_dma: implement support for BAM locking

[Vinod Koul <vkoul@kernel.org>]

On 19-02-26, 20:10, Manivannan Sadhasivam wrote:
> On Thu, Feb 19, 2026 at 07:30:04AM -0600, Bartosz Golaszewski wrote:
> > On Thu, 19 Feb 2026 13:12:09 +0100, Manivannan Sadhasivam
> > <mani@kernel.org> said:
> > > On Fri, Jan 09, 2026 at 03:15:38PM +0100, Bartosz Golaszewski wrote:
> > >> On Fri, Jan 9, 2026 at 3:27 AM Vinod Koul <vkoul@kernel.org> wrote:
> > >> >
> > >> > >
> > >> > > We need an API because we send a locking descriptor, then a regular
> > >> > > descriptor (or descriptors) for the actual transaction(s) and then an
> > >> > > unlocking descriptor. It's a thing the user of the DMA engine needs to
> > >> > > decide on, not the DMA engine itself.
> > >> >
> > >> > I think downstream sends lock descriptor always. What is the harm in
> > >> > doing that every time if we go down that path?
> > >>
> > >> No, in downstream it too depends on the user setting the right bits.
> > >> Currently the only user of the BAM locking downstream is the NAND
> > >> driver. I don't think the code where the crypto driver uses it is
> > >> public yet.
> > >>
> > >> And yes, there is harm - it slightly impacts performance. For QCE it
> > >> doesn't really matter as any users wanting to offload skcipher or SHA
> > >> are better off using the Arm Crypto Extensions anyway as they are
> > >> faster by an order of magnitude (!). It's also the default upstream,
> > >> where the priorities are set such that the ARM CEs are preferred over
> > >> the QCE. QCE however, is able to coordinate with the TrustZone and
> > >> will be used to support the DRM use-cases.
> > >>
> > >> I prefer to avoid impacting any other users of BAM DMA.
> > >>
> > >
> > > Sorry for jumping late. But I disagree with the argument that the client drivers
> > > have to set the LOCK/UNLOCK bit. These bits are specific to BAM DMA IP for
> > > serializing the command descriptors from multiple entities. So DMA clients like
> > > Crypto/NAND have no business in setting this flag. It is the job of the BAM
> > > dmaengine driver to set/unset it at the start and end of the descriptor chain.
> > >
> > 
> > But what if a given client does not need locking? We don't want to enable it
> > for everyone - as I explained before.
> > 
> 
> That's not going to hurt. AFAIK, enabling locking wouldn't cause any notable
> performance overhead.

I was always skeptical on this one. I had never seen why locking should
be pushed to clients. As Bjorn said it leads to more mess than worth it.
Thanks Mnai

> 
> > >> > Reg Dmitry question above, this is dma hw capability, how will client
> > >> > know if it has to lock on older rev of hardware or not...?
> > >> >
> > >> > > Also: only the crypto engine needs it for now, not all the other users
> > >> > > of the BAM engine.
> > >> >
> > >>
> > >> Trying to set the lock/unlock bits will make
> > >> dmaengine_desc_attach_metadata() fail if HW does not support it.
> > >>
> > >
> > > The BAM dmaengine driver *must* know based on the IP version whether it supports
> > > the LOCK/UNLOCK bits or not, not the client drivers. How can the client drivers
> > > know about the BAM DMA IP capability?

Lock bits are on the BAM DMA IP or client? Can we not add this
capability to BAM driver and lock for IPs that support

> > >
> > 
> > FYI: the current version of this is v10[1].
> > 
> > In it (and in this one too but let's discuss the current one) the BAM driver
> > *does* know *based on IP version* whether is supports locking or not. The client
> > requests a lock but this will fail if the BAM does not support it. The
> > client does
> > not check the BAM IP revision. So yes: it's the BAM driver that's in charge.
> > 
> 
> This design looks flawed. The client *doesn't* know whether it needs locking or
> not. If the BAM supports locking, it should enable it for all descriptors.

Ack

> 
> > > For all these reasons, BAM driver should handle the locking mechanism internaly.
> > > This will allow the client drivers to work without any modifications.
> > >
> > 
> > Ok, I'm open to alternatives but please help me figure out the "hows": How do
> > you tell the BAM driver that the client needs (or does not) locking?
> 
> As said above, BAM doesn't need to know. Locking is the hardware capability of
> the BAM, not clients.
> 
> > How do
> > you handle the case where we need to lock the BAM, send an arbitrary number
> > of descriptors from the client and then unlock it? How can the BAM know *when*
> > to lock/unlock?
> > 
> 
> BAM driver has to perform lock during issue_pending() and unlock while reporting
> the completion using vchan_cookie_complete().

Sounds good to me, thanks Mani

-- 
~Vinod