Fix dma and mmio and gracefull shutdown

* DMA - Fix a GPU memory leak, add memory barriers
 and shutdown gracefully
 * SPI - Add option to start on slow clock and then move to fast clock
 and add memory barriers
* ili9341 - Add slow initialization and shutdown gracefully

gb/src/rpi_gpio/dma.rs

@@ -96,7 +96,7 @@ impl GpuMemory{
     const ALLOCATE_MEMORY_TAG:u32 = 0x3000C;
     const LOCK_MEMORY_TAG:u32 = 0x3000D;
     const UNLOCK_MEMORY_TAG:u32 = 0x3000E;
-    const RELEASE_MEMORY_TAG:u32 = 0x3000E;
+    const RELEASE_MEMORY_TAG:u32 = 0x3000F;
     const PAGE_SIZE:u32 = 4096;
 
     // This function converts the from the bus address of the SDRAM uncached memory to the arm physical address
@@ -107,8 +107,16 @@ impl GpuMemory{
     fn allocate(mbox:&Mailbox, size:u32, mem_fd:c_int)->GpuMemory{
         let flags = (Self::MEM_ALLOC_FLAG_COHERENT | Self::MEM_ALLOC_FLAG_DIRECT) as u32;
         let handle = mbox.send_command(Self::ALLOCATE_MEMORY_TAG, [size, Self::PAGE_SIZE, flags]);
-
+        // This is not documented well but after testing - on out of Gpu memory mailbox returns handle = 0
+        if handle == 0{
+            std::panic!("Error allocating Gpu memory! perhaps there is not enough free Gpu memory");
+        }
         let bus_address = mbox.send_command(Self::LOCK_MEMORY_TAG, [handle]);
+        // This is not documented well but after testing - on invalid handle mailbox returns bus_address = 0
+        if bus_address == 0{
+            std::panic!("Error locking Gpu memory!");
+        }
+
         let virtual_address = unsafe{libc::mmap(
             std::ptr::null_mut(),
             size as libc::size_t,
@@ -123,18 +131,15 @@ impl GpuMemory{
 
     fn release(&self, mbox:&Mailbox){
         unsafe{
-            let result = libc::munmap(self.virtual_address_ptr as *mut c_void, self.size as libc::size_t);
-            if result != 0 {
+            if libc::munmap(self.virtual_address_ptr as *mut c_void, self.size as libc::size_t) != 0 {
                 libc_abort("Error while trying to un map gpu memory");
             }
         }
-        let status = mbox.send_command(Self::UNLOCK_MEMORY_TAG, [self.mailbox_memory_handle]);
-        if status != 0{
+        if mbox.send_command(Self::UNLOCK_MEMORY_TAG, [self.mailbox_memory_handle]) != 0{
             std::panic!("Error while trying to unlock gpu memory using mailbox");
         }
-        let status = mbox.send_command(Self::RELEASE_MEMORY_TAG, [self.mailbox_memory_handle]);
-        if status != 0{
-            std::panic!("Error while to release gpu memory using mailbox");
+        if mbox.send_command(Self::RELEASE_MEMORY_TAG, [self.mailbox_memory_handle]) != 0{
+            std::panic!("Error while trying to release gpu memory using mailbox");
         }
     }
 }
@@ -278,6 +283,8 @@ impl DmaSpiTransferer{
 
         unsafe{dma_controller.init_dma_control_blocks()};
 
+        log::info!("Initialized dma contorller");
+
         return dma_controller;
     }
 
@@ -348,6 +355,10 @@ impl DmaSpiTransferer{
 
     pub fn start_dma_transfer(&mut self, data:&[u8; SPI_BUFFER_SIZE], transfer_active_flag:u8){        
         unsafe{
+            if (*self.tx_dma).read_cs() & 0x100 != 0{
+                log::error!("Error in the tx dma");
+            }
+
             let data_len = Self::DMA_SPI_CHUNK_SIZE - Self::DMA_SPI_HEADER_SIZE as usize;  // Removing the first 4 bytes from this length param
             let header = [transfer_active_flag, 0, (data_len & 0xFF) as u8,  /*making sure this is little endian order*/ (data_len >> 8) as u8];
 
@@ -365,9 +376,12 @@ impl DmaSpiTransferer{
             (*self.tx_dma).write_conblk_ad(self.tx_control_block_memory.bus_address);
             (*self.rx_dma).write_conblk_ad(self.rx_control_block_memory.bus_address);
 
+            memory_barrier();   // Sync all the memory operations happened in this function 
             // Starting the dma transfer
             (*self.tx_dma).write_cs(Self::DMA_CS_ACTIVE | Self::DMA_CS_END);
             (*self.rx_dma).write_cs(Self::DMA_CS_ACTIVE | Self::DMA_CS_END);
+            // Since the DMA controller writes to the SPI registers adding a barrier (even though it wrties afterwards to the DMA registers)
+            memory_barrier();   // Change DMA to SPI
         }
     }
 
@@ -400,11 +414,17 @@ impl DmaSpiTransferer{
 
 impl Drop for DmaSpiTransferer{
     fn drop(&mut self) {
+        // Finish current dma operation
+        self.end_dma_transfer();
+
         // reset the dma channels before releasing the memory
         unsafe{
             // reset the dma channels
             (*self.tx_dma).write_cs(Self::DMA_CS_RESET);
             (*self.rx_dma).write_cs(Self::DMA_CS_RESET);
+            // clear the permaps for the channels
+            (*self.tx_dma).control_block.write_ti(0);
+            (*self.rx_dma).control_block.write_ti(0);
             // disable the channels I used
             let mask = !((1 << Self::TX_CHANNEL_NUMBER) | (1 << Self::RX_CHANNEL_NUMBER));
             write_volatile(self.dma_enable_register_ptr, read_volatile(self.dma_enable_register_ptr) & mask);
@@ -415,5 +435,7 @@ impl Drop for DmaSpiTransferer{
         self.rx_control_block_memory.release(&self.mbox);
         self.source_buffer_memory.release(&self.mbox);
         self.tx_control_block_memory.release(&self.mbox);
+
+        log::info!("Successfuly release dma resources");
     }
 }

gb/src/rpi_gpio/ili9341_controller.rs

@@ -41,6 +41,7 @@ pub const SPI_BUFFER_SIZE:usize = TARGET_SCREEN_HEIGHT * TARGET_SCREEN_WIDTH * s
 
 pub trait SpiController {
     fn new(dc_pin_number:u8)->Self;
+    fn fast_mode(&mut self);
     fn write<const SIZE:usize>(&mut self, command:Ili9341Commands, data:&[u8;SIZE]);
     fn write_buffer(&mut self, command:Ili9341Commands, data:&[u8;SPI_BUFFER_SIZE]);
 }
@@ -101,7 +102,7 @@ impl<SC:SpiController> Ili9341Contoller<SC>{
         // Configuring the screen
         spi.write(Ili9341Commands::MemoryAccessControl, &[0x20]); // This command tlit the screen 90 degree
         spi.write(Ili9341Commands::PixelFormatSet, &[0x55]);     // set pixel format to 16 bit per pixel;
-        spi.write(Ili9341Commands::FrameRateControl, &[0x0, 0x1F /*According to the docs this is 61 hrz */]);
+        spi.write(Ili9341Commands::FrameRateControl, &[0x0, 0x10 /*According to the docs this is 119 hrz, setting this option in order to avoid screen tearing on rpi zero2 */]);
         spi.write(Ili9341Commands::DisplayFunctionControl, &[0x8, 0x82, 0x27]);
 
         // Gamma values - pretty sure its redundant
@@ -117,14 +118,21 @@ impl<SC:SpiController> Ili9341Contoller<SC>{
         /*---------------------------------------------------------------------------------------------------------------------- */
         //End of fbcp-ili9341 inpired implementation
 
+        log::info!("Finish configuring ili9341");
+
+        // turn backlight on
+        led_pin.set_high();
+
         // Clear screen
         spi.write(Ili9341Commands::ColumnAddressSet, &[0,0,((ILI9341_SCREEN_WIDTH -1) >> 8) as u8, ((ILI9341_SCREEN_WIDTH -1) & 0xFF) as u8]);
         spi.write(Ili9341Commands::PageAddressSet, &[0,0,((ILI9341_SCREEN_HEIGHT -1) >> 8) as u8, ((ILI9341_SCREEN_HEIGHT -1) & 0xFF) as u8]);
         // using write and not write buffer since this is not the correct size 
         spi.write(Ili9341Commands::MemoryWrite, &Self::CLEAN_BUFFER);
 
-        // turn backlight on
-        led_pin.set_high();
+        // need to sleep before changing the clock after transferring pixels to the lcd
+        Self::sleep_ms(120);
+
+        spi.fast_mode();
 
         log::info!("finish ili9341 device init");
 
@@ -159,6 +167,7 @@ impl<SC:SpiController> Ili9341Contoller<SC>{
 
 impl<SC:SpiController> Drop for Ili9341Contoller<SC>{
     fn drop(&mut self) {
+        self.spi.write(Ili9341Commands::DisplayOff, &[]);
         self.led_pin.set_low();
         self.reset_pin.set_high();
         Self::sleep_ms(1);
@@ -209,7 +218,7 @@ impl<SC:SpiController> GfxDevice for Ili9341GfxDevice<SC>{
         self.time_counter = self.time_counter.add(time.duration_since(self.last_time));
         self.last_time = time;
         if self.time_counter.as_millis() > 1000{
-            log::info!("FPS: {}", self.frames_counter);
+            log::debug!("FPS: {}", self.frames_counter);
             self.frames_counter = 0;
             self.time_counter = std::time::Duration::ZERO;
         }

gb/src/rpi_gpio/mmio_spi.rs

@@ -3,10 +3,11 @@ use rppal::gpio::{OutputPin, IoPin};
 
 use crate::rpi_gpio::dma::DmaSpiTransferer;
 
-use super::{ili9341_controller::{Ili9341Commands, SpiController, SPI_BUFFER_SIZE}, decl_write_volatile_field, decl_read_volatile_field};
+use super::{ili9341_controller::{Ili9341Commands, SpiController, SPI_BUFFER_SIZE}, decl_write_volatile_field, decl_read_volatile_field, memory_barrier};
 
 const BCM_SPI0_BASE_ADDRESS:usize = 0x20_4000;
-const SPI_CLOCK_DIVISOR:u32 = 4;    // the smaller the faster (on my system below 4 there are currptions)
+const FAST_SPI_CLOCK_DIVISOR:u32 = 4;   // the smaller the faster (on my ili9341 screen below 4 there are currptions)
+const INIT_SPI_CLOCK_DIVISOR:u32 = 34;  // slow clock for verifiying the initialization goes smooth with no corruptions   
 
 // The register are 4 bytes each so making sure the allignment and padding are correct
 #[repr(C, align(4))]
@@ -65,12 +66,14 @@ impl MmioSpi{
             // ChipSelect = 0, ClockPhase = 0, ClockPolarity = 0
             spi_cs0.set_low();
             Self::setup_poll_fast_transfer(&mut *spi_registers);
-            (*spi_registers).write_clk(SPI_CLOCK_DIVISOR);
+            (*spi_registers).write_clk(INIT_SPI_CLOCK_DIVISOR);
         }
 
-
+        memory_barrier();   // Change SPI to DMA
         let dma_transferer = DmaSpiTransferer::new(&bcm_host, Self::SPI_CS_DMAEN);
+        memory_barrier();   // Change DMA to SPI
 
+        log::info!("Finish initializing spi mmio interface");
         MmioSpi { 
             _bcm:bcm_host, spi_registers, dc_pin, _spi_pins: spi_pins, _spi_cs0: spi_cs0, last_transfer_was_dma: false,
             dma_transferer
@@ -88,7 +91,9 @@ impl MmioSpi{
 
     fn prepare_for_transfer(&mut self) {
         if self.last_transfer_was_dma{
+            memory_barrier();   // Change SPI to DMA
             self.dma_transferer.end_dma_transfer();
+            memory_barrier();   // Change DMA to SPI
             Self::setup_poll_fast_transfer(self.spi_registers);
         }
     }
@@ -139,7 +144,9 @@ impl MmioSpi{
     // Since generic_const_exprs is not stable yet Im reserving the first 4 bytes of the data variable for internal use
     fn write_dma_raw(&mut self, data:&[u8;SPI_BUFFER_SIZE]){
         unsafe{(*self.spi_registers).write_cs(Self::SPI_CS_DMAEN | Self::SPI_CS_CLEAR)};
+        memory_barrier();   // Change SPI to DMA
         self.dma_transferer.start_dma_transfer(data, Self::SPI_CS_TA as u8);
+        memory_barrier();   // Change DMA to SPI
     }
 }
 
@@ -161,4 +168,14 @@ impl SpiController for MmioSpi{
     fn write_buffer(&mut self, command:Ili9341Commands, data:&[u8;SPI_BUFFER_SIZE]) {
         self.write_dma(command, data);
     }
+
+    fn fast_mode(&mut self) {
+        unsafe{(*self.spi_registers).write_clk(FAST_SPI_CLOCK_DIVISOR)};
+    }
+}
+
+impl Drop for MmioSpi{
+    fn drop(&mut self) {
+        unsafe{(*self.spi_registers).write_cs(Self::SPI_CS_CLEAR)};
+    }
 }

gb/src/rpi_gpio/mod.rs

@@ -15,6 +15,12 @@ fn libc_abort(message:&str){
     std::io::Result::<&str>::Err(std::io::Error::last_os_error()).expect(message);
 }
 
+// According to the docs the raspberrypi requires memory barrier between reads and writes to differnet peripherals 
+#[inline] 
+pub(self) fn memory_barrier(){
+    std::sync::atomic::fence(std::sync::atomic::Ordering::SeqCst);
+}
+
 macro_rules! decl_write_volatile_field{
     ($function_name:ident, $field_name:ident) =>{
         #[inline] unsafe fn $function_name(&mut self,value:u32){

gb/src/rpi_gpio/rppal_spi.rs

@@ -44,4 +44,9 @@ impl SpiController for RppalSpi{
     fn write_buffer(&mut self, command:Ili9341Commands, data:&[u8;SPI_BUFFER_SIZE]) {
         self.write(command, data);
     }
+
+    fn fast_mode(&mut self) {
+        // No need to change the clock here, 
+        // since we have no access directly to the clock register we cant set it high enough for it to matter :()
+    }
 }