[PATCH] drm/i915/dp: Flush any outstanding work to turn the VDD off

[PATCH] drm/i915/dp: Flush any outstanding work to turn the VDD off

[Chris Wilson]

As we may kick off a delayed workqueue task to switch of the VDD lines, we
need to complete that task prior to turning off the panel (which itself
depends upon VDD being off).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f07652b..7c2b5e1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1040,6 +1040,12 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
 	}
 }
 
+static void ironlake_panel_vdd_off_flush(struct intel_dp *intel_dp)
+{
+	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+	ironlake_panel_vdd_off_sync(intel_dp);
+}
+
 static void ironlake_panel_vdd_work(struct work_struct *__work)
 {
 	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
@@ -1128,6 +1134,7 @@ static void ironlake_edp_panel_off(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("Turn eDP power off\n");
 
 	WARN(intel_dp->want_panel_vdd, "Cannot turn power off while VDD is on\n");
+	ironlake_panel_vdd_off_flush(intel_dp);
 
 	pp = ironlake_get_pp_control(dev_priv);
 	pp &= ~(POWER_TARGET_ON | EDP_FORCE_VDD | PANEL_POWER_RESET | EDP_BLC_ENABLE);
-- 
1.7.10

Re: [PATCH] drm/i915/dp: Flush any outstanding work to turn the VDD off

[Daniel Vetter]

On Mon, Apr 16, 2012 at 03:18:33PM +0100, Chris Wilson wrote:
> As we may kick off a delayed workqueue task to switch of the VDD lines, we
> need to complete that task prior to turning off the panel (which itself
> depends upon VDD being off).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Keith Packard <keithp@keithp.com>

I've looked a bit at this and I think we just need a call to
ironlake_panel_vdd_off_sync instead of the WARN. The work will do the
right thing when the vdd is already off, so no problem there.

What problem we have though is that when we call cancel_work_sync we're
holding the config mutex, which the vdd work needs, too. Which is a nice
deadlock (which can currently only happen at module unload). I guess we
need a vdd_power mutex in the intel_dp struct just for this, so that we
can cancel the work without holding it, but protect all the vdd state
tracking from concurrent changes, still. The work would the only grab the
vdd_power mutex.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f07652b..7c2b5e1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1040,6 +1040,12 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  	}
>  }
>  
> +static void ironlake_panel_vdd_off_flush(struct intel_dp *intel_dp)
> +{
> +	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> +	ironlake_panel_vdd_off_sync(intel_dp);
> +}
> +
>  static void ironlake_panel_vdd_work(struct work_struct *__work)
>  {
>  	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> @@ -1128,6 +1134,7 @@ static void ironlake_edp_panel_off(struct intel_dp *intel_dp)
>  	DRM_DEBUG_KMS("Turn eDP power off\n");
>  
>  	WARN(intel_dp->want_panel_vdd, "Cannot turn power off while VDD is on\n");
> +	ironlake_panel_vdd_off_flush(intel_dp);
>  
>  	pp = ironlake_get_pp_control(dev_priv);
>  	pp &= ~(POWER_TARGET_ON | EDP_FORCE_VDD | PANEL_POWER_RESET | EDP_BLC_ENABLE);
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915/dp: Flush any outstanding work to turn the VDD off

[Chris Wilson]

On Mon, 16 Apr 2012 22:39:51 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Apr 16, 2012 at 03:18:33PM +0100, Chris Wilson wrote:
> > As we may kick off a delayed workqueue task to switch of the VDD lines, we
> > need to complete that task prior to turning off the panel (which itself
> > depends upon VDD being off).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Keith Packard <keithp@keithp.com>
> 
> I've looked a bit at this and I think we just need a call to
> ironlake_panel_vdd_off_sync instead of the WARN. The work will do the
> right thing when the vdd is already off, so no problem there.

The WARN is valid and did help to document the design of the code.
Good point about the cancel_sync taking the mutex again, oops, and
indeed all we need here is just the call to sync.

As far as I can see, the timer will expire gracefully under normal
conditions with the only cancel_sync being during shutdown, so
serialisation of the tasklet under mode_config looks robust.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915/dp: Flush any outstanding work to turn the VDD off

[Chris Wilson]

As we may kick off a delayed workqueue task to switch of the VDD lines, we
need to complete that task prior to turning off the panel (which itself
depends upon VDD being off).

v2: Don't cancel the outstanding work as this may trigger a deadlock

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f07652b..05d2f60 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1128,6 +1128,7 @@ static void ironlake_edp_panel_off(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("Turn eDP power off\n");
 
 	WARN(intel_dp->want_panel_vdd, "Cannot turn power off while VDD is on\n");
+	ironlake_panel_vdd_off_sync(intel_dp); /* finish any pending work */
 
 	pp = ironlake_get_pp_control(dev_priv);
 	pp &= ~(POWER_TARGET_ON | EDP_FORCE_VDD | PANEL_POWER_RESET | EDP_BLC_ENABLE);
-- 
1.7.10

Re: [PATCH] drm/i915/dp: Flush any outstanding work to turn the VDD off

[Daniel Vetter]

On Mon, Apr 16, 2012 at 10:01:42PM +0100, Chris Wilson wrote:
> On Mon, 16 Apr 2012 22:39:51 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Apr 16, 2012 at 03:18:33PM +0100, Chris Wilson wrote:
> > > As we may kick off a delayed workqueue task to switch of the VDD lines, we
> > > need to complete that task prior to turning off the panel (which itself
> > > depends upon VDD being off).
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Keith Packard <keithp@keithp.com>
> > 
> > I've looked a bit at this and I think we just need a call to
> > ironlake_panel_vdd_off_sync instead of the WARN. The work will do the
> > right thing when the vdd is already off, so no problem there.
> 
> The WARN is valid and did help to document the design of the code.
> Good point about the cancel_sync taking the mutex again, oops, and
> indeed all we need here is just the call to sync.

Oops, yeah I've misread what want_panle_vdd means, the WARN looks good
as-is.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915/dp: Flush any outstanding work to turn the VDD off

[Daniel Vetter]

On Mon, Apr 16, 2012 at 10:43:42PM +0100, Chris Wilson wrote:
> As we may kick off a delayed workqueue task to switch of the VDD lines, we
> need to complete that task prior to turning off the panel (which itself
> depends upon VDD being off).
> 
> v2: Don't cancel the outstanding work as this may trigger a deadlock
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Keith Packard <keithp@keithp.com>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48