RFC: Drm-connector properties managed by another driver / privacy screen support

[Hans de Goede]

Hi All,

Somewhat long mail, so I've divided it into "chapters", sorry.

1. Back ground info
-------------------

First some quick background, some recent Lenovo models have
a builtin privacy screen which can be turned on/off in software
and the kernel recently has gotten support for this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad291272d52e0a40a06157a3d9ba17

We have been looking into adding support for this to GNOME,
but the userspace API added by the above commit is very
Thinkpad specific, and we would rather not rely on an
userspace API which is specific to 1 series of laptops.

When we started discussing this I had already seen some versions
of Rajat's "drm/i915 Support for integrated privacy screen" series:

https://patchwork.freedesktop.org/series/74650/

Which adds support for integrated privacy screens
as a drm-connector property. Anyone who has been involved
in the backlight brightness control saga we have with
the sysfs backlight class instantly knows/feels that
this is the right way to handle this.

So now we want to export the Thinkpad lcdshadow
attribute as a drm-connector property.


2. Problem + Possible solutions
-------------------------------

The problem is that the code to get/set the lcdshadow
setting and also the code to listen for firmware (hardcoded
hotkeys) triggered state changes all lives inside the thinkpad_acpi
driver; and to export the lcdshadow setting as a drm property
we need to access that code (and it is too big to just copy
over).

One thing which makes this trickier is that all properties must
be attached to the connector before it is registered, we cannot
add it at a later time.

I see 3 possible solutions here:

i. Export some symbols from thinkpad_acpi and directly call these
from drm_connector_create_standard_properties and other
drm_connector functions if the thinkpad_acpi module is enabled.
Note this should be done in the core drm_connector functions since
the GPU might be one of i915 / amdgpu / nouveau. I believe that
it is clear that this solution is not very elegant.

A variant of this would be to have a separate helper module
(probaly a few static inlines in a .h) which exports some hooks for
i915 / amdgpu / nouveau to call this way we at least keep the
ugliness out of the core and keep the module-dep on thinkpad_acpi
limited to the i915 / amdgpu / nouveau modules. This might
actually not be too bad, except that currently the thinkpad_acpi
module refuses to load on non thinkpads...


ii. Currently the "privacy-screen" property added by Rajat's
patch-set is an enum with 2 possible values:
"Enabled"
"Disabled"

We could add a third value "Not Available", which would be the
default and then for internal panels always add the property
so that we avoid the problem that detecting if the laptop has
an internal privacy screen needs to be done before the connector
is registered. Then we can add some hooks which allow an
lcdshadow-driver to register itself against a connector later
(which is non trivial wrt probe order, but lets ignore that for now).


iii. We can add a generic framework to allow drivers outside
of the drm-subsys to register something drm_property(ish) specifying
a dev_name() and connector-name to which to attach the property
when that connector gets created on that device.

This requires the driver registering this property to be fully
loaded before the connector gets registered.


3. Picking a solution
---------------------

i. is just ugly, full stop, but also very KISS which still makes
it somewhat appealing. Also i. does not scale if we get other
vendor specific interfaces for interacting with these
privacy screens.


ii. is ugly from an userspace API pov, if there is no
"privacy-screen" then we really should not have the property at
all rather then setting it to "Not Available". Still it might be
an acceptable compromise I guess


iii. is/was the one I liked, until I started looking at the
drm_connector code and saw that all properties must be attached
before registering the connector, bummer. Then this morning I
remembered that the i915 driver has a similar issue with the
gvt stuff / the kvmgt module. The kvmgt module must be loaded
and fully initialized before the i915 driver loads. This has
been solved with module softdeps.

I think that we can do the same here having either the
i915+nouveau+amdgpu drivers; or the drm-core have a softdep on
thinkpad_acpi so that it can register the lcdshadow property
with the to-be-written framework for externally managed props
before the internal panels drm-connector gets created.

This also allows the thinkpad_acpi module_init function to
return -ENODEV on non Thinkpad devices, so that it will
not take up memory, where as with i. we would always need to
have it in memory.

I'm currently leaning towards iii. combined with using
MODULE_SOFTDEP("pre: thinkpad_acpi") to make sure the thinkpad_acpi
driver can do its thing before the internal display connector gets
created.

Daniel (and others) does this (iii. + softdeps) sound like
something which might be acceptable (I know it will also
depend on the resulting code implementing this idea) ?

Any other ideas / suggestions are appreciated here.

Regards,

Hans