Re: [Intel-gfx] [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()
On Thu, 1 Nov 2012 20:06:00 +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@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.syrj...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()
On Fri, Nov 02, 2012 at 01:26:56PM +, Chris Wilson wrote: On Thu, 1 Nov 2012 20:06:00 +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@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.syrj...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()
On Fri, 2 Nov 2012 16:02:39 +0200, Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Fri, Nov 02, 2012 at 01:26:56PM +, Chris Wilson wrote: On Thu, 1 Nov 2012 20:06:00 +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@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.syrj...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()
On Thu, 1 Nov 2012 20:06:00 +0200, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@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.syrj...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()
On Fri, Nov 02, 2012 at 03:25:59PM +, Chris Wilson wrote:
On Thu, 1 Nov 2012 20:06:00 +0200, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@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.syrj...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()
On Fri, 2 Nov 2012 19:26:37 +0200, Ville Syrjälä ville.syrj...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()
From: Ville Syrjälä ville.syrj...@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.syrj...@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
