[PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Egbert Eich]

Before testing if the panel VDD is enabled on eDP cancel any pending
disable worker. This makes sure the worker doesn't fire when we expect
VDD to be enabled.

https://bugs.freedesktop.org/show_bug.cgi?id=86201

Signed-off-by: Egbert Eich <eich@suse.de>
---
 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 70bb8d0b..81f959d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
 	if (!is_edp(intel_dp))
 		return false;
 
+	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
 	intel_dp->want_panel_vdd = true;
 
 	if (edp_have_panel_vdd(intel_dp))
-- 
1.8.4.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Ville Syrjälä]

On Mon, Nov 24, 2014 at 05:56:20PM +0100, Egbert Eich wrote:
> Before testing if the panel VDD is enabled on eDP cancel any pending
> disable worker. This makes sure the worker doesn't fire when we expect
> VDD to be enabled.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=86201
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  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 70bb8d0b..81f959d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
>  	if (!is_edp(intel_dp))
>  		return false;
>  
> +	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);

This can deadlock since we're already holding pps_mutex and
edp_panel_vdd_work() also grabs it.

I'm thinking we may want something like mod_delayed_work() instead.


>  	intel_dp->want_panel_vdd = true;
>  
>  	if (edp_have_panel_vdd(intel_dp))
> -- 
> 1.8.4.5

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Ville Syrjälä]

On Mon, Nov 24, 2014 at 07:32:49PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 24, 2014 at 05:56:20PM +0100, Egbert Eich wrote:
> > Before testing if the panel VDD is enabled on eDP cancel any pending
> > disable worker. This makes sure the worker doesn't fire when we expect
> > VDD to be enabled.
> > 
> > https://bugs.freedesktop.org/show_bug.cgi?id=86201
> > 
> > Signed-off-by: Egbert Eich <eich@suse.de>
> > ---
> >  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 70bb8d0b..81f959d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
> >  	if (!is_edp(intel_dp))
> >  		return false;
> >  
> > +	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> 
> This can deadlock since we're already holding pps_mutex and
> edp_panel_vdd_work() also grabs it.
> 
> I'm thinking we may want something like mod_delayed_work() instead.

Or rather just cancel_delayed_work() is enough here I guess. W/o the
_sync we shouldn't deadlock. And if we always cancel here the timer
shoduln't be pending in edp_panel_vdd_schedule_off() anymore, so
mod_delayed_work() there would anyway just be the same as
schedule_delayed_work().

> 
> 
> >  	intel_dp->want_panel_vdd = true;
> >  
> >  	if (edp_have_panel_vdd(intel_dp))
> > -- 
> > 1.8.4.5
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Egbert Eich]

Ville Syrjälä writes:
 > On Mon, Nov 24, 2014 at 07:32:49PM +0200, Ville Syrjälä wrote:
 > > On Mon, Nov 24, 2014 at 05:56:20PM +0100, Egbert Eich wrote:
 > > > Before testing if the panel VDD is enabled on eDP cancel any pending
 > > > disable worker. This makes sure the worker doesn't fire when we expect
 > > > VDD to be enabled.
 > > > 
 > > > https://bugs.freedesktop.org/show_bug.cgi?id=86201
 > > > 
 > > > Signed-off-by: Egbert Eich <eich@suse.de>
 > > > ---
 > > >  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 70bb8d0b..81f959d 100644
 > > > --- a/drivers/gpu/drm/i915/intel_dp.c
 > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
 > > > @@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
 > > >  	if (!is_edp(intel_dp))
 > > >  		return false;
 > > >  
 > > > +	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
 > > 
 > > This can deadlock since we're already holding pps_mutex and
 > > edp_panel_vdd_work() also grabs it.
 > > 
 > > I'm thinking we may want something like mod_delayed_work() instead.
 > 
 > Or rather just cancel_delayed_work() is enough here I guess. W/o the
 > _sync we shouldn't deadlock. And if we always cancel here the timer
 > shoduln't be pending in edp_panel_vdd_schedule_off() anymore, so
 > mod_delayed_work() there would anyway just be the same as
 > schedule_delayed_work().

Right. I was thinking we need to make sure we are not in the midst of
the worker but the pps_mutexes indeed serialize this as well.

Will resend the patch.

Thanks!
Cheers,
	Egbert.
 > 
 > > 
 > > 
 > > >  	intel_dp->want_panel_vdd = true;
 > > >  
 > > >  	if (edp_have_panel_vdd(intel_dp))
 > > > -- 
 > > > 1.8.4.5
 > > 
 > > -- 
 > > Ville Syrjälä
 > > Intel OTC
 > 
 > -- 
 > Ville Syrjälä
 > Intel OTC

-- 
Any jackass can report a bug, but it takes a good engineer to fix one.
   Loosely base on 
   	 Sam Rayburn (48th, 50th and 52nd Speaker of the US House of Rep.)

Egbert Eich (Res. & Dev.)                SUSE LINUX Products GmbH
SUSE Labs KMS / DRM / X Window System Development
Tel: +49 911-740 53 0                       http://www.suse.de 
-----------------------------------------------------------------
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 21284 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Daniel Vetter]

On Mon, Nov 24, 2014 at 5:56 PM, Egbert Eich <eich@suse.de> wrote:
> Before testing if the panel VDD is enabled on eDP cancel any pending
> disable worker. This makes sure the worker doesn't fire when we expect
> VDD to be enabled.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=86201
>
> Signed-off-by: Egbert Eich <eich@suse.de>

This shouldn't be needed at all:
- The vdd off rechecks ->want_panel_vdd under the pps lock.
- The off function sets that and also reschedules the work (to make
sure it doesn't kill vdd to early) again all under the same lock.

So no one can sneak in and the work racing with us isn't an issue. Or
shouldn't be at least. So if this helps we need to dig a bit deeper.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Ville Syrjälä]

On Mon, Nov 24, 2014 at 08:46:22PM +0100, Daniel Vetter wrote:
> On Mon, Nov 24, 2014 at 5:56 PM, Egbert Eich <eich@suse.de> wrote:
> > Before testing if the panel VDD is enabled on eDP cancel any pending
> > disable worker. This makes sure the worker doesn't fire when we expect
> > VDD to be enabled.
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=86201
> >
> > Signed-off-by: Egbert Eich <eich@suse.de>
> 
> This shouldn't be needed at all:
> - The vdd off rechecks ->want_panel_vdd under the pps lock.
> - The off function sets that and also reschedules the work (to make
> sure it doesn't kill vdd to early) again all under the same lock.

It uses schedule_delayed_work() which does nothing if the work is
already pending. Hence the delay will be counted from the first vdd_off,
not the last one.

But doing the cancel up front instead of using mod_delayed_work() has
the extra benefit of making it a bit less likely vdd will get turned off
if the transfer of a single AUX message takes longer than the vdd off
delay.

However even with the cancel it'ss still possible the vdd off work already
started executing (but is blocked by pps_mutex) by the time we call
edp_panel_vdd_on(), in which case vdd would still be turned off as soon
as pps_mutex is released. We could track the last time vdd was used
to avoid that, but I'm not sure it's really worth the extra effort.

> 
> So no one can sneak in and the work racing with us isn't an issue. Or
> shouldn't be at least. So if this helps we need to dig a bit deeper.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Daniel Vetter]

On Tue, Nov 25, 2014 at 9:43 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Nov 24, 2014 at 08:46:22PM +0100, Daniel Vetter wrote:
>> On Mon, Nov 24, 2014 at 5:56 PM, Egbert Eich <eich@suse.de> wrote:
>> > Before testing if the panel VDD is enabled on eDP cancel any pending
>> > disable worker. This makes sure the worker doesn't fire when we expect
>> > VDD to be enabled.
>> >
>> > https://bugs.freedesktop.org/show_bug.cgi?id=86201
>> >
>> > Signed-off-by: Egbert Eich <eich@suse.de>
>>
>> This shouldn't be needed at all:
>> - The vdd off rechecks ->want_panel_vdd under the pps lock.
>> - The off function sets that and also reschedules the work (to make
>> sure it doesn't kill vdd to early) again all under the same lock.
>
> It uses schedule_delayed_work() which does nothing if the work is
> already pending. Hence the delay will be counted from the first vdd_off,
> not the last one.
>
> But doing the cancel up front instead of using mod_delayed_work() has
> the extra benefit of making it a bit less likely vdd will get turned off
> if the transfer of a single AUX message takes longer than the vdd off
> delay.
>
> However even with the cancel it'ss still possible the vdd off work already
> started executing (but is blocked by pps_mutex) by the time we call
> edp_panel_vdd_on(), in which case vdd would still be turned off as soon
> as pps_mutex is released. We could track the last time vdd was used
> to avoid that, but I'm not sure it's really worth the extra effort.

Hm right. So at least the commit message needs a bit more clarity
since the check in the worker will ensure that we don't disable vdd
when we need it. The real problem is that the hysteresis isn't fully
obeyed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Egbert Eich]

Before testing if the panel VDD is enabled on eDP cancel any pending
disable worker. This makes sure the worker will be triggered with a
delay from the last time edp_panel_vdd_schedule_off() is called, not
the first time. This avoids unnecessary overhead.

https://bugs.freedesktop.org/show_bug.cgi?id=86201

v2: use cancel_delayed_work() instead of cancel_delayed_work_sync()
as the pps_mutexes will provide the required serialization with
edp_panel_vdd_work() while the sync variant may deadlock. Suggested
by Ville Syrjälä <ville.syrjala@linux.intel.com>.
Made commit message a bit clearer.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 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 70bb8d0b..7ab39d4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
 	if (!is_edp(intel_dp))
 		return false;
 
+	cancel_delayed_work(&intel_dp->panel_vdd_work);
 	intel_dp->want_panel_vdd = true;
 
 	if (edp_have_panel_vdd(intel_dp))
-- 
1.8.4.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Daniel Vetter]

On Tue, Nov 25, 2014 at 12:54:57PM +0100, Egbert Eich wrote:
> Before testing if the panel VDD is enabled on eDP cancel any pending
> disable worker. This makes sure the worker will be triggered with a
> delay from the last time edp_panel_vdd_schedule_off() is called, not
> the first time. This avoids unnecessary overhead.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=86201
> 
> v2: use cancel_delayed_work() instead of cancel_delayed_work_sync()
> as the pps_mutexes will provide the required serialization with
> edp_panel_vdd_work() while the sync variant may deadlock. Suggested
> by Ville Syrjälä <ville.syrjala@linux.intel.com>.
> Made commit message a bit clearer.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

Yeah makes sense now to me with the updated commit message and all ;-)

Queued for -next, thanks for the patch.
-Daniel

> ---
>  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 70bb8d0b..7ab39d4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
>  	if (!is_edp(intel_dp))
>  		return false;
>  
> +	cancel_delayed_work(&intel_dp->panel_vdd_work);
>  	intel_dp->want_panel_vdd = true;
>  
>  	if (edp_have_panel_vdd(intel_dp))
> -- 
> 1.8.4.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker

[Egbert Eich]

Daniel Vetter writes:
 > On Mon, Nov 24, 2014 at 5:56 PM, Egbert Eich <eich@suse.de> wrote:
 > > Before testing if the panel VDD is enabled on eDP cancel any pending
 > > disable worker. This makes sure the worker doesn't fire when we expect
 > > VDD to be enabled.
 > >
 > > https://bugs.freedesktop.org/show_bug.cgi?id=86201
 > >
 > > Signed-off-by: Egbert Eich <eich@suse.de>
 > 
 > This shouldn't be needed at all:
 > - The vdd off rechecks ->want_panel_vdd under the pps lock.
 > - The off function sets that and also reschedules the work (to make
 > sure it doesn't kill vdd to early) again all under the same lock.
 
No. edp_panel_vdd_off() calls edp_panel_vdd_schedule_off() when not called
with sync == true. edp_panel_vdd_schedule_off() calls schedule_delayed_work()
which doesn't reschedule pending work.

 > 
 > So no one can sneak in and the work racing with us isn't an issue. Or
 > shouldn't be at least. So if this helps we need to dig a bit deeper.

Daniel, I came across this when I was looking for the problem in fdo#86201.
And I agree, it is not strictly needed, however if you follow
fdo#86201 you will see a list of calls to edp_panel_vdd_off_sync()
soon to be followed by calls to edp_panel_vdd_on(). Many of them are 
unnecessary and can be gotten rid of the uneeded ones by canelling the work 
queue. (When you follow fdo#86201 you will see why there was an abnormal
situation and what caused it). 
Of course due to the locking we already have serialization and canelling 
pending workers did not resolve the issue but it at least got rid of some
unneeded overhead.

Cheers,
	Egbert.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx