On Tue, Jun 03, 2014 at 03:50:12PM -0300, Paulo Zanoni wrote:
> 2014-05-22 11:48 GMT-03:00 <ville.syrjala@xxxxxxxxxxxxxxx>:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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?
I think I had it something like that originally but changed it at some
point when I got tired of the extra characters ;) I can change it back.
>
>
> > +
> > + /*
> > + * 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
The precise part is there in the comments for each struct member.
>
>
> > + */
> > + 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.
I added such a comment for the flip counter check, but forgot to
add one here. Will add it.
>
>
> > +{
> > + 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).
Yeah the other one gets killed in one of the following patches.
>
>
> > +{
> > + 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.
Here the caller is responsible for the locking. I should sprinkle some
lockdep asserts around.
>
>
> > +}
> > +
> > +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?
In case the target vblank passed already. Though this is racy since we do
the check before drm_vblank_get() so we might still miss the interrupt
and then have to wait for the next one. I should reorder this a bit...
Although I now have a rather generic vblank notify mechanism cooking as
part of my fbc patches that doesn't suffer from this race, and my plan
is to use it also for watermarks. But back when I wrote the watermark
patches I didn't have the vblank notify thing coded up yet. Also I've
not yet tried to use it for watermarks, so it might require a bit of
further tuning before it can handle such things. This is just a FYI
really. I don't think we should stall the watermark patches behind
the vblank notify thing.
>
>
> > +
> > + 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.
This function is only called at init/resume. It populates the software
state with something that matches the current hardware state. I guess
a comment explaning the purpose of the function is the best we can do
here, or do you have a better idea?
>
>
> > }
> >
> > 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.
The crtc disable will eventually wait for the wm update, although it
would seem to be a good idea to cancel the work in case it got
scheduled twice and the first run already managed to apply all the
pending watermarks.
And in fact maybe we don't want to wait for the pending watermark update
at crtc disable. Everything is going to get turned off anyway soon, so
letting it roll with the current watermarks till the end seems perfectly
acceptable, and it might shave off one vblank wait from the modeset.
Anyways I'll see about adding some explicit cancelling somewhere.
>
> 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.
Yeah there are a few patches here that don't make a whole lot of sense
on their own.
>
>
> >
> > if (HAS_FBC(dev)) {
> > if (INTEL_INFO(dev)->gen >= 7) {
> > --
> > 1.8.5.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
