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]>
---
drivers/gpu/drm/i915/i915_drv.c | 53 ++++------------------------------------
1 file changed, 6 insertions(+), 47 deletions(-)
Index: linux-2.6/drivers/gpu/drm/i915/i915_drv.c
===================================================================
--- linux-2.6.orig/drivers/gpu/drm/i915/i915_drv.c
+++ linux-2.6/drivers/gpu/drm/i915/i915_drv.c
@@ -464,6 +418,8 @@ static struct drm_driver driver = {
.lastclose = i915_driver_lastclose,
.preclose = i915_driver_preclose,
.postclose = i915_driver_postclose,
+ .suspend = i915_suspend,
+ .resume = i915_resume,
.device_is_agp = i915_driver_device_is_agp,
.enable_vblank = i915_enable_vblank,
.disable_vblank = i915_disable_vblank,
On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
>
> 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?
Linus
On Fri, 8 Jan 2010 16:01:46 -0800 (PST)
Linus Torvalds <[email protected]> wrote:
>
>
> On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> >
> > 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?
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.
--
Jesse Barnes, Intel Open Source Technology Center
On Fri, 8 Jan 2010 16:06:59 -0800
Jesse Barnes <[email protected]> wrote:
> On Fri, 8 Jan 2010 16:01:46 -0800 (PST)
> Linus Torvalds <[email protected]> wrote:
>
> >
> >
> > On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> > >
> > > 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?
>
> 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?
--
Jesse Barnes, Intel Open Source Technology Center
On Saturday 09 January 2010, Linus Torvalds wrote:
>
> On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> >
> > 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?
Hmm. Do you mean set DRIVER_MODESET unconditionally before calling drm_init()
and only reset it later if necessary?
Rafael
On Fri, 8 Jan 2010 16:01:46 -0800 (PST)
Linus Torvalds <[email protected]> wrote:
>
>
> On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> >
> > 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?
This is the minimal fix I think (totally untested):
diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c index a0a2cad..1364c3e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -541,6 +541,11 @@ static int __init i915_init(void)
driver.driver_features &= ~DRIVER_MODESET;
#endif
+ if (!(driver.driver_features & DRIVER_MODESET)) {
+ driver.suspend = i915_suspend;
+ driver.resume = i915_resume;
+ }
+
return drm_init(&driver);
}
--
Jesse Barnes, Intel Open Source Technology Center
On Saturday 09 January 2010, Jesse Barnes wrote:
> On Fri, 8 Jan 2010 16:06:59 -0800
> Jesse Barnes <[email protected]> wrote:
>
> > On Fri, 8 Jan 2010 16:01:46 -0800 (PST)
> > Linus Torvalds <[email protected]> wrote:
> >
> > >
> > >
> > > On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> > > >
> > > > 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?
> >
> > 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?
No, with modeset=0 it doesn't register the PCI driver as well.
Rafael
On Saturday 09 January 2010, Jesse Barnes wrote:
> On Fri, 8 Jan 2010 16:01:46 -0800 (PST)
> Linus Torvalds <[email protected]> wrote:
>
> >
> >
> > On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> > >
> > > 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?
>
> This is the minimal fix I think (totally untested):
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c index a0a2cad..1364c3e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -541,6 +541,11 @@ static int __init i915_init(void)
> driver.driver_features &= ~DRIVER_MODESET;
> #endif
>
> + if (!(driver.driver_features & DRIVER_MODESET)) {
> + driver.suspend = i915_suspend;
> + driver.resume = i915_resume;
> + }
> +
> return drm_init(&driver);
> }
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.
Rafael
On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
>
> 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.
Linus
On Fri, 8 Jan 2010 16:50:57 -0800 (PST)
Linus Torvalds <[email protected]> wrote:
>
>
> On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> >
> > 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.
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.
--
Jesse Barnes, Intel Open Source Technology Center
> > 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?
>
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.
Dave.
On Sat, 9 Jan 2010 02:15:41 +0000 (GMT)
Dave Airlie <[email protected]> wrote:
> > > 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?
> >
>
> 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...
--
Jesse Barnes, Intel Open Source Technology Center
On Fri, Jan 08, 2010 at 06:50:41PM -0800, Jesse Barnes wrote:
> On Sat, 9 Jan 2010 02:15:41 +0000 (GMT)
> Dave Airlie <[email protected]> wrote:
>
> > > > 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?
> > >
> >
> > 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...
>
> --
> Jesse Barnes, Intel Open Source Technology Center
I among those who would love such things to happen :)
Cheers,
Jerome
On Saturday 09 January 2010, Jesse Barnes wrote:
> On Fri, 8 Jan 2010 16:50:57 -0800 (PST)
> Linus Torvalds <[email protected]> wrote:
>
> >
> >
> > On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> > >
> > > 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.
>
> 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.
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: DRM / i915: Always register as a PCI driver
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.
The reason of the breakage is that in the non-KMS case i915 is
not registered as a PCI driver and uses device class suspend and
resume callbacks that, in turn, execute the callbacks defined in the
struct drm_driver object. Consequently, after commit cbda12d77ea59,
the driver's suspend and resume callbacks are not executed at all in
the non-KMS case.
To prevent this from happening, set DRIVER_MODESET unconditinally
in driver.driver_features before calling drm_init(), so that it
always registeres us as a PCI driver, and make i915_pci_probe()
reset DRIVER_MODESET in the non-KMS case. This way the driver's PCI
suspend and resume callbacks will always be used and the problem
will be avoided.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/gpu/drm/drm_stub.c | 3 +-
drivers/gpu/drm/i915/i915_drv.c | 44 +++++++++++++++++++++-------------------
2 files changed, 26 insertions(+), 21 deletions(-)
Index: linux-2.6/drivers/gpu/drm/i915/i915_drv.c
===================================================================
--- linux-2.6.orig/drivers/gpu/drm/i915/i915_drv.c
+++ linux-2.6/drivers/gpu/drm/i915/i915_drv.c
@@ -372,6 +372,28 @@ int i965_reset(struct drm_device *dev, u
static int __devinit
i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
+ /*
+ * If CONFIG_DRM_I915_KMS is set, default to KMS unless
+ * explicitly disabled with the module pararmeter.
+ *
+ * Otherwise, just follow the parameter (defaulting to off).
+ *
+ * Allow optional vga_text_mode_force boot option to override
+ * the default behavior.
+ */
+ if (i915_modeset == 0)
+ driver.driver_features &= ~DRIVER_MODESET;
+
+#ifndef CONFIG_DRM_I915_KMS
+ if (i915_modeset != 1)
+ driver.driver_features &= ~DRIVER_MODESET;
+#endif
+
+#ifdef CONFIG_VGA_CONSOLE
+ if (vgacon_text_force() && i915_modeset == -1)
+ driver.driver_features &= ~DRIVER_MODESET;
+#endif
+
return drm_get_dev(pdev, ent, &driver);
}
@@ -520,26 +542,8 @@ static int __init i915_init(void)
i915_gem_shrinker_init();
- /*
- * If CONFIG_DRM_I915_KMS is set, default to KMS unless
- * explicitly disabled with the module pararmeter.
- *
- * Otherwise, just follow the parameter (defaulting to off).
- *
- * Allow optional vga_text_mode_force boot option to override
- * the default behavior.
- */
-#if defined(CONFIG_DRM_I915_KMS)
- if (i915_modeset != 0)
- driver.driver_features |= DRIVER_MODESET;
-#endif
- if (i915_modeset == 1)
- driver.driver_features |= DRIVER_MODESET;
-
-#ifdef CONFIG_VGA_CONSOLE
- if (vgacon_text_force() && i915_modeset == -1)
- driver.driver_features &= ~DRIVER_MODESET;
-#endif
+ /* Make drm_init() always register us as a PCI driver. */
+ driver.driver_features |= DRIVER_MODESET;
return drm_init(&driver);
}
Index: linux-2.6/drivers/gpu/drm/drm_stub.c
===================================================================
--- linux-2.6.orig/drivers/gpu/drm/drm_stub.c
+++ linux-2.6/drivers/gpu/drm/drm_stub.c
@@ -419,8 +419,9 @@ int drm_get_dev(struct pci_dev *pdev, co
goto err_g2;
}
+ pci_set_drvdata(pdev, dev);
+
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
- pci_set_drvdata(pdev, dev);
ret = drm_get_minor(dev, &dev->control, DRM_MINOR_CONTROL);
if (ret)
goto err_g2;
On Sat, 9 Jan 2010, Jerome Glisse wrote:
> On Fri, Jan 08, 2010 at 06:50:41PM -0800, Jesse Barnes wrote:
> >
> > 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.
Linus
On Sun, Jan 10, 2010 at 4:17 AM, Linus Torvalds
<[email protected]> wrote:
>
>
> On Sat, 9 Jan 2010, Jerome Glisse wrote:
>
>> On Fri, Jan 08, 2010 at 06:50:41PM -0800, Jesse Barnes wrote:
>> >
>> > 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.
Dave.
Dave.
On Sat, Jan 9, 2010 at 11:35 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Saturday 09 January 2010, Jesse Barnes wrote:
>> On Fri, 8 Jan 2010 16:50:57 -0800 (PST)
>> Linus Torvalds <[email protected]> wrote:
>>
>> >
>> >
>> > On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
>> > >
>> > > 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.
>>
>> 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.
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.
Dave.
On Saturday 09 January 2010, Dave Airlie wrote:
> On Sat, Jan 9, 2010 at 11:35 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Saturday 09 January 2010, Jesse Barnes wrote:
> >> On Fri, 8 Jan 2010 16:50:57 -0800 (PST)
> >> Linus Torvalds <[email protected]> wrote:
> >>
> >> >
> >> >
> >> > On Sat, 9 Jan 2010, Rafael J. Wysocki wrote:
> >> > >
> >> > > 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.
> >>
> >> 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.
>
> 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.
OK, so my original patch is the right one in that case.
Rafael
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.
> 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.
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.
--
Jesse Barnes, Intel Open Source Technology Center
On Tue, Jan 12, 2010 at 2:38 AM, Jesse Barnes <[email protected]> wrote:
> 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.
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,
>
> 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.
Dave.
On Tue, 12 Jan 2010 06:12:37 +1000
Dave Airlie <[email protected]> wrote:
> On Tue, Jan 12, 2010 at 2:38 AM, Jesse Barnes
> <[email protected]> wrote:
> > 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.
>
> 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,
Yeah, it may have been better to just revert that early on, but some
users really wanted the power saving features too, so it wasn't totally
clear cut (btw stable has a revert patch queued up now that fixes things
for several people).
> > 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.
Ouch, it just went from 2-3 to 3-4. But really the other drm drivers
have to get converted anyway before we can really start killing code.
--
Jesse Barnes, Intel Open Source Technology Center
On Monday 11 January 2010, Dave Airlie wrote:
> On Tue, Jan 12, 2010 at 2:38 AM, Jesse Barnes <[email protected]> wrote:
> > 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.
>
> 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.
Rafael
On Mon, Jan 11, 2010 at 22:04:36 +0100, Rafael J. Wysocki wrote:
> 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.
Cheers,
Julien
On Monday 11 January 2010, Julien Cristau wrote:
> On Mon, Jan 11, 2010 at 22:04:36 +0100, Rafael J. Wysocki wrote:
>
> > 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.
Rafael
On Tue, Jan 12, 2010 at 8:22 AM, Rafael J. Wysocki <[email protected]> wrote:
> On Monday 11 January 2010, Julien Cristau wrote:
>> On Mon, Jan 11, 2010 at 22:04:36 +0100, Rafael J. Wysocki wrote:
>>
>> > 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.
>
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.
Since ppl aren't targetting stable properly distros are left to fend
for themselves
when it comes to backporting large amounts of changes, and you find most
distros just don't bother.
Dave.
On Tuesday 12 January 2010, Dave Airlie wrote:
> On Tue, Jan 12, 2010 at 8:22 AM, Rafael J. Wysocki <[email protected]> wrote:
> > On Monday 11 January 2010, Julien Cristau wrote:
> >> On Mon, Jan 11, 2010 at 22:04:36 +0100, Rafael J. Wysocki wrote:
> >>
> >> > 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.
> >
>
> Are you using the Novell radeonhd driver? (I think SuSE default to this for all
> cards > r500).
No I'm not.
> 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.
I know, I'm not using it.
> 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.
Well, openSUSE upgrades the kernel to -stable quite timely. Which is not the
case with the Xorg drivers, so apparently it depends which distro you
are on. :-)
Rafael
On Tuesday 12 January 2010, Eric Anholt wrote:
> On Sat, 9 Jan 2010 00:45:33 +0100, "Rafael J. Wysocki" <[email protected]> wrote:
> > 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]>
>
> Applied, with a little comment for the next poor person having to figure
> out the difference between these two suspend/resume paths.
Linus has applied this one already.
Rafael