Hello Hans, On Mon, Jul 6, 2020 at 5:50 PM Rajat Jain <rajatja@xxxxxxxxxx> wrote: > > Hello Hans, > > On Mon, Jul 6, 2020 at 5:51 AM Hans de Goede <hdegoede@xxxxxxxxxx> 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 <rajatja@xxxxxxxxxx> > > > > 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__ */ > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
