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

[Hans de Goede]

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 at 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 at 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