Re: [PATCH v7 4/4] drm/i915: Add support for integrated privacy screen

Hi Jani,

I have 1 question / need 1 help about this patch:

On Mon, Mar 9, 2020 at 5:06 PM Rajat Jain rajatja@google.com wrote:

Add support for an ACPI based integrated privacy screen that is available on some systems.

Signed-off-by: Rajat Jain rajatja@google.com

v7: * Move the privacy-screen property back into drm core. * Do the actual HW EPS toggling at commit time. * Provide a sample ACPI node for reference in comments. v6: Always initialize prop in intel_attach_privacy_screen_property() v5: fix a cosmetic checkpatch warning v4: Fix a typo in intel_privacy_screen.h v3: * Change license to GPL-2.0 OR MIT * Move privacy screen enum from UAPI to intel_display_types.h * Rename parameter name and some other minor changes. v2: Formed by splitting the original patch into multiple patches. - All code has been moved into i915 now. - Privacy screen is a i915 property - Have a local state variable to store the prvacy screen. Don't read it from hardware.

drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/display/intel_atomic.c | 1 + drivers/gpu/drm/i915/display/intel_dp.c | 30 ++- .../drm/i915/display/intel_privacy_screen.c | 175 ++++++++++++++++++ .../drm/i915/display/intel_privacy_screen.h | 27 +++ 5 files changed, 234 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.c create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9f887a86e555d..da42389107f9c 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -209,7 +209,8 @@ i915-y += \ display/intel_vga.o i915-$(CONFIG_ACPI) += \ display/intel_acpi.o \

•   display/intel_opregion.o
  

•   display/intel_opregion.o \
  

•   display/intel_privacy_screen.o
  

i915-$(CONFIG_DRM_FBDEV_EMULATION) += \ display/intel_fbdev.o

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index d043057d2fa03..fc6264b4a7f73 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -150,6 +150,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio || new_conn_state->base.content_type != old_conn_state->base.content_type || new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode ||

•       new_conn_state->base.privacy_screen_status != old_conn_state->base.privacy_screen_status ||
        !blob_equal(new_conn_state->base.hdr_output_metadata,
                    old_conn_state->base.hdr_output_metadata))
            crtc_state->mode_changed = true;
  

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 41c623b029464..a39b0c420b50a 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -62,6 +62,7 @@ #include "intel_lspcon.h" #include "intel_lvds.h" #include "intel_panel.h" +#include "intel_privacy_screen.h" #include "intel_psr.h" #include "intel_sideband.h" #include "intel_tc.h" @@ -5886,6 +5887,12 @@ intel_dp_connector_register(struct drm_connector *connector) dev_priv->acpi_scan_done = true; }

•   /* Check for integrated Privacy screen support */
  

•   if (intel_privacy_screen_present(to_intel_connector(connector)))
  

•           drm_connector_attach_privacy_screen_property(connector);
  

•   else
  

•           drm_connector_destroy_privacy_screen_property(connector);
  

•   DRM_DEBUG_KMS("registering %s bus for %s\n",
                  intel_dp->aux.name, connector->kdev->kobj.name);
  
  

@@ -6881,9 +6888,30 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect drm_connector_attach_scaling_mode_property(connector, allowed_scalers);

            connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;

•           drm_connector_create_privacy_screen_property(connector);
    }
  

}

+static void intel_dp_update_privacy_screen(struct intel_encoder *encoder,

•                           const struct intel_crtc_state *crtc_state,
  

•                           const struct drm_connector_state *conn_state)
  

+{

•   struct drm_connector *connector = conn_state->connector;
  

•   if (connector->privacy_screen_property)
  

•           intel_privacy_screen_set_val(to_intel_connector(connector),
  

•                                        conn_state->privacy_screen_status);
  

+}

+static void intel_dp_update_pipe(struct intel_encoder *encoder,

•                            const struct intel_crtc_state *crtc_state,
  

•                            const struct drm_connector_state *conn_state)
  

+{

•   intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
  

•   intel_panel_update_backlight(encoder, crtc_state, conn_state);
  

+}

static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp) { intel_dp->panel_power_off_time = ktime_get_boottime(); @@ -7825,7 +7853,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv, intel_encoder->compute_config = intel_dp_compute_config; intel_encoder->get_hw_state = intel_dp_get_hw_state; intel_encoder->get_config = intel_dp_get_config;

•   intel_encoder->update_pipe = intel_panel_update_backlight;
  

•   intel_encoder->update_pipe = intel_dp_update_pipe;
  

For my testing purposes, I'm testing this using the proptest userspace utility in our distribution (I think from https://github.com/CPFL/drm/blob/master/tests/proptest/proptest.c). I notice that when I change the value of the property from userspace, even though the drm_connector_state->privacy_screen_status gets updated and reflects the change, the encoder->update_pipe() is not getting called. Just wanted to ask if this is expected since you seem to have implied that this update_pipe() might *not* get called if there *is* a full modeset? (What is the hook that gets called for a full modeset where i915 driver should commit this property change to the hardware?)

Thanks a lot for all your help,

Best Regards,

Rajat

    intel_encoder->suspend = intel_dp_encoder_suspend;
    if (IS_CHERRYVIEW(dev_priv)) {
            intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;

diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.c b/drivers/gpu/drm/i915/display/intel_privacy_screen.c new file mode 100644 index 0000000000000..6ff61ddf4c0a4 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c @@ -0,0 +1,175 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/*

• • Intel ACPI privacy screen code
• • Copyright © 2020 Google Inc.
• • This code can help detect and control an integrated EPS (electronic
• • privacy screen) via ACPI functions. It expects an ACPI node for the
• • drm connector device with the following elements:
• • UUID should be "c7033113-8720-4ceb-9090-9d52b3e52d73"
• • _ADR = ACPI address per Spec (also see intel_acpi_device_id_update())
• • https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
• • Pages 1119 - 1123.
• • _DSM method that will perform the following functions according to
• • Local1 argument passed to it:
• • • Local1 = 0 (EPS capabilities): Report EPS presence and capabilities.
• • • Local1 = 1 (EPS State) : _DSM returns 1 if EPS is enabled, 0 otherwise.
• • • Local1 = 2 (EPS Enable) : _DSM enables EPS
• • • Local1 = 3 (EPS Disable): _DSM disables EPS
• • Here is a sample ACPI node:
• • Scope (_SB.PCI0.GFX0) // Intel graphics device (PCI device)
• • {

• •  Method (_DOD, 0, NotSerialized)  // _DOD: Display Output Devices
    

• •  {
    

• •      Return (Package (0x01)
    

• •      {
    

• •          0x80010400
    

• •      })
    

• •  }
    

• •  Device (LCD)
    

• •  {
    

• •      Name (_ADR, 0x80010400)  // _ADR: Address
    

• •      Name (_STA, 0x0F)  // _STA: Status
    

• •      Method (EPSP, 0, NotSerialized) // EPS Present
    

• •      {
    

• •          Return (0x01)
    

• •      }
    

• •      Method (EPSS, 0, NotSerialized) // EPS State
    

• •      {
    

• •          Local0 = \_SB.PCI0.GRXS (0xCD)
    

• •          Return (Local0)
    

• •      }
    

• •      Method (EPSE, 0, NotSerialized) // EPS Enable
    

• •      {
    

• •          \_SB.PCI0.STXS (0xCD)
    

• •      }
    

• •      Method (EPSD, 0, NotSerialized) // EPS Disable
    

• •      {
    

• •          \_SB.PCI0.CTXS (0xCD)
    

• •      }
    

• •      Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
    

• •      {
    

• •          ToBuffer (Arg0, Local0)
    

• •          If ((Local0 == ToUUID ("c7033113-8720-4ceb-9090-9d52b3e52d73")))
    

• •          {
    

• •              ToInteger (Arg2, Local1)
    

• •              If ((Local1 == Zero))
    

• •              {
    

• •                  Local2 = EPSP ()
    

• •                  If ((Local2 == One))
    

• •                  {
    

• •                      Return (Buffer (One)
    

• •                      {
    

• •                           0x0F
    

• •                      })
    

• •                  }
    

• •              }
    

• •              If ((Local1 == One))
    

• •              {
    

• •                  Return (EPSS ())
    

• •              }
    

• •              If ((Local1 == 0x02))
    

• •              {
    

• •                  EPSE ()
    

• •              }
    

• •              If ((Local1 == 0x03))
    

• •              {
    

• •                  EPSD ()
    

• •              }
    

• •              Return (Buffer (One)
    

• •              {
    

• •                   0x00
    

• •              })
    

• •          }
    

• •          Return (Buffer (One)
    

• •          {
    

• •               0x00
    

• •          })
    

• •      }
    

• •  }
    

• • }
• */

+#include <linux/acpi.h>

+#include "intel_privacy_screen.h"

+#define CONNECTOR_DSM_REVID 1

+#define CONNECTOR_DSM_FN_PRIVACY_ENABLE 2 +#define CONNECTOR_DSM_FN_PRIVACY_DISABLE 3

+static const guid_t drm_conn_dsm_guid =

•   GUID_INIT(0xC7033113, 0x8720, 0x4CEB,
  

•             0x90, 0x90, 0x9D, 0x52, 0xB3, 0xE5, 0x2D, 0x73);
  

+/* Makes _DSM call to set privacy screen status */ +static void acpi_privacy_screen_call_dsm(struct intel_connector *connector,

•                                    u64 func)
  

+{

•   union acpi_object *obj;
  

•   acpi_handle acpi_handle = connector->acpi_handle;
  

•   if (!acpi_handle)
  

•           return;
  

•   obj = acpi_evaluate_dsm(acpi_handle, &drm_conn_dsm_guid,
  

•                           CONNECTOR_DSM_REVID, func, NULL);
  

•   if (!obj) {
  

•           drm_err(connector->base.dev,
  

•                   "failed to evaluate _DSM for fn %llx\n", func);
  

•           return;
  

•   }
  

•   ACPI_FREE(obj);
  

+}

+void intel_privacy_screen_set_val(struct intel_connector *connector,

•                             enum drm_privacy_screen_status val)
  

+{

•   if (val == PRIVACY_SCREEN_DISABLED)
  

•           acpi_privacy_screen_call_dsm(connector,
  

•                                        CONNECTOR_DSM_FN_PRIVACY_DISABLE);
  

•   else if (val == PRIVACY_SCREEN_ENABLED)
  

•           acpi_privacy_screen_call_dsm(connector,
  

•                                        CONNECTOR_DSM_FN_PRIVACY_ENABLE);
  

•   else
  

•           drm_err(connector->base.dev,
  

•                   "Cannot set privacy screen to invalid val %u\n", val);
  

+}

+bool intel_privacy_screen_present(struct intel_connector *connector) +{

•   acpi_handle handle = connector->acpi_handle;
  

•   if (!handle)
  

•           return false;
  

•   if (!acpi_check_dsm(handle, &drm_conn_dsm_guid,
  

•                       CONNECTOR_DSM_REVID,
  

•                       1 << CONNECTOR_DSM_FN_PRIVACY_ENABLE |
  

•                       1 << CONNECTOR_DSM_FN_PRIVACY_DISABLE)) {
  

•           drm_dbg_kms(connector->base.dev,
  

•                       "ACPI node but no privacy scrn\n");
  

•           return false;
  

•   }
  

•   drm_info(connector->base.dev, "supports privacy screen\n");
  

•   return true;
  

+} diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.h b/drivers/gpu/drm/i915/display/intel_privacy_screen.h new file mode 100644 index 0000000000000..f8d2e246ea0bd --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/*

• • Copyright © 2019 Google Inc.
• */

+#ifndef __DRM_PRIVACY_SCREEN_H__ +#define __DRM_PRIVACY_SCREEN_H__

+#include "intel_display_types.h"

+#ifdef CONFIG_ACPI +bool intel_privacy_screen_present(struct intel_connector *connector); +void intel_privacy_screen_set_val(struct intel_connector *connector,

•                             enum drm_privacy_screen_status val);
  

+#else +static bool intel_privacy_screen_present(struct intel_connector *connector) +{

•   return false;
  

+}

+static void +intel_privacy_screen_set_val(struct intel_connector *connector,

•                        enum drm_privacy_screen_status val)
  

+{ } +#endif /* CONFIG_ACPI */

+#endif /* __DRM_PRIVACY_SCREEN_H__ */

2.25.1.481.gfbce0eb801-goog