adafruit / Adafruit-GFX-Library

Adafruit GFX graphics core Arduino library, this is the 'core' class that all our other graphics libraries derive from
https://learn.adafruit.com/adafruit-gfx-graphics-library
Other
2.4k stars 1.55k forks source link

problem with DMA using SAMD51 #349

Open keystoneclimber opened 3 years ago

keystoneclimber commented 3 years ago

I'm using an Adafruit 1.54" 240x240 TFT and driving it with a Feather M4 Express using the following code...

#include <Arduino.h>
#include <Wire.h>
#include "SPI.h"
#include "Adafruit_GFX.h"
#include "Adafruit_ST7789.h"

#define TFT_CS      10
#define TFT_RST     9
#define TFT_DC      6

Adafruit_ST7789 tft = Adafruit_ST7789(&SPI, TFT_CS, TFT_DC, TFT_RST);

void setup(void) {
    Serial.begin(9600);
    while(!Serial);
    tft.init(240, 240);
    tft.fillScreen(ST77XX_BLACK);
    Serial.println("code never reaches this point with DMA enabled");
}

void loop() {
    while(1);
}

The code runs fine until I enable DMA in Adafruit_SPITFT. I've tracked the problem down to the Adafruit_SPITFT::writeColor() function. Specifically, the while (dma_busy) is in an endless loop because dma_callback() never fires.

I have verified that DMA works with the hardware listed when I disable it in the library and do it manually via this...

#include <Arduino.h>
#include <Wire.h>
#include "SPI.h"
#include "Adafruit_GFX.h"
#include "Adafruit_ST7789.h"
#include "Adafruit_ZeroDMA.h"

#define TFT_CS      10
#define TFT_RST     9
#define TFT_DC      6

static volatile bool dma_busy = false;
uint16_t dmaBuf[2][240];
uint8_t dmaIdx = 0;
DmacDescriptor *descriptor;
Adafruit_ST7789 tft = Adafruit_ST7789(TFT_CS, TFT_DC, TFT_RST);
SPISettings settings(12000000, MSBFIRST, SPI_MODE0);
Adafruit_ZeroDMA  dma;

static void dma_callback(Adafruit_ZeroDMA *dma) {
  dma_busy = false;
}

void initDMA() {
    volatile uint32_t *data_reg;
    dma.allocate();
    dma.setTrigger(SERCOM1_DMAC_ID_TX);
    data_reg = &SERCOM1->SPI.DATA.reg;
    dma.setAction(DMA_TRIGGER_ACTON_BEAT);
    descriptor = dma.addDescriptor(NULL, (void *)data_reg, sizeof dmaBuf[0], DMA_BEAT_SIZE_BYTE, true, false);
    dma.setCallback(dma_callback);
}

void fillScreenBlackUsingDma() {
    uint16_t *dmaPtr;
    SPI.beginTransaction(settings);
    digitalWrite(TFT_CS, LOW);
    tft.setAddrWindow(0, 0, 240, 240);
    digitalWrite(TFT_CS, LOW);
    digitalWrite(TFT_DC, HIGH);

    for (uint8_t y = 0; y < 240; y++) {
        dmaPtr = &dmaBuf[dmaIdx][0];
        for (uint8_t x = 0; x < 240; x++) {
            *dmaPtr++ = (uint16_t)0x0000;
        }
        while(dma_busy);
        descriptor->SRCADDR.reg = (uint32_t)&dmaBuf[dmaIdx] + sizeof dmaBuf[0];
        dma_busy = true;
        dma.startJob();
        dmaIdx = 1 - dmaIdx;
    }
    while(dma_busy);
    digitalWrite(TFT_CS, HIGH);
    SPI.endTransaction();
}

void setup() {
    Serial.begin(9600);
    while(!Serial);
    tft.init(240, 240);
    initDMA();
    fillScreenBlackUsingDma();
    Serial.println("code reaches this point just fine using hand-rolled DMA");
}

void loop() {
    while(1);
}

As such, it appears there is a problem with the library code relating to DMA on the SAMD51.

ladyada commented 3 years ago

please try previous versions of the library, see if there was a time it did work, then we could find the PR/version where it stopped!

keystoneclimber commented 3 years ago

I tried rolling back Adafruit_GFX to each of the following...

1.10.0 through 1.10.7 did not work 1.6.0 did not work 1.5.0 would not compile... Adafruit ST7735 and ST7789 Library\Adafruit_ST77xx.cpp:130:24: error: no matching function for call to Adafruit_ST77xx::initSPI()

I also tried rolling back... Adafruit Zero DMA Adafruit ST7735 and ST7789 Adafruit BusIO ...each to the closest (dated) version to Adafruit_GFX 1.6.0 (the oldest version I could get to compile) again with no luck getting it to work.

I also tried using the 2.4" 320x240 TFT FeatherWing display & ILI9341 library with the M4 Express with DMA enabled in Adafruit_GFX and it also did not work.

ladyada commented 3 years ago

could be maybe never worked, if you can fix it please submit a pr

RudolphRiedel commented 2 years ago

Just found this issue, I believe I have the same issue.

I have this example that builds for a whole bunch of Arduino targets: https://github.com/RudolphRiedel/FT800-FT813/tree/5.x/examples/EVE_Test_Arduino_PlatformIO

When I build it for the Metro M4 I only get one image on the display and then it hangs. Checking with the logic analyzer showed that everything is fine to the end of the DMA transfer, chip-select is not going high as it is supposed to. This means that my dma_callback() function is never called.

It works just fine though when I remove the dependancy for my library from platformio.ini and copy the files for my library over to the "src" folder. My theory was that my callback function falls victim to some linker issue.

