From: Rajat Jain <rajatja@google.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Sean Paul <seanpaul@google.com>, David Airlie <airlied@linux.ie>,
Sugumaran Lacshiminarayanan <slacshiminar@lenovo.com>,
dri-devel <dri-devel@lists.freedesktop.org>,
Chris Wilson <chris@chris-wilson.co.uk>,
Daniel Thompson <daniel.thompson@linaro.org>,
Jonathan Corbet <corbet@lwn.net>,
Mark Pearson <mpearson@lenovo.com>,
Tomoki Maruichi <maruichit@lenovo.com>,
Jesse Barnes <jsbarnes@google.com>,
Rajat Jain <rajatxjain@gmail.com>,
intel-gfx@lists.freedesktop.org, Mat King <mathewk@google.com>,
Maxime Ripard <mripard@kernel.org>,
Duncan Laurie <dlaurie@google.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Pavel Machek <pavel@denx.de>, Nitin Joshi1 <njoshi1@lenovo.com>
Subject: Re: [Intel-gfx] [PATCH v9 5/5] drm/i915: Enable support for integrated privacy screen
Date: Tue, 9 Mar 2021 12:38:22 -0800 [thread overview]
Message-ID: <CACK8Z6EVo7rJJSbFfySTSKRyduPY31oj2BtBBFbyy0QWVb5h0w@mail.gmail.com> (raw)
In-Reply-To: <CACK8Z6Hd8WqEKi9MQdyoW0bMpjGiOsUrzYTm0HRaXscRWYS4aA@mail.gmail.com>
Hello Hans,
On Mon, Jul 6, 2020 at 5:50 PM Rajat Jain <rajatja@google.com> wrote:
>
> Hello Hans,
>
> On Mon, Jul 6, 2020 at 5:51 AM Hans de Goede <hdegoede@redhat.com> 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@google.com>
> >
> > 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
next prev parent reply other threads:[~2021-03-09 20:39 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-12 18:56 [Intel-gfx] [PATCH v9 0/5] drm/i915 Support for integrated privacy screen Rajat Jain
2020-03-12 18:56 ` [Intel-gfx] [PATCH v9 1/5] intel_acpi: Rename drm_dev local variable to dev Rajat Jain
2020-03-12 18:56 ` [Intel-gfx] [PATCH v9 2/5] drm/connector: Add support for privacy-screen property Rajat Jain
2020-03-12 18:56 ` [Intel-gfx] [PATCH v9 3/5] drm/i915: Lookup and attach ACPI device node for connectors Rajat Jain
2020-03-12 18:56 ` [Intel-gfx] [PATCH v9 4/5] drm/i915: Add helper code for ACPI privacy screen Rajat Jain
2020-03-12 18:56 ` [Intel-gfx] [PATCH v9 5/5] drm/i915: Enable support for integrated " Rajat Jain
2020-07-06 12:51 ` Hans de Goede
2020-07-07 0:50 ` Rajat Jain
2021-03-09 20:38 ` Rajat Jain [this message]
2020-03-12 19:12 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915 Support " Patchwork
2020-03-12 19:35 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-03-13 9:20 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-03-25 18:21 ` [Intel-gfx] [PATCH v9 0/5] " Rajat Jain
2020-07-06 12:55 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915 Support for integrated privacy screen (rev2) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACK8Z6EVo7rJJSbFfySTSKRyduPY31oj2BtBBFbyy0QWVb5h0w@mail.gmail.com \
--to=rajatja@google.com \
--cc=airlied@linux.ie \
--cc=chris@chris-wilson.co.uk \
--cc=corbet@lwn.net \
--cc=daniel.thompson@linaro.org \
--cc=dlaurie@google.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=hdegoede@redhat.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jsbarnes@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maruichit@lenovo.com \
--cc=mathewk@google.com \
--cc=mpearson@lenovo.com \
--cc=mripard@kernel.org \
--cc=njoshi1@lenovo.com \
--cc=pavel@denx.de \
--cc=rajatxjain@gmail.com \
--cc=seanpaul@google.com \
--cc=slacshiminar@lenovo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).