Skip to content

Conversation

@seokhun-eom24
Copy link
Contributor

@seokhun-eom24 seokhun-eom24 commented Oct 29, 2025

1. Add centralized state to lcd driver

Add new mipi_state to replace lcdonoff field to make centralized lcd state management.
And make a new function to handle this state transitions safely.
Also refactors power on/off functions to use the new state machine.

2. Add simple image library support for BMP image load

Add new image_lib.c module to handle 16-bit BMP image decoding and conversion to RGB565 format for LCD display.
Now we can load image to buffer with this common image library function.
Update Make.defs to include the new source file in LCD driver build.

3. Add config for lcd splash image

Add CONFIG_LCD_SPLASH_IMAGE to enable splash image feature.
Only when this config is enabled, image_lib.c will be compiled.
And add config for image format and silent boot support.
Modifies the code to include splash image functionality conditionally based on Kconfig settings

4. Update lcd config for loadable ext ddr mode

5. Add splash image ops for lcd_dev_s

Add splash image ops for lcd_dev_s struct.
This splash image feature is general for all lcd driver.
Set existing splash image load function to this ops.

6. Rename vendor ID cmd retry config to vendor init cmd retry

Rename CONFIG_LCD_SEND_VENDOR_ID_CMD_RETRY_COUNT to CONFIG_LCD_SEND_VENDOR_INIT_CMD_RETRY_COUNT in Kconfig and source files.
This change makes more clear that this configuration is used to retry sending vendor specific initialization commands.

7. Update lcd config for init cmd retry

Update defconfig to use CONFIG_LCD_SEND_VENDOR_INIT_CMD_RETRY_COUNT instead of CONFIG_LCD_SEND_VENDOR_ID_CMD_RETRY_COUNT.

8. Replace common lcd function to lcd_dev.c

Replace lcd_rotate_buffer, lcd_render_bmp, lcd_load_splash from mipi_lcd.c to lcd_dev.c
In lcddev_register, handle splash image rendering feature as a part of common lcd driver.

In the previous, image_load_bmp_file function rotate buffer and save into buffer,
because this image was rendered without sw rotation.
From now splash image also use putarea that works with sw rotation.
So remove rotation from image_load_bmp_file.

