Upstream: rework or add comments to snd_sof_create_page_table() #652

Activity

1. plbossart

   added

   upstream blockerMandatory for upstreamMandatory for upstream

   on Feb 14, 2019
2. wenqingfu

   assigned

   lyakh

   on Feb 27, 2019
3. plbossart commented on Mar 6, 2019

   

   plbossart

   on Mar 6, 2019

   MemberAuthor

   More actions

   @lyakh @lgirdwood @ranj063 could you double-check the points below:

   1. I believe no one is using the offset field in the following structure to represent the ring buffer:

   /* stream ring info */
   struct sof_ipc_host_buffer {
   	struct sof_ipc_hdr hdr;
   	uint32_t phy_addr;
   	uint32_t pages;
   	uint32_t size;
   	uint32_t offset;
   	uint32_t reserved[2];
   } __packed;

   Should we reclaim this offset value as a reserved element?

   2. We have an implicit little-endian ordering of the compressed page table. Each page table is represented with 20 bits, so 2.5 bytes, and in the firmware they are extracted this way:

   for (i = 0; i < ring->pages; i++) {
   		idx = (((i << 2) + i)) >> 1;
   		phy_addr = page_table[idx] | (page_table[idx + 1] << 8)
   				| (page_table[idx + 2] << 16);
   
   		if (i & 0x1)
   			phy_addr <<= 8;
   		else
   			phy_addr <<= 12;
   		phy_addr &= 0xfffff000;

   That looks totally LittleEndian to me.

   3. should we then change the kernel code to explicitly use __le32 and cpu_to_le32() when creating the page table?

   4. or should we allow for a configurable endianess, signaled a reserved bit? That would be the more generic way of doing things.

   5. I believe what we do is borderline correct in terms of address alignment. In snd_sof_create_page_table(), we use a pointer to u32, but the address will be aligned to bytes 2, 5, 7, 10, etc. It'd really be better to explicitly use byte access the same way it's done in firmware if we want to have portable code.
4. dbaluta commented on Mar 6, 2019

   

   dbaluta

   on Mar 6, 2019

   Collaborator

   More actions

   @plbossart Two questions here:

   1. Why there is a need to compress the page addresses. Is the memory that small?
   2. I noticed that page addresses are used on FW only if CONFIG_HOST_PTABLE is set. My question is how does the Host component know from where to copy the audio buffer data in the case where CONFIG_HOST_PTABLE is not defined (e.g on AppolloLake).
5. plbossart commented on Mar 6, 2019

   

   plbossart

   on Mar 6, 2019 via email

   MemberAuthor

   More actions

   1. Why there is a need to compress the page addresses. Is the memory that small?

   I am asking myself the very same question. We are trying to save 12 bits out of 32, and in the process add endianness, alignment and format issues while having obscure code that is far from being self-explanatory. Plus this actually saves no memory at all since the page table needs to be decompressed by the firmware, so we are talking about a saving of 1.5 per page passed over IPC. I believe it's really over-engineering... @lgirdwood @keyonjie, is this 32:20 compression based on an actual need? It's too late to remove it from the ABI but could be use linear tables moving forward?

   2. I noticed that page addresses are used on FW only if CONFIG_HOST_PTABLE is set. My question is how does the Host component know from where to copy the audio buffer data in the case where CONFIG_HOST_PTABLE is not defined (e.g on AppolloLake).

   I believe for ApolloLake the firmware does make implicit DMA requests by moving its read/write pointers but the DMA is programmed using the HDaudio BDL (Buffer Descriptor List). The firmware does not program access to external memory, only the DMA knows about it. For Baytrail the firmware programs the SG list and makes the DMA requests, so we do need to pass the location of DDR DMA-able pages to the firmware. If I am mistaken I am sure someone on the Intel team will throw rocks at my explanation ;-)
6. dbaluta commented on Mar 6, 2019

   

   dbaluta

   on Mar 6, 2019

   Last edited by dbaluta

   Collaborator

   More actions

   I believe for ApolloLake the firmware does make implicit DMA requests by moving its read/write pointers but the DMA is programmed using the HDaudio BDL (Buffer Descriptor List). The firmware does not program access to external memory, only the DMA knows about it.

   Ok, will re-investigate the HDA DMA / Linux code. It is strange that from Linux side I couldn't find any info about the DMA buffer address passed to FW. Do you say that the DMA is programmed with some implicit values from the Linux side? Because otherwise I don't know how the DMA driver on FW knows about the source address of audio buffer (in case of playback for example).

   For Baytrail the firmware programs the SG list and makes the DMA requests, so we do need to pass the location of DDR DMA-able pages to the firmware. If I am mistaken I am sure someone on the Intel team will throw rocks at my explanation ;-)

   Yes for Baytrail I could get a grasp of it.

   PS: I accidentally closed and re-opened this issue. Sorry!
