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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 17, 2021 at 06:42:04PM +0200, Hans de Goede wrote:
> 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?
>

I guess it could be just something like this:

intel_digital_connector_duplicate_state()
{
	foo_changed = false;
}

intel_digital_connector_atomic_check()
{
	if (old_foo != new_foo) {
		mode_changed = true;
		foo_changed = true;
	}
}

update_pipe()
{
	if (foo_changed)
		update_foo();
}

-- 
Ville Syrjälä
Intel