| Message ID | 200912091402.47945.alberto.milone@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Stefan Bader |
| Headers | show |
Alberto Milone wrote: > Hi all, > > SRU Justification: > > The problem described in this email was reported in a private OEM bug report > for Dell and has been solved by upstream. It's a regression in the drm code > which causes a massive system slowdown if we turn off the VGA output. > > The patch is sane and minimal and is available in the drm-intel branch and in > the linux-next branch (see the link to the commit in the bug report). > > I have applied (and slightly adapted, as it didn't apply cleanly to our lucid > and karmic git branches) and tested the patch successfully in Karmic. > > The integration of this patch is very important for our Dell projects and for > the Ubuntu desktop at large. > > Please include the attached patch in both Karmic and Lucid ASAP. > > Bug #494461 > > Thanks in advance for your time. > > Regards, > > This should actually have gone to stable as well as it fixes a regression. Also the description sounds like the integration path should be stable as well (as long as that is open). The patch itself looks like it changes semantics of an exported function. Before additional calls to drm_vblank_get() would return ok and increment the ref count. After the change, additional calls will return an error and not increment the count. On the good side it seems that function is only used within drm_irq.c and the usage seems to be ok to handle that change. Currently not 100% convinced to ack, but might get convinced. -Stefan -Stefan
On 12/09/2009 07:52 AM, Stefan Bader wrote: > Alberto Milone wrote: >> Hi all, >> >> SRU Justification: >> >> The problem described in this email was reported in a private OEM bug report >> for Dell and has been solved by upstream. It's a regression in the drm code >> which causes a massive system slowdown if we turn off the VGA output. >> >> The patch is sane and minimal and is available in the drm-intel branch and in >> the linux-next branch (see the link to the commit in the bug report). >> >> I have applied (and slightly adapted, as it didn't apply cleanly to our lucid >> and karmic git branches) and tested the patch successfully in Karmic. >> >> The integration of this patch is very important for our Dell projects and for >> the Ubuntu desktop at large. >> >> Please include the attached patch in both Karmic and Lucid ASAP. >> >> Bug #494461 >> >> Thanks in advance for your time. >> >> Regards, >> >> > This should actually have gone to stable as well as it fixes a regression. Also > the description sounds like the integration path should be stable as well (as > long as that is open). > The patch itself looks like it changes semantics of an exported function. Before > additional calls to drm_vblank_get() would return ok and increment the ref count. > After the change, additional calls will return an error and not increment the > count. On the good side it seems that function is only used within drm_irq.c and > the usage seems to be ok to handle that change. > Currently not 100% convinced to ack, but might get convinced. > > -Stefan > > -Stefan > This patch is more complete version of a patch that's in moblin, and is one of the patches we're evaluating for possible decreased kernel boot time. Based on having been fielded in moblin, and on usage as Stefan described above, Ack Steve
On Wednesday 09 Dec 2009 14:52:41 you wrote: > Alberto Milone wrote: > > Hi all, > > > > SRU Justification: > > > > The problem described in this email was reported in a private OEM bug > > report for Dell and has been solved by upstream. It's a regression in the > > drm code which causes a massive system slowdown if we turn off the VGA > > output. > > > > The patch is sane and minimal and is available in the drm-intel branch > > and in the linux-next branch (see the link to the commit in the bug > > report). > > > > I have applied (and slightly adapted, as it didn't apply cleanly to our > > lucid and karmic git branches) and tested the patch successfully in > > Karmic. > > > > The integration of this patch is very important for our Dell projects and > > for the Ubuntu desktop at large. > > > > Please include the attached patch in both Karmic and Lucid ASAP. > > > > Bug #494461 > > > > Thanks in advance for your time. > > > > Regards, > > This should actually have gone to stable as well as it fixes a regression. > Also the description sounds like the integration path should be stable as > well (as long as that is open). > The patch itself looks like it changes semantics of an exported function. > Before additional calls to drm_vblank_get() would return ok and increment > the ref count. After the change, additional calls will return an error and > not increment the count. On the good side it seems that function is only > used within drm_irq.c and the usage seems to be ok to handle that change. > Currently not 100% convinced to ack, but might get convinced. > > -Stefan > > -Stefan > Hi Stefan, I discussed this with Jesse (who reviewed the patch). It's ok to return an error in drm_vblank_get(). Think of the case in which a client (a direct rendered 3d app such as compiz or a game) tries to wait on a pipe that is off (e.g. due to dpms), the client would do so, but if the pipe went off after a client waited, the client might never wake up. Furthermore if a pipe went off we might not catch it for a while. For this reason it's ok to return the error to the client. Furthermore the patch seems to be in the stable branch too: http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6- stable.git;a=commit;h=778c902640530371a169ad1c03566e7c51b09874 I hope both facts (especially the latter) can fully convince you, and if they don't, I'll try harder ;) Regards,
On Wed, Dec 09, 2009 at 02:02:47PM +0100, Alberto Milone wrote: > Hi all, > > SRU Justification: > > The problem described in this email was reported in a private OEM bug report > for Dell and has been solved by upstream. It's a regression in the drm code > which causes a massive system slowdown if we turn off the VGA output. > > The patch is sane and minimal and is available in the drm-intel branch and in > the linux-next branch (see the link to the commit in the bug report). > > I have applied (and slightly adapted, as it didn't apply cleanly to our lucid > and karmic git branches) and tested the patch successfully in Karmic. > > The integration of this patch is very important for our Dell projects and for > the Ubuntu desktop at large. > > Please include the attached patch in both Karmic and Lucid ASAP. > > Bug #494461 > > Thanks in advance for your time. > > Regards, > > -- > Alberto Milone > Sustaining Engineer (system) > Foundations Team > Canonical OEM Services > From 778c902640530371a169ad1c03566e7c51b09874 Mon Sep 17 00:00:00 2001 > From: Li Peng <peng.li@linux.intel.com> > Date: Mon, 9 Nov 2009 12:51:22 +0800 > Subject: [PATCH 1/1] drm/i915: Fix sync to vblank when VGA output is turned off > > In current vblank-wait implementation, if we turn off VGA output, > drm_wait_vblank will still wait on the disabled pipe until timeout, > because vblank on the pipe is assumed be enabled. This would cause > slow system response on some system such as moblin. > > This patch resolve the issue by adding a drm helper function > drm_vblank_off which explicitly clear vblank_enabled[crtc], wake up > any waiting queue and save last vblank counter before turning off > crtc. It also slightly change drm_vblank_get to ensure that we will > will return immediately if trying to wait on a disabled pipe. > > Signed-off-by: Li Peng <peng.li@intel.com> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> > [anholt: hand-applied for conflicts with overlay changes] > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index f85aaf2..f298434 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -402,15 +402,21 @@ int drm_vblank_get(struct drm_device *dev, int crtc) > > spin_lock_irqsave(&dev->vbl_lock, irqflags); > /* Going from 0->1 means we have to enable interrupts again */ > - if (atomic_add_return(1, &dev->vblank_refcount[crtc]) == 1 && > - !dev->vblank_enabled[crtc]) { > - ret = dev->driver->enable_vblank(dev, crtc); > - DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret); > - if (ret) > + if (atomic_add_return(1, &dev->vblank_refcount[crtc]) == 1) { > + if (!dev->vblank_enabled[crtc]) { > + ret = dev->driver->enable_vblank(dev, crtc); > + DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret); > + if (ret) > + atomic_dec(&dev->vblank_refcount[crtc]); > + else { > + dev->vblank_enabled[crtc] = 1; > + drm_update_vblank_count(dev, crtc); > + } > + } > + } else { > + if (!dev->vblank_enabled[crtc]) { > atomic_dec(&dev->vblank_refcount[crtc]); > - else { > - dev->vblank_enabled[crtc] = 1; > - drm_update_vblank_count(dev, crtc); > + ret = -EINVAL; > } > } > spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > @@ -437,6 +443,18 @@ void drm_vblank_put(struct drm_device *dev, int crtc) > } > EXPORT_SYMBOL(drm_vblank_put); > > +void drm_vblank_off(struct drm_device *dev, int crtc) > +{ > + unsigned long irqflags; > + > + spin_lock_irqsave(&dev->vbl_lock, irqflags); > + DRM_WAKEUP(&dev->vbl_queue[crtc]); > + dev->vblank_enabled[crtc] = 0; > + dev->last_vblank[crtc] = dev->driver->get_vblank_counter(dev, crtc); > + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > +} > +EXPORT_SYMBOL(drm_vblank_off); > + > /** > * drm_vblank_pre_modeset - account for vblanks across mode sets > * @dev: DRM device > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 318ba47..652e9ac 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1540,6 +1540,7 @@ static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode) > intel_update_watermarks(dev); > /* Give the overlay scaler a chance to disable if it's on this pipe */ > //intel_crtc_dpms_video(crtc, FALSE); TODO > + drm_vblank_off(dev, pipe); > > /* Disable the VGA plane that we never use */ > i915_disable_vga(dev); > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 45b67d9..4637dce 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1268,6 +1268,7 @@ extern u32 drm_vblank_count(struct drm_device *dev, int crtc); > extern void drm_handle_vblank(struct drm_device *dev, int crtc); > extern int drm_vblank_get(struct drm_device *dev, int crtc); > extern void drm_vblank_put(struct drm_device *dev, int crtc); > +extern void drm_vblank_off(struct drm_device *dev, int crtc); > extern void drm_vblank_cleanup(struct drm_device *dev); > /* Modesetting support */ > extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc); Although the patch is bit bit looking the change seems reasonable to me and its testable. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
Alberto Milone wrote: > Hi all, > > SRU Justification: > > The problem described in this email was reported in a private OEM bug report > for Dell and has been solved by upstream. It's a regression in the drm code > which causes a massive system slowdown if we turn off the VGA output. > > The patch is sane and minimal and is available in the drm-intel branch and in > the linux-next branch (see the link to the commit in the bug report). > > I have applied (and slightly adapted, as it didn't apply cleanly to our lucid > and karmic git branches) and tested the patch successfully in Karmic. > > The integration of this patch is very important for our Dell projects and for > the Ubuntu desktop at large. > > Please include the attached patch in both Karmic and Lucid ASAP. > > Bug #494461 > > Thanks in advance for your time. > > Regards, > > Ok, I just submitted a request to put that into stable and would apply it prematurely to our Karmic tree. Just one thing: Alberto, can you please create a public bug report with the necessary non private information in it to understand why this is needed? Thanks -Stefan
Alberto Milone wrote: > On Wednesday 09 Dec 2009 14:52:41 you wrote: >> Alberto Milone wrote: >>> Hi all, >>> >>> SRU Justification: >>> >>> The problem described in this email was reported in a private OEM bug >>> report for Dell and has been solved by upstream. It's a regression in the >>> drm code which causes a massive system slowdown if we turn off the VGA >>> output. >>> >>> The patch is sane and minimal and is available in the drm-intel branch >>> and in the linux-next branch (see the link to the commit in the bug >>> report). >>> >>> I have applied (and slightly adapted, as it didn't apply cleanly to our >>> lucid and karmic git branches) and tested the patch successfully in >>> Karmic. >>> >>> The integration of this patch is very important for our Dell projects and >>> for the Ubuntu desktop at large. >>> >>> Please include the attached patch in both Karmic and Lucid ASAP. >>> >>> Bug #494461 >>> >>> Thanks in advance for your time. >>> >>> Regards, >> This should actually have gone to stable as well as it fixes a regression. >> Also the description sounds like the integration path should be stable as >> well (as long as that is open). >> The patch itself looks like it changes semantics of an exported function. >> Before additional calls to drm_vblank_get() would return ok and increment >> the ref count. After the change, additional calls will return an error and >> not increment the count. On the good side it seems that function is only >> used within drm_irq.c and the usage seems to be ok to handle that change. >> Currently not 100% convinced to ack, but might get convinced. >> >> -Stefan >> >> -Stefan >> > > Hi Stefan, > > I discussed this with Jesse (who reviewed the patch). It's ok to return an > error in drm_vblank_get(). > > Think of the case in which a client (a direct rendered 3d app such as compiz > or a game) tries to wait on a pipe that is off (e.g. due to dpms), the client > would do so, but if the pipe went off after a client waited, the client might > never wake up. Furthermore if a pipe went off we might not catch it for a > while. For this reason it's ok to return the error to the client. > > Furthermore the patch seems to be in the stable branch too: > http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6- > stable.git;a=commit;h=778c902640530371a169ad1c03566e7c51b09874 Just to note, this is the unified stable tree, which is not stable. I believe it has Linus tree as master and stable branches which, as I was told, get automatically created from the real stable trees. -Stefan > I hope both facts (especially the latter) can fully convince you, and if they > don't, I'll try harder ;) > > Regards, >
On Wednesday 16 Dec 2009 16:40:13 you wrote: > Alberto Milone wrote: > > Hi all, > > > > SRU Justification: > > > > The problem described in this email was reported in a private OEM bug > > report for Dell and has been solved by upstream. It's a regression in the > > drm code which causes a massive system slowdown if we turn off the VGA > > output. > > > > The patch is sane and minimal and is available in the drm-intel branch > > and in the linux-next branch (see the link to the commit in the bug > > report). > > > > I have applied (and slightly adapted, as it didn't apply cleanly to our > > lucid and karmic git branches) and tested the patch successfully in > > Karmic. > > > > The integration of this patch is very important for our Dell projects and > > for the Ubuntu desktop at large. > > > > Please include the attached patch in both Karmic and Lucid ASAP. > > > > Bug #494461 > > > > Thanks in advance for your time. > > > > Regards, > > Ok, I just submitted a request to put that into stable and would apply > it prematurely to our Karmic tree. Just one thing: Alberto, can you > please create a public bug report with the necessary non private > information in it to understand why this is needed? Thanks > > -Stefan > Hi Stefan, First of all, thanks for your work. Bug #439778 is private while bug #494461 is public. If the latter lacks the information you need, I'm available to add further details there. Let me know. Regards,
On Wednesday 16 Dec 2009 16:42:00 you wrote: > Alberto Milone wrote: > > On Wednesday 09 Dec 2009 14:52:41 you wrote: > >> Alberto Milone wrote: > >>> Hi all, > >>> > >>> SRU Justification: > >>> > >>> The problem described in this email was reported in a private OEM bug > >>> report for Dell and has been solved by upstream. It's a regression in > >>> the drm code which causes a massive system slowdown if we turn off the > >>> VGA output. > >>> > >>> The patch is sane and minimal and is available in the drm-intel branch > >>> and in the linux-next branch (see the link to the commit in the bug > >>> report). > >>> > >>> I have applied (and slightly adapted, as it didn't apply cleanly to our > >>> lucid and karmic git branches) and tested the patch successfully in > >>> Karmic. > >>> > >>> The integration of this patch is very important for our Dell projects > >>> and for the Ubuntu desktop at large. > >>> > >>> Please include the attached patch in both Karmic and Lucid ASAP. > >>> > >>> Bug #494461 > >>> > >>> Thanks in advance for your time. > >>> > >>> Regards, > >> > >> This should actually have gone to stable as well as it fixes a > >> regression. Also the description sounds like the integration path should > >> be stable as well (as long as that is open). > >> The patch itself looks like it changes semantics of an exported > >> function. Before additional calls to drm_vblank_get() would return ok > >> and increment the ref count. After the change, additional calls will > >> return an error and not increment the count. On the good side it seems > >> that function is only used within drm_irq.c and the usage seems to be ok > >> to handle that change. Currently not 100% convinced to ack, but might > >> get convinced. > >> > >> -Stefan > >> > >> -Stefan > > > > Hi Stefan, > > > > I discussed this with Jesse (who reviewed the patch). It's ok to return > > an error in drm_vblank_get(). > > > > Think of the case in which a client (a direct rendered 3d app such as > > compiz or a game) tries to wait on a pipe that is off (e.g. due to dpms), > > the client would do so, but if the pipe went off after a client waited, > > the client might never wake up. Furthermore if a pipe went off we might > > not catch it for a while. For this reason it's ok to return the error to > > the client. > > > > Furthermore the patch seems to be in the stable branch too: > > http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6- > > stable.git;a=commit;h=778c902640530371a169ad1c03566e7c51b09874 > > Just to note, this is the unified stable tree, which is not > stable. I believe it has Linus tree as master and stable branches > which, as I was told, get automatically created from the real > stable trees. > > -Stefan > Ok, thanks for making this clear. Regards,
Alberto Milone wrote: > Hi all, > > SRU Justification: > > The problem described in this email was reported in a private OEM bug report > for Dell and has been solved by upstream. It's a regression in the drm code > which causes a massive system slowdown if we turn off the VGA output. > > The patch is sane and minimal and is available in the drm-intel branch and in > the linux-next branch (see the link to the commit in the bug report). > > I have applied (and slightly adapted, as it didn't apply cleanly to our lucid > and karmic git branches) and tested the patch successfully in Karmic. > > The integration of this patch is very important for our Dell projects and for > the Ubuntu desktop at large. > > Please include the attached patch in both Karmic and Lucid ASAP. > > Bug #494461 > > Thanks in advance for your time. > > Regards, > > Ok, thanks Alberto. I applied the patch now for Karmic. For Lucid it hopefully gets down from stable, but we / you probably should keep an eye on it for before the release. -Stefan
On Wednesday 16 Dec 2009 18:02:25 you wrote: > Alberto Milone wrote: > > Hi all, > > > > SRU Justification: > > > > The problem described in this email was reported in a private OEM bug > > report for Dell and has been solved by upstream. It's a regression in the > > drm code which causes a massive system slowdown if we turn off the VGA > > output. > > > > The patch is sane and minimal and is available in the drm-intel branch > > and in the linux-next branch (see the link to the commit in the bug > > report). > > > > I have applied (and slightly adapted, as it didn't apply cleanly to our > > lucid and karmic git branches) and tested the patch successfully in > > Karmic. > > > > The integration of this patch is very important for our Dell projects and > > for the Ubuntu desktop at large. > > > > Please include the attached patch in both Karmic and Lucid ASAP. > > > > Bug #494461 > > > > Thanks in advance for your time. > > > > Regards, > > Ok, thanks Alberto. I applied the patch now for Karmic. For Lucid it > hopefully gets down from stable, but we / you probably should keep an eye > on it for before the release. > > -Stefan > Ok, thanks a lot. Regards,
From 778c902640530371a169ad1c03566e7c51b09874 Mon Sep 17 00:00:00 2001 From: Li Peng <peng.li@linux.intel.com> Date: Mon, 9 Nov 2009 12:51:22 +0800 Subject: [PATCH 1/1] drm/i915: Fix sync to vblank when VGA output is turned off In current vblank-wait implementation, if we turn off VGA output, drm_wait_vblank will still wait on the disabled pipe until timeout, because vblank on the pipe is assumed be enabled. This would cause slow system response on some system such as moblin. This patch resolve the issue by adding a drm helper function drm_vblank_off which explicitly clear vblank_enabled[crtc], wake up any waiting queue and save last vblank counter before turning off crtc. It also slightly change drm_vblank_get to ensure that we will will return immediately if trying to wait on a disabled pipe. Signed-off-by: Li Peng <peng.li@intel.com> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> [anholt: hand-applied for conflicts with overlay changes] Signed-off-by: Eric Anholt <eric@anholt.net> --- diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index f85aaf2..f298434 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -402,15 +402,21 @@ int drm_vblank_get(struct drm_device *dev, int crtc) spin_lock_irqsave(&dev->vbl_lock, irqflags); /* Going from 0->1 means we have to enable interrupts again */ - if (atomic_add_return(1, &dev->vblank_refcount[crtc]) == 1 && - !dev->vblank_enabled[crtc]) { - ret = dev->driver->enable_vblank(dev, crtc); - DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret); - if (ret) + if (atomic_add_return(1, &dev->vblank_refcount[crtc]) == 1) { + if (!dev->vblank_enabled[crtc]) { + ret = dev->driver->enable_vblank(dev, crtc); + DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret); + if (ret) + atomic_dec(&dev->vblank_refcount[crtc]); + else { + dev->vblank_enabled[crtc] = 1; + drm_update_vblank_count(dev, crtc); + } + } + } else { + if (!dev->vblank_enabled[crtc]) { atomic_dec(&dev->vblank_refcount[crtc]); - else { - dev->vblank_enabled[crtc] = 1; - drm_update_vblank_count(dev, crtc); + ret = -EINVAL; } } spin_unlock_irqrestore(&dev->vbl_lock, irqflags); @@ -437,6 +443,18 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put); +void drm_vblank_off(struct drm_device *dev, int crtc) +{ + unsigned long irqflags; + + spin_lock_irqsave(&dev->vbl_lock, irqflags); + DRM_WAKEUP(&dev->vbl_queue[crtc]); + dev->vblank_enabled[crtc] = 0; + dev->last_vblank[crtc] = dev->driver->get_vblank_counter(dev, crtc); + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); +} +EXPORT_SYMBOL(drm_vblank_off); + /** * drm_vblank_pre_modeset - account for vblanks across mode sets * @dev: DRM device diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 318ba47..652e9ac 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1540,6 +1540,7 @@ static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode) intel_update_watermarks(dev); /* Give the overlay scaler a chance to disable if it's on this pipe */ //intel_crtc_dpms_video(crtc, FALSE); TODO + drm_vblank_off(dev, pipe); /* Disable the VGA plane that we never use */ i915_disable_vga(dev); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 45b67d9..4637dce 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1268,6 +1268,7 @@ extern u32 drm_vblank_count(struct drm_device *dev, int crtc); extern void drm_handle_vblank(struct drm_device *dev, int crtc); extern int drm_vblank_get(struct drm_device *dev, int crtc); extern void drm_vblank_put(struct drm_device *dev, int crtc); +extern void drm_vblank_off(struct drm_device *dev, int crtc); extern void drm_vblank_cleanup(struct drm_device *dev); /* Modesetting support */ extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);