[PATCH 0/3] drm/i915: i915_gem_execbuffer_wait_for_flips and other flip stuff

[PATCH 0/3] drm/i915: i915_gem_execbuffer_wait_for_flips and other flip stuff

[ville.syrjala]

Another attempt at getting rid of i915_gem_execbuffer_wait_for_flips().

There's two versions of the patch:
 3a just skips the function on SNB+
 3b kills it completely

I'll let someone else make the decision which way to go.

Just dropping it looks reasonably safe to me. xf86-video-intel never seemed to
depend on it feature, based on the fact that when the page flip ioctl was
introduced, the event was used from the get go to complete the DRI2 swaps.
DRI2 has always (well as long as it had page flip support) throttled the
client in the GetBuffers request if there were pending swaps.

Of course I can't be 100% sure that there isn't some other piece of user
space code depending on this feature.

This series was basically just boot tested.

[PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

intel_pipe_set_base() never actually waited for any pending page flips
on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
the current front buffer. But the pending flips were actually tracked
in the BO of the previous front buffer, so the call to intel_finish_fb()
never did anything useful.

Now even the pending_flip counter is gone, so we should just
use intel_crtc_wait_for_pending_flips(), but since we're already holding
struct_mutex when we would call that function, we need another version
of it, that itself doesn't lock struct_mutex.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   51 +++++++++++++++++++++------------
 1 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1a38267..7bf4749 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2228,6 +2228,37 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
 	}
 }
 
+static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long flags;
+	bool pending;
+
+	if (atomic_read(&dev_priv->mm.wedged))
+		return false;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	pending = to_intel_crtc(crtc)->unpin_work != NULL;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	return pending;
+}
+
+static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (crtc->fb == NULL)
+		return;
+
+	wait_event(dev_priv->pending_flip_queue,
+		   !intel_crtc_has_pending_flip(crtc));
+
+	intel_finish_fb(crtc->fb);
+}
+
 static int
 intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		    struct drm_framebuffer *fb)
@@ -2261,8 +2292,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return ret;
 	}
 
-	if (crtc->fb)
-		intel_finish_fb(crtc->fb);
+	intel_crtc_wait_for_pending_flips_locked(crtc);
 
 	ret = dev_priv->display.update_plane(crtc, fb, x, y);
 	if (ret) {
@@ -2901,23 +2931,6 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
 	udelay(100);
 }
 
-static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long flags;
-	bool pending;
-
-	if (atomic_read(&dev_priv->mm.wedged))
-		return false;
-
-	spin_lock_irqsave(&dev->event_lock, flags);
-	pending = to_intel_crtc(crtc)->unpin_work != NULL;
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
-	return pending;
-}
-
 static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-- 
1.7.8.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Wake up pending flip waiters when the GPU hangs

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Anyone stuck waiting for pending flips should get woken up when the GPU
hangs.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b92e6bfb..a3c168f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1442,6 +1442,7 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
 		 */
 		for_each_ring(ring, dev_priv, i)
 			wake_up_all(&ring->irq_queue);
+		wake_up(&dev_priv->pending_flip_queue);
 	}
 
 	queue_work(dev_priv->wq, &dev_priv->error_work);
-- 
1.7.8.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3a/3] drm/i915: Avoid i915_gem_execbuffer_wait_for_flips() on SNB+

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

i915_gem_execbuffer_wait_for_flips() only works on some old hardware,
and it's not required by sane user space code. So assume that any
insane user space is limited to <= gen5, and just skip
i915_gem_execbuffer_wait_for_flips() for anything more recent.

Also eliminate the extra pending flip wait in intel_finish_fb()
which is based on the pending_flip counter. This wait doesn't
actually do anything when used on the current crtc front buffer,
so the whole thing is pointless. There's a new mechanism for
waiting for pending flips, which actually does the right thing.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++++
 drivers/gpu/drm/i915/intel_display.c       |    4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 91d43d5..14761ac 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -616,6 +616,10 @@ i915_gem_execbuffer_wait_for_flips(struct intel_ring_buffer *ring, u32 flips)
 	u32 plane, flip_mask;
 	int ret;
 