@seokhun-eom24 seokhun-eom24 marked this pull request as draft October 29, 2025 08:07
@seokhun-eom24 seokhun-eom24 marked this pull request as ready for review October 29, 2025 09:00
Comment on lines 665 to 668
static int lcd_render_bmp(FAR struct lcd_dev_s *dev, const char *bmp_filename)
{
int xres = LCD_XRES;
int yres = LCD_YRES;
uint8_t *fullscreen_buffer = NULL;
FAR struct mipi_lcd_dev_s *priv = (FAR struct mipi_lcd_dev_s *)dev;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not mipi specific function.
SPI LCDc might need this function.

Let's move it to lcd_dev.c

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I move this feature to lcddev_register in lcd_dev.c

Comment on lines 710 to 745
static int lcd_load_splash(struct lcd_dev_s *dev, const char* image_path)
{
bool is_silent_mode;
char bmp_file_path[50];
int ret = OK;
#ifdef CONFIG_LCD_SPLASH_IMAGE
int ret;
FAR struct mipi_lcd_dev_s *priv = (FAR struct mipi_lcd_dev_s *)dev;

is_silent_mode = silent_reboot_is_silent_mode();
if (!is_silent_mode) {
snprintf(bmp_file_path, sizeof(bmp_file_path), "%s/%dx%d/splash_normal.bmp", CONFIG_LCD_SPLASH_IMAGE_PATH, CONFIG_LCD_YRES, CONFIG_LCD_XRES);
} else {
snprintf(bmp_file_path, sizeof(bmp_file_path), "%s/%dx%d/splash_silent.bmp", CONFIG_LCD_SPLASH_IMAGE_PATH, CONFIG_LCD_YRES, CONFIG_LCD_XRES);
}

// Check if BMP file exists before rendering
FILE *test_file = fopen(bmp_file_path, "rb");
if (!test_file) {
lcddbg("BMP file not found at %s. LCD OFF\n", bmp_file_path);
return ERROR;
}
fclose(test_file);

while (sem_wait(&priv->sem) != OK) {
ASSERT(get_errno() == EINTR);
}

ret = lcd_power_on(priv);
if (ret != OK) {
lcddbg("Failed to turn on the LCD\n");
sem_post(&priv->sem);
return ret;
}

ret = lcd_render_bmp(dev, bmp_file_path);
ret = lcd_render_bmp(dev, image_path);
if (ret != OK) {
lcd_power_off(priv);
sem_post(&priv->sem);
return ret;
}
priv->config->backlight(CONFIG_LCD_MAXPOWER);
priv->power = CONFIG_LCD_MAXPOWER;
ret = set_mipi_state(priv, MIPI_STATE_VIDEO_ON);
if (ret != OK) {
lcddbg("Failed to switch to video mode\n");
lcd_power_off(priv);
sem_post(&priv->sem);
}
sem_post(&priv->sem);
priv->config->mipi_mode_switch(VIDEO_MODE);
priv->lcdonoff = LCD_ON;

return OK;
#else
return ERROR;
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also not mipi-dsi sepecific functionallity

If need to make switching mipi mode and controilling backlight, you can add api enable/disable/backlight to ops

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right this is not dependent to mipi.
I moved this function and lcd_rotate_buffer to lcd_dev.c

{
int retries = CONFIG_LCD_SEND_VENDOR_ID_CMD_RETRY_COUNT;
mipi_state_t old_state = priv->mipi_state;
lcm_setting_table_t display_off_cmd = {0x28, 0, {0x00}};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make definition about it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to use mipi_dsi_dcs_set_display_off that mipidsi driver's function.

Comment on lines 489 to 516
#ifdef CONFIG_LCD_SPLASH_SILENT_BOOT
is_silent_mode = silent_reboot_is_silent_mode();
if (!is_silent_mode) {
snprintf(splash_image_path, sizeof(splash_image_path), "%s/%dx%d/splash_normal.bmp", CONFIG_LCD_SPLASH_IMAGE_PATH, CONFIG_LCD_YRES, CONFIG_LCD_XRES);
} else {
snprintf(splash_image_path, sizeof(splash_image_path), "%s/%dx%d/splash_silent.bmp", CONFIG_LCD_SPLASH_IMAGE_PATH, CONFIG_LCD_YRES, CONFIG_LCD_XRES);
}
#else
snprintf(splash_image_path, sizeof(splash_image_path), "%s/%dx%d/splash_normal.bmp", CONFIG_LCD_SPLASH_IMAGE_PATH, CONFIG_LCD_YRES, CONFIG_LCD_XRES);
#endif

// Check if splash image file exists before rendering
FILE *test_file = fopen(splash_image_path, "rb");
if (test_file != NULL && dev->loadsplash) {
ret = dev->loadsplash(dev, splash_image_path);
if (ret == OK) { // LCD ON
#ifdef CONFIG_PM
(void)pm_suspend(lcd_info->pm_domain);
(void)pm_suspend(lcd_info->pm_domain);
#endif
silent_reboot_lock();
silent_reboot_lock();
}
} else if (test_file == NULL) {
lcddbg("Image file not found at %s. LCD OFF\n", splash_image_path);
} else {
lcddbg("ERROR: Failed to load splash image %s\n", splash_image_path);
}
fclose(test_file);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is not needed checking CONFIG_LCD_SPLASH_IMAGE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought when CONFIG_LCD_SPLASH_IMAGE is not defined, loadsplash will return error and nothing happen for this scenario.
But wrapping with CONFIG_LCD_SPLASH_IMAGE and remove unnecessary function calling would be better.
Thank you.


static int set_mipi_state(struct mipi_lcd_dev_s *priv, mipi_state_t new_state)
{
int retries = CONFIG_LCD_SEND_VENDOR_ID_CMD_RETRY_COUNT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think, the config name CONFIG_LCD_SEND_VENDOR_INIT_CMD_RETRY_COUNT is better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. CONFIG_LCD_SEND_VENDOR_INIT_CMD_RETRY_COUNT would be more precise naming for now.
I updated Kconfig and defconfig.

return OK;
}

switch(new_state) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems beneficial to consolidate the code related to state changes in one place. 👍👍😊

It seems you considerd case of all state swap 👍

send_cmd(priv, display_off_cmd);
/* The power off must operate only when LCD is ON */
priv->config->power_off();
if (priv->mipi_state == MIPI_STATE_CMD_OFF) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you make set_mipi_state api.
How about replace it to get_mipi_state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made get_mipi_state and use this function to get mipi state.
This approach would be more safe.
Thank you.

Comment on lines 538 to 541
if (set_mipi_state(priv, MIPI_STATE_CMD_ON) != OK) {
return ERROR;
}

return OK;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (set_mipi_state(priv, MIPI_STATE_CMD_ON) != OK) {
return ERROR;
}
return OK;
return set_mipi_state(priv, MIPI_STATE_CMD_ON);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for suggest.
I applied it for lcd_power_on and lcd_power_off.

This path is used find splash image during booting.
Final resource path is defined like "{LCD_SPLASH_IMAGE_PATH}/{LCD_YRES}x{LCD_XRES}/splash_{normal/silent}.bmp"

choice
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently format of LCD pixel data is hard coded in mipi as 16bit only
amebasmart_mipi.c
priv->MIPI_InitStruct->MIPI_VideoDataFormat = MIPI_VIDEO_DATA_FORMAT_RGB565;
So we have to make sure that if we make new config like this for 24 bit and use that without changing mipi code, it will have issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thank you for pointing out an important point.

In my view, the MIPI format and the splash image format are independent.

To support new format like 24 bit, I think the two things below should be implemented.

  1. A Kconfig for the MIPI format must be provided.

    • Supporting various formats through such a configuration would require a large‑scale overhaul, involving changes to the init command, buffer handling, render functions, and other parts of mipi and lcd.
  2. Code that converts between the splash format and the MIPI format (e.g., rgb565 → rgb888) should be supplied in image_lib.c.

    • It would be preferable to make not only the color format but also the image format mutually compatible.

If we only need to support a configuration such as LCD_SPLASH_IMAGE_BMP_24BIT, we can simply provide conversion code in image_lib.c.
However, supporting a configuration like MIPI_VIDEO_DATA_FORMAT_RGB888 would require a substantial amount of work.

Therefore, at this stage I believe it is realistic to either leave things as they are or to prepare the configuration so that the MIPI format is supported only with the single option MIPI_VIDEO_DATA_FORMAT_RGB565.
If support for additional formats is required, it should be handled in a separate PR.

May I ask what you think about this approach?

@seokhun-eom24 seokhun-eom24 force-pushed the 251028-lcd-refactor branch 2 times, most recently from 8d31448 to 64eccee Compare October 30, 2025 01:24
int img_width;
int img_height;

bmp_file = fopen(filename, "rb");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

posix(open) is faster than libc (fopen, fread)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use open and other libc functions.
Thank you.


ret = lcd_init_put_image(dev);
if (ret == OK) { // LCD ON
#ifdef CONFIG_LCD_SPLASH_IMAGE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is VFS layer, so from line 490 to 499 should be moved to 'loadsplash'. Each of lower driver (actual device) knows that path of image & resolution. Hence you can get rid of below KCONFIGs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for advice.
But I thought loading image is not dependent to specific lcd driver.
So I replaced all load image related feature to lcd_dev.c after your review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definately, Splash image has dependecies.

  1. Type of Product
  2. Type of GUI guide
  3. Resolution of LCD
    How will you handle all of these in VFS layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point.
Normally, an LCD driver doesn’t know which image it should display. Therefore, each specific LCD must provide information about itself and the splash image; the driver obtains this data through its operations and then renders the image.

For now, the splash‑image path is predefined by the TizenRT platform, so this approach works. However, we should implement a more flexible solution in the future.

#endif /* CONFIG_LCD_SPLASH_SILENT_BOOT */

// Check if splash image file exists before rendering
FILE *test_file = fopen(splash_image_path, "rb");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Think below is better.

	FILE *test_file = fopen(splash_image_path, "rb");
	if (test_file == NULL) {
		lcddbg("Image file not found at %s. LCD OFF\n", splash_image_path);
		return -ENOENT;
	}
	if (dev->loadsplash) {
		lcddbg("ERROR: loadsplash is not supported);
		fclose(test_file);
		return -ENOSYS;
	}
	
	ret = dev->loadsplash(dev, splash_image_path);
	if (ret == OK) { // LCD ON
#ifdef CONFIG_PM
		(void)pm_suspend(lcd_info->pm_domain);
#endif
		silent_reboot_lock();
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied this approach for checking test_file and call functions.
Thank you.

@@ -0,0 +1,213 @@
/****************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can discuss with team internally.
IMO, this is not only related to LCD. Other module can use this in future..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This mini image library can be used by other modules in the future.
We should discuss whether to adopt an open‑source library or develop our own implementation.
I hope we can have that discussion soon.

Add new mipi_state to replace lcdonoff field to make centralized lcd state management.
And make a new function to handle this state transitions safely.
Also refactors power on/off functions to use the new state machine.

Signed-off-by: seokhun-eom <[email protected]>
Add new image_lib.c module to handle 16-bit BMP image decoding and conversion to RGB565 format for LCD display.
Now we can load image to buffer with this common image library function.
Update Make.defs to include the new source file in LCD driver build.

Signed-off-by: seokhun-eom <[email protected]>
Add CONFIG_LCD_SPLASH_IMAGE to enable splash image feature.
Only when this config is enabled, image_lib.c will be compiled.
And add config for image format and silent boot support.
Modifies the code to include splash image functionality conditionally based on Kconfig settings

Signed-off-by: seokhun-eom <[email protected]>
Add splash image ops for lcd_dev_s struct.
This splash image feature is general for all lcd driver.
Set existing splash image load function to this ops.

Signed-off-by: seokhun-eom <[email protected]>
…retry

Rename CONFIG_LCD_SEND_VENDOR_ID_CMD_RETRY_COUNT to CONFIG_LCD_SEND_VENDOR_INIT_CMD_RETRY_COUNT in Kconfig and source files.
This change makes more clear that this configuration is used to retry sending vendor specific initialization commands.

Signed-off-by: seokhun-eom <[email protected]>
Update defconfig to use CONFIG_LCD_SEND_VENDOR_INIT_CMD_RETRY_COUNT instead of CONFIG_LCD_SEND_VENDOR_ID_CMD_RETRY_COUNT.

Signed-off-by: seokhun-eom <[email protected]>
Replace lcd_rotate_buffer, lcd_render_bmp, lcd_load_splash from mipi_lcd.c to lcd_dev.c
In lcddev_register, handle splash image rendering feature as a part of common lcd driver.

In the previous, image_load_bmp_file function rotate buffer and save into buffer,
because this image was rendered without sw rotation.
From now splash image also use putarea that works with sw rotation.
So remove rotation from image_load_bmp_file.

Signed-off-by: seokhun-eom <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants