On Tue, 2013-01-29 at 11:10 +0100, Daniel Vetter wrote: > 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@xxxxxxxxx> 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. > sure, will update in next version. > - 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? > yep. will send out V2 soon. thanks for your comments. -rui -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
