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@xxxxxxxxxx> 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@xxxxxxxxx>
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
