SDL display driver doesn't support API calls from a thread other than main

[kartben]

Describe the bug

Trying to call display functions (ex display_write) from a thread other than main does not seem to work.

To Reproduce
Steps to reproduce the behavior:

1. change samples/drivers/display/src/main.c so that the contents of main() is effectively executed in a different thread
2. build and run on native_sim --> screen is black

Expected behavior

SDL driver should work in a multithreaded environment, just like other display drivers.

Impact

can't run custom GUI code that requires a dedicated thread using native sim

Environment (please complete the following information):

Linux x86_64, 3.6.99 f419ea7

[aescolar]

@kartben can you provide a minimal repro case?

[kartben]

@aescolar Tweaking the display driver sample like the below is the minimal repro. After applying the change, sample still works fine on e.g. M5Stack Core 2 (ILI9342C display controller), but SDL/native_sim_64 gives a black screen.

diff --git a/samples/drivers/display/src/main.c b/samples/drivers/display/src/main.c
index a9267b51c7..af14abc3b0 100644
--- a/samples/drivers/display/src/main.c
+++ b/samples/drivers/display/src/main.c
@@ -170,8 +170,24 @@ static inline void fill_buffer_mono10(enum corner corner, uint8_t grey,
 	fill_buffer_mono(corner, grey, 0xFFu, 0x00u, buf, buf_size);
 }
 
-int main(void)
+int gui_entry_point(void *arg1, void *arg2, void *arg3);
+
+K_THREAD_STACK_DEFINE(thread_stack, 4096);
+static struct k_thread thread;
+
+int main(void) {
+	k_thread_create(&thread, thread_stack, K_THREAD_STACK_SIZEOF(thread_stack),
+			(k_thread_entry_t)gui_entry_point, NULL, NULL, NULL,
+			K_PRIO_PREEMPT(5), 0, K_NO_WAIT);
+	return 0;
+}
+
+int gui_entry_point(void *arg1, void *arg2, void *arg3)
 {
+	ARG_UNUSED(arg1);
+	ARG_UNUSED(arg2);
+	ARG_UNUSED(arg3);
+	
 	size_t x;
 	size_t y;
 	size_t rect_w;

SDL documentation is pretty clear on the fact that e.g. SDL_RenderPresent() or SDL_PollEvent() should only be called from the thread that initialized the video system.

[aescolar]

CC @jfischer-no @danieldegrasse @dcpleung

[aescolar]

@kartben I can reproduce it with that example.

SDL documentation is pretty clear on the fact that e.g. SDL_RenderPresent() or SDL_PollEvent() should only be called from the thread that initialized the video system.

sdl_display_init() is run in the same "zephyr boot" thread that will eventually run the app main()
So it may be that you are right and that is the cause.
Also, as with a quick hack like this below, on top of your patch it renders properly (so sdl_display_init_bottom is called from the same thread that does the display updates):

diff --git a/drivers/display/display_sdl.c b/drivers/display/display_sdl.c
index f21288d1b0f..44013c5adfe 100644
--- a/drivers/display/display_sdl.c
+++ b/drivers/display/display_sdl.c
@@ -48,7 +48,10 @@ static inline uint32_t mono_pixel_order(uint32_t order)
        }
 }
 
-static int sdl_display_init(const struct device *dev)
+static int sdl_display_init(const struct device *dev) {
+       return 0;
+}
+int _sdl_display_init(const struct device *dev)
 {
        const struct sdl_display_config *config = dev->config;
        struct sdl_display_data *disp_data = dev->data;

diff --git a/samples/drivers/display/src/main.c b/samples/drivers/display/src/main.c
index a9267b51c74..1142d36ed30 100644
--- a/samples/drivers/display/src/main.c
+++ b/samples/drivers/display/src/main.c
@@ -189,6 +204,9 @@ int main(void)
        fill_buffer fill_buffer_fnc = NULL;
 
        display_dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_display));
+       _sdl_display_init(display_dev);
        if (!device_is_ready(display_dev)) {
                LOG_ERR("Device %s not found. Aborting sample.",
                        display_dev->name);

Anyhow, SDL is a bit far from my expertise.
But I guess that if this is indeed the reason, a fix would be to move all the sdl display handling into the same thread.
I won't have time to do this now though.
So I better remove myself as assignee.

Maybe you or one of the display maintainers has the bandwith?

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[KimBP]

@kartben I can reproduce it with that example.

SDL documentation is pretty clear on the fact that e.g. SDL_RenderPresent() or SDL_PollEvent() should only be called from the thread that initialized the video system.

sdl_display_init() is run in the same "zephyr boot" thread that will eventually run the app main() So it may be that you are right and that is the cause. Also, as with a quick hack like this below, on top of your patch it renders properly (so sdl_display_init_bottom is called from the same thread that does the display updates):

diff --git a/drivers/display/display_sdl.c b/drivers/display/display_sdl.c
index f21288d1b0f..44013c5adfe 100644
--- a/drivers/display/display_sdl.c
+++ b/drivers/display/display_sdl.c
@@ -48,7 +48,10 @@ static inline uint32_t mono_pixel_order(uint32_t order)
        }
 }
 
-static int sdl_display_init(const struct device *dev)
+static int sdl_display_init(const struct device *dev) {
+       return 0;
+}
+int _sdl_display_init(const struct device *dev)
 {
        const struct sdl_display_config *config = dev->config;
        struct sdl_display_data *disp_data = dev->data;

diff --git a/samples/drivers/display/src/main.c b/samples/drivers/display/src/main.c
index a9267b51c74..1142d36ed30 100644
--- a/samples/drivers/display/src/main.c
+++ b/samples/drivers/display/src/main.c
@@ -189,6 +204,9 @@ int main(void)
        fill_buffer fill_buffer_fnc = NULL;
 
        display_dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_display));
+       _sdl_display_init(display_dev);
        if (!device_is_ready(display_dev)) {
                LOG_ERR("Device %s not found. Aborting sample.",
                        display_dev->name);

Anyhow, SDL is a bit far from my expertise. But I guess that if this is indeed the reason, a fix would be to move all the sdl display handling into the same thread. I won't have time to do this now though. So I better remove myself as assignee.

Maybe you or one of the display maintainers has the bandwith?

This "workaround" wont work if you also add lvgl into your application. lvgl_init() is called too early and will miss a display to work at.
Forcing lv_task_handler() to be called in context of main-thread like suggested by @jakkra I also find very limiting.
Wouldn't a solution be to (optionally) make the sdl-display-driver spawn a thread being responsible for all the SDL critical stuff like initializing 'video system' and calling RenderPresent etc?
I doubt the overhead of a thread in a native_sim application would cause any resource issues

[aescolar]

@KimBP just to be clear: That (#71410 (comment)) was not a workaround, and I would very much discourage anybody from doing that. That comment was only meant to provide information about the possible cause of the issue.

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.