Re: Debugging Thinkpad T430s occasional suspend failure.

[Daniel Vetter]

On Sun, Feb 17, 2013 at 2:38 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> 2. The new i915 force restore code is indeed missing a safety check
> compared to the old crtc helper based one:
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6eb3882..095094c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9153,7 +9153,13 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>
>         if (force_restore) {
>                 for_each_pipe(pipe) {
> -                       intel_crtc_restore_mode(dev_priv->pipe_to_crtc_mapping[pipe]);
> +                       struct drm_crtc * crtc =
> +                               dev_priv->pipe_to_crtc_mapping[pipe];
> +
> +                       if (!crtc->enabled)
> +                               continue;
> +
> +                       intel_crtc_restore_mode(crtc);
>                 }
>
>                 i915_redisable_vga(dev);
>
> The issue is that we save a pointer to the fb (since those are refcounted)
> but copy the mode into the crtc struct (since modes are not refcounted).
> So on restore the mode will always be non-NULL, which is wrong if the crtc
> is off (and so also has a NULL fb).
>
> The problem I have with that patch is figuring out how this ever worked. I
> think I need more coffee ;-)

Ok, coffee seems to be working now. I think the above diff shouldn't
change anything, since we already have a crtc->enabled check in
intel_modeset_affected_pipes in intel_display.c. Still would be good
if you can prove this one way or the other.

For those wondering why this check is buried this deeply: We're in the
middle of a massive rework of our modeset code, moving from
one-crtc-at-a-time to global modeset. We need that to implement some
fancy features like fastboot or better handling of global resource
constraints (shared clocks, bw limits, ...). In the new world we set
up the desired state in staging pointers/data structures. Then the
modeset code diffs that with the current state and computes the best
way to do the transition. But since we're still converting code some
pieces pass in the new state explicitly, but lower levels then ignore
some pieces when not required to reach the desired state.

The new lid restore code relies on that by updating the tracked hw
state from the real hw one and restoring the last desired state (which
is still around from the last modeset call).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/