+	/* This function exists only to please legacy userland. */
+	if (INTEL_INFO(ring->dev)->gen >= 6)
+		return 0;
+
 	/* Check for any pending flips. As we only maintain a flip queue depth
 	 * of 1, we can simply insert a WAIT for the next display flip prior
 	 * to executing the batch and avoid stalling the CPU.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7bf4749..b1e8150 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2182,10 +2182,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
 	bool was_interruptible = dev_priv->mm.interruptible;
 	int ret;
 
-	wait_event(dev_priv->pending_flip_queue,
-		   atomic_read(&dev_priv->mm.wedged) ||
-		   atomic_read(&obj->pending_flip) == 0);
-
 	/* Big Hammer, we also need to ensure that any pending
 	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
 	 * current scanout is retired before unpinning the old
-- 
1.7.8.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3b/3] drm/i915: Kill i915_gem_execbuffer_wait_for_flips()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

As per Chris Wilson's suggestion make
i915_gem_execbuffer_wait_for_flips() go away.

This was used to stall the GPU ring while there are pending
page flips involving the relevant BO. Ie. while the BO is still
being scanned out by the display controller.

The recommended alternative is to use the page flip events to
wait for the page flips to fully complete before reusing the BO
of the old front buffer. Or use more buffers.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
v2: Fixed the wait_event() logic mixup of v1

 drivers/gpu/drm/i915/i915_drv.h            |    7 ----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   42 ----------------------------
 drivers/gpu/drm/i915/intel_display.c       |   13 --------
 3 files changed, 0 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2c5e89..facfd2e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1055,13 +1055,6 @@ struct drm_i915_gem_object {
 
 	/** for phy allocated objects */
 	struct drm_i915_gem_phys_object *phys_obj;
-
-	/**
-	 * Number of crtcs where this object is currently the fb, but
-	 * will be page flipped away on the next vblank.  When it
-	 * reaches 0, dev_priv->pending_flip_queue will be woken up.
-	 */
-	atomic_t pending_flip;
 };
 
 #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 91d43d5..2e527be 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -611,44 +611,11 @@ err:
 }
 
 static int
-i915_gem_execbuffer_wait_for_flips(struct intel_ring_buffer *ring, u32 flips)
-{
-	u32 plane, flip_mask;
-	int ret;
-
-	/* Check for any pending flips. As we only maintain a flip queue depth
-	 * of 1, we can simply insert a WAIT for the next display flip prior
-	 * to executing the batch and avoid stalling the CPU.
-	 */
-
-	for (plane = 0; flips >> plane; plane++) {
-		if (((flips >> plane) & 1) == 0)
-			continue;
-
-		if (plane)
-			flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
-		else
-			flip_mask = MI_WAIT_FOR_PLANE_A_FLIP;
-
-		ret = intel_ring_begin(ring, 2);
-		if (ret)
-			return ret;
-
-		intel_ring_emit(ring, MI_WAIT_FOR_EVENT | flip_mask);
-		intel_ring_emit(ring, MI_NOOP);
-		intel_ring_advance(ring);
-	}
-
-	return 0;
-}
-
-static int
 i915_gem_execbuffer_move_to_gpu(struct intel_ring_buffer *ring,
 				struct list_head *objects)
 {
 	struct drm_i915_gem_object *obj;
 	uint32_t flush_domains = 0;
-	uint32_t flips = 0;
 	int ret;
 
 	list_for_each_entry(obj, objects, exec_list) {
@@ -659,18 +626,9 @@ i915_gem_execbuffer_move_to_gpu(struct intel_ring_buffer *ring,
 		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
 			i915_gem_clflush_object(obj);
 
-		if (obj->base.pending_write_domain)
-			flips |= atomic_read(&obj->pending_flip);
-
 		flush_domains |= obj->base.write_domain;
 	}
 
-	if (flips) {
-		ret = i915_gem_execbuffer_wait_for_flips(ring, flips);
-		if (ret)
-			return ret;
-	}
-
 	if (flush_domains & I915_GEM_DOMAIN_CPU)
 		intel_gtt_chipset_flush();
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7bf4749..68bea06 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2182,10 +2182,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
 	bool was_interruptible = dev_priv->mm.interruptible;
 	int ret;
 
-	wait_event(dev_priv->pending_flip_queue,
-		   atomic_read(&dev_priv->mm.wedged) ||
-		   atomic_read(&obj->pending_flip) == 0);
-
 	/* Big Hammer, we also need to ensure that any pending
 	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
 	 * current scanout is retired before unpinning the old
@@ -6897,9 +6893,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
 
 	obj = work->old_fb_obj;
 
-	atomic_clear_mask(1 << intel_crtc->plane,
-			  &obj->pending_flip.counter);
-
 	wake_up(&dev_priv->pending_flip_queue);
 	schedule_work(&work->work);
 
@@ -7241,11 +7234,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 	work->enable_stall_check = true;
 
-	/* Block clients from rendering to the new back buffer until
-	 * the flip occurs and the object is no longer visible.
-	 */
-	atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);
-
 	ret = dev_priv->display.queue_flip(dev, crtc, fb, obj);
 	if (ret)
 		goto cleanup_pending;
@@ -7259,7 +7247,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	return 0;
 
 cleanup_pending:
-	atomic_sub(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);
 	drm_gem_object_unreference(&work->old_fb_obj->base);
 	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.7.8.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Thu,  1 Nov 2012 20:06:00 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_pipe_set_base() never actually waited for any pending page flips
> on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> the current front buffer. But the pending flips were actually tracked
> in the BO of the previous front buffer, so the call to intel_finish_fb()
> never did anything useful.
> 
> Now even the pending_flip counter is gone, so we should just
> use intel_crtc_wait_for_pending_flips(), but since we're already holding
> struct_mutex when we would call that function, we need another version
> of it, that itself doesn't lock struct_mutex.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Your earlier point was that intel_finish_fb() is being called in the wrong
place, if you fix that first you should not need the major surgery.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Wake up pending flip waiters when the GPU hangs

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]

