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

[Arnd Bergmann]

On Sat, May 18, 2019 at 2:34 AM Alex Elder <elder@xxxxxxxxxx> 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