-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
samples: drivers: display: Support line alignment constrained displays #83167
samples: drivers: display: Support line alignment constrained displays #83167
Conversation
I am not sure I understand the proposed change. Isn't this basically implementing a workaround in the sample code for something that should be addressed by the driver (ex. it should fail if we're trying to draw an incomplete line?). We don't want all user code to have to deal with similar corner cases and to have to add tweaks to their UI code as they can't know a priori about all the limitations some classes of display might have, right? |
The draws do currently (correctly) fail, that's where the error log messages come from in the linked issue. This PR makes the sample functional by changing it to only draw full lines, but you are correct that an equally valid implementation would be for the sample app to error out early with a nice error message that it requires the As long as this capability exists in the display driver API, I don't think it's wrong to require that code that wants to work on all displays handles it (either by erroring out or by dealing with it by changing their implementation). Whether this should have existed in the first place vs. having the driver implement an internal framebuffer to fill the gaps between the display API and the hardware capabilities is a different question. |
It should also be mentioned that higher level graphics APIs like LVGL deal with this themselves -- LVGL invalidates the entire line whenever you draw anything on it, and correctly passes full lines from its framebuffer to the underlying display driver. This is done by checking the capability in zephyr/modules/lvgl/lvgl_display_mono.c Lines 100 to 102 in f530571
So an (optional) internal framebuffer in the ls0xx driver would make the raw Zephyr display API nicer to use on this display, but I'm not sure if it makes sense, since the the concept of "damage outside the drawn pixels" is something higher level graphics APIs generally deal with anyway. |
I think we do actually want user code to deal with corner cases like this- typically abstraction is best, but for displays buffer alignment and size requirements can be really important from a performance perspective. Generally speaking, if the display API is used in a way that does not follow the hardware design you need some form of intermediate buffer, and end up with poor performance. A good example of this is the eLCDIF- this display engine requires a full framebuffer, but the Zephyr display driver supports partial writes by storing a framebuffer within the driver itself, and copying data into that framebuffer. However, these partial writes drop performance by 5x or so in the LVGL samples (IIRC) Realistically, I would not expect most production applications to choose to use the driver framebuffer with the eLCDIF. They would instead render a full framebuffer, and pass that to the display driver. When using a framework like LVGL, this vastly improves performance versus partial renders. So then the broader question is whose responsibility is it to handle each display engine's alignment and buffer size requirements- the display driver, or the API consumer? I would argue it is actually better for the API consumer to handle these differences- usually we want to maximize abstraction, but the performance hit to doing that here is often quite high. So if we do choose to have the API consumer handle differences, we probably want the display driver sample to adjust its behavior based on the We also could require display drivers to support arbitrarily sized framebuffers, with the understanding that performance may suffer- and rely on the API consumer to check the |
@kartben based on #83167 (comment) it sounds like this PR should be ok. Any further thoughts on it? |
9d295bf
to
9070030
Compare
@kartben did you intend to approve as well as part of your force push yesterday? CI is green, FWIW. |
No, sorry, force push was to try and go through some PRs affected by a GitHub outage.
If that is the case indeed, the difference in behavior should actually be explicited in the sample (or at a very minimum as a comment in the code) as a way to make it really clear that yes, indeed, a client application may have to make local changes/optimizations to deal with the sometimes limited capabilities of the display driver. For completeness, I do realize that the README is already probably not perfect, as it's referring to "colors", and RGB order, which don't really make a lot of sense for monochrome displays... but I still would like at least this PR to not add to the confusion Hope this helps... thanks! |
eda1426
9070030
to
eda1426
Compare
Made an attempt at adding some info to the README about how the sample will present itself when faced with constrained displays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great actually, thanks @asmellby! -- minor final request for change
The sample app draws rectangles in the corner of the display. On devices with the SCREEN_INFO_X_ALIGNMENT_WIDTH capability, each draw needs to transmit a complete line. Make the sample work on such displays (e.g. the `ls0xx` driver) by making the rectangles take up the full width of the display. Co-authored-by: Benjamin Cabé <[email protected]> Signed-off-by: Aksel Skauge Mellbye <[email protected]>
c8afad1
to
27e5afa
Compare
The sample app draws rectangles in the corner of the display. On devices with the
SCREEN_INFO_X_ALIGNMENT_WIDTH
capability, each draw needs to transmit a complete line. Make the sample work on such displays (e.g. thels0xx
driver) by making the rectangles take up the full width of the display.Fixes #80463