7. dbaluta

   closed this as completedon Mar 6, 2019
8. dbaluta

   reopened this on Mar 6, 2019
9. keyonjie commented on Mar 7, 2019

   

   keyonjie

   on Mar 7, 2019

   More actions

   I believe for ApolloLake the firmware does make implicit DMA requests by moving its read/write pointers but the DMA is programmed using the HDaudio BDL (Buffer Descriptor List). The firmware does not program access to external memory, only the DMA knows about it.

   Ok, will re-investigate the HDA DMA / Linux code. It is strange that from Linux side I couldn't find any info about the DMA buffer address passed to FW. Do you say that the DMA is programmed with some implicit values from the Linux side? Because otherwise I don't know how the DMA driver on FW knows about the source address of audio buffer (in case of playback for example).

   For HDA host DMA controller on Intel SKL+, it requires configure host side buffer(size, address) from the host/Linux side, that is BDL as Pierre mentioned. The DMA driver on FW will only take care of the DSP side configuration to the DMA controller, e.g. setting the FW side buffer size and address.

   So in general, if your host DMAC(or actually just directly memcy without DMA) can be configured totally inside FW, you can do it in SKL-(e.g. baytrail) style.

   For Baytrail the firmware programs the SG list and makes the DMA requests, so we do need to pass the location of DDR DMA-able pages to the firmware. If I am mistaken I am sure someone on the Intel team will throw rocks at my explanation ;-)

   Yes for Baytrail I could get a grasp of it.

   PS: I accidentally closed and re-opened this issue. Sorry!
10. keyonjie commented on Mar 7, 2019

    

    keyonjie

    on Mar 7, 2019

    Last edited by keyonjie

    More actions

    1. Why there is a need to compress the page addresses. Is the memory that small?
       I am asking myself the very same question. We are trying to save 12 bits out of 32, and in the process add endianness, alignment and format issues while having obscure code that is far from being self-explanatory. Plus this actually saves no memory at all since the page table needs to be decompressed by the firmware, so we are talking about a saving of 1.5 per page passed over IPC. I believe it's really over-engineering... @lgirdwood @keyonjie, is this 32:20 compression based on an actual need? It's too late to remove it from the ABI but could be use linear tables moving forward?

    I remember we did the 32->20 bits compression with the intention to save IPC message size on baytrail at the early stage, where we have downlink(host->fw) mailbox could be smaller than 1KB, imagine if we configure ALSA buffer larger than 256 Pages which means we need 4B * 256 = 1KB can occupy the whole mailbox(or even overflow).
11. plbossart commented on Mar 7, 2019

    

    plbossart

    on Mar 7, 2019

    MemberAuthor

    More actions

    @keyonjie 256 pages of 4096 bytes gives you 1048576 bytes for a ring buffer. That's 5.46s of stereo audio at 48kHz/16 bits, or 170ms for 192 kHz/8ch/32 bits. Was this really what you wanted to support?
12. keyonjie commented on Mar 7, 2019

    

    keyonjie

    on Mar 7, 2019

    More actions

    @keyonjie 256 pages of 4096 bytes gives you 1048576 bytes for a ring buffer. That's 5.46s of stereo audio at 48kHz/16 bits, or 170ms for 192 kHz/8ch/32 bits. Was this really what you wanted to support?

    We tried 128 pages previously, but never for 256 yet. So I think that is only a reason we considered, we also considered the possibility of putting different IPC payloads in mailbox at the same time to support simultaneous IPC handling. Another reason is that this(compressing) is inherited from OTC byt/bdw driver.

    So those are what I can recall about why we are using compression here, @lgirdwood any comments?
13. plbossart commented on Mar 7, 2019

    

    plbossart

    on Mar 7, 2019

    MemberAuthor

    More actions

    This is addressed by #695 (but could be part of a different PR if we wanted to be consistent)
14. lyakh commented on Mar 18, 2019

    

    lyakh

    on Mar 18, 2019

    Collaborator

    More actions

    resolved by #711
15. lyakh

    closed this as completedon Mar 19, 2019