On Thu, 3 Jun 2021 at 12:59, Hans de Goede hdegoede@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@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