Skip to content

drivers: display: sdl: Ensure draw is performed from init thread #87883

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

Merged
merged 1 commit into from
Apr 8, 2025

Conversation

faxe1008
Copy link
Collaborator

@faxe1008 faxe1008 commented Mar 31, 2025

Add a thread to the SDL display driver to ensure that init and renderer calls are performed from the same thread.

Fixes #71410.

Ping @Finomnis @PetervdPerk-NXP

@faxe1008 faxe1008 force-pushed the sdl_draw_thread branch 2 times, most recently from 59c93b7 to a2f0219 Compare March 31, 2025 08:18
@faxe1008 faxe1008 requested review from aescolar and JarmouniA March 31, 2025 08:46
pdgendt
pdgendt previously approved these changes Mar 31, 2025
VynDragon
VynDragon previously approved these changes Mar 31, 2025
Copy link
Collaborator

@VynDragon VynDragon left a comment

Choose a reason for hiding this comment

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

Tested functionaility, works. Looks like it makes sense and i see no issue with code.

Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

Thank you very much @faxe1008

disp_data->mutex, disp_data->texture,
disp_data->background_texture, disp_data->buf,
disp_data->display_on, desc->frame_incomplete);
if (k_current_get() == &disp_data->sdl_thread) {
Copy link
Member

Choose a reason for hiding this comment

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

Honest question: Given that it seems this sdl_thread is a thread meant only to call the bottom , how do you expect this to happen?
(same comment for sdl_display_blanking_*)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very good question. Currently the handling of input events happens in another thread:
https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/native/common/sdl/sdl_events.c#L20

So when adding logging to see where write, SDL event handling and input callback are invoked I get this:

sdl_display_write  THREAD ID: 0x61cb80
sdl_handle_events  THREAD ID: 0x510980
sdl_input_callback THREAD ID: 0x510980

I feel like this needs to also happen within the sdl thread that I am adding here, since there is also manipulation of the renderer going on within sdl_handle_window_event (https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/native/common/sdl/sdl_events_bottom.c#L16) . Eventually this means that sdl_input_callback callback will happen in the same thread as sdl_display_write if one selects CONFIG_INPUT_MODE_SYNCHRONOUS. Currently since the msgq can only hold one item this would be an issue, I think. Maybe @Finomnis can chime in here since this concern was originally raised by him.

@faxe1008 faxe1008 dismissed stale reviews from VynDragon and pdgendt via 1754400 March 31, 2025 11:19
@PetervdPerk-NXP
Copy link
Collaborator

Screenshot 2025-03-31 133142
Running my code with both SDL_DISPLAY_USE_HARDWARE_ACCELERATOR=n and SDL_DISPLAY_USE_HARDWARE_ACCELERATOR=y gives me this checkerboard.

The code seems to be running and accepting inputs based on looking on the console output.

@faxe1008 faxe1008 force-pushed the sdl_draw_thread branch 3 times, most recently from 9969a03 to dad3b57 Compare April 1, 2025 14:41
@aescolar aescolar dismissed their stale review April 1, 2025 15:19

Addressed

pdgendt
pdgendt previously approved these changes Apr 1, 2025
@aescolar
Copy link
Member

aescolar commented Apr 1, 2025

Yeah the mutex needs to protect that as well.

@faxe1008 I look forward to that last change, so I approve :)

@@ -80,4 +80,10 @@ config SDL_DISPLAY_TRANSPARENCY_GRID_CELL_COLOR_2
help
The color of the even cells in the transparency grid.

config SDL_DISPLAY_THREAD_PRIORITY
int "SDL display thread priority"
default 0
Copy link
Contributor

@Finomnis Finomnis Apr 1, 2025

Choose a reason for hiding this comment

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

I'd put the default priority way lower, I'd almost argue I'd put it to the lowest priority possible. Otherwise low priority threads calling draw functions would cause a priority inversion.

Is there a way to use priority inheritance to bump this priority dynamically when higher priority threads are waiting for this?

I know mutexes can do priority inheritance, right? Is there a way to achieve the same with the semaphore?

Copy link
Member

@aescolar aescolar Apr 2, 2025

Choose a reason for hiding this comment

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

@Finomnis note this is only for native_sim, where you are not constrained like in an embedded device.
But more importantly note that this thread would only get unblocked if the thread that wants to draw has just been scheduled. So even though this thread has high priority, it is only running as a result of that requesting thread running.
=> A high priority for it seems sensible in my eyes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the default priority to NUM_PREEMPT_PRIORITIES.

@faxe1008 faxe1008 force-pushed the sdl_draw_thread branch 2 times, most recently from 9e4dea6 to f04f321 Compare April 7, 2025 12:53
Add a thread to the SDL display driver to ensure that init and renderer
calls are performed from the same thread.

Fixes zephyrproject-rtos#71410.

Signed-off-by: Fabian Blatz <[email protected]>
@kartben kartben merged commit 11b9eba into zephyrproject-rtos:main Apr 8, 2025
24 checks passed
@pdgendt
Copy link
Collaborator

pdgendt commented Apr 8, 2025

@faxe1008 note for the future: do not add Fixes #.... to the commit message body as per the guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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