DRM / i915: Fix resume regression on MSI Wind U100 w/o KMS

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Quoted 32 lines · On Fri, 8 Jan 2010 16:01:46 -0800 (PST) Linus Torvalds…

Quoted 10 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

Quoted 2 lines

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Yeah, but that would be more invasive. In the KMS case the driver (which is registered as PCI) does a lot of the initialization that the core takes care of in the non-KMS case, and some of it happens later at ioctl time. I'm afraid of that code since it seems like whenever you change something obvious it subtly breaks an old userland.

Quoted 35 lines · On Fri, 8 Jan 2010 16:06:59 -0800 Jesse Barnes <[email protected]> wrote:

Quoted 32 lines · On Fri, 8 Jan 2010 16:01:46 -0800 (PST) Linus Torvalds…

Quoted 10 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

Quoted 8 lines

Quoted 2 lines

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Yeah, but that would be more invasive. In the KMS case the driver (which is registered as PCI) does a lot of the initialization that the core takes care of in the non-KMS case, and some of it happens later at ioctl time. I'm afraid of that code since it seems like whenever you change something obvious it subtly breaks an old userland.

Hm, maybe it's not as bad as I was afraid it was... we already support i915.modeset=0 even on a KMS enabled driver, which should be fairly equivalent. Rafael, if you build i915 with KMS enabled but modeset=0 do you get the right suspend/resume behavior?

Quoted 35 lines · On Fri, 8 Jan 2010 16:06:59 -0800 Jesse Barnes <[email protected]> wrote:

Quoted 32 lines · On Fri, 8 Jan 2010 16:01:46 -0800 (PST) Linus Torvalds…

Quoted 10 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

Quoted 8 lines

Quoted 2 lines

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Yeah, but that would be more invasive. In the KMS case the driver (which is registered as PCI) does a lot of the initialization that the core takes care of in the non-KMS case, and some of it happens later at ioctl time. I'm afraid of that code since it seems like whenever you change something obvious it subtly breaks an old userland.

Hm, maybe it's not as bad as I was afraid it was... we already support i915.modeset=0 even on a KMS enabled driver, which should be fairly equivalent. Rafael, if you build i915 with KMS enabled but modeset=0 do you get the right suspend/resume behavior?

Quoted 32 lines · On Fri, 8 Jan 2010 16:01:46 -0800 (PST) Linus Torvalds…

Quoted 10 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

Quoted 2 lines

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Yeah, but that would be more invasive. In the KMS case the driver (which is registered as PCI) does a lot of the initialization that the core takes care of in the non-KMS case, and some of it happens later at ioctl time. I'm afraid of that code since it seems like whenever you change something obvious it subtly breaks an old userland.

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Quoted 26 lines · On Fri, 8 Jan 2010 16:01:46 -0800 (PST) Linus Torvalds…

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

Quoted 7 lines

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

This is the minimal fix I think (totally untested):

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.c index a0a2cad..1364c3e 100644

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Quoted 4 lines · Rafael J. Wysocki

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Ahh, right you are - that class suspend function does a check for DRIVER_MODESET, and only does the suspend/resume if it's not a MODESET driver.

Ok, so I withdraw my objections to your original patch - it's confusing, but that's just because DRM is such a horrible mess with subtle things.

Quoted 8 lines · On Fri, 8 Jan 2010 16:50:57 -0800 (PST) Linus Torvalds…

Quoted 4 lines · Rafael J. Wysocki

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Quoted 7 lines

Ahh, right you are - that class suspend function does a check for DRIVER_MODESET, and only does the suspend/resume if it's not a MODESET driver.

Ok, so I withdraw my objections to your original patch - it's confusing, but that's just because DRM is such a horrible mess with subtle things.

Yeah the non-KMS paths just suck.

Acked-by Jesse Barnes <[email protected]>

Though hopefully you can get the PCI driver registration working w/o too much trouble; that would be even better.

Quoted 19 lines · Jesse Barnes

Quoted 8 lines · On Fri, 8 Jan 2010 16:50:57 -0800 (PST) Linus Torvalds…

Quoted 4 lines · Rafael J. Wysocki

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Quoted 7 lines

Ahh, right you are - that class suspend function does a check for DRIVER_MODESET, and only does the suspend/resume if it's not a MODESET driver.

