by Takashi Iwai

[permalink] [raw]

Subject: Re: Commit 500f7147cf5bafd139056d521536b10c2bc2e154 breaks _resume_

At Mon, 7 Feb 2011 13:02:46 +0800,
Jeff Chua wrote:
>
> On Mon, Feb 7, 2011 at 12:48 PM, Jeff Chua <[email protected]> wrote:
> > On Sun, Feb 6, 2011 at 11:27 PM, Chris Wilson <[email protected]> wrote:
> >> One last step: move contents of intel_crtc_reset() back to
> >> intel_crtc_init() one by one.
> >>
> >> The active flag is my suspicion. I was thinking that we brought up the
> >> outputs in a similar manner upon resume as upon initial boot. On
> >> reflection, this is the not case.
> >>
> >> However, the first action we take inside modesetting is to disable the
> >> outputs about to be reconfigured. So setting active should be the right
> >> course of action so that cleanup any residual state from resume.
> >>
> >> So I am intrigued as to which line is the cause, and just where the
> >> machine becomes unresponsive...
> >
> > It's this line causing the problem.
> >
> > intel_crtc->active = true; /* force the pipe off on setup_init_config */
> >
> >
> > When it's called before entering intel_crtc_reset(&intel_crtc->base),
> > it works, but if called within the function, it doesn't work. Strange.
> > Not sure whether is passing the correct value to to_intel_crtc(crtc)?
>
> I've added printk() below and the function returns a different value
> of intel_crtc.
>
>
> static void intel_crtc_reset(struct drm_crtc *crtc)
> {
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> printk("intel_crtc %p\n", intel_crtc); ===> intel_crtc ffff8802349d1000
>
> }
>
> printk("intel_crtc %p\n", intel_crtc); ===> intel_crtc ffff8802349d0000
> intel_crtc_reset(&intel_crtc->base);

That's weird. Since base is the first member, both intel_crtc and crtc
must be identical.

Takashi

2011-02-07 08:36:38

On Tue, Feb 8, 2011 at 9:36 PM, Chris Wilson <[email protected]> wrote:
> On Mon, 7 Feb 2011 22:11:29 +0800, Jeff Chua <[email protected]> wrote:
>> Same issue encountered even after a fresh build.
>
> I've been testing on the same hardware (x201s, 64bit, with and without
> external DP) and I've not encountered the same issue. Every time I look
> at the code, by not setting crtc->active we may incorrectly skip disabling
> an active output after resume. If the output is inactive, then disabling it
> *should* be a no-op, and thus safe. So I am still at a loss to understand
> what fails here and so whether we have a bigger problem on our hands.

Same X201s ... that's good. I'm using "echo mem >/sys/power/state" to suspend.
>
> Please can you add drm.debug=0xe to your boot parameters (or echo 0xe >
> /sys/module/drm/parameters/debug might just be enough) and attach the
> dmesg whilst resuming. Also attaching the failing Xorg.0.log would be
> useful to rule out any other issues.

Attached. The Xorg.0.log didn't log anything after resume because the
system hanged, and kernel log is "partially" logged. I've attached my
kernel .config as well.

And the console hangs even without starting X.

Thanks,
Jeff

Attachments:

kernel (142.76 kB)
Xorg.0.log.old (18.65 kB)
.config (73.85 kB)
Download all attachments

2011-02-09 01:05:30

On Wed, February 9, 2011 10:32, Chris Wilson wrote:
> On Wed, 9 Feb 2011 03:56:36 +0100 (CET), "Indan Zupancic" <[email protected]> wrote:
>> All in all it seems quite wrong, no matter if it happens to work, because
>> it depends on the calling order done by the drm layer. If *_crtc_enable()
>> is called instead it won't do anything because of that active = true thing.
>> This seems to be happening in your case.
>
> The order is very well defined.
>
> modesetting (upon resume we set the previous mode):
> for each enabled crtc:
> crtc_helper->prepare -> intel_crtc_disable()
> crtc->mode_set -> intel_crtc_mode_set()
> crtc_helper->commit -> intel_crtc_enable()
> for each !enabled crtc:
> crtc->disable -> intel_crtc_disable()

Prepare for what? That could mean anything. Is it only used to
prepare changing a mode, or suspend, or what? Same for commit.
Should it have been named modes_set_prepare and mode_set_commit?

It's clear if you've worked a lot on the code, but if you're just
debugging it's had to tell when anything will be called, the
function pointer chasing isn't fun. And that doesn't guarantee
a driver function won't be called in a different way in the
future anyway.

I'd go as far as saying that if that's always the order, then
just get rid of prepare and commit and do what's needed in the
mode_set function. If prepare and commit don't have any other
purpose or reason for existence.

And back to the original thing, where does that reset() callback
fit in? It's absent from the very well defined order. And judging
by its name, it should be okay to call at any moment, but it seems
that isn't the case.

But the real wrong thing is fiddling with "active" outside of those
state changing functions to force a certain behaviour later on if a
specific function is called just after this (fingers crossed).

My advise is to just remove that active = true line and fix any
breakage that results in some other way. It acts as an out-of-band
message to *crtc_disable() to re-disable already disabled crtcs,
which doesn't always work (either now or in the future). If the state
wasn't already messed up, it is after doing the active = true thing.

Greetings,

Indan