2014-06-03 16:32 GMT-03:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
> 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?
The problem is that we can use the get_hw_state functions not only to
check driver state a init/resume, but also to do state tracking
assertions at certain points of the code. Since most (all?) the other
HW state readout functions don't have side-effects, there's a
possibility that someone may add code to do HW state assertion at some
points, and just call these things without realizing the potential
side effects. A comment would help, but moving the assignment to
another place would also solve the problem for me. You choose.
>
>>
>>
>> > }
>> >
>> > 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
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
