drm/i915/eDP: When enabling panel VDD cancel pending disable worker
Subsystems (2)
PATCH 1/1 drm/i915/eDP: When enabling panel VDD cancel pending disable worker
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
--- drivers/gpu/drm/i915/intel_dp.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpu/drm/i915/intel_dp.c +1 -0
@@ -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))
Signature · 6 lines
1.8.4.5 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 16 lines · 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 <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c static bool edp_panel_vdd_on(struct intel_dp *intel_dp) · line 1506
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -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.
Quoted 5 lines
drivers/gpu/drm/i915/intel_dp.c +0
-0intel_dp->want_panel_vdd = true;if (edp_have_panel_vdd (intel_dp))Signature · 1 line
1.8.4.5
Signature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 20 lines · Ville Syrjälä
Quoted 19 lines · 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 <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -1503,6 +1503,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)if (!is_edp (intel_dp))return false;
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c static bool edp_panel_vdd_on(struct intel_dp *intel_dp) · line 1506
Quoted 1 line
drivers/gpu/drm/i915/intel_dp.c +1
-0+cancel_delayed_work_sync (&intel_dp->panel_vdd_work);This can deadlock since we're already holding
pps_mutexandedp_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().
Quoted 11 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +0
-0intel_dp->want_panel_vdd = true;if (edp_have_panel_vdd (intel_dp))Signature · 1 line
1.8.4.5Signature · 2 lines
Ville Syrjälä Intel OTC
Signature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 26 lines · Ville Syrjälä
Quoted 25 lines · Ville Syrjälä
Quoted 16 lines · 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 <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/i915/intel_dp.c +0
-0drivers/gpu/drm/i915/intel_dp.c static bool edp_panel_vdd_on(struct intel_dp *intel_dp) · line 1506
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -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_mutexandedp_panel_vdd_work()also grabs it.
Quoted 7 lines
Quoted 1 line
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_syncwe shouldn't deadlock. And if we always cancel here the timer shoduln't be pending inedp_panel_vdd_schedule_off()anymore, somod_delayed_work()there would anyway just be the same asschedule_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.
Quoted 16 lines
Quoted 12 lines
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +0
-0intel_dp->want_panel_vdd = true;if (edp_have_panel_vdd (intel_dp))Signature · 1 line
1.8.4.5Signature · 2 lines
Ville Syrjälä Intel OTCSignature · 2 lines
Ville Syrjälä Intel OTC
Signature · 17 lines
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 [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 7 lines · 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 <[email protected]>
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
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 5 lines · Daniel Vetter
Quoted 4 lines · 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.
Quoted 8 lines
Quoted 3 lines
https://bugs.freedesktop.org/show_bug.cgi?id=86201
Signed-off-by Egbert Eich <[email protected]>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.
Quoted 12 lines
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
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 24 lines · Ville Syrjälä
Quoted 5 lines · Daniel Vetter
Quoted 4 lines · 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.
Quoted 8 lines
Quoted 3 lines
https://bugs.freedesktop.org/show_bug.cgi?id=86201
Signed-off-by Egbert Eich <[email protected]>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 firstvdd_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.
Quoted 5 lines
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 calledp_panel_vdd_on(), in which case vdd would still be turned off as soon aspps_mutexis 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
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v2 1/1 drm/i915/eDP: When enabling panel VDD cancel pending disable worker
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ä <[email protected]>.
Made commit message a bit clearer.
--- drivers/gpu/drm/i915/intel_dp.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpu/drm/i915/intel_dp.c +1 -0
@@ -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))
Signature · 6 lines
1.8.4.5 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter
AppliedQuoted 7 lines · 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.
Quoted 7 lines
v2: use
cancel_delayed_work()instead ofcancel_delayed_work_sync()as thepps_mutexeswill provide the required serialization withedp_panel_vdd_work()while the sync variant may deadlock. Suggested by Ville Syrjälä <[email protected]>.
Made commit message a bit clearer.Signed-off-by Egbert Eich <[email protected]>
Yeah makes sense now to me with the updated commit message and all ;-)
Queued for -next, thanks for the patch.
-Daniel
Quoted 19 lines
--- drivers/gpu/drm/i915/intel_dp.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -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))Signature · 1 line
1.8.4.5
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 5 lines · Daniel Vetter
Quoted 4 lines · 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.
Quoted 8 lines
Quoted 3 lines
https://bugs.freedesktop.org/show_bug.cgi?id=86201
Signed-off-by Egbert Eich <[email protected]>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.
Quoted 3 lines
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx