drivers/lcd/st7789: update putarea() method

[Cynerd]

Summary

This is the follow-up to #6551. The change in that PR broke LCD drivers with putarea implemented. This updates the documentation to reflect changes there and also updates driver for ST7789.

The discussion on mailing-list: https://lists.apache.org/thread/4yk5z9qlb9qhslwpbyzgkt07ff578or9

Impact

This should give hint to other developers working on LCD` drivers that expected behavior changed and that semantic of passed argument buffer is now different.

Testing

Tested on custom SAME70N21 based board.

[lupyuen]

Hi @Cynerd this commit seems to cause my ST7789 display to render a screen with random noise. I'm using Pine64's PineDio Stack BL604 RISC-V Board with integrated ST7789 display.

The previous version of st7789_wrram (that doesn't write multiples of size while skipping some values) works OK for me:

https://github.com/lupyuen/incubator-nuttx/blob/f67a25341101b8a07dd957a078b3ca1f7f1e2bcb/drivers/lcd/st7789.c

I tested with this LVGL app: https://github.com/lupyuen/lvgltest-nuttx

Which is a simplified version of the NuttX LVGL Demo: https://github.com/apache/incubator-nuttx-apps/tree/master/examples/lvgldemo

Any idea how I can troubleshoot this? Thanks!

[pkarashchenko]

@lupyuen could you please attach some picture of a situation that you are describing, just to understand what "random noise" mean?

[lupyuen]

Here's the ST7789 display with the random noise. Note that the top row of text looks OK, the rest is garbled:

The correct display should be this:

[Cynerd]

Can you please try the fb example? It draws recognizable pattern and it is easier to see what might have gone wrong from it than from this.

I tested it and right now developing my application with LVGL with this patch and thus, this might be something platform or setting dependent.

[lupyuen]

Sorry I'm using LCD Device for ST7789, not Framebuffer. I'm calling board_lcd_initialize and board_lcd_getdev similar to this:

https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/esp32c3/esp32c3-devkit/src/esp32c3_st7789.c

How do I use Framebuffer with ST7789?

[Cynerd]

@lupyuen I think that you don't have to use framebuffer. You only have to change how you use putarea(). The issue I was fixing is that the expected behaviour of that function changed, or rather was changed to better fit frame buffer implementation. This is also why I updated documentation for the putarea function. If you use LCD directly then you are calling these functions and most likely you are calling putarea, not with a full buffer but only the selected part you want to update.

[lupyuen]

(Sorry if I misunderstood this) Does it mean that the lvgldemo example might need to be updated to call putarea correctly?

https://github.com/apache/incubator-nuttx-apps/blob/master/examples/lvgldemo/lcddev.c#L145-L159

static void lcddev_sync_flush(lv_disp_drv_t *disp_drv, const lv_area_t *area, lv_color_t *color_p) {
  struct lcddev_area_s lcd_area;
  lcd_area.row_start = area->y1;
  lcd_area.row_end = area->y2;
  lcd_area.col_start = area->x1;
  lcd_area.col_end = area->x2;
  lcd_area.data = (uint8_t *)color_p;
  ret = ioctl(state.fd, LCDDEVIO_PUTAREA,
              (unsigned long)((uintptr_t)&lcd_area));

Lemme trace the call to putarea and understand how it renders the screen.

As you have suggested, I'll pass a recognizable pattern to putarea and see how it renders the pattern. Thanks!

UPDATE: Here's what I found out: The lvgldemo example calls putarea to render 20 rows of pixels at a time:

https://gist.github.com/lupyuen/f9a5fb2a578be5c3060e87fa3fdd37e7

// Note that y2 - y1 is always 19
x1=0, y1=0,  x2=239, y2=19, buffer1=0x42018f08, color_p=0x42018f08
x1=0, y1=20, x2=239, y2=39, buffer1=0x42018f08, color_p=0x42018f08
x1=0, y1=40, x2=239, y2=59, buffer1=0x42018f08, color_p=0x42018f08

The buffer passed to putarea is always the same, it's the 20 row of pixels buffer1 that we have allocated for LVGL rendering.

Checking the implementation of putarea, we see that putarea expects to receive the entire Display Buffer of 240 rows (not just a chunk of 20 rows):

https://github.com/apache/incubator-nuttx/blob/master/drivers/lcd/st7789.c#L560-L576

static int st7789_putarea(FAR struct lcd_dev_s *dev,
                          fb_coord_t row_start, fb_coord_t row_end,
                          fb_coord_t col_start, fb_coord_t col_end,
                          FAR const uint8_t *buffer) {
  ...
  // We send a partial chunk of `buffer` for rendering
  st7789_wrram(priv, src + (ST7789_XRES * row_start) + col_start,
               bsiz, ST7789_XRES - bsiz, row_end - row_start + 1);

  // Previously we sent the entire `buffer` for rendering
  // st7789_wrram(priv, src,
  //             (row_end - row_start + 1) * (col_end - col_start + 1));

Maybe I missed something: If putarea expects to receive the entire Display Buffer, what if we can't fit the entire Display Buffer into RAM?

Is there another function that we could call to render the screen in small buffers, like with LVGL? Thanks!

[Cynerd]

I see. The lvgldemo's branch that uses LCD device has to be updated. I am using a frame buffer that is why I missed that.

You probably want to switch from putarea to putrun then. It expects only one line of pixes, and thus multiple calls might be required, but it is the solution.

The second option would be to add a new function that would draw just the specified rectangle (the original putarea behaviour). The revert was declined on the mailing list, and thus expanding API might be the better option here to satisfy both use cases.

[lupyuen]

Thanks I'll change the lvgldemo example from putarea to putrun and render one row of pixels at a time.

It's a great pity that we can no longer blast out multiple rows of pixels in a single SPI DMA operation to ST7789. Screen redraws will now be slower.

Or maybe I should negative-offset the pointer to putarea, so that it pretends to be the entire Display Buffer? It's an awful hack but it will perform better with DMA and render faster. 🤔

[xiaoxiang781216]

If we want to update a partial region, the memory layout for framebuffer and lcd is different. To speed up both case, we need extend the lcd_ops(add new callback or new argument to put_area) to indicate which case is requested. Maybe the better approach is kept put_area as the old behavior, but add a new callback for framebuffer.

[lupyuen]

Thanks Xiao Xiang, yes I prefer the previous implementation of putarea

The current implementation of putarea might have a performance concern. Right now we execute one SPI Transfer per row of pixels:

https://github.com/apache/incubator-nuttx/blob/3693d387637e70a42b1ddbfda86c78ea73595d93/drivers/lcd/st7789.c#L450-L463

Previously we blasted the entire region of pixels in a single SPI Transfer (which performs really well with DMA):

https://github.com/apache/incubator-nuttx/blob/f3dbc7bc63073f6b2f45296063c9759e279a5b16/drivers/lcd/st7789.c#L448-L454

To refresh a 240x240 display, the current implementation requires 240 SPI Transfers (one per row).

The previous implementation requires only 12 SPI Transfers. (Assuming LVGL buffers 20 rows, as mentioned earlier)

We could combine the SPI Transfers in st7789_wrram, but we'll need additional checks: skip must be 0, etc

[Cynerd]

The current implementation of putarea might have a performance concern.

The same can be said with the previous implementation and frame buffer usage. There are just two approaches needed and thus one function can't satisfy it.

add new callback or new argument to put_area

I would not complicate the putarea call and instead, add something like putrect.

I am also not sure if reverting to the previous putarea behaviour is the best idea. My investigation showed that LCD drivers (except of one) were already updated. The revert of all of those changes would have to be performed. I also like the idea that putarea updates a specified area from the whole buffer, while putrect would update provided rectangle. On the other hand, revert would ensure consistency with the previous behaviour.

[lupyuen]

Yep this is a tough decision. In case anybody is curious how ST7789 is implemented in Zephyr: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/display/display_st7789v.c

UPDATE: I see that Zephyr's ST7789 Driver combines multiple SPI Transfers into one. Perhaps we could do the same for our current implementation of st7789_wrram? This would solve our performance concern.

https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/display/display_st7789v.c#L189-L202

        // If the region we are rendering is not the entire Display Width (I think?)
	if (desc->pitch > desc->width) {
                // We will execute one SPI Transfer per row
		write_h = 1U;    // Send one row of pixels per SPI Transfer
		nbr_of_writes = desc->height;    // One SPI Transfer per row
	} else {
                // If the region we are rendering is the entire Display Width,
                // we will execute one single SPI Transfer
		write_h = desc->height;    // Send all rows of pixels in the SPI Transfer
		nbr_of_writes = 1U;    // One SPI Transfer in total
	}

        // Execute the SPI Transfers: 
        // Either one SPI Transfer per row, or one SPI Transfer for the entire region
	for (uint16_t write_cnt = 0U; write_cnt < nbr_of_writes; ++write_cnt) {
		st7789v_transmit(dev, write_cnt == 0U ? ST7789V_CMD_RAMWR : ST7789V_CMD_NONE,
				(void *) write_data_start,
				desc->width * ST7789V_PIXEL_SIZE * write_h);
		write_data_start += (desc->pitch * ST7789V_PIXEL_SIZE);
	}

Meanwhile I have converted my lvgldemo to call putrun instead of putarea, and it works OK:

lupyuen/lvgltest-nuttx@95c76d4

[xiaoxiang781216]

Yes, we need some thing like pitch or a new callback to indicate the different usage. @Cynerd @lupyuen could you provide the patch for this?

[tmedicci]

Hi there!

I've been facing this same problem: the ST7789 + lvgldemo based on lcddev broke the code. However, the lcddev is the fallback for the lvgldemo: simply disabling it (CONFIG_LCD_DEV not set) and enabling the framebuffer (CONFIG_LCD_FRAMEBUFFER=y) "fix" the problem of the example code. More work on the lcd_dev driver may be needed.

[lupyuen]

Hi @tmedicci this patch for lvgldemo/lcddev.c works OK for my ST7789:

lupyuen/lvgltest-nuttx@95c76d4

static void lcddev_sync_flush(lv_disp_drv_t *disp_drv, const lv_area_t *area,
                              lv_color_t *color_p)
{
  // TODO: Change LCDDEVIO_PUTRUN back to LCDDEVIO_PUTAREA so that it performs better with SPI DMA
  // https://github.com/apache/incubator-nuttx/pull/6657#issuecomment-1200094716

  lv_color_t *data = color_p;

  for (lv_coord_t row = area->y1; row <= area->y2; row++)
    {
      int ret;
      struct lcddev_run_s lcd_run;

      lcd_run.row     = row;
      lcd_run.col     = area->x1;
      lcd_run.data    = (uint8_t *)data;
      lcd_run.npixels = area->x2 - area->x1 + 1;
      data += lcd_run.npixels;

      ret = ioctl(state.fd, LCDDEVIO_PUTRUN,
                  (unsigned long)((uintptr_t)&lcd_run));

      if (ret < 0)
        {
          int errcode = errno;

          gerr("ioctl(LCDDEVIO_PUTRUN) failed: %d\n", errcode);
          close(state.fd);
          return;
        }
    }

  /* Tell the flushing is ready */

  lv_disp_flush_ready(disp_drv);
}

This patch renders one row of pixels at a time. So it might not render as quickly as the previous version. (Especially with SPI DMA)

UPDATE: This patch is no longer needed, see #6837 (comment)

[tmedicci]

Hi everyone!

I’ve been thinking about this lcddev x framebuffer behavior since last week. I’ve read all this discussion, ones from the NuttX mailing list, ones related to the implementation of the lcddev itself and the discussions about framebuffer driver usage (#6564 #6551 ).

By analyzing Zephyr’s ST7789 implementation (thank you @lupyuen), I’ve seen that the data buffer contains only the data that actually would be necessary to draw the display (note that write_data_start points to the received buffer during the first iteration). That is not true for the current implementation of framebuffer’s putarea callback as it refers the fbmem buffer and forwards this pointer to the display’s drivers, i. e, the buffer that contains the data to draw the entire display: https://github.com/apache/incubator-nuttx/blob/fa2e1897eab8bc6838f0e8bc791a0d7fbaa58972/drivers/lcd/lcd_framebuffer.c#L216-L234.
Actually, this is what commit fb0fdea
states correctly while fixing putarea’s header description. However, this behavior is not compliant with lcddev (and maybe other implementations that don’t allocate a buffer that contains data from the entire display).

The problems:

1. Current implementation does not allow using displays’ putarea within lcddev driver. Applications should allocate a buffer to represent the entire display even if it only needs to update an area of the display or 2) either not using putarea, replacing it to putrun, row by row. Efficiency might be affected.

My suggestions:
There’d be no need to keep a reference for the buffer that represents the entire display. Framebuffer’s driver callback could forward to display’s driver the reference to the start of the area that actually would be sent. That would be very similar to Zephyr’s implementation. In fact, that was true before 664d45d.
Note that https://github.com/apache/incubator-nuttx/blob/881902d2cdf84902863cf848fb506be1613c8ac1/drivers/lcd/lcd_framebuffer.c#L219-L220 calculates the init of the buffer that contains data which is being refreshed. By applying that for putarea too (currently, only putrun does that), it’s possible to handle framebuffer and lcddev, optimizing memory and speed (by checking into displays’ drivers if it’s performing a full screen/for row mode, as stated:
https://github.com/apache/incubator-nuttx/blob/664d45dcbace03a879017aa99566592be31f4308/drivers/lcd/lcd_framebuffer.c#L218-L226

What do you think, @xiaoxiang781216, @Cynerd, @lupyuen, and @adamkaliszan? May I submit a PR for that?

There would be no need for a different callback and we can overcome the best of both worlds: efficiency for those who use lcddev or framebuffer. I’ve already checked that this works and minimal changes would be required (only for the drivers that use putearea and were defined recently. Namely, a small fix for ST7789 and APA102). I have a ST7789 and @acassis can test APA102, I think.

I look forward to hearing from you ;)

[xiaoxiang781216]

I agree to pass the start of drawing area to putarea, but we need an additional parameter to pass the frame buffer stride to lcd driver. Otherwise, lcd driver is impossible to support the partial refresh for frame buffer case.

[tmedicci]

Hi!

Thank you for your ack, @xiaoxiang781216 and @lupyuen !

I understand stride as similar as the pitch from Zephyr's. However, it may not be necessary because de LCD driver usually keeps track of its own bpp and is able to calculate the stride. That is the case for st7789 at least. The current implementation does some kind of "row by row" write that enables partial refresh without using the stride from the framebuffer:

https://github.com/apache/incubator-nuttx/blob/b8b541fbf5c75f13387560bb0639a4a5b27ec881/drivers/lcd/st7789.c#L450-L466

size and skip are defined as follows:
https://github.com/apache/incubator-nuttx/blob/b8b541fbf5c75f13387560bb0639a4a5b27ec881/drivers/lcd/st7789.c#L567-L576

That is possible because of the st7789_select(dev->spi, ST7789_BYTESPP * 8); and the definition of the buffer as a uint16_t.

I've tested a partial write for the ST7789 using framebuffer and everything works as expected (tested full row mode and an area that doesn't start at col_start = 0 and that doesn't end at col_end = xres - 1)

Also, it'd be possible to call getplaneinfo to retrieve the stride as well if needed by some driver.

I may be missing something here, so I'd be glad to hear from you if that makes sense ;)

[xiaoxiang781216]

Hi!

Thank you for your ack, @xiaoxiang781216 and @lupyuen !

I understand stride as similar as the pitch from Zephyr's.

Yes, you are right.

However, it may not be necessary because de LCD driver usually keeps track of its own bpp and is able to calculate the stride.

The stride is very different between frame buffer and lcd:

1. The stride for frame buffer is normally the full width(may add some padding) regardless the refresh region
2. The stride for lcd is always same as the refresh region

So, for example, if the screen is 480x640@RGB565 and the refresh region is (0, 0)-(240, 320):

1. The stride for frame buffer is 960 bytes
2. The stride for lcd buffer is 480 bytes

That is the case for st7789 at least. The current implementation does some kind of "row by row" write that enables partial refresh without using the stride from the framebuffer:

https://github.com/apache/incubator-nuttx/blob/b8b541fbf5c75f13387560bb0639a4a5b27ec881/drivers/lcd/st7789.c#L450-L466

size and skip are defined as follows:

https://github.com/apache/incubator-nuttx/blob/b8b541fbf5c75f13387560bb0639a4a5b27ec881/drivers/lcd/st7789.c#L567-L576

That is possible because of the st7789_select(dev->spi, ST7789_BYTESPP * 8); and the definition of the buffer as a uint16_t.

I've tested a partial write for the ST7789 using framebuffer and everything works as expected (tested full row mode and an area that doesn't start at col_start = 0 and that doesn't end at col_end = xres - 1)

If you use lcd driver interface, you most want the skip is zero. Basically, the lcd driver interface want the buffer only has the content of refresh region.

Also, it'd be possible to call getplaneinfo to retrieve the stride as well if needed by some driver.

I may be missing something here, so I'd be glad to hear from you if that makes sense ;)