Merge branch 'bugfix/dma2d_dequeue_mechanism_v6.1' into 'release/v6.1'

fix(dma2d): add a dequeue mechanism for dma2d driver (v6.1)

See merge request espressif/esp-idf!50512

components/esp_driver_dma/include/esp_private/dma2d.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2023-2026 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -152,6 +152,29 @@ typedef struct {
  */
 esp_err_t dma2d_enqueue(dma2d_pool_handle_t dma2d_pool, const dma2d_trans_config_t *trans_desc, dma2d_trans_t *trans_placeholder);
 
+/**
+ * @brief Dequeue a pending 2D-DMA transaction from a 2D-DMA pool
+ *
+ * Remove a transaction that was previously enqueued by `dma2d_enqueue` but has not yet been picked up for
+ * processing (i.e. it is still waiting in the pool's pending queue). This is useful when the upper driver wants
+ * to cancel a transaction that has not started yet.
+ *
+ * @note This API only operates on transactions that are still pending. To end a transaction that is already
+ *       in-flight (its channels have been acquired), use `dma2d_force_end` instead.
+ *
+ * @note After this function returns ESP_OK, the upper driver can safely free the transaction placeholder, as the
+ *       `on_job_picked` callback of the transaction will no longer be called.
+ *
+ * @param[in] dma2d_pool 2D-DMA pool handle, allocated by `dma2d_acquire_pool`
+ * @param[in] trans Pointer to the 2D-DMA transaction context
+ * @return
+ *      - ESP_OK: Dequeue the pending 2D-DMA transaction successfully
+ *      - ESP_ERR_INVALID_ARG: Dequeue failed because of invalid argument
+ *      - ESP_ERR_NOT_FOUND: The transaction is not in the pending queue, because it has already been picked up
+ *                           for processing or it was never enqueued to this pool
+ */
+esp_err_t dma2d_dequeue(dma2d_pool_handle_t dma2d_pool, dma2d_trans_t *trans);
+
 /**
  * @brief Force end an in-flight 2D-DMA transaction
  *
@@ -167,7 +190,7 @@ esp_err_t dma2d_enqueue(dma2d_pool_handle_t dma2d_pool, const dma2d_trans_config
  * @param[in] trans Pointer to the 2D-DMA transaction context
  * @param[out] need_yield Pointer to a status flag to record whether a task switch is needed if this API is being called in an ISR
  * @return
- *      - ESP_OK: Force end an in-flight transaction successfully
+ *      - ESP_OK: The transaction has been ended (either force-ended by this call, or it had already completed)
  *      - ESP_ERR_INVALID_ARG: Force end failed because of invalid argument
  *      - ESP_ERR_INVALID_STATE: Force end failed because the transaction is not yet in-flight
  */

components/esp_driver_dma/src/dma2d.c

@@ -167,7 +167,35 @@ revert:
     return found;
 }
 
