On Thu, Sep 20, 2012 at 10:56 AM, Chris Wilson <chris at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
