This patchset adds support for integrated privacy screen on some laptops
using the ACPI methods to detect and control the feature.
Rajat Jain (5):
intel_acpi: Rename drm_dev local variable to dev
drm/connector: Add support for privacy-screen property
drm/i915: Lookup and attach ACPI device node for connectors
drm/i915: Add helper code for ACPI privacy screen
drm/i915: Enable support for integrated privacy screen
drivers/gpu/drm/drm_atomic_uapi.c | 4 +
drivers/gpu/drm/drm_connector.c | 51 +++++
drivers/gpu/drm/i915/Makefile | 3 +-
drivers/gpu/drm/i915/display/intel_acpi.c | 30 ++-
drivers/gpu/drm/i915/display/intel_atomic.c | 2 +
drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
.../drm/i915/display/intel_display_types.h | 5 +
drivers/gpu/drm/i915/display/intel_dp.c | 48 ++++-
drivers/gpu/drm/i915/display/intel_dp.h | 5 +
.../drm/i915/display/intel_privacy_screen.c | 184 ++++++++++++++++++
.../drm/i915/display/intel_privacy_screen.h | 27 +++
drivers/gpu/drm/i915/i915_drv.h | 2 +
include/drm/drm_connector.h | 24 +++
13 files changed, 382 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
--
2.25.1.481.gfbce0eb801-goog
Add support for generic electronic privacy screen property, that
can be added by systems that have an integrated EPS.
Signed-off-by: Rajat Jain <[email protected]>
---
v9: rebased on top of https://cgit.freedesktop.org/drm/drm-tip:drm-tip
v8: Remove the ...destroy_privacy_screen() method and let the property
be destroyed along with others at the time of device destruction.
(because drm core doesn't also like properties destroyed in
late_register()).
v7: * Initial version, formed by moving the privacy-screen property into
drm core.
* Break the init_property() into create_property() and attach_property()
so that property can be created while registering connector, but
attached in late_register() (after ACPI node detection).
drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
drivers/gpu/drm/drm_connector.c | 51 +++++++++++++++++++++++++++++++
include/drm/drm_connector.h | 24 +++++++++++++++
3 files changed, 79 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a1e5e262bae2d..843a8cdacd149 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -766,6 +766,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
fence_ptr);
} else if (property == connector->max_bpc_property) {
state->max_requested_bpc = val;
+ } else if (property == connector->privacy_screen_property) {
+ state->privacy_screen_status = val;
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
@@ -842,6 +844,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
*val = 0;
} else if (property == connector->max_bpc_property) {
*val = state->max_requested_bpc;
+ } else if (property == connector->privacy_screen_property) {
+ *val = state->privacy_screen_status;
} else if (connector->funcs->atomic_get_property) {
return connector->funcs->atomic_get_property(connector,
state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 644f0ad106717..182aa557962b2 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1186,6 +1186,10 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
* can also expose this property to external outputs, in which case they
* must support "None", which should be the default (since external screens
* have a built-in scaler).
+ *
+ * privacy-screen:
+ * This optional property can be used to enable / disable an integrated
+ * electronic privacy screen that is available on some displays.
*/
int drm_connector_create_standard_properties(struct drm_device *dev)
@@ -2152,6 +2156,53 @@ int drm_connector_set_panel_orientation_with_quirk(
}
EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
+static const struct drm_prop_enum_list privacy_screen_enum[] = {
+ { PRIVACY_SCREEN_DISABLED, "Disabled" },
+ { PRIVACY_SCREEN_ENABLED, "Enabled" },
+};
+
+/**
+ * drm_connector_create_privacy_screen_property -
+ * create the drm connecter's privacy-screen property.
+ * @connector: connector for which to create the privacy-screen property
+ *
+ * This function creates the "privacy-screen" property for the
+ * connector. It is not attached.
+ */
+void
+drm_connector_create_privacy_screen_property(struct drm_connector *connector)
+{
+ if (connector->privacy_screen_property)
+ return;
+
+ connector->privacy_screen_property =
+ drm_property_create_enum(connector->dev, DRM_MODE_PROP_ENUM,
+ "privacy-screen", privacy_screen_enum,
+ ARRAY_SIZE(privacy_screen_enum));
+}
+EXPORT_SYMBOL(drm_connector_create_privacy_screen_property);
+
+/**
+ * drm_connector_attach_privacy_screen_property -
+ * attach the drm connecter's privacy-screen property.
+ * @connector: connector on which to attach the privacy-screen property
+ *
+ * This function attaches the "privacy-screen" property to the
+ * connector. Initial state of privacy-screen is set to disabled.
+ */
+void
+drm_connector_attach_privacy_screen_property(struct drm_connector *connector)
+{
+ struct drm_property *prop = connector->privacy_screen_property;
+
+ if (!prop)
+ return;
+
+ drm_object_attach_property(&connector->base, prop,
+ PRIVACY_SCREEN_DISABLED);
+}
+EXPORT_SYMBOL(drm_connector_attach_privacy_screen_property);
+
int drm_connector_set_obj_prop(struct drm_mode_object *obj,
struct drm_property *property,
uint64_t value)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 19ae6bb5c85be..f9ce89cc13542 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -271,6 +271,20 @@ struct drm_monitor_range_info {
u8 max_vfreq;
};
+/**
+ * enum drm_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 drm_privacy_screen_status {
+ PRIVACY_SCREEN_DISABLED = 0,
+ PRIVACY_SCREEN_ENABLED = 1,
+};
+
/*
* This is a consolidated colorimetry list supported by HDMI and
* DP protocol standard. The respective connectors will register
@@ -686,6 +700,8 @@ struct drm_connector_state {
*/
u8 max_bpc;
+ enum drm_privacy_screen_status privacy_screen_status;
+
/**
* @hdr_output_metadata:
* DRM blob property for HDR output metadata
@@ -1285,6 +1301,12 @@ struct drm_connector {
*/
struct drm_property *max_bpc_property;
+ /**
+ * @privacy_screen_property: Optional property for the connector to
+ * control the integrated privacy screen, if available.
+ */
+ struct drm_property *privacy_screen_property;
+
#define DRM_CONNECTOR_POLL_HPD (1 << 0)
#define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
#define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1598,6 +1620,8 @@ int drm_connector_set_panel_orientation_with_quirk(
int width, int height);
int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
int min, int max);
+void drm_connector_create_privacy_screen_property(struct drm_connector *conn);
+void drm_connector_attach_privacy_screen_property(struct drm_connector *conn);
/**
* struct drm_tile_group - Tile group metadata
--
2.25.1.481.gfbce0eb801-goog
Add support for an ACPI based integrated privacy screen that is
available on some systems.
Signed-off-by: Rajat Jain <[email protected]>
---
v9: same as v8
v8: - separate the APCI privacy screen into a separate patch.
- Don't destroy the property if there is no privacy screen (because
drm core doesn't like destroying property in late_register()).
- The setting change needs to be committed in ->update_pipe() for
ddi.c as well as dp.c and both of them call intel_dp_add_properties()
v7: Look for ACPI node in ->late_register() hook.
Do the scan only once per drm_device (instead of 1 per drm_connector)
v6: Addressed minor comments from Jani at
https://lkml.org/lkml/2020/1/24/1143
- local variable renamed.
- used drm_dbg_kms()
- used acpi_device_handle()
- Used opaque type acpi_handle instead of void*
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_atomic.c | 2 ++
drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
drivers/gpu/drm/i915/display/intel_dp.c | 34 ++++++++++++++++++++-
drivers/gpu/drm/i915/display/intel_dp.h | 5 +++
4 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index d043057d2fa03..9898d8980e7ce 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -150,6 +150,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
new_conn_state->base.content_type != old_conn_state->base.content_type ||
new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode ||
+ new_conn_state->base.privacy_screen_status !=
+ old_conn_state->base.privacy_screen_status ||
!blob_equal(new_conn_state->base.hdr_output_metadata,
old_conn_state->base.hdr_output_metadata))
crtc_state->mode_changed = true;
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 73d0f4648c06a..69a5423216dc5 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3708,6 +3708,7 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder,
if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
+ intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
intel_hdcp_update_pipe(encoder, crtc_state, conn_state);
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 3ddc424b028c1..5f33ebb466135 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -62,6 +62,7 @@
#include "intel_lspcon.h"
#include "intel_lvds.h"
#include "intel_panel.h"
+#include "intel_privacy_screen.h"
#include "intel_psr.h"
#include "intel_sideband.h"
#include "intel_tc.h"
@@ -5886,6 +5887,10 @@ intel_dp_connector_register(struct drm_connector *connector)
dev_priv->acpi_scan_done = true;
}
+ /* Check for integrated Privacy screen support */
+ if (intel_privacy_screen_present(to_intel_connector(connector)))
+ drm_connector_attach_privacy_screen_property(connector);
+
DRM_DEBUG_KMS("registering %s bus for %s\n",
intel_dp->aux.name, connector->kdev->kobj.name);
@@ -6883,6 +6888,33 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;
}
+
+ /*
+ * Created here, but depending on result of probing for privacy-screen
+ * in intel_dp_connector_register(), gets attached in that function.
+ * Need to create here because the drm core doesn't like creating
+ * properties during ->late_register().
+ */
+ drm_connector_create_privacy_screen_property(connector);
+}
+
+void
+intel_dp_update_privacy_screen(struct intel_encoder *encoder,
+ const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state)
+{
+ struct drm_connector *connector = conn_state->connector;
+
+ intel_privacy_screen_set_val(to_intel_connector(connector),
+ conn_state->privacy_screen_status);
+}
+
+static void intel_dp_update_pipe(struct intel_encoder *encoder,
+ const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state)
+{
+ intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
+ intel_panel_update_backlight(encoder, crtc_state, conn_state);
}
static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
@@ -7826,7 +7858,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
intel_encoder->compute_config = intel_dp_compute_config;
intel_encoder->get_hw_state = intel_dp_get_hw_state;
intel_encoder->get_config = intel_dp_get_config;
- intel_encoder->update_pipe = intel_panel_update_backlight;
+ intel_encoder->update_pipe = intel_dp_update_pipe;
intel_encoder->suspend = intel_dp_encoder_suspend;
if (IS_CHERRYVIEW(dev_priv)) {
intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index 0c7be8ed1423a..e4594e27ce5a8 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -123,4 +123,9 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
u32 intel_dp_mode_to_fec_clock(u32 mode_clock);
+void
+intel_dp_update_privacy_screen(struct intel_encoder *encoder,
+ const struct intel_crtc_state *crtc_state,
+ const struct drm_connector_state *conn_state);
+
#endif /* __INTEL_DP_H__ */
--
2.25.1.481.gfbce0eb801-goog
Hi Jani,
On Thu, Mar 12, 2020 at 11:56 AM Rajat Jain <[email protected]> wrote:
>
> This patchset adds support for integrated privacy screen on some laptops
> using the ACPI methods to detect and control the feature.
>
> Rajat Jain (5):
> intel_acpi: Rename drm_dev local variable to dev
> drm/connector: Add support for privacy-screen property
> drm/i915: Lookup and attach ACPI device node for connectors
> drm/i915: Add helper code for ACPI privacy screen
> drm/i915: Enable support for integrated privacy screen
Just checking to see if you got a chance to look at this latest
patchset. This takes care of all your review comments.
Thanks & Best Regards,
Rajat
>
> drivers/gpu/drm/drm_atomic_uapi.c | 4 +
> drivers/gpu/drm/drm_connector.c | 51 +++++
> drivers/gpu/drm/i915/Makefile | 3 +-
> drivers/gpu/drm/i915/display/intel_acpi.c | 30 ++-
> drivers/gpu/drm/i915/display/intel_atomic.c | 2 +
> drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
> .../drm/i915/display/intel_display_types.h | 5 +
> drivers/gpu/drm/i915/display/intel_dp.c | 48 ++++-
> drivers/gpu/drm/i915/display/intel_dp.h | 5 +
> .../drm/i915/display/intel_privacy_screen.c | 184 ++++++++++++++++++
> .../drm/i915/display/intel_privacy_screen.h | 27 +++
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> include/drm/drm_connector.h | 24 +++
> 13 files changed, 382 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
>
> --
> 2.25.1.481.gfbce0eb801-goog
>
Hi,
On 3/12/20 7:56 PM, Rajat Jain wrote:
> Add support for an ACPI based integrated privacy screen that is
> available on some systems.
>
> Signed-off-by: Rajat Jain <[email protected]>
So as discussed a while ago I'm working on adding support for the
privacy-screen on Lenovo Thinkpads, introducing a small new
subsystem / helper-class as intermediary for when the privacy-screen
is controlled by e.g. some random drivers/platform/x86 driver rather
then directly by the GPU driver.
I'm almost ready to send out v1. I was working on hooking things
up in the i915 code and I was wondering what you were doing when
the property is actually changed and we need to commit the new
privacy-screen state to the hardware.
This made me look at this patch, some comments inline:
> ---
> v9: same as v8
> v8: - separate the APCI privacy screen into a separate patch.
> - Don't destroy the property if there is no privacy screen (because
> drm core doesn't like destroying property in late_register()).
> - The setting change needs to be committed in ->update_pipe() for
> ddi.c as well as dp.c and both of them call intel_dp_add_properties()
> v7: Look for ACPI node in ->late_register() hook.
> Do the scan only once per drm_device (instead of 1 per drm_connector)
> v6: Addressed minor comments from Jani at
> https://lkml.org/lkml/2020/1/24/1143
> - local variable renamed.
> - used drm_dbg_kms()
> - used acpi_device_handle()
> - Used opaque type acpi_handle instead of void*
> 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_atomic.c | 2 ++
> drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
> drivers/gpu/drm/i915/display/intel_dp.c | 34 ++++++++++++++++++++-
> drivers/gpu/drm/i915/display/intel_dp.h | 5 +++
> 4 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index d043057d2fa03..9898d8980e7ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -150,6 +150,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
> new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
> new_conn_state->base.content_type != old_conn_state->base.content_type ||
> new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode ||
> + new_conn_state->base.privacy_screen_status !=
> + old_conn_state->base.privacy_screen_status ||
> !blob_equal(new_conn_state->base.hdr_output_metadata,
> old_conn_state->base.hdr_output_metadata))
> crtc_state->mode_changed = true;
Right I was planning on doing this to.
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 73d0f4648c06a..69a5423216dc5 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3708,6 +3708,7 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder,
> if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
>
> + intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
> intel_hdcp_update_pipe(encoder, crtc_state, conn_state);
> }
>
And this too.
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 3ddc424b028c1..5f33ebb466135 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -62,6 +62,7 @@
> #include "intel_lspcon.h"
> #include "intel_lvds.h"
> #include "intel_panel.h"
> +#include "intel_privacy_screen.h"
> #include "intel_psr.h"
> #include "intel_sideband.h"
> #include "intel_tc.h"
> @@ -5886,6 +5887,10 @@ intel_dp_connector_register(struct drm_connector *connector)
> dev_priv->acpi_scan_done = true;
> }
>
> + /* Check for integrated Privacy screen support */
> + if (intel_privacy_screen_present(to_intel_connector(connector)))
> + drm_connector_attach_privacy_screen_property(connector);
> +
> DRM_DEBUG_KMS("registering %s bus for %s\n",
> intel_dp->aux.name, connector->kdev->kobj.name);
>
> @@ -6883,6 +6888,33 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;
>
> }
> +
> + /*
> + * Created here, but depending on result of probing for privacy-screen
> + * in intel_dp_connector_register(), gets attached in that function.
> + * Need to create here because the drm core doesn't like creating
> + * properties during ->late_register().
> + */
> + drm_connector_create_privacy_screen_property(connector);
> +}
> +
> +void
> +intel_dp_update_privacy_screen(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + const struct drm_connector_state *conn_state)
> +{
> + struct drm_connector *connector = conn_state->connector;
> +
> + intel_privacy_screen_set_val(to_intel_connector(connector),
> + conn_state->privacy_screen_status);
> +}
> +
> +static void intel_dp_update_pipe(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + const struct drm_connector_state *conn_state)
> +{
> + intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
> + intel_panel_update_backlight(encoder, crtc_state, conn_state);
> }
>
> static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
> @@ -7826,7 +7858,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
> intel_encoder->compute_config = intel_dp_compute_config;
> intel_encoder->get_hw_state = intel_dp_get_hw_state;
> intel_encoder->get_config = intel_dp_get_config;
> - intel_encoder->update_pipe = intel_panel_update_backlight;
> + intel_encoder->update_pipe = intel_dp_update_pipe;
> intel_encoder->suspend = intel_dp_encoder_suspend;
> if (IS_CHERRYVIEW(dev_priv)) {
> intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
And this too.
One problem here is that AFAICT the update_pipe callback is only called on
fast modesets. So if the privacy_screen state is changed as part of a
full modeset, then the change will be ignored.
Even if we ignore that for now, this means that we end up calling
intel_privacy_screen_set_val(), or my equivalent of that for
each fast modeset.
In patch 4/5 intel_privacy_screen_set_val() is defined like this:
+void intel_privacy_screen_set_val(struct intel_connector *connector,
+ enum drm_privacy_screen_status val)
+{
+ struct drm_device *drm = connector->base.dev;
+
+ if (val == PRIVACY_SCREEN_DISABLED) {
+ drm_dbg_kms(drm, "%s: disabling privacy-screen\n",
+ CONN_NAME(connector));
+ acpi_privacy_screen_call_dsm(connector,
+ CONNECTOR_DSM_FN_PRIVACY_DISABLE);
+ } else {
+ drm_dbg_kms(drm, "%s: enabling privacy-screen\n",
+ CONN_NAME(connector));
+ acpi_privacy_screen_call_dsm(connector,
+ CONNECTOR_DSM_FN_PRIVACY_ENABLE);
+ }
+}
+
There are 2 problems with this:
1. It makes the call even if there is no privacy-screen, and then
acpi_privacy_screen_call_dsm() will log an error (if the connector has an
associated handle but not the DSM).
2. It makes this call on any modeset, even if the property did non change
(and even if there is no privacy-screen) and AFAIK these ACPI calls are somewhat
expensive to make.
1. Should be easy to fix, fixing 2. is trickier. We really need access
to the new and old connector_state here to only make the ACPI calls when
necessary. But ATM all callbacks only ever get passed the new-state and
these callbacks are all called after drm_atomic_helper_swap_state() at
which point there is no way to get the old_state from the new_state.
I've chosen to instead do this to update the privacy-screen change:
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15501,6 +15503,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
intel_color_load_luts(new_crtc_state);
}
+ for_each_new_connector_in_state(&state->base, connector, new_connector_state, i)
+ drm_connector_update_privacy_screen(connector, &state->base);
+
/*
* Now that the vblank has passed, we can go ahead and program the
* optimal watermarks on platforms that need two-step watermark
With drm_connector_update_privacy_screen() looking like this:
+void drm_connector_update_privacy_screen(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+ struct drm_connector_state *new_connector_state, *old_connector_state;
+ int ret;
+
+ if (!connector->privacy_screen)
+ return;
+
+ new_connector_state = drm_atomic_get_new_connector_state(state, connector);
+ old_connector_state = drm_atomic_get_old_connector_state(state, connector);
+
+ if (new_connector_state->privacy_screen_sw_state ==
+ old_connector_state->privacy_screen_sw_state)
+ return;
+
+ ret = drm_privacy_screen_set_sw_state(connector->privacy_screen,
+ new_connector_state->privacy_screen_sw_state);
+ if (ret)
+ drm_err(connector->dev, "Error updating privacy-screen sw_state\n");
+}
Which avoids all the problems described above.
REgards,
Hans
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 0c7be8ed1423a..e4594e27ce5a8 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -123,4 +123,9 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>
> u32 intel_dp_mode_to_fec_clock(u32 mode_clock);
>
> +void
> +intel_dp_update_privacy_screen(struct intel_encoder *encoder,
> + const struct intel_crtc_state *crtc_state,
> + const struct drm_connector_state *conn_state);
> +
> #endif /* __INTEL_DP_H__ */
>
Hello Hans,
On Mon, Jul 6, 2020 at 5:51 AM Hans de Goede <[email protected]> wrote:
>
> Hi,
>
> On 3/12/20 7:56 PM, Rajat Jain wrote:
> > Add support for an ACPI based integrated privacy screen that is
> > available on some systems.
> >
> > Signed-off-by: Rajat Jain <[email protected]>
>
> So as discussed a while ago I'm working on adding support for the
> privacy-screen on Lenovo Thinkpads, introducing a small new
> subsystem / helper-class as intermediary for when the privacy-screen
> is controlled by e.g. some random drivers/platform/x86 driver rather
> then directly by the GPU driver.
>
> I'm almost ready to send out v1. I was working on hooking things
> up in the i915 code and I was wondering what you were doing when
> the property is actually changed and we need to commit the new
> privacy-screen state to the hardware.
>
> This made me look at this patch, some comments inline:
>
> > ---
> > v9: same as v8
> > v8: - separate the APCI privacy screen into a separate patch.
> > - Don't destroy the property if there is no privacy screen (because
> > drm core doesn't like destroying property in late_register()).
> > - The setting change needs to be committed in ->update_pipe() for
> > ddi.c as well as dp.c and both of them call intel_dp_add_properties()
> > v7: Look for ACPI node in ->late_register() hook.
> > Do the scan only once per drm_device (instead of 1 per drm_connector)
> > v6: Addressed minor comments from Jani at
> > https://lkml.org/lkml/2020/1/24/1143
> > - local variable renamed.
> > - used drm_dbg_kms()
> > - used acpi_device_handle()
> > - Used opaque type acpi_handle instead of void*
> > 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_atomic.c | 2 ++
> > drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
> > drivers/gpu/drm/i915/display/intel_dp.c | 34 ++++++++++++++++++++-
> > drivers/gpu/drm/i915/display/intel_dp.h | 5 +++
> > 4 files changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index d043057d2fa03..9898d8980e7ce 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -150,6 +150,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
> > new_conn_state->base.content_type != old_conn_state->base.content_type ||
> > new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode ||
> > + new_conn_state->base.privacy_screen_status !=
> > + old_conn_state->base.privacy_screen_status ||
> > !blob_equal(new_conn_state->base.hdr_output_metadata,
> > old_conn_state->base.hdr_output_metadata))
> > crtc_state->mode_changed = true;
>
> Right I was planning on doing this to.
>
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 73d0f4648c06a..69a5423216dc5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3708,6 +3708,7 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder,
> > if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
> >
> > + intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
> > intel_hdcp_update_pipe(encoder, crtc_state, conn_state);
> > }
> >
>
> And this too.
>
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 3ddc424b028c1..5f33ebb466135 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -62,6 +62,7 @@
> > #include "intel_lspcon.h"
> > #include "intel_lvds.h"
> > #include "intel_panel.h"
> > +#include "intel_privacy_screen.h"
> > #include "intel_psr.h"
> > #include "intel_sideband.h"
> > #include "intel_tc.h"
> > @@ -5886,6 +5887,10 @@ intel_dp_connector_register(struct drm_connector *connector)
> > dev_priv->acpi_scan_done = true;
> > }
> >
> > + /* Check for integrated Privacy screen support */
> > + if (intel_privacy_screen_present(to_intel_connector(connector)))
> > + drm_connector_attach_privacy_screen_property(connector);
> > +
> > DRM_DEBUG_KMS("registering %s bus for %s\n",
> > intel_dp->aux.name, connector->kdev->kobj.name);
> >
> > @@ -6883,6 +6888,33 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> > connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;
> >
> > }
> > +
> > + /*
> > + * Created here, but depending on result of probing for privacy-screen
> > + * in intel_dp_connector_register(), gets attached in that function.
> > + * Need to create here because the drm core doesn't like creating
> > + * properties during ->late_register().
> > + */
> > + drm_connector_create_privacy_screen_property(connector);
> > +}
> > +
> > +void
> > +intel_dp_update_privacy_screen(struct intel_encoder *encoder,
> > + const struct intel_crtc_state *crtc_state,
> > + const struct drm_connector_state *conn_state)
> > +{
> > + struct drm_connector *connector = conn_state->connector;
> > +
> > + intel_privacy_screen_set_val(to_intel_connector(connector),
> > + conn_state->privacy_screen_status);
> > +}
> > +
> > +static void intel_dp_update_pipe(struct intel_encoder *encoder,
> > + const struct intel_crtc_state *crtc_state,
> > + const struct drm_connector_state *conn_state)
> > +{
> > + intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
> > + intel_panel_update_backlight(encoder, crtc_state, conn_state);
> > }
> >
> > static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
> > @@ -7826,7 +7858,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
> > intel_encoder->compute_config = intel_dp_compute_config;
> > intel_encoder->get_hw_state = intel_dp_get_hw_state;
> > intel_encoder->get_config = intel_dp_get_config;
> > - intel_encoder->update_pipe = intel_panel_update_backlight;
> > + intel_encoder->update_pipe = intel_dp_update_pipe;
> > intel_encoder->suspend = intel_dp_encoder_suspend;
> > if (IS_CHERRYVIEW(dev_priv)) {
> > intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
>
> And this too.
>
> One problem here is that AFAICT the update_pipe callback is only called on
> fast modesets. So if the privacy_screen state is changed as part of a
> full modeset, then the change will be ignored.
I'm actually new to the drm / i915, so I did what I thought was right
at the time and was working on my setup. But, yeah, that might be a
possible issue it seems.
>
> Even if we ignore that for now, this means that we end up calling
> intel_privacy_screen_set_val(), or my equivalent of that for
> each fast modeset.
>
> In patch 4/5 intel_privacy_screen_set_val() is defined like this:
>
> +void intel_privacy_screen_set_val(struct intel_connector *connector,
> + enum drm_privacy_screen_status val)
> +{
> + struct drm_device *drm = connector->base.dev;
> +
> + if (val == PRIVACY_SCREEN_DISABLED) {
> + drm_dbg_kms(drm, "%s: disabling privacy-screen\n",
> + CONN_NAME(connector));
> + acpi_privacy_screen_call_dsm(connector,
> + CONNECTOR_DSM_FN_PRIVACY_DISABLE);
> + } else {
> + drm_dbg_kms(drm, "%s: enabling privacy-screen\n",
> + CONN_NAME(connector));
> + acpi_privacy_screen_call_dsm(connector,
> + CONNECTOR_DSM_FN_PRIVACY_ENABLE);
> + }
> +}
> +
>
> There are 2 problems with this:
>
> 1. It makes the call even if there is no privacy-screen, and then
> acpi_privacy_screen_call_dsm() will log an error (if the connector has an
> associated handle but not the DSM).
>
> 2. It makes this call on any modeset, even if the property did non change
> (and even if there is no privacy-screen) and AFAIK these ACPI calls are somewhat
> expensive to make.
Ack to both these problems.
>
> 1. Should be easy to fix, fixing 2. is trickier. We really need access
> to the new and old connector_state here to only make the ACPI calls when
> necessary. But ATM all callbacks only ever get passed the new-state and
> these callbacks are all called after drm_atomic_helper_swap_state() at
> which point there is no way to get the old_state from the new_state.
>
> I've chosen to instead do this to update the privacy-screen change:
>
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -15501,6 +15503,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> intel_color_load_luts(new_crtc_state);
> }
>
> + for_each_new_connector_in_state(&state->base, connector, new_connector_state, i)
> + drm_connector_update_privacy_screen(connector, &state->base);
> +
> /*
> * Now that the vblank has passed, we can go ahead and program the
> * optimal watermarks on platforms that need two-step watermark
>
> With drm_connector_update_privacy_screen() looking like this:
>
> +void drm_connector_update_privacy_screen(struct drm_connector *connector,
> + struct drm_atomic_state *state)
> +{
> + struct drm_connector_state *new_connector_state, *old_connector_state;
> + int ret;
> +
> + if (!connector->privacy_screen)
> + return;
> +
> + new_connector_state = drm_atomic_get_new_connector_state(state, connector);
> + old_connector_state = drm_atomic_get_old_connector_state(state, connector);
> +
> + if (new_connector_state->privacy_screen_sw_state ==
> + old_connector_state->privacy_screen_sw_state)
> + return;
> +
> + ret = drm_privacy_screen_set_sw_state(connector->privacy_screen,
> + new_connector_state->privacy_screen_sw_state);
> + if (ret)
> + drm_err(connector->dev, "Error updating privacy-screen sw_state\n");
> +}
>
> Which avoids all the problems described above.
Ack. This looks like a better way since it takes care of these
problems. Please feel free to use my patches as you see fit (I didn't
see much activity on them since last many months so I have moved on to
something else now).
Thanks & Best Regards,
Rajat
>
> REgards,
>
> Hans
>
>
>
>
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > index 0c7be8ed1423a..e4594e27ce5a8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -123,4 +123,9 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
> >
> > u32 intel_dp_mode_to_fec_clock(u32 mode_clock);
> >
> > +void
> > +intel_dp_update_privacy_screen(struct intel_encoder *encoder,
> > + const struct intel_crtc_state *crtc_state,
> > + const struct drm_connector_state *conn_state);
> > +
> > #endif /* __INTEL_DP_H__ */
> >
>
Hello Hans,
On Mon, Jul 6, 2020 at 5:50 PM Rajat Jain <[email protected]> wrote:
>
> Hello Hans,
>
> On Mon, Jul 6, 2020 at 5:51 AM Hans de Goede <[email protected]> wrote:
> >
> > Hi,
> >
> > On 3/12/20 7:56 PM, Rajat Jain wrote:
> > > Add support for an ACPI based integrated privacy screen that is
> > > available on some systems.
> > >
> > > Signed-off-by: Rajat Jain <[email protected]>
> >
> > So as discussed a while ago I'm working on adding support for the
> > privacy-screen on Lenovo Thinkpads, introducing a small new
> > subsystem / helper-class as intermediary for when the privacy-screen
> > is controlled by e.g. some random drivers/platform/x86 driver rather
> > then directly by the GPU driver.
> >
> > I'm almost ready to send out v1. I was working on hooking things
> > up in the i915 code and I was wondering what you were doing when
> > the property is actually changed and we need to commit the new
> > privacy-screen state to the hardware.
> >
> > This made me look at this patch, some comments inline:
> >
> > > ---
> > > v9: same as v8
> > > v8: - separate the APCI privacy screen into a separate patch.
> > > - Don't destroy the property if there is no privacy screen (because
> > > drm core doesn't like destroying property in late_register()).
> > > - The setting change needs to be committed in ->update_pipe() for
> > > ddi.c as well as dp.c and both of them call intel_dp_add_properties()
> > > v7: Look for ACPI node in ->late_register() hook.
> > > Do the scan only once per drm_device (instead of 1 per drm_connector)
> > > v6: Addressed minor comments from Jani at
> > > https://lkml.org/lkml/2020/1/24/1143
> > > - local variable renamed.
> > > - used drm_dbg_kms()
> > > - used acpi_device_handle()
> > > - Used opaque type acpi_handle instead of void*
> > > 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_atomic.c | 2 ++
> > > drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
> > > drivers/gpu/drm/i915/display/intel_dp.c | 34 ++++++++++++++++++++-
> > > drivers/gpu/drm/i915/display/intel_dp.h | 5 +++
> > > 4 files changed, 41 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > index d043057d2fa03..9898d8980e7ce 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > @@ -150,6 +150,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > > new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
> > > new_conn_state->base.content_type != old_conn_state->base.content_type ||
> > > new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode ||
> > > + new_conn_state->base.privacy_screen_status !=
> > > + old_conn_state->base.privacy_screen_status ||
> > > !blob_equal(new_conn_state->base.hdr_output_metadata,
> > > old_conn_state->base.hdr_output_metadata))
> > > crtc_state->mode_changed = true;
> >
> > Right I was planning on doing this to.
> >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 73d0f4648c06a..69a5423216dc5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3708,6 +3708,7 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder,
> > > if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > > intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
> > >
> > > + intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
> > > intel_hdcp_update_pipe(encoder, crtc_state, conn_state);
> > > }
> > >
> >
> > And this too.
> >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 3ddc424b028c1..5f33ebb466135 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -62,6 +62,7 @@
> > > #include "intel_lspcon.h"
> > > #include "intel_lvds.h"
> > > #include "intel_panel.h"
> > > +#include "intel_privacy_screen.h"
> > > #include "intel_psr.h"
> > > #include "intel_sideband.h"
> > > #include "intel_tc.h"
> > > @@ -5886,6 +5887,10 @@ intel_dp_connector_register(struct drm_connector *connector)
> > > dev_priv->acpi_scan_done = true;
> > > }
> > >
> > > + /* Check for integrated Privacy screen support */
> > > + if (intel_privacy_screen_present(to_intel_connector(connector)))
> > > + drm_connector_attach_privacy_screen_property(connector);
> > > +
> > > DRM_DEBUG_KMS("registering %s bus for %s\n",
> > > intel_dp->aux.name, connector->kdev->kobj.name);
> > >
> > > @@ -6883,6 +6888,33 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> > > connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;
> > >
> > > }
> > > +
> > > + /*
> > > + * Created here, but depending on result of probing for privacy-screen
> > > + * in intel_dp_connector_register(), gets attached in that function.
> > > + * Need to create here because the drm core doesn't like creating
> > > + * properties during ->late_register().
> > > + */
> > > + drm_connector_create_privacy_screen_property(connector);
> > > +}
> > > +
> > > +void
> > > +intel_dp_update_privacy_screen(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state,
> > > + const struct drm_connector_state *conn_state)
> > > +{
> > > + struct drm_connector *connector = conn_state->connector;
> > > +
> > > + intel_privacy_screen_set_val(to_intel_connector(connector),
> > > + conn_state->privacy_screen_status);
> > > +}
> > > +
> > > +static void intel_dp_update_pipe(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state,
> > > + const struct drm_connector_state *conn_state)
> > > +{
> > > + intel_dp_update_privacy_screen(encoder, crtc_state, conn_state);
> > > + intel_panel_update_backlight(encoder, crtc_state, conn_state);
> > > }
> > >
> > > static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
> > > @@ -7826,7 +7858,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
> > > intel_encoder->compute_config = intel_dp_compute_config;
> > > intel_encoder->get_hw_state = intel_dp_get_hw_state;
> > > intel_encoder->get_config = intel_dp_get_config;
> > > - intel_encoder->update_pipe = intel_panel_update_backlight;
> > > + intel_encoder->update_pipe = intel_dp_update_pipe;
> > > intel_encoder->suspend = intel_dp_encoder_suspend;
> > > if (IS_CHERRYVIEW(dev_priv)) {
> > > intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
> >
> > And this too.
> >
> > One problem here is that AFAICT the update_pipe callback is only called on
> > fast modesets. So if the privacy_screen state is changed as part of a
> > full modeset, then the change will be ignored.
>
> I'm actually new to the drm / i915, so I did what I thought was right
> at the time and was working on my setup. But, yeah, that might be a
> possible issue it seems.
>
> >
> > Even if we ignore that for now, this means that we end up calling
> > intel_privacy_screen_set_val(), or my equivalent of that for
> > each fast modeset.
> >
> > In patch 4/5 intel_privacy_screen_set_val() is defined like this:
> >
> > +void intel_privacy_screen_set_val(struct intel_connector *connector,
> > + enum drm_privacy_screen_status val)
> > +{
> > + struct drm_device *drm = connector->base.dev;
> > +
> > + if (val == PRIVACY_SCREEN_DISABLED) {
> > + drm_dbg_kms(drm, "%s: disabling privacy-screen\n",
> > + CONN_NAME(connector));
> > + acpi_privacy_screen_call_dsm(connector,
> > + CONNECTOR_DSM_FN_PRIVACY_DISABLE);
> > + } else {
> > + drm_dbg_kms(drm, "%s: enabling privacy-screen\n",
> > + CONN_NAME(connector));
> > + acpi_privacy_screen_call_dsm(connector,
> > + CONNECTOR_DSM_FN_PRIVACY_ENABLE);
> > + }
> > +}
> > +
> >
> > There are 2 problems with this:
> >
> > 1. It makes the call even if there is no privacy-screen, and then
> > acpi_privacy_screen_call_dsm() will log an error (if the connector has an
> > associated handle but not the DSM).
> >
> > 2. It makes this call on any modeset, even if the property did non change
> > (and even if there is no privacy-screen) and AFAIK these ACPI calls are somewhat
> > expensive to make.
>
> Ack to both these problems.
>
> >
> > 1. Should be easy to fix, fixing 2. is trickier. We really need access
> > to the new and old connector_state here to only make the ACPI calls when
> > necessary. But ATM all callbacks only ever get passed the new-state and
> > these callbacks are all called after drm_atomic_helper_swap_state() at
> > which point there is no way to get the old_state from the new_state.
> >
> > I've chosen to instead do this to update the privacy-screen change:
> >
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15501,6 +15503,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> > intel_color_load_luts(new_crtc_state);
> > }
> >
> > + for_each_new_connector_in_state(&state->base, connector, new_connector_state, i)
> > + drm_connector_update_privacy_screen(connector, &state->base);
> > +
> > /*
> > * Now that the vblank has passed, we can go ahead and program the
> > * optimal watermarks on platforms that need two-step watermark
> >
> > With drm_connector_update_privacy_screen() looking like this:
> >
> > +void drm_connector_update_privacy_screen(struct drm_connector *connector,
> > + struct drm_atomic_state *state)
> > +{
> > + struct drm_connector_state *new_connector_state, *old_connector_state;
> > + int ret;
> > +
> > + if (!connector->privacy_screen)
> > + return;
> > +
> > + new_connector_state = drm_atomic_get_new_connector_state(state, connector);
> > + old_connector_state = drm_atomic_get_old_connector_state(state, connector);
> > +
> > + if (new_connector_state->privacy_screen_sw_state ==
> > + old_connector_state->privacy_screen_sw_state)
> > + return;
> > +
> > + ret = drm_privacy_screen_set_sw_state(connector->privacy_screen,
> > + new_connector_state->privacy_screen_sw_state);
> > + if (ret)
> > + drm_err(connector->dev, "Error updating privacy-screen sw_state\n");
> > +}
> >
> > Which avoids all the problems described above.
>
> Ack. This looks like a better way since it takes care of these
> problems. Please feel free to use my patches as you see fit (I didn't
> see much activity on them since last many months so I have moved on to
> something else now).
I'm curious to know what was the fate of these patches. I know you
were working on a version of it. Did the privacy-screen feature
actually find some traction upstream and was accepted in some form?
Thanks,
Rajat
>
> Thanks & Best Regards,
>
> Rajat
>
> >
> > REgards,
> >
> > Hans
> >
> >
> >
> >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > > index 0c7be8ed1423a..e4594e27ce5a8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > > @@ -123,4 +123,9 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
> > >
> > > u32 intel_dp_mode_to_fec_clock(u32 mode_clock);
> > >
> > > +void
> > > +intel_dp_update_privacy_screen(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state,
> > > + const struct drm_connector_state *conn_state);
> > > +
> > > #endif /* __INTEL_DP_H__ */
> > >
> >