* [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi
@ 2019-12-20 20:03 Rajat Jain
2019-12-20 20:03 ` [PATCH v5 2/3] drm/i915: Lookup and attach ACPI device node for connectors Rajat Jain
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Rajat Jain @ 2019-12-20 20:03 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
linux-kernel, dri-devel, intel-gfx, gregkh, mathewk,
Daniel Thompson, Jonathan Corbet, Pavel Machek, seanpaul,
Duncan Laurie, jsbarnes, Thierry Reding
Cc: Rajat Jain, rajatxjain
Move the code that populates the ACPI device ID for devices, into
more appripriate intel_acpi.c. This is done in preparation for more
users of this code (in next patch).
Signed-off-by: Rajat Jain <rajatja@google.com>
---
v5: same as v4
v4: Same as v3
v3: * Renamed the function to intel_acpi_*
* Used forward declaration for structure instead of header file inclusion.
* Fix a typo
v2: v1 doesn't exist. Found existing code in i915 driver to assign the ACPI ID
which is what I plan to re-use.
drivers/gpu/drm/i915/display/intel_acpi.c | 89 +++++++++++++++++++
drivers/gpu/drm/i915/display/intel_acpi.h | 5 ++
drivers/gpu/drm/i915/display/intel_opregion.c | 80 +----------------
3 files changed, 98 insertions(+), 76 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
index 3456d33feb46..e21fb14d5e07 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.c
+++ b/drivers/gpu/drm/i915/display/intel_acpi.c
@@ -10,6 +10,7 @@
#include "i915_drv.h"
#include "intel_acpi.h"
+#include "intel_display_types.h"
#define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
#define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
@@ -156,3 +157,91 @@ void intel_register_dsm_handler(void)
void intel_unregister_dsm_handler(void)
{
}
+
+/*
+ * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
+ * Attached to the Display Adapter).
+ */
+#define ACPI_DISPLAY_INDEX_SHIFT 0
+#define ACPI_DISPLAY_INDEX_MASK (0xf << 0)
+#define ACPI_DISPLAY_PORT_ATTACHMENT_SHIFT 4
+#define ACPI_DISPLAY_PORT_ATTACHMENT_MASK (0xf << 4)
+#define ACPI_DISPLAY_TYPE_SHIFT 8
+#define ACPI_DISPLAY_TYPE_MASK (0xf << 8)
+#define ACPI_DISPLAY_TYPE_OTHER (0 << 8)
+#define ACPI_DISPLAY_TYPE_VGA (1 << 8)
+#define ACPI_DISPLAY_TYPE_TV (2 << 8)
+#define ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL (3 << 8)
+#define ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL (4 << 8)
+#define ACPI_VENDOR_SPECIFIC_SHIFT 12
+#define ACPI_VENDOR_SPECIFIC_MASK (0xf << 12)
+#define ACPI_BIOS_CAN_DETECT (1 << 16)
+#define ACPI_DEPENDS_ON_VGA (1 << 17)
+#define ACPI_PIPE_ID_SHIFT 18
+#define ACPI_PIPE_ID_MASK (7 << 18)
+#define ACPI_DEVICE_ID_SCHEME (1ULL << 31)
+
+static u32 acpi_display_type(struct intel_connector *connector)
+{
+ u32 display_type;
+
+ switch (connector->base.connector_type) {
+ case DRM_MODE_CONNECTOR_VGA:
+ case DRM_MODE_CONNECTOR_DVIA:
+ display_type = ACPI_DISPLAY_TYPE_VGA;
+ break;
+ case DRM_MODE_CONNECTOR_Composite:
+ case DRM_MODE_CONNECTOR_SVIDEO:
+ case DRM_MODE_CONNECTOR_Component:
+ case DRM_MODE_CONNECTOR_9PinDIN:
+ case DRM_MODE_CONNECTOR_TV:
+ display_type = ACPI_DISPLAY_TYPE_TV;
+ break;
+ case DRM_MODE_CONNECTOR_DVII:
+ case DRM_MODE_CONNECTOR_DVID:
+ case DRM_MODE_CONNECTOR_DisplayPort:
+ case DRM_MODE_CONNECTOR_HDMIA:
+ case DRM_MODE_CONNECTOR_HDMIB:
+ display_type = ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL;
+ break;
+ case DRM_MODE_CONNECTOR_LVDS:
+ case DRM_MODE_CONNECTOR_eDP:
+ case DRM_MODE_CONNECTOR_DSI:
+ display_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
+ break;
+ case DRM_MODE_CONNECTOR_Unknown:
+ case DRM_MODE_CONNECTOR_VIRTUAL:
+ display_type = ACPI_DISPLAY_TYPE_OTHER;
+ break;
+ default:
+ MISSING_CASE(connector->base.connector_type);
+ display_type = ACPI_DISPLAY_TYPE_OTHER;
+ break;
+ }
+
+ return display_type;
+}
+
+void intel_acpi_device_id_update(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *drm_dev = &dev_priv->drm;
+ struct intel_connector *connector;
+ struct drm_connector_list_iter conn_iter;
+ u8 display_index[16] = {};
+
+ /* Populate the ACPI IDs for all connectors for a given drm_device */
+ drm_connector_list_iter_begin(drm_dev, &conn_iter);
+ for_each_intel_connector_iter(connector, &conn_iter) {
+ u32 device_id, type;
+
+ device_id = acpi_display_type(connector);
+
+ /* Use display type specific display index. */
+ type = (device_id & ACPI_DISPLAY_TYPE_MASK)
+ >> ACPI_DISPLAY_TYPE_SHIFT;
+ device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
+
+ connector->acpi_device_id = device_id;
+ }
+ drm_connector_list_iter_end(&conn_iter);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h
index 1c576b3fb712..e8b068661d22 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.h
+++ b/drivers/gpu/drm/i915/display/intel_acpi.h
@@ -6,12 +6,17 @@
#ifndef __INTEL_ACPI_H__
#define __INTEL_ACPI_H__
+struct drm_i915_private;
+
#ifdef CONFIG_ACPI
void intel_register_dsm_handler(void);
void intel_unregister_dsm_handler(void);
+void intel_acpi_device_id_update(struct drm_i915_private *i915);
#else
static inline void intel_register_dsm_handler(void) { return; }
static inline void intel_unregister_dsm_handler(void) { return; }
+static inline
+void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; }
#endif /* CONFIG_ACPI */
#endif /* __INTEL_ACPI_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
index 969ade623691..6422384f199e 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -35,6 +35,7 @@
#include "display/intel_panel.h"
#include "i915_drv.h"
+#include "intel_acpi.h"
#include "intel_display_types.h"
#include "intel_opregion.h"
@@ -242,29 +243,6 @@ struct opregion_asle_ext {
#define SWSCI_SBCB_POST_VBE_PM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
-/*
- * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
- * Attached to the Display Adapter).
- */
-#define ACPI_DISPLAY_INDEX_SHIFT 0
-#define ACPI_DISPLAY_INDEX_MASK (0xf << 0)
-#define ACPI_DISPLAY_PORT_ATTACHMENT_SHIFT 4
-#define ACPI_DISPLAY_PORT_ATTACHMENT_MASK (0xf << 4)
-#define ACPI_DISPLAY_TYPE_SHIFT 8
-#define ACPI_DISPLAY_TYPE_MASK (0xf << 8)
-#define ACPI_DISPLAY_TYPE_OTHER (0 << 8)
-#define ACPI_DISPLAY_TYPE_VGA (1 << 8)
-#define ACPI_DISPLAY_TYPE_TV (2 << 8)
-#define ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL (3 << 8)
-#define ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL (4 << 8)
-#define ACPI_VENDOR_SPECIFIC_SHIFT 12
-#define ACPI_VENDOR_SPECIFIC_MASK (0xf << 12)
-#define ACPI_BIOS_CAN_DETECT (1 << 16)
-#define ACPI_DEPENDS_ON_VGA (1 << 17)
-#define ACPI_PIPE_ID_SHIFT 18
-#define ACPI_PIPE_ID_MASK (7 << 18)
-#define ACPI_DEVICE_ID_SCHEME (1 << 31)
-
#define MAX_DSLP 1500
static int swsci(struct drm_i915_private *dev_priv,
@@ -662,54 +640,12 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
}
}
-static u32 acpi_display_type(struct intel_connector *connector)
-{
- u32 display_type;
-
- switch (connector->base.connector_type) {
- case DRM_MODE_CONNECTOR_VGA:
- case DRM_MODE_CONNECTOR_DVIA:
- display_type = ACPI_DISPLAY_TYPE_VGA;
- break;
- case DRM_MODE_CONNECTOR_Composite:
- case DRM_MODE_CONNECTOR_SVIDEO:
- case DRM_MODE_CONNECTOR_Component:
- case DRM_MODE_CONNECTOR_9PinDIN:
- case DRM_MODE_CONNECTOR_TV:
- display_type = ACPI_DISPLAY_TYPE_TV;
- break;
- case DRM_MODE_CONNECTOR_DVII:
- case DRM_MODE_CONNECTOR_DVID:
- case DRM_MODE_CONNECTOR_DisplayPort:
- case DRM_MODE_CONNECTOR_HDMIA:
- case DRM_MODE_CONNECTOR_HDMIB:
- display_type = ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL;
- break;
- case DRM_MODE_CONNECTOR_LVDS:
- case DRM_MODE_CONNECTOR_eDP:
- case DRM_MODE_CONNECTOR_DSI:
- display_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
- break;
- case DRM_MODE_CONNECTOR_Unknown:
- case DRM_MODE_CONNECTOR_VIRTUAL:
- display_type = ACPI_DISPLAY_TYPE_OTHER;
- break;
- default:
- MISSING_CASE(connector->base.connector_type);
- display_type = ACPI_DISPLAY_TYPE_OTHER;
- break;
- }
-
- return display_type;
-}
-
static void intel_didl_outputs(struct drm_i915_private *dev_priv)
{
struct intel_opregion *opregion = &dev_priv->opregion;
struct intel_connector *connector;
struct drm_connector_list_iter conn_iter;
int i = 0, max_outputs;
- int display_index[16] = {};
/*
* In theory, did2, the extended didl, gets added at opregion version
@@ -721,20 +657,12 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
max_outputs = ARRAY_SIZE(opregion->acpi->didl) +
ARRAY_SIZE(opregion->acpi->did2);
+ intel_acpi_device_id_update(dev_priv);
+
drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
for_each_intel_connector_iter(connector, &conn_iter) {
- u32 device_id, type;
-
- device_id = acpi_display_type(connector);
-
- /* Use display type specific display index. */
- type = (device_id & ACPI_DISPLAY_TYPE_MASK)
- >> ACPI_DISPLAY_TYPE_SHIFT;
- device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
-
- connector->acpi_device_id = device_id;
if (i < max_outputs)
- set_did(opregion, i, device_id);
+ set_did(opregion, i, connector->acpi_device_id);
i++;
}
drm_connector_list_iter_end(&conn_iter);
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v5 2/3] drm/i915: Lookup and attach ACPI device node for connectors
2019-12-20 20:03 [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi Rajat Jain
@ 2019-12-20 20:03 ` Rajat Jain
2020-01-24 11:37 ` Jani Nikula
2019-12-20 20:03 ` [PATCH v5 3/3] drm/i915: Add support for integrated privacy screens Rajat Jain
2020-01-24 11:03 ` [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi Jani Nikula
2 siblings, 1 reply; 9+ messages in thread
From: Rajat Jain @ 2019-12-20 20:03 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
linux-kernel, dri-devel, intel-gfx, gregkh, mathewk,
Daniel Thompson, Jonathan Corbet, Pavel Machek, seanpaul,
Duncan Laurie, jsbarnes, Thierry Reding
Cc: Rajat Jain, rajatxjain
Lookup and attach ACPI nodes for intel connectors. The lookup is done
in compliance with ACPI Spec 6.3
https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
(Ref: Pages 1119 - 1123).
This can be useful for any connector specific platform properties. (This
will be used for privacy screen in next patch).
Signed-off-by: Rajat Jain <rajatja@google.com>
---
v5: same as v4
v4: Same as v3
v3: fold the code into existing acpi_device_id_update() function
v2: formed by splitting the original patch into ACPI lookup, and privacy
screen property. Also move it into i915 now that I found existing code
in i915 that can be re-used.
drivers/gpu/drm/i915/display/intel_acpi.c | 24 +++++++++++++++++++
.../drm/i915/display/intel_display_types.h | 3 +++
drivers/gpu/drm/i915/display/intel_dp.c | 3 +++
3 files changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
index e21fb14d5e07..101a56c08996 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.c
+++ b/drivers/gpu/drm/i915/display/intel_acpi.c
@@ -222,11 +222,23 @@ static u32 acpi_display_type(struct intel_connector *connector)
return display_type;
}
+/*
+ * Ref: ACPI Spec 6.3
+ * https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
+ * Pages 1119 - 1123 describe, what I believe, a standard way of
+ * identifying / addressing "display panels" in the ACPI. It provides
+ * a way for the ACPI to define devices for the display panels attached
+ * to the system. It thus provides a way for the BIOS to export any panel
+ * specific properties to the system via ACPI (like device trees).
+ */
void intel_acpi_device_id_update(struct drm_i915_private *dev_priv)
{
struct drm_device *drm_dev = &dev_priv->drm;
struct intel_connector *connector;
struct drm_connector_list_iter conn_iter;
+ struct device *dev = &drm_dev->pdev->dev;
+ struct acpi_device *conn_dev;
+ u64 conn_addr;
u8 display_index[16] = {};
/* Populate the ACPI IDs for all connectors for a given drm_device */
@@ -242,6 +254,18 @@ void intel_acpi_device_id_update(struct drm_i915_private *dev_priv)
device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
connector->acpi_device_id = device_id;
+
+ /* Build the _ADR to look for */
+ conn_addr = device_id | ACPI_DEVICE_ID_SCHEME |
+ ACPI_BIOS_CAN_DETECT;
+
+ DRM_DEV_INFO(dev, "Checking connector ACPI node at _ADR=%llX\n",
+ conn_addr);
+
+ /* Look up the connector device, under the PCI device */
+ conn_dev = acpi_find_child_device(ACPI_COMPANION(dev),
+ conn_addr, false);
+ connector->acpi_handle = conn_dev ? conn_dev->handle : NULL;
}
drm_connector_list_iter_end(&conn_iter);
}
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 1a7334dbe802..0a4a04116091 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -407,6 +407,9 @@ struct intel_connector {
/* ACPI device id for ACPI and driver cooperation */
u32 acpi_device_id;
+ /* ACPI handle corresponding to this connector display, if found */
+ void *acpi_handle;
+
/* Reads out the current hw, returning true if the connector is enabled
* and active (i.e. dpms ON state). */
bool (*get_hw_state)(struct intel_connector *);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index b05b2191b919..93cece8e2516 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -45,6 +45,7 @@
#include "i915_debugfs.h"
#include "i915_drv.h"
#include "i915_trace.h"
+#include "intel_acpi.h"
#include "intel_atomic.h"
#include "intel_audio.h"
#include "intel_connector.h"
@@ -6623,6 +6624,8 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;
+ /* Lookup the ACPI node corresponding to the connector */
+ intel_acpi_device_id_update(dev_priv);
}
}
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v5 3/3] drm/i915: Add support for integrated privacy screens
2019-12-20 20:03 [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi Rajat Jain
2019-12-20 20:03 ` [PATCH v5 2/3] drm/i915: Lookup and attach ACPI device node for connectors Rajat Jain
@ 2019-12-20 20:03 ` Rajat Jain
2020-01-21 7:25 ` Rajat Jain
2020-01-25 0:11 ` [v5,3/3] " Guenter Roeck
2020-01-24 11:03 ` [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi Jani Nikula
2 siblings, 2 replies; 9+ messages in thread
From: Rajat Jain @ 2019-12-20 20:03 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
linux-kernel, dri-devel, intel-gfx, gregkh, mathewk,
Daniel Thompson, Jonathan Corbet, Pavel Machek, seanpaul,
Duncan Laurie, jsbarnes, Thierry Reding
Cc: Rajat Jain, rajatxjain
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@google.com>
---
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 | 13 +++-
.../gpu/drm/i915/display/intel_connector.c | 35 +++++++++
.../gpu/drm/i915/display/intel_connector.h | 1 +
.../drm/i915/display/intel_display_types.h | 18 +++++
drivers/gpu/drm/i915/display/intel_dp.c | 6 ++
.../drm/i915/display/intel_privacy_screen.c | 72 +++++++++++++++++++
.../drm/i915/display/intel_privacy_screen.h | 27 +++++++
8 files changed, 171 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 90dcf09f52cc..f7067c8f0407 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -197,7 +197,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 c2875b10adf9..c73b81c4c3f6 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 1133c4e97bb4..f3e041c737de 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -296,3 +296,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 0a4a04116091..a0addd2c5376 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -433,6 +433,23 @@ 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;
+};
+
+/**
+ * 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,
};
struct intel_digital_connector_state {
@@ -440,6 +457,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 93cece8e2516..d5376d667929 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"
@@ -6596,6 +6597,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
{
struct drm_i915_private *dev_priv = to_i915(connector->dev);
enum port port = dp_to_dig_port(intel_dp)->base.port;
+ struct intel_connector *intel_connector = to_intel_connector(connector);
if (!IS_G4X(dev_priv) && port != PORT_A)
intel_attach_force_audio_property(connector);
@@ -6626,6 +6628,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
/* Lookup the ACPI node corresponding to the connector */
intel_acpi_device_id_update(dev_priv);
+
+ /* 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..c8a5b64f94fb
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * 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 *connector,
+ enum intel_privacy_screen_status val)
+{
+ acpi_handle acpi_handle = 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);
+ else
+ DRM_WARN("%s: Cannot set privacy screen to invalid val %u\n",
+ dev_name(connector->base.dev->dev), 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_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
+ dev_name(connector->base.dev->dev));
+ return false;
+ }
+ DRM_DEV_INFO(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..74013a7885c7
--- /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 intel_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 intel_privacy_screen_status val)
+{ }
+#endif /* CONFIG_ACPI */
+
+#endif /* __DRM_PRIVACY_SCREEN_H__ */
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 3/3] drm/i915: Add support for integrated privacy screens
2019-12-20 20:03 ` [PATCH v5 3/3] drm/i915: Add support for integrated privacy screens Rajat Jain
@ 2020-01-21 7:25 ` Rajat Jain
2020-01-25 0:11 ` [v5,3/3] " Guenter Roeck
1 sibling, 0 replies; 9+ messages in thread
From: Rajat Jain @ 2020-01-21 7:25 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
Linux Kernel Mailing List, dri-devel, intel-gfx,
Greg Kroah-Hartman, Mat King, Daniel Thompson, Jonathan Corbet,
Pavel Machek, Sean Paul, Duncan Laurie, Jesse Barnes,
Thierry Reding
Cc: Rajat Jain
Hello Jani,
On Fri, Dec 20, 2019 at 12:04 PM Rajat Jain <rajatja@google.com> 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@google.com>
I was wondering if you got a chance to look at this patchset. I'd
appreciate it if you could please take a look and provide your
comments.
Thanks & Best Regards,
Rajat Jain
> ---
> 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 | 13 +++-
> .../gpu/drm/i915/display/intel_connector.c | 35 +++++++++
> .../gpu/drm/i915/display/intel_connector.h | 1 +
> .../drm/i915/display/intel_display_types.h | 18 +++++
> drivers/gpu/drm/i915/display/intel_dp.c | 6 ++
> .../drm/i915/display/intel_privacy_screen.c | 72 +++++++++++++++++++
> .../drm/i915/display/intel_privacy_screen.h | 27 +++++++
> 8 files changed, 171 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 90dcf09f52cc..f7067c8f0407 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -197,7 +197,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 c2875b10adf9..c73b81c4c3f6 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 1133c4e97bb4..f3e041c737de 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> @@ -296,3 +296,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 0a4a04116091..a0addd2c5376 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -433,6 +433,23 @@ 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;
> +};
> +
> +/**
> + * 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,
> };
>
> struct intel_digital_connector_state {
> @@ -440,6 +457,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 93cece8e2516..d5376d667929 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"
> @@ -6596,6 +6597,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> {
> struct drm_i915_private *dev_priv = to_i915(connector->dev);
> enum port port = dp_to_dig_port(intel_dp)->base.port;
> + struct intel_connector *intel_connector = to_intel_connector(connector);
>
> if (!IS_G4X(dev_priv) && port != PORT_A)
> intel_attach_force_audio_property(connector);
> @@ -6626,6 +6628,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>
> /* Lookup the ACPI node corresponding to the connector */
> intel_acpi_device_id_update(dev_priv);
> +
> + /* 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..c8a5b64f94fb
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * 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 *connector,
> + enum intel_privacy_screen_status val)
> +{
> + acpi_handle acpi_handle = 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);
> + else
> + DRM_WARN("%s: Cannot set privacy screen to invalid val %u\n",
> + dev_name(connector->base.dev->dev), 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_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
> + dev_name(connector->base.dev->dev));
> + return false;
> + }
> + DRM_DEV_INFO(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..74013a7885c7
> --- /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 intel_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 intel_privacy_screen_status val)
> +{ }
> +#endif /* CONFIG_ACPI */
> +
> +#endif /* __DRM_PRIVACY_SCREEN_H__ */
> --
> 2.24.1.735.g03f4e72817-goog
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi
2019-12-20 20:03 [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi Rajat Jain
2019-12-20 20:03 ` [PATCH v5 2/3] drm/i915: Lookup and attach ACPI device node for connectors Rajat Jain
2019-12-20 20:03 ` [PATCH v5 3/3] drm/i915: Add support for integrated privacy screens Rajat Jain
@ 2020-01-24 11:03 ` Jani Nikula
2 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2020-01-24 11:03 UTC (permalink / raw)
To: Rajat Jain, Maarten Lankhorst, Maxime Ripard, Sean Paul,
David Airlie, Daniel Vetter, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
linux-kernel, dri-devel, intel-gfx, gregkh, mathewk,
Daniel Thompson, Jonathan Corbet, Pavel Machek, seanpaul,
Duncan Laurie, jsbarnes, Thierry Reding
Cc: Rajat Jain, rajatxjain
On Fri, 20 Dec 2019, Rajat Jain <rajatja@google.com> wrote:
> Move the code that populates the ACPI device ID for devices, into
> more appripriate intel_acpi.c. This is done in preparation for more
> users of this code (in next patch).
Sorry for the delay, thanks for the patch, pushed to
drm-intel-next-queued.
BR,
Jani.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v5: same as v4
> v4: Same as v3
> v3: * Renamed the function to intel_acpi_*
> * Used forward declaration for structure instead of header file inclusion.
> * Fix a typo
> v2: v1 doesn't exist. Found existing code in i915 driver to assign the ACPI ID
> which is what I plan to re-use.
>
> drivers/gpu/drm/i915/display/intel_acpi.c | 89 +++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_acpi.h | 5 ++
> drivers/gpu/drm/i915/display/intel_opregion.c | 80 +----------------
> 3 files changed, 98 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> index 3456d33feb46..e21fb14d5e07 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -10,6 +10,7 @@
>
> #include "i915_drv.h"
> #include "intel_acpi.h"
> +#include "intel_display_types.h"
>
> #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */
> #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */
> @@ -156,3 +157,91 @@ void intel_register_dsm_handler(void)
> void intel_unregister_dsm_handler(void)
> {
> }
> +
> +/*
> + * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
> + * Attached to the Display Adapter).
> + */
> +#define ACPI_DISPLAY_INDEX_SHIFT 0
> +#define ACPI_DISPLAY_INDEX_MASK (0xf << 0)
> +#define ACPI_DISPLAY_PORT_ATTACHMENT_SHIFT 4
> +#define ACPI_DISPLAY_PORT_ATTACHMENT_MASK (0xf << 4)
> +#define ACPI_DISPLAY_TYPE_SHIFT 8
> +#define ACPI_DISPLAY_TYPE_MASK (0xf << 8)
> +#define ACPI_DISPLAY_TYPE_OTHER (0 << 8)
> +#define ACPI_DISPLAY_TYPE_VGA (1 << 8)
> +#define ACPI_DISPLAY_TYPE_TV (2 << 8)
> +#define ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL (3 << 8)
> +#define ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL (4 << 8)
> +#define ACPI_VENDOR_SPECIFIC_SHIFT 12
> +#define ACPI_VENDOR_SPECIFIC_MASK (0xf << 12)
> +#define ACPI_BIOS_CAN_DETECT (1 << 16)
> +#define ACPI_DEPENDS_ON_VGA (1 << 17)
> +#define ACPI_PIPE_ID_SHIFT 18
> +#define ACPI_PIPE_ID_MASK (7 << 18)
> +#define ACPI_DEVICE_ID_SCHEME (1ULL << 31)
> +
> +static u32 acpi_display_type(struct intel_connector *connector)
> +{
> + u32 display_type;
> +
> + switch (connector->base.connector_type) {
> + case DRM_MODE_CONNECTOR_VGA:
> + case DRM_MODE_CONNECTOR_DVIA:
> + display_type = ACPI_DISPLAY_TYPE_VGA;
> + break;
> + case DRM_MODE_CONNECTOR_Composite:
> + case DRM_MODE_CONNECTOR_SVIDEO:
> + case DRM_MODE_CONNECTOR_Component:
> + case DRM_MODE_CONNECTOR_9PinDIN:
> + case DRM_MODE_CONNECTOR_TV:
> + display_type = ACPI_DISPLAY_TYPE_TV;
> + break;
> + case DRM_MODE_CONNECTOR_DVII:
> + case DRM_MODE_CONNECTOR_DVID:
> + case DRM_MODE_CONNECTOR_DisplayPort:
> + case DRM_MODE_CONNECTOR_HDMIA:
> + case DRM_MODE_CONNECTOR_HDMIB:
> + display_type = ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL;
> + break;
> + case DRM_MODE_CONNECTOR_LVDS:
> + case DRM_MODE_CONNECTOR_eDP:
> + case DRM_MODE_CONNECTOR_DSI:
> + display_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
> + break;
> + case DRM_MODE_CONNECTOR_Unknown:
> + case DRM_MODE_CONNECTOR_VIRTUAL:
> + display_type = ACPI_DISPLAY_TYPE_OTHER;
> + break;
> + default:
> + MISSING_CASE(connector->base.connector_type);
> + display_type = ACPI_DISPLAY_TYPE_OTHER;
> + break;
> + }
> +
> + return display_type;
> +}
> +
> +void intel_acpi_device_id_update(struct drm_i915_private *dev_priv)
> +{
> + struct drm_device *drm_dev = &dev_priv->drm;
> + struct intel_connector *connector;
> + struct drm_connector_list_iter conn_iter;
> + u8 display_index[16] = {};
> +
> + /* Populate the ACPI IDs for all connectors for a given drm_device */
> + drm_connector_list_iter_begin(drm_dev, &conn_iter);
> + for_each_intel_connector_iter(connector, &conn_iter) {
> + u32 device_id, type;
> +
> + device_id = acpi_display_type(connector);
> +
> + /* Use display type specific display index. */
> + type = (device_id & ACPI_DISPLAY_TYPE_MASK)
> + >> ACPI_DISPLAY_TYPE_SHIFT;
> + device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
> +
> + connector->acpi_device_id = device_id;
> + }
> + drm_connector_list_iter_end(&conn_iter);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h
> index 1c576b3fb712..e8b068661d22 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.h
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.h
> @@ -6,12 +6,17 @@
> #ifndef __INTEL_ACPI_H__
> #define __INTEL_ACPI_H__
>
> +struct drm_i915_private;
> +
> #ifdef CONFIG_ACPI
> void intel_register_dsm_handler(void);
> void intel_unregister_dsm_handler(void);
> +void intel_acpi_device_id_update(struct drm_i915_private *i915);
> #else
> static inline void intel_register_dsm_handler(void) { return; }
> static inline void intel_unregister_dsm_handler(void) { return; }
> +static inline
> +void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; }
> #endif /* CONFIG_ACPI */
>
> #endif /* __INTEL_ACPI_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> index 969ade623691..6422384f199e 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -35,6 +35,7 @@
> #include "display/intel_panel.h"
>
> #include "i915_drv.h"
> +#include "intel_acpi.h"
> #include "intel_display_types.h"
> #include "intel_opregion.h"
>
> @@ -242,29 +243,6 @@ struct opregion_asle_ext {
> #define SWSCI_SBCB_POST_VBE_PM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
> #define SWSCI_SBCB_ENABLE_DISABLE_AUDIO SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
>
> -/*
> - * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
> - * Attached to the Display Adapter).
> - */
> -#define ACPI_DISPLAY_INDEX_SHIFT 0
> -#define ACPI_DISPLAY_INDEX_MASK (0xf << 0)
> -#define ACPI_DISPLAY_PORT_ATTACHMENT_SHIFT 4
> -#define ACPI_DISPLAY_PORT_ATTACHMENT_MASK (0xf << 4)
> -#define ACPI_DISPLAY_TYPE_SHIFT 8
> -#define ACPI_DISPLAY_TYPE_MASK (0xf << 8)
> -#define ACPI_DISPLAY_TYPE_OTHER (0 << 8)
> -#define ACPI_DISPLAY_TYPE_VGA (1 << 8)
> -#define ACPI_DISPLAY_TYPE_TV (2 << 8)
> -#define ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL (3 << 8)
> -#define ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL (4 << 8)
> -#define ACPI_VENDOR_SPECIFIC_SHIFT 12
> -#define ACPI_VENDOR_SPECIFIC_MASK (0xf << 12)
> -#define ACPI_BIOS_CAN_DETECT (1 << 16)
> -#define ACPI_DEPENDS_ON_VGA (1 << 17)
> -#define ACPI_PIPE_ID_SHIFT 18
> -#define ACPI_PIPE_ID_MASK (7 << 18)
> -#define ACPI_DEVICE_ID_SCHEME (1 << 31)
> -
> #define MAX_DSLP 1500
>
> static int swsci(struct drm_i915_private *dev_priv,
> @@ -662,54 +640,12 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
> }
> }
>
> -static u32 acpi_display_type(struct intel_connector *connector)
> -{
> - u32 display_type;
> -
> - switch (connector->base.connector_type) {
> - case DRM_MODE_CONNECTOR_VGA:
> - case DRM_MODE_CONNECTOR_DVIA:
> - display_type = ACPI_DISPLAY_TYPE_VGA;
> - break;
> - case DRM_MODE_CONNECTOR_Composite:
> - case DRM_MODE_CONNECTOR_SVIDEO:
> - case DRM_MODE_CONNECTOR_Component:
> - case DRM_MODE_CONNECTOR_9PinDIN:
> - case DRM_MODE_CONNECTOR_TV:
> - display_type = ACPI_DISPLAY_TYPE_TV;
> - break;
> - case DRM_MODE_CONNECTOR_DVII:
> - case DRM_MODE_CONNECTOR_DVID:
> - case DRM_MODE_CONNECTOR_DisplayPort:
> - case DRM_MODE_CONNECTOR_HDMIA:
> - case DRM_MODE_CONNECTOR_HDMIB:
> - display_type = ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL;
> - break;
> - case DRM_MODE_CONNECTOR_LVDS:
> - case DRM_MODE_CONNECTOR_eDP:
> - case DRM_MODE_CONNECTOR_DSI:
> - display_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
> - break;
> - case DRM_MODE_CONNECTOR_Unknown:
> - case DRM_MODE_CONNECTOR_VIRTUAL:
> - display_type = ACPI_DISPLAY_TYPE_OTHER;
> - break;
> - default:
> - MISSING_CASE(connector->base.connector_type);
> - display_type = ACPI_DISPLAY_TYPE_OTHER;
> - break;
> - }
> -
> - return display_type;
> -}
> -
> static void intel_didl_outputs(struct drm_i915_private *dev_priv)
> {
> struct intel_opregion *opregion = &dev_priv->opregion;
> struct intel_connector *connector;
> struct drm_connector_list_iter conn_iter;
> int i = 0, max_outputs;
> - int display_index[16] = {};
>
> /*
> * In theory, did2, the extended didl, gets added at opregion version
> @@ -721,20 +657,12 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
> max_outputs = ARRAY_SIZE(opregion->acpi->didl) +
> ARRAY_SIZE(opregion->acpi->did2);
>
> + intel_acpi_device_id_update(dev_priv);
> +
> drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> for_each_intel_connector_iter(connector, &conn_iter) {
> - u32 device_id, type;
> -
> - device_id = acpi_display_type(connector);
> -
> - /* Use display type specific display index. */
> - type = (device_id & ACPI_DISPLAY_TYPE_MASK)
> - >> ACPI_DISPLAY_TYPE_SHIFT;
> - device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
> -
> - connector->acpi_device_id = device_id;
> if (i < max_outputs)
> - set_did(opregion, i, device_id);
> + set_did(opregion, i, connector->acpi_device_id);
> i++;
> }
> drm_connector_list_iter_end(&conn_iter);
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/3] drm/i915: Lookup and attach ACPI device node for connectors
2019-12-20 20:03 ` [PATCH v5 2/3] drm/i915: Lookup and attach ACPI device node for connectors Rajat Jain
@ 2020-01-24 11:37 ` Jani Nikula
2020-01-24 20:23 ` Rajat Jain
0 siblings, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2020-01-24 11:37 UTC (permalink / raw)
To: Rajat Jain, Maarten Lankhorst, Maxime Ripard, Sean Paul,
David Airlie, Daniel Vetter, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
linux-kernel, dri-devel, intel-gfx, gregkh, mathewk,
Daniel Thompson, Jonathan Corbet, Pavel Machek, seanpaul,
Duncan Laurie, jsbarnes, Thierry Reding
Cc: Rajat Jain, rajatxjain
On Fri, 20 Dec 2019, Rajat Jain <rajatja@google.com> wrote:
> Lookup and attach ACPI nodes for intel connectors. The lookup is done
> in compliance with ACPI Spec 6.3
> https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
> (Ref: Pages 1119 - 1123).
>
> This can be useful for any connector specific platform properties. (This
> will be used for privacy screen in next patch).
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v5: same as v4
> v4: Same as v3
> v3: fold the code into existing acpi_device_id_update() function
> v2: formed by splitting the original patch into ACPI lookup, and privacy
> screen property. Also move it into i915 now that I found existing code
> in i915 that can be re-used.
>
> drivers/gpu/drm/i915/display/intel_acpi.c | 24 +++++++++++++++++++
> .../drm/i915/display/intel_display_types.h | 3 +++
> drivers/gpu/drm/i915/display/intel_dp.c | 3 +++
> 3 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> index e21fb14d5e07..101a56c08996 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -222,11 +222,23 @@ static u32 acpi_display_type(struct intel_connector *connector)
> return display_type;
> }
>
> +/*
> + * Ref: ACPI Spec 6.3
> + * https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
> + * Pages 1119 - 1123 describe, what I believe, a standard way of
> + * identifying / addressing "display panels" in the ACPI. It provides
> + * a way for the ACPI to define devices for the display panels attached
> + * to the system. It thus provides a way for the BIOS to export any panel
> + * specific properties to the system via ACPI (like device trees).
> + */
> void intel_acpi_device_id_update(struct drm_i915_private *dev_priv)
> {
> struct drm_device *drm_dev = &dev_priv->drm;
> struct intel_connector *connector;
> struct drm_connector_list_iter conn_iter;
> + struct device *dev = &drm_dev->pdev->dev;
Hmm, already pushed patch 1 with the unfortunate "drm_dev" local. We use
"dev" for struct drm_device * and almost never use struct device.
> + struct acpi_device *conn_dev;
> + u64 conn_addr;
> u8 display_index[16] = {};
>
> /* Populate the ACPI IDs for all connectors for a given drm_device */
> @@ -242,6 +254,18 @@ void intel_acpi_device_id_update(struct drm_i915_private *dev_priv)
> device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
>
> connector->acpi_device_id = device_id;
> +
> + /* Build the _ADR to look for */
> + conn_addr = device_id | ACPI_DEVICE_ID_SCHEME |
> + ACPI_BIOS_CAN_DETECT;
> +
> + DRM_DEV_INFO(dev, "Checking connector ACPI node at _ADR=%llX\n",
> + conn_addr);
This is more than a little verbose. One line of INFO level dmesg for
every connector at boot and at resume.
Please use the new drm_dbg_kms() macro for this.
> +
> + /* Look up the connector device, under the PCI device */
> + conn_dev = acpi_find_child_device(ACPI_COMPANION(dev),
> + conn_addr, false);
> + connector->acpi_handle = conn_dev ? conn_dev->handle : NULL;
acpi_device_handle(conn_dev)
> }
> drm_connector_list_iter_end(&conn_iter);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 1a7334dbe802..0a4a04116091 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -407,6 +407,9 @@ struct intel_connector {
> /* ACPI device id for ACPI and driver cooperation */
> u32 acpi_device_id;
>
> + /* ACPI handle corresponding to this connector display, if found */
> + void *acpi_handle;
> +
The type is acpi_handle. It's none of our business to know what the
underlying type is.
> /* Reads out the current hw, returning true if the connector is enabled
> * and active (i.e. dpms ON state). */
> bool (*get_hw_state)(struct intel_connector *);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index b05b2191b919..93cece8e2516 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -45,6 +45,7 @@
> #include "i915_debugfs.h"
> #include "i915_drv.h"
> #include "i915_trace.h"
> +#include "intel_acpi.h"
> #include "intel_atomic.h"
> #include "intel_audio.h"
> #include "intel_connector.h"
> @@ -6623,6 +6624,8 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>
> connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;
>
> + /* Lookup the ACPI node corresponding to the connector */
> + intel_acpi_device_id_update(dev_priv);
Auch, this is problematic. It iterates all connectors, for every DP
connector being added. In the middle of registering all connectors.
From the POV of this patch alone, this is also unnecessary. This gets
called via intel_opregion_register() after all connectors have been
registered.
I am aware it's not enough for your next patch, because it will need the
acpi handle right here.
I'm wondering if we need to maintain display_index[] in struct
drm_i915_private, and update that as connectors get added instead of all
at once in the end. connector->acpi_device_id never changes, does it,
even though we keep updating it?
BR,
Jani.
> }
> }
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/3] drm/i915: Lookup and attach ACPI device node for connectors
2020-01-24 11:37 ` Jani Nikula
@ 2020-01-24 20:23 ` Rajat Jain
0 siblings, 0 replies; 9+ messages in thread
From: Rajat Jain @ 2020-01-24 20:23 UTC (permalink / raw)
To: Jani Nikula
Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
Linux Kernel Mailing List, dri-devel, intel-gfx,
Greg Kroah-Hartman, Mat King, Daniel Thompson, Jonathan Corbet,
Pavel Machek, Sean Paul, Duncan Laurie, Jesse Barnes,
Thierry Reding, Rajat Jain
Hi Jani,
Thank you for the review. Please see inline.
On Fri, Jan 24, 2020 at 3:37 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Fri, 20 Dec 2019, Rajat Jain <rajatja@google.com> wrote:
> > Lookup and attach ACPI nodes for intel connectors. The lookup is done
> > in compliance with ACPI Spec 6.3
> > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
> > (Ref: Pages 1119 - 1123).
> >
> > This can be useful for any connector specific platform properties. (This
> > will be used for privacy screen in next patch).
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > v5: same as v4
> > v4: Same as v3
> > v3: fold the code into existing acpi_device_id_update() function
> > v2: formed by splitting the original patch into ACPI lookup, and privacy
> > screen property. Also move it into i915 now that I found existing code
> > in i915 that can be re-used.
> >
> > drivers/gpu/drm/i915/display/intel_acpi.c | 24 +++++++++++++++++++
> > .../drm/i915/display/intel_display_types.h | 3 +++
> > drivers/gpu/drm/i915/display/intel_dp.c | 3 +++
> > 3 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
> > index e21fb14d5e07..101a56c08996 100644
> > --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> > @@ -222,11 +222,23 @@ static u32 acpi_display_type(struct intel_connector *connector)
> > return display_type;
> > }
> >
> > +/*
> > + * Ref: ACPI Spec 6.3
> > + * https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
> > + * Pages 1119 - 1123 describe, what I believe, a standard way of
> > + * identifying / addressing "display panels" in the ACPI. It provides
> > + * a way for the ACPI to define devices for the display panels attached
> > + * to the system. It thus provides a way for the BIOS to export any panel
> > + * specific properties to the system via ACPI (like device trees).
> > + */
> > void intel_acpi_device_id_update(struct drm_i915_private *dev_priv)
> > {
> > struct drm_device *drm_dev = &dev_priv->drm;
> > struct intel_connector *connector;
> > struct drm_connector_list_iter conn_iter;
> > + struct device *dev = &drm_dev->pdev->dev;
>
> Hmm, already pushed patch 1 with the unfortunate "drm_dev" local. We use
> "dev" for struct drm_device * and almost never use struct device.
Sorry, I did not know. I'll send an independent fixup patch for patch
1 on top of drm-intel-next-queued (or let me know if you want to
handle it). I will also change this patch to rename the variable.
>
> > + struct acpi_device *conn_dev;
> > + u64 conn_addr;
> > u8 display_index[16] = {};
> >
> > /* Populate the ACPI IDs for all connectors for a given drm_device */
> > @@ -242,6 +254,18 @@ void intel_acpi_device_id_update(struct drm_i915_private *dev_priv)
> > device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
> >
> > connector->acpi_device_id = device_id;
> > +
> > + /* Build the _ADR to look for */
> > + conn_addr = device_id | ACPI_DEVICE_ID_SCHEME |
> > + ACPI_BIOS_CAN_DETECT;
> > +
> > + DRM_DEV_INFO(dev, "Checking connector ACPI node at _ADR=%llX\n",
> > + conn_addr);
>
> This is more than a little verbose. One line of INFO level dmesg for
> every connector at boot and at resume.
>
> Please use the new drm_dbg_kms() macro for this.
Will do.
>
> > +
> > + /* Look up the connector device, under the PCI device */
> > + conn_dev = acpi_find_child_device(ACPI_COMPANION(dev),
> > + conn_addr, false);
> > + connector->acpi_handle = conn_dev ? conn_dev->handle : NULL;
>
> acpi_device_handle(conn_dev)
>
Will do.
> > }
> > drm_connector_list_iter_end(&conn_iter);
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 1a7334dbe802..0a4a04116091 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -407,6 +407,9 @@ struct intel_connector {
> > /* ACPI device id for ACPI and driver cooperation */
> > u32 acpi_device_id;
> >
> > + /* ACPI handle corresponding to this connector display, if found */
> > + void *acpi_handle;
> > +
>
> The type is acpi_handle. It's none of our business to know what the
> underlying type is.
Will do.
>
> > /* Reads out the current hw, returning true if the connector is enabled
> > * and active (i.e. dpms ON state). */
> > bool (*get_hw_state)(struct intel_connector *);
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index b05b2191b919..93cece8e2516 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -45,6 +45,7 @@
> > #include "i915_debugfs.h"
> > #include "i915_drv.h"
> > #include "i915_trace.h"
> > +#include "intel_acpi.h"
> > #include "intel_atomic.h"
> > #include "intel_audio.h"
> > #include "intel_connector.h"
> > @@ -6623,6 +6624,8 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> >
> > connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;
> >
> > + /* Lookup the ACPI node corresponding to the connector */
> > + intel_acpi_device_id_update(dev_priv);
>
> Auch, this is problematic. It iterates all connectors, for every DP
> connector being added. In the middle of registering all connectors.
>
> From the POV of this patch alone, this is also unnecessary. This gets
> called via intel_opregion_register() after all connectors have been
> registered.
>
> I am aware it's not enough for your next patch, because it will need the
> acpi handle right here.
>
> I'm wondering if we need to maintain display_index[] in struct
> drm_i915_private, and update that as connectors get added instead of all
> at once in the end.
Sure, I can do that.
> connector->acpi_device_id never changes, does it,
> even though we keep updating it?
This is the part I am not so sure about - I hypothesized that theory
because of the current behavior in code (i.e. it is getting updated in
intel_opregion_resume() path). May be it does not change, I was not
sure, so I did not want to create any regression in the intel_opregion
code that I did not understand. I tried on my system and as far as I
could experiment, I did not see it changing. Please let me know if you
would like me to change my code to:
1) Maintain drm_i915_private->display_index[] and update it as
connectors are added.
2) Remove the code to update it on every resume and while registering
the connector.
Thanks & Best Regards,
Rajat
>
> BR,
> Jani.
>
>
> > }
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [v5,3/3] drm/i915: Add support for integrated privacy screens
2019-12-20 20:03 ` [PATCH v5 3/3] drm/i915: Add support for integrated privacy screens Rajat Jain
2020-01-21 7:25 ` Rajat Jain
@ 2020-01-25 0:11 ` Guenter Roeck
2020-01-28 1:33 ` Rajat Jain
1 sibling, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2020-01-25 0:11 UTC (permalink / raw)
To: Rajat Jain
Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
linux-kernel, dri-devel, intel-gfx, gregkh, mathewk,
Daniel Thompson, Jonathan Corbet, Pavel Machek, seanpaul,
Duncan Laurie, jsbarnes, Thierry Reding, rajatxjain
On Fri, Dec 20, 2019 at 12:03:53PM -0800, Rajat Jain 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@google.com>
> ---
> 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 | 13 +++-
> .../gpu/drm/i915/display/intel_connector.c | 35 +++++++++
> .../gpu/drm/i915/display/intel_connector.h | 1 +
> .../drm/i915/display/intel_display_types.h | 18 +++++
> drivers/gpu/drm/i915/display/intel_dp.c | 6 ++
> .../drm/i915/display/intel_privacy_screen.c | 72 +++++++++++++++++++
> .../drm/i915/display/intel_privacy_screen.h | 27 +++++++
> 8 files changed, 171 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 90dcf09f52cc..f7067c8f0407 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -197,7 +197,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 c2875b10adf9..c73b81c4c3f6 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 1133c4e97bb4..f3e041c737de 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> @@ -296,3 +296,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);
If intel_connector->privacy_screen_property is not NULL, prop
will be undefined, causing all kinds of fun.
I would expect this to happen whenever this function is called
for the second time.
Guenter
> +}
> 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 0a4a04116091..a0addd2c5376 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -433,6 +433,23 @@ 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;
> +};
> +
> +/**
> + * 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,
> };
>
> struct intel_digital_connector_state {
> @@ -440,6 +457,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 93cece8e2516..d5376d667929 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"
> @@ -6596,6 +6597,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> {
> struct drm_i915_private *dev_priv = to_i915(connector->dev);
> enum port port = dp_to_dig_port(intel_dp)->base.port;
> + struct intel_connector *intel_connector = to_intel_connector(connector);
>
> if (!IS_G4X(dev_priv) && port != PORT_A)
> intel_attach_force_audio_property(connector);
> @@ -6626,6 +6628,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>
> /* Lookup the ACPI node corresponding to the connector */
> intel_acpi_device_id_update(dev_priv);
> +
> + /* 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..c8a5b64f94fb
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * 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 *connector,
> + enum intel_privacy_screen_status val)
> +{
> + acpi_handle acpi_handle = 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);
> + else
> + DRM_WARN("%s: Cannot set privacy screen to invalid val %u\n",
> + dev_name(connector->base.dev->dev), 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_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
> + dev_name(connector->base.dev->dev));
> + return false;
> + }
> + DRM_DEV_INFO(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..74013a7885c7
> --- /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 intel_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 intel_privacy_screen_status val)
> +{ }
> +#endif /* CONFIG_ACPI */
> +
> +#endif /* __DRM_PRIVACY_SCREEN_H__ */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [v5,3/3] drm/i915: Add support for integrated privacy screens
2020-01-25 0:11 ` [v5,3/3] " Guenter Roeck
@ 2020-01-28 1:33 ` Rajat Jain
0 siblings, 0 replies; 9+ messages in thread
From: Rajat Jain @ 2020-01-28 1:33 UTC (permalink / raw)
To: Guenter Roeck
Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ville Syrjälä,
Chris Wilson, Imre Deak, José Roberto de Souza,
Linux Kernel Mailing List, dri-devel, intel-gfx,
Greg Kroah-Hartman, Mat King, Daniel Thompson, Jonathan Corbet,
Pavel Machek, Sean Paul, Duncan Laurie, Jesse Barnes,
Thierry Reding, Rajat Jain
On Fri, Jan 24, 2020 at 4:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Dec 20, 2019 at 12:03:53PM -0800, Rajat Jain 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@google.com>
> > ---
> > 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 | 13 +++-
> > .../gpu/drm/i915/display/intel_connector.c | 35 +++++++++
> > .../gpu/drm/i915/display/intel_connector.h | 1 +
> > .../drm/i915/display/intel_display_types.h | 18 +++++
> > drivers/gpu/drm/i915/display/intel_dp.c | 6 ++
> > .../drm/i915/display/intel_privacy_screen.c | 72 +++++++++++++++++++
> > .../drm/i915/display/intel_privacy_screen.h | 27 +++++++
> > 8 files changed, 171 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 90dcf09f52cc..f7067c8f0407 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -197,7 +197,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 c2875b10adf9..c73b81c4c3f6 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 1133c4e97bb4..f3e041c737de 100644
> > --- a/drivers/gpu/drm/i915/display/intel_connector.c
> > +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> > @@ -296,3 +296,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);
>
> If intel_connector->privacy_screen_property is not NULL, prop
> will be undefined, causing all kinds of fun.
Thank you for catching this. I will fix this in my next iteration.
Thanks,
Rajat
>
> I would expect this to happen whenever this function is called
> for the second time.
>
> Guenter
>
> > +}
> > 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 0a4a04116091..a0addd2c5376 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -433,6 +433,23 @@ 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;
> > +};
> > +
> > +/**
> > + * 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,
> > };
> >
> > struct intel_digital_connector_state {
> > @@ -440,6 +457,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 93cece8e2516..d5376d667929 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"
> > @@ -6596,6 +6597,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> > {
> > struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > enum port port = dp_to_dig_port(intel_dp)->base.port;
> > + struct intel_connector *intel_connector = to_intel_connector(connector);
> >
> > if (!IS_G4X(dev_priv) && port != PORT_A)
> > intel_attach_force_audio_property(connector);
> > @@ -6626,6 +6628,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> >
> > /* Lookup the ACPI node corresponding to the connector */
> > intel_acpi_device_id_update(dev_priv);
> > +
> > + /* 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..c8a5b64f94fb
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c
> > @@ -0,0 +1,72 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/*
> > + * 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 *connector,
> > + enum intel_privacy_screen_status val)
> > +{
> > + acpi_handle acpi_handle = 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);
> > + else
> > + DRM_WARN("%s: Cannot set privacy screen to invalid val %u\n",
> > + dev_name(connector->base.dev->dev), 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_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
> > + dev_name(connector->base.dev->dev));
> > + return false;
> > + }
> > + DRM_DEV_INFO(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..74013a7885c7
> > --- /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 intel_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 intel_privacy_screen_status val)
> > +{ }
> > +#endif /* CONFIG_ACPI */
> > +
> > +#endif /* __DRM_PRIVACY_SCREEN_H__ */
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-01-28 1:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20 20:03 [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi Rajat Jain
2019-12-20 20:03 ` [PATCH v5 2/3] drm/i915: Lookup and attach ACPI device node for connectors Rajat Jain
2020-01-24 11:37 ` Jani Nikula
2020-01-24 20:23 ` Rajat Jain
2019-12-20 20:03 ` [PATCH v5 3/3] drm/i915: Add support for integrated privacy screens Rajat Jain
2020-01-21 7:25 ` Rajat Jain
2020-01-25 0:11 ` [v5,3/3] " Guenter Roeck
2020-01-28 1:33 ` Rajat Jain
2020-01-24 11:03 ` [PATCH v5 1/3] drm/i915: Move the code to populate ACPI device ID into intel_acpi Jani Nikula
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).