Add LCD_CAM I8080 Driver

[Dominaezzz]

This is a driver for the I8080 mode of the LCD part of the LCD_CAM peripheral on the ESP32S3 (and eventually ESP32P4).

This was tested on a WT32-SC01 Plus.

TODO:

• Write documentation.
• Expose sensible configuration for users (and figure out the async FIFO threshold register).
• Figure out a sensible way to expose the dummy phase of the peripheral.
• Calculate clock divider from user provided frequency.
• Sort out peripheral split and GDMA channel split.
• Add support for both 8 bit and 16 bit buses.
  - [ ] Add support for continuous DMA mode. Will do in a separate PR. The peripheral doesn't wait for FIFO.
• Make example more interesting than just a red line.
• Consider supporting display-interface.
  - [ ] Consider using mipidsi for the example. Will do in a separate PR. I need to make a PR to mipidsi first for this display.

Must

• The code compiles without errors or warnings.
• All examples work.
• cargo fmt was run.
• Your changes were added to the CHANGELOG.md in the proper section.
• You updated existing examples or added examples (if applicable).
• Added examples are checked in CI

Nice to have

• You add a description of your work to this PR.
• You added proper docs for your newly added features and code.

[yanshay]

Make example more interesting than just a red line.

I think a good example would be to flip color of full screen and log average fps. I reached 28 fps in such a test which is a huge improvement over what I could achieve without it which was around 10 fps.

[yanshay]

I integrated this into my app (did it a bit hacky at this time) and it's working great! From around 8 fps I'm now on 32 fps which includes app rendering. So many thanks for this!

Here are a few comments:

1. For full optimization I wanted to do double buffering. For that I split the send to start_send and wait_for_send_end. So my code (1) renders (2) wait for previous send to end and then (3) starts send again which returns immeIately and I can immeIately render the next part while the data is sent by the dma.
2. My buffer is generated with little endian, so I needed to change the lcd_8bits_order bit. I added this as an argument to the fn new(...).
3. (2) raises two questions: (a) which of the lcd_cam interface parameters need to expose to the user, many are relevant, maybe all and (b) what's the approach to expose those as there are many. There's probably a common approach in esp-hal but I haven't used yet any peripheral that requires many params (or maybe I did and didn't notice which is good)
4. The send seems to me to be limited to 8192 bytes. I'm not that familiar with programming the DMA, I only know every descriptor can handle up to 4096 bytes, so strange that it does utilize two descriptors but not more than that. This is required for a scenario of large buffers dma, even full screen. That would also require psram, and there I think psram dma is different than dram dma, but not sure.

I can contribute my changes mentioned above, just don't think it's possible for me to add to the PR? Anyway, both changes I've made are really simple (but important).

Looking to see this quickly merged.
A first quick step can be the changes to shared files outside the lcd_cam folder that are required to get this working and then it's possible to use it as a maual patch locally until it's fully finalized.

[Dominaezzz]

1. Yes I'm planning to do something like this before I merge the PR. Most of the other peripherals already support this so I was gonna follow suit.

2. Yeah I'm not sure yet. I want to expose them all but idk how. This is where I would really like feedback/advice from the maintainers as to how to proceed.

3. Yeah the length register is 13 bits wide. The LCD_CAM can handle unlimited data but I didn't add support for it in the first commit. I need to first confirm how the DMA linked list is setup before I add support. On the ESP32-S3 you can DMA from PSRAM.

A first quick step can be the changes to shared files outside the lcd_cam folder that are required to get this working and then it's possible to use it as a maual patch locally until it's fully finalized.

I was actually considering this today. Though a lot of the stuff is private anyway so I don't think it would make a difference to users really.

[Dominaezzz]

This is what the config looks like in my esp-idf project.

pub struct I8080Config {
    pub clock_config: ClockConfig,
    pub format: Format,

    /// Setup cycles expected = this value + 1. (6 bits)
    pub setup_cycles: usize,
    /// Hold cycles expected = this value + 1. (13 bits)
    pub hold_cycles: usize,

    /// The output LCD_CD is delayed by module clock LCD_CLK.
    pub cd_mode: DelayMode,
    /// The output data bits are delayed by module clock LCD_CLK.
    pub output_bit_mode: DelayMode,
}

pub struct ClockConfig {
    /// Select LCD module source clock.
    ///   0: clock source is disabled.
    ///   1: XTAL_CLK.
    ///   2: PLL_D2_CLK.
    ///   3: PLL_F160M_CLK.
    pub clk_sel: usize,

