display: add display_show

[Finomnis]

Fixes #79798.

Indicates to the display that all previous writes should be flushed to the display.

This adds support for display drivers that internally contain a double-buffering or latching mechanism.
Currently the only way to implement those displays is to present all writes to the user immediately, which could cause tearing artifacts for multi-write renders. (it's quite common for UI managers to render to partial framebuffers in low-memory situations. LVGL, for example, recommends 1/10th of the screen size)

Open questions:

• Name of the 'end-of-frame' display driver api function?
  • display_show (current PR code)
  • display_present (could be confused with "display is present")
  • display_flush (sounds like it's optional)
  • display_finalize_frame (might be a little too verbose)
  • display_flip (probably not because could be confused with x or y pixel order setting, like, 'flipping' the image)
  • display_pageflip (might be too usecase specific)

[pdgendt]

Introducing an API change should be accompanied with at least one driver that implements it.

That being said, this would tie the display driver closely to the lvgl way of working, which might require some more discussion.

[Finomnis]

Introducing an API change should be accompanied with at least one driver that implements it.

I don't think there is an upstream driver that requires this right now :/
An FPGA driver in our own project requires it.

That being said, this would tie the display driver closely to the lvgl way of working, which might require some more discussion.

I kind of disagree with this (that it ties to LVGL, i'm of course open to discussions). I think every display works in frames, and every frame has an end.

An alternative API would be display_start_frame and display_end_frame, but I don't really know a usecase where start_frame would be necessary. A display usually knows that a frame started when the first write arrives.

[Finomnis]

The problem I have is that regardless of LVGL, the display_write function nowhere states that it should be the only write per frame. And as it does not, there needs to be some way to signal double-buffered/latched displays that the modified content should now be displayed. This functionality is simply missing.

[Finomnis]

Introducing an API change should be accompanied with at least one driver that implements it.

Implemented it in dummy_display driver.

[Finomnis]

Is the name bothering? Would display_flush be better than display_finalize_frame? Or display_present, display_flip, display_show or similar?

EDIT: added a couple of options and thoughts in the PR description

[pdgendt]

This PR should be split into different commits:

• Introduce API function
• Implement in dummy driver
• Calling from LVGL

[pdgendt]

The problem I have is that regardless of LVGL, the display_write function nowhere states that it should be the only write per frame. And as it does not, there needs to be some way to signal double-buffered/latched displays that the modified content should now be displayed. This functionality is simply missing.

@danieldegrasse @jfischer-no can you weigh in here?

[pdgendt]

Introducing an API change should be accompanied with at least one driver that implements it.

This PR should be split into different commits

@pdgendt I feel like those are contradictory statements :) I'm happy to split it, as long as I don't have to merge them again because of the 'should be accompanied with at least one driver' requirement.

I am not contradicting myself: 1 PR, 3 commits please

[Finomnis]

Aaaah. Now it makes sense. Thanks for clarifying.

[danieldegrasse]

@Finomnis good idea to use the sdl display for it 👍

Agreed, thank you for doing this!

Something I was considered regarding the API as we have it now- what display_show effectively indicates to the display driver is that the rendering framework (LVGL or otherwise) is done with the current frame.

Another use case I'm looking at right now that would benefit from this information is the "tearing enable" signal used by some displays. The idea of this signal is that it indicates when the display controller will start reading from its internal graphics ram- so if used correctly, the MCU can time writes to that graphics RAM so that the read pointer of the display controller never overlaps with the write pointer from the MCU, and the tearing effect is avoided.

Here's a timing diagram that makes this a bit more clear (from the ST7796S controller datasheet):

When using a framebuffer that only covers part of the screen (IE 1/10), we would ideally like to stream all the framebuffers for a given frame after one TE output signal- to do this, the display controller needs a way to know which framebuffer is the last one in the given frame. The driver would wait for the TE signal during the next display_write call, since that call would be for a new frame.

We could use display_show for this, but we would be adding this API to nearly every display in Zephyr- the TE signal is very common across SPI displays.

I know we previously discussed extending the display_write API in some way- have we considered extending the display_buffer_descriptor structure to add a last field? This wouldn't break downstream applications, and would allow LVGL or other frameworks to signal to display drivers which frame is the last.

[Finomnis]

@danieldegrasse I specifically don't want to add it to write, because it's asynchronous to that.

Meaning, you can reuse the partial frame buffer already while show is still processing. This is especially useful in LVGL threaded present mode, where the render thread can continue while the 'present' thread still runs show.

That said, this is already possible with the show api, no? Just add a boolean that tracks if we are the first write after a show.

On the other hand, if you say "let's not introduce an API that breaks downstream GUI frameworks", that would annoy me a little, because that's exactly what I was saying a couple of weeks ago, and where other people here said it's fine.

[Finomnis]

@danieldegrasse The reason I'm hesitant is because I don't see what the screen_info flag would solve.

Even if the UI manager would know that the display supports flushing, there still wouldn't be a conditional based on it in the UI. I would implement it in a way that it would simply always issue flushes, no matter what the display capabilities are, because in the worst case, the flush simply doesn't do anything. So the flag doesn't really carry any useful information to the UI.

Further, lets say its description is "when this flag is set, the display expects a flush call before it shows its content on screen". That immediately breaks all existing UIs, meaning, it won't show anything on screen. (Except of LVGL, which we patch here to behave that way).

The reason this happens is because this flag introduces a "MUST" requirement. Like, "if this flag is set, the UI MUST issue a flush call". Backwards compatibility only works with "MAY" requirements.

Like, if we were to introduce a way for the UI manager to tell the display "I am capable of issuing flush calls, you MAY buffer the pixel data and not display it right away", then everything would be fine, and all existing UI managers simply would not tell the display that it is allowed to do so.

One way to do so would be to introduce another disable_immediate_mode or allow_buffering API function, which tells the display that it is allowed to buffer. The display may simply ignore that and stay in immediate mode, and then ignore subsequent flush calls, so everything is fine (= backwards compatibility of displays). On the other hand, UIs can simply not issue it and displays stay in immediate mode (= backwards compatibility of UIs). But if both are updated to the new API, the UI can issue that function and the display understands it and we have the new sync behaviour.

The disadvantage of such an approach would be that it would change the internal state of the display driver permanently. Maybe a better alternative would be to add an allow_buffering to the display_buffer_descriptor that is disabled by default. That would solve the corner case where multiple UIs write to a display (maybe a split framebuffer with two UIs, like PiP mode?) and one UI has the capability and the other doesn't.

EDIT: Actually, I retract the suggestion of adding it to the display_buffer_descriptor. It introduces undefined behavior in existing UIs that do not initialize that field. I think that adding a second API function that opts-in to this behavior would be the better solution.

@danieldegrasse That's kind of exactly what I was talking about here, and what was dismissed.

So I though the fact that displays will no longer be backward compatible with existing GUI managers is something everyone understands and accepts.

[Finomnis]

Yeah, this is a great explanation- thank you for laying it out. It seems like LVGL has lv_display_flush_is_last for this use case. Personally I see the reasoning for this API- I'm not sure how else we can do this effectively with partial rendering. Maybe we could add a flag to display_write? I think a new API is better here though, since we don't break downstream applications that way.

@danieldegrasse Also you said this, which I thought includes the display_buffer_descriptor.

[Finomnis]

I know we previously discussed extending the display_write API in some way- have we considered extending the display_buffer_descriptor structure to add a last field? This wouldn't break downstream applications, and would allow LVGL or other frameworks to signal to display drivers which frame is the last.

This would introduce the same problem as was previously discussed, though. All displays that only show their content once the last field gets delivered now won't ever show any image. That's why you guys wanted to add the display cap that it requires a 'show'.

My previous suggestion was to add a allow_buffering field or is_not_last field to the display_buffer_descriptor, so that by default old GUIs simply issue a present in every write.

That has the drawback, though, that every GUI that doesn't explicitly zero this bit technically leaves it uninitialized, and hence causes undefined behavior. I think it will get zeroed in most cases, but it still is undefined behavior in C I think.

[Finomnis]

@danieldegrasse Summary:

• I'm fine with either
• Advantage of display_buffer_descriptor flag is that it doesn't break anything (assuming uninitialized fields in structs get zeroed automatically) and everything is backwards compatible
• Advantage of current approach is that display_show can get called while the previous write buffer gets reused elsewhere already (which is how I implemented it in LVGL already, so this is not a hypothetical advantage). Although it is valid to reason if that would actually be an advantage, or if it's an overoptimization.

Disregarding the asynchronism advantage, I think adding a flag in display_buffer_descriptor has many advantages. Although I would negate it so that the default 'false' value does not break new display implementations, like has_continuation or similar.

So your decision: Should I refactor? I can do it, if requested, no problem. That will also make this change a lot smaller.

[danieldegrasse]

@danieldegrasse I specifically don't want to add it to write, because it's asynchronous to that.

Meaning, you can reuse the partial frame buffer already while show is still processing. This is especially useful in LVGL threaded present mode, where the render thread can continue while the 'present' thread still runs show.

Okay, I follow- essentially display_show may take some time, but it should not block LVGL from using rendering buffers. The reasoning makes sense here.

@danieldegrasse That's kind of exactly what I was talking about here, and what was dismissed.

So I though the fact that displays will no longer be backward compatible with existing GUI managers is something everyone understands and accepts.

I apologize if I or anyone else came off as dismissive here- I think the concern is that we wanted to avoid a hard migration- that is changing the display_write API outright. With a flag like the PR uses now, GUI managers will work fine with existing displays, and will only need to be updated gradually as more displays use this API.

My previous suggestion was to add a allow_buffering field or is_not_last field to the display_buffer_descriptor, so that by default old GUIs simply issue a present in every write.

That has the drawback, though, that every GUI that doesn't explicitly zero this bit technically leaves it uninitialized, and hence causes undefined behavior. I think it will get zeroed in most cases, but it still is undefined behavior in C I think.

I think this is undefined behavior, yeah. I agree though that modifying the display_buffer_descriptor seems like an attractive option.

To be clear, I'm not opposed to the current state of the PR- I just wanted to engage because I realized that displays using the tearing enable signal will also benefit from the GUI framework communicating the "last frame" to them in some way- the reason I leaned towards the display buffer descriptor is that the MIPI DBI layer is used for SPI based displays- if we extended the display buffer descriptor, we could pass the "last frame" flag to the MIPI DBI controller (where it would actually be used) very easily

[Finomnis]

Okay, I follow- essentially display_show may take some time, but it should not block LVGL from using rendering buffers. The reasoning makes sense here.

@danieldegrasse To be honest, the more I think about it the less this seems like an actual advantage. You can already use double buffering and the additional present thread for that; so there is already some kind of asynchronism.

I'm kind of leaning towards a refactor. Although at this point I might even create a new PR as it will be completely different.

[danieldegrasse]

Disregarding the asynchronism advantage, I think adding a flag in display_buffer_descriptor has many advantages. Although I would negate it so that the default 'false' value does not break new display implementations, like has_continuation or similar.

So your decision: Should I refactor? I can do it, if requested, no problem. That will also make this change a lot smaller.

I personally am in favor of using a flag in the display_buffer_descriptor, but I want to avoid unnecessary work for you, so let's get a few other opinions.

To summarize- we have the following use cases:

• displays that write data to an external framebuffer that will only be displayed once requested. These displays need to know when the entire framebuffer is sent so they can explicitly display it.
• displays that write data to an external framebuffer that is displayed on a fixed interval. These displays need to know when the entire framebuffer is sent so they can time the next write to be after the current framebuffer has been displayed.

The API changes we'd add to enable this:

• add a "frame_incomplete" flag to the struct display_buffer_descriptor. When set, this indicates the display buffer can be buffered, and is not the final buffer in the frame. If clear, the display controller should write this buffer directly to the display.

GUI frameworks can use this feature by setting the "frame_incomplete" flag for all writes except the final write in a display frame. This way, the controller is effectively aware which write is the final one, and can take appropriate action.

The only open issue here to solve is how to deal with GUI frameworks that have not been updated and do not initialize the "frame_incomplete" flag- the flag would be an undefined value if the display buffer descriptor was declared on the stack.

@Finomnis does this seem like a good summary?

[Finomnis]

@Finomnis does this seem like a good summary?

@danieldegrasse Yes, I think so.

The only open issue here to solve is how to deal with GUI frameworks that have not been updated and do not initialize the "bufferable" flag- the flag would be an undefined value if the display buffer descriptor was declared on the stack.

In the case they do

struct display_buffer_descriptor desc = {
    .member = foo,
    .member2 = foo2,
};

then it should work, I think, I think missing members do get zeroed. (Which is how we use most of our APIs; if drivers don't implement an API function we assume it is a null pointer)

However, if they do

struct display_buffer_descriptor desc;
desc.member = foo;
desc.member2 = foo2;

... then I think it's undefined behavior. Which is why I strongly favor the first version everywhere I can.

[danieldegrasse]

The only open issue here to solve is how to deal with GUI frameworks that have not been updated and do not initialize the "bufferable" flag- the flag would be an undefined value if the display buffer descriptor was declared on the stack.

One option here is to add a check to all display drivers that throws an error if the "bufferable" flag is set- as display drivers enable support for this, we would remove this check. Not sure how I feel about that- it would quickly "catch" GUI managers that were not initializing the flag, but once we add support for the "bufferable" flag in a display driver then it would be impossible to tell the difference between a GUI framework that recognized the flag and was setting it, and one that was leaving it uninitialized.

[danieldegrasse]

@danieldegrasse Yes, I think so.

Great. Again, I apologize for the churn here- I should have considered the tearing enable case a bit more during my initial review. @pdgendt, @faxe1008, @JarmouniA and @jfischer-no do you (or anyone else) have thoughts on this change?

Edit: comment where the proposed change is described: #79936 (comment)

[danieldegrasse]

... then I think it's undefined behavior. Which is why I strongly favor the first version everywhere I can

Yeah, I believe it is. I'll note that structures have been extended like this in the past- see an example here #77502

[Finomnis]

@danieldegrasse be aware that you want to wait before the first frame of the image, not after the last, in your TE case. Otherwise the GUI needs to render the frame after the TE and the data will come later than desired.

[danieldegrasse]

@danieldegrasse be aware that you want to wait before the first frame of the image, not after the last, in your TE case. Otherwise the GUI needs to render the frame after the TE and the data will come later than desired.

Sure, thanks for the clarification- I'm aware that display drivers would still need some kind of flag to indicate "the last framebuffer we received wasn't bufferable, so this next framebuffer should be sent at the TE interval"

[Finomnis]

Sure, thanks for the clarification- I'm aware that display drivers would still need some kind of flag to indicate "the last framebuffer we received wasn't bufferable, so this next framebuffer should be sent at the TE interval"

This is very confusing to read :D maybe we should not call it bufferable.

Maybe frame_incomplete, incomplete, not_last, something like that? What's your opinion, @danieldegrasse?

[danieldegrasse]

Maybe frame_incomplete, incomplete, not_last, something like that? What's your opinion, @danieldegrasse?

Sure, I like frame_incomplete. I'll edit the initial comment with that name

[Finomnis]

@danieldegrasse The only thing I dislike about this solution is that display_buffer_descriptor is also used for read, for which it makes no sense. So it feels like the wrong place, although it's the most elegant solution so far I think.

[Finomnis]

@danieldegrasse For the rework, could you take a look at lvgl_display_mono.c and explain it to me? Specifically:

• how display_blanking_on and display_blanking_off is dealt with here (misuse/abuse of blanking for what frame_incomplete is actually intended? imo this should be removed, as drivers can now easily use frame_incomplete to achieve the same internally)

• SCREEN_DOUBLE_BUFFER? It seems like that one is just completely random, it just writes the exact same data twice; why?? That's not what I know as 'double buffering' ... or am I missing something? If that is meant to write to a back buffer, then flip and write to a front buffer (to keep two buffers synced), then this will definitely break with the introduction of frame_incomplete ...

  Even the comment here indicates that it is a poor version of what we introduce with frame_incomplete ...

  /*
   * Turn on display blanking when using an EPD
   * display. This prevents updates and the associated
   * flicker if the screen is rendered in multiple
   * steps.
   */
  

  ... just that they didn't realize that this is something more generic, and instead hard-coded it to the specific usecase ...

  Should I keep it or remove it?

[Finomnis]

@danieldegrasse Rework at #81250.

[danieldegrasse]

... just that they didn't realize that this is something more generic, and instead hard-coded it to the specific usecase ...

Should I keep it or remove it?

I'm less familiar with e-ink displays, but I would guess that tearing is much more obvious there due to the slower refresh rate. It does seem like the change was intended to prevent flicker: zephyrproject-rtos/lvgl@0db5fca

SCREEN_DOUBLE_BUFFER? It seems like that one is just completely random, it just writes the exact same data twice; why?? That's not what I know as 'double buffering' ... or am I missing something? If that is meant to write to a back buffer, then flip and write to a front buffer (to keep two buffers synced), then this will definitely break with the introduction of frame_incomplete ...

I'll be honest, this one stumps me. It has been present in the LVGL port since the initial port to Zephyr: cbfcae7#diff-aff42d60cfd55aebd4f8e00f4415a92a6a40d300a0b673cd381d4a3ba6ed4377R28

The only driver that was using it was the SSD1673: 49ffca4#diff-9d44933c67e307bb75712984b2aa52ed1b07a4feb1edd6bd1674c0fe6cc6c1b0R339.

No drivers in tree currently use it:

rg SCREEN_INFO_DOUBLE_BUFFER .

./modules/lvgl/lvgl_display_mono.c
37:     if (data->cap.screen_info & SCREEN_INFO_DOUBLE_BUFFER) {

./include/zephyr/drivers/display.h
87:     SCREEN_INFO_DOUBLE_BUFFER       = BIT(3),

I suspect that the double buffer flag might have been related to this code that exists in the current version of the SSD1673 driver:

zephyr/drivers/display/ssd16xx.c

Line 484 in c50777a

err = ssd16xx_write_cmd(dev, SSD16XX_CMD_WRITE_RAM,

It seems that the driver will swap between two buffers when using a partial refresh, so two writes are needed to be sure the same framebuffer is in use for both the front and back buffer.

IMO, both of these should be removed (thank you for catching them), but we don't have to do it as part of this PR- we can log an issue against these and remove them in the future

[Finomnis]

Superseded by #81250.