On Tue, Apr 21, 2020 at 7:46 AM Pekka Paalanen ppaalanen@gmail.com wrote:
On Tue, 21 Apr 2020 14:37:41 +0200 Hans de Goede hdegoede@redhat.com wrote:
TL;DR: Yes there will be races, because of both userspace + the firmware having; and potentially using r/w access to the privacy-screen state. But in practice I expect these to not really be an issue. Important here is that userspace only commits the property in a transaction to commit if it actually intends to change the property so as to not needlessly create a situation where we might hit the race.
As for 1 vs 2 properties for this I guess that in preparation for potential devices where the state is locked, having a r/w sw-state + a ro hw-state property makes sense.
So I suggest that we replace the current "privacy-screen" property from Rajat's patch-set with 2 props named:
"privacy-screen-sw-state" (r/w) "privacy-screen-hw-state" (ro)
Where for current gen hardware the privacy-screen-hw-state is just a mirror of the sw-state.
Just to make sure I understand the semantics correctly:
- The "privacy-screen-hw-state" shall be read-only, and can be modified by: - Hardware (e.g. HW kill switch). - Firmware. - (Potentially) needs a notification/irq to the kernel when this changes (or may be kernel can read it only when userspace queries for it).
- The "privacy-screen-sw-state" shall be read-write, and can only be modified by user space. - If user space toggles it, the kernel will attempt to "request" the change to hardware. - Whether the request to hardware was successful or not, the "privacy-screen-sw-state" will always reflect the latest value userspace wrote. - If the request to hardware was successful, the "privacy-screen-hw-state" will also change (probably via a separate notification/irq from HW). - We expect the user space to write to "privacy-screen-sw-state" only if it really wants to toggle the value.
What is not clear to me is if any change to"privacy-screen-hw-state" shall be propagated to "privacy-screen-sw-state"? - If yes, then I think we are not solving any problems of single property. - If no, then why do we require userspace to write to sw state only if something has changed?
Also, it seems to me that in my current patchset, the property I have already behaves like "privacy-screen-sw-state". Do I just need to rename it?
Thanks,
Rajat
Hi,
this sounds like a good plan to me, assuming the kernel writes only to the ro property and never to the r/w property.
I understand that as long as firmware hotkeys will toggle actual state, there is no design that could work reliably if userspace will always commit all KMS state even when it is not necessary. But not committing KMS state unless it is actually necessary is really For implementing the "privacy-screen-sw-state".a new requirement on userspace, so that needs to be documented before it's too late.
It's not enough to document "don't set it unless you want to overwrite/change it" for privacy screen properties. It needs to be documented as a general rule that userspace must follow with *unknown* properties as well. "Do not restore unrecognized properties unless the kernel KMS state might be incorrect compared to what you used to have."
This means that with a display server that does not understand privacy screen properties, the end user will lose the privacy screen state on every VT-switch back to the display server.
However, if we had a way to query the kernel for the default state to reset unknown properties to, the kernel implementation could return the current value of the privacy screen property instead of "off" to not lose the firmware state. Assuming firmware hotkeys exist, but if they don't then return just "off". The point is that the kernel who knows all the properties makes the decision what a sane reset value is. Userspace can always override the reset value for the properties it recognizes.
Thanks,For implementing the "privacy-screen-sw-state". pq