[PATCH 05/10] drm/connector: Add a drm_connector privacy-screen helper functions (v2)
Add 2 drm_connector privacy-screen helper functions:
1. drm_connector_attach_privacy_screen_provider(), this function creates
and attaches the standard privacy-screen properties and registers a
generic notifier for generating sysfs-connector-status-events on external
changes to the privacy-screen status.
2. drm_connector_update_privacy_screen(), update the privacy-screen's
sw_state if the connector has a privacy-screen.
Changes in v2:
- Do not update connector->state->privacy_screen_sw_state on
atomic-commits.
- Change drm_connector_update_privacy_screen() to take drm_connector_state
as argument instead of a full drm_atomic_state. This allows the helper
to be called by drivers when they are enabling crtcs/encoders/connectors.
Reviewed-by: Emil Velikov
Reviewed-by: Lyude Paul
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_connector.c | 102
include/drm/drm_connector.h | 11
2 files changed, 113 insertions(+)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b2f1f1b1bfb4..00cf3b6135f6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
@@ -462,6 +463,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
DRM_CONNECTOR_REGISTERED))
drm_connector_unregister(connector);
+ if (connector->privacy_screen) {
+ drm_privacy_screen_put(connector->privacy_screen);
+ connector->privacy_screen = NULL;
+ }
+
if (connector->tile_group) {
drm_mode_put_tile_group(dev, connector->tile_group);
connector->tile_group = NULL;
@@ -543,6 +549,10 @@ int drm_connector_register(struct drm_connector *connector)
/* Let userspace know we have a new connector */
drm_sysfs_hotplug_event(connector->dev);
+ if (connector->privacy_screen)
+ drm_privacy_screen_register_notifier(connector->privacy_screen,
+ &connector->privacy_screen_notifier);
+
mutex_lock(&connector_list_lock);
list_add_tail(&connector->global_connector_list_entry, &connector_list);
mutex_unlock(&connector_list_lock);
@@ -578,6 +588,11 @@ void drm_connector_unregister(struct drm_connector
*connector)
list_del_init(&connector->global_connector_list_entry);
mutex_unlock(&connector_list_lock);
+ if (connector->privacy_screen)
+ drm_privacy_screen_unregister_notifier(
+ connector->privacy_screen,
+ &connector->privacy_screen_notifier);
+
if (connector->funcs->early_unregister)
connector->funcs->early_unregister(connector);
@@ -2442,6 +2457,93 @@ drm_connector_attach_privacy_screen_properties(struct
drm_connector *connector)
}
EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
+static void drm_connector_update_privacy_screen_properties(
+ struct drm_connector *connector, bool set_sw_state)
+{
+ enum drm_privacy_screen_status sw_state, hw_state;
+
+ drm_privacy_screen_get_state(connector->privacy_screen,
+&sw_state, &hw_state);
+
+ if (set_sw_state)
+ connector->state->privacy_screen_sw_state = sw_state;
+ drm_object_property_set_value(&connector->base,
+ connector->privacy_screen_hw_state_property, hw_state);
+}
+
+static int drm_connector_privacy_screen_notifier(
+ struct notifier_block *nb, unsigned long action, void *data)
+{
+ struct drm_connector *connector =
+ container_of(nb, struct drm_connector, privacy_screen_notifier);
+ struct drm_device *dev = connector->dev;
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+ drm_connector_update_privacy_screen_properties(connector, true);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+ drm_sysfs_connector_status_event(connector,
+ connector->privacy_screen_sw_state_property);
+ drm_sysfs_connector_status_event(connector,
+ connector->privacy_screen_hw_state_property);
+
+ return NOTIFY_DONE;
+}
+
+/**
+ * drm_connector_attach_privacy_screen_provider - attach a privacy-screen to
+ *the connector
+ * @connector: connector to attach the privacy-screen to
+ * @priv: drm_privacy_screen to attach
+ *
+ * Create and attach the standard privacy-screen properties and register
+ * a generic notifier for generating sysfs-connector-status-events
+ * on external changes to the privacy-screen status.
+ * This function takes ownership of the passed in drm_privacy_screen and will
+ * call drm_privacy_screen_put() on it when the connector is destroyed.
+ */
+void drm_connector_attach_privacy_screen
Re: [PATCH 05/10] drm/connector: Add a drm_connector privacy-screen helper functions (v2)
On Sat, Oct 02, 2021 at 06:36:13PM +0200, Hans de Goede wrote:
> Add 2 drm_connector privacy-screen helper functions:
>
> 1. drm_connector_attach_privacy_screen_provider(), this function creates
> and attaches the standard privacy-screen properties and registers a
> generic notifier for generating sysfs-connector-status-events on external
> changes to the privacy-screen status.
>
> 2. drm_connector_update_privacy_screen(), update the privacy-screen's
> sw_state if the connector has a privacy-screen.
>
> Changes in v2:
> - Do not update connector->state->privacy_screen_sw_state on
> atomic-commits.
> - Change drm_connector_update_privacy_screen() to take drm_connector_state
> as argument instead of a full drm_atomic_state. This allows the helper
> to be called by drivers when they are enabling crtcs/encoders/connectors.
>
> Reviewed-by: Emil Velikov
> Reviewed-by: Lyude Paul
> Signed-off-by: Hans de Goede
> ---
> drivers/gpu/drm/drm_connector.c | 102
> include/drm/drm_connector.h | 11
> 2 files changed, 113 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b2f1f1b1bfb4..00cf3b6135f6 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -462,6 +463,11 @@ void drm_connector_cleanup(struct drm_connector
> *connector)
> DRM_CONNECTOR_REGISTERED))
> drm_connector_unregister(connector);
>
> + if (connector->privacy_screen) {
> + drm_privacy_screen_put(connector->privacy_screen);
> + connector->privacy_screen = NULL;
> + }
> +
> if (connector->tile_group) {
> drm_mode_put_tile_group(dev, connector->tile_group);
> connector->tile_group = NULL;
> @@ -543,6 +549,10 @@ int drm_connector_register(struct drm_connector
> *connector)
> /* Let userspace know we have a new connector */
> drm_sysfs_hotplug_event(connector->dev);
>
> + if (connector->privacy_screen)
> + drm_privacy_screen_register_notifier(connector->privacy_screen,
> +&connector->privacy_screen_notifier);
> +
> mutex_lock(&connector_list_lock);
> list_add_tail(&connector->global_connector_list_entry, &connector_list);
> mutex_unlock(&connector_list_lock);
> @@ -578,6 +588,11 @@ void drm_connector_unregister(struct drm_connector
> *connector)
> list_del_init(&connector->global_connector_list_entry);
> mutex_unlock(&connector_list_lock);
>
> + if (connector->privacy_screen)
> + drm_privacy_screen_unregister_notifier(
> + connector->privacy_screen,
> + &connector->privacy_screen_notifier);
> +
> if (connector->funcs->early_unregister)
> connector->funcs->early_unregister(connector);
>
> @@ -2442,6 +2457,93 @@ drm_connector_attach_privacy_screen_properties(struct
> drm_connector *connector)
> }
> EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
>
> +static void drm_connector_update_privacy_screen_properties(
> + struct drm_connector *connector, bool set_sw_state)
> +{
> + enum drm_privacy_screen_status sw_state, hw_state;
> +
> + drm_privacy_screen_get_state(connector->privacy_screen,
> + &sw_state, &hw_state);
> +
> + if (set_sw_state)
> + connector->state->privacy_screen_sw_state = sw_state;
> + drm_object_property_set_value(&connector->base,
> + connector->privacy_screen_hw_state_property, hw_state);
> +}
> +
> +static int drm_connector_privacy_screen_notifier(
> + struct notifier_block *nb, unsigned long action, void *data)
> +{
> + struct drm_connector *connector =
> + container_of(nb, struct drm_connector, privacy_screen_notifier);
> + struct drm_device *dev = connector->dev;
> +
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> + drm_connector_update_privacy_screen_properties(connector, true);
This thing still seems pretty unatomic in essence. The standard rule
is that non-immutable properties do not change under external
influence. So if userspace is unaware of the change then it could
just flip the state back to where it was previously. Ie. seems racy
at least which could in theory lead to some funny ping pong in the
state.
To go proper atomic route here the state of the prop should be
fully cotrolled by userspace. Is that not possible for some reason?
> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> + drm_sysfs_connector_status_event(connector,
> + connector->privacy_screen_sw_state_property);
> + drm_sysfs_connector_status_event(connector,
> + connector->pr
Re: [PATCH 05/10] drm/connector: Add a drm_connector privacy-screen helper functions (v2)
Hi,
On 10/4/21 5:22 PM, Ville Syrjälä wrote:
> On Sat, Oct 02, 2021 at 06:36:13PM +0200, Hans de Goede wrote:
>> Add 2 drm_connector privacy-screen helper functions:
>>
>> 1. drm_connector_attach_privacy_screen_provider(), this function creates
>> and attaches the standard privacy-screen properties and registers a
>> generic notifier for generating sysfs-connector-status-events on external
>> changes to the privacy-screen status.
>>
>> 2. drm_connector_update_privacy_screen(), update the privacy-screen's
>> sw_state if the connector has a privacy-screen.
>>
>> Changes in v2:
>> - Do not update connector->state->privacy_screen_sw_state on
>> atomic-commits.
>> - Change drm_connector_update_privacy_screen() to take drm_connector_state
>> as argument instead of a full drm_atomic_state. This allows the helper
>> to be called by drivers when they are enabling crtcs/encoders/connectors.
>>
>> Reviewed-by: Emil Velikov
>> Reviewed-by: Lyude Paul
>> Signed-off-by: Hans de Goede
>> ---
>> drivers/gpu/drm/drm_connector.c | 102
>> include/drm/drm_connector.h | 11
>> 2 files changed, 113 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c
>> index b2f1f1b1bfb4..00cf3b6135f6 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -28,6 +28,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>>
>> #include
>> @@ -462,6 +463,11 @@ void drm_connector_cleanup(struct drm_connector
>> *connector)
>> DRM_CONNECTOR_REGISTERED))
>> drm_connector_unregister(connector);
>>
>> +if (connector->privacy_screen) {
>> +drm_privacy_screen_put(connector->privacy_screen);
>> +connector->privacy_screen = NULL;
>> +}
>> +
>> if (connector->tile_group) {
>> drm_mode_put_tile_group(dev, connector->tile_group);
>> connector->tile_group = NULL;
>> @@ -543,6 +549,10 @@ int drm_connector_register(struct drm_connector
>> *connector)
>> /* Let userspace know we have a new connector */
>> drm_sysfs_hotplug_event(connector->dev);
>>
>> +if (connector->privacy_screen)
>> +drm_privacy_screen_register_notifier(connector->privacy_screen,
>> + &connector->privacy_screen_notifier);
>> +
>> mutex_lock(&connector_list_lock);
>> list_add_tail(&connector->global_connector_list_entry, &connector_list);
>> mutex_unlock(&connector_list_lock);
>> @@ -578,6 +588,11 @@ void drm_connector_unregister(struct drm_connector
>> *connector)
>> list_del_init(&connector->global_connector_list_entry);
>> mutex_unlock(&connector_list_lock);
>>
>> +if (connector->privacy_screen)
>> +drm_privacy_screen_unregister_notifier(
>> +connector->privacy_screen,
>> +&connector->privacy_screen_notifier);
>> +
>> if (connector->funcs->early_unregister)
>> connector->funcs->early_unregister(connector);
>>
>> @@ -2442,6 +2457,93 @@ drm_connector_attach_privacy_screen_properties(struct
>> drm_connector *connector)
>> }
>> EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
>>
>> +static void drm_connector_update_privacy_screen_properties(
>> +struct drm_connector *connector, bool set_sw_state)
>> +{
>> +enum drm_privacy_screen_status sw_state, hw_state;
>> +
>> +drm_privacy_screen_get_state(connector->privacy_screen,
>> + &sw_state, &hw_state);
>> +
>> +if (set_sw_state)
>> +connector->state->privacy_screen_sw_state = sw_state;
>> +drm_object_property_set_value(&connector->base,
>> +connector->privacy_screen_hw_state_property, hw_state);
>> +}
>> +
>> +static int drm_connector_privacy_screen_notifier(
>> +struct notifier_block *nb, unsigned long action, void *data)
>> +{
>> +struct drm_connector *connector =
>> +container_of(nb, struct drm_connector, privacy_screen_notifier);
>> +struct drm_device *dev = connector->dev;
>> +
>> +drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +drm_connector_update_privacy_screen_properties(connector, true);
>
> This thing still seems pretty unatomic in essence. The standard rule
> is that non-immutable properties do not change under external
> influence. So if userspace is unaware of the change then it could
> just flip the state back to where it was previously. Ie. seems racy
> at least which could in theory lead to some funny ping pong in the
> state.
>
> To go proper atomic route here the state of the prop should be
> fully cotrolled by userspace. Is that not possible for some reason?
No, the privacy-screen can be toggled on/off with a Fn + somekey
hotkey combo on the laptop's keyboard, this is fully handled
by the laptop's embedded-controller.
[PATCH 05/10] drm/connector: Add a drm_connector privacy-screen helper functions (v2)
Add 2 drm_connector privacy-screen helper functions:
1. drm_connector_attach_privacy_screen_provider(), this function creates
and attaches the standard privacy-screen properties and registers a
generic notifier for generating sysfs-connector-status-events on external
changes to the privacy-screen status.
2. drm_connector_update_privacy_screen(), update the privacy-screen's
sw_state if the connector has a privacy-screen.
Changes in v2:
- Do not update connector->state->privacy_screen_sw_state on
atomic-commits.
- Change drm_connector_update_privacy_screen() to take drm_connector_state
as argument instead of a full drm_atomic_state. This allows the helper
to be called by drivers when they are enabling crtcs/encoders/connectors.
Reviewed-by: Emil Velikov
Reviewed-by: Lyude Paul
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_connector.c | 102
include/drm/drm_connector.h | 11
2 files changed, 113 insertions(+)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b2f1f1b1bfb4..00cf3b6135f6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
@@ -462,6 +463,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
DRM_CONNECTOR_REGISTERED))
drm_connector_unregister(connector);
+ if (connector->privacy_screen) {
+ drm_privacy_screen_put(connector->privacy_screen);
+ connector->privacy_screen = NULL;
+ }
+
if (connector->tile_group) {
drm_mode_put_tile_group(dev, connector->tile_group);
connector->tile_group = NULL;
@@ -543,6 +549,10 @@ int drm_connector_register(struct drm_connector *connector)
/* Let userspace know we have a new connector */
drm_sysfs_hotplug_event(connector->dev);
+ if (connector->privacy_screen)
+ drm_privacy_screen_register_notifier(connector->privacy_screen,
+ &connector->privacy_screen_notifier);
+
mutex_lock(&connector_list_lock);
list_add_tail(&connector->global_connector_list_entry, &connector_list);
mutex_unlock(&connector_list_lock);
@@ -578,6 +588,11 @@ void drm_connector_unregister(struct drm_connector
*connector)
list_del_init(&connector->global_connector_list_entry);
mutex_unlock(&connector_list_lock);
+ if (connector->privacy_screen)
+ drm_privacy_screen_unregister_notifier(
+ connector->privacy_screen,
+ &connector->privacy_screen_notifier);
+
if (connector->funcs->early_unregister)
connector->funcs->early_unregister(connector);
@@ -2442,6 +2457,93 @@ drm_connector_attach_privacy_screen_properties(struct
drm_connector *connector)
}
EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
+static void drm_connector_update_privacy_screen_properties(
+ struct drm_connector *connector, bool set_sw_state)
+{
+ enum drm_privacy_screen_status sw_state, hw_state;
+
+ drm_privacy_screen_get_state(connector->privacy_screen,
+&sw_state, &hw_state);
+
+ if (set_sw_state)
+ connector->state->privacy_screen_sw_state = sw_state;
+ drm_object_property_set_value(&connector->base,
+ connector->privacy_screen_hw_state_property, hw_state);
+}
+
+static int drm_connector_privacy_screen_notifier(
+ struct notifier_block *nb, unsigned long action, void *data)
+{
+ struct drm_connector *connector =
+ container_of(nb, struct drm_connector, privacy_screen_notifier);
+ struct drm_device *dev = connector->dev;
+
+ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+ drm_connector_update_privacy_screen_properties(connector, true);
+ drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+ drm_sysfs_connector_status_event(connector,
+ connector->privacy_screen_sw_state_property);
+ drm_sysfs_connector_status_event(connector,
+ connector->privacy_screen_hw_state_property);
+
+ return NOTIFY_DONE;
+}
+
+/**
+ * drm_connector_attach_privacy_screen_provider - attach a privacy-screen to
+ *the connector
+ * @connector: connector to attach the privacy-screen to
+ * @priv: drm_privacy_screen to attach
+ *
+ * Create and attach the standard privacy-screen properties and register
+ * a generic notifier for generating sysfs-connector-status-events
+ * on external changes to the privacy-screen status.
+ * This function takes ownership of the passed in drm_privacy_screen and will
+ * call drm_privacy_screen_put() on it when the connector is destroyed.
+ */
+void drm_connector_attach_privacy_screen
