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
