[RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Hans de Goede]

Hi,

On 5/12/20 9:49 AM, Pekka Paalanen wrote:
> On Mon, 11 May 2020 19:47:24 +0200
> Hans de Goede <hdegoede at redhat.com> wrote:
> 
>> From: Rajat Jain <rajatja at google.com>
>>
>> Add support for generic electronic privacy screen properties, that
>> can be added by systems that have an integrated EPS.
>>
>> Changes in v2 (Hans de Goede)
>> - Create 2 properties, "privacy-screen sw-state" and
>>    "privacy-screen hw-state", to deal with devices where the OS might be
>>    locked out of making state changes
>> - Write kerneldoc explaining how the 2 properties work together, what
>>    happens when changes to the state are made outside of the DRM code's
>>    control, etc.
>>
>> Signed-off-by: Rajat Jain <rajatja at google.com>
>> Co-authored-by: Hans de Goede <hdegoede at redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>   Documentation/gpu/drm-kms.rst     |   2 +
>>   drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
>>   drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       |  50 +++++++++++++++
>>   4 files changed, 158 insertions(+)
> 
> ...
> 
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 644f0ad10671..01360edc2376 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>    *	can also expose this property to external outputs, in which case they
>>    *	must support "None", which should be the default (since external screens
>>    *	have a built-in scaler).
>> + *
>> + * privacy-screen sw-state, privacy-screen hw-state:
>> + *	These 2 optional properties can be used to query the state of the
>> + *	electronic privacy screen that is available on some displays; and in
>> + *	some cases also control the state. If a driver implements these
>> + *	properties then both properties must be present.
>> + *
>> + *	"privacy-screen hw-state" is read-only and reflects the actual state
>> + *	of the privacy-screen, possible values: "Enabled", "Disabled,
>> + *	"Enabled, locked", "Disabled, locked". The locked states indicate
>> + *	that the state cannot be changed through the DRM API. E.g. there
>> + *	might be devices where the firmware-setup options, or a hardware
>> + *	slider-switch, offer always on / off modes.
>> + *
>> + *	"privacy-screen sw-state" can be set to change the privacy-screen state
>> + *	when not locked. In this case the driver must update the hw-state
>> + *	property to reflect the new state on completion of the commit of the
>> + *	sw-state property. Setting the sw-state property when the hw-state is
>> + *	locked must be interpreted by the driver as a request to change the
>> + *	state to the set state when the hw-state becomes unlocked. E.g. if
>> + *	"privacy-screen hw-state" is "Enabled, locked" and the sw-state
>> + *	gets set to "Disabled" followed by the user unlocking the state by
>> + *	changing the slider-switch position, then the driver must set the
>> + *	state to "Disabled" upon receiving the unlock event.
>> + *
>> + *	In some cases the privacy-screen state might change outside of control
>> + *	of the DRM code. E.g. there might be a firmware handled hotkey which
>> + *	toggles the state, or the state might be changed through another
> 
> Hi,
> 
> in the above three lines, I'd use the term "hardware state" instead of
> just "state" to be explicit. Or should it be "physical state" since
> "hardware state" might be confused with "hw-state" property state?

Maybe "actual state"? That is what is used a few lines higher:

'"privacy-screen hw-state" is read-only and reflects the actual state'

And you use it yourself to describe what we want below:

> I don't mind as long as it's unambiguous and distinguishes explicitly
> between actual and the two property states.

So since you and I both naturally described it as "actual state" without
thinking too much what we where writing at the time (I guess that applies
to your use too), "actual state" seems a good fit ?


>> + *	userspace API such as writing /proc/acpi/ibm/lcdshadow. In this case
>> + *	the driver must update both the hw-state and the sw-state to reflect
>> + *	the new value, overwriting any pending state requests in the sw-state.
>> + *
>> + *	Note that the ability for the state to change outside of control of
>> + *	the DRM master process means that userspace must not cache the value
>> + *	of the sw-state. Ccaching the sw-state value and including it in later
> 
> Extra 'c' in "Caching".

Ack, will fix.

>> + *	atomic commits may lead to overriding a state change done through e.g.
>> + *	a firmware handled hotkey. Therefor userspace must not include the
>> + *	privacy-screen sw-state in an atomic commit unless it wants to change
>> + *	its value.
>>    */
> 
> This documentation and intended behaviour looks perfect to me.

Great I'm glad you like it.

> If you
> can record an R-b just for the doc, please have:
> 
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.com>
> 
> You can also downgrade that to Acked-by for the whole patch from my
> behalf.

I don't know of a way to add partial Reviewed-by tags, so unless
someone educates me on that I will change it to an Acked-by for the next
version.

Regards,

Hans