All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Sonny.Quintanilla@dell.com,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Mario Limonciello <mario.limonciello@dell.com>,
	David Airlie <airlied@linux.ie>,
	dri-devel@lists.freedesktop.org,
	Jared Dominguez <jaredz@redhat.com>,
	Rajat Jain <rajatja@google.com>,
	Mark Pearson <mpearson@lenovo.com>
Subject: Re: [RFC v2] drm/connector: Add support for privacy-screen properties (v2)
Date: Tue, 12 May 2020 10:02:12 +0200	[thread overview]
Message-ID: <d6421971-cff2-9aa7-30c5-9e6d9bea3f62@redhat.com> (raw)
In-Reply-To: <20200512104912.419270ff@eldfell.localdomain>

Hi,

On 5/12/20 9:49 AM, Pekka Paalanen wrote:
> On Mon, 11 May 2020 19:47:24 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> From: Rajat Jain <rajatja@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@google.com>
>> Co-authored-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@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@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2020-05-12  8:02 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-11 17:47 [RFC v2 0/1] drm/connector: Add support for privacy-screen properties Hans de Goede
2020-05-11 17:47 ` [RFC v2] drm/connector: Add support for privacy-screen properties (v2) Hans de Goede
2020-05-11 20:04   ` Rajat Jain
2020-05-12  8:12     ` Hans de Goede
2020-05-12  7:49   ` Pekka Paalanen
2020-05-12  8:02     ` Hans de Goede [this message]
2020-05-12 14:14       ` Pekka Paalanen
2020-05-12 20:44   ` Mario.Limonciello
2020-05-12 21:30     ` Hans de Goede
2020-05-11 19:55 ` [RFC v2 0/1] drm/connector: Add support for privacy-screen properties Rajat Jain
2020-05-12  8:18   ` Hans de Goede
2020-05-12 14:20     ` Pekka Paalanen
2020-05-12 16:09       ` Hans de Goede
2020-05-12 17:38         ` Rajat Jain
2020-05-13  7:49         ` Pekka Paalanen
2020-05-13 18:28           ` Rajat Jain
2020-05-14  8:11             ` Pekka Paalanen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d6421971-cff2-9aa7-30c5-9e6d9bea3f62@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=Sonny.Quintanilla@dell.com \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jaredz@redhat.com \
    --cc=mario.limonciello@dell.com \
    --cc=mpearson@lenovo.com \
    --cc=ppaalanen@gmail.com \
    --cc=rajatja@google.com \
    --cc=tzimmermann@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.