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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 9/16/21 3:45 PM, Ville Syrjälä wrote:
> 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.

Including the old state? AFAICT the old-state is being thrown away
from drm_atomic_helper_swap_state(), so if we do this in a different
place then we don't have access to the old-state.


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

But this is not a modeset, this is more like changing the backlight brightness,
atm the code does not set the needs_modeset when only the privacy-screen
sw-state has changed.

Also in my experience the firmware (AML) code which we end up calling
for this is not the highest quality code, often it has interesting
issues / unhandled corner cases. So in my experience with ACPI we
really should try to avoid these calls unless we absolutely must make them,
but I guess not making unnecessary calls is something which could be handled
inside the actual privacy-screen driver instead.

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

Right, but again this is not a full modeset.

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

It is not being poked, it is only being read, also this is happening
before swap_state.

Note I'm open for suggestions to handle this differently,
including changing the drm_connector_update_privacy_screen()
helper which currently relies on being passed the state before swap_state
is called:

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");
		return;
	}

So if you have any suggestions how to do this differently, please let me know
and I will take a shot at implementing those suggestions.

Please keep in mind that the drm_privacy_screen_set_sw_state() call also
needs to happens when just the connector_state->privacy_screen_sw_state changes,
which is not a reason to do a full modeset (iow needs_modeset maybe 0 during
the commit)

Regards,

Hans