Re: [Intel-gfx] [PATCH-V3 1/2] drm/i915/audio: add codec wakeup override enabled/disable callback
Hi Takashi,
Our target is to apply the patches into intel-drm-nightly tree, It will be
great if you can help merge the patches into your tree. However I get a
negative test result on BYT platform auto test, I'll confirm if it's caused by
my patches, so please do not merge now and I'll give you feedback ASAP. Thanks.
Hi Jani,
Is it OK to merge both patches onto Takashi's tree? In case you may would like
to merge the patches also, I'm not sure if there will be conflict or
something...
BR,
Han Lu
-Original Message-
From: Takashi Iwai [mailto:ti...@suse.de]
Sent: Thursday, April 30, 2015 10:21 PM
To: Lu, Han
Cc: Vetter, Daniel; Nikula, Jani; Yang, Libin; Lin, Mengdong; intel-
g...@lists.freedesktop.org
Subject: Re: [PATCH-V3 1/2] drm/i915/audio: add codec wakeup override
enabled/disable callback
At Wed, 29 Apr 2015 17:49:25 +0800,
han...@intel.com wrote:
From: Lu, Han han...@intel.com
Add support for enabling codec wakeup override signal to allow
re-enumeration of the controller on SKL after resume from low power
state.
v3 by Jani: Simplify to only support toggling the appropriate chicken bit.
Signed-off-by: Lu, Han han...@intel.com
Signed-off-by: Jani Nikula jani.nik...@intel.com
The patch series look OK to me.
But who will merge these?
I can merge these two patches to a branch so that it can be shared between
sound and i915 trees. Of course, only when I get ack about the first patch.
thanks,
Takashi
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h index 36805b6..435c372 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6881,6 +6881,9 @@ enum skl_disp_power_wells {
#define AUDIO_CP_READY(trans)((1 1) ((trans) * 4))
#define AUDIO_ELD_VALID(trans) ((1 0) ((trans) * 4))
+#define HSW_AUD_CHICKENBIT 0x65f10
+#define SKL_AUD_CODEC_WAKE_SIGNAL(1 15)
+
/* HSW Power Wells */
#define HSW_PWR_WELL_BIOS 0x45400 /* CTL1 */
#define HSW_PWR_WELL_DRIVER0x45404 /* CTL2 */
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index f72e93a..ceb2328 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -474,6 +474,32 @@ static void
i915_audio_component_put_power(struct device *dev)
intel_display_power_put(dev_to_i915(dev),
POWER_DOMAIN_AUDIO); }
+static void i915_audio_component_codec_wake_override(struct device
*dev,
+bool enable)
+{
+ struct drm_i915_private *dev_priv = dev_to_i915(dev);
+ u32 tmp;
+
+ if (!IS_SKYLAKE(dev_priv))
+ return;
+
+ /*
+* Enable/disable generating the codec wake signal, overriding the
+* internal logic to generate the codec wake to controller.
+*/
+ tmp = I915_READ(HSW_AUD_CHICKENBIT);
+ tmp = ~SKL_AUD_CODEC_WAKE_SIGNAL;
+ I915_WRITE(HSW_AUD_CHICKENBIT, tmp);
+ usleep_range(1000, 1500);
+
+ if (enable) {
+ tmp = I915_READ(HSW_AUD_CHICKENBIT);
+ tmp |= SKL_AUD_CODEC_WAKE_SIGNAL;
+ I915_WRITE(HSW_AUD_CHICKENBIT, tmp);
+ usleep_range(1000, 1500);
+ }
+}
+
/* Get CDCLK in kHz */
static int i915_audio_component_get_cdclk_freq(struct device *dev) {
@@ -495,6 +521,7 @@ static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE,
.get_power = i915_audio_component_get_power,
.put_power = i915_audio_component_put_power,
+ .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
};
diff --git a/include/drm/i915_component.h
b/include/drm/i915_component.h index 3e2f22e..c9a8b64 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -31,6 +31,7 @@ struct i915_audio_component {
struct module *owner;
void (*get_power)(struct device *);
void (*put_power)(struct device *);
+ void (*codec_wake_override)(struct device *, bool enable);
int (*get_cdclk_freq)(struct device *);
} *ops;
};
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On 04/15/2015 03:03 AM, Mario Kleiner wrote:
On 04/02/2015 01:34 PM, Chris Wilson wrote:
On vblank instant-off systems, we can get into a situation where the cost
of enabling and disabling the vblank IRQ around a drmWaitVblank query
dominates. However, we know that if the user wants the current vblank
counter, they are also very likely to immediately queue a vblank wait
and so we can keep the interrupt around and only turn it off if we have
no further vblank requests in the interrupt interval.
After vblank event delivery there is a shadow of one vblank where the
interrupt is kept alive for the user to query and queue another vblank
event. Similarly, if the user is using blocking drmWaitVblanks, the
interrupt will be disabled on the IRQ following the wait completion.
However, if the user is simply querying the current vblank counter and
timestamp, the interrupt will be disabled after every IRQ and the user
will enabled it again on the first query following the IRQ.
Testcase: igt/kms_vblank
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Daniel Vetter dan...@ffwll.ch
Cc: Michel Dänzer mic...@daenzer.net
Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
Cc: Dave Airlie airl...@redhat.com,
Cc: Mario Kleiner mario.kleiner...@gmail.com
---
drivers/gpu/drm/drm_irq.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..6f5dc18779e2 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1091,9 +1091,9 @@ void drm_vblank_put(struct drm_device *dev, int
crtc)
if (atomic_dec_and_test(vblank-refcount)) {
if (drm_vblank_offdelay == 0)
return;
-else if (dev-vblank_disable_immediate || drm_vblank_offdelay
0)
+else if (drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
-else
+else if (!dev-vblank_disable_immediate)
mod_timer(vblank-disable_timer,
jiffies + ((drm_vblank_offdelay * HZ)/1000));
}
@@ -1697,6 +1697,17 @@ bool drm_handle_vblank(struct drm_device *dev,
int crtc)
spin_lock_irqsave(dev-event_lock, irqflags);
You could move the code before the spin_lock_irqsave(dev-event_lock,
irqflags); i think it doesn't need that lock?
+if (dev-vblank_disable_immediate
!atomic_read(vblank-refcount)) {
Also check for (drm_vblank_offdelay 0) to make sure we have a way out
of instant disable here, and the same meaning of of drm_vblank_offdelay
like we have in the current implementation.
This hunk ...
+unsigned long vbl_lock_irqflags;
+
+spin_lock_irqsave(dev-vbl_lock, vbl_lock_irqflags);
+if (atomic_read(vblank-refcount) == 0 vblank-enabled) {
+DRM_DEBUG(disabling vblank on crtc %d\n, crtc);
+vblank_disable_and_save(dev, crtc);
+}
+spin_unlock_irqrestore(dev-vbl_lock, vbl_lock_irqflags);
... is the same as a call to vblank_disable_fn((unsigned long) vblank);
Maybe replace by that call?
You could also return here already, as the code below will just take a
lock, realize vblanks are now disabled and then release the locks and exit.
+}
+
/* Need timestamp lock to prevent concurrent execution with
* vblank enable/disable, as this would cause inconsistent
* or corrupted timestamps and vblank counts.
I think the logic itself is fine and at least basic testing of the patch
on a Intel HD Ironlake didn't show problems, so with the above taken
into account it would have my slightly uneasy reviewed-by.
One thing that worries me a little bit about the disable inside vblank
irq are the potential races between the disable code and the display
engine which could cause really bad off-by-one errors for clients on a
imperfect driver. These races can only happen if vblank enable or
disable happens close to or inside the vblank. This approach lets the
instant disable happen exactly inside vblank when there is the highest
chance of triggering that condition.
This doesn't seem to be a problem for intel kms, but other drivers don't
have instant disable yet, so we don't know how well we could do it
there. Additionally things like dynamic power management tend to operate
inside vblank, sometimes with funny side effects to other stuff, e.g.,
dpm on AMD, as i remember from some long debug session with Michel and
Alex last summer where dpm played a role. Therefore it seems more safe
to me to avoid actions inside vblank that could be done outside. E.g.,
instead of doing the disable inside the vblank irq one could maybe just
schedule an exact timer to do the disable a few milliseconds later in
the middle of active scanout to avoid these potential issues?
-mario
After testing this, one more thing that would make sense is to move the
disable block at the end of drm_handle_vblank() instead of at the top.
Turns out
Re: [Intel-gfx] [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
On 04/16/2015 03:03 PM, Daniel Vetter wrote:
On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
On 04/15/2015 01:31 PM, Daniel Vetter wrote:
On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
Hi Daniel,
On 04/15/2015 03:17 AM, Daniel Vetter wrote:
This was a bit too much cargo-culted, so lets make it solid:
- vblank-count doesn't need to be an atomic, writes are always done
under the protection of dev-vblank_time_lock. Switch to an unsigned
long instead and update comments. Note that atomic_read is just a
normal read of a volatile variable, so no need to audit all the
read-side access specifically.
- The barriers for the vblank counter seqlock weren't complete: The
read-side was missing the first barrier between the counter read and
the timestamp read, it only had a barrier between the ts and the
counter read. We need both.
- Barriers weren't properly documented. Since barriers only work if
you have them on boths sides of the transaction it's prudent to
reference where the other side is. To avoid duplicating the
write-side comment 3 times extract a little store_vblank() helper.
In that helper also assert that we do indeed hold
dev-vblank_time_lock, since in some cases the lock is acquired a
few functions up in the callchain.
Spotted while reviewing a patch from Chris Wilson to add a fastpath to
the vblank_wait ioctl.
Cc: Chris Wilson ch...@chris-wilson.co.uk
Cc: Mario Kleiner mario.kleiner...@gmail.com
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Michel Dänzer mic...@daenzer.net
Signed-off-by: Daniel Vetter daniel.vet...@intel.com
---
drivers/gpu/drm/drm_irq.c | 92 ---
include/drm/drmP.h| 8 +++--
2 files changed, 54 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..23bfbc61a494 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -74,6 +74,33 @@ module_param_named(vblankoffdelay, drm_vblank_offdelay, int,
0600);
module_param_named(timestamp_precision_usec, drm_timestamp_precision, int,
0600);
module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
+static void store_vblank(struct drm_device *dev, int crtc,
+unsigned vblank_count_inc,
+struct timeval *t_vblank)
+{
+ struct drm_vblank_crtc *vblank = dev-vblank[crtc];
+ u32 tslot;
+
+ assert_spin_locked(dev-vblank_time_lock);
+
+ if (t_vblank) {
+ tslot = vblank-count + vblank_count_inc;
+ vblanktimestamp(dev, crtc, tslot) = *t_vblank;
+ }
+
+ /*
+* vblank timestamp updates are protected on the write side with
+* vblank_time_lock, but on the read side done locklessly using a
+* sequence-lock on the vblank counter. Ensure correct ordering using
+* memory barrriers. We need the barrier both before and also after the
+* counter update to synchronize with the next timestamp write.
+* The read-side barriers for this are in drm_vblank_count_and_time.
+*/
+ smp_wmb();
+ vblank-count += vblank_count_inc;
+ smp_wmb();
The comment and the code are each self-contradictory.
If vblank-count writes are always protected by vblank_time_lock (something I
did not verify but that the comment above asserts), then the trailing write
barrier is not required (and the assertion that it is in the comment is
incorrect).
A spin unlock operation is always a write barrier.
Hm yeah. Otoh to me that's bordering on code too clever for my own good.
That the spinlock is held I can assure. That no one goes around and does
multiple vblank updates (because somehow that code raced with the hw
itself) I can't easily assure with a simple assert or something similar.
It's not the case right now, but that can changes.
The algorithm would be broken if multiple updates for the same vblank
count were allowed; that's why it checks to see if the vblank count has
not advanced before storing a new timestamp.
Otherwise, the read side would not be able to determine that the
timestamp is valid by double-checking that the vblank count has not
changed.
And besides, even if the code looped without dropping the spinlock,
the correct write order would still be observed because it would still
be executing on the same cpu.
My objection to the write memory barrier is not about optimization;
it's about correct code.
Well diff=0 is not allowed, I guess I could enforce this with some
WARN_ON. And I still think my point of non-local correctness is solid.
With the smp_wmb() removed the following still works correctly:
spin_lock(vblank_time_lock);
store_vblank(dev, crtc, 1, ts1);
spin_unlock(vblank_time_lock);
spin_lock(vblank_time_lock);
store_vblank(dev, crtc, 1, ts2);
spin_unlock(vblank_time_lock);
But with the smp_wmb(); removed the following would be broken:
Re: [Intel-gfx] [PATCH v3] drm/i915: Setup static bias for GPU
On Wednesday 29 April 2015 02:59 PM, Ville Syrjälä wrote:
On Wed, Apr 29, 2015 at 08:36:24AM +0530, deepa...@linux.intel.com wrote:
From: Deepak S deepa...@linux.intel.com
Based on the spec, Setting up static BIAS for GPU to improve the
rps performace.
v2: rename reg defn to match spec. (Ville)
v3: Updated bias setting for chv (Deepak)
Signed-off-by: Deepak S deepa...@linux.intel.com
Matches the spec. Whether the chosen bias is really the best, I can't
really say. But favoring the GPU does seem like a sensible idea if we
want to keep the UI stuff fluid enough while there's some CPU heavy
tasks running at the same time.
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
Thanks Ville for reviewing,
Yes our aim is to keep user experience smooth.
---
drivers/gpu/drm/i915/i915_reg.h | 6 ++
drivers/gpu/drm/i915/intel_pm.c | 12
2 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 36805b6..048987e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -670,6 +670,12 @@ enum skl_disp_power_wells {
#define FB_FMAX_VMIN_FREQ_LO_SHIFT 27
#define FB_FMAX_VMIN_FREQ_LO_MASK 0xf800
+#define VLV_TURBO_SOC_OVERRIDE 0x04
+#defineVLV_OVERRIDE_EN 1
+#defineVLV_SOC_TDP_EN (1 1)
+#defineVLV_BIAS_CPU_125_SOC_875 (6 2)
+#defineCHV_BIAS_CPU_50_SOC_50 (3 2)
+
#define VLV_CZ_CLOCK_TO_MILLI_SEC 10
/* vlv2 north clock has */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 78c89ff..3689d0e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5065,6 +5065,12 @@ static void cherryview_enable_rps(struct drm_device *dev)
GEN6_RP_UP_BUSY_AVG |
GEN6_RP_DOWN_IDLE_AVG);
+ /* Setting Fixed Bias */
+ val = VLV_OVERRIDE_EN |
+ VLV_SOC_TDP_EN |
+ CHV_BIAS_CPU_50_SOC_50;
+ vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
/* RPS code assumes GPLL is used */
@@ -5149,6 +5155,12 @@ static void valleyview_enable_rps(struct drm_device *dev)
I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
+ /* Setting Fixed Bias */
+ val = VLV_OVERRIDE_EN |
+ VLV_SOC_TDP_EN |
+ VLV_BIAS_CPU_125_SOC_875;
+ vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
/* RPS code assumes GPLL is used */
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
