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