drm/i915: Fix sync to vblank when VGA output is turned off
 

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
", 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
", 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

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

drm/i915: Fix sync to vblank when VGA output is turned off
 

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=778c902640530371a169ad1c0356 6e7c51b09874

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,
>


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

drm/i915: Fix sync to vblank when VGA output is turned off
 

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=778c902640530371a169ad1c0356 6e7c51b09874
>
> 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
Sustaining Engineer (system)
Foundations Team
Canonical OEM Services

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team