| Message ID | 1365690550-5716-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 11, 2013 at 04:29:05PM +0200, Daniel Vetter wrote: > Yet again our current confusion between doing the modeset globally, > but only having the new parameters for one crtc at a time. > > This time things blew up when restoring modes in the switchless resume > code - intel_modeset_affected_pipes figured out that pipe 2 should > be restored, but since pipe 1 was disabled there was no mode nor fb > when trying to restore the first crtc. > > Hilarity ensued and broke resume on my i945gme machine since the > pipe_config_set_bpp added in > > commit 4e53c2e010e531b4a014692199e978482d471c7e > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Wed Mar 27 00:44:58 2013 +0100 > > drm/i915: precompute pipe bpp before touching the hw > > fell over the lack of an fb. > > Fix this mess by now by justing shunting all the cool new global > modeset logic in intel_modeset_affected_pipes. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Potentially the same bug reported against 3.8.1: Bugzill: https://bugzilla.redhat.com/show_bug.cgi?id=917725 Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/intel_display.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f60493b..58c6bb6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7629,6 +7629,10 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes, > /* ... and mask these out. */ > *modeset_pipes &= ~(*disable_pipes); > *prepare_pipes &= ~(*disable_pipes); > + > + /* HACK: We don't (yet) fully support global modesets. */ > + *modeset_pipes &= 1 << intel_crtc->pipe; > + *prepare_pipes &= 1 << intel_crtc->pipe; > } > > static bool intel_crtc_in_use(struct drm_crtc *crtc) > -- > 1.7.10.4 >
On Thu, Apr 11, 2013 at 11:09:00PM +0200, Daniel Vetter wrote: > On Thu, Apr 11, 2013 at 04:29:05PM +0200, Daniel Vetter wrote: > > Yet again our current confusion between doing the modeset globally, > > but only having the new parameters for one crtc at a time. > > > > This time things blew up when restoring modes in the switchless resume > > code - intel_modeset_affected_pipes figured out that pipe 2 should > > be restored, but since pipe 1 was disabled there was no mode nor fb > > when trying to restore the first crtc. > > > > Hilarity ensued and broke resume on my i945gme machine since the > > pipe_config_set_bpp added in > > > > commit 4e53c2e010e531b4a014692199e978482d471c7e > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Wed Mar 27 00:44:58 2013 +0100 > > > > drm/i915: precompute pipe bpp before touching the hw > > > > fell over the lack of an fb. > > > > Fix this mess by now by justing shunting all the cool new global > > modeset logic in intel_modeset_affected_pipes. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Potentially the same bug reported against 3.8.1: > > Bugzill: https://bugzilla.redhat.com/show_bug.cgi?id=917725 > Cc: stable@vger.kernel.org References: http://www.mail-archive.com/stable@vger.kernel.org/msg38084.html Tested-by: Richard Cochran <richardcochran@gmail.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index f60493b..58c6bb6 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -7629,6 +7629,10 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes, > > /* ... and mask these out. */ > > *modeset_pipes &= ~(*disable_pipes); > > *prepare_pipes &= ~(*disable_pipes); > > + > > + /* HACK: We don't (yet) fully support global modesets. */ > > + *modeset_pipes &= 1 << intel_crtc->pipe; > > + *prepare_pipes &= 1 << intel_crtc->pipe; > > } > > > > static bool intel_crtc_in_use(struct drm_crtc *crtc) > > -- > > 1.7.10.4 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Apr 11, 2013 at 04:29:05PM +0200, Daniel Vetter wrote: > Yet again our current confusion between doing the modeset globally, > but only having the new parameters for one crtc at a time. > > This time things blew up when restoring modes in the switchless resume > code - intel_modeset_affected_pipes figured out that pipe 2 should > be restored, but since pipe 1 was disabled there was no mode nor fb > when trying to restore the first crtc. > > Hilarity ensued and broke resume on my i945gme machine since the > pipe_config_set_bpp added in I can see how the hack works, but I'm not clear on how we got into the situation of trying to enable multiple pipes in the first place. Do you mind expanding upon the failure conditions a bit more? -Chris
On Fri, Apr 12, 2013 at 11:46 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Apr 11, 2013 at 04:29:05PM +0200, Daniel Vetter wrote: >> Yet again our current confusion between doing the modeset globally, >> but only having the new parameters for one crtc at a time. >> >> This time things blew up when restoring modes in the switchless resume >> code - intel_modeset_affected_pipes figured out that pipe 2 should >> be restored, but since pipe 1 was disabled there was no mode nor fb >> when trying to restore the first crtc. >> >> Hilarity ensued and broke resume on my i945gme machine since the >> pipe_config_set_bpp added in > > I can see how the hack works, but I'm not clear on how we got into the > situation of trying to enable multiple pipes in the first place. Do you > mind expanding upon the failure conditions a bit more? Yeah, that's worth spilling a few words ;-) So the issue is that intel_set_mode essentially already does a global modeset: intel_modeset_affected_pipes compares the current state with where we want to go to (which is carefully set up by intel_crtc_set_config) and then goes through the modeset sequence for any crtc which needs updating. Now the issue is that the actual interface with the remaining code still only works on one crtc, and so we only pass in one fb and one mode. In intel_set_mode we also only compute one intel_crtc_config (which should be the one for the crtc we're doing a modeset on). The reason for that mismatch is twofold: - We want to eventually do all modeset as global state changes, so it's just infrastructure prep. - But even the old semantics can change more than one crtc when you e.g. move a connector from crtc A to crtc B, then both crtc A and B need to be updated. Usually that means one pipe is disabled and the other enabled. This is also the reason why the hack doesn't touch the disable_pipes mask. Now hilarity ensued in our kms config restore paths when we actually try to do a modeset on all crtcs: If the first crtc should be off and the second should be on, then the call on the first crtc will notice that the 2nd one should be switched on and so tries to compute the pipe_config. But due to a lack of passed-in fb (crtc 1 should be off after all) it only results in tears. This case is ridiculously easy to hit on gen2/3 where the lvds output is restricted to pipe B ;-) Also, before the pipe_config bpp rework gen2/3 didn't care really about the fb->depth and so just stumbled a bit, but never fell over completely. But apparently Ajax also managed to blow up pch platforms, probably with some randomized configs, and pch platforms trip up over the lack of an fb even in the old code. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f60493b..58c6bb6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7629,6 +7629,10 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes, /* ... and mask these out. */ *modeset_pipes &= ~(*disable_pipes); *prepare_pipes &= ~(*disable_pipes); + + /* HACK: We don't (yet) fully support global modesets. */ + *modeset_pipes &= 1 << intel_crtc->pipe; + *prepare_pipes &= 1 << intel_crtc->pipe; } static bool intel_crtc_in_use(struct drm_crtc *crtc)
Yet again our current confusion between doing the modeset globally, but only having the new parameters for one crtc at a time. This time things blew up when restoring modes in the switchless resume code - intel_modeset_affected_pipes figured out that pipe 2 should be restored, but since pipe 1 was disabled there was no mode nor fb when trying to restore the first crtc. Hilarity ensued and broke resume on my i945gme machine since the pipe_config_set_bpp added in commit 4e53c2e010e531b4a014692199e978482d471c7e Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Wed Mar 27 00:44:58 2013 +0100 drm/i915: precompute pipe bpp before touching the hw fell over the lack of an fb. Fix this mess by now by justing shunting all the cool new global modeset logic in intel_modeset_affected_pipes. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 4 ++++ 1 file changed, 4 insertions(+)