Ok, so I withdraw my objections to your original patch - it's confusing, but that's just because DRM is such a horrible mess with subtle things.

Quoted 6 lines

Yeah the non-KMS paths just suck.

Acked-by Jesse Barnes <[email protected]>

Though hopefully you can get the PCI driver registration working w/o too much trouble; that would be even better.

Actually, I have a working patch, with one tiny detail I'm not sure of.

Namely, I need to call pci_set_drvdata(pdev, dev) unconditionally in drm_stub.c for the things to work, but I _think_ it won't hurt even if we're not going to use the pdev's private data.

The benefit of this is having just one code path for suspend/resume instead of two different code paths depending on whether the driver is using the KMS or not, which is well worth it IMO.

The patch is appended.

Quoted 33 lines · Rafael J. Wysocki

Quoted 19 lines · Jesse Barnes

Quoted 8 lines · On Fri, 8 Jan 2010 16:50:57 -0800 (PST) Linus Torvalds…

Quoted 4 lines · Rafael J. Wysocki

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Quoted 7 lines

Ahh, right you are - that class suspend function does a check for DRIVER_MODESET, and only does the suspend/resume if it's not a MODESET driver.

Ok, so I withdraw my objections to your original patch - it's confusing, but that's just because DRM is such a horrible mess with subtle things.

Quoted 6 lines

Yeah the non-KMS paths just suck.

Acked-by Jesse Barnes <[email protected]>

Though hopefully you can get the PCI driver registration working w/o too much trouble; that would be even better.

Actually, I have a working patch, with one tiny detail I'm not sure of.

Namely, I need to call pci_set_drvdata(pdev, dev) unconditionally in drm_stub.c for the things to work, but I _think_ it won't hurt even if we're not going to use the pdev's private data.

Quoted 5 lines

The benefit of this is having just one code path for suspend/resume instead of two different code paths depending on whether the driver is using the KMS or not, which is well worth it IMO.

The patch is appended.

NAK

for the reasons I explained in the previous email. This conflicts with systems where intelfb and intel drm are used together, this is something that ppl do use prior to KMS happening.

We just need to document in the headers why the hooks are needed, and maybe a bit of patch review to make sure nobody removes them again.

Quoted 33 lines · Rafael J. Wysocki

Quoted 19 lines · Jesse Barnes

Quoted 8 lines · On Fri, 8 Jan 2010 16:50:57 -0800 (PST) Linus Torvalds…

Quoted 4 lines · Rafael J. Wysocki

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Quoted 7 lines

Ahh, right you are - that class suspend function does a check for DRIVER_MODESET, and only does the suspend/resume if it's not a MODESET driver.

Ok, so I withdraw my objections to your original patch - it's confusing, but that's just because DRM is such a horrible mess with subtle things.

Quoted 6 lines

Yeah the non-KMS paths just suck.

Acked-by Jesse Barnes <[email protected]>

Though hopefully you can get the PCI driver registration working w/o too much trouble; that would be even better.

Actually, I have a working patch, with one tiny detail I'm not sure of.

Namely, I need to call pci_set_drvdata(pdev, dev) unconditionally in drm_stub.c for the things to work, but I _think_ it won't hurt even if we're not going to use the pdev's private data.

Quoted 5 lines

The benefit of this is having just one code path for suspend/resume instead of two different code paths depending on whether the driver is using the KMS or not, which is well worth it IMO.

The patch is appended.

NAK

for the reasons I explained in the previous email. This conflicts with systems where intelfb and intel drm are used together, this is something that ppl do use prior to KMS happening.

We just need to document in the headers why the hooks are needed, and maybe a bit of patch review to make sure nobody removes them again.

Quoted 19 lines · Jesse Barnes

Quoted 8 lines · On Fri, 8 Jan 2010 16:50:57 -0800 (PST) Linus Torvalds…

Quoted 4 lines · Rafael J. Wysocki

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Quoted 7 lines

Ahh, right you are - that class suspend function does a check for DRIVER_MODESET, and only does the suspend/resume if it's not a MODESET driver.

Ok, so I withdraw my objections to your original patch - it's confusing, but that's just because DRM is such a horrible mess with subtle things.

Quoted 6 lines

Yeah the non-KMS paths just suck.

Acked-by Jesse Barnes <[email protected]>

Though hopefully you can get the PCI driver registration working w/o too much trouble; that would be even better.

Actually, I have a working patch, with one tiny detail I'm not sure of.

Namely, I need to call pci_set_drvdata(pdev, dev) unconditionally in drm_stub.c for the things to work, but I _think_ it won't hurt even if we're not going to use the pdev's private data.

The benefit of this is having just one code path for suspend/resume instead of two different code paths depending on whether the driver is using the KMS or not, which is well worth it IMO.

The patch is appended.

Quoted 8 lines · On Fri, 8 Jan 2010 16:50:57 -0800 (PST) Linus Torvalds…

Quoted 4 lines · Rafael J. Wysocki

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Quoted 7 lines

Ahh, right you are - that class suspend function does a check for DRIVER_MODESET, and only does the suspend/resume if it's not a MODESET driver.

Ok, so I withdraw my objections to your original patch - it's confusing, but that's just because DRM is such a horrible mess with subtle things.

Yeah the non-KMS paths just suck.

Acked-by Jesse Barnes <[email protected]>

Though hopefully you can get the PCI driver registration working w/o too much trouble; that would be even better.

Quoted 4 lines · Rafael J. Wysocki

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Ahh, right you are - that class suspend function does a check for DRIVER_MODESET, and only does the suspend/resume if it's not a MODESET driver.

Ok, so I withdraw my objections to your original patch - it's confusing, but that's just because DRM is such a horrible mess with subtle things.

Which is functionally equivalent to my patch, because i915_suspend/resume() won't be called by drm_class_suspend/resume() in the KMS case anyway.

Quoted 26 lines · On Fri, 8 Jan 2010 16:01:46 -0800 (PST) Linus Torvalds…

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

Quoted 7 lines

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

This is the minimal fix I think (totally untested):

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_drv.c index a0a2cad..1364c3e 100644

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Quoted 8 lines

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

Quoted 28 lines

Quoted 9 lines

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

Quoted 3 lines

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

This was mainly due to pre-existing fb drivers binding to the device, and the drm drivers having to work around that, with KMS since we have fb in the drm driver its correct to bind, pre-kms its just a mess I'd rather stay away from.

Quoted 28 lines · On Sat, 9 Jan 2010 02:15:41 +0000 (GMT) Dave Airlie <[email protected]> wrote:

Quoted 28 lines

Quoted 9 lines

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

Quoted 8 lines

Quoted 3 lines

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

This was mainly due to pre-existing fb drivers binding to the device, and the drm drivers having to work around that, with KMS since we have fb in the drm driver its correct to bind, pre-kms its just a mess I'd rather stay away from.

Linus, can we ever drop those old paths? Maybe after the new bits have been around for awhile? Users of really old userspace stacks would lose 3D support, but they'd still have 2D, so it wouldn't be a complete break. The non-KMS paths sometimes break like this anyway without us noticing (especially some of the weirder 3D paths)...

Just thinking out loud, we could really kill a lot of really bad code...

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Quoted 8 lines · Jesse Barnes

Linus, can we ever drop those old paths? Maybe after the new bits have been around for awhile? Users of really old userspace stacks would lose 3D support, but they'd still have 2D, so it wouldn't be a complete break. The non-KMS paths sometimes break like this anyway without us noticing (especially some of the weirder 3D paths)...

Just thinking out loud, we could really kill a lot of really bad code...

I among those who would love such things to happen :)

Quoted 11 lines · Jerome Glisse

Quoted 8 lines · Jesse Barnes

Linus, can we ever drop those old paths?  Maybe after the new bits have been around for awhile?  Users of really old userspace stacks would lose 3D support, but they'd still have 2D, so it wouldn't be a complete break.  The non-KMS paths sometimes break like this anyway without us noticing (especially some of the weirder 3D paths)...

Just thinking out loud, we could really kill a lot of really bad code...

I among those who would love such things to happen :)

I don't want to drop it _yet_, but "ever"? Sure. When people are sure that KMS actually handles all the cases that old X does (maybe that's true now), and we've had more than just a couple of kernel releases of _stable_ Intel KMS, I suspect we can start thinking about "ok, nobody seriously uses 3D on Intel integrated graphics _and_ updates the kernel".

The fact that they'd still have a working X setup would make it generally much more palatable, I think.

But we definitely need more than just a couple of kernel releases. So we're talking timescales of "more than a year of stable code". Whether that is "six months from now" or "two years from now", I can't judge.

And people can try to convince me to be more or less aggressive about it, so take the above as a more of a personal opinion that is open to change than anything definite and final.

I'm in the 2-3 years at a minimum, with at least one kernel with no serious regressions in Intel KMS, which we haven't gotten close to yet. I'm not even sure the Intel guys are taking stable seriously enough yet. So far I don't think there is one kernel release (even stable) that works on all Intel chipsets without backporting patches. 2.6.32 needs the changes to remove the messed up render clock hacks which should really have been reverted a lot earlier since we had a lot of regression reports. The number of users using powersave=0 to get anything approaching useable is growing etc.

We do have ppl who run latest kernels on RHEL5 userspace and I'd rather not have that break badly, I'm guessing more than 3D will break if we remove this, since we need the DRM to allocate memory for 2D stuff, and will probably find the fallback to AGP is broken. Again Intel ppl would have to do a lot of testing on the fallback before removing anything, which is time I don't see anyone willing to spend.

Quoted 9 lines · On Sun, 10 Jan 2010 07:32:30 +1000 Dave Airlie <[email protected]> wrote:

I'm in the 2-3 years at a minimum, with at least one kernel with no serious regressions in Intel KMS, which we haven't gotten close to yet. I'm not even sure the Intel guys are taking stable seriously enough yet. So far I don't think there is one kernel release (even stable) that works on all Intel chipsets without backporting patches. 2.6.32 needs the changes to remove the messed up render clock hacks which should really have been reverted a lot earlier since we had a lot of regression reports. The number of users using powersave=0 to get anything approaching useable is growing etc.

But you could apply that argument to the existing DRM code (not just Intel) as well; lots of things are broken or unimplemented and never get fixed.  I'd say the right metric isn't whether regressions are introduced (usually due to new features) but whether the driver is better than the old userspace code.  For Intel at least, I think we're already there.  The quality of the kernel driver is higher and it has many more features than the userspace implementation ever did.  That's just my subjective opinion, but I've done a *lot* of work on our bugs both in userspace and in the kernel, so I think it's an accurate statement.

It doesn't have to happen anytime soon, I was just thinking that removing the old, pre-KMS code would make it easier to avoid introducing regressions since we'd have one less config (a bit one atthat) to worry about.

Quoted 13 lines · Jesse Barnes

Quoted 10 lines · On Sun, 10 Jan 2010 07:32:30 +1000 Dave Airlie <[email protected]> wrote:

I'm in the 2-3 years at a minimum, with at least one kernel with no serious regressions in Intel KMS, which we haven't gotten close to yet. I'm not even sure the Intel guys are taking stable seriously enough yet. So far I don't think there is one kernel release (even stable) that works on all Intel chipsets without backporting patches. 2.6.32 needs the changes to remove the messed up render clock hacks which should really have been reverted a lot earlier since we had a lot of regression reports. The number of users using powersave=0 to get anything approaching useable is growing etc.

Quoted 10 lines

But you could apply that argument to the existing DRM code (not just Intel) as well; lots of things are broken or unimplemented and never get fixed.  I'd say the right metric isn't whether regressions are introduced (usually due to new features) but whether the driver is better than the old userspace code.  For Intel at least, I think we're already there.  The quality of the kernel driver is higher and it has many more features than the userspace implementation ever did.  That's just my subjective opinion, but I've done a *lot* of work on our bugs both in userspace and in the kernel, so I think it's an accurate statement.

The problem is at any single point in time I'm not sure a kms kernel exists that works across all the Intel hw, which from a distro POV is a real pain in the ass, a regression gets fixed on one piece of hw just as another on a different piece gets introduced.

I'd really like if Intel devs could either slow it down and do more testing before pushing to Linus, or be a lot quicker with the reverts when stuff is identified. The main thing is the render reclocking lately, thats been a nightmare and as far as I can see 2.6.32.3 still has all the issues,

Quoted 4 lines

It doesn't have to happen anytime soon, I was just thinking that removing the old, pre-KMS code would make it easier to avoid introducing regressions since we'd have one less config (a bit one atthat) to worry about.

Maybe in 3-4 years.

Quoted 13 lines · Jesse Barnes

Quoted 10 lines · On Sun, 10 Jan 2010 07:32:30 +1000 Dave Airlie <[email protected]> wrote:

I'm in the 2-3 years at a minimum, with at least one kernel with no serious regressions in Intel KMS, which we haven't gotten close to yet. I'm not even sure the Intel guys are taking stable seriously enough yet. So far I don't think there is one kernel release (even stable) that works on all Intel chipsets without backporting patches. 2.6.32 needs the changes to remove the messed up render clock hacks which should really have been reverted a lot earlier since we had a lot of regression reports. The number of users using powersave=0 to get anything approaching useable is growing etc.

Quoted 10 lines

But you could apply that argument to the existing DRM code (not just Intel) as well; lots of things are broken or unimplemented and never get fixed.  I'd say the right metric isn't whether regressions are introduced (usually due to new features) but whether the driver is better than the old userspace code.  For Intel at least, I think we're already there.  The quality of the kernel driver is higher and it has many more features than the userspace implementation ever did.  That's just my subjective opinion, but I've done a *lot* of work on our bugs both in userspace and in the kernel, so I think it's an accurate statement.

The problem is at any single point in time I'm not sure a kms kernel exists that works across all the Intel hw, which from a distro POV is a real pain in the ass, a regression gets fixed on one piece of hw just as another on a different piece gets introduced.

I'd really like if Intel devs could either slow it down and do more testing before pushing to Linus, or be a lot quicker with the reverts when stuff is identified. The main thing is the render reclocking lately, thats been a nightmare and as far as I can see 2.6.32.3 still has all the issues,

Quoted 4 lines

It doesn't have to happen anytime soon, I was just thinking that removing the old, pre-KMS code would make it easier to avoid introducing regressions since we'd have one less config (a bit one atthat) to worry about.

Maybe in 3-4 years.

Quoted 12 lines · Jesse Barnes

Quoted 9 lines · On Sun, 10 Jan 2010 07:32:30 +1000 Dave Airlie <[email protected]> wrote:

I'm in the 2-3 years at a minimum, with at least one kernel with no serious regressions in Intel KMS, which we haven't gotten close to yet. I'm not even sure the Intel guys are taking stable seriously enough yet. So far I don't think there is one kernel release (even stable) that works on all Intel chipsets without backporting patches. 2.6.32 needs the changes to remove the messed up render clock hacks which should really have been reverted a lot earlier since we had a lot of regression reports. The number of users using powersave=0 to get anything approaching useable is growing etc.

Quoted 10 lines

But you could apply that argument to the existing DRM code (not just Intel) as well; lots of things are broken or unimplemented and never get fixed. I'd say the right metric isn't whether regressions are introduced (usually due to new features) but whether the driver is better than the old userspace code. For Intel at least, I think we're already there. The quality of the kernel driver is higher and it has many more features than the userspace implementation ever did. That's just my subjective opinion, but I've done a *lot* of work on our bugs both in userspace and in the kernel, so I think it's an accurate statement.

The problem is at any single point in time I'm not sure a kms kernel exists that works across all the Intel hw, which from a distro POV is a real pain in the ass, a regression gets fixed on one piece of hw just as another on a different piece gets introduced.

I'd really like if Intel devs could either slow it down and do more testing before pushing to Linus, or be a lot quicker with the reverts when stuff is identified. The main thing is the render reclocking lately, thats been a nightmare and as far as I can see 2.6.32.3 still has all the issues,

Hmm, are you trying to say radeon is better at that?

My experience is quite the opposite to be honest.

Quoted 4 lines · Rafael J. Wysocki

Hmm, are you trying to say radeon is better at that?

My experience is quite the opposite to be honest.

radeon kms is in staging, doesn't pretend to be stable and force all users to the experimental paths. So yes, I would say radeon is better at that.

Quoted 9 lines · Julien Cristau

Quoted 4 lines · Rafael J. Wysocki

Hmm, are you trying to say radeon is better at that?

My experience is quite the opposite to be honest.

radeon kms is in staging, doesn't pretend to be stable and force all users to the experimental paths.  So yes, I would say radeon is better at that.

I guess I should have been more precise.

All of my test boxes with ATI/AMD graphics hardware regressed after upgrading from openSUSE 11.1 to openSUSE 11.2, in different ways, because of the user space part of the radeon driver.  Of course, you can argue that the dristro picked up particularly bad release of the driver, but from the user's point of view it actually doesn't matter whether the breakage is in the kernel part or in the user space part of the driver.  The difference is, however, that the breakage in the kernel is fixed _way_ faster than the breakage in the user space, so I very much prefer the Intel people pushing new features aggressively and fixing bugs related to that, then the situation where I need to deal with the broken user space driver, while the KMS radeon is still not reliable enough.

IOW, if your user space driver worked 100% of the time, I'd totally agree, but that's not the case, at least as far as I see it.

Quoted 25 lines · Rafael J. Wysocki

Quoted 9 lines · Julien Cristau

Quoted 4 lines · Rafael J. Wysocki

Hmm, are you trying to say radeon is better at that?

My experience is quite the opposite to be honest.

radeon kms is in staging, doesn't pretend to be stable and force all users to the experimental paths. So yes, I would say radeon is better at that.

I guess I should have been more precise.

All of my test boxes with ATI/AMD graphics hardware regressed after upgrading from openSUSE 11.1 to openSUSE 11.2, in different ways, because of the user space part of the radeon driver. Of course, you can argue that the dristro picked up particularly bad release of the driver, but from the user's point of view it actually doesn't matter whether the breakage is in the kernel part or in the user space part of the driver. The difference is, however, that the breakage in the kernel is fixed _way_ faster than the breakage in the user space, so I very much prefer the Intel people pushing new features aggressively and fixing bugs related to that, then the situation where I need to deal with the broken user space driver, while the KMS radeon is still not reliable enough.

Quoted 3 lines

IOW, if your user space driver worked 100% of the time, I'd totally agree, but that's not the case, at least as far as I see it.

Are you using the Novell radeonhd driver? (I think SuSE default to this for all cards > r500).

This isn't the driver that is developed by the opensource community and really your distro is where you complain about that sort of regression.

The wierd thing is we see distro picking up fixes for userspace drivers *much* quicker
if their teams are the on the ball since they are only a small component to upgrade,
with the kernel you find most distro fire and forget, so if 2.6.31 doesn't work on your
hw you'll wait 6 months to find out that 2.634 doesn't work either.

Quoted 25 lines · Rafael J. Wysocki

Quoted 9 lines · Julien Cristau

Quoted 4 lines · Rafael J. Wysocki

Hmm, are you trying to say radeon is better at that?

My experience is quite the opposite to be honest.

radeon kms is in staging, doesn't pretend to be stable and force all users to the experimental paths. So yes, I would say radeon is better at that.

I guess I should have been more precise.

All of my test boxes with ATI/AMD graphics hardware regressed after upgrading from openSUSE 11.1 to openSUSE 11.2, in different ways, because of the user space part of the radeon driver. Of course, you can argue that the dristro picked up particularly bad release of the driver, but from the user's point of view it actually doesn't matter whether the breakage is in the kernel part or in the user space part of the driver. The difference is, however, that the breakage in the kernel is fixed _way_ faster than the breakage in the user space, so I very much prefer the Intel people pushing new features aggressively and fixing bugs related to that, then the situation where I need to deal with the broken user space driver, while the KMS radeon is still not reliable enough.

Quoted 3 lines

IOW, if your user space driver worked 100% of the time, I'd totally agree, but that's not the case, at least as far as I see it.

Are you using the Novell radeonhd driver? (I think SuSE default to this for all cards > r500).

This isn't the driver that is developed by the opensource community and really your distro is where you complain about that sort of regression.

The wierd thing is we see distro picking up fixes for userspace drivers *much* quicker
if their teams are the on the ball since they are only a small component to upgrade,
with the kernel you find most distro fire and forget, so if 2.6.31 doesn't work on your
hw you'll wait 6 months to find out that 2.634 doesn't work either.

Quoted 9 lines · Julien Cristau

Quoted 4 lines · Rafael J. Wysocki

Hmm, are you trying to say radeon is better at that?

My experience is quite the opposite to be honest.

radeon kms is in staging, doesn't pretend to be stable and force all users to the experimental paths.  So yes, I would say radeon is better at that.

I guess I should have been more precise.

