| Message ID | 1348131363-24529-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 20, 2012 at 10:56 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > We need to wait for pending operations on the CRTC to retire before we > can modify the CRTC. For example, if userspace has queued a batch that > uses a WAIT_FOR_EVENT associated with the current FB, we can not modify > the pipe with that outstanding, as we may then prevent that > WAIT_FOR_EVENT from ever completing and so hanging the GPU. (Imagine a > scanline wait waiting for line 1024 and the pipe being adjusted to > 600-line mode.) There is also the sequencing issue of the immediate > update versus a pending pageflip. > > In both cases the function to serialise the modeset with the pending > operations existed but was simply not being called, or called after the > damage was already done. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> I've looked at this situation again and we do have a wait_for_pending_flips already in per-platform crtc_disable functions, which are called for for switching off crtcs and also only just disabling them for a modeset. So I think this finish_fb call in set_base is totally unnecessary and can be just removed. Moving it to the crtc_set_config function doesn't help (and this patch misses the case where we disable other crtcs than set->crtc). -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 48de2b1..5527589 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2275,9 +2275,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > return ret; > } > > - if (crtc->fb) > - intel_finish_fb(crtc->fb); > - > ret = dev_priv->display.update_plane(crtc, fb, x, y); > if (ret) { > intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj); > @@ -7475,6 +7472,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > save_set.y = set->crtc->y; > save_set.fb = set->crtc->fb; > > + /* Synchronize pending operations before apply immediate changes */ > + intel_crtc_wait_for_pending_flips(set->crtc); > + > /* Compute whether we need a full modeset, only an fb base update or no > * change at all. In the future we might also check whether only the > * mode changed, e.g. for LVDS where we only change the panel fitter in > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 20, 2012 at 11:17:51AM +0200, Daniel Vetter wrote: > On Thu, Sep 20, 2012 at 10:56 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > We need to wait for pending operations on the CRTC to retire before we > > can modify the CRTC. For example, if userspace has queued a batch that > > uses a WAIT_FOR_EVENT associated with the current FB, we can not modify > > the pipe with that outstanding, as we may then prevent that > > WAIT_FOR_EVENT from ever completing and so hanging the GPU. (Imagine a > > scanline wait waiting for line 1024 and the pipe being adjusted to > > 600-line mode.) There is also the sequencing issue of the immediate > > update versus a pending pageflip. > > > > In both cases the function to serialise the modeset with the pending > > operations existed but was simply not being called, or called after the > > damage was already done. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > I've looked at this situation again and we do have a > wait_for_pending_flips already in per-platform crtc_disable functions, > which are called for for switching off crtcs and also only just > disabling them for a modeset. > > So I think this finish_fb call in set_base is totally unnecessary and > can be just removed. Moving it to the crtc_set_config function doesn't > help (and this patch misses the case where we disable other crtcs than > set->crtc). This whole wait for pending flips mechanism looks iffy to me. First, when we schedule a flip, we record the fact that there is a pending flip into the old bo's pending_flip atomic mask thingy. When we later want to wait for the CRTC operations to finish, we do 'intel_finish_fb(crtc->fb)'. But notice that 'crtc->fb' is now the fb we flipped _to_, so it can't actually tell us whether there's still a flip pending.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 48de2b1..5527589 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2275,9 +2275,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, return ret; } - if (crtc->fb) - intel_finish_fb(crtc->fb); - ret = dev_priv->display.update_plane(crtc, fb, x, y); if (ret) { intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj); @@ -7475,6 +7472,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set) save_set.y = set->crtc->y; save_set.fb = set->crtc->fb; + /* Synchronize pending operations before apply immediate changes */ + intel_crtc_wait_for_pending_flips(set->crtc); + /* Compute whether we need a full modeset, only an fb base update or no * change at all. In the future we might also check whether only the * mode changed, e.g. for LVDS where we only change the panel fitter in
We need to wait for pending operations on the CRTC to retire before we can modify the CRTC. For example, if userspace has queued a batch that uses a WAIT_FOR_EVENT associated with the current FB, we can not modify the pipe with that outstanding, as we may then prevent that WAIT_FOR_EVENT from ever completing and so hanging the GPU. (Imagine a scanline wait waiting for line 1024 and the pipe being adjusted to 600-line mode.) There is also the sequencing issue of the immediate update versus a pending pageflip. In both cases the function to serialise the modeset with the pending operations existed but was simply not being called, or called after the damage was already done. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_display.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)