Re: [PATCH 9/9] drm/i915: Add privacy-screen support

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: 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>,
	Lyude <lyude@redhat.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>,
	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: [PATCH 9/9] drm/i915: Add privacy-screen support
Date: Thu, 16 Sep 2021 16:45:43 +0300	[thread overview]
Message-ID: <YUNKh9xcIGoi1eol@intel.com> (raw)
In-Reply-To: <20210906073519.4615-10-hdegoede@redhat.com>

On Mon, Sep 06, 2021 at 09:35:19AM +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.

Pretty sure the full atomic state is plumbed all the way
down these days.

> 
> 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.

I doubt anyone is going to care about a bit of overhead for a modeset.
The usual rule is that a modeset doesn't skip anything. That way we
can be 100% sure we remeber to update everythinbg. For fastsets I guess
one could argue skipping it if not needed, but not sure even that is
warranted.

The current code you have in there is cettainly 110% dodgy. Since the
sw_state is stored in the connector state I presume it's at least
trying to be an atomic property, which means you shouldn't go poking
at it after the swap_state ever. swap_state just swaps the pointers
which is all that you need. So at least that part should get nuked.
The immutable hw_state I guess should get updated when we program the
actual hw.

-- 
Ville Syrjälä
Intel