All of my test boxes with ATI/AMD graphics hardware regressed after upgrading from openSUSE 11.1 to openSUSE 11.2, in different ways, because of the user space part of the radeon driver.  Of course, you can argue that the dristro picked up particularly bad release of the driver, but from the user's point of view it actually doesn't matter whether the breakage is in the kernel part or in the user space part of the driver.  The difference is, however, that the breakage in the kernel is fixed _way_ faster than the breakage in the user space, so I very much prefer the Intel people pushing new features aggressively and fixing bugs related to that, then the situation where I need to deal with the broken user space driver, while the KMS radeon is still not reliable enough.

IOW, if your user space driver worked 100% of the time, I'd totally agree, but that's not the case, at least as far as I see it.

Quoted 4 lines · Rafael J. Wysocki

Hmm, are you trying to say radeon is better at that?

My experience is quite the opposite to be honest.

radeon kms is in staging, doesn't pretend to be stable and force all users to the experimental paths. So yes, I would say radeon is better at that.

Hmm, are you trying to say radeon is better at that?

My experience is quite the opposite to be honest.

Quoted 12 lines · Jesse Barnes

Quoted 9 lines · On Sun, 10 Jan 2010 07:32:30 +1000 Dave Airlie <[email protected]> wrote:

I'm in the 2-3 years at a minimum, with at least one kernel with no serious regressions in Intel KMS, which we haven't gotten close to yet. I'm not even sure the Intel guys are taking stable seriously enough yet. So far I don't think there is one kernel release (even stable) that works on all Intel chipsets without backporting patches. 2.6.32 needs the changes to remove the messed up render clock hacks which should really have been reverted a lot earlier since we had a lot of regression reports. The number of users using powersave=0 to get anything approaching useable is growing etc.

Quoted 10 lines

But you could apply that argument to the existing DRM code (not just Intel) as well; lots of things are broken or unimplemented and never get fixed. I'd say the right metric isn't whether regressions are introduced (usually due to new features) but whether the driver is better than the old userspace code. For Intel at least, I think we're already there. The quality of the kernel driver is higher and it has many more features than the userspace implementation ever did. That's just my subjective opinion, but I've done a *lot* of work on our bugs both in userspace and in the kernel, so I think it's an accurate statement.

The problem is at any single point in time I'm not sure a kms kernel exists that works across all the Intel hw, which from a distro POV is a real pain in the ass, a regression gets fixed on one piece of hw just as another on a different piece gets introduced.

I'd really like if Intel devs could either slow it down and do more testing before pushing to Linus, or be a lot quicker with the reverts when stuff is identified. The main thing is the render reclocking lately, thats been a nightmare and as far as I can see 2.6.32.3 still has all the issues,

Quoted 9 lines · On Sun, 10 Jan 2010 07:32:30 +1000 Dave Airlie <[email protected]> wrote:

I'm in the 2-3 years at a minimum, with at least one kernel with no serious regressions in Intel KMS, which we haven't gotten close to yet. I'm not even sure the Intel guys are taking stable seriously enough yet. So far I don't think there is one kernel release (even stable) that works on all Intel chipsets without backporting patches. 2.6.32 needs the changes to remove the messed up render clock hacks which should really have been reverted a lot earlier since we had a lot of regression reports. The number of users using powersave=0 to get anything approaching useable is growing etc.

But you could apply that argument to the existing DRM code (not just Intel) as well; lots of things are broken or unimplemented and never get fixed.  I'd say the right metric isn't whether regressions are introduced (usually due to new features) but whether the driver is better than the old userspace code.  For Intel at least, I think we're already there.  The quality of the kernel driver is higher and it has many more features than the userspace implementation ever did.  That's just my subjective opinion, but I've done a *lot* of work on our bugs both in userspace and in the kernel, so I think it's an accurate statement.

It doesn't have to happen anytime soon, I was just thinking that removing the old, pre-KMS code would make it easier to avoid introducing regressions since we'd have one less config (a bit one atthat) to worry about.

I'm in the 2-3 years at a minimum, with at least one kernel with no serious regressions in Intel KMS, which we haven't gotten close to yet. I'm not even sure the Intel guys are taking stable seriously enough yet. So far I don't think there is one kernel release (even stable) that works on all Intel chipsets without backporting patches. 2.6.32 needs the changes to remove the messed up render clock hacks which should really have been reverted a lot earlier since we had a lot of regression reports. The number of users using powersave=0 to get anything approaching useable is growing etc.

We do have ppl who run latest kernels on RHEL5 userspace and I'd rather not have that break badly, I'm guessing more than 3D will break if we remove this, since we need the DRM to allocate memory for 2D stuff, and will probably find the fallback to AGP is broken. Again Intel ppl would have to do a lot of testing on the fallback before removing anything, which is time I don't see anyone willing to spend.

Quoted 11 lines · Jerome Glisse

Quoted 8 lines · Jesse Barnes

Linus, can we ever drop those old paths?  Maybe after the new bits have been around for awhile?  Users of really old userspace stacks would lose 3D support, but they'd still have 2D, so it wouldn't be a complete break.  The non-KMS paths sometimes break like this anyway without us noticing (especially some of the weirder 3D paths)...

Just thinking out loud, we could really kill a lot of really bad code...

I among those who would love such things to happen :)

I don't want to drop it _yet_, but "ever"? Sure. When people are sure that KMS actually handles all the cases that old X does (maybe that's true now), and we've had more than just a couple of kernel releases of _stable_ Intel KMS, I suspect we can start thinking about "ok, nobody seriously uses 3D on Intel integrated graphics _and_ updates the kernel".

The fact that they'd still have a working X setup would make it generally much more palatable, I think.

But we definitely need more than just a couple of kernel releases. So we're talking timescales of "more than a year of stable code". Whether that is "six months from now" or "two years from now", I can't judge.

And people can try to convince me to be more or less aggressive about it, so take the above as a more of a personal opinion that is open to change than anything definite and final.

Quoted 28 lines · On Sat, 9 Jan 2010 02:15:41 +0000 (GMT) Dave Airlie <[email protected]> wrote:

Quoted 28 lines

Quoted 9 lines

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

Quoted 8 lines

Quoted 3 lines

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

This was mainly due to pre-existing fb drivers binding to the device, and the drm drivers having to work around that, with KMS since we have fb in the drm driver its correct to bind, pre-kms its just a mess I'd rather stay away from.

Linus, can we ever drop those old paths? Maybe after the new bits have been around for awhile? Users of really old userspace stacks would lose 3D support, but they'd still have 2D, so it wouldn't be a complete break. The non-KMS paths sometimes break like this anyway without us noticing (especially some of the weirder 3D paths)...

Just thinking out loud, we could really kill a lot of really bad code...

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Quoted 28 lines

Quoted 9 lines

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

Quoted 3 lines

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

This was mainly due to pre-existing fb drivers binding to the device, and the drm drivers having to work around that, with KMS since we have fb in the drm driver its correct to bind, pre-kms its just a mess I'd rather stay away from.

Quoted 8 lines

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Hmm. I get the feeling that perhaps the of the drm_driver callbacks was very muchintentional, and that the code presumably wants to be called purely through the PCI layer, and not through the "drm class" logic at all?

Your patch seems like it would always execute the silly class suspend even though we explicitly don't want to. And a much nicer fix would seem to register the thing properly as a PCI driver even if you don't then use KMS.

So it looks to me like the problem is that drm_init() will register the driver as a real PCI driver only if

	driver->driver_features & DRIVER_MODESET

and otherwise it does that very odd "stealth mode manual scanning" thing which doesn't register it as a proper PCI driver.

So could we instead make that "disable KSM" _just_ disable the mode setting part, not disable the "I'm a real driver" part?

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Signed-off-by Rafael J. Wysocki <[email protected]>

Quoted 7 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Quoted 3 lines

Fix this by reverting that part of commit cbda12d77ea59.

Signed-off-by Rafael J. Wysocki <[email protected]>

Applied, with a little comment for the next poor person having to figure out the difference between these two suspend/resume paths.

Quoted 7 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Quoted 3 lines

Fix this by reverting that part of commit cbda12d77ea59.

Signed-off-by Rafael J. Wysocki <[email protected]>

Applied, with a little comment for the next poor person having to figure out the difference between these two suspend/resume paths.

From: Rafael J. Wysocki <[email protected]>

Commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915: implement new pm ops for i915), among other things, removed the .suspend and .resume pointers from the struct drm_driver object in i915_drv.c, which broke resume without KMS on my MSI Wind U100.

Fix this by reverting that part of commit cbda12d77ea59.

Signed-off-by Rafael J. Wysocki <[email protected]>