Skip to content

Heap refinement Part2 -- Dw dma fixes #4664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
keyonjie wants to merge 2 commits into from
Closed

Heap refinement Part2 -- Dw dma fixes #4664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7fa2f7
Revert "dw-dma: set LLI alignment to platform specific"
keyonjie Aug 20, 2021
30e70f3
dw-dma: change to use runtime_shared heap for lli buffer
keyonjie Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/drivers/dw/dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,9 +557,8 @@ static int dw_dma_set_config(struct dma_chan_data *channel,
if (dw_chan->lli)
rfree(dw_chan->lli);

dw_chan->lli = rballoc_align(0, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA,
sizeof(struct dw_lli) * channel->desc_count,
PLATFORM_DCACHE_ALIGN);
dw_chan->lli = rmalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM,
sizeof(struct dw_lli) * channel->desc_count);
Copy link
Collaborator

@lyakh lyakh Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it have to be zeroed? It wasn't zeroed before. If that was causing problems because we expect zeroes somewhere, then that should be fixed explicitly! Otherwise rmalloc() should do just fine.

Copy link
Member

@plbossart plbossart Sep 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you don't need alignment any longer? That seems very odd for hardware descriptors?

Copy link
Contributor Author

@keyonjie keyonjie Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you don't need alignment any longer? That seems very odd for hardware descriptors?

@plbossart the rmalloc() internal guarantees the return pointer is PLATFORM_DCACHE_ALIGN aligned, please see the source here:

sof/src/lib/alloc.c

Line 718 in 8fc5247

return get_ptr_from_heap(heap, flags, caps, bytes, PLATFORM_DCACHE_ALIGN);

if (!dw_chan->lli) {
tr_err(&dwdma_tr, "dw_dma_set_config(): dma %d channel %d lli alloc failed",
channel->dma->plat_data.id,
Expand Down
11 changes: 3 additions & 8 deletions src/include/sof/drivers/dw-dma.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,6 @@
#define DW_DMA_BUFFER_ALIGNMENT 0x4
#define DW_DMA_COPY_ALIGNMENT 0x4

/* LLI address alignment, in Bytes */
#ifndef DW_DMA_LLI_ALIGN
#define DW_DMA_LLI_ALIGN 32
#endif

/* TODO: add FIFO sizes */
struct dw_chan_data {
uint16_t class;
Expand All @@ -155,10 +150,10 @@ struct dw_lli {
uint32_t sstat;
uint32_t dstat;

/* align to required bytes to make sure every item
* is aligned in case of more than two items
/* align to 32 bytes to not cross cache line
* in case of more than two items
*/
uint8_t reserved[DW_DMA_LLI_ALIGN - sizeof(uint32_t) * 7];
uint32_t reserved;
Copy link
Collaborator

@lyakh lyakh Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was aeda015 a guess or a documented feature? How did we come up with 128 bytes on TGL?

Copy link
Contributor Author

@keyonjie keyonjie Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was aeda015 a guess or a documented feature? How did we come up with 128 bytes on TGL?

Unfortunately, It was a guess only.

} __packed;

extern const struct dma_ops dw_dma_ops;
Expand Down
3 changes: 0 additions & 3 deletions src/platform/tigerlake/include/platform/drivers/dw-dma.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@

#include <cavs/drivers/dw-dma.h>

/* LLI address alignment, in Bytes */
#define DW_DMA_LLI_ALIGN 128

#endif /* __PLATFORM_DRIVERS_DW_DMA_H__ */

#else
Expand Down