On Thu, 2021-09-16 at 11:06 +0200, Hans de Goede wrote:
> Hi,
>
> On 9/15/21 10:26 PM, Lyude Paul wrote:
> > On Mon, 2021-09-06 at 09:35 +0200, Hans de Goede wrote:
> > > Add support for privacy-screen consumers to register a notifier to
> > > be notified of external (e.g. done by the hw itself on a hotkey press)
> > > state changes.
> > >
> > > Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/drm_privacy_screen.c | 67 +++++++++++++++++++++++
> > > include/drm/drm_privacy_screen_consumer.h | 15 +++++
> > > include/drm/drm_privacy_screen_driver.h | 4 ++
> > > 3 files changed, 86 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_privacy_screen.c
> > > b/drivers/gpu/drm/drm_privacy_screen.c
> > > index 294a09194bfb..7a5f878c3171 100644
> > > --- a/drivers/gpu/drm/drm_privacy_screen.c
> > > +++ b/drivers/gpu/drm/drm_privacy_screen.c
> > > @@ -255,6 +255,49 @@ void drm_privacy_screen_get_state(struct
> > > drm_privacy_screen *priv,
> > > }
> > > EXPORT_SYMBOL(drm_privacy_screen_get_state);
> > >
> > > +/**
> > > + * drm_privacy_screen_register_notifier - register a notifier
> > > + * @priv: Privacy screen to register the notifier with
> > > + * @nb: Notifier-block for the notifier to register
> > > + *
> > > + * Register a notifier with the privacy-screen to be notified of
> > > changes
> > > made
> > > + * to the privacy-screen state from outside of the privacy-screen
> > > class.
> > > + * E.g. the state may be changed by the hardware itself in response to
> > > a
> > > + * hotkey press.
> > > + *
> > > + * The notifier is called with no locks held. The new hw_state and
> > > sw_state
> > > + * can be retrieved using the drm_privacy_screen_get_state() function.
> > > + * A pointer to the drm_privacy_screen's struct is passed as the void
> > > *data
> > > + * argument of the notifier_block's notifier_call.
> > > + *
> > > + * The notifier will NOT be called when changes are made through
> > > + * drm_privacy_screen_set_sw_state(). It is only called for external
> > > changes.
> > > + *
> > > + * Return: 0 on success, negative error code on failure.
> > > + */
> > > +int drm_privacy_screen_register_notifier(struct drm_privacy_screen
> > > *priv,
> > > + struct notifier_block *nb)
> > > +{
> > > + return blocking_notifier_chain_register(&priv->notifier_head,
> > > nb);
> > > +}
> > > +EXPORT_SYMBOL(drm_privacy_screen_register_notifier);
> > > +
> > > +/**
> > > + * drm_privacy_screen_unregister_notifier - unregister a notifier
> > > + * @priv: Privacy screen to register the notifier with
> > > + * @nb: Notifier-block for the notifier to register
> > > + *
> > > + * Unregister a notifier registered with
> > > drm_privacy_screen_register_notifier().
> > > + *
> > > + * Return: 0 on success, negative error code on failure.
> > > + */
> > > +int drm_privacy_screen_unregister_notifier(struct drm_privacy_screen
> > > *priv,
> > > + struct notifier_block *nb)
> > > +{
> > > + return blocking_notifier_chain_unregister(&priv->notifier_head,
> > > nb);
> > > +}
> > > +EXPORT_SYMBOL(drm_privacy_screen_unregister_notifier);
> > > +
> > > /*** drm_privacy_screen_driver.h functions ***/
> > >
> > > static ssize_t sw_state_show(struct device *dev,
> > > @@ -352,6 +395,7 @@ struct drm_privacy_screen
> > > *drm_privacy_screen_register(
> > > return ERR_PTR(-ENOMEM);
> > >
> > > mutex_init(&priv->lock);
> > > + BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head);
> > >
> > > priv->dev.class = drm_class;
> > > priv->dev.type = &drm_privacy_screen_type;
> > > @@ -399,3 +443,26 @@ void drm_privacy_screen_unregister(struct
> > > drm_privacy_screen *priv)
> > > device_unregister(&priv->dev);
> > > }
> > > EXPORT_SYMBOL(drm_privacy_screen_unregister);
> > > +
> > > +/**
> > > + * drm_privacy_screen_call_notifier_chain - notify consumers of state
> > > change
> > > + * @priv: Privacy screen to register the notifier with
> > > + *
> > > + * A privacy-screen provider driver can call this functions upon
> > > external
> > > + * changes to the privacy-screen state. E.g. the state may be changed
> > > by
> > > the
> > > + * hardware itself in response to a hotkey press.
> > > + * This function must be called without holding the privacy-screen
> > > lock.
> > > + * the driver must update sw_state and hw_state to reflect the new
> > > state
> > > before
> > > + * calling this function.
> > > + * The expected behavior from the driver upon receiving an external
> > > state
> > > + * change event is: 1. Take the lock; 2. Update sw_state and hw_state;
> > > + * 3. Release the lock. 4. Call
> > > drm_privacy_screen_call_notifier_chain().
> > > + */
> > > +void drm_privacy_screen_call_notifier_chain(struct drm_privacy_screen
> > > *priv)
> > > +{
> > > + if (WARN_ON(mutex_is_locked(&priv->lock)))
> > > + return;
> >
> > Are we sure about this check? mutex_is_locked() checks whether a mutex is
> > locked by anyone, not just us. So this seems like it would cause us to
> > WARN_ON() and abort if anyone else (not just ourselves) is holding the
> > lock to
> > read the privacy screen state.
>
> Thank you for catching this, yes this check indeed is wrong. AFAIK
> there is no way to check that the mutex has been locked by us, so this
> extra sanity check simply needs to be removed.
>
> I'll drop the check before pushing this to drm-misc-next (more on
> that in a reply to the cover letter), if that is ok with you.
Sounds fine to me!
>
> Or do you want me to do a new version addressing this?
>
> Regards,
>
> Hans
>
>
>
> >
> > > +
> > > + blocking_notifier_call_chain(&priv->notifier_head, 0, priv);
> > > +}
> > > +EXPORT_SYMBOL(drm_privacy_screen_call_notifier_chain);
> > > diff --git a/include/drm/drm_privacy_screen_consumer.h
> > > b/include/drm/drm_privacy_screen_consumer.h
> > > index 0cbd23b0453d..7f66a90d15b7 100644
> > > --- a/include/drm/drm_privacy_screen_consumer.h
> > > +++ b/include/drm/drm_privacy_screen_consumer.h
> > > @@ -24,6 +24,11 @@ int drm_privacy_screen_set_sw_state(struct
> > > drm_privacy_screen *priv,
> > > void drm_privacy_screen_get_state(struct drm_privacy_screen *priv,
> > > enum drm_privacy_screen_status
> > > *sw_state_ret,
> > > enum drm_privacy_screen_status
> > > *hw_state_ret);
> > > +
> > > +int drm_privacy_screen_register_notifier(struct drm_privacy_screen
> > > *priv,
> > > + struct notifier_block *nb);
> > > +int drm_privacy_screen_unregister_notifier(struct drm_privacy_screen
> > > *priv,
> > > + struct notifier_block *nb);
> > > #else
> > > static inline struct drm_privacy_screen *drm_privacy_screen_get(struct
> > > device *dev,
> > > const
> > > char
> > > *con_id)
> > > @@ -45,6 +50,16 @@ static inline void
> > > drm_privacy_screen_get_state(struct
> > > drm_privacy_screen *priv,
> > > *sw_state_ret = PRIVACY_SCREEN_DISABLED;
> > > *hw_state_ret = PRIVACY_SCREEN_DISABLED;
> > > }
> > > +static inline int drm_privacy_screen_register_notifier(struct
> > > drm_privacy_screen *priv,
> > > + struct
> > > notifier_block
> > > *nb)
> > > +{
> > > + return -ENODEV;
> > > +}
> > > +static inline int drm_privacy_screen_unregister_notifier(struct
> > > drm_privacy_screen *priv,
> > > + struct
> > > notifier_block *nb)
> > > +{
> > > + return -ENODEV;
> > > +}
> > > #endif
> > >
> > > #endif
> > > diff --git a/include/drm/drm_privacy_screen_driver.h
> > > b/include/drm/drm_privacy_screen_driver.h
> > > index 5187ae52eb03..24591b607675 100644
> > > --- a/include/drm/drm_privacy_screen_driver.h
> > > +++ b/include/drm/drm_privacy_screen_driver.h
> > > @@ -54,6 +54,8 @@ struct drm_privacy_screen {
> > > struct mutex lock;
> > > /** @list: privacy-screen devices list list-entry. */
> > > struct list_head list;
> > > + /** @notifier_head: privacy-screen notifier head. */
> > > + struct blocking_notifier_head notifier_head;
> > > /**
> > > * @ops: &struct drm_privacy_screen_ops for this privacy-screen.
> > > * This is NULL if the driver has unregistered the privacy-
> > > screen.
> > > @@ -77,4 +79,6 @@ struct drm_privacy_screen
> > > *drm_privacy_screen_register(
> > > struct device *parent, const struct drm_privacy_screen_ops
> > > *ops);
> > > void drm_privacy_screen_unregister(struct drm_privacy_screen *priv);
> > >
> > > +void drm_privacy_screen_call_notifier_chain(struct drm_privacy_screen
> > > *priv);
> > > +
> > > #endif
> >
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
