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

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

Hi,

On 9/17/21 6:25 PM, Ville Syrjälä wrote:
> On Fri, Sep 17, 2021 at 04:37:14PM +0200, Hans de Goede wrote:
>> 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(),
> 
> No. That's just when those annoying foo_state->state pointers get
> clobbered. We've been moving away from using those and just
> plumbing the entire atomic state everywhere.
> 
> Nothing actually gets freed until the whole drm_atomic_state gets
> nuked after the commit is done.
> 
>> 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.
> 
> In general fastset is is just an optimized modeset. Userspace asked
> for a modeset, but we noticed it doesn't need it. I don't think
> there is a particular expectation that it's super fast.
> 
> But if this is really annoyingly slow in some actual usecase

Yeah these acpi-calls might take like a 100 ms easily, so
we really want to avoid it if it is not necessary.

> then
> one way to avoid that need to compare against the old state is just
> introduce another foo_changed flag.

Ok, so I have the feeling that you have an idea of how you think this
should be done / how this code should look instead of what I have
currently.

Can you perhaps provide a rough sketch / description of how you
think this should be done (instead of the current implementation) ?

Should I do the update from the the encoder update_pipe (for fast-sets)
and enable (for full modesets) callbacks instead as I mention in
the commit message ?

And since I still only want to do the call if there is an actual
change, where could I best do the old / new sw_state change cmp to
set the new foo_changed flag?




> 
>>
>>>
>>> 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.
> 
> You cut the code too soon. Just after this you call the other
> update_privacy_screen() thing which does poke at
> connector->state->stuff AFAICS.

True, the idea here is to only update the hw_state, the returned sw_state
should always be the one which we just set. But I agree it would be better to
change the code here so that drm_connector_update_privacy_screen() only
updates privacy_screen_hw_state I will change the code to do this in the
next version of this patch-set.

Regards,

Hans