Screen syscall/HIL improvements #3047

dcz-self · 2022-05-25T17:51:31Z

Pull Request Overview

This pull request changes the Screen syscalls and HIL to incorporate more changes that seem rather simple.

It's another step on the way to #3011, and includes feedback from #3035 . Other notable changes:

improved docs
removed power on tracking in the last commit - it doesn't add much value, and it prevents set_power call from succeeding at startup

Testing Strategy

This pull request was tested on LPM013M126 driver, out of tree: https://framagit.org/jazda/tock/-/commit/18e7b9bd0ee94d3c3b6b44c2f6771d12457e1ed2 (points to last commit), this does not exercise Setup interface.

TODO or Help Wanted

This pull request still needs testing of the Setup interface, because I don't have the device that uses it. I'm also not ready yet to extend my own out of tree driver with this functionality just to test it.

There are some notes inline where I'd like to get feedback.

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

dcz-self · 2022-05-25T17:52:39Z

capsules/src/screen.rs

@@ -383,16 +284,12 @@ impl<'a> Screen<'a> {
    }

    fn schedule_callback(&self, data1: usize, data2: usize, data3: usize) {
-        if !self.screen_ready.get() {
-            self.screen_ready.set(true);


What was the point of this? This callback would have not been forwarded anywhere. I don't get it, and don't want to break this by accident.

Whenever the underlying driver returns a command_complete the screen driver might have to execute a queued command. This is done by the run_next_command function. This schedules a callback for the previous action and then runs the next one.

The lines that you are asking about are the way in which schedule_callback differentiates between the screen_is_ready and the command_complete notifications from the underlying driver.

I think this is working, because I haven't noticed any bad beaviour coming from the removal of those lines. On the other hand, the original seems wrong to me because it drops the command_complete, so I'm not 100% sure. Either way, upcalls get scheduled as expected, so I guess all is good.

dcz-self · 2022-05-25T17:53:00Z

@alexandruradovici I'd appreciate your feedback.

dcz-self · 2022-05-29T16:25:59Z

I tested this, and it works for me, so I removed the WIP.

alexandruradovici

It looks good overall. I was wondering if we could make this a single app driver, inspired by the model of the LSM driver (https://github.com/tock/tock/blob/master/capsules/src/lsm303agr.rs).

The first application that sends a request would own the display. As soon as this application finishes or is restarted, another application would be able to own the driver and send commands.

alexandruradovici · 2022-05-30T13:50:10Z

capsules/src/screen.rs

@@ -118,7 +110,6 @@ pub struct Screen<'a> {
    screen: &'a dyn hil::screen::Screen,
    screen_setup: Option<&'a dyn hil::screen::ScreenSetup>,
    apps: Grant<App, UpcallCount<1>, AllowRoCount<{ ro_allow::COUNT }>, AllowRwCount<0>>,
-    screen_ready: Cell<bool>,


The small screens that I used to need a significant power-up time, in the range of a few hundreds of milliseconds. Without a callback to notify applications about this, applications will continuously issue commands that will fail with BUSY until the initialization is done.

Right now, applications can issue a command which the driver will queue until it receives a screen_is_ready event.

The callback is still getting through according to the way I tested. It's just the flag that is not actually needed anywhere.

Which callback are you referring to?

The callback on screen_is_ready. All the others reach the application too.

I think we can drop it now as we have established the new way of working for power on, no?

Sorry, I don't know what way you mean.

capsules/src/screen.rs

doc/syscalls/90001_screen.md

dcz-self · 2022-05-30T14:23:54Z

I was wondering if we could make this a single app driver

I'm not against it, but I'd rather take smaller steps for now.

alexandruradovici · 2022-05-30T14:32:39Z

I was wondering if we could make this a single app driver

I'm not against it, but I'd rather take smaller steps for now.

I suggest (above) stating this in the driver's documentation, more precisely that the driver does accept commands from multiple without returning an actual error.

dcz-self · 2022-05-31T12:30:08Z

I've updated this proposal with a consideration regarding when OFF can be returned.

Along the way I noticed that some atomic application of configuration could be useful in the future, but I don't have hardware that needs it, so not going to try that.

I think syscall callbacks should be better documented. It seems that only one callback per client can be in flight, and the client must know which, because the callbacks don't carry any info to distinguish them.

dcz-self

Trying to un-Pending my comments

doc/syscalls/90001_screen.md

capsules/src/screen.rs

hudson-ayers · 2022-06-02T22:45:29Z

I've updated this proposal with a consideration regarding when OFF can be returned.

Along the way I noticed that some atomic application of configuration could be useful in the future, but I don't have hardware that needs it, so not going to try that.

I think syscall callbacks should be better documented. It seems that only one callback per client can be in flight, and the client must know which, because the callbacks don't carry any info to distinguish them.

Applications can attach data to upcalls which the kernel promises to return unmodified. This allows applications to distinguish distinct callbacks. This is documented here: https://github.com/tock/tock/blob/master/doc/reference/trd104-syscalls.md#42-subscribe-class-id-1

dcz-self · 2022-06-09T13:39:10Z

Is there anything left here that should get improved/redesigned to get a preliminary approval?

hudson-ayers · 2022-06-10T05:21:17Z

The contents look good to me, but I have been deferring to @alexandruradovici as the primary reviewer who actually uses the screen HIL

alexandruradovici · 2022-06-10T08:41:34Z

kernel/src/hil/screen.rs

+pub enum UpdateType {
+    /// All settings have been applied
+    Complete,
+    /// Not all settings have been applied
+    Partial,
+}
+


I am not sure that I fully understand the purpose of this. Can you provide an example?

I addressed this in another comment.

alexandruradovici · 2022-06-10T08:42:27Z

Everything looks good to me so far, we just need to clear out the screen_is_ready event and I think this is good to go.

alexandruradovici · 2022-06-10T16:57:39Z

kernel/src/hil/screen.rs

+    /// Some screens need some time to start, this function is called when the screen is ready.
+    fn screen_is_ready(&self, readiness: UpdateType);


Suggested change

/// Some screens need some time to start, this function is called when the screen is ready.

fn screen_is_ready(&self, readiness: UpdateType);

I think you are right and we can delete this. Applications should issue a power command that should initialize the screen. Any command sent before that should fail with OFF.

I do not fully understand the UpdateType.

UpdateType is a consequence of allowing atomic requests. Let's say that the device is able to turn on with a given resolution and color space already configured (for example because the "reset" call has space for them, and setting them after power up takes extra time). The user will want them to be configured as soon as the "ready" signal arrives.

However, we can't unconditionally require this ability from all drivers in the HIL. Another device may initialize in the native resolution no matter what. Then "UpdateType" comes in, allowing the user to know which device was just started.

The first ("atomic") device successfully started and already set the resolution. It returns UptateType::Complete.

The second ("sequential") device started, but ignored the requested resolution. It returns UpdateType::Partial.

I included this atomic mode setting, because it's the same as setting brightness before startup, and presumably costs very little extra complexity.

I think I understand somehow your use case. My take in this is that the two things should not be mixed. Powering on the device should just do that and applications should not make any assumption upon resolution and color mode. As soon as the device powers on, applications should request the color mode and resolution, and if required, set them.

I think that adding UpdateType adds to much complexity. My suggestion is to keep only one single callback, command_complete and state that all commands will fail if the display is power off.

This is the same as setting brightness. You do not want to turn on your device and end up blinding the user for a split second because brightness is detached from power. Similarly, you do not want to have flicker because you're changing resolution or color mode.

Sometimes you can't get away with it, but I think it's a reasonable thing to have foreseen in an API.

I think that setting the brightness before power is a very basic thing and I'm going to have to be convinced to remove it, even if it adds complexity, because of how useful it is for user experience.

I do not exactly understand how the UpdateType can stop the flicker.

I am not convinced that power on and brightness should be mixed. In my experience with working with low end displays, powering them off means they might loose any configuration that was made. From this point of view, an application needs to assume that when powering down a display means it might loose any configurations. If an application wants to turn off the screen without loosing configuration, it should set the brightness to 0.

Moreover, low end displays are not configurable when powered down (they might actually be powered down).

My view on this is the following:

applications should consider that displays need to be powered on before they can be configured or used. Any command issues before that may fail and might not be reflected after power up

drivers should set the brightness of the screens to 0 after powering on (for the simple displays that I have used this means that they do not actually show anything)

applications should set a brightness above 0 whenever they need to show content to the user (this might mean that applications can draw, but the user does not see anything unless the brightness is set above 0)

I think we can skip UpdateType and atomic mode changes for this MR, for the sake of not holding up uncontroversial improvements. I'll update it in the coming days.

For an immediate explanation:

the application issues a set_brightness command, the driver memorizes it and will execute it after the power on command

the application sends a power on command, the driver atomically powers on the display at selected brightness

the application receives the upcall with UpdateType::Complete

OR

the application issues a set_brightness command, the driver memorizes it and will execute it after the power on command

the application sends a power on command, the driver sequentially powers on the display, and then attempts to set brightness but fails

the application receives the upcall with UpdateType::Incomplete

This makes things more clear now. The issue that I see is that if there are more parameters than brightness, the application has no idea which one failed and there is not much that it can do (maybe power cycle the display?).

Instead of q quick fix now, I'd rather find a more general approach that allows the application to take an action.

I already wrote about this: https://github.com/tock/tock/pull/3047/files#diff-14db7584389d3cf0f83131309a857eb9dc3726982e89eaf34486adde917a9785R34

I'm sorry, but I think your example needs to be more clear, I personally do not understand this from your code example.

Could you please use code that actually compiles, instead of functions like ignore_off which do no actually exist. I think it's important to have clear working examples. Of course, you can skip the registering of the callback function.

I've removed this entire train of thought from this proposal. We can consider it when it becomes needed.

The interface for read-only syscalls was needlessly complicated. Now, when the driver knows something due to HIL, the userspace gets unconditional access to it.

dcz-self · 2022-06-27T15:19:02Z

I've rebased this on master as the lpm* driver came in.

alexandruradovici

Remove the UpdateType enum and I think we can merge it.

alexandruradovici · 2022-06-27T18:34:21Z

kernel/src/hil/screen.rs

+/// Returned for combined operations
+pub enum UpdateType {
+    /// All settings have been applied
+    Complete,
+    /// Not all settings have been applied
+    Partial,
+}
+


I think you can delete this as it is not used anymore.

Thanks for noticing that. It's gone.

hudson-ayers

bors r+

@alexandruradovici thank you for all your time reviewing this and getting it to such a good spot!

bors · 2022-06-27T19:42:47Z

Build succeeded:

github-actions bot added the HIL This affects a Tock HIL interface. label May 25, 2022

dcz-self commented May 25, 2022

View reviewed changes

dcz-self mentioned this pull request May 26, 2022

RFC: Display HIL #3011

Closed

2 tasks

dcz-self changed the title ~~WIP: Screen syscall/HIL improvements~~ Screen syscall/HIL improvements May 29, 2022

alexandruradovici requested changes May 30, 2022

View reviewed changes

dcz-self force-pushed the dispsys branch 2 times, most recently from cd2a435 to e10f02b Compare May 31, 2022 12:23

dcz-self force-pushed the dispsys branch 2 times, most recently from 19b7cac to 1482b42 Compare May 31, 2022 12:56

dcz-self commented Jun 1, 2022

View reviewed changes

doc/syscalls/90001_screen.md Outdated Show resolved Hide resolved

doc/syscalls/90001_screen.md Outdated Show resolved Hide resolved

capsules/src/screen.rs Show resolved Hide resolved

alistair23 previously approved these changes Jun 10, 2022

View reviewed changes

alexandruradovici reviewed Jun 10, 2022

View reviewed changes

alexandruradovici mentioned this pull request Jun 10, 2022

Unify text and graphics display and TRD #3055

Draft

2 tasks

alexandruradovici requested changes Jun 10, 2022

View reviewed changes

dcz added 2 commits June 27, 2022 15:08

Screen syscalls: use synchronous syscalls

0b7e1c8

The interface for read-only syscalls was needlessly complicated. Now, when the driver knows something due to HIL, the userspace gets unconditional access to it.

screen: Combine invert on/off into one SetInvert

e81d1e5

dcz-self dismissed alistair23’s stale review via 8b22c90 June 27, 2022 15:17

dcz-self force-pushed the dispsys branch from 1482b42 to 8b22c90 Compare June 27, 2022 15:17

alexandruradovici requested changes Jun 27, 2022

View reviewed changes

screen syscalls: Stop tracking power state

9f7cf87

dcz-self force-pushed the dispsys branch from 8b22c90 to 9f7cf87 Compare June 27, 2022 18:39

alexandruradovici approved these changes Jun 27, 2022

View reviewed changes

hudson-ayers approved these changes Jun 27, 2022

View reviewed changes

bors bot merged commit fd65512 into tock:master Jun 27, 2022

dcz-self mentioned this pull request Jul 9, 2022

Snync Screen syscalls #3035

Closed

ppannuto mentioned this pull request Sep 1, 2022

Release 2.1 Breaking Changes and Release Notes #3141

Closed

		/// Some screens need some time to start, this function is called when the screen is ready.
		fn screen_is_ready(&self, readiness: UpdateType);

Screen syscall/HIL improvements #3047

Screen syscall/HIL improvements #3047

Conversation

dcz-self commented May 25, 2022 • edited Loading

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcz-self commented May 25, 2022

dcz-self commented May 29, 2022

alexandruradovici left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcz-self commented May 30, 2022

alexandruradovici commented May 30, 2022

dcz-self commented May 31, 2022

dcz-self left a comment

Choose a reason for hiding this comment

hudson-ayers commented Jun 2, 2022

dcz-self commented Jun 9, 2022

hudson-ayers commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandruradovici commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcz-self commented Jun 27, 2022

alexandruradovici left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudson-ayers left a comment

Choose a reason for hiding this comment

bors bot commented Jun 27, 2022

dcz-self commented May 25, 2022 •

edited

Loading