From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.6 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_INVALID,DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 06676C433DB for ; Tue, 9 Mar 2021 20:39:03 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 90BE764F4B for ; Tue, 9 Mar 2021 20:39:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 90BE764F4B Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3211B6E96A; Tue, 9 Mar 2021 20:39:02 +0000 (UTC) Received: from mail-lj1-x229.google.com (mail-lj1-x229.google.com [IPv6:2a00:1450:4864:20::229]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1FF216E96A for ; Tue, 9 Mar 2021 20:39:01 +0000 (UTC) Received: by mail-lj1-x229.google.com with SMTP id p15so22531893ljc.13 for ; Tue, 09 Mar 2021 12:39:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=zMwwa8yu3ZnyIGmWM3xIJxPV47GpT6zhiy8aZvE7KQc=; b=Rm+8yVVyqVAqWmZ9v0inuIYfjDJnheKeGDfTrUVdu+57n8vFZQiUh4veJx6VUp1paa wSId0acTVjlHgXo0HdJuup1SxvhGMJrxGmf9Mi9RYmdGPK7Nolmiqlz/es73BymfqA68 qlglywaNi7D8Rtpg+M+ns3LGqUqw0LI7tyKJ4YwYDYqSHT6yLSFCZS94OqAph9AQFuZH B9S95xcKvahCvL+ytuyoIRyIACu5SIYg6J4LHJ37X3tgx8m1T+URAqoRXNpTT7nAIai8 eYzU83u0vVFk16858RN4OhNJY1Ie4dKvzSeWyjn3+VIugKhvlsmycJi3qxJodUUKKK+C oaZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zMwwa8yu3ZnyIGmWM3xIJxPV47GpT6zhiy8aZvE7KQc=; b=pA3gKc02SbWiAVmomFWEDO4ebQngn/d0glkNS/9jXZmtak6siFAHjkhZaxRRQLbhFG 38aVfzhc47M1pZwCoi9zBXZUuTw/lIZL9VyV7oX1Ag3JFP8pFzwypj0Skp5OsPoPFjqK gCQdT6LNFb2qb32zQWS+UltiREjZZzjnlO9tnNBYR3Jltssq0ytdTO+NtV3cFImRZVnC d34h7BTUkhjywKCphnWRnvkDwJdfOb4WED3ijww8TYCn2Iz6hSaA9Sk/34n54LdVKeOc 4pTcvI4Kz7B+zKrNZTwmgt4N616GV9yLtVJ92Lv5P2wgBN1WssP+fQdrOzIK6isDUHag KjQA== X-Gm-Message-State: AOAM530ndaC7n8Kom8IU04I2oKQUAY/2AHUjGSsfKzHwoa7fe5HuGF3r NVrJEaIPHs/qOYDLJoPXdSwuwiSzY8sOQ6yDlq010g== X-Google-Smtp-Source: ABdhPJw5fjWk71nOKny9Xvb3l1nQBQr9GHIPnH/4seWqpbTGGIoETGefTmswEHTxcKlurybig+1SBJPOCSavkIGLI3k= X-Received: by 2002:a2e:91c2:: with SMTP id u2mr12581600ljg.301.1615322339148; Tue, 09 Mar 2021 12:38:59 -0800 (PST) MIME-Version: 1.0 References: <20200312185629.141280-1-rajatja@google.com> <20200312185629.141280-6-rajatja@google.com> <76d1a721-5f7b-1e86-b8ee-183bffb78ff1@redhat.com> In-Reply-To: From: Rajat Jain Date: Tue, 9 Mar 2021 12:38:22 -0800 Message-ID: To: Hans de Goede Subject: Re: [Intel-gfx] [PATCH v9 5/5] drm/i915: Enable support for integrated privacy screen X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Sean Paul , David Airlie , Sugumaran Lacshiminarayanan , dri-devel , Chris Wilson , Daniel Thompson , Jonathan Corbet , Mark Pearson , Tomoki Maruichi , Jesse Barnes , Rajat Jain , intel-gfx@lists.freedesktop.org, Mat King , Maxime Ripard , Duncan Laurie , Greg Kroah-Hartman , Linux Kernel Mailing List , Pavel Machek , Nitin Joshi1 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Hello Hans, On Mon, Jul 6, 2020 at 5:50 PM Rajat Jain wrote: > > Hello Hans, > > On Mon, Jul 6, 2020 at 5:51 AM Hans de Goede 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 > > > > 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx