[Intel-gfx] [PATCH v2 07/16] drm/i915: Add vblank based delayed watermark update mechanism
Paulo Zanoni
przanoni at gmail.com
Tue Jun 3 20:50:12 CEST 2014
2014-05-22 11:48 GMT-03:00 <ville.syrjala at linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Add a mechanism by which you can queue up watermark update to happen
> after the vblank counter has reached a certain value. The vblank
> interrupt handler will schedule a work which will do the actual
> watermark programming in process context.
>
> v2: Rebase and s/intel_crtc/crtc/
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> drivers/gpu/drm/i915/i915_irq.c | 12 ++-
> drivers/gpu/drm/i915/intel_display.c | 1 +
> drivers/gpu/drm/i915/intel_drv.h | 27 +++++++
> drivers/gpu/drm/i915/intel_pm.c | 144 +++++++++++++++++++++++++++++++++++
> 5 files changed, 183 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a2302a7..c90d5ac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1541,6 +1541,8 @@ struct drm_i915_private {
> * state as well as the actual hardware registers
> */
> struct mutex mutex;
> +
> + struct work_struct work;
> } wm;
>
> struct i915_runtime_pm pm;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 304f86a..c680020 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2067,8 +2067,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> DRM_ERROR("Poison interrupt\n");
>
> for_each_pipe(pipe) {
> - if (de_iir & DE_PIPE_VBLANK(pipe))
> + if (de_iir & DE_PIPE_VBLANK(pipe)) {
> intel_pipe_handle_vblank(dev, pipe);
> + ilk_update_pipe_wm(dev, pipe);
> + }
>
> if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> @@ -2117,8 +2119,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> intel_opregion_asle_intr(dev);
>
> for_each_pipe(pipe) {
> - if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
> + if (de_iir & (DE_PIPE_VBLANK_IVB(pipe))) {
> intel_pipe_handle_vblank(dev, pipe);
> + ilk_update_pipe_wm(dev, pipe);
> + }
>
> /* plane/pipes map 1:1 on ilk+ */
> if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
> @@ -2260,8 +2264,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> continue;
>
> pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> - if (pipe_iir & GEN8_PIPE_VBLANK)
> + if (pipe_iir & GEN8_PIPE_VBLANK) {
> intel_pipe_handle_vblank(dev, pipe);
> + ilk_update_pipe_wm(dev, pipe);
> + }
>
> if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
> intel_prepare_page_flip(dev, pipe);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a11bd78..408b238 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10961,6 +10961,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> intel_crtc->plane = !pipe;
> }
>
> + spin_lock_init(&intel_crtc->wm.lock);
> init_waitqueue_head(&intel_crtc->vbl_wait);
>
> BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d75cc2b..72f01b1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -409,6 +409,32 @@ struct intel_crtc {
> * protected by dev_priv->wm.mutex
> */
> struct intel_pipe_wm active;
> + /*
> + * watermarks queued for next vblank
> + * protected by dev_priv->wm.mutex
> + */
> + struct intel_pipe_wm pending;
> +
> + /*
> + * the vblank count after which we can switch over to 'pending'
> + * protected by intel_crtc->wm.lock
> + */
> + u32 pending_vbl_count;
> + /*
> + * indicates that 'pending' contains changed watermarks
> + * protected by intel_crtc->wm.lock
> + */
> + bool dirty;
> + /*
> + * watermark update has a vblank reference?
> + * protected by intel_crtc->wm.lock
> + */
> + bool vblank;
I would rename this variable. Maybe has_vblank_ref?
> +
> + /*
> + * protects some intel_crtc->wm state
Please be more precise on what "some" actually means, since it's easy
to guess that a spinlock protects "some related state" :P
> + */
> + spinlock_t lock;
> } wm;
>
> wait_queue_head_t vbl_wait;
> @@ -974,6 +1000,7 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
> void ilk_wm_get_hw_state(struct drm_device *dev);
> +void ilk_update_pipe_wm(struct drm_device *dev, enum pipe pipe);
This chunk needs rebase.
>
>
> /* intel_sdvo.c */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2a63418..6fc6416 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2752,6 +2752,65 @@ static bool ilk_disable_lp_wm(struct drm_device *dev)
> return changed;
> }
>
> +static bool vbl_count_after_eq(struct drm_device *dev, u32 a, u32 b)
Since I had to stop and think for more than 10 seconds to discover if
this was checking for A >= B or A <= B or something else, I think this
function deserves a nice comment explaining what it's supposed to do.
> +{
> + u32 mask = dev->max_vblank_count;
> +
> + /* just the msb please */
> + mask &= ~(mask >> 1);
> +
> + return !((a - b) & mask);
> +}
> +
> +static bool ilk_pending_watermarks_ready(struct intel_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + u32 vbl_count;
> +
> + assert_spin_locked(&crtc->wm.lock);
> +
> + if (!crtc->wm.dirty)
> + return false;
> +
> + vbl_count = dev->driver->get_vblank_counter(dev, crtc->pipe);
> +
> + if (!vbl_count_after_eq(dev, vbl_count, crtc->wm.pending_vbl_count))
> + return false;
> +
> + if (crtc->wm.vblank) {
> + drm_vblank_put(dev, crtc->pipe);
> + crtc->wm.vblank = false;
> + }
> +
> + return true;
> +}
> +
> +static bool ilk_refresh_pending_watermarks(struct drm_device *dev)
> +{
> + struct intel_crtc *crtc;
> + bool changed = false;
> +
> + for_each_intel_crtc(dev, crtc) {
> + bool ready;
> +
> + spin_lock_irq(&crtc->wm.lock);
> +
> + ready = ilk_pending_watermarks_ready(crtc);
> + if (ready)
> + crtc->wm.dirty = false;
> +
> + spin_unlock_irq(&crtc->wm.lock);
> +
> + if (!ready)
> + continue;
> +
> + crtc->wm.active = crtc->wm.pending;
> + changed = true;
> + }
> +
> + return changed;
> +}
> +
> static void ilk_program_watermarks(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2810,6 +2869,87 @@ static void ilk_update_wm(struct drm_crtc *crtc)
> mutex_unlock(&dev_priv->wm.mutex);
> }
>
> +static void ilk_update_watermarks(struct drm_device *dev)
Now we have ilk_update_wm() and ilk_update_watermarks(). This is
confusing, since one can't really tell the difference just by reading
the name. Maybe we should rename one of them (unless a future patch
kills one of them, which I didn't check yet).
> +{
> + bool changed;
> +
> + changed = ilk_refresh_pending_watermarks(dev);
> +
> + if (changed)
> + ilk_program_watermarks(dev);
Also, don't we need to grab dev_priv->wm.mutex here? I'm asking since
the other caller of ilk_program_watermarks() has the mutex locked. It
should probably help if we had assert_mutex_is_locked() at some
places. Or maybe the callers of ilk_program_watermarks() would need
the lock.
> +}
> +
> +static void ilk_setup_pending_watermarks(struct intel_crtc *crtc,
> + const struct intel_pipe_wm *pipe_wm,
> + u32 vbl_count)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + enum pipe pipe = crtc->pipe;
> +
> + WARN(!crtc->active, "pipe %c should be enabled\n",
> + pipe_name(pipe));
> +
> + /* do the watermarks actually need changing? */
> + if (!memcmp(&crtc->wm.pending, pipe_wm, sizeof(*pipe_wm)))
> + return;
> +
> + crtc->wm.pending = *pipe_wm;
> +
> + spin_lock_irq(&crtc->wm.lock);
> + crtc->wm.pending_vbl_count = (vbl_count + 1) & dev->max_vblank_count;
> + crtc->wm.dirty = true;
> + spin_unlock_irq(&crtc->wm.lock);
> +
> + /* try to update immediately */
> + ilk_update_watermarks(dev);
I'm failing to imagine a case where this would help. Can you please tell me?
> +
> + spin_lock_irq(&crtc->wm.lock);
> +
> + /* did the immediate update succeed? */
> + if (!crtc->wm.dirty)
> + goto unlock;
> +
> + /*
> + * We might already have a pending watermark update, in
> + * which case we shouldn't grab another vblank reference.
> + */
> + if (!crtc->wm.vblank && drm_vblank_get(dev, pipe) == 0)
> + crtc->wm.vblank = true;
> +
> + WARN(!crtc->wm.vblank,
> + "unable to set up watermarks for pipe %c\n", pipe_name(pipe));
> +
> + unlock:
> + spin_unlock_irq(&crtc->wm.lock);
> +}
> +
> +static void ilk_watermark_work(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, struct drm_i915_private, wm.work);
> +
> + mutex_lock(&dev_priv->wm.mutex);
> +
> + ilk_update_watermarks(dev_priv->dev);
> +
> + mutex_unlock(&dev_priv->wm.mutex);
> +}
> +
> +/* Called from vblank irq */
> +void ilk_update_pipe_wm(struct drm_device *dev, enum pipe pipe)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_crtc *crtc =
> + to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +
> + spin_lock(&crtc->wm.lock);
> +
> + if (ilk_pending_watermarks_ready(crtc))
> + schedule_work(&dev_priv->wm.work);
> +
> + spin_unlock(&crtc->wm.lock);
> +}
> +
> static void ilk_update_sprite_wm(struct drm_plane *plane,
> struct drm_crtc *crtc,
> uint32_t sprite_width, int pixel_size,
> @@ -2872,6 +3012,9 @@ static void _ilk_pipe_wm_hw_to_sw(struct drm_crtc *crtc)
> for (level = 0; level <= max_level; level++)
> active->wm[level].enable = true;
> }
> +
> + /* no update pending */
> + intel_crtc->wm.pending = intel_crtc->wm.active;
It feels a little weird that the function that gets the HW state will
also cancel any scheduled watermark updates. Programmers from the
future are really bad, they will probably not notice this and
introduce bugs.
> }
>
> void ilk_wm_get_hw_state(struct drm_device *dev)
> @@ -6385,6 +6528,7 @@ void intel_init_pm(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> mutex_init(&dev_priv->wm.mutex);
> + INIT_WORK(&dev_priv->wm.work, ilk_watermark_work);
Don't we also need to cancel the work in the places where we disable
interrupts? Maybe it is possible to schedule a WM update, then disable
everything and runtime suspend before the scheduled work happens... Or
do something similar at S3 or driver unload.
Since there's no caller for ilk_setup_pending_watermarks() I guess
this patch will introduce a compiler warning here. I'm not sure what's
our policy for that... I guess I'd vote to merge only when the next
patches also have their RB tags.
>
> if (HAS_FBC(dev)) {
> if (INTEL_INFO(dev)->gen >= 7) {
> --
> 1.8.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list