Re: [Intel-gfx] [PATCH 0/9] drm: Add privacy-screen class and connector properties

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 16 Sep 2021, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 9/15/21 11:12 PM, Lyude Paul wrote:
>> OK! Looked over all of these patches. Patches 2 and 4 have some comments that
>> should be addressed, but otherwise this series is:
>> 
>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> Thank you!
>
>> Let me know when/if you need help pushing this upstream
>
> My plan was to just straight forward push the entire series to
> drm-misc-next. The only non drm bits are the drivers/platform/x86/thinkpad_acpi.c
> changes and I'm the drivers/platform/x86 subsys-maintainer and this
> plan has my blessing :)
>
> That only leaves the last patch in the series:
> "drm/i915: Add privacy-screen support" 
>
> As something which could potentially be troublesome. I can also
> leave that out, while pushing the test to drm-misc-next and then
> push the i915 patch after a drm-misc-next merge into drm-intel-next.
>
> Jani, how would you like to handle the single i915 patch in this
> series?

I think it's easiest to merge that via the same route as everything else
goes. It's a fairly small patch with not that much conflict potential.

That said, there are some issues in that patch I think still need
addressing. I've commented on the patch. I don't mind you pushing the
rest already, unless you think addressing the issues is easier by
modifying the other patches (I don't think so).


BR,
Jani.

>
> Regards,
>
> Hans
>
>
>
>
>
>> 
>> On Mon, 2021-09-06 at 09:35 +0200, Hans de Goede wrote:
>>> Hi all,
>>>
>>> Here is the privacy-screen related code which I last posted in April 2021
>>> To the best of my knowledge there is consensus about / everyone is in
>>> agreement with the new userspace API (2 connector properties) this
>>> patch-set add (patch 1 of the series).
>>>
>>> This is unchanged (except for a rebase on drm-tip), what has changed is
>>> that the first userspace consumer of the new properties is now fully ready
>>> for merging (it is just waiting for the kernel bits to land first):
>>>
>>>  -
>>> https://gitlab.gnome.org/GNOME/gsettings-desktop-schemas/-/merge_requests/49
>>>  - https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1952
>>>  - https://gitlab.gnome.org/GNOME/gnome-control-center/-/merge_requests/1032
>>>
>>> Having a userspace-consumer of the API fully ready for merging, clears the
>>> last blocker for this series. It has already has been reviewed before
>>> by Emil Velikov, but it could really do with another review.
>>>
>>> The new API works as designed and add the following features to GNOME:
>>>
>>> 1. Showing an OSD notification when the privacy-screen is toggled on/off
>>>    through hotkeys handled by the embedded-controller
>>> 2. Allowing control of the privacy-screen from the GNOME control-panel,
>>>    including the on/off slider shown there updating to match the hw-setting
>>>    when the setting is changed with the control-panel open.
>>> 3. Restoring the last user-setting at login
>>>
>>> This series consists of a number of different parts:
>>>
>>> 1. A new version of Rajat's privacy-screen connector properties patch,
>>> this adds new userspace API in the form of new properties
>>>
>>> 2. Since on most devices the privacy screen is actually controlled by
>>> some vendor specific ACPI/WMI interface which has a driver under
>>> drivers/platform/x86, we need some "glue" code to make this functionality
>>> available to KMS drivers. Patches 2-4 add a new privacy-screen class for
>>> this, which allows non KMS drivers (and possibly KMS drivers too) to
>>> register a privacy-screen device and also adds an interface for KMS drivers
>>> to get access to the privacy-screen associated with a specific connector.
>>> This is modelled similar to how we deal with e.g. PWMs and GPIOs in the
>>> kernel, including separate includes for consumers and providers(drivers).
>>>
>>> 3. Some drm_connector helper functions to keep the actual changes needed
>>> for this in individual KMS drivers as small as possible (patch 5).
>>>
>>> 4. Make the thinkpad_acpi code register a privacy-screen device on
>>> ThinkPads with a privacy-screen (patches 6-8)
>>>
>>> 5. Make the i915 driver export the privacy-screen functionality through
>>> the connector properties on the eDP connector.
>>>
>>> I believe that it would be best to merge the entire series, including
>>> the thinkpad_acpi changes through drm-misc in one go. As the pdx86
>>> subsys maintainer I hereby give my ack for merging the thinkpad_acpi
>>> changes through drm-misc.
>>>
>>> There is one small caveat with this series, which it is good to be
>>> aware of. The i915 driver will now return -EPROBE_DEFER on Thinkpads
>>> with an eprivacy screen, until the thinkpad_acpi driver is loaded.
>>> This means that initrd generation tools will need to be updated to
>>> include thinkpad_acpi when the i915 driver is added to the initrd.
>>> Without this the loading of the i915 driver will be delayed to after
>>> the switch to real rootfs.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>> Hans de Goede (8):
>>>   drm: Add privacy-screen class (v3)
>>>   drm/privacy-screen: Add X86 specific arch init code
>>>   drm/privacy-screen: Add notifier support
>>>   drm/connector: Add a drm_connector privacy-screen helper functions
>>>   platform/x86: thinkpad_acpi: Add hotkey_notify_extended_hotkey()
>>>     helper
>>>   platform/x86: thinkpad_acpi: Get privacy-screen / lcdshadow ACPI
>>>     handles only once
>>>   platform/x86: thinkpad_acpi: Register a privacy-screen device
>>>   drm/i915: Add privacy-screen support
>>>
>>> Rajat Jain (1):
>>>   drm/connector: Add support for privacy-screen properties (v4)
>>>
>>>  Documentation/gpu/drm-kms-helpers.rst        |  15 +
>>>  Documentation/gpu/drm-kms.rst                |   2 +
>>>  MAINTAINERS                                  |   8 +
>>>  drivers/gpu/drm/Kconfig                      |   4 +
>>>  drivers/gpu/drm/Makefile                     |   1 +
>>>  drivers/gpu/drm/drm_atomic_uapi.c            |   4 +
>>>  drivers/gpu/drm/drm_connector.c              | 214 +++++++++
>>>  drivers/gpu/drm/drm_drv.c                    |   4 +
>>>  drivers/gpu/drm/drm_privacy_screen.c         | 468 +++++++++++++++++++
>>>  drivers/gpu/drm/drm_privacy_screen_x86.c     |  86 ++++
>>>  drivers/gpu/drm/i915/display/intel_display.c |   5 +
>>>  drivers/gpu/drm/i915/display/intel_dp.c      |  10 +
>>>  drivers/gpu/drm/i915/i915_pci.c              |  12 +
>>>  drivers/platform/x86/Kconfig                 |   2 +
>>>  drivers/platform/x86/thinkpad_acpi.c         | 131 ++++--
>>>  include/drm/drm_connector.h                  |  56 +++
>>>  include/drm/drm_privacy_screen_consumer.h    |  65 +++
>>>  include/drm/drm_privacy_screen_driver.h      |  84 ++++
>>>  include/drm/drm_privacy_screen_machine.h     |  46 ++
>>>  19 files changed, 1175 insertions(+), 42 deletions(-)
>>>  create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
>>>  create mode 100644 drivers/gpu/drm/drm_privacy_screen_x86.c
>>>  create mode 100644 include/drm/drm_privacy_screen_consumer.h
>>>  create mode 100644 include/drm/drm_privacy_screen_driver.h
>>>  create mode 100644 include/drm/drm_privacy_screen_machine.h
>>>
>> 
>

-- 
Jani Nikula, Intel Open Source Graphics Center