Quoting Sagar Arun Kamble (2018-03-16 14:28:27)
>
>
> On 3/14/2018 3:07 PM, Chris Wilson wrote:
> > void intel_gt_pm_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> > {
> > struct intel_rps *rps = &dev_priv->gt_pm.rps;
> >
> > - if (pm_iir & rps->pm_events) {
> > + if (rps->active && pm_iir & rps->pm_events) {
> rps->active is updated under struct_mutex rps->lock so i think it will
> not be synchronized properly
It's an optimistic read, later on inside the worker is where we do the
check. On the enable path, it doesn't matter as we don't care about the
early interrupt, there will be more and we want to set our own
frequency; on the disable path the interrupt is serialised.
> > spin_lock(&dev_priv->irq_lock);
> > gen6_mask_pm_irq(dev_priv, pm_iir & rps->pm_events);
> > - if (rps->interrupts_enabled) {
> > - rps->pm_iir |= pm_iir & rps->pm_events;
> > - schedule_work(&rps->work);
> > - }
> > + rps->pm_iir |= pm_iir & rps->pm_events;
> > spin_unlock(&dev_priv->irq_lock);
> > +
> > + schedule_work(&rps->work);
> > }
> > }
> >
> > -void gen6_rps_busy(struct drm_i915_private *dev_priv)
> > +void intel_gt_pm_busy(struct drm_i915_private *dev_priv)
> > {
> > struct intel_rps *rps = &dev_priv->gt_pm.rps;
> > + u8 freq;
> >
> > if (!HAS_RPS(dev_priv))
> > return;
> >
> > - mutex_lock(&rps->lock);
> > - if (rps->enabled) {
> > - u8 freq;
> > + GEM_BUG_ON(rps->pm_iir);
> > + GEM_BUG_ON(rps->active);
> this BUG_ON should move under rps->lock
It's sufficiently serialised by the caller.
> >
> > - I915_WRITE(GEN6_PMINTRMSK,
> > - gen6_rps_pm_mask(dev_priv, rps->cur_freq));
> > + mutex_lock(&rps->lock);
> > + rps->active = true;
> >
> > - enable_rps_interrupts(dev_priv);
> > - memset(&rps->ei, 0, sizeof(rps->ei));
> > + /*
> > + * Use the user's desired frequency as a guide, but for better
> > + * performance, jump directly to RPe as our starting frequency.
> > + */
> > + freq = max(rps->cur_freq, rps->efficient_freq);
> > + if (intel_set_rps(dev_priv,
> > + clamp(freq,
> > + rps->min_freq_softlimit,
> > + rps->max_freq_softlimit)))
> > + DRM_DEBUG_DRIVER("Failed to set busy frequency\n");
> >
> > - /*
> > - * Use the user's desired frequency as a guide, but for better
> > - * performance, jump directly to RPe as our starting frequency.
> > - */
> > - freq = max(rps->cur_freq,
> > - rps->efficient_freq);
> > + rps->last_adj = 0;
> >
> > - if (intel_set_rps(dev_priv,
> > - clamp(freq,
> > - rps->min_freq_softlimit,
> > - rps->max_freq_softlimit)))
> > - DRM_DEBUG_DRIVER("Failed to set idle frequency\n");
> > + if (INTEL_GEN(dev_priv) >= 6) {
> > + memset(&rps->ei, 0, sizeof(rps->ei));
> > + enable_rps_interrupts(dev_priv);
> > }
> > +
> > mutex_unlock(&rps->lock);
> > }
> >
> > -void gen6_rps_idle(struct drm_i915_private *dev_priv)
> > +void intel_gt_pm_idle(struct drm_i915_private *dev_priv)
> > {
> > struct intel_rps *rps = &dev_priv->gt_pm.rps;
> >
> > - if (!HAS_RPS(dev_priv))
> > + if (!rps->active)
> this too
Again, serialised by the caller. This is important later...
> > return;
> >
> > - /*
> > - * Flush our bottom-half so that it does not race with us
> > - * setting the idle frequency and so that it is bounded by
> > - * our rpm wakeref. And then disable the interrupts to stop any
> > - * futher RPS reclocking whilst we are asleep.
> > - */
> > + mutex_lock(&rps->lock);
> > +
> > disable_rps_interrupts(dev_priv);
> >
> this is not protected by INTEL_GEN() >=6 check.
We don't guard this for it has to handle all gen.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