-/* This function will free up the RX channel and its bundled TX channels, then check for whether there is next transaction to be picked up */
+/* Atomically claim the teardown of the transaction that currently owns `rx_chan`.
+ *
+ * `status.transaction` is published (set) under the group spinlock when channels are
+ * acquired for a transaction, and is used here as a combined identity + claim token:
+ * the claim succeeds only if the channel is still owned by `expected` (the transaction
+ * the caller believes is in-flight). The winner clears it to NULL so that the other
+ * teardown path (the natural-completion ISR vs. dma2d_force_end) will fail its own
+ * claim and therefore will not free the same channels a second time.
+ *
+ * Passing a NULL `expected` never claims (there is nothing to tear down).
+ */
+FORCE_INLINE_ATTR bool claim_rx_transaction(dma2d_group_t *group, dma2d_rx_channel_t *rx_chan, dma2d_trans_t *expected)
+{
+    bool claimed = false;
+    esp_os_enter_critical_safe(&group->spinlock);
+    if (expected != NULL && rx_chan->base.status.transaction == expected) {
+        rx_chan->base.status.transaction = NULL;
+        claimed = true;
+    }
+    esp_os_exit_critical_safe(&group->spinlock);
+    return claimed;
+}
+
+/* This function will free up the RX channel and its bundled TX channels, then check for whether there is next transaction to be picked up.
+ *
+ * Precondition: the caller must have successfully claimed the teardown of this RX channel's transaction
+ * (i.e. cleared `rx_chan->base.status.transaction` from the owning transaction to NULL under the group
+ * spinlock, via `claim_rx_transaction` or the equivalent inline claim in `dma2d_force_end`). This
+ * guarantees the channels cannot be reassigned and that only one path performs the teardown. */
 static bool free_up_channels(dma2d_group_t *group, dma2d_rx_channel_t *rx_chan)
 {
     bool need_yield = false;
@@ -215,6 +243,7 @@ static bool free_up_channels(dma2d_group_t *group, dma2d_rx_channel_t *rx_chan)
     // 2. Check if next pending transaction in the tailq can start
     bool channels_found = false;
     const dma2d_trans_config_t *next_trans = NULL;
+    uint32_t total_channel_num = 0;
     dma2d_trans_channel_info_t channel_handle_array[DMA2D_MAX_CHANNEL_NUM_PER_TRANSACTION];
 
     esp_os_enter_critical_safe(&group->spinlock);
@@ -234,14 +263,9 @@ static bool free_up_channels(dma2d_group_t *group, dma2d_rx_channel_t *rx_chan)
 
     if (channels_found) {
         TAILQ_REMOVE(&group->pending_trans_tailq, next_trans_elm, entry);
-    }
-    esp_os_exit_critical_safe(&group->spinlock);
-
-    if (channels_found) {
-        // If the transaction can be processed, let consumer handle the transaction
-        uint32_t total_channel_num = next_trans->tx_channel_num + next_trans->rx_channel_num;
         // Store the acquired rx_chan into trans_elm (dma2d_trans_t) in case upper driver later need it to call `dma2d_force_end`
         // Upper driver controls the life cycle of trans_elm
+        total_channel_num = next_trans->tx_channel_num + next_trans->rx_channel_num;
         for (int i = 0; i < total_channel_num; i++) {
             if (channel_handle_array[i].dir == DMA2D_CHANNEL_DIRECTION_RX) {
                 next_trans_elm->rx_chan = channel_handle_array[i].chan;
@@ -249,7 +273,17 @@ static bool free_up_channels(dma2d_group_t *group, dma2d_rx_channel_t *rx_chan)
             // Also save the transaction pointer
             channel_handle_array[i].chan->status.transaction = next_trans_elm;
         }
+        // Mark the pick->start window: ownership is published (so the transaction already looks
+        // in-flight to dma2d_force_end) but on_job_picked has not configured/started the channels
+        // yet. dma2d_force_end must wait for `started` to become true before tearing channels down.
+        atomic_store(&next_trans_elm->started, false);
+    }
+    esp_os_exit_critical_safe(&group->spinlock);
+
+    if (channels_found) {
+        // If the transaction can be processed, let consumer handle the transaction
         need_yield |= next_trans->on_job_picked(total_channel_num, channel_handle_array, next_trans->user_config);
+        atomic_store(&next_trans_elm->started, true);
     }
     return need_yield;
 }
@@ -307,16 +341,26 @@ static NOINLINE_ATTR bool _dma2d_default_rx_isr(dma2d_group_t *group, int channe
     }
 
     // If last transaction completes (regardless success or not), free the channels
+    bool transaction_claimed = false;
     if (intr_status & (DMA2D_LL_EVENT_RX_SUC_EOF | DMA2D_LL_EVENT_RX_ERR_EOF | DMA2D_LL_EVENT_RX_DESC_ERROR | DMA2D_LL_EVENT_RX_DESC_EMPTY)) {
-        if (!(intr_status & DMA2D_LL_EVENT_RX_ERR_EOF)) {
-            assert(dma2d_ll_rx_is_fsm_idle(group->hal.dev, channel_id));
+        // Only free the channels if we win the claim for the transaction that owned this RX
+        // channel when the interrupt fired (edata.transaction, captured above).
+        // If dma2d_force_end already claimed it, or the channel has already been freed and reassigned to another transaction,
+        // the claim fails and we must not free here, otherwise we would tear down channels that no longer belong to this transaction.
+        // Likewise for the FSM idle sanity check.
+        if (claim_rx_transaction(group, rx_chan, edata.transaction)) {
+            transaction_claimed = true;
+            if (!(intr_status & DMA2D_LL_EVENT_RX_ERR_EOF)) {
+                assert(dma2d_ll_rx_is_fsm_idle(group->hal.dev, channel_id));
+            }
+            need_yield |= free_up_channels(group, rx_chan);
         }
-        need_yield |= free_up_channels(group, rx_chan);
     }
 
     // Handle last transaction's end callbacks (at this point, last transaction's channels are completely freed,
-    // therefore, we don't pass in channel handle to the callbacks anymore)
-    if (intr_status & DMA2D_LL_EVENT_RX_SUC_EOF) {
+    // therefore, we don't pass in channel handle to the callbacks anymore).
+    // Deliver the EOF callback is probably not expected if we lost the claim (i.e. transaction was ended by dma2d_force_end).
+    if (transaction_claimed && (intr_status & DMA2D_LL_EVENT_RX_SUC_EOF)) {
         if (on_recv_eof) {
             edata.rx_eof_desc_addr = suc_eof_desc_addr;
             need_yield |= on_recv_eof(NULL, &edata, user_data);
@@ -1013,57 +1057,111 @@ esp_err_t dma2d_enqueue(dma2d_pool_handle_t dma2d_pool, const dma2d_trans_config
         } else {
             TAILQ_INSERT_HEAD(&dma2d_group->pending_trans_tailq, trans_placeholder, entry);
         }
-    }
-    esp_os_exit_critical_safe(&dma2d_group->spinlock);
-    if (!enqueue) {
-        // Free channels available, start transaction immediately
+    } else { // free channels available and acquired
         // Store the acquired rx_chan into trans_placeholder (dma2d_trans_t) in case upper driver later need it to call `dma2d_force_end`
         // Upper driver controls the life cycle of trans_placeholder
         for (int i = 0; i < total_channel_num; i++) {
             if (channel_handle_array[i].dir == DMA2D_CHANNEL_DIRECTION_RX) {
                 trans_placeholder->rx_chan = channel_handle_array[i].chan;
             }
-            // Also save the transaction pointer
+            // Also save the transaction pointer to declare the ownership of the channels (has to be assigned in the same critical section)
             channel_handle_array[i].chan->status.transaction = trans_placeholder;
         }
+        // Mark the pick->start window (see free_up_channels): ownership is published but on_job_picked
+        // has not started the hardware yet, so dma2d_force_end must wait for `started` before tearing channels down.
+        atomic_store(&trans_placeholder->started, false);
+    }
+    esp_os_exit_critical_safe(&dma2d_group->spinlock);
+    if (!enqueue) { // start transaction immediately
         trans_desc->on_job_picked(total_channel_num, channel_handle_array, trans_desc->user_config);
+        atomic_store(&trans_placeholder->started, true);
     }
 
 err:
     return ret;
 }
 
+esp_err_t dma2d_dequeue(dma2d_pool_handle_t dma2d_pool, dma2d_trans_t *trans)
+{
+    ESP_RETURN_ON_FALSE(dma2d_pool && trans, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    dma2d_group_t *dma2d_group = dma2d_pool;
+
+    bool found = false;
+    esp_os_enter_critical(&dma2d_group->spinlock);
+    // The given transaction may have already been picked up (and thus removed from the queue) or may never
+    // have been enqueued. Removing such an element directly would corrupt the queue, so search the queue
+    // first and only remove it if it is genuinely still pending.
+    dma2d_trans_t *trans_elm;
+    TAILQ_FOREACH(trans_elm, &dma2d_group->pending_trans_tailq, entry) {
+        if (trans_elm == trans) {
+            // Safe to remove inside the loop because we break out immediately and never dereference the invalidated link pointer afterwards
+            TAILQ_REMOVE(&dma2d_group->pending_trans_tailq, trans, entry);
+            found = true;
+            break;
+        }
+    }
+    esp_os_exit_critical(&dma2d_group->spinlock);
+
+    // ESP_ERR_NOT_FOUND indicates the transaction is no longer (or was never) pending
+    // i.e. it has already been picked up for processing or it does not exist in this pool's queue
+    return found ? ESP_OK : ESP_ERR_NOT_FOUND;
+}
+
 esp_err_t dma2d_force_end(dma2d_trans_t *trans, bool *need_yield)
 {
-    ESP_RETURN_ON_FALSE_ISR(trans && trans->rx_chan && need_yield, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    ESP_RETURN_ON_FALSE_ISR(trans && need_yield, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
+    ESP_RETURN_ON_FALSE_ISR(trans->rx_chan, ESP_ERR_INVALID_STATE, TAG, "transaction still pending in the queue");
     assert(trans->rx_chan->direction == DMA2D_CHANNEL_DIRECTION_RX);
 
     dma2d_group_t *group = trans->rx_chan->group;
+    dma2d_rx_channel_t *rx_chan = group->rx_chans[trans->rx_chan->channel_id];
+
+    *need_yield = false;
 
     bool in_flight = false;
-    // We judge whether the transaction is in-flight by checking the RX channel it uses is being occupied or free
+    // We judge whether the transaction is in-flight by checking that the RX channel it uses is still owned by *this* transaction.
+    // Clearing the ownership in the same critical section claims the teardown: the natural
+    // completion ISR will then fail its own claim and will not free these channels again.
     esp_os_enter_critical_safe(&group->spinlock);
-    if (!(group->rx_channel_free_mask & (1 << trans->rx_chan->channel_id))) {
+    if (rx_chan->base.status.transaction == trans) {
         in_flight = true;
         dma2d_ll_rx_enable_interrupt(group->hal.dev, trans->rx_chan->channel_id, UINT32_MAX, false);
+        rx_chan->base.status.transaction = NULL; // claim the teardown of this transaction
         // DMA consumer could generate an error in both cases:
         // 1. when TX or RX is transferring data (channel not in idle state)
         // 2. TX successfully passed data to the module, but module cannot process the data, so RX has no data to delivery (RX channel in idle state)
     }
     esp_os_exit_critical_safe(&group->spinlock);
-    ESP_RETURN_ON_FALSE_ISR(in_flight, ESP_ERR_INVALID_STATE, TAG, "transaction not in-flight");
 
-    dma2d_rx_channel_t *rx_chan = group->rx_chans[trans->rx_chan->channel_id];
-    // Stop the RX channel and its bundled TX channels first
-    dma2d_stop(&rx_chan->base);
-    uint32_t tx_chans = rx_chan->bundled_tx_channel_mask;
-    for (int i = 0; i < DMA2D_LL_GET(TX_CHANS_PER_INST); i++) {
-        if (tx_chans & (1 << i)) {
-            dma2d_stop(&group->tx_chans[i]->base);
+    // If the transaction is no longer owned by this RX channel, it has already completed: its RX
+    // completion ISR has run and freed (and possibly reassigned) the channels. There is nothing to
+    // abort - the transaction has ended, which is exactly what the caller wants - so this is a no-op
+    // success.
+    if (in_flight) {
+        // We now exclusively own the teardown: the channels cannot be reassigned (their free-mask
+        // bits are still marked busy until free_up_channels runs) and a concurrent ISR will bail.
+
+        // The transaction may have been picked but not yet started: the completion ISR (or the
+        // immediate-start path in dma2d_enqueue) publishes ownership (making it look in-flight here)
+        // before calling `on_job_picked`, which configures and starts the channels outside the group
+        // spinlock. Wait for that start to finish before touching the channels, otherwise
+        // `on_job_picked` would configure/start channels that we are concurrently stopping and freeing.
+        // The wait is bounded (on_job_picked runs in ISR context and does not block) and can only
+        // happen across cores.
+        while (!atomic_load(&trans->started)) {
         }
+
+        // Stop the RX channel and its bundled TX channels first
+        dma2d_stop(&rx_chan->base);
+        uint32_t tx_chans = rx_chan->bundled_tx_channel_mask;
+        for (int i = 0; i < DMA2D_LL_GET(TX_CHANS_PER_INST); i++) {
+            if (tx_chans & (1 << i)) {
+                dma2d_stop(&group->tx_chans[i]->base);
+            }
+        }
+        // Then release channels
+        *need_yield = free_up_channels(group, rx_chan);
     }
-    // Then release channels
-    *need_yield = free_up_channels(group, rx_chan);
 
     return ESP_OK;
 }

components/esp_driver_dma/src/dma2d_priv.h

@@ -8,6 +8,7 @@
 
 #include <stdint.h>
 #include <sys/queue.h>
+#include <stdatomic.h>
 #include "sdkconfig.h"
 #include "freertos/FreeRTOS.h"
 #include "esp_intr_alloc.h"
@@ -45,6 +46,7 @@ struct dma2d_trans_s {
     TAILQ_ENTRY(dma2d_trans_s) entry;     // Link entry
     const dma2d_trans_config_t *desc;     // Pointer to the structure containing all configuration items of a transaction
     dma2d_channel_handle_t rx_chan;       // Pointer to the RX channel handle that will be used to do the transaction
+    _Atomic bool started;                 // False after the transaction's channels are picked (ownership published) until its `on_job_picked` (which configures/starts the hardware) has returned, then true. `dma2d_force_end` must wait for this to become true before tearing the channels down, otherwise it would race the in-progress start.
 };
 
 struct dma2d_group_t {

components/esp_driver_jpeg/jpeg_decode.c

@@ -382,7 +382,14 @@ esp_err_t jpeg_decoder_process(jpeg_decoder_handle_t decoder_engine, const jpeg_
     return ESP_OK;
 
 err1:
-    dma2d_force_end(decoder_engine->trans_desc, &need_yield);
+    // The transaction may still be pending in the pool queue (never picked up), so try to dequeue it first.
+    // Dequeuing is atomic against the pick that turns a pending transaction into an in-flight one, so if the
+    // transaction is no longer in the queue, it is already in-flight and we force end it instead.
+    if (dma2d_dequeue(decoder_engine->dma2d_group_handle, decoder_engine->trans_desc) != ESP_OK) {
+        if (dma2d_force_end(decoder_engine->trans_desc, &need_yield) != ESP_OK) {
+            ESP_LOGE(TAG, "failed to end the transaction, it is neither pending nor in-flight");
+        }
+    }
 err2:
     xSemaphoreGive(decoder_engine->codec_base->codec_mutex);
 #if CONFIG_PM_ENABLE

components/esp_driver_jpeg/jpeg_encode.c

@@ -346,7 +346,14 @@ esp_err_t jpeg_encoder_process(jpeg_encoder_handle_t encoder_engine, const jpeg_
     return ESP_OK;
 
 err1:
-    dma2d_force_end(encoder_engine->trans_desc, &need_yield);
+    // The transaction may still be pending in the pool queue (never picked up), so try to dequeue it first.
+    // Dequeuing is atomic against the pick that turns a pending transaction into an in-flight one, so if the
+    // transaction is no longer in the queue, it is already in-flight and we force end it instead.
+    if (dma2d_dequeue(encoder_engine->dma2d_group_handle, encoder_engine->trans_desc) != ESP_OK) {
+        if (dma2d_force_end(encoder_engine->trans_desc, &need_yield) != ESP_OK) {
+            ESP_LOGE(TAG, "failed to end the transaction, it is neither pending nor in-flight");
+        }
+    }
 err2:
     xSemaphoreGive(encoder_engine->codec_base->codec_mutex);
 #if CONFIG_PM_ENABLE

components/esp_hal_dma/esp32p4/include/hal/dma2d_ll.h

@@ -1054,7 +1054,7 @@ static inline void dma2d_ll_tx_configure_color_space_conv(dma2d_dev_t *dev, uint
         input_sel = 7;
         break;
     case DMA2D_CSC_TX_SCRAMBLE:
-        input_sel = 3; // Other 3-byte/pixel input path
+        input_sel = 3; // only 3 bytes/pixel format is supported for scrambling
         proc_en = false;
         output_sel = 2;
         break;

components/esp_hal_dma/esp32s31/include/hal/dma2d_ll.h

@@ -1031,7 +1031,7 @@ static inline void dma2d_ll_tx_configure_color_space_conv(dma2d_dev_t *dev, uint
         input_sel = 7;
         break;
     case DMA2D_CSC_TX_SCRAMBLE:
-        input_sel = 3; // Other 3-byte/pixel input path
+        input_sel = 3; // only 3 bytes/pixel format is supported for scrambling
         proc_en = false;
         output_sel = 2;
         break;

components/esp_hal_dma/include/hal/dma2d_types.h

@@ -133,6 +133,7 @@ typedef enum {
  * @brief Enumeration of 2D-DMA pixel bytes scamble order in color space conversion
  *
  * Assuming the original pixel byte order (from MSB to LSB) is 2-1-0 .
+ * This scramble only fits 3 bytes/pixel format. For non-3 bytes/pixel format, the pixels will be messed up if scrambling is used.
  */
 typedef enum {
     DMA2D_SCRAMBLE_ORDER_NONE,          /*!< 2D-DMA pixel data not scrambled */