[2/2] drm/i915: Don't reprobe on resume
diff mbox

Message ID 1437058071-4035-2-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter July 16, 2015, 2:47 p.m. UTC
If we don't force the connector state to unknown there's no reason any
more to force a reprobe. Also no other driver bothers with this, so
probably it's not required - userspace handles lid/resume events
through other channels already.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Chris Wilson July 16, 2015, 3:32 p.m. UTC | #1
On Thu, Jul 16, 2015 at 04:47:51PM +0200, Daniel Vetter wrote:
> If we don't force the connector state to unknown there's no reason any
> more to force a reprobe. Also no other driver bothers with this, so
> probably it's not required - userspace handles lid/resume events
> through other channels already.

No, we don't. We don't synthesize any events at all for changing
connectors whilst suspended and userspace doesn't know about being
suspended.
-Chris
Rui Tiago Cação Matos July 16, 2015, 5:41 p.m. UTC | #2
On Thu, Jul 16, 2015 at 5:32 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Jul 16, 2015 at 04:47:51PM +0200, Daniel Vetter wrote:
>> If we don't force the connector state to unknown there's no reason any
>> more to force a reprobe. Also no other driver bothers with this, so
>> probably it's not required - userspace handles lid/resume events
>> through other channels already.
>
> No, we don't. We don't synthesize any events at all for changing
> connectors whilst suspended

I haven't tried this patch yet but note that even the current kernel
(4.1.2) isn't sending HOTPLUG uevents out in precisely that case. It
would be nice if that got fixed.

> userspace doesn't know about being
> suspended.

For GNOME, we just rely on systemd to suspend so we do know when we
resume and we can easily trigger a reprobe then. It's up to you to
decide if this should be left to userspace but please
advertise/document it clearly.

Thanks,
Rui
Daniel Vetter July 16, 2015, 6:25 p.m. UTC | #3
On Thu, Jul 16, 2015 at 07:41:14PM +0200, Rui Tiago Cação Matos wrote:
> On Thu, Jul 16, 2015 at 5:32 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Jul 16, 2015 at 04:47:51PM +0200, Daniel Vetter wrote:
> >> If we don't force the connector state to unknown there's no reason any
> >> more to force a reprobe. Also no other driver bothers with this, so
> >> probably it's not required - userspace handles lid/resume events
> >> through other channels already.
> >
> > No, we don't. We don't synthesize any events at all for changing
> > connectors whilst suspended
> 
> I haven't tried this patch yet but note that even the current kernel
> (4.1.2) isn't sending HOTPLUG uevents out in precisely that case. It
> would be nice if that got fixed.

You only get a hotplug event when something has actually changed, not for
every suspend/resume.

> > userspace doesn't know about being
> > suspended.
> 
> For GNOME, we just rely on systemd to suspend so we do know when we
> resume and we can easily trigger a reprobe then. It's up to you to
> decide if this should be left to userspace but please
> advertise/document it clearly.

I'm not fully sure, but I think all other drivers don't trigger a probe
over suspend/resume either.
-Daniel
Daniel Vetter July 16, 2015, 6:29 p.m. UTC | #4
On Thu, Jul 16, 2015 at 04:32:44PM +0100, Chris Wilson wrote:
> On Thu, Jul 16, 2015 at 04:47:51PM +0200, Daniel Vetter wrote:
> > If we don't force the connector state to unknown there's no reason any
> > more to force a reprobe. Also no other driver bothers with this, so
> > probably it's not required - userspace handles lid/resume events
> > through other channels already.
> 
> No, we don't. We don't synthesize any events at all for changing
> connectors whilst suspended and userspace doesn't know about being
> suspended.

The problem is that since 

commit 816da85a0990c2b52cfffa77637d1c770d6790e9
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Oct 23 18:23:33 2012 +0000

    drm: handle HPD and polled connectors separately

this has partially been broken (we only checked hpd connectors and not all
of them), and apparently no one noticed or complained. Also none of the
other drivers check this either. If we really need this then we need to
fix it correctly, but right now I don't see much evidence for this really.
-Daniel
Rui Tiago Cação Matos July 16, 2015, 7:18 p.m. UTC | #5
On 7/16/15, Daniel Vetter <daniel@ffwll.ch> wrote:
> You only get a hotplug event when something has actually changed, not for
> every suspend/resume.

Sure. But what about the case where something did get
plugged/unplugged while suspended. That's what I was referring to,
sorry for not being clear.

Rui
Daniel Vetter July 17, 2015, 6:38 a.m. UTC | #6
On Thu, Jul 16, 2015 at 04:32:44PM +0100, Chris Wilson wrote:
> On Thu, Jul 16, 2015 at 04:47:51PM +0200, Daniel Vetter wrote:
> > If we don't force the connector state to unknown there's no reason any
> > more to force a reprobe. Also no other driver bothers with this, so
> > probably it's not required - userspace handles lid/resume events
> > through other channels already.
> 
> No, we don't. We don't synthesize any events at all for changing
> connectors whilst suspended and userspace doesn't know about being
> suspended.

One night of sleep does wonders ;-) I agree the patch is crap and my
thinking that it's been broken since ages is also: We start the poll
helper right away and that will take care of all the non-hpd ports. It's
all fine as-is.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6f6fdb44e17a..8e86da63cf5d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -733,8 +733,6 @@  static int i915_drm_resume(struct drm_device *dev)
 	 * notifications.
 	 * */
 	intel_hpd_init(dev_priv);
-	/* Config may have changed between suspend and resume */
-	drm_helper_hpd_irq_event(dev);
 
 	intel_opregion_init(dev);