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

[Alex Elder]

On 5/15/19 7:35 AM, Alex Elder wrote:
> On 5/15/19 3:16 AM, Arnd Bergmann wrote:
>> On Sun, May 12, 2019 at 3:25 AM Alex Elder <elder@xxxxxxxxxx> wrote:
>>
>>> +/* Initialize header space in IPA local memory */
>>> +int ipa_cmd_hdr_init_local(struct ipa *ipa, u32 offset, u32 size)
>>> +{
>>> +       struct ipa_imm_cmd_hw_hdr_init_local *payload;
>>> +       struct device *dev = &ipa->pdev->dev;
>>> +       dma_addr_t addr;
>>> +       void *virt;
>>> +       u32 flags;
>>> +       u32 max;
>>> +       int ret;
>>> +
>>> +       /* Note: size *can* be zero in this case */
>>> +       if (size > field_max(IPA_CMD_HDR_INIT_FLAGS_TABLE_SIZE_FMASK))
>>> +               return -EINVAL;
>>> +
>>> +       max = field_max(IPA_CMD_HDR_INIT_FLAGS_HDR_ADDR_FMASK);
>>> +       if (offset > max || ipa->shared_offset > max - offset)
>>> +               return -EINVAL;
>>> +       offset += ipa->shared_offset;
>>> +
>>> +       /* A zero-filled buffer of the right size is all that's required */
>>> +       virt = dma_alloc_coherent(dev, size, &addr, GFP_KERNEL);
>>> +       if (!virt)
>>> +               return -ENOMEM;
>>> +
>>> +       payload = kzalloc(sizeof(*payload), GFP_KERNEL);
>>> +       if (!payload) {
>>> +               ret = -ENOMEM;
>>> +               goto out_dma_free;
>>> +       }
>>> +
>>> +       payload->hdr_table_addr = addr;
>>> +       flags = u32_encode_bits(size, IPA_CMD_HDR_INIT_FLAGS_TABLE_SIZE_FMASK);
>>> +       flags |= u32_encode_bits(offset, IPA_CMD_HDR_INIT_FLAGS_HDR_ADDR_FMASK);
>>> +       payload->flags = flags;
>>> +
>>> +       ret = ipa_cmd(ipa, IPA_CMD_HDR_INIT_LOCAL, payload, sizeof(*payload));
>>> +
>>> +       kfree(payload);
>>> +out_dma_free:
>>> +       dma_free_coherent(dev, size, virt, addr);
>>> +
>>> +       return ret;
>>> +}
>>
>> 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?

					-Alex


> Here's what I think happened.  There are two parts of
> initializing these tables.  One part tells the hardware
> where the table is located.  Another part zeroes the
> contents of those tables.  (The zeroing part could be
> accomplished when the table is allocated, but there
> are cases where they have to be zeroed again without
> needing to tell the hardware so we need to at least
> be able to do that independently.)
> 
> I think I was assuming this was the function that did
> the zeroing, and I thought that adding the comment about
> "all we need is a zero-filled buffer" addressed what
> you thought should be made clearer.
> 
> I will definitely fix this, and I'm glad you repeated
> it so I was forced to take another look.
> 
> If I again misunderstand your point, please let me know.
> 
> 					-Alex
> 
>> The issue I see is that you do an expensive dma_alloc_coherent()
>> but then never actually use the pointer returned by it, only the
>> dma address that cannot be turned back into a virtual address
>> in order to access the data in it.
>>
>> If you can't actually use payload->hdr_table_addr, why even allocate
>> it here?
>>
>>      Arnd
>>
>