I modified my EVE_start_dma_transfer() function in EVE_cpp_target.cpp: void EVE_start_dma_transfer(void) { myDMA.changeDescriptor(desc, (void ) (((uint32_t) &EVE_dma_buffer[0])+1), NULL, (EVE_dma_buffer_index4)-1); SERCOM2->SPI.CTRLB.bit.RXEN = 0; EVE_cs_set(); EVE_dma_busy = 42; myDMA.startJob();

DELAY_MS(1); //myDMA.abort(); SERCOM2->SPI.CTRLB.bit.RXEN = 1; EVE_dma_busy = 0; EVE_cs_clear(); }

And this should work since the DMA transfer only takes 450µs. grafik

But every DMA transfer after the initial one is empty: grafik

When I enable the myDMA.abort() line above the display is getting updated.

So the issue is not diretly that the callback function is never called, the issue seems to be that the IRQ handler to end the DMA transfer is not called at all.

RudolphRiedel commented 2 years ago

Damn, sorry, to make it work with myDMA.abort() I also had to comment out the myDMA.setCallback() line. So actually something is going on with the callback function as well and maybe the DMA not ending properly is a separate issue.

RudolphRiedel commented 2 years ago

I am looking at this with a debugger now and found that the program gets trapped in the Dummy_Handler().

keystoneclimber commented 2 years ago

I got around the issue entirely by hand rolling the DMA and subclassing the drawpixel() & display() functions...

class  frameBuffer : public Adafruit_GFX {
public:
    uint16_t *buffer;
    uint16_t *bufferMidpoint;
    uint32_t frameBytes;
    frameBuffer(int16_t w, int16_t h): Adafruit_GFX(w, h) {
        uint16_t frameWords = w * h;
        frameBytes = 2 * frameWords;
        buffer = (uint16_t*)malloc(frameBytes);
        memset(buffer, 0, frameWords);
        bufferMidpoint = buffer + (frameWords / 2);
    }

    void drawPixel( int16_t x, int16_t y, uint16_t color) {
        if (x > 239) return;
        if (x < 0) return;
        if (y > 239) return;
        if (y < 0) return;
        #if defined(USE_DMA)
            // Adafruit_GFX normally uses SPI.transfer16() to communicate with displays 
            // and it reverses the stored 16-bit (RGB565) color byte order before sending it to the display
            // DMA must do the same, otherwise the colors will be inverted
            // since all graphics routines ultimately call drawPixel, this should enable the entire library to use DMA display writes
            buffer[x + y * _width] = word(lowByte(color), highByte(color));
        #else
            buffer[x + y * _width] = color;
        #endif
    }

    void display() {
        SPI.beginTransaction(settings);     // SPI init
        digitalWrite(TFT_CS, LOW);          // Chip select
        tft.setAddrWindow(0, 0, 240, 240);  // Set address window to full screen
        digitalWrite(TFT_CS, LOW);          // Re-select after addr function
        digitalWrite(TFT_DC, HIGH);         // Data mode...
        #if defined(USE_DMA)
            dma_busy = true;          // Mark as busy (DMA callback clears this)
            dma.startJob();           // Start new DMA transfer
            while(dma_busy);          // Wait for last DMA transfer to complete
        #else
            for (uint16_t i = 0; i < 240 * 240; i++) {
                SPI.transfer16(buffer[i]);
            }
        #endif
        digitalWrite(TFT_CS, HIGH); // Deselect
        SPI.endTransaction();       // SPI done
    }

};

frameBuffer fb(DISPLAY_WIDTH, DISPLAY_HEIGHT);

static void dma_callback(Adafruit_ZeroDMA *dma) {
    (void) dma;
    if (callbackSourceIsDescriptor_1) {
        callbackSourceIsDescriptor_1 = false;
    } else {
        callbackSourceIsDescriptor_1 = true;
        dma_busy = false;
    }
}

void initDMA() {
    dma.allocate();
    dma.setTrigger(SERCOM1_DMAC_ID_TX);
    dma.setAction(DMA_TRIGGER_ACTON_BEAT);
    // The ZeroDMA library declares count as a uint32_t but it simply writes to the DMAC Descriptor BTCNT.reg (dmac.h) which is uint16_t
    // as such, a single DMA transfer is limited to a 65,536 bytes count
    // since a display frame contains 240 x 240 16bit color values (115,200 bytes) this needs to be broken up into 2 transfers...
    descriptor_1 = dma.addDescriptor(fb.buffer, (void *)(&SERCOM1->SPI.DATA.reg), fb.frameBytes / 2, DMA_BEAT_SIZE_BYTE, true, false);
    descriptor_1->BTCTRL.bit.BLOCKACT = DMA_BLOCK_ACTION_INT; // fire callback at end of descriptor_1 transfer
    descriptor_2 = dma.addDescriptor(fb.bufferMidpoint, (void *)(&SERCOM1->SPI.DATA.reg), fb.frameBytes / 2, DMA_BEAT_SIZE_BYTE, true, false);
    descriptor_2->BTCTRL.bit.BLOCKACT = DMA_BLOCK_ACTION_INT; // fire callback at end of descriptor_2 transfer
    dma.setCallback(dma_callback);
}
RudolphRiedel commented 2 years ago

Well now, I am usually doing my own DMA code but for the Arduino example of my library I wanted to use an implementation that also plays nice with other libraries doing other stuff. :-) So fixing this for me was not why I looked into this issue.

homeodor commented 1 year ago

Hey everyone,

So I have plenty of DMA channels in use (10 I think), including ADC, SPI and others. I use Adafruit’s ZeroDMA as a wrapper, and the device tended to freeze all the time randomly, seemingly the same issue as described here. When I’ve investigated, I’ve found that it always gets stuck on dmaBusy check of the GFX library.

I’ve found that the TFT SPI code sets priority of the DMA to the highest (3). Upon changing this manually to 0 I had no problems whatsoever. Counterintuitively, the highest priority somehow makes the interrupts fail, or something. This also possibly explains why @keystoneclimber’s rewrite works.

I don’t know the reasons for this setting in the first place, so it might be worth to consider changing this in the library code for good. To me it kinda seems like setting the priority so high while dealing with such a data-heavy transfer is indeed asking for trouble, but I’m no expert and DMA is a very complex topic.

Hope this helps!

RudolphRiedel commented 1 year ago

I don't know why, but apparently no one is attending to the libraries for a while now. I get the attitude of it-is-open-source-fix-it-yourself. However putting in the work, finding out what the issue is, putting in a pull request only to see it ignored - not good.