On Sat, Mar 23, 2019 at 01:55:34AM +0200, Ville Syrjälä wrote: > On Fri, Mar 22, 2019 at 09:08:35PM +0000, Chris Wilson wrote: > > Quoting Ville Syrjala (2019-03-22 18:08:03) > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > The AGPBUSY thing doesn't work on i945gm anymore. This means > > > the gmch is incapable of waking the CPU from C3 when an interrupt > > > is generated. The interrupts just get postponed indefinitely until > > > something wakes up the CPU. This is rather annoying for vblank > > > interrupts as we are unable to maintain a steady framerate > > > unless the machine is sufficiently loaded to stay out of C3. > > > > > > To combat this let's use pm_qos to prevent C3 whenever vblank > > > interrupts are enabled. To maintain reasonable amount of powersaving > > > we will attempt to limit this to C3 only while leaving C1 and C2 > > > enabled. > > > > Interesting compromise. Frankly, I had considered pm_qos in an > > all-or-nothing approach, partly because finding the C transitions is a > > bit opaque. > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=30364 > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 8 +++ > > > drivers/gpu/drm/i915/i915_irq.c | 88 +++++++++++++++++++++++++++++++++ > > > 2 files changed, 96 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index f723b15527f8..0c736f8ca1b2 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -2042,6 +2042,14 @@ struct drm_i915_private { > > > struct i915_vma *scratch; > > > } gt; > > > > > > + /* For i945gm vblank irq vs. C3 workaround */ > > > + struct { > > > + struct work_struct work; > > > + struct pm_qos_request pm_qos; > > > + u8 c3_disable_latency; > > > > Ok, looks a bit tight, but checks out. > > > > > + u8 enabled; > > > + } i945gm_vblank; > > > + > > > /* perform PHY state sanity checks? */ > > > bool chv_phy_assert[2]; > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > index 2f788291cfe0..33386f0acab3 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -30,6 +30,7 @@ > > > > > > #include <linux/sysrq.h> > > > #include <linux/slab.h> > > > +#include <linux/cpuidle.h> > > > #include <linux/circ_buf.h> > > > #include <drm/drm_irq.h> > > > #include <drm/drm_drv.h> > > > @@ -3131,6 +3132,16 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe) > > > return 0; > > > } > > > > > > +static int i945gm_enable_vblank(struct drm_device *dev, unsigned int pipe) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > + > > > + if (dev_priv->i945gm_vblank.enabled++ == 0) > > > + schedule_work(&dev_priv->i945gm_vblank.work); > > > > I was thinking u8, isn't that a bit dangerous. But the max counter here > > should be num_pipes. Hmm, is the vblank spinlock local to a pipe? Nope, > > dev->vbl_lock. > > > > > + > > > + return i8xx_enable_vblank(dev, pipe); > > > +} > > > + > > > static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe) > > > { > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > @@ -3195,6 +3206,16 @@ static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe) > > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > > } > > > > > > +static void i945gm_disable_vblank(struct drm_device *dev, unsigned int pipe) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > + > > > + i8xx_disable_vblank(dev, pipe); > > > + > > > + if (--dev_priv->i945gm_vblank.enabled == 0) > > > + schedule_work(&dev_priv->i945gm_vblank.work); > > > +} > > > + > > > static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe) > > > { > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > @@ -3228,6 +3249,60 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe) > > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > > } > > > > > > +static void i945gm_vblank_work_func(struct work_struct *work) > > > +{ > > > + struct drm_i915_private *dev_priv = > > > + container_of(work, struct drm_i915_private, i945gm_vblank.work); > > > + > > > + /* > > > + * Vblank interrupts fail to wake up the device from C3, > > > + * hence we want to prevent C3 usage while vblank interrupts > > > + * are enabled. > > > + */ > > > + pm_qos_update_request(&dev_priv->i945gm_vblank.pm_qos, > > > + dev_priv->i945gm_vblank.enabled ? > > > + dev_priv->i945gm_vblank.c3_disable_latency : > > > + PM_QOS_DEFAULT_VALUE); > > > > Worker is required as the update may block. > > > > I'd prefer that as READ_ONCE(dev_priv->i945gm_vblank.enabled) > > Yeah, that does seem appropriate. Pushed with that before I have chance to regret the 0.7W. Thanks for the review. -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
