Hi,
On 6/1/21 5:31 PM, Emil Velikov wrote:
Hi Hans,
What happened with this series, did it fall through the cracks?
Sorta, as Marco already mentioned I think people are waiting for the user-space branches which he has on his personal git repos to be submitted as offical merge-req-s to GNOME.
On Wed, 21 Apr 2021 at 21:48, Hans de Goede hdegoede@redhat.com wrote:
--- /dev/null +++ b/drivers/gpu/drm/drm_privacy_screen.c
+#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".
Note this was not present in v2. As I mentioned in the commit msg:
Changes in v2: - Make CONFIG_DRM_PRIVACY_SCREEN a bool which controls if the drm_privacy code gets built as part of the main drm module rather then making it a tristate which builds its own module. - Add a #if IS_ENABLED(CONFIG_DRM_PRIVACY_SCREEN) check to drm_privacy_screen_consumer.h and define stubs when the check fails. Together these 2 changes fix several dependency issues. - Remove module related code now that this is part of the main drm.ko - Use drm_class as class for the privacy-screen devices instead of adding a separate class for this
This is something which I changed in v2. I changed this since I didn't really see any good reason for drm_privacy_screen devices having their own class, rather then just having them sit under /sys/class/drm .
I'm open to changing this if people dislike this choice.
--- /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).
--- /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 .
And there also is the later added dep from drm_privacy_screen.c on the drm_class symbol, which means there are now symbol-deps in both directions, which makes building the code into drm.ko the only option.
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 ?
Theoretically one could also remove the `depends on DRM` from patch 8/9 but I'm not sure how much that saves us.
The depends in is necessary since CONFIG_DRM_PRIVACY_SCREEN just controls if privacy-screen support will be added to drm.ko, if it is enabled we still need drm.ko ti actually be built for things to work.
HTH
Yes, actually getting a review of this code helps a lot, thank you.
Regards,
Hans