[Intel-gfx] [PATCH v2 2/9] drm: Add privacy-screen class (v2)

[Emil Velikov]

On Thu, 3 Jun 2021 at 12:59, Hans de Goede <hdegoede at redhat.com> wrote:

> >
> >> +#include "drm_internal.h"
> >
> > I think we don't need this include, do we?
>
> The drm_privacy_screen device registered by a provider
> uses /sys/class/drm as its class, quoting from
> drm_privacy_screen.c drm_privacy_screen_register():
>
>         priv->dev.class = drm_class;
>         priv->dev.type = &drm_privacy_screen_type;
>         priv->dev.parent = parent;
>         priv->dev.release = drm_privacy_screen_device_release;
>         dev_set_name(&priv->dev, "privacy_screen-%s", dev_name(parent));
>         priv->ops = ops;
>
>         priv->ops->get_hw_state(priv);
>
>         ret = device_register(&priv->dev);
>
> Notice the "priv->dev.class = drm_class", the drm_class
> variable is declared in "drm_internal.h".
>
I have been looking at v1 while replying here, oopsie.

> >> --- /dev/null
> >> +++ b/include/drm/drm_privacy_screen_consumer.h
> >
> >> +#include <drm/drm_connector.h>
> >
> > Ditto
>
> The "enum drm_privacy_screen_status" used in various places
> comes from drm/drm_connector.h (it is the same enum which is
> used for the possible values of the drm-connector properties).
>
Hmm indeed.

If really needed one could move/duplicate/duplicate-and-namespace the
enum. If duplicating, cross references would be great and with
namespacing BUILD_BUG_ON(drm_privacy_screen_status::foo !=
custom-enum::foo) to enforce consistency.

Each feels dirty and I'm not sure if it's worth it -  just a silly
idea, don't read too much into it.

>
> >> --- /dev/null
> >> +++ b/include/drm/drm_privacy_screen_driver.h
> >
> >> +#include <drm/drm_connector.h>
> >
> > Ditto
> >
> > I like how you avoided leaking any DRM details within the new code,
> > modulo the includes above.
>
> I'm glad you like it. I did indeed try to make the code mostly
> independent, but as you can see above there are still some
> inter-dependencies.
>
> Because of this, the CONFIG_DRM_PRIVACY_SCREEN option also does
> not control building this into a separate module. Like many other
> DRM Kconfig options, this controls if the privacy-screen code will
> be added to drm.ko or not.
>
> Despite being 99% independent, the 2 are still intertwined at such
> a level that this is necessary. Specifically drm_core_init() calls
> drm_privacy_screen_lookup_init() to initialize the static lookup
> table which is used to see if there is a privacy-screen (and to which
> GPU,output combo it should be mapped). So if CONFIG_DRM_PRIVACY_SCREEN
> is enabled and drm.ko is builtin then it must be builtin too, at which
> point it is easiest to just make it part of drm.ko .
>
Yes, the initialisation is called from core drm - it has to happen somewhere.

What I was thinking of, is that we can reuse it (with minor tweaks) if
vendors deploy the privacy screen principle for audio, camera, etc.
Kind of crazy examples, but who knows.

> > With above tweaks, the series is:
> > Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>
>
> As I've tried to explain, the includes are necessary, does your
> Reviewed-by still stands when I keep the includes ?
>
Yes, r-b it stands.

Thanks for the in the detailed reply and drm_class pointer :-P
-Emil