-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drivers/lcd/st7789: update putarea() method #6657
Conversation
The putarea documentation originally suggested that provided buffer contains just the rectangle to be updated. The commit 664d45d changed the expected behavior for lcd_framebuffer but failed to propagate this change to the driver's documentation. Now the expected behavior is that the whole frame is passed in buffer and it is driver's responsibility to pick the correct pixels according to the provided rectangle coordinates. This change requires update of the LCD drivers if they implement this function.
756e706
to
e84222a
Compare
'putarea' function now has to select the appropriate values from the whole frame buffer instead of the previous expectation of having just specified rectangle passed to it. This required extension of WRRAM handling function as we now are not writing simply buffer but we want to skip some values and do that multiple times. Having two implementation in this case is worst as this function is actually called only twice in the whole code (thus making dedicated function for every call if we would have two variants). Calling `st7789_wrram` multiple times is not an option as command ST7789_RAMWR resets the position and thus calling it multiple times just overwrites the previous values.
cf0a2ed
to
b5725c1
Compare
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 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! |
@lupyuen could you please attach some picture of a situation that you are describing, just to understand what "random noise" mean? |
Can you please try the I tested it and right now developing my application with LVGL with this patch and thus, this might be something platform or setting dependent. |
Sorry I'm using LCD Device for ST7789, not Framebuffer. I'm calling How do I use Framebuffer with ST7789? |
@lupyuen I think that you don't have to use framebuffer. You only have to change how you use |
(Sorry if I misunderstood this) Does it mean that the 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 As you have suggested, I'll pass a recognizable pattern to UPDATE: Here's what I found out: The https://gist.github.com/lupyuen/f9a5fb2a578be5c3060e87fa3fdd37e7
The buffer passed to Checking the implementation of 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 Is there another function that we could call to render the screen in small buffers, like with LVGL? Thanks! |
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 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. |
Thanks I'll change the 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 |
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. |
Thanks Xiao Xiang, yes I prefer the previous implementation of The current implementation of Previously we blasted the entire region of pixels in a single SPI Transfer (which performs really well with DMA): 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 |
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.
I would not complicate the I am also not sure if reverting to the previous |
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 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 |
Hi there! I've been facing this same problem: the ST7789 + |
Hi @tmedicci this patch for 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) |
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 The problems:
My suggestions: 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 ;) |
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. |
Hi! Thank you for your ack, @xiaoxiang781216 and @lupyuen ! I understand
That is possible because of the 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 Also, it'd be possible to call I may be missing something here, so I'd be glad to hear from you if that makes sense ;) |
Yes, you are right.
The stride is very different between frame buffer and lcd:
So, for example, if the screen is 480x640@RGB565 and the refresh region is (0, 0)-(240, 320):
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.
|
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.