    /// Integral LCD clock divider value. (8 bits)
    pub clkm_div_num: usize,
    /// Fractional clock divider numerator value. (6 bits)
    pub clkm_div_b: usize,
    /// Fractional clock divider denominator value. (6 bits)
    pub clkm_div_a: usize,

    /// LCD_PCLK = LCD_CLK / this (6 bits)
    pub pclk_divider: Option<usize>,
}

#[derive(Default)]
pub struct Format {
    pub invert_every_two_bytes: bool,
    pub invert_bit_order: bool,
    pub invert_byte_order: bool,
    pub enable_2byte_mode: bool,
}

#[repr(u8)]
#[derive(Default, Copy, Clone, Eq, PartialEq)]
pub enum DelayMode {
    /// Output without delay.
    #[default]
    None = 0,
    /// Delayed by the rising edge of LCD_CLK.
    RaisingEdge = 1,
    /// Delayed by the falling edge of LCD_CLK.
    FallingEdge = 2,
}

[yanshay]

BTW: Doesn't esp-idf already have lcd_cam support? I thought it was just a wrapper around the C/C++ version of esp-idf and there is already support for lcd_cam over there.

Comments on the config:

• For clk_sel can use an enum to make it clearer and less error prone.
• It could also be helpful to have a function that automatically sets the proper settings for bus speed based on just the display spec w/o having to calculate the dividers, etc. This is something that can also be added later on.
• What is invert_every_two_bytes vs. invert_byte_order? If one is for 16bits and other for 8bits bus (as it is in the register) then can combine into a single config and apply correctly based on the eneble_2byte_mode
• RaisingEdge -> RisingEdge (as in the comment above it)
• Is the enable2byte_mode required? Isn't it driven by the bus size which is known from the gpio pins supplied when the driver is created? Or can 8 bits bus could use 8bits / 16bits mode?

The config looks good to me but I don't know what the conventions for esp-hal API's.
It may also be worth considering the option of using naming as in esp-idf configuration so it would be easier to port C/C++ code. That API is documented at https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/lcd.html under I8080 interface. I looks like it's structured similar with different naming.

[Dominaezzz]

Yes esp-idf already has LCD_CAM support but I wrote my own driver in anticipation of going no_std.

Thanks for the comments, I'll apply them where sensible. I won't be using the config struct as is, it's just a reference.

Is the enable2byte_mode required? Isn't it driven by the bus size which is known from the gpio pins supplied when the driver is created? Or can 8 bits bus could use 8bits / 16bits mode?

16 bit buses may want to send 8 bit data sometimes. Practically every command that isn't about drawing is 8 bits wide.

[Dominaezzz]

@yanshay You should be able to use this without having to modify it now. Both byte order config and background transfers are available.

Let me know if it works for you.

[yanshay]

I managed to get this working!

It wasn't as trivial though as it was before, took me some time to figure out how to get this work and why it works that way. Probably worth explaining the idea in comments.

I understand that you introduced the Transfer for the non waiting send, for safety, so the i8080 won't be used while transfer is in progress, so the Transfer takes ownership over the I8080 and the buffer so they won't be accessed/modified while in transfer, right?
This caused some extra work to get this working but I understand the rational.

Several things I have questions about:

1. I had to make my buffer mut, why is that needed if data is only being sent?
2. The Transfer taking ownership over the buffer, this is to prevent it being modified while being sent, right? But in GUI with limited memory it may be reasonable to allow writing while reading (against rust concepts) even if it messes the displayed screen data, sometimes it is unavoidable. So I would't rush to impose this.
3. My buffer is of type defined by the UI framework I'm using (some Rgb565Pixel), so I couldn't use it directly (compiler comlained it didn't implement ReadTarget, and because it's from another lib Rust doesn't allow me to implement it), so I had to force cast it to array of u8.

let buffer_as_u8_ptr: *mut u8 = buffer as *mut _ as *mut u8;
let slice_buffer: &mut [u8] = unsafe {slice::from_raw_parts_mut(buffer_as_u8_ptr, buffer.len()*2) };

Is that the best approach?
Also, in such case the buffer is not really owned by Transfer since it's an additional generated unsafe reference. Is there a nicer way to do it?
Ans in general, seems to me that because in many cases, when working with UI frameworks, the buffer will be custom, then this buffer ownership won't be in effect unless they are prepared for thid ReadBuffer, add to that (2) above, maybe no need to own the buffer and return, just expect a reference to a [u8] instead.

[yanshay]

Another point after some more testing - this version is a bit slower than previous one, when I switch to this version the rendering+display takes about 5% more time.

In total I test on 6720 short dma transfers and the added time is about 20-25 microsec. Because of the way my application renders and it is in parallel to the display it could be that only in some cases the slowdown is affecting time measurement, so could be the difference of the pure display part is more.

Causes I can think of are:

1. Maybe more commands being sent per transfer than previously
2. Something to do maybe with changes in clock settings calculations?
3. Maybe Rust overhead for the new model.

BTW: Do you know what's the max freq that can be used with WT32-SC01-Plus?
You set it to 20Mhz, I tried 40Mhz and it improved a bit, tried also 80Mhz which didn't change anything, but does it support such frequency? Should it have worked at 80Mhz?

[Dominaezzz]

I understand that you introduced the Transfer for the non waiting send, for safety, so the i8080 won't be used while transfer is in progress, so the Transfer takes ownership over the I8080 and the buffer so they won't be accessed/modified while in transfer, right?

It that but more importantly it's a defense against leakapocalypse. We need to make sure the user doesn't free the buffer whilst the DMA transfer is happening.

I had to make my buffer mut, why is that needed if data is only being sent?

The buffer doesn't need to be mut, it's just convenient for the caller to do so when ownership is being passed around.

The Transfer taking ownership over the buffer, this is to prevent it being modified while being sent, right? But in GUI with limited memory it may be reasonable to allow writing while reading (against rust concepts) even if it messes the displayed screen data, sometimes it is unavoidable. So I would't rush to impose this.

This is something I want to allow, and there's precedent for it in esp-hal. You'll see it called "circlular" DMA, which let's you stream data to the peripheral. I've got a TODO in the PR description.

Is that the best approach?

I'm not sure to be honest, need some proper thought. I don't think the embedded Rust community has an answer for this yet.

Maybe Rust overhead for the new model.

I could buy this as a reason. Are you calling is_done() at all or just using wait()?

Do you know what's the max freq that can be used with WT32-SC01-Plus?

http://www.lcdwiki.com/res/MRB3951/ST7796S-Sitronix.pdf

Looks like 15kHz. (66ns per write)

[yanshay]

It that but more importantly it's a defense against leakapocalypse. We need to make sure the user doesn't free the buffer whilst the DMA transfer is happening.

That makes sense, didn't think of it, even though with display applications it sounds like more common for fixed buffer. Still most likely in Rust not u8 but rather some other type (like Rgb565), and potentially render only a slice of a buffer, so caller will need workaround this passing ownership. At least I couldn't find a way not to, but I still have what to learn in Rust.

I could buy this as a reason. Are you calling is_done() at all or just using wait()?

Only calling wait()

Looks like 15kHz. (66ns per write)

66ns -> 15 Mhz.
So how does it work when I specify 20Mhz, 40Mhz and 80Mhz?
Shouldn't it fail at such higher freq?
80Mhz would mean 260 fps, doesn't sound like such a display can handle something like that.
Or maybe the clock calculations aren't correct?
I'll check the pure display speed - maybe it does get there.

[Dominaezzz]

So how does it work when I specify 20Mhz, 40Mhz and 80Mhz? Shouldn't it fail at such higher freq?

I think 15 MHz is just the tested frequency, higher may work but it's out of spec.

[Dominaezzz]

@bjoernQ mind if I could get a review? (You seem to be most up to date on the problems I'm trying to solve here)

I'm particularly interested in what you think about how I handled the peripheral split and the split of the GDMA channel.

I'm also not sure how to support 8 bit vs 16 bit buses. 8 bit buses can send one byte at a time but 16 bit buses can either send one byte or two bytes at a time. Are there any other driver that solve this same problem?

Last and least important, any thoughts on how to expose the config to users?

[bjoernQ]

Thanks for working on this!

The code looks quite sane to me. I think the PARL_IO peripheral on C6/H2 is closest to LCD_CAM. The peripheral split is following the same basic idea but PARL_IO currently takes the full DMA channel - but the approach here looks good to me.

In PARL_IO the configuration of the data pins is even a bit more flexible - there we have ways to configure 1,2,4,8 and (only for C6 and only if RX is not used) 16 data lines. (in the TX example this is used here:

esp-hal/esp32c6-hal/examples/parl_io_tx.rs

Line 47 in 8ac075a

let tx_pins = TxFourBits::new(io.pins.gpio1, io.pins.gpio2, io.pins.gpio3, io.pins.gpio4);

)

Probably something similar to that could be used here.

From a brief look at the TRM there are basically two configuration "options": RGB and i8080? Since with the current approach a user would already decide on the mode by using the constructor of e.g. i8080 the user might just pass a config-struct into the constructor. Probably the user would need to set all of them always so we probably won't really need to use a builder for that. We could provide common defaults (if they exist) as Default so the user could use those directly or at least could use struct-update syntax to avoid mentioning fields which are default

[Dominaezzz]

This isn't 100% done (I want to support larger transactions) but I'm ready for a proper review now.

[bjoernQ]

One thing I unfortunately forgot about before:
We limit which channel can be used for which peripheral (e.g. for GDMA here:

esp-hal/esp-hal-common/src/dma/gdma.rs

Lines 621 to 635 in f542eb4

	pub struct [<SuitablePeripheral $num>] {}
	impl PeripheralMarker for [<SuitablePeripheral $num>] {}
	// with GDMA every channel can be used for any peripheral
	impl SpiPeripheral for [<SuitablePeripheral $num>] {}
	impl Spi2Peripheral for [<SuitablePeripheral $num>] {}
	#[cfg(esp32s3)]
	impl Spi3Peripheral for [<SuitablePeripheral $num>] {}
	impl I2sPeripheral for [<SuitablePeripheral $num>] {}
	impl I2s0Peripheral for [<SuitablePeripheral $num>] {}
	impl I2s1Peripheral for [<SuitablePeripheral $num>] {}
	#[cfg(parl_io)]
	impl ParlIoPeripheral for [<SuitablePeripheral $num>] {}
	#[cfg(aes)]
	impl AesPeripheral for [<SuitablePeripheral $num>] {}

)

While on the S3 every channel can be used for every peripheral that is not the case for ESP32 and S2 - so in this case we wouldn't really need it currently but we should add the LcdCamPeripheral in dma and also require the trait bounds

Another thing we want is module level documentation like we have for other peripherals (but you already mentioned you are not 100% done)

There are a few TODOs left and some commented-out code like // w.lcd_afifo_reset().set_bit();

Usually register modifications look more like this in other drivers

lcd_cam.lcd_user().modify(|_, w| {
            w.lcd_8bits_order().bit(false).
            lcd_bit_order().bit(false).
            lcd_byte_order().bit(false).
            lcd_2byte_en().bit(is_2byte_mode).

            // Be able to send data out in LCD sequence when LCD starts.
            lcd_dout().set_bit().
            // Data length in fixed mode. (13 bits)
            lcd_dout_cyclelen().variant(0).
            // Disable continuous output.
            lcd_always_out_en().clear_bit()
        });

Overall, this looks quite good to me already. Maybe @jessebraham or @MabezDev have additonal comments?

One last thing: I am unfortunately not able to really test this since I don't have a compatible display but I think @yanshay already tested this successfully. Would be good to have it tested when the code is final

[yanshay]

One last thing: I am unfortunately not able to really test this since I don't have a compatible display but I think @yanshay already tested this successfully. Would be good to have it tested when the code is final

Sure, once it's final let me know and I'll test what I can.

Just note that some (not all) of what I can test requires an application I'm working on, and I compile it by cherry picking lcd_cam over 0.14.1 since I can't get main to compile with my app due to various dependency issues conflicts with other libs (embassy related mostly).

[Dominaezzz]

Changed it to use INT_RAW now, mind testing again?

[Dominaezzz]

I think I've address all your comments now bjoernQ.

[bjoernQ]

seems this comment was forgotten to adjust

[bjoernQ]

I only have that one last comment regarding the example comment - otherwise looks great. Once we have the comment adjusted, I'm fine with this

Maybe @jessebraham or @MabezDev have something to add? If not, we can get this in

Would be great if @yanshay could double check that the higher frequency work fine again

Thanks!

[bjoernQ]

LGTM - Thanks for all the effort!

Will merge it after lunch if no one else comes up with a good reason to not merge it 😄

[MabezDev]

Sorry, I had been meaning to do a review of this for a lil while. Took some time now, it all looks good to me! Very clean implementation overall. It would be nice after a mipidsi display driver is added to come back to this example and update it.

[yanshay]

I retested at high frequency and I still see some issues at high frequency. Much less but still there.
It is definitely much much better and all in all work even at 40Mhz but not perfect.
Above 10Mhz there are glitches from time to time. Sometimes it's a single corrupted line, sometimes a larger portion of the screen.

I played with the code a bit and found that if I only enable interrupts in the start_send it fixes everything (so didn't need to modify the wait nor the tear_down_send which also dealt with the interrupts).
I also checked, and if I enable the bit once after first initialization of i8080 (in my application) then it's not fixing that, so seems it has to be done before every start_send (probably something else clears that bit though I couldn't find any code doing that).

Maybe a solution is to expose some option to use the standard interrupts, I assume that would be useful to have anyway in some applications. For example, for proper async support, to wake up when dma completes.

Thoughts?

[Dominaezzz]

The glitches you're seeing are likely due to screen tearing, which tends to happen with these I8080 displays that have a frame buffer. Coordinating the drawing with the TE (tear enable) pin helps but I've never tried it.

[yanshay]

The glitches you're seeing are likely due to screen tearing, which tends to happen with these I8080 displays that have a frame buffer. Coordinating the drawing with the TE (tear enable) pin helps but I've never tried it.

I'm not sure I understand. What is that 'TE' pin you refer to?

With the interrupt enabled it works perfectly, without there are glitches. It's very consistent behavior.

Also note that I'm not rendering and sending full screen to the display but rather render and send line by line. So I don't think what I'm familiar with as screen tearing the issue. (for example, sometimes I see wrong colors on correct layout, pretty strange). Also, when it's a single line it could stay there on the screen, meaning wrong data reached the frame buffer.

[Dominaezzz]

With the interrupt enabled it works perfectly, without there are glitches. It's very consistent behavior.

I'm curious what's going on with the hardware at this point, that's very strange.

[yanshay]

With the interrupt enabled it works perfectly, without there are glitches. It's very consistent behavior.

I'm curious what's going on with the hardware at this point, that's very strange.

@bjoernQ any ideas what's causing this behavior?

[bjoernQ]

With the interrupt enabled it works perfectly, without there are glitches. It's very consistent behavior.

I'm curious what's going on with the hardware at this point, that's very strange.

@bjoernQ any ideas what's causing this behavior?

to be honest it really doesn't make any sense to me. INT_ST is INT_ENA & INT_RAW.
Also, since the peripheral interrupt isn't mapped to any CPU interrupt it shouldn't make any difference IMHO

[yanshay]

@Dominaezzz I tried to reproduce the issue with a simple program so I started with your example and found something really strange.

Normaly your example fill the display blue/red. When I build it in release (so cargo run --example lcd_i8080 --release) it changes to black/white instead. And buffers handed to the DMA don't change.

Can you check if the same happens with your device?

The only thing I could think of to cause this is too fast commands to the display, so I added delays in the example between the initialization send commands, and reduced the freqyency to 5Mjz, but it didn't help.

[Dominaezzz]

Yeah it's black and white for me as well. (sigh)

I wonder what could be going wrong here.

[yanshay]

I think it's something to do with the initialization phase, because in my app this doesn't happen. I'm initializing the display before switching to i8080 and from that perspective it works fine.

The example code initializes the display also with the i8080 driver.
It seems the the data is interpreted differently by the display. It switches to some mode where it refers only to the 3 lowest bits (out of the 16) as rgb. All other bits don't seem to have any affect in my tests.

But even when I added delays, also inside the driver code where command data is being sent, it didn't change anything.

Are there limitations maybe on how fast internal esp32 registers can be written to? Release is so much faster than non release, paractically graphics application seem to be unusable in non release mode.

[Dominaezzz]

I've figured it out. Worst part is, I was aware of the hardware issue before I made the PR but forgot after I pushed the POC.

The LCD_CAM peripheral is impatient. It sends out data even when it's FIFO is empty, which means it ends up sending out garbage when the FIFO is empty.

Due to this fact, even though I start the DMA transfer before starting the peripheral like so....

self.start_write_bytes_dma(ptr as _, len * size_of::<P::Word>())?;
self.start_send();

...the DMA doesn't have enough time to fill up the peripheral's FIFO before it wants to send data, which means it sends out a little bit of garbage before sending out the actual data. The overhead of debug mode was sufficient to provide this delay but release mode means there's not enough of a delay.

This would explain why the initialization is broken and why you occasionally get some random white lines when you draw.

[Dominaezzz]

Or maybe not. It's now hit or miss with a delay between the DMA start and the LCD_CAM start.

[yanshay]

Interesting, do you know how they handle that in the non-rust esp-idf? Maybe they have the delay in their code there?

Is there a way of checking on the FIFO when it's full enough and not relying on a delay?

[Dominaezzz]

esp-idf uses esp_rom_delay_us(1).

I'm not aware of a way to check the FIFO, maybe the maintainers have a clue.

[MabezDev]

There doesn't seem to be a way to inspect the FIFO, at least there is nothing I can see in the documentation we have. I think we should just go ahead and use esp_rom_delay_us(1). It might be called something different likeesp_delay_us(1) but we have it and already use it in some places in esp-hal.

[yanshay]

Is it ets_delay_us(1)?

Anyway, I tried adding a delay using this function and in some other way between start_write_bytes and start_send (two places in the code) including very long delays and it didn't fix the issue.