Re: [PATCH v2 3/3] drm/i915: Add support for integrated privacy screens

From: Jani Nikula
Date: Wed Nov 20 2019 - 10:11:28 EST

On Tue, 12 Nov 2019, Rajat Jain <rajatja@xxxxxxxxxx> wrote:
> On Mon, Nov 4, 2019 at 11:41 AM Rajat Jain <rajatja@xxxxxxxxxx> wrote:
>>
>> Certain laptops now come with panels that have integrated privacy
>> screens on them. This patch adds support for such panels by adding
>> a privacy-screen property to the intel_connector for the panel, that
>> the userspace can then use to control and check the status.
>>
>> Identifying the presence of privacy screen, and controlling it, is done
>> via ACPI _DSM methods.
>>
>> Currently, this is done only for the Intel display ports. But in future,
>> this can be done for any other ports if the hardware becomes available
>> (e.g. external monitors supporting integrated privacy screens?).
>>
>> Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>
>> Change-Id: Ic9ff07fc4a50797d2d0dfb919f11aa0821a4b548
>
>
> Hi Folks,
>
> I posted a v2 taking care of the comments I received (also split it
> into 3 patches now, and resused some ACPI code I found in i915
> driver). . Wondering if any one got a chance to look at this?

For future reference, please post the updated series standalone, *not*
in reply to long, old threads. Besides myself, it'll also help our CI
find your patches and crunch a bunch of tests on them.

Also, do you have an open userspace for this? See [1]. I think this
looks like good stuff to me, but then I'm not responsible for any
userspace component that would actually use this.

BR,
Jani.


[1] https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements



>
> Thanks,
>
> Rajat
>
>> ---
>> 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 | 13 +++-
>> .../gpu/drm/i915/display/intel_connector.c | 35 ++++++++++
>> .../gpu/drm/i915/display/intel_connector.h | 1 +
>> .../drm/i915/display/intel_display_types.h | 4 ++
>> drivers/gpu/drm/i915/display/intel_dp.c | 5 ++
>> .../drm/i915/display/intel_privacy_screen.c | 70 +++++++++++++++++++
>> .../drm/i915/display/intel_privacy_screen.h | 25 +++++++
>> include/uapi/drm/i915_drm.h | 14 ++++
>> 9 files changed, 166 insertions(+), 4 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 2587ea834f06..3589ebcf27bc 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -185,7 +185,8 @@ i915-y += \
>> display/intel_tc.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 d3fb75bb9eb1..378772d3449c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
>> @@ -37,6 +37,7 @@
>> #include "intel_atomic.h"
>> #include "intel_display_types.h"
>> #include "intel_hdcp.h"
>> +#include "intel_privacy_screen.h"
>> #include "intel_sprite.h"
>>
>> /**
>> @@ -57,11 +58,14 @@ int intel_digital_connector_atomic_get_property(struct drm_connector *connector,
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> struct intel_digital_connector_state *intel_conn_state =
>> to_intel_digital_connector_state(state);
>> + struct intel_connector *intel_connector = to_intel_connector(connector);
>>
>> if (property == dev_priv->force_audio_property)
>> *val = intel_conn_state->force_audio;
>> else if (property == dev_priv->broadcast_rgb_property)
>> *val = intel_conn_state->broadcast_rgb;
>> + else if (property == intel_connector->privacy_screen_property)
>> + *val = intel_conn_state->privacy_screen_status;
>> else {
>> DRM_DEBUG_ATOMIC("Unknown property [PROP:%d:%s]\n",
>> property->base.id, property->name);
>> @@ -89,15 +93,18 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> struct intel_digital_connector_state *intel_conn_state =
>> to_intel_digital_connector_state(state);
>> + struct intel_connector *intel_connector = to_intel_connector(connector);
>>
>> if (property == dev_priv->force_audio_property) {
>> intel_conn_state->force_audio = val;
>> return 0;
>> - }
>> -
>> - if (property == dev_priv->broadcast_rgb_property) {
>> + } else if (property == dev_priv->broadcast_rgb_property) {
>> intel_conn_state->broadcast_rgb = val;
>> return 0;
>> + } else if (property == intel_connector->privacy_screen_property) {
>> + intel_privacy_screen_set_val(intel_connector, val);
>> + intel_conn_state->privacy_screen_status = val;
>> + return 0;
>> }
>>
>> DRM_DEBUG_ATOMIC("Unknown property [PROP:%d:%s]\n",
>> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
>> index 308ec63207ee..3ccbf52aedf9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_connector.c
>> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
>> @@ -281,3 +281,38 @@ intel_attach_colorspace_property(struct drm_connector *connector)
>> drm_object_attach_property(&connector->base,
>> connector->colorspace_property, 0);
>> }
>> +
>> +static const struct drm_prop_enum_list privacy_screen_enum[] = {
>> + { PRIVACY_SCREEN_DISABLED, "Disabled" },
>> + { PRIVACY_SCREEN_ENABLED, "Enabled" },
>> +};
>> +
>> +/**
>> + * intel_attach_privacy_screen_property -
>> + * create and attach the connecter's privacy-screen property. *
>> + * @connector: connector for which to init the privacy-screen property
>> + *
>> + * This function creates and attaches the "privacy-screen" property to the
>> + * connector. Initial state of privacy-screen is set to disabled.
>> + */
>> +void
>> +intel_attach_privacy_screen_property(struct drm_connector *connector)
>> +{
>> + struct intel_connector *intel_connector = to_intel_connector(connector);
>> + struct drm_property *prop;
>> +
>> + if (!intel_connector->privacy_screen_property) {
>> + prop = drm_property_create_enum(connector->dev,
>> + DRM_MODE_PROP_ENUM,
>> + "privacy-screen",
>> + privacy_screen_enum,
>> + ARRAY_SIZE(privacy_screen_enum));
>> + if (!prop)
>> + return;
>> +
>> + intel_connector->privacy_screen_property = prop;
>> + }
>> +
>> + drm_object_attach_property(&connector->base, prop,
>> + PRIVACY_SCREEN_DISABLED);
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h
>> index 93a7375c8196..61005f37a338 100644
>> --- a/drivers/gpu/drm/i915/display/intel_connector.h
>> +++ b/drivers/gpu/drm/i915/display/intel_connector.h
>> @@ -31,5 +31,6 @@ void intel_attach_force_audio_property(struct drm_connector *connector);
>> void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
>> void intel_attach_aspect_ratio_property(struct drm_connector *connector);
>> void intel_attach_colorspace_property(struct drm_connector *connector);
>> +void intel_attach_privacy_screen_property(struct drm_connector *connector);
>>
>> #endif /* __INTEL_CONNECTOR_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index c2706afc069b..83b8c98049a7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -426,6 +426,9 @@ struct intel_connector {
>> struct work_struct modeset_retry_work;
>>
>> struct intel_hdcp hdcp;
>> +
>> + /* Optional "privacy-screen" property for the connector panel */
>> + struct drm_property *privacy_screen_property;
>> };
>>
>> struct intel_digital_connector_state {
>> @@ -433,6 +436,7 @@ struct intel_digital_connector_state {
>>
>> enum hdmi_force_audio force_audio;
>> int broadcast_rgb;
>> + enum intel_privacy_screen_status privacy_screen_status;
>> };
>>
>> #define to_intel_digital_connector_state(x) container_of(x, struct intel_digital_connector_state, base)
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 4fac408a4299..1963e92404ba 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"
>> @@ -6358,6 +6359,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>>
>> /* Lookup the ACPI node corresponding to the connector */
>> intel_connector_lookup_acpi_node(intel_connector);
>> +
>> + /* Check for integrated Privacy screen support */
>> + if (intel_privacy_screen_present(intel_connector))
>> + intel_attach_privacy_screen_property(connector);
>> }
>> }
>>
>> 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 000000000000..4c422e38c51a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Intel ACPI privacy screen code
>> + *
>> + * Copyright  2019 Google Inc.
>> + */
>> +
>> +#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(acpi_handle conn_handle, u64 func)
>> +{
>> + union acpi_object *obj;
>> +
>> + obj = acpi_evaluate_dsm(conn_handle, &drm_conn_dsm_guid,
>> + CONNECTOR_DSM_REVID, func, NULL);
>> + if (!obj) {
>> + DRM_DEBUG_DRIVER("failed to evaluate _DSM for fn %llx\n", func);
>> + return;
>> + }
>> +
>> + ACPI_FREE(obj);
>> +}
>> +
>> +void intel_privacy_screen_set_val(struct intel_connector *intel_connector,
>> + enum intel_privacy_screen_status val)
>> +{
>> + acpi_handle acpi_handle = intel_connector->acpi_handle;
>> +
>> + if (!acpi_handle)
>> + return;
>> +
>> + if (val == PRIVACY_SCREEN_DISABLED)
>> + acpi_privacy_screen_call_dsm(acpi_handle,
>> + CONNECTOR_DSM_FN_PRIVACY_DISABLE);
>> + else if (val == PRIVACY_SCREEN_ENABLED)
>> + acpi_privacy_screen_call_dsm(acpi_handle,
>> + CONNECTOR_DSM_FN_PRIVACY_ENABLE);
>> +}
>> +
>> +bool intel_privacy_screen_present(struct intel_connector *intel_connector)
>> +{
>> + acpi_handle handle = intel_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_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
>> + dev_name(intel_connector->base.dev->dev));
>> + return false;
>> + }
>> + DRM_DEV_INFO(intel_connector->base.dev->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 000000000000..212f73349a00
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * 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 *intel_connector);
>> +void intel_privacy_screen_set_val(struct intel_connector *intel_connector,
>> + enum intel_privacy_screen_status val);
>> +#else
>> +bool intel_privacy_screen_present(struct intel_connector *intel_connector);
>> +{
>> + return false;
>> +}
>> +void intel_privacy_screen_set_val(struct intel_connector *intel_connector,
>> + enum intel_privacy_screen_status val)
>> +{ }
>> +#endif /* CONFIG_ACPI */
>> +
>> +#endif /* __DRM_PRIVACY_SCREEN_H__ */
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 469dc512cca3..cf08d5636363 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -2123,6 +2123,20 @@ struct drm_i915_query_engine_info {
>> struct drm_i915_engine_info engines[];
>> };
>>
>> +/**
>> + * enum intel_privacy_screen_status - privacy_screen status
>> + *
>> + * This enum is used to track and control the state of the integrated privacy
>> + * screen present on some display panels, via the "privacy-screen" property.
>> + *
>> + * @PRIVACY_SCREEN_DISABLED: The privacy-screen on the panel is disabled
>> + * @PRIVACY_SCREEN_ENABLED: The privacy-screen on the panel is enabled
>> + **/
>> +enum intel_privacy_screen_status {
>> + PRIVACY_SCREEN_DISABLED = 0,
>> + PRIVACY_SCREEN_ENABLED = 1,
>> +};
>> +
>> #if defined(__cplusplus)
>> }
>> #endif
>> --
>> 2.24.0.rc1.363.gb1bccd3e3d-goog
>>

--
Jani Nikula, Intel Open Source Graphics Center