From: Hans de Goede <hdegoede@redhat.com>
To: Lyude Paul <lyude@redhat.com>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
Rajat Jain <rajatja@google.com>,
Jani Nikula <jani.nikula@linux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>,
Mark Gross <mgross@linux.intel.com>,
Andy Shevchenko <andy@infradead.org>
Cc: Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
Pekka Paalanen <pekka.paalanen@collabora.com>,
Mario Limonciello <mario.limonciello@outlook.com>,
Mark Pearson <markpearson@lenovo.com>,
Sebastien Bacher <seb128@ubuntu.com>,
Marco Trevisan <marco.trevisan@canonical.com>,
Emil Velikov <emil.l.velikov@gmail.com>,
intel-gfx <intel-gfx@lists.freedesktop.org>,
dri-devel@lists.freedesktop.org,
platform-driver-x86@vger.kernel.org
Subject: Re: [Intel-gfx] [PATCH 9/9] drm/i915: Add privacy-screen support
Date: Thu, 16 Sep 2021 11:12:01 +0200 [thread overview]
Message-ID: <e58d334a-ce05-29a9-d0f5-39068e2b94b9@redhat.com> (raw)
In-Reply-To: <65286f7effc8b336f28f0c6f92fa9ae65e6e621b.camel@redhat.com>
Hi,
On 9/15/21 11:11 PM, Lyude Paul wrote:
> On Mon, 2021-09-06 at 09:35 +0200, Hans de Goede wrote:
>> Add support for eDP panels with a built-in privacy screen using the
>> new drm_privacy_screen class.
>>
>> One thing which stands out here is the addition of these 2 lines to
>> intel_atomic_commit_tail:
>>
>> for_each_new_connector_in_state(&state->base, connector, ...
>> drm_connector_update_privacy_screen(connector, state);
>>
>> It may seem more logical to instead take care of updating the
>> privacy-screen state by marking the crtc as needing a modeset and then
>> do this in both the encoder update_pipe (for fast-sets) and enable
>> (for full modesets) callbacks. But ATM these callbacks only get passed
>> the new connector_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 was going to suggest that you workaround this simply by adding a variable
> that corresponds to the most recently committed privacy screen state somewhere
> in a driver private structure. But, then I realized that's basically the same
> as what you're doing now except that your current solution stores said state
> in a shared struct. So, I think you probably do have the right idea here as
> long as we don't get any non-ACPI providers in the future. This also seems
> like something that wouldn't be difficult to fixup down the line if that ends
> up changing.
Ack, this is all kernel internal stuff so we can always rework it if necessary.
Regards,
Hans
>
>>
>> Without access to the old state, we do not know if the sw_state of
>> the privacy-screen has changes so we would need to call
>> drm_privacy_screen_set_sw_state() unconditionally. This is undesirable
>> since all current known privacy-screen providers use ACPI calls which
>> are somewhat expensive to make.
>>
>> Also, as all providers use ACPI calls, rather then poking GPU registers,
>> there is no need to order this together with other encoder operations.
>> Since no GPU poking is involved having this as a separate step of the
>> commit process actually is the logical thing to do.
>>
>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_display.c | 5 +++++
>> drivers/gpu/drm/i915/display/intel_dp.c | 10 ++++++++++
>> drivers/gpu/drm/i915/i915_pci.c | 12 ++++++++++++
>> 3 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>> b/drivers/gpu/drm/i915/display/intel_display.c
>> index 5560d2f4c352..7285873d329a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -10140,6 +10140,8 @@ static void intel_atomic_commit_tail(struct
>> intel_atomic_state *state)
>> struct drm_device *dev = state->base.dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> struct intel_crtc_state *new_crtc_state, *old_crtc_state;
>> + struct drm_connector_state *new_connector_state;
>> + struct drm_connector *connector;
>> struct intel_crtc *crtc;
>> u64 put_domains[I915_MAX_PIPES] = {};
>> intel_wakeref_t wakeref = 0;
>> @@ -10237,6 +10239,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
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 7f8e8865048f..3aa2072cccf6 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -37,6 +37,7 @@
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_dp_helper.h>
>> #include <drm/drm_edid.h>
>> +#include <drm/drm_privacy_screen_consumer.h>
>> #include <drm/drm_probe_helper.h>
>>
>> #include "g4x_dp.h"
>> @@ -5217,6 +5218,7 @@ static bool intel_edp_init_connector(struct intel_dp
>> *intel_dp,
>> struct drm_connector *connector = &intel_connector->base;
>> struct drm_display_mode *fixed_mode = NULL;
>> struct drm_display_mode *downclock_mode = NULL;
>> + struct drm_privacy_screen *privacy_screen;
>> bool has_dpcd;
>> enum pipe pipe = INVALID_PIPE;
>> struct edid *edid;
>> @@ -5308,6 +5310,14 @@ static bool intel_edp_init_connector(struct intel_dp
>> *intel_dp,
>> fixed_mode->hdisplay, fixed_mode->vdisplay);
>> }
>>
>> + privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
>> + if (!IS_ERR(privacy_screen)) {
>> + drm_connector_attach_privacy_screen_provider(connector,
>> +
>> privacy_screen);
>> + } else if (PTR_ERR(privacy_screen) != -ENODEV) {
>> + drm_warn(&dev_priv->drm, "Error getting privacy-screen\n");
>> + }
>> +
>> return true;
>>
>> out_vdd_off:
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c
>> b/drivers/gpu/drm/i915/i915_pci.c
>> index 146f7e39182a..d6913f567a1c 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -25,6 +25,7 @@
>> #include <linux/vga_switcheroo.h>
>>
>> #include <drm/drm_drv.h>
>> +#include <drm/drm_privacy_screen_consumer.h>
>> #include <drm/i915_pciids.h>
>>
>> #include "i915_drv.h"
>> @@ -1167,6 +1168,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)
>> {
>> struct intel_device_info *intel_info =
>> (struct intel_device_info *) ent->driver_data;
>> + struct drm_privacy_screen *privacy_screen;
>> int err;
>>
>> if (intel_info->require_force_probe &&
>> @@ -1195,7 +1197,17 @@ static int i915_pci_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)
>> if (vga_switcheroo_client_probe_defer(pdev))
>> return -EPROBE_DEFER;
>>
>> + /*
>> + * We do not handle -EPROBE_DEFER further into the probe process, so
>> + * check if we have a laptop-panel privacy-screen for which the
>> driver
>> + * has not loaded yet here.
>> + */
>> + privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL);
>> + if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -
>> EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> +
>> err = i915_driver_probe(pdev, ent);
>> + drm_privacy_screen_put(privacy_screen);
>> if (err)
>> return err;
>>
>
next prev parent reply other threads:[~2021-09-16 9:12 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-06 7:35 [Intel-gfx] [PATCH 0/9] drm: Add privacy-screen class and connector properties Hans de Goede
2021-09-06 7:35 ` [Intel-gfx] [PATCH 1/9] drm/connector: Add support for privacy-screen properties (v4) Hans de Goede
2021-09-15 19:48 ` Lyude Paul
2021-09-16 8:26 ` Jani Nikula
2021-09-06 7:35 ` [Intel-gfx] [PATCH 2/9] drm: Add privacy-screen class (v3) Hans de Goede
2021-09-15 20:01 ` Lyude Paul
2021-09-16 8:49 ` Hans de Goede
2021-09-06 7:35 ` [Intel-gfx] [PATCH 3/9] drm/privacy-screen: Add X86 specific arch init code Hans de Goede
2021-09-16 8:51 ` Jani Nikula
2021-09-16 9:18 ` Hans de Goede
2021-09-06 7:35 ` [Intel-gfx] [PATCH 4/9] drm/privacy-screen: Add notifier support Hans de Goede
2021-09-15 20:26 ` Lyude Paul
2021-09-16 9:06 ` Hans de Goede
2021-09-16 16:50 ` Lyude Paul
2021-09-06 7:35 ` [Intel-gfx] [PATCH 5/9] drm/connector: Add a drm_connector privacy-screen helper functions Hans de Goede
2021-09-06 7:35 ` [Intel-gfx] [PATCH 6/9] platform/x86: thinkpad_acpi: Add hotkey_notify_extended_hotkey() helper Hans de Goede
2021-09-06 7:35 ` [Intel-gfx] [PATCH 7/9] platform/x86: thinkpad_acpi: Get privacy-screen / lcdshadow ACPI handles only once Hans de Goede
2021-09-06 7:35 ` [Intel-gfx] [PATCH 8/9] platform/x86: thinkpad_acpi: Register a privacy-screen device Hans de Goede
2021-09-15 20:55 ` Lyude Paul
2021-09-16 9:09 ` Hans de Goede
2021-09-06 7:35 ` [Intel-gfx] [PATCH 9/9] drm/i915: Add privacy-screen support Hans de Goede
2021-09-15 21:11 ` Lyude Paul
2021-09-16 9:12 ` Hans de Goede [this message]
2021-09-16 9:40 ` Jani Nikula
2021-09-16 10:32 ` Hans de Goede
2021-09-20 21:06 ` Lyude Paul
2021-09-16 14:04 ` Ville Syrjälä
2021-09-17 14:23 ` Hans de Goede
2021-09-16 13:45 ` Ville Syrjälä
2021-09-17 14:37 ` Hans de Goede
2021-09-17 16:25 ` Ville Syrjälä
2021-09-17 16:42 ` Hans de Goede
2021-09-17 17:04 ` Ville Syrjälä
2021-09-06 8:46 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm: Add privacy-screen class and connector properties (rev4) Patchwork
2021-09-06 8:49 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-09-06 9:17 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-09-06 11:16 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2021-09-15 21:12 ` [Intel-gfx] [PATCH 0/9] drm: Add privacy-screen class and connector properties Lyude Paul
2021-09-16 9:30 ` Hans de Goede
2021-09-16 10:14 ` Jani Nikula
-- strict thread matches above, loose matches on Subject: below --
2020-07-08 16:43 Hans de Goede
2020-07-08 16:43 ` [Intel-gfx] [PATCH 9/9] drm/i915: Add privacy-screen support Hans de Goede
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=e58d334a-ce05-29a9-d0f5-39068e2b94b9@redhat.com \
--to=hdegoede@redhat.com \
--cc=airlied@linux.ie \
--cc=andy@infradead.org \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=emil.l.velikov@gmail.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=joonas.lahtinen@linux.intel.com \
--cc=lyude@redhat.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=marco.trevisan@canonical.com \
--cc=mario.limonciello@outlook.com \
--cc=markpearson@lenovo.com \
--cc=mgross@linux.intel.com \
--cc=mripard@kernel.org \
--cc=pekka.paalanen@collabora.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rajatja@google.com \
--cc=rodrigo.vivi@intel.com \
--cc=seb128@ubuntu.com \
--cc=tzimmermann@suse.de \
/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).