* [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI
@ 2013-07-26 16:17 Chris Wilson
2013-07-26 16:46 ` Daniel Vetter
2013-07-26 18:57 ` Chris Wilson
0 siblings, 2 replies; 8+ messages in thread
From: Chris Wilson @ 2013-07-26 16:17 UTC (permalink / raw)
To: intel-gfx
Otherwise we get flooded by the kernel warning us that we are doing
long sequences of IO without serialisation. For example,
WARNING: CPU: 0 PID: 11136 at drivers/gpu/drm/i915/intel_sideband.c:40 vlv_sideband_rw+0x48/0x1ef()
Modules linked in:
CPU: 0 PID: 11136 Comm: kworker/u2:0 Tainted: G W 3.11.0-rc2+ #4
Call Trace:
[<c2028564>] ? warn_slowpath_common+0x63/0x78
[<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
[<c20285dd>] ? warn_slowpath_null+0xf/0x13
[<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
[<c227b060>] ? vlv_dpio_write+0x1c/0x21
[<c2262b3b>] ? intel_dp_set_signal_levels+0x24a/0x385
[<c2264909>] ? intel_dp_complete_link_train+0x25/0x1d1
[<c2264c55>] ? intel_dp_check_link_status+0xf7/0x106
[<c2238ced>] ? i915_hotplug_work_func+0x17b/0x221
[<c203a204>] ? process_one_work+0x12e/0x210
[<c203a5e4>] ? worker_thread+0x116/0x1ad
[<c203a4ce>] ? rescuer_thread+0x1cb/0x1cb
[<c203d8f5>] ? kthread+0x67/0x6c
[<c2457ebb>] ? ret_from_kernel_thread+0x1b/0x30
[<c203d88e>] ? init_completion+0x18/0x18
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_dp.c | 6 ++++++
drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
2 files changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4a7ba5e..2a455b8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1719,6 +1719,7 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
int pipe = intel_crtc->pipe;
u32 val;
+ mutex_lock(&dev_priv->dpio_lock);
val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
val = 0;
if (pipe)
@@ -1732,6 +1733,7 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
0x00760018);
vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
0x00400888);
+ mutex_unlock(&dev_priv->dpio_lock);
}
}
@@ -1746,6 +1748,7 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
return;
/* Program Tx lane resets to default */
+ mutex_lock(&dev_priv->dpio_lock);
vlv_dpio_write(dev_priv, DPIO_PCS_TX(port),
DPIO_PCS_TX_LANE2_RESET |
DPIO_PCS_TX_LANE1_RESET);
@@ -1759,6 +1762,7 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
vlv_dpio_write(dev_priv, DPIO_PCS_STAGGER1(port), 0x00750f00);
vlv_dpio_write(dev_priv, DPIO_TX_CTL(port), 0x00001500);
vlv_dpio_write(dev_priv, DPIO_TX_LANE(port), 0x40400000);
+ mutex_unlock(&dev_priv->dpio_lock);
}
/*
@@ -1970,6 +1974,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
return 0;
}
+ mutex_lock(&dev_priv->dpio_lock);
vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port), 0x00000000);
vlv_dpio_write(dev_priv, DPIO_TX_SWING_CTL4(port), demph_reg_value);
vlv_dpio_write(dev_priv, DPIO_TX_SWING_CTL2(port),
@@ -1978,6 +1983,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
vlv_dpio_write(dev_priv, DPIO_PCS_STAGGER0(port), 0x00030000);
vlv_dpio_write(dev_priv, DPIO_PCS_CTL_OVER1(port), preemph_reg_value);
vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port), 0x80000000);
+ mutex_unlock(&dev_priv->dpio_lock);
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 044d11d..5fa3035 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1033,6 +1033,7 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
return;
/* Enable clock channels for this port */
+ mutex_lock(&dev_priv->dpio_lock);
val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
val = 0;
if (pipe)
@@ -1063,6 +1064,7 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
0x00760018);
vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
0x00400888);
+ mutex_unlock(&dev_priv->dpio_lock);
}
static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
@@ -1076,6 +1078,7 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
return;
/* Program Tx lane resets to default */
+ mutex_lock(&dev_priv->dpio_lock);
vlv_dpio_write(dev_priv, DPIO_PCS_TX(port),
DPIO_PCS_TX_LANE2_RESET |
DPIO_PCS_TX_LANE1_RESET);
@@ -1094,6 +1097,7 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
0x00002000);
vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port),
DPIO_TX_OCALINIT_EN);
+ mutex_lock(&dev_priv->dpio_lock);
}
static void intel_hdmi_post_disable(struct intel_encoder *encoder)
--
1.8.3.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI
2013-07-26 16:17 [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI Chris Wilson
@ 2013-07-26 16:46 ` Daniel Vetter
2013-07-26 17:11 ` Chris Wilson
2013-07-26 18:57 ` Chris Wilson
1 sibling, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2013-07-26 16:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Fri, Jul 26, 2013 at 6:17 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Otherwise we get flooded by the kernel warning us that we are doing
> long sequences of IO without serialisation. For example,
>
> WARNING: CPU: 0 PID: 11136 at drivers/gpu/drm/i915/intel_sideband.c:40 vlv_sideband_rw+0x48/0x1ef()
> Modules linked in:
> CPU: 0 PID: 11136 Comm: kworker/u2:0 Tainted: G W 3.11.0-rc2+ #4
> Call Trace:
> [<c2028564>] ? warn_slowpath_common+0x63/0x78
> [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> [<c20285dd>] ? warn_slowpath_null+0xf/0x13
> [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> [<c227b060>] ? vlv_dpio_write+0x1c/0x21
> [<c2262b3b>] ? intel_dp_set_signal_levels+0x24a/0x385
> [<c2264909>] ? intel_dp_complete_link_train+0x25/0x1d1
> [<c2264c55>] ? intel_dp_check_link_status+0xf7/0x106
> [<c2238ced>] ? i915_hotplug_work_func+0x17b/0x221
> [<c203a204>] ? process_one_work+0x12e/0x210
> [<c203a5e4>] ? worker_thread+0x116/0x1ad
> [<c203a4ce>] ? rescuer_thread+0x1cb/0x1cb
> [<c203d8f5>] ? kthread+0x67/0x6c
> [<c2457ebb>] ? ret_from_kernel_thread+0x1b/0x30
> [<c203d88e>] ? init_completion+0x18/0x18
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
The problem here is that Jesse was lazy and grabs the lock in
vlv_crtc_enable, so I think your code here will deadlock. But I agree
that grabbing the lock where we actually frob the sideband is what we
want ...
-Daniel
> ---
> drivers/gpu/drm/i915/intel_dp.c | 6 ++++++
> drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4a7ba5e..2a455b8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1719,6 +1719,7 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
> int pipe = intel_crtc->pipe;
> u32 val;
>
> + mutex_lock(&dev_priv->dpio_lock);
> val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
> val = 0;
> if (pipe)
> @@ -1732,6 +1733,7 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
> 0x00760018);
> vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
> 0x00400888);
> + mutex_unlock(&dev_priv->dpio_lock);
> }
> }
>
> @@ -1746,6 +1748,7 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
> return;
>
> /* Program Tx lane resets to default */
> + mutex_lock(&dev_priv->dpio_lock);
> vlv_dpio_write(dev_priv, DPIO_PCS_TX(port),
> DPIO_PCS_TX_LANE2_RESET |
> DPIO_PCS_TX_LANE1_RESET);
> @@ -1759,6 +1762,7 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
> vlv_dpio_write(dev_priv, DPIO_PCS_STAGGER1(port), 0x00750f00);
> vlv_dpio_write(dev_priv, DPIO_TX_CTL(port), 0x00001500);
> vlv_dpio_write(dev_priv, DPIO_TX_LANE(port), 0x40400000);
> + mutex_unlock(&dev_priv->dpio_lock);
> }
>
> /*
> @@ -1970,6 +1974,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
> return 0;
> }
>
> + mutex_lock(&dev_priv->dpio_lock);
> vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port), 0x00000000);
> vlv_dpio_write(dev_priv, DPIO_TX_SWING_CTL4(port), demph_reg_value);
> vlv_dpio_write(dev_priv, DPIO_TX_SWING_CTL2(port),
> @@ -1978,6 +1983,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
> vlv_dpio_write(dev_priv, DPIO_PCS_STAGGER0(port), 0x00030000);
> vlv_dpio_write(dev_priv, DPIO_PCS_CTL_OVER1(port), preemph_reg_value);
> vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port), 0x80000000);
> + mutex_unlock(&dev_priv->dpio_lock);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 044d11d..5fa3035 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1033,6 +1033,7 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
> return;
>
> /* Enable clock channels for this port */
> + mutex_lock(&dev_priv->dpio_lock);
> val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
> val = 0;
> if (pipe)
> @@ -1063,6 +1064,7 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
> 0x00760018);
> vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
> 0x00400888);
> + mutex_unlock(&dev_priv->dpio_lock);
> }
>
> static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> @@ -1076,6 +1078,7 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> return;
>
> /* Program Tx lane resets to default */
> + mutex_lock(&dev_priv->dpio_lock);
> vlv_dpio_write(dev_priv, DPIO_PCS_TX(port),
> DPIO_PCS_TX_LANE2_RESET |
> DPIO_PCS_TX_LANE1_RESET);
> @@ -1094,6 +1097,7 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> 0x00002000);
> vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port),
> DPIO_TX_OCALINIT_EN);
> + mutex_lock(&dev_priv->dpio_lock);
> }
>
> static void intel_hdmi_post_disable(struct intel_encoder *encoder)
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI
2013-07-26 16:46 ` Daniel Vetter
@ 2013-07-26 17:11 ` Chris Wilson
2013-07-26 17:43 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2013-07-26 17:11 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Fri, Jul 26, 2013 at 06:46:43PM +0200, Daniel Vetter wrote:
> On Fri, Jul 26, 2013 at 6:17 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Otherwise we get flooded by the kernel warning us that we are doing
> > long sequences of IO without serialisation. For example,
> >
> > WARNING: CPU: 0 PID: 11136 at drivers/gpu/drm/i915/intel_sideband.c:40 vlv_sideband_rw+0x48/0x1ef()
> > Modules linked in:
> > CPU: 0 PID: 11136 Comm: kworker/u2:0 Tainted: G W 3.11.0-rc2+ #4
> > Call Trace:
> > [<c2028564>] ? warn_slowpath_common+0x63/0x78
> > [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> > [<c20285dd>] ? warn_slowpath_null+0xf/0x13
> > [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> > [<c227b060>] ? vlv_dpio_write+0x1c/0x21
> > [<c2262b3b>] ? intel_dp_set_signal_levels+0x24a/0x385
> > [<c2264909>] ? intel_dp_complete_link_train+0x25/0x1d1
> > [<c2264c55>] ? intel_dp_check_link_status+0xf7/0x106
> > [<c2238ced>] ? i915_hotplug_work_func+0x17b/0x221
> > [<c203a204>] ? process_one_work+0x12e/0x210
> > [<c203a5e4>] ? worker_thread+0x116/0x1ad
> > [<c203a4ce>] ? rescuer_thread+0x1cb/0x1cb
> > [<c203d8f5>] ? kthread+0x67/0x6c
> > [<c2457ebb>] ? ret_from_kernel_thread+0x1b/0x30
> > [<c203d88e>] ? init_completion+0x18/0x18
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> The problem here is that Jesse was lazy and grabs the lock in
> vlv_crtc_enable, so I think your code here will deadlock. But I agree
> that grabbing the lock where we actually frob the sideband is what we
> want ...
Looks like vlv_crtc_enable() is the only place that takes the lock around
callbacks, so I propose dropping the locking in that function (and
scoping it around the sideband sequences as above).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI
2013-07-26 17:11 ` Chris Wilson
@ 2013-07-26 17:43 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2013-07-26 17:43 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, intel-gfx
On Fri, Jul 26, 2013 at 7:11 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> The problem here is that Jesse was lazy and grabs the lock in
>> vlv_crtc_enable, so I think your code here will deadlock. But I agree
>> that grabbing the lock where we actually frob the sideband is what we
>> want ...
>
> Looks like vlv_crtc_enable() is the only place that takes the lock around
> callbacks, so I propose dropping the locking in that function (and
> scoping it around the sideband sequences as above).
Yeah, I think moving the locking into callbacks/lower-level functions
is the right approach.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI
2013-07-26 16:17 [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI Chris Wilson
2013-07-26 16:46 ` Daniel Vetter
@ 2013-07-26 18:57 ` Chris Wilson
2013-07-29 13:20 ` Jani Nikula
1 sibling, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2013-07-26 18:57 UTC (permalink / raw)
To: intel-gfx
Otherwise we get flooded by the kernel warning us that we are doing
long sequences of IO without serialisation. For example,
WARNING: CPU: 0 PID: 11136 at drivers/gpu/drm/i915/intel_sideband.c:40 vlv_sideband_rw+0x48/0x1ef()
Modules linked in:
CPU: 0 PID: 11136 Comm: kworker/u2:0 Tainted: G W 3.11.0-rc2+ #4
Call Trace:
[<c2028564>] ? warn_slowpath_common+0x63/0x78
[<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
[<c20285dd>] ? warn_slowpath_null+0xf/0x13
[<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
[<c227b060>] ? vlv_dpio_write+0x1c/0x21
[<c2262b3b>] ? intel_dp_set_signal_levels+0x24a/0x385
[<c2264909>] ? intel_dp_complete_link_train+0x25/0x1d1
[<c2264c55>] ? intel_dp_check_link_status+0xf7/0x106
[<c2238ced>] ? i915_hotplug_work_func+0x17b/0x221
[<c203a204>] ? process_one_work+0x12e/0x210
[<c203a5e4>] ? worker_thread+0x116/0x1ad
[<c203a4ce>] ? rescuer_thread+0x1cb/0x1cb
[<c203d8f5>] ? kthread+0x67/0x6c
[<c2457ebb>] ? ret_from_kernel_thread+0x1b/0x30
[<c203d88e>] ? init_completion+0x18/0x18
v2: Retire the locking in vlv_crtc_enable() and do it close to the meat.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 4 ----
drivers/gpu/drm/i915/intel_dp.c | 6 ++++++
drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b3389d7..6bb9017 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3653,8 +3653,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
intel_crtc->active = true;
intel_update_watermarks(dev);
- mutex_lock(&dev_priv->dpio_lock);
-
for_each_encoder_on_crtc(dev, crtc, encoder)
if (encoder->pre_pll_enable)
encoder->pre_pll_enable(encoder);
@@ -3679,8 +3677,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
intel_crtc_update_cursor(crtc, true);
intel_update_fbc(dev);
-
- mutex_unlock(&dev_priv->dpio_lock);
}
static void i9xx_crtc_enable(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4a7ba5e..2a455b8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1719,6 +1719,7 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
int pipe = intel_crtc->pipe;
u32 val;
+ mutex_lock(&dev_priv->dpio_lock);
val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
val = 0;
if (pipe)
@@ -1732,6 +1733,7 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
0x00760018);
vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
0x00400888);
+ mutex_unlock(&dev_priv->dpio_lock);
}
}
@@ -1746,6 +1748,7 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
return;
/* Program Tx lane resets to default */
+ mutex_lock(&dev_priv->dpio_lock);
vlv_dpio_write(dev_priv, DPIO_PCS_TX(port),
DPIO_PCS_TX_LANE2_RESET |
DPIO_PCS_TX_LANE1_RESET);
@@ -1759,6 +1762,7 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
vlv_dpio_write(dev_priv, DPIO_PCS_STAGGER1(port), 0x00750f00);
vlv_dpio_write(dev_priv, DPIO_TX_CTL(port), 0x00001500);
vlv_dpio_write(dev_priv, DPIO_TX_LANE(port), 0x40400000);
+ mutex_unlock(&dev_priv->dpio_lock);
}
/*
@@ -1970,6 +1974,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
return 0;
}
+ mutex_lock(&dev_priv->dpio_lock);
vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port), 0x00000000);
vlv_dpio_write(dev_priv, DPIO_TX_SWING_CTL4(port), demph_reg_value);
vlv_dpio_write(dev_priv, DPIO_TX_SWING_CTL2(port),
@@ -1978,6 +1983,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
vlv_dpio_write(dev_priv, DPIO_PCS_STAGGER0(port), 0x00030000);
vlv_dpio_write(dev_priv, DPIO_PCS_CTL_OVER1(port), preemph_reg_value);
vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port), 0x80000000);
+ mutex_unlock(&dev_priv->dpio_lock);
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 044d11d..5fa3035 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1033,6 +1033,7 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
return;
/* Enable clock channels for this port */
+ mutex_lock(&dev_priv->dpio_lock);
val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
val = 0;
if (pipe)
@@ -1063,6 +1064,7 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
0x00760018);
vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
0x00400888);
+ mutex_unlock(&dev_priv->dpio_lock);
}
static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
@@ -1076,6 +1078,7 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
return;
/* Program Tx lane resets to default */
+ mutex_lock(&dev_priv->dpio_lock);
vlv_dpio_write(dev_priv, DPIO_PCS_TX(port),
DPIO_PCS_TX_LANE2_RESET |
DPIO_PCS_TX_LANE1_RESET);
@@ -1094,6 +1097,7 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
0x00002000);
vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port),
DPIO_TX_OCALINIT_EN);
+ mutex_lock(&dev_priv->dpio_lock);
}
static void intel_hdmi_post_disable(struct intel_encoder *encoder)
--
1.8.3.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI
2013-07-26 18:57 ` Chris Wilson
@ 2013-07-29 13:20 ` Jani Nikula
2013-07-29 14:50 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2013-07-29 13:20 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On Fri, 26 Jul 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Otherwise we get flooded by the kernel warning us that we are doing
> long sequences of IO without serialisation. For example,
>
> WARNING: CPU: 0 PID: 11136 at drivers/gpu/drm/i915/intel_sideband.c:40 vlv_sideband_rw+0x48/0x1ef()
> Modules linked in:
> CPU: 0 PID: 11136 Comm: kworker/u2:0 Tainted: G W 3.11.0-rc2+ #4
> Call Trace:
> [<c2028564>] ? warn_slowpath_common+0x63/0x78
> [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> [<c20285dd>] ? warn_slowpath_null+0xf/0x13
> [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> [<c227b060>] ? vlv_dpio_write+0x1c/0x21
> [<c2262b3b>] ? intel_dp_set_signal_levels+0x24a/0x385
> [<c2264909>] ? intel_dp_complete_link_train+0x25/0x1d1
> [<c2264c55>] ? intel_dp_check_link_status+0xf7/0x106
> [<c2238ced>] ? i915_hotplug_work_func+0x17b/0x221
> [<c203a204>] ? process_one_work+0x12e/0x210
> [<c203a5e4>] ? worker_thread+0x116/0x1ad
> [<c203a4ce>] ? rescuer_thread+0x1cb/0x1cb
> [<c203d8f5>] ? kthread+0x67/0x6c
> [<c2457ebb>] ? ret_from_kernel_thread+0x1b/0x30
> [<c203d88e>] ? init_completion+0x18/0x18
>
> v2: Retire the locking in vlv_crtc_enable() and do it close to the
> meat.
Grumble about throwing the fix and the refactoring together for no real
reason, and having a slightly misleading subject. But since we have the
warnings in place, the patch is small, and the end result is what we
want, I'll let it pass. Just this once. ;)
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_display.c | 4 ----
> drivers/gpu/drm/i915/intel_dp.c | 6 ++++++
> drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
> 3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b3389d7..6bb9017 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3653,8 +3653,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> intel_crtc->active = true;
> intel_update_watermarks(dev);
>
> - mutex_lock(&dev_priv->dpio_lock);
> -
> for_each_encoder_on_crtc(dev, crtc, encoder)
> if (encoder->pre_pll_enable)
> encoder->pre_pll_enable(encoder);
> @@ -3679,8 +3677,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> intel_crtc_update_cursor(crtc, true);
>
> intel_update_fbc(dev);
> -
> - mutex_unlock(&dev_priv->dpio_lock);
> }
>
> static void i9xx_crtc_enable(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4a7ba5e..2a455b8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1719,6 +1719,7 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
> int pipe = intel_crtc->pipe;
> u32 val;
>
> + mutex_lock(&dev_priv->dpio_lock);
> val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
> val = 0;
> if (pipe)
> @@ -1732,6 +1733,7 @@ static void intel_pre_enable_dp(struct intel_encoder *encoder)
> 0x00760018);
> vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
> 0x00400888);
> + mutex_unlock(&dev_priv->dpio_lock);
> }
> }
>
> @@ -1746,6 +1748,7 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
> return;
>
> /* Program Tx lane resets to default */
> + mutex_lock(&dev_priv->dpio_lock);
> vlv_dpio_write(dev_priv, DPIO_PCS_TX(port),
> DPIO_PCS_TX_LANE2_RESET |
> DPIO_PCS_TX_LANE1_RESET);
> @@ -1759,6 +1762,7 @@ static void intel_dp_pre_pll_enable(struct intel_encoder *encoder)
> vlv_dpio_write(dev_priv, DPIO_PCS_STAGGER1(port), 0x00750f00);
> vlv_dpio_write(dev_priv, DPIO_TX_CTL(port), 0x00001500);
> vlv_dpio_write(dev_priv, DPIO_TX_LANE(port), 0x40400000);
> + mutex_unlock(&dev_priv->dpio_lock);
> }
>
> /*
> @@ -1970,6 +1974,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
> return 0;
> }
>
> + mutex_lock(&dev_priv->dpio_lock);
> vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port), 0x00000000);
> vlv_dpio_write(dev_priv, DPIO_TX_SWING_CTL4(port), demph_reg_value);
> vlv_dpio_write(dev_priv, DPIO_TX_SWING_CTL2(port),
> @@ -1978,6 +1983,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
> vlv_dpio_write(dev_priv, DPIO_PCS_STAGGER0(port), 0x00030000);
> vlv_dpio_write(dev_priv, DPIO_PCS_CTL_OVER1(port), preemph_reg_value);
> vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port), 0x80000000);
> + mutex_unlock(&dev_priv->dpio_lock);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 044d11d..5fa3035 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1033,6 +1033,7 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
> return;
>
> /* Enable clock channels for this port */
> + mutex_lock(&dev_priv->dpio_lock);
> val = vlv_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
> val = 0;
> if (pipe)
> @@ -1063,6 +1064,7 @@ static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
> 0x00760018);
> vlv_dpio_write(dev_priv, DPIO_PCS_CLOCKBUF8(port),
> 0x00400888);
> + mutex_unlock(&dev_priv->dpio_lock);
> }
>
> static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> @@ -1076,6 +1078,7 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> return;
>
> /* Program Tx lane resets to default */
> + mutex_lock(&dev_priv->dpio_lock);
> vlv_dpio_write(dev_priv, DPIO_PCS_TX(port),
> DPIO_PCS_TX_LANE2_RESET |
> DPIO_PCS_TX_LANE1_RESET);
> @@ -1094,6 +1097,7 @@ static void intel_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> 0x00002000);
> vlv_dpio_write(dev_priv, DPIO_TX_OCALINIT(port),
> DPIO_TX_OCALINIT_EN);
> + mutex_lock(&dev_priv->dpio_lock);
> }
>
> static void intel_hdmi_post_disable(struct intel_encoder *encoder)
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI
2013-07-29 13:20 ` Jani Nikula
@ 2013-07-29 14:50 ` Chris Wilson
2013-08-05 5:35 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2013-07-29 14:50 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Mon, Jul 29, 2013 at 04:20:28PM +0300, Jani Nikula wrote:
> On Fri, 26 Jul 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Otherwise we get flooded by the kernel warning us that we are doing
> > long sequences of IO without serialisation. For example,
> >
> > WARNING: CPU: 0 PID: 11136 at drivers/gpu/drm/i915/intel_sideband.c:40 vlv_sideband_rw+0x48/0x1ef()
> > Modules linked in:
> > CPU: 0 PID: 11136 Comm: kworker/u2:0 Tainted: G W 3.11.0-rc2+ #4
> > Call Trace:
> > [<c2028564>] ? warn_slowpath_common+0x63/0x78
> > [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> > [<c20285dd>] ? warn_slowpath_null+0xf/0x13
> > [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> > [<c227b060>] ? vlv_dpio_write+0x1c/0x21
> > [<c2262b3b>] ? intel_dp_set_signal_levels+0x24a/0x385
> > [<c2264909>] ? intel_dp_complete_link_train+0x25/0x1d1
> > [<c2264c55>] ? intel_dp_check_link_status+0xf7/0x106
> > [<c2238ced>] ? i915_hotplug_work_func+0x17b/0x221
> > [<c203a204>] ? process_one_work+0x12e/0x210
> > [<c203a5e4>] ? worker_thread+0x116/0x1ad
> > [<c203a4ce>] ? rescuer_thread+0x1cb/0x1cb
> > [<c203d8f5>] ? kthread+0x67/0x6c
> > [<c2457ebb>] ? ret_from_kernel_thread+0x1b/0x30
> > [<c203d88e>] ? init_completion+0x18/0x18
> >
> > v2: Retire the locking in vlv_crtc_enable() and do it close to the
> > meat.
>
> Grumble about throwing the fix and the refactoring together for no real
> reason, and having a slightly misleading subject. But since we have the
> warnings in place, the patch is small, and the end result is what we
> want, I'll let it pass. Just this once. ;)
Meh, easier than working which paths were covered by the current locking
scheme and which weren't - which I suppect conflict anyway. The
intention was just to do as the subject said, but then I had to fix the
deadlock presented by the patch...
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI
2013-07-29 14:50 ` Chris Wilson
@ 2013-08-05 5:35 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2013-08-05 5:35 UTC (permalink / raw)
To: Chris Wilson, Jani Nikula, intel-gfx
On Mon, Jul 29, 2013 at 03:50:14PM +0100, Chris Wilson wrote:
> On Mon, Jul 29, 2013 at 04:20:28PM +0300, Jani Nikula wrote:
> > On Fri, 26 Jul 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Otherwise we get flooded by the kernel warning us that we are doing
> > > long sequences of IO without serialisation. For example,
> > >
> > > WARNING: CPU: 0 PID: 11136 at drivers/gpu/drm/i915/intel_sideband.c:40 vlv_sideband_rw+0x48/0x1ef()
> > > Modules linked in:
> > > CPU: 0 PID: 11136 Comm: kworker/u2:0 Tainted: G W 3.11.0-rc2+ #4
> > > Call Trace:
> > > [<c2028564>] ? warn_slowpath_common+0x63/0x78
> > > [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> > > [<c20285dd>] ? warn_slowpath_null+0xf/0x13
> > > [<c227ad43>] ? vlv_sideband_rw+0x48/0x1ef
> > > [<c227b060>] ? vlv_dpio_write+0x1c/0x21
> > > [<c2262b3b>] ? intel_dp_set_signal_levels+0x24a/0x385
> > > [<c2264909>] ? intel_dp_complete_link_train+0x25/0x1d1
> > > [<c2264c55>] ? intel_dp_check_link_status+0xf7/0x106
> > > [<c2238ced>] ? i915_hotplug_work_func+0x17b/0x221
> > > [<c203a204>] ? process_one_work+0x12e/0x210
> > > [<c203a5e4>] ? worker_thread+0x116/0x1ad
> > > [<c203a4ce>] ? rescuer_thread+0x1cb/0x1cb
> > > [<c203d8f5>] ? kthread+0x67/0x6c
> > > [<c2457ebb>] ? ret_from_kernel_thread+0x1b/0x30
> > > [<c203d88e>] ? init_completion+0x18/0x18
> > >
> > > v2: Retire the locking in vlv_crtc_enable() and do it close to the
> > > meat.
> >
> > Grumble about throwing the fix and the refactoring together for no real
> > reason, and having a slightly misleading subject. But since we have the
> > warnings in place, the patch is small, and the end result is what we
> > want, I'll let it pass. Just this once. ;)
>
> Meh, easier than working which paths were covered by the current locking
> scheme and which weren't - which I suppect conflict anyway. The
> intention was just to do as the subject said, but then I had to fix the
> deadlock presented by the patch...
Queued for -next, thanks for the patch. And I agree that in this case
splitting stuff further is just more awkward ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-08-05 5:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-26 16:17 [PATCH] drm/i915: Acquire dpio_lock for VLV sideband programming in DP/HDMI Chris Wilson
2013-07-26 16:46 ` Daniel Vetter
2013-07-26 17:11 ` Chris Wilson
2013-07-26 17:43 ` Daniel Vetter
2013-07-26 18:57 ` Chris Wilson
2013-07-29 13:20 ` Jani Nikula
2013-07-29 14:50 ` Chris Wilson
2013-08-05 5:35 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.