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

["Ville Syrjälä" <ville.syrjala@linux.intel.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