| Message ID | 1390838706-10934-2-git-send-email-deepak.s@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ville, Can you please review this patch. If Ok, Can we push this to nightly? Thanks Deepak On 1/29/2014 9:13 PM, S, Deepak wrote: > > > -----Original Message----- > From: S, Deepak > Sent: Monday, January 27, 2014 9:35 PM > To: intel-gfx@lists.freedesktop.org > Cc: S, Deepak > Subject: [PATCH v5 1/2] drm/i915: Disable/Enable PM Intrrupts based on the current freq. > > From: Deepak S <deepak.s@intel.com> > > When current delay is already at max delay, Let's disable the PM UP THRESHOLD INTRRUPTS, so that we will not get further interrupts until current delay is less than max delay, Also request for the PM DOWN THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and viceversa for PM DOWN THRESHOLD INTRRUPTS. > > v2: Use bool variables (Daniel) > > v3: Fix Interrupt masking bit (Deepak) > > v4: Use existing symbolic constants in i915_reg.h (Daniel) > > v5: Add pm interrupt mask after new_delay calculation (Ville) > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_pm.c | 3 +++ > 3 files changed, 45 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 56c720b..f19de66 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt { > u8 rp0_delay; > u8 hw_max; > > + bool rp_up_masked; > + bool rp_down_masked; > + > int last_adj; > enum { LOW_POWER, BETWEEN, HIGH_POWER } power; > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 01a8686..69a5214 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev, > i915_queue_hangcheck(dev); > } > > +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv, > + u32 pm_iir, int *new_delay) > +{ > + if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) { > + if (*new_delay >= dev_priv->rps.max_delay) { > + /* Mask UP THRESHOLD Interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, > + I915_READ(GEN6_PMINTRMSK) | > + GEN6_PM_RP_UP_THRESHOLD); > + dev_priv->rps.rp_up_masked = true; > + } > + if (dev_priv->rps.rp_down_masked) { > + /* UnMask DOWN THRESHOLD Interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, > + I915_READ(GEN6_PMINTRMSK) & > + ~GEN6_PM_RP_DOWN_THRESHOLD); > + dev_priv->rps.rp_down_masked = false; > + } > + } else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) { > + if (*new_delay <= dev_priv->rps.min_delay) { > + /* Mask DOWN THRESHOLD Interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, > + I915_READ(GEN6_PMINTRMSK) | > + GEN6_PM_RP_DOWN_THRESHOLD); > + dev_priv->rps.rp_down_masked = true; > + } > + > + if (dev_priv->rps.rp_up_masked) { > + /* UnMask UP THRESHOLD Interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, > + I915_READ(GEN6_PMINTRMSK) & > + ~GEN6_PM_RP_UP_THRESHOLD); > + dev_priv->rps.rp_up_masked = false; > + } > + } > +} > + > static void gen6_pm_rps_work(struct work_struct *work) { > drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, @@ -1029,6 +1066,8 @@ static void gen6_pm_rps_work(struct work_struct *work) > */ > new_delay = clamp_t(int, new_delay, > dev_priv->rps.min_delay, dev_priv->rps.max_delay); > + > + gen6_set_pm_mask(dev_priv, pm_iir, &new_delay); > dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_delay; > > if (IS_VALLEYVIEW(dev_priv->dev)) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index b9b4fe4..c6a07c9 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3615,6 +3615,9 @@ static void valleyview_enable_rps(struct drm_device *dev) > > valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > + dev_priv->rps.rp_up_masked = false; > + dev_priv->rps.rp_down_masked = false; > + > gen6_enable_rps_interrupts(dev); > > gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > -- > 1.8.5.2 >
On Mon, Jan 27, 2014 at 09:35:05PM +0530, deepak.s@intel.com wrote: > From: Deepak S <deepak.s@intel.com> > > When current delay is already at max delay, Let's disable the PM UP > THRESHOLD INTRRUPTS, so that we will not get further interrupts until > current delay is less than max delay, Also request for the PM DOWN > THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and > viceversa for PM DOWN THRESHOLD INTRRUPTS. > > v2: Use bool variables (Daniel) > > v3: Fix Interrupt masking bit (Deepak) > > v4: Use existing symbolic constants in i915_reg.h (Daniel) > > v5: Add pm interrupt mask after new_delay calculation (Ville) > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 3 +++ > drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_pm.c | 3 +++ > 3 files changed, 45 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 56c720b..f19de66 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt { > u8 rp0_delay; > u8 hw_max; > > + bool rp_up_masked; > + bool rp_down_masked; > + > int last_adj; > enum { LOW_POWER, BETWEEN, HIGH_POWER } power; > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 01a8686..69a5214 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev, > i915_queue_hangcheck(dev); > } > > +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv, > + u32 pm_iir, int *new_delay) Just a minor nit here. We don't modify new_delay in this function, so passing by value would be better. Otherwise the patch looks good to me. So if you change that, you can add: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > +{ > + if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) { > + if (*new_delay >= dev_priv->rps.max_delay) { > + /* Mask UP THRESHOLD Interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, > + I915_READ(GEN6_PMINTRMSK) | > + GEN6_PM_RP_UP_THRESHOLD); > + dev_priv->rps.rp_up_masked = true; > + } > + if (dev_priv->rps.rp_down_masked) { > + /* UnMask DOWN THRESHOLD Interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, > + I915_READ(GEN6_PMINTRMSK) & > + ~GEN6_PM_RP_DOWN_THRESHOLD); > + dev_priv->rps.rp_down_masked = false; > + } > + } else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) { > + if (*new_delay <= dev_priv->rps.min_delay) { > + /* Mask DOWN THRESHOLD Interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, > + I915_READ(GEN6_PMINTRMSK) | > + GEN6_PM_RP_DOWN_THRESHOLD); > + dev_priv->rps.rp_down_masked = true; > + } > + > + if (dev_priv->rps.rp_up_masked) { > + /* UnMask UP THRESHOLD Interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, > + I915_READ(GEN6_PMINTRMSK) & > + ~GEN6_PM_RP_UP_THRESHOLD); > + dev_priv->rps.rp_up_masked = false; > + } > + } > +} > + > static void gen6_pm_rps_work(struct work_struct *work) > { > drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > @@ -1029,6 +1066,8 @@ static void gen6_pm_rps_work(struct work_struct *work) > */ > new_delay = clamp_t(int, new_delay, > dev_priv->rps.min_delay, dev_priv->rps.max_delay); > + > + gen6_set_pm_mask(dev_priv, pm_iir, &new_delay); > dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_delay; > > if (IS_VALLEYVIEW(dev_priv->dev)) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index b9b4fe4..c6a07c9 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3615,6 +3615,9 @@ static void valleyview_enable_rps(struct drm_device *dev) > > valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > + dev_priv->rps.rp_up_masked = false; > + dev_priv->rps.rp_down_masked = false; > + > gen6_enable_rps_interrupts(dev); > > gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > -- > 1.8.5.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jan 29, 2014 at 05:59:23PM +0200, Ville Syrjälä wrote: > On Mon, Jan 27, 2014 at 09:35:05PM +0530, deepak.s@intel.com wrote: > > From: Deepak S <deepak.s@intel.com> > > > > When current delay is already at max delay, Let's disable the PM UP > > THRESHOLD INTRRUPTS, so that we will not get further interrupts until > > current delay is less than max delay, Also request for the PM DOWN > > THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and > > viceversa for PM DOWN THRESHOLD INTRRUPTS. > > > > v2: Use bool variables (Daniel) > > > > v3: Fix Interrupt masking bit (Deepak) > > > > v4: Use existing symbolic constants in i915_reg.h (Daniel) > > > > v5: Add pm interrupt mask after new_delay calculation (Ville) > > > > Signed-off-by: Deepak S <deepak.s@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 3 +++ > > drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_pm.c | 3 +++ > > 3 files changed, 45 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 56c720b..f19de66 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt { > > u8 rp0_delay; > > u8 hw_max; > > > > + bool rp_up_masked; > > + bool rp_down_masked; > > + > > int last_adj; > > enum { LOW_POWER, BETWEEN, HIGH_POWER } power; > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 01a8686..69a5214 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev, > > i915_queue_hangcheck(dev); > > } > > > > +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv, > > + u32 pm_iir, int *new_delay) > > Just a minor nit here. We don't modify new_delay in this function, so > passing by value would be better. I've fixed this up and merged the patch. I've also polished the whitespace a bit, please run patches through scripts/checkpatch.pl before submitting. I usually don't all go whitespace-nazi about this, but generally the suggestions result in more uniform and hence readable sources. > Otherwise the patch looks good to me. So if you change that, you can > add: > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks, Daniel
On 1/30/2014 1:00 AM, Daniel Vetter wrote: > On Wed, Jan 29, 2014 at 05:59:23PM +0200, Ville Syrjälä wrote: >> On Mon, Jan 27, 2014 at 09:35:05PM +0530, deepak.s@intel.com wrote: >>> From: Deepak S <deepak.s@intel.com> >>> >>> When current delay is already at max delay, Let's disable the PM UP >>> THRESHOLD INTRRUPTS, so that we will not get further interrupts until >>> current delay is less than max delay, Also request for the PM DOWN >>> THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and >>> viceversa for PM DOWN THRESHOLD INTRRUPTS. >>> >>> v2: Use bool variables (Daniel) >>> >>> v3: Fix Interrupt masking bit (Deepak) >>> >>> v4: Use existing symbolic constants in i915_reg.h (Daniel) >>> >>> v5: Add pm interrupt mask after new_delay calculation (Ville) >>> >>> Signed-off-by: Deepak S <deepak.s@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 3 +++ >>> drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> drivers/gpu/drm/i915/intel_pm.c | 3 +++ >>> 3 files changed, 45 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>> index 56c720b..f19de66 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt { >>> u8 rp0_delay; >>> u8 hw_max; >>> >>> + bool rp_up_masked; >>> + bool rp_down_masked; >>> + >>> int last_adj; >>> enum { LOW_POWER, BETWEEN, HIGH_POWER } power; >>> >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >>> index 01a8686..69a5214 100644 >>> --- a/drivers/gpu/drm/i915/i915_irq.c >>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>> @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev, >>> i915_queue_hangcheck(dev); >>> } >>> >>> +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv, >>> + u32 pm_iir, int *new_delay) >> >> Just a minor nit here. We don't modify new_delay in this function, so >> passing by value would be better. > > I've fixed this up and merged the patch. I've also polished the whitespace > a bit, please run patches through scripts/checkpatch.pl before submitting. > I usually don't all go whitespace-nazi about this, but generally the > suggestions result in more uniform and hence readable sources. > >> Otherwise the patch looks good to me. So if you change that, you can >> add: >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Thanks, Daniel Thanks Daniel.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 56c720b..f19de66 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt { u8 rp0_delay; u8 hw_max; + bool rp_up_masked; + bool rp_down_masked; + int last_adj; enum { LOW_POWER, BETWEEN, HIGH_POWER } power; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 01a8686..69a5214 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev, i915_queue_hangcheck(dev); } +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv, + u32 pm_iir, int *new_delay) +{ + if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) { + if (*new_delay >= dev_priv->rps.max_delay) { + /* Mask UP THRESHOLD Interrupts */ + I915_WRITE(GEN6_PMINTRMSK, + I915_READ(GEN6_PMINTRMSK) | + GEN6_PM_RP_UP_THRESHOLD); + dev_priv->rps.rp_up_masked = true; + } + if (dev_priv->rps.rp_down_masked) { + /* UnMask DOWN THRESHOLD Interrupts */ + I915_WRITE(GEN6_PMINTRMSK, + I915_READ(GEN6_PMINTRMSK) & + ~GEN6_PM_RP_DOWN_THRESHOLD); + dev_priv->rps.rp_down_masked = false; + } + } else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) { + if (*new_delay <= dev_priv->rps.min_delay) { + /* Mask DOWN THRESHOLD Interrupts */ + I915_WRITE(GEN6_PMINTRMSK, + I915_READ(GEN6_PMINTRMSK) | + GEN6_PM_RP_DOWN_THRESHOLD); + dev_priv->rps.rp_down_masked = true; + } + + if (dev_priv->rps.rp_up_masked) { + /* UnMask UP THRESHOLD Interrupts */ + I915_WRITE(GEN6_PMINTRMSK, + I915_READ(GEN6_PMINTRMSK) & + ~GEN6_PM_RP_UP_THRESHOLD); + dev_priv->rps.rp_up_masked = false; + } + } +} + static void gen6_pm_rps_work(struct work_struct *work) { drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, @@ -1029,6 +1066,8 @@ static void gen6_pm_rps_work(struct work_struct *work) */ new_delay = clamp_t(int, new_delay, dev_priv->rps.min_delay, dev_priv->rps.max_delay); + + gen6_set_pm_mask(dev_priv, pm_iir, &new_delay); dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_delay; if (IS_VALLEYVIEW(dev_priv->dev)) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index b9b4fe4..c6a07c9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3615,6 +3615,9 @@ static void valleyview_enable_rps(struct drm_device *dev) valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); + dev_priv->rps.rp_up_masked = false; + dev_priv->rps.rp_down_masked = false; + gen6_enable_rps_interrupts(dev); gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);