Persist DMA buffer in sdmmc_read_sectors/sdmmc_write_sectors (IDFGH-12770)

[ducalex]

Is your feature request related to a problem?

When internal memory runs out, SD card reads and writes start returning ESP_ERR_NO_MEM because this line fails:

void* tmp_buf = heap_caps_malloc(block_size, MALLOC_CAP_DMA);

In theory SPIRAM_MALLOC_RESERVE_INTERNAL should ensure that memory is always available but in practice it doesn't always work.

There are likely also performance implications to allocating/deallocating all the time (in some cases).

Having no DMA-capable memory left is obviously an undesirable situation in general, but it can happen and when it does we might still need SD card access.

Describe the solution you'd like.

My understanding is that block_size is at most 512 bytes: out_csd->sector_size = MIN(read_bl_size, 512);.

Therefore there is no reason to not keep the buffer around after it's first allocated for future use. In fact with that size it could be allocated preemptively when the SD card is initialized, or even placed in static BSS. Because if it's needed once, it will very likely be needed again!

Describe alternatives you've considered.

My current solution is this:

1. Reserve a modest DMA-capable memory block on start
2. Wrap sdmmc_host_t's do_transaction() to detect errors
3. If an error occurs, free the reserved block and try the transaction again
4. If the transaction succeeds, try to grab the block back

But this is far from ideal and doesn't always work...

Additional context.

#6596 suggested the same but was closed because of the emphasis on performance. I think the emphasis should be on stability improvements if this change is introduced.

[huming2207]

My understanding is that block_size is at most 512 bytes

Hmm I think that's not true? There are quite a lot of those big SD cards that comes with 1024 or even 4096 bytes sector.

Even some SD NAND chips for industrial purposes uses 1024 bytes sector. I've definitely seen a 16GB one doing that but I can't remember the part number

[rrtandler]

Hi @ducalex,
From what you have described so far, it looks like the rest of application is using DMA capable memory space so heavily, that there isn't even 512B block left at certain moment, when it comes to SD card access. I think keeping the buffer for SD allocated statically aka "forever" would just expose another place in the code, where the DMA capable memory won't be available when needed. Could you check the rest of your code for source of potential leak / fragmentation of DMA capable RAM ?

[ducalex]

Hmm I think that's not true? There are quite a lot of those big SD cards that comes with 1024 or even 4096 bytes sector.

That is what I thought at first, but when I looked up the actual code in esp-idf, the only assignment I've found was the line I've quoted. Which would result in 512 or less.

Perhaps I missed another code path somewhere?

[huming2207]

That is what I thought at first, but when I looked up the actual code in esp-idf, the only assignment I've found was the line I've quoted. Which would result in 512 or less.

Ah crap, yea I look down the code and indeed I can find them here:

• esp-idf/components/sdmmc/sdmmc_sd.c

  Line 388 in d4cd437

  out_csd->sector_size = MIN(read_bl_size, 512);

• esp-idf/components/sdmmc/sdmmc_mmc.c

  Line 202 in d4cd437

  out_csd->sector_size = MIN(read_bl_size, 512);

I think this might be a bug?? Maybe it supposed to be MAX() instead? Otherwise any newer/larger SD cards won't be read/write properly.

Meanwhile, I believe these sort of DMA buffers shouldn't be statically allocated, because we do have use cases where two or more SD cards are needed (that shares with the same SPI but different CS). We have PSRAM here so we have sufficient RAM for heap allocations anyway.

[ducalex]

I agree that it shouldn't be static, it was hyperbole on my part! I also agree that running out of DMA-capable memory indicates bigger problems.

However I maintain that it would be beneficial to keep the buffer after it's been used for the first time, because it will undoubtedly be needed again so that space is already pseudo-reserved. Making it explicit is simply more efficient and improves resilience.

That being said, my proposal hinges on the fact that the buffer is small, 1KB or less. If there is a bug and the sector size can in fact be 4KB or even more then yes, it is obviously unreasonable to keep that around!

[huming2207]

However I maintain that it would be beneficial to keep the buffer after it's been used for the first time, because it will undoubtedly be needed again so that space is already pseudo-reserved. Making it explicit is simply more efficient and improves resilience.

Hmm yep, we do need to allocate at initialisation and free on deinit, not on every write op.

I will submit a pull request first to fix the MIN() issue, that will definitely become a big troublemaker on our firmware. Then I will see if I can work out a way to fix this heap allocation.

[igrr]

Thanks for the idea @ducalex! I agree that avoiding repeated allocations/deallocations could help with reliability.

Recently we have added a new dma_aligned_buffer member to sdmmc_host_t, which has a similar purpose: it provides a DMA-capable buffer which can be reused between operations. It's kind of misplaced in sdmmc_host_t, actually, as the usage occurs in the sdmmc component (protocol layer), not in the host driver, but that's something we have to refactor...
I think we could reuse this dma_aligned_buffer for read/write commands as well.