On Thu,  1 Nov 2012 20:06:01 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Anyone stuck waiting for pending flips should get woken up when the GPU
> hangs.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3b/3] drm/i915: Kill i915_gem_execbuffer_wait_for_flips()

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On Thu,  1 Nov 2012 20:06:03 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> As per Chris Wilson's suggestion make
> i915_gem_execbuffer_wait_for_flips() go away.
> 
> This was used to stall the GPU ring while there are pending
> page flips involving the relevant BO. Ie. while the BO is still
> being scanned out by the display controller.
> 
> The recommended alternative is to use the page flip events to
> wait for the page flips to fully complete before reusing the BO
> of the old front buffer. Or use more buffers.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Needs an ack from either Jesse or Kristian.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

[Ville Syrjälä]

On Fri, Nov 02, 2012 at 01:26:56PM +0000, Chris Wilson wrote:
> On Thu,  1 Nov 2012 20:06:00 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > intel_pipe_set_base() never actually waited for any pending page flips
> > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> > the current front buffer. But the pending flips were actually tracked
> > in the BO of the previous front buffer, so the call to intel_finish_fb()
> > never did anything useful.
> > 
> > Now even the pending_flip counter is gone, so we should just
> > use intel_crtc_wait_for_pending_flips(), but since we're already holding
> > struct_mutex when we would call that function, we need another version
> > of it, that itself doesn't lock struct_mutex.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Your earlier point was that intel_finish_fb() is being called in the wrong
> place, if you fix that first you should not need the major surgery.

I don't think it's the wrong place as such. We do need it for the
panning case. The only issue with the current place is that we end up
calling it twice in the full modeset path; once in crtc_disable(),
and then later in intel_pipe_set_base().

I could move the call up from intel_pipe_set_base() to intel_crtc_set_config()
so that it only gets called for panning. This would also solve the
locking issue, but it doesn't seem as efficient as the current
sequence, because we'd end up pinning the new buffer after waiting
for page flips. With the current sequence the flip can complete in
parallel while we're doing the pin operation.

Another alternative would be to leave the call where it is in
intel_pipe_set_base(), but simply drop and reacquire struct_mutex
around the call. That would avoid the need for the _locked()
variant. That would still leave us with the double call for
full modeset, but I'm not sure that part is worth fixing. If we really
want to fix it, then we could add a paramter to intel_pipe_set_base(),
or maybe look at some other bit of state to detect the full modeset
case, and skip the call.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

On Fri, 2 Nov 2012 16:02:39 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Nov 02, 2012 at 01:26:56PM +0000, Chris Wilson wrote:
> > On Thu,  1 Nov 2012 20:06:00 +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > intel_pipe_set_base() never actually waited for any pending page flips
> > > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> > > the current front buffer. But the pending flips were actually tracked
> > > in the BO of the previous front buffer, so the call to intel_finish_fb()
> > > never did anything useful.
> > > 
> > > Now even the pending_flip counter is gone, so we should just
> > > use intel_crtc_wait_for_pending_flips(), but since we're already holding
> > > struct_mutex when we would call that function, we need another version
> > > of it, that itself doesn't lock struct_mutex.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Your earlier point was that intel_finish_fb() is being called in the wrong
> > place, if you fix that first you should not need the major surgery.
> 
> I don't think it's the wrong place as such. We do need it for the
> panning case. The only issue with the current place is that we end up
> calling it twice in the full modeset path; once in crtc_disable(),
> and then later in intel_pipe_set_base().
> 
> I could move the call up from intel_pipe_set_base() to intel_crtc_set_config()
> so that it only gets called for panning. This would also solve the
> locking issue, but it doesn't seem as efficient as the current
> sequence, because we'd end up pinning the new buffer after waiting
> for page flips. With the current sequence the flip can complete in
> parallel while we're doing the pin operation.

Oh well, I thought we could arrange the code such that we only had a
single place were we needed to wait. The simplicity of that was
appealing. In light of that, your approach looks reasonable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

On Thu,  1 Nov 2012 20:06:00 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_pipe_set_base() never actually waited for any pending page flips
> on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> the current front buffer. But the pending flips were actually tracked
> in the BO of the previous front buffer, so the call to intel_finish_fb()
> never did anything useful.
> 
> Now even the pending_flip counter is gone, so we should just
> use intel_crtc_wait_for_pending_flips(), but since we're already holding
> struct_mutex when we would call that function, we need another version
> of it, that itself doesn't lock struct_mutex.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   51 +++++++++++++++++++++------------
>  1 files changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1a38267..7bf4749 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2228,6 +2228,37 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
>  	}
>  }
>  
> +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned long flags;
> +	bool pending;
> +
> +	if (atomic_read(&dev_priv->mm.wedged))
> +		return false;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	return pending;
> +}
> +
> +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
> +{

Can we rearrange this such that the waiting logic is inside _locked()
and then intel_crtc_wiat_for_pending_flips() becomes a wrapper that
acquires the struct_mutex and then calls _locked()? Just to keep the
code simpler at the expense of the pathological case.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

[Ville Syrjälä]

On Fri, Nov 02, 2012 at 03:25:59PM +0000, Chris Wilson wrote:
> On Thu,  1 Nov 2012 20:06:00 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > intel_pipe_set_base() never actually waited for any pending page flips
> > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> > the current front buffer. But the pending flips were actually tracked
> > in the BO of the previous front buffer, so the call to intel_finish_fb()
> > never did anything useful.
> > 
> > Now even the pending_flip counter is gone, so we should just
> > use intel_crtc_wait_for_pending_flips(), but since we're already holding
> > struct_mutex when we would call that function, we need another version
> > of it, that itself doesn't lock struct_mutex.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   51 +++++++++++++++++++++------------
> >  1 files changed, 32 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1a38267..7bf4749 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2228,6 +2228,37 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
> >  	}
> >  }
> >  
> > +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	unsigned long flags;
> > +	bool pending;
> > +
> > +	if (atomic_read(&dev_priv->mm.wedged))
> > +		return false;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +	return pending;
> > +}
> > +
> > +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
> > +{
> 
> Can we rearrange this such that the waiting logic is inside _locked()
> and then intel_crtc_wiat_for_pending_flips() becomes a wrapper that
> acquires the struct_mutex and then calls _locked()? Just to keep the
> code simpler at the expense of the pathological case.

Yeah that looks doable. It does mean we'll be holding struct_mutex
around the wait_event() always. As I was already doing that for the
panning case, doing the same in the crtc_disable() case shouldn't
be any worse.

But now I started to wonder a bit about the performance implications
of keeping struct_mutex locked for that long...

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On Fri, 2 Nov 2012 19:26:37 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> But now I started to wonder a bit about the performance implications
> of keeping struct_mutex locked for that long...

Unless you are worrying about doing a modeset for every vblank, don't.
We treat modesetting as a rare, stop-the-world event for the time being,
with the emphasis upon making the darn thing reliable. Long term goal
would be to decouple the locks so that we could have indepedent users
on a multi-head device, but for now we can assume that our sole user is
so distracted by the screen going blank to not worry about his
background folding@home task stalling for ~100 ms...

On the other hand, with output slaves it is likely that a render stall
will become more noticeable even for a single user.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3b/3] drm/i915: Kill i915_gem_execbuffer_wait_for_flips()

[Jesse Barnes]

On Fri, 02 Nov 2012 13:29:20 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu,  1 Nov 2012 20:06:03 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > As per Chris Wilson's suggestion make
> > i915_gem_execbuffer_wait_for_flips() go away.
> > 
> > This was used to stall the GPU ring while there are pending
> > page flips involving the relevant BO. Ie. while the BO is still
> > being scanned out by the display controller.
> > 
> > The recommended alternative is to use the page flip events to
> > wait for the page flips to fully complete before reusing the BO
> > of the old front buffer. Or use more buffers.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Needs an ack from either Jesse or Kristian.

Yeah I think this is safe.  the kernel doesn't need to protect
userspace from doing something silly like rendering on top of the
current framebuffer.

This patch would also address Eric's complaint about the async patches
in that the last buffer will always be available for rendering.

Generally this won't be an issue anyway, since subsequent rendering
will be behind any flips that will replace the current scanout, so you
won't see drawing in progress even in the async flip case.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3b/3] drm/i915: Kill i915_gem_execbuffer_wait_for_flips()

[Kristian Høgsberg]

On Fri, Nov 2, 2012 at 9:29 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu,  1 Nov 2012 20:06:03 +0200, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> As per Chris Wilson's suggestion make
>> i915_gem_execbuffer_wait_for_flips() go away.
>>
>> This was used to stall the GPU ring while there are pending
>> page flips involving the relevant BO. Ie. while the BO is still
>> being scanned out by the display controller.
>>
>> The recommended alternative is to use the page flip events to
>> wait for the page flips to fully complete before reusing the BO
>> of the old front buffer. Or use more buffers.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Needs an ack from either Jesse or Kristian.

I wouldn't mind seeing this go; we don't rely on this behavior in
Weston, we use the events.  However, this is a user visible change in
behavior of the pageflip ioctl.  I don't remember if X needs this for
the case where it's pageflipping to fullscreen client buffers... I
think it might (or at least might have), since we did it this way so
that the client wouldn't have to wait for a protocol event before
starting the next frame.  With this the client can start rendering and
submit the first batch before it has to block.  It also minimizes the
time from pageflip done to the client can start rendering, since the
kernel now just unblocks the client directly.  Of course you can argue
that we can fix that with more buffers, and maybe it's fixed in newer
servers / ddx drivers, but it doesn't change that this is how it used
to work.  Without this, the  client doesn't know when it's new
backbuffer done scanning out.

Kristian
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3b/3] drm/i915: Kill i915_gem_execbuffer_wait_for_flips()

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

On Fri, 2 Nov 2012 14:24:12 -0400, Kristian Høgsberg <krh@bitplanet.net> wrote:
> On Fri, Nov 2, 2012 at 9:29 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu,  1 Nov 2012 20:06:03 +0200, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> As per Chris Wilson's suggestion make
> >> i915_gem_execbuffer_wait_for_flips() go away.
> >>
> >> This was used to stall the GPU ring while there are pending
> >> page flips involving the relevant BO. Ie. while the BO is still
> >> being scanned out by the display controller.
> >>
> >> The recommended alternative is to use the page flip events to
> >> wait for the page flips to fully complete before reusing the BO
> >> of the old front buffer. Or use more buffers.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > Needs an ack from either Jesse or Kristian.
> 
> I wouldn't mind seeing this go; we don't rely on this behavior in
> Weston, we use the events.  However, this is a user visible change in
> behavior of the pageflip ioctl.  I don't remember if X needs this for
> the case where it's pageflipping to fullscreen client buffers... I
> think it might (or at least might have), since we did it this way so
> that the client wouldn't have to wait for a protocol event before
> starting the next frame.  With this the client can start rendering and
> submit the first batch before it has to block.  It also minimizes the
> time from pageflip done to the client can start rendering, since the
> kernel now just unblocks the client directly.  Of course you can argue
> that we can fix that with more buffers, and maybe it's fixed in newer
> servers / ddx drivers, but it doesn't change that this is how it used
> to work.  Without this, the  client doesn't know when it's new
> backbuffer done scanning out.

We never relied on this in userspace, as you provided the flip-completed
event at the same time as you introduced pageflipping and that was used
in the original ddx to throttle the client. And from experience, I can say
that this blocking is not sufficient to prevent userspace from being
stupid...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx