Re: [Intel-gfx] [RFC PATCH 3/3] i915: ignore lid open event when resuming

[Zhang Rui <rui.zhang@intel.com>]

On Mon, 2013-01-28 at 09:31 +0100, Daniel Vetter wrote:
> On Mon, Jan 28, 2013 at 3:36 AM, Zhang Rui <rui.zhang@intel.com> wrote:
> >> Given that this essentially requires users to manually set this module
> >> option to make stuff work I don't like this.
> >>
> > sorry, I do not understand.
> > this patch just disables modeset_on_lid during suspend.
> 
> Pardon me, the misunderstanding is on my side - I've mixed up
> dev_priv->modeset_on_lid with the corresponding module option.
> 
> Is my understanding correct that only with the new SCI pm state this
> can happen, since that still allows acpi events to be processed (and
> so our lid_notifier being called?
> 
yep.

> >> I see a few possible options:
> >> - plug the races between all the different parts - I've never really
> >> understood why acpi sends us events before the resume code has
> >> completed ... If that's indeed intentional, we could delay the
> >> handling a bit until at least the i915 resume code completed.
> >
> > IMO, the real question is that, during a suspend/resume cycle, if we
> > need to take care of the lid open event or not.
> > To me, the answer is no, because when resuming from STR, i915 is not
> > aware of such an event, and the i915 resume code works well, right?
> > so I do not think it is a problem to ignore the lid event for another
> > lightweight suspend state, which is introduced in Patch 1/3 in this
> > series.
> >
> >> - Judging from the diff context you're not on the latest 3.8-rc
> >> codebase - we've applied a few fixes to this lid hack recently. Please
> >> retest on a kernel with
> >>
> > the patch is based on 3.8.0-rc3, which already includes the commit
> > below.
> > And yes, the problem still exists.
> 
> Ok, I think I see the issue now. But I suspect that we need some
> additional locking to make this solid, since currently
> dev_priv->modeset_on_lid updates can race with our lid notifier
> handler. Let me think about this a bit more.

hmm,
with this patch, intel_lid_notify() will return immediately if
modeset_on_lid is set to -1. So the only problem in my mind is that we
got a lid open event during i915_suspend().

Say,
lid is opened -> i915_lid_notify() is invoked (modeset_on_lid is 1 at
this time) -> i915_suspend() set modeset_on_lid to -1, just before
intel_modeset_setup_hw_state() being invoked in i915_lid_notify() ->
intel_modeset_setup_hw_state() breaks the system.

but my first question would be is this (lid open on suspend) possible in
real world?
If the answer is yes, then my second question is that, this problem
exists for STR as well because SCI is still valid at this time when
suspending to memory, have we seen such issues for STR before?

Anyway, if the current code does not break STR, this patch should be
enough for the new suspend state.
what do you think?

thanks,
rui