From: Rajat Jain <rajatja@google.com>
To: "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Maxime Ripard" <mripard@kernel.org>,
"Sean Paul" <sean@poorly.run>, "David Airlie" <airlied@linux.ie>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Jani Nikula" <jani.nikula@linux.intel.com>,
"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
"Chris Wilson" <chris@chris-wilson.co.uk>,
"Imre Deak" <imre.deak@intel.com>,
"José Roberto de Souza" <jose.souza@intel.com>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
intel-gfx@lists.freedesktop.org,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Mat King" <mathewk@google.com>,
"Daniel Thompson" <daniel.thompson@linaro.org>,
"Jonathan Corbet" <corbet@lwn.net>,
"Pavel Machek" <pavel@denx.de>, "Sean Paul" <seanpaul@google.com>,
"Duncan Laurie" <dlaurie@google.com>,
"Jesse Barnes" <jsbarnes@google.com>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Mark Pearson" <mpearson@lenovo.com>,
"Nitin Joshi1" <njoshi1@lenovo.com>,
"Sugumaran Lacshiminarayanan" <slacshiminar@lenovo.com>,
"Tomoki Maruichi" <maruichit@lenovo.com>
Cc: Rajat Jain <rajatxjain@gmail.com>
Subject: Re: [Intel-gfx] [PATCH v7 4/4] drm/i915: Add support for integrated privacy screen
Date: Mon, 9 Mar 2020 17:18:32 -0700 [thread overview]
Message-ID: <CACK8Z6FsN2WNF8z0OrKv-1sPPwJFeJVYf8taHMZn0QpwLj+t=Q@mail.gmail.com> (raw)
In-Reply-To: <20200310000617.20662-5-rajatja@google.com>
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-03-10 0:19 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-10 0:06 [Intel-gfx] [PATCH v7 0/4] drm: Add support for integrated privacy screen Rajat Jain
2020-03-10 0:06 ` [Intel-gfx] [PATCH v7 1/4] intel_acpi: Rename drm_dev local variable to dev Rajat Jain
2020-03-10 0:06 ` [Intel-gfx] [PATCH v7 2/4] drm/connector: Add support for privacy-screen property Rajat Jain
2020-03-10 0:06 ` [Intel-gfx] [PATCH v7 3/4] drm/i915: Lookup and attach ACPI device node for connectors Rajat Jain
2020-03-10 0:06 ` [Intel-gfx] [PATCH v7 4/4] drm/i915: Add support for integrated privacy screen Rajat Jain
2020-03-10 0:18 ` Rajat Jain [this message]
2020-03-12 5:55 ` Rajat Jain
2020-03-12 11:24 ` Jani Nikula
2020-04-06 22:07 ` Rajat Jain
2020-03-10 0:23 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm: " Patchwork
2020-03-10 16:37 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-03-10 21:08 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACK8Z6FsN2WNF8z0OrKv-1sPPwJFeJVYf8taHMZn0QpwLj+t=Q@mail.gmail.com' \
--to=rajatja@google.com \
--cc=airlied@linux.ie \
--cc=chris@chris-wilson.co.uk \
--cc=corbet@lwn.net \
--cc=daniel.thompson@linaro.org \
--cc=daniel@ffwll.ch \
--cc=dlaurie@google.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=joonas.lahtinen@linux.intel.com \
--cc=jose.souza@intel.com \
--cc=jsbarnes@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=maruichit@lenovo.com \
--cc=mathewk@google.com \
--cc=mpearson@lenovo.com \
--cc=mripard@kernel.org \
--cc=njoshi1@lenovo.com \
--cc=pavel@denx.de \
--cc=rajatxjain@gmail.com \
--cc=rodrigo.vivi@intel.com \
--cc=sean@poorly.run \
--cc=seanpaul@google.com \
--cc=slacshiminar@lenovo.com \
--cc=thierry.reding@gmail.com \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).