Re: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker
On Mon, Nov 24, 2014 at 08:46:22PM +0100, Daniel Vetter wrote: On Mon, Nov 24, 2014 at 5:56 PM, Egbert Eich e...@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 e...@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: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker
On Tue, Nov 25, 2014 at 9:43 AM, Ville Syrjälä ville.syrj...@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 e...@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 e...@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
Re: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker
Daniel Vetter writes: On Mon, Nov 24, 2014 at 5:56 PM, Egbert Eich e...@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 e...@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
[Intel-gfx] [PATCH] 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 Signed-off-by: Egbert Eich e...@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: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker
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 e...@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: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker
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 e...@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: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker
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 e...@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: [Intel-gfx] [PATCH] drm/i915/eDP: When enabling panel VDD cancel pending disable worker
On Mon, Nov 24, 2014 at 5:56 PM, Egbert Eich e...@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 e...@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
