[PATCH] drm/i915: Flush pending operations to the CRTC prior to modeset

[PATCH] drm/i915: Flush pending operations to the CRTC prior to modeset

[Chris Wilson]

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(-)

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

Re: [PATCH] drm/i915: Flush pending operations to the CRTC prior to modeset

[Daniel Vetter]

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Flush pending operations to the CRTC prior to modeset

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC