drivers: dma: esp32: added support for multiple descriptors #74974

epc-ake · 2024-06-25T15:12:21Z

In the past, configuring the GDMA was limited to using a single descriptor, which restricted memory transfers to a maximum of 4kB.

This pull request introduces support for multiple descriptors, allowing users to define multiple dma_blocks.
The maximum number of descriptors available can be configured using the CONFIG_DMA_ESP32_DESCRIPTOR_NUM option.
Additionally, the dma_get_status() function now provides the index of the currently processed descriptor through the status.read_position and status.write_position fields.

This is my first PR, and I welcome any feedback!

github-actions · 2024-06-25T15:13:03Z

Hello @epc-ake, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

marekmatej · 2024-06-25T17:17:34Z

@epc-ake please fix the marked compliance errors, also please use the single new-line in code.

drivers/dma/dma_esp32_gdma.c

epc-ake · 2024-07-01T16:20:10Z

I'm not happy about the descriptors getting defined twice, once trough the API and and then again in the driver to convert dma_config_blocks to dma_descriptor_t.
An alternative I could think of is to ignore the dma_descriptor_t max buffer size when defining a dma_config_block block and then generate as many dma_descriptor_t descriptors as needed to fit the size of a dma_config_block block.
At least this would reduce the number of dma_config_block blocks. (most of the time to 1 block i guess)

Any inputs on that?

sylvioalves · 2024-07-02T11:46:45Z

@raffarost, can you take a look?

raffarost · 2024-07-02T18:04:05Z

hi @epc-ake,
thanks for submitting your code as a PR. I tried you changes locally with the default Zephyr tests for DMA but it won't pass them:

tests/drivers/dma/loop_transfer
tests/drivers/uart/uart_async_api

would you be willing to take a look and make sure the original functionality is preserved? we could afterwards do a full review on your code. A first question I'd have is: should the number of descriptors be a constant? Considering other subsystems in Zephyr might use only one descriptor (as in the original code), how would that work out?

epc-ake · 2024-07-03T08:44:03Z

@raffarost Thanks for reviewing and pointing me to these tests—I wasn't aware of them.

I had a bug in the TX path, but it’s resolved now.

tests/drivers/dma/loop_transfer seems to pass now.
However, tests/drivers/uart/uart_async_api still has two failing tests, which were already failing before my changes.
A quick look suggests that the driver cannot handle access to external memory (e.g., this line fails for external memory):
https://github.com/zephyrproject-rtos/zephyr/blob/8d3b16e13bbb80f22c52e23535907975651f8644/drivers/dma/dma_esp32_gdma.c#L167
It might be best to open a new issue for this since the GDMA should be able to access external memory.

Regarding your question, CONFIG_DMA_ESP32_DESCRIPTOR_NUM reserves memory for the specified number of descriptors.
If an application specifies only one block, only one descriptor is initialized in that memory.
Initially, I allocated the needed memory for the DMA dynamically on the heap, but the API documentation advises against this, and I agree with their recommendation:
https://docs.zephyrproject.org/latest/hardware/peripherals/dma.html#transfer-descriptor-memory-management

As I mentioned, I’m not entirely satisfied with the current solution, but it’s the only feasible option using the DMA API.
You can find a usage example for this PR in the following link, where you might also see the problem I described earlier.
https://github.com/zephyrproject-rtos/zephyr/blob/a705d8595e1a724da22a44dbf102ab0cf18967f2/drivers/video/video_esp32_dvp.c#L161

raffarost

Just minor checks and suggestions, overall I think your code looks good

raffarost · 2024-07-03T12:46:48Z

drivers/dma/dma_esp32_gdma.c

@@ -80,12 +80,16 @@ static void IRAM_ATTR dma_esp32_isr_handle_rx(const struct device *dev,

 	gdma_ll_rx_clear_interrupt_status(data->hal.dev, rx->channel_id, intr_status);

-	if (intr_status & (GDMA_LL_EVENT_RX_SUC_EOF | GDMA_LL_EVENT_RX_DONE)) {
-		intr_status &= ~(GDMA_LL_EVENT_RX_SUC_EOF | GDMA_LL_EVENT_RX_DONE);
+	if (intr_status == (GDMA_LL_EVENT_RX_SUC_EOF | GDMA_LL_EVENT_RX_DONE)) {


Here I would avoid reassigning the variable with a different data type.
Is there another way to pass the status without doing this?

The only way I see is to create an extra variable for it.
I can do that if you like, but I wait for your feedback.

drivers/dma/dma_esp32_gdma.c

raffarost · 2024-07-03T12:48:49Z

drivers/dma/dma_esp32_gdma.c

 {
+	dma_descriptor_t *desc_iter = dma_channel->desc_list;


Can you move this right before the loop?
If validation of input parameter fails, running this instruction can be avoided.

drivers/dma/dma_esp32_gdma.c

drivers/dma/Kconfig.esp32

raffarost · 2024-07-03T14:38:56Z

I'm not happy about the descriptors getting defined twice, once trough the API and and then again in the driver to convert dma_config_blocks to dma_descriptor_t. An alternative I could think of is to ignore the dma_descriptor_t max buffer size when defining a dma_config_block block and then generate as many dma_descriptor_t descriptors as needed to fit the size of a dma_config_block block. At least this would reduce the number of dma_config_block blocks. (most of the time to 1 block i guess)

Any inputs on that?

I see now your concern regarding initializing descriptors. Do you think converting desc_list[] from data type dma_descriptor_t to dma_config_block and adjusting the internals of the driver for that change is possible? To me it looks like doing that loop inside the driver might not even be a productive task for the driver to do, and could be done outside, unless there's some unavoidable validation to be done internally or some lower layer detail forbids us to do so.

Edit: I checked again and it seems unlikely we could adapt the LL workings with Zephyr's data type. We could either leave it as you implemented or do the initialization of dma_descriptor_t * instead of dma_block_config * outside and pass it to the driver. This would violate data typing (terrible idea) but the current implementation seems to not use the other fields of dma_block_config. Other than this, I'd prefer others to give their opinions on this regard.

raffarost · 2024-07-22T15:24:22Z

Edit: I checked again and it seems unlikely we could adapt the LL workings with Zephyr's data type. We could either leave it as you implemented or do the initialization of dma_descriptor_t * instead of dma_block_config * outside and pass it to the driver. This would violate data typing (terrible idea) but the current implementation seems to not use the other fields of dma_block_config. Other than this, I'd prefer others to give their opinions on this regard.

I changed my mind. I think this might be the better option for now and then try to alter the DMA API? E.g. removing the dma_block_config struct and passing a void pointer instead. Or removing the dma_block_config completely from the DMA API and let the DMA drivers deal with how to split up the memory into descriptors. Also zephyrs dma_block_config is quiet overloaded (e.g. >=30bytes per block). The dma_descriptor_t from esp hal only uses 12bytes.

Hi @epc-ake,
I'm somewhat new to Zephyr, so I'm not sure I can give a more qualified opinion on this. My first thought is that it would be interesting if dma_block_config had a void field, so we could pass the descriptors already filled in the right format. I'm not sure this is a genuine claim though, and would ask first if there are other drivers which implement multiple descriptors and how they do it. I won't be able to dig into this now, as I have to work on other things, but I thought I might just share what I think for now. It would be nice if someone more experienced in this subsystem chimed in.

teburd · 2024-07-22T15:59:50Z

Edit: I checked again and it seems unlikely we could adapt the LL workings with Zephyr's data type. We could either leave it as you implemented or do the initialization of dma_descriptor_t * instead of dma_block_config * outside and pass it to the driver. This would violate data typing (terrible idea) but the current implementation seems to not use the other fields of dma_block_config. Other than this, I'd prefer others to give their opinions on this regard.

I changed my mind. I think this might be the better option for now and then try to alter the DMA API? E.g. removing the dma_block_config struct and passing a void pointer instead. Or removing the dma_block_config completely from the DMA API and let the DMA drivers deal with how to split up the memory into descriptors. Also zephyrs dma_block_config is quiet overloaded (e.g. >=30bytes per block). The dma_descriptor_t from esp hal only uses 12bytes.

This is the downside generally of the DMA API as an attempt to sort of cover all possible hardware with a single API. In DMA we don't end up with a least common denominator but instead a union of all DMA functionality all hardware in Zephyr supports.

Mostly this isn't a problem to be honest though, DMA is typically embedded in various bus drivers rather than being exposed as an application API. It has one application level user in SoF which requires some specific DMA drivers to ensure things work as expected.

teburd · 2024-07-22T16:00:56Z

Edit: I checked again and it seems unlikely we could adapt the LL workings with Zephyr's data type. We could either leave it as you implemented or do the initialization of dma_descriptor_t * instead of dma_block_config * outside and pass it to the driver. This would violate data typing (terrible idea) but the current implementation seems to not use the other fields of dma_block_config. Other than this, I'd prefer others to give their opinions on this regard.

I changed my mind. I think this might be the better option for now and then try to alter the DMA API? E.g. removing the dma_block_config struct and passing a void pointer instead. Or removing the dma_block_config completely from the DMA API and let the DMA drivers deal with how to split up the memory into descriptors. Also zephyrs dma_block_config is quiet overloaded (e.g. >=30bytes per block). The dma_descriptor_t from esp hal only uses 12bytes.

Hi @epc-ake, I'm somewhat new to Zephyr, so I'm not sure I can give a more qualified opinion on this. My first thought is that it would be interesting if dma_block_config had a void field, so we could pass the descriptors already filled in the right format. I'm not sure this is a genuine claim though, and would ask first if there are other drivers which implement multiple descriptors and how they do it. I won't be able to dig into this now, as I have to work on other things, but I thought I might just share what I think for now. It would be nice if someone more experienced in this subsystem chimed in.

This is wrong, if you wish to implement the DMA API today, you would convert from the dma.h descriptors to your hardware descriptors. That's it. If you are unhappy with the zephyr dma.h API and descriptors there's nothing preventing your device from saying "please use the hal driver if you wish to use DMA directly".

LucasTambor · 2024-08-06T12:50:55Z

@epc-ake Would you mind fixing the compliance checks and conflicts so we can merge this PR? Nice work!

raffarost · 2024-08-06T16:30:45Z

@epc-ake Would you mind fixing the compliance checks and conflicts so we can merge this PR? Nice work!

@epc-ake a rebase will be needed as well

drivers/dma/dma_esp32_gdma.c

Previously, configuring the GDMA was limited to a single descriptor, restricting memory transfers to a maximum of 4kB. This update introduces support for multiple descriptors, enabling users to define multiple dma_blocks. The maximum number of descriptors can be configured via the CONFIG_DMA_ESP32_DESCRIPTOR_NUM option. Additionally, the dma_get_status() function now reports the index of the currently processed descriptor through the status.read_position and status.write_position fields. Signed-off-by: Armin Kessler <[email protected]>

sylvioalves · 2024-08-12T12:21:39Z

drivers/dma/dma_esp32_gdma.c

@@ -127,17 +132,43 @@ static void IRAM_ATTR dma_esp32_isr_handle(const struct device *dev, uint8_t rx_
 #endif

 static int dma_esp32_config_rx_descriptor(struct dma_esp32_channel *dma_channel,
-					   struct dma_block_config *block)
+						struct dma_block_config *block)
 {


Would you update this to meet compliance check? Same for other places.

drivers/dma/Kconfig.esp32

sylvioalves · 2024-08-12T12:23:04Z

LGTM, need to check compliance stuff.

github-actions · 2024-08-12T23:24:20Z

Hi @epc-ake!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

zephyrbot added platform: ESP32 Espressif ESP32 area: DMA Direct Memory Access labels Jun 25, 2024

zephyrbot requested review from LucasTambor, marekmatej, sylvioalves, teburd and uLipe June 25, 2024 15:12

zephyrbot assigned teburd Jun 25, 2024

epc-ake force-pushed the gdma_multi_descriptor branch from 76e1dae to eba4979 Compare June 26, 2024 09:37

marekmatej reviewed Jun 26, 2024

View reviewed changes

drivers/dma/dma_esp32_gdma.c Outdated Show resolved Hide resolved

marekmatej reviewed Jun 26, 2024

View reviewed changes

drivers/dma/dma_esp32_gdma.c Outdated Show resolved Hide resolved

epc-ake force-pushed the gdma_multi_descriptor branch 2 times, most recently from 37a1e9f to d7edb29 Compare June 26, 2024 11:26

epc-ake marked this pull request as draft July 2, 2024 07:16

epc-ake force-pushed the gdma_multi_descriptor branch from d7edb29 to 64c1d91 Compare July 2, 2024 11:29

epc-ake force-pushed the gdma_multi_descriptor branch 2 times, most recently from 54d2ba4 to 27c316a Compare July 2, 2024 14:04

teburd added this to the v4.0.0 milestone Jul 2, 2024

epc-ake mentioned this pull request Jul 2, 2024

Camera interface esp32s3 #75331

Closed

epc-ake mentioned this pull request Jul 3, 2024

[Page Add][Enhanced Function]Support the camera interface for XIAO ESP32S3 Sense, by Zephyr Seeed-Studio/wiki-documents#1060

Closed

epc-ake force-pushed the gdma_multi_descriptor branch from 27c316a to 8d3b16e Compare July 3, 2024 08:18

raffarost reviewed Jul 3, 2024

View reviewed changes

drivers/dma/Kconfig.esp32 Outdated Show resolved Hide resolved

uLipe previously approved these changes Jul 16, 2024

View reviewed changes

teburd previously approved these changes Jul 22, 2024

View reviewed changes

LucasTambor previously approved these changes Aug 6, 2024

View reviewed changes

epc-ake dismissed stale reviews from LucasTambor, teburd, and uLipe via 50f41b7 August 6, 2024 13:10

epc-ake force-pushed the gdma_multi_descriptor branch 2 times, most recently from 50f41b7 to 8965005 Compare August 6, 2024 15:08

epc-ake force-pushed the gdma_multi_descriptor branch from 8965005 to 070d5af Compare August 7, 2024 06:52

raffarost reviewed Aug 8, 2024

View reviewed changes

drivers/dma/dma_esp32_gdma.c Outdated Show resolved Hide resolved

epc-ake force-pushed the gdma_multi_descriptor branch from 070d5af to b37adee Compare August 12, 2024 06:55

raffarost approved these changes Aug 12, 2024

View reviewed changes

sylvioalves reviewed Aug 12, 2024

View reviewed changes

drivers/dma/Kconfig.esp32 Show resolved Hide resolved

LucasTambor requested review from sylvioalves, marekmatej and LucasTambor August 12, 2024 16:10

LucasTambor approved these changes Aug 12, 2024

View reviewed changes

teburd approved these changes Aug 12, 2024

View reviewed changes

nashif merged commit 237c49a into zephyrproject-rtos:main Aug 12, 2024
22 checks passed

epc-ake mentioned this pull request Aug 15, 2024

drivers: video: Add SMH option for video buffer #76982

Merged

epc-ake deleted the gdma_multi_descriptor branch September 19, 2024 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: dma: esp32: added support for multiple descriptors #74974

drivers: dma: esp32: added support for multiple descriptors #74974

epc-ake commented Jun 25, 2024

github-actions bot commented Jun 25, 2024

marekmatej commented Jun 25, 2024

epc-ake commented Jul 1, 2024

sylvioalves commented Jul 2, 2024 •

edited

Loading

raffarost commented Jul 2, 2024

epc-ake commented Jul 3, 2024

raffarost left a comment

raffarost Jul 3, 2024

epc-ake Jul 3, 2024 •

edited

Loading

raffarost Jul 3, 2024

raffarost commented Jul 3, 2024 •

edited

Loading

raffarost commented Jul 22, 2024

teburd commented Jul 22, 2024

teburd commented Jul 22, 2024

LucasTambor commented Aug 6, 2024

raffarost commented Aug 6, 2024

sylvioalves Aug 12, 2024 •

edited

Loading

sylvioalves commented Aug 12, 2024 •

edited

Loading

github-actions bot commented Aug 12, 2024

drivers: dma: esp32: added support for multiple descriptors #74974

drivers: dma: esp32: added support for multiple descriptors #74974

Conversation

epc-ake commented Jun 25, 2024

github-actions bot commented Jun 25, 2024

marekmatej commented Jun 25, 2024

epc-ake commented Jul 1, 2024

sylvioalves commented Jul 2, 2024 • edited Loading

raffarost commented Jul 2, 2024

epc-ake commented Jul 3, 2024

raffarost left a comment

Choose a reason for hiding this comment

raffarost Jul 3, 2024

Choose a reason for hiding this comment

epc-ake Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

raffarost Jul 3, 2024

Choose a reason for hiding this comment

raffarost commented Jul 3, 2024 • edited Loading

raffarost commented Jul 22, 2024

teburd commented Jul 22, 2024

teburd commented Jul 22, 2024

LucasTambor commented Aug 6, 2024

raffarost commented Aug 6, 2024

sylvioalves Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

sylvioalves commented Aug 12, 2024 • edited Loading

github-actions bot commented Aug 12, 2024

sylvioalves commented Jul 2, 2024 •

edited

Loading

epc-ake Jul 3, 2024 •

edited

Loading

raffarost commented Jul 3, 2024 •

edited

Loading

sylvioalves Aug 12, 2024 •

edited

Loading

sylvioalves commented Aug 12, 2024 •

edited

Loading