Re: [PATCH 12/18] soc: qcom: ipa: immediate commands

From: Arnd Bergmann <arnd@arndb.de>
To: Alex Elder <elder@linaro.org>
Cc: David Miller <davem@davemloft.net>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	syadagir@codeaurora.org, mjavid@codeaurora.org,
	evgreen@chromium.org, Ben Chan <benchan@google.com>,
	Eric Caruso <ejcaruso@google.com>,
	abhishek.esse@gmail.com,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 12/18] soc: qcom: ipa: immediate commands
Date: Mon, 20 May 2019 16:50:27 +0200	[thread overview]
Message-ID: <CAK8P3a3161Oc3ALB74LTuDny-aO9E4VGYpmqQSNoDNbj6PhRsQ@mail.gmail.com> (raw)
In-Reply-To: <5d948d74-f739-0cfa-8fae-b15c20fbe7ec@linaro.org>

On Sat, May 18, 2019 at 2:34 AM Alex Elder <elder@linaro.org> wrote:
> On 5/15/19 7:35 AM, Alex Elder wrote:
> > On 5/15/19 3:16 AM, Arnd Bergmann wrote:
> >>
> >> This looks rather strange. I think I looked at it before and you explained
> >> it, but I have since forgotten what you do it for, so I assume everyone else
> >> that tries to understand this will have problems too.
> >
> > This is a bug.  I think I misunderstood why you were
> > puzzled before.  Now I get it.  I need to save that
> > DMA address and not free it at the end of the function
> > (except on error).
>
> OK, now I'm going to correct myself.  I hope I don't make
> any mistakes here because things are confused enough...
>
> Part of what I described previously is still true, namely
> there are tables that need to be initialized (i.e., the
> IPA needs to be told where they reside), and there is a
> separate step is available to zero the content of the tables.
>
> But there really is no need for the AP to hang onto this
> DMA memory after this immediate command has been issued.
> I will add comments in the code to make it less surprising.
>
> But here's a summary of why.
>
> I think there are two things at play that make it confusing.
>
> The first thing is that these "header tables" are actually
> located in a region of shared memory ("smem") that is local
> to the IPA (not the AP).  The the IPA_CMD_HDR_INIT_LOCAL
> immediate command is meant to:
> 1) define the header table location in IPA local memory
> 2) define the header table size
> 3) provide a buffer used to fill the table with its initial
>    contents
>
> The location and size are encoded in the flags field
> of the payload (offset and size).
>
> The initial contents are filled via DMA from a buffer
> in main memory, whose DMA address is supplied in the
> hdr_table_addr parameter in the payload.  The initial
> contents we supply are all zero.  So this is why we
> need to allocate DMA memory.
>
> The second thing is that this is an instance where the
> AP is responsible for performing some initialization
> of resources it may not "own" thereafter.  The IPA
> hardware owns this table, even though the AP needs to
> tell it where it sits in IPA local memory.  The AP is
> able to copy (using DMA) content into that table, but
> doing so involves a DMA transfer.
>
> More advanced features of the IPA would make more use
> of this header table, but those features not yet
> supported so this initialization (and a subsequent,
> seemingly redundant zeroing) is all we do.
>
> Does that make sense?

Ok, that sounds reasonable, yes. I'm not sure if
dma_alloc_coherent() guarantees zero-initialization
though, so if that is required, you may have to
add a memset().

       Arnd