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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 28, 2013 at 06:06:38PM +0800, Zhang Rui wrote:
> 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?

I think I have two wishlist changes for your patch ;-)

- Convert dev_priv->modeset_on_lid to an enum, so that we have descriptive
  names instead of magic numbers.

- I think we can close all races by adding a new lid_notifier mutex (I
  prefer a new lock instead of overloading an existing one with). If we
  hold that in the suspend/resume paths just for changing modeset_on_lid
  and also for the entire lid notifier callback (i.e. grab the lock before
  first looking at modest_on_lid, only drop it once the modeset fixup has
  been completed at the end of the function) we should be race-free in all
  cases.

  Also, I think we should move the modeset_on_lid updates in the
  suspend/resume paths to the very beginning/end.

Can you pls update your patch with these changes? Or do you see an
additional race we need to plug somewhere?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch