Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 12:35 -0700, Jesse Barnes wrote: > > [...] like Michel said, if only one pipe's vblank is > enabled, only the primary vblank counter should be updated (regardless > of *which* vblank count is enabled). But maybe that can be done at a > higher level, or maybe we can change that behavior and just make things > per-crtc as I've done with most of the code. Yeah, the above is just an i915 specific attempt to provide backwards compatibility for old 2D and 3D drivers that didn't (fully) support vblank interrupts on both pipes. The current 2D driver only enables the interrupt on pipe B together with pipe A, and the current i915 3D drivers can handle it as well. That leaves the i965 3D driver for now. One way to prevent unsupported combinations from timing out could be for the driver enable_vblank hook to return failure when the CRTC is inactive and the core to return immediately in that case. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 22:43 +0300, Ville Syrjälä wrote: > On Tue, Jun 12, 2007 at 08:59:23AM +0200, Michel Dänzer wrote: > > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote: > > > On Monday, June 11, 2007 11:36:10 Keith Packard wrote: > > > > ick. just read the registers and return the value here. We should place > > > > wrap-detection in the core code by reporting the range of the register > > > > values; with the offset suggested above, that would result in a single > > > > addition to convert from raw to cooked frame counts. > > > > > > Ok, here's an updated version: > > > - move wraparound logic to the core > > > - add pre/post modeset ioctls (per-driver right now, making them core > > > would > > > mean lots more DDX changes I think), > > > > Shouldn't really matter, DDX drivers can call driver independent ioctls. > > > > > hope I got this right > > > - add vblank_get/put calls so interrupts are enabled at the right time > > > > > > I haven't implemented Ville's suggestion of adding a short timer before > > > disabling interrupts again, but it should be easy now that the get/put > > > routines are in place and we think it's worth it (might make vblank > > > calls a little cheaper, but it would probably be hard to detect). > > > > Yeah, I'm doubtful. Ville, can you explain some use cases you're > > thinking of? > > Was this discussion only for wait for vblank? I was mainly thinking > about triple buffering w/ page flipping. Now that I think about it the > interrupt would only need to be enabled until the flip is actually > completed by the hardware. Yes, that's what i915_vblank_swap() should do. > With interlaced displays that event might in reality be two vblank > interrupts away (assuming the API allows one to sync to a specific > field). Not explicitly, though the driver could probably use the counter for fields instead of frames, but that might require corresponding changes in the 3D driver. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 12:35 -0700, Jesse Barnes wrote: > But maybe that can be done at a > higher level, or maybe we can change that behavior and just make things > per-crtc as I've done with most of the code. Yeah, this seems nicer. Eventually, we'll fix GL to pick the 'right' crtc. -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 13:44 -0700, Jesse Barnes wrote: > Does that belong in the vblank_enable routine or in a new modeset > routine? Hmm. Probably about the best we can do is wakeup if the frame count on the desired crtc has past *or* the crtc is now idle; the latter probably needs to be part of a new post-mode-set function. -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tuesday, June 12, 2007 10:05:46 Keith Packard wrote: > On Tue, 2007-06-12 at 09:40 -0700, Jesse Barnes wrote: > > Hm, we might get nonsensical values or a non-incrementing vblank > > count, but otoh userspace didn't bother to enable vblank events for > > the pipe it just requested one for, so maybe undefined behavior is > > ok? > > The 'undefined' behaviour here is that the frame count will never > arrive so the client will block forever. > > > If not, it would be easy to check the dev_priv->vblank_pipe value > > for sanity here. > > That, and we should also wake everyone up when crtcs are reconfigured > so clients can reset which crtc they're waiting for. Does that belong in the vblank_enable routine or in a new modeset routine? Btw, I pushed all the latest stuff (including the changes Michel recommended) into the vblank-rework tree of mesa/drm. I've tested on one machine here at hasn't crashed yet and things seem to be working ok... Now to test without the vblank disable stuff Eric put in the DDX driver... Jesse - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, Jun 12, 2007 at 08:59:23AM +0200, Michel Dänzer wrote: > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote: > > On Monday, June 11, 2007 11:36:10 Keith Packard wrote: > > > ick. just read the registers and return the value here. We should place > > > wrap-detection in the core code by reporting the range of the register > > > values; with the offset suggested above, that would result in a single > > > addition to convert from raw to cooked frame counts. > > > > Ok, here's an updated version: > > - move wraparound logic to the core > > - add pre/post modeset ioctls (per-driver right now, making them core > > would > > mean lots more DDX changes I think), > > Shouldn't really matter, DDX drivers can call driver independent ioctls. > > > hope I got this right > > - add vblank_get/put calls so interrupts are enabled at the right time > > > > I haven't implemented Ville's suggestion of adding a short timer before > > disabling interrupts again, but it should be easy now that the get/put > > routines are in place and we think it's worth it (might make vblank > > calls a little cheaper, but it would probably be hard to detect). > > Yeah, I'm doubtful. Ville, can you explain some use cases you're > thinking of? Was this discussion only for wait for vblank? I was mainly thinking about triple buffering w/ page flipping. Now that I think about it the interrupt would only need to be enabled until the flip is actually completed by the hardware. With interlaced displays that event might in reality be two vblank interrupts away (assuming the API allows one to sync to a specific field). -- Ville Syrjälä [EMAIL PROTECTED] http://www.sci.fi/~syrjala/ - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tuesday, June 12, 2007 10:58:24 Keith Packard wrote:
> On Tue, 2007-06-12 at 19:23 +0200, Michel Dänzer wrote:
> > That would mean one register read sequence per waiter per interrupt
> > whereas otherwise it's one read sequence per CRTC (which is enabled
> > and has waiters) per interrupt. Looks like the latter can be made
> > to be more efficient.
>
> So, just so I understand the basic control flow:
>
> wait_for_vblank (crtc, seq)
> {
> enable_irq (crtc)
> while ((seq - crtc->current_seq) < 0)
> block ();
> disable_irq (crtc)
> }
>
> enable_irq (crtc)
> {
> if (enable_count++ == 0) {
> set_interrupts_enabled (crtc);
> crtc->current_seq = read_frame_count (crtc)
> }
> }
>
> disable_irq (crtc)
> {
> if (--enable_count == 0)
> set_interrupts_disabled (crtc);
> }
>
> irq ()
> {
> if (status & interrupt_crtc0)
> crtc0->current_seq = read_frame_count (crtc0);
> if (status & interrupt_crtc1)
> crtc1->current_seq = read_frame_count (crtc1);
> wakeup ();
> }
This looks good, but like Michel said, if only one pipe's vblank is
enabled, only the primary vblank counter should be updated (regardless
of *which* vblank count is enabled). But maybe that can be done at a
higher level, or maybe we can change that behavior and just make things
per-crtc as I've done with most of the code.
Jesse
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 19:23 +0200, Michel Dänzer wrote:
> That would mean one register read sequence per waiter per interrupt
> whereas otherwise it's one read sequence per CRTC (which is enabled and
> has waiters) per interrupt. Looks like the latter can be made to be more
> efficient.
So, just so I understand the basic control flow:
wait_for_vblank (crtc, seq)
{
enable_irq (crtc)
while ((seq - crtc->current_seq) < 0)
block ();
disable_irq (crtc)
}
enable_irq (crtc)
{
if (enable_count++ == 0) {
set_interrupts_enabled (crtc);
crtc->current_seq = read_frame_count (crtc)
}
}
disable_irq (crtc)
{
if (--enable_count == 0)
set_interrupts_disabled (crtc);
}
irq ()
{
if (status & interrupt_crtc0)
crtc0->current_seq = read_frame_count (crtc0);
if (status & interrupt_crtc1)
crtc1->current_seq = read_frame_count (crtc1);
wakeup ();
}
--
[EMAIL PROTECTED]
signature.asc
Description: This is a digitally signed message part
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 19:23 +0200, Michel Dänzer wrote: > That would mean one register read sequence per waiter per interrupt > whereas otherwise it's one read sequence per CRTC (which is enabled and > has waiters) per interrupt. Looks like the latter can be made to be more > efficient. Ok, seems like a sensible plan. Should we have per-crtc wait queues to avoid the extra wakeups as well then? Or is that just overkill? -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 10:16 -0700, Keith Packard wrote: > On Tue, 2007-06-12 at 19:03 +0200, Michel Dänzer wrote: > > > Hmm yeah, I guess you could just update both counters unconditionally. > > Or let the waiters fetch the value themselves? That would mean one register read sequence per waiter per interrupt whereas otherwise it's one read sequence per CRTC (which is enabled and has waiters) per interrupt. Looks like the latter can be made to be more efficient. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 19:03 +0200, Michel Dänzer wrote: > Hmm yeah, I guess you could just update both counters unconditionally. Or let the waiters fetch the value themselves? > Yes, I'm not worried about the software side of it, as this should only > occur when there's a bug somewhere. I'm rather wondering whether > enabling the vblank interrupt for an inactive pipe might cause trouble > on the hardware side, but maybe that's not an issue. Keith? It's not going to cause trouble on the intel chips, but you aren't going to get a lot of interrupts if the crtc isn't running... -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 19:04 +0200, Michel Dänzer wrote:
> On Tue, 2007-06-12 at 09:40 -0700, Jesse Barnes wrote:
> > On Monday, June 11, 2007 11:59:23 Michel Dänzer wrote:
> > > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote:
> > > >
> > > > @@ -313,14 +313,14 @@ irqreturn_t
> > > > i915_driver_irq_handler(DRM_IRQ_ARGS)
> > > > (DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B))
> > > > == (DRM_I915_VBLANK_PIPE_A |
> > > > DRM_I915_VBLANK_PIPE_B)) {
> > > > if (temp & VSYNC_PIPEA_FLAG)
> > > > - atomic_inc(&dev->vbl_received);
> > > > + atomic_inc(&dev->vblank_count[0]);
> > > > if (temp & VSYNC_PIPEB_FLAG)
> > > > - atomic_inc(&dev->vbl_received2);
> > > > + atomic_inc(&dev->vblank_count[1]);
> > > > } else if (((temp & VSYNC_PIPEA_FLAG) &&
> > > > (vblank_pipe & DRM_I915_VBLANK_PIPE_A)) ||
> > > >((temp & VSYNC_PIPEB_FLAG) &&
> > > > (vblank_pipe & DRM_I915_VBLANK_PIPE_B)))
> > > > - atomic_inc(&dev->vbl_received);
> > > > + atomic_inc(&dev->vblank_count[0]);
> > > >
> > > > DRM_WAKEUP(&dev->vbl_queue);
> > > > drm_vbl_send_signals(dev);
> > >
> > > Maybe this should use i915_get_vblank_counter() instead of just
> > > incrementing, in case e.g. an interrupt is missed.
> >
> > Yeah, that's a good idea. And I think it could simplify this logic a
> > bit?
>
> Hmm yeah, I guess you could just update both counters unconditionally.
Or maybe not - the purpose of the logic is that when the X driver has
only enabled the vblank interrupt for one pipe, it is always treated as
primary no matter which pipe it is. Maybe that would need to be handled
within i915_get_vblank_counter() though.
--
Earthling Michel Dänzer | http://tungstengraphics.com
Libre software enthusiast | Debian, X and DRI developer
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tuesday, June 12, 2007 7:53:13 Ian Romanick wrote: > If an app is running with swap buffers synchronized to vblank, won't > it go through the following: > > - Render scene. > - Start to wait for vblank. > - Enable vblank int. > - Wait. > - Disable vblank int. > - Do swap. > - Repeat. > > Isn't there some cost associated with the extra enable / disable of > the vblank interrupt? Yeah, but it's typically just one additional write as Michel pointed out. > In addition, the "fake" wall clock based vblank counter might have > accuracy problems with periods of only one or two vblanks. > Unfortunately, these are the cases where apps will care the most > about accuracy. Really? Refresh rates aren't very high relative to the event accuracy we have in the kernel; we should be able to schedule an event with good granularity, hopefully more than enough to hit a vblank window. But like I said in my original post, drivers that don't have a vblank counter register have a couple of options: 1) never disable interrupts, continue tracking events as usual 2) use a timer to keep the vblank counter accurate across interrupt off periods If it turns out that accuracy really is an issue, we can easily add the delay Ville talked about to keep things easy for those drivers, but I suspect option (1) is probably the best one for such hardware anyway. Jesse - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 09:40 -0700, Jesse Barnes wrote:
> On Monday, June 11, 2007 11:59:23 Michel Dänzer wrote:
> > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote:
> > >
> > > Ok, here's an updated version:
> > > - move wraparound logic to the core
> > > - add pre/post modeset ioctls (per-driver right now, making them
> > > core would mean lots more DDX changes I think),
> >
> > Shouldn't really matter, DDX drivers can call driver independent
> > ioctls.
>
> Yeah, that's true. It could also be called from the server's new
> modesetting code, so that converted drivers would automatically
> benefit. I just wanted to avoid having to update every driver, but I
> think we can avoid that in either case.
Well, driver specific ioctls actually require driver changes both in the
DRM and the DDX, whereas core ioctls shouldn't require any DRM driver
changes and could be handled outside of drivers in the DDX down the road
as well. So the latter seem preferable.
> > > @@ -311,15 +347,15 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
> > > }
> > > }
> > >
> > > - if (dev->vbl_pending >= 100) {
> > > + if (atomic_read(&dev->vbl_pending[crtc]) >= 100) {
> > > spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > > return -EBUSY;
> > > }
> >
> > Previously, dev->vbl_pending was only used to make sure userspace
> > can't exhaust memory by scheduling an infinite number of signals. I
> > don't think this is necessary for blocking calls (as every process
> > can only do one such call at a time), is it?
>
> I don't think so... we should be able to catch that case through a
> regular system wide process count limit (though we'll probably hit a
> memory or other resource problem first).
That's what I was thinking, so I'd say don't bother with that and keep
tracking pending signals separately for this purpose. Otherwise an app
could prevent others from using the blocking interface by scheduling as
many signals as it can.
> > > @@ -313,14 +313,14 @@ irqreturn_t
> > > i915_driver_irq_handler(DRM_IRQ_ARGS)
> > >(DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B))
> > > == (DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B)) {
> > > if (temp & VSYNC_PIPEA_FLAG)
> > > - atomic_inc(&dev->vbl_received);
> > > + atomic_inc(&dev->vblank_count[0]);
> > > if (temp & VSYNC_PIPEB_FLAG)
> > > - atomic_inc(&dev->vbl_received2);
> > > + atomic_inc(&dev->vblank_count[1]);
> > > } else if (((temp & VSYNC_PIPEA_FLAG) &&
> > > (vblank_pipe & DRM_I915_VBLANK_PIPE_A)) ||
> > > ((temp & VSYNC_PIPEB_FLAG) &&
> > > (vblank_pipe & DRM_I915_VBLANK_PIPE_B)))
> > > - atomic_inc(&dev->vbl_received);
> > > + atomic_inc(&dev->vblank_count[0]);
> > >
> > > DRM_WAKEUP(&dev->vbl_queue);
> > > drm_vbl_send_signals(dev);
> >
> > Maybe this should use i915_get_vblank_counter() instead of just
> > incrementing, in case e.g. an interrupt is missed.
>
> Yeah, that's a good idea. And I think it could simplify this logic a
> bit?
Hmm yeah, I guess you could just update both counters unconditionally.
> > > @@ -489,15 +458,116 @@ int i915_irq_wait(DRM_IOCTL_ARGS)
> > > return i915_wait_irq(dev, irqwait.irq_seq);
> > > }
> > >
> > > -static void i915_enable_interrupt (drm_device_t *dev)
> > > +void i915_enable_vblank(drm_device_t *dev, int crtc)
> > > {
> > > drm_i915_private_t *dev_priv = (drm_i915_private_t *)
> > > dev->dev_private;
> > >
> > > - dev_priv->irq_enable_reg = USER_INT_FLAG;
> > > - if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_A)
> > > + switch (crtc) {
> > > + case 0:
> > > dev_priv->irq_enable_reg |= VSYNC_PIPEA_FLAG;
> > > - if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_B)
> > > + break;
> > > + case 1:
> > > dev_priv->irq_enable_reg |= VSYNC_PIPEB_FLAG;
> > > + break;
> > > + default:
> > > + DRM_ERROR("tried to enable vblank on non-existent crtc %d\n",
> > > + crtc);
> > > + break;
> > > + }
> > > +
> > > + I915_WRITE16(I915REG_INT_ENABLE_R, dev_priv->irq_enable_reg);
> > > +}
> >
> > Does this need to check that the X server enabled the vblank
> > interrupt for the pipe? What would happen if this ended up enabling
> > it for an inactive pipe?
>
> Hm, we might get nonsensical values or a non-incrementing vblank count,
> but otoh userspace didn't bother to enable vblank events for the pipe
> it just requested one for, so maybe undefined behavior is ok?
Yes, I'm not worried about the software side of it, as this should only
occur when there's a bug somewhere. I'm rather wondering whether
enabling the vblank interrupt for an inactive pipe might cause trouble
on the hardware side, b
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 09:40 -0700, Jesse Barnes wrote: > Hm, we might get nonsensical values or a non-incrementing vblank count, > but otoh userspace didn't bother to enable vblank events for the pipe > it just requested one for, so maybe undefined behavior is ok? The 'undefined' behaviour here is that the frame count will never arrive so the client will block forever. > If not, it would be easy to check the dev_priv->vblank_pipe value for > sanity here. That, and we should also wake everyone up when crtcs are reconfigured so clients can reset which crtc they're waiting for. -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 17:39 +0200, Michel Dänzer wrote: > That's what I'm thinking. That said, one possible solution would be for > the core to wait for the same number of interrupts as the duration of > the last wait before disabling the interrupt. The core should get interrupts disabled as fast as possible; that increases the available sleep time for the CPU. Drivers for hardware without frame counters are welcome to delay the actual interrupt disable for a while if they like; they've got a tick running which will make it quite easy for them. -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Monday, June 11, 2007 11:59:23 Michel Dänzer wrote:
> On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote:
> > On Monday, June 11, 2007 11:36:10 Keith Packard wrote:
> > > ick. just read the registers and return the value here. We should
> > > place wrap-detection in the core code by reporting the range of
> > > the register values; with the offset suggested above, that would
> > > result in a single addition to convert from raw to cooked frame
> > > counts.
> >
> > Ok, here's an updated version:
> > - move wraparound logic to the core
> > - add pre/post modeset ioctls (per-driver right now, making them
> > core would mean lots more DDX changes I think),
>
> Shouldn't really matter, DDX drivers can call driver independent
> ioctls.
Yeah, that's true. It could also be called from the server's new
modesetting code, so that converted drivers would automatically
benefit. I just wanted to avoid having to update every driver, but I
think we can avoid that in either case.
> > @@ -265,13 +301,13 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
> > }
> >
> > flags = vblwait.request.type & _DRM_VBLANK_FLAGS_MASK;
> > + crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
> >
> > if (!drm_core_check_feature(dev, (flags & _DRM_VBLANK_SECONDARY)
> > ? DRIVER_IRQ_VBL2 : DRIVER_IRQ_VBL))
> > return -EINVAL;
> >
> > - seq = atomic_read((flags & _DRM_VBLANK_SECONDARY) ?
> > &dev->vbl_received2 - : &dev->vbl_received);
> > + seq = atomic_read(&dev->vblank_count[crtc]);
> >
> > switch (vblwait.request.type & _DRM_VBLANK_TYPES_MASK) {
> > case _DRM_VBLANK_RELATIVE:
>
> This value is used as the basis for relative waits, so you need to
> make sure it's up to date.
Yeah, Keith mentioned this too, I've fixed it in my tree.
> > @@ -311,15 +347,15 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
> > }
> > }
> >
> > - if (dev->vbl_pending >= 100) {
> > + if (atomic_read(&dev->vbl_pending[crtc]) >= 100) {
> > spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > return -EBUSY;
> > }
>
> Previously, dev->vbl_pending was only used to make sure userspace
> can't exhaust memory by scheduling an infinite number of signals. I
> don't think this is necessary for blocking calls (as every process
> can only do one such call at a time), is it?
I don't think so... we should be able to catch that case through a
regular system wide process count limit (though we'll probably hit a
memory or other resource problem first).
>
> > @@ -313,14 +313,14 @@ irqreturn_t
> > i915_driver_irq_handler(DRM_IRQ_ARGS)
> > (DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B))
> > == (DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B)) {
> > if (temp & VSYNC_PIPEA_FLAG)
> > - atomic_inc(&dev->vbl_received);
> > + atomic_inc(&dev->vblank_count[0]);
> > if (temp & VSYNC_PIPEB_FLAG)
> > - atomic_inc(&dev->vbl_received2);
> > + atomic_inc(&dev->vblank_count[1]);
> > } else if (((temp & VSYNC_PIPEA_FLAG) &&
> > (vblank_pipe & DRM_I915_VBLANK_PIPE_A)) ||
> >((temp & VSYNC_PIPEB_FLAG) &&
> > (vblank_pipe & DRM_I915_VBLANK_PIPE_B)))
> > - atomic_inc(&dev->vbl_received);
> > + atomic_inc(&dev->vblank_count[0]);
> >
> > DRM_WAKEUP(&dev->vbl_queue);
> > drm_vbl_send_signals(dev);
>
> Maybe this should use i915_get_vblank_counter() instead of just
> incrementing, in case e.g. an interrupt is missed.
Yeah, that's a good idea. And I think it could simplify this logic a
bit?
> > @@ -489,15 +458,116 @@ int i915_irq_wait(DRM_IOCTL_ARGS)
> > return i915_wait_irq(dev, irqwait.irq_seq);
> > }
> >
> > -static void i915_enable_interrupt (drm_device_t *dev)
> > +void i915_enable_vblank(drm_device_t *dev, int crtc)
> > {
> > drm_i915_private_t *dev_priv = (drm_i915_private_t *)
> > dev->dev_private;
> >
> > - dev_priv->irq_enable_reg = USER_INT_FLAG;
> > - if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_A)
> > + switch (crtc) {
> > + case 0:
> > dev_priv->irq_enable_reg |= VSYNC_PIPEA_FLAG;
> > - if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_B)
> > + break;
> > + case 1:
> > dev_priv->irq_enable_reg |= VSYNC_PIPEB_FLAG;
> > + break;
> > + default:
> > + DRM_ERROR("tried to enable vblank on non-existent crtc %d\n",
> > + crtc);
> > + break;
> > + }
> > +
> > + I915_WRITE16(I915REG_INT_ENABLE_R, dev_priv->irq_enable_reg);
> > +}
>
> Does this need to check that the X server enabled the vblank
> interrupt for the pipe? What would happen if this ended up enabling
> it for an inactive pipe?
Hm, we might ge
Re: [RFC] update DRM core vblank code to be more power friendly
On Tue, 2007-06-12 at 07:53 -0700, Ian Romanick wrote: > > Michel Dänzer wrote: > > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote: > > > >> - add vblank_get/put calls so interrupts are enabled at the right time > >> > >> I haven't implemented Ville's suggestion of adding a short timer before > >> disabling interrupts again, but it should be easy now that the get/put > >> routines are in place and we think it's worth it (might make vblank > >> calls a little cheaper, but it would probably be hard to detect). > > > > Yeah, I'm doubtful. Ville, can you explain some use cases you're > > thinking of? > > If an app is running with swap buffers synchronized to vblank, won't it > go through the following: > > - - Render scene. > - - Start to wait for vblank. > - - Enable vblank int. > - - Wait. > - - Disable vblank int. > - - Do swap. > - - Repeat. > > Isn't there some cost associated with the extra enable / disable of the > vblank interrupt? Should generally be one register write for each transition, no big deal. > In addition, the "fake" wall clock based vblank counter might have > accuracy problems with periods of only one or two vblanks. > Unfortunately, these are the cases where apps will care the most about > accuracy. You mean the hypothetical fake counter? :) Does disabling vblank interrupts matter for any hardware that doesn't have hardware counters? > My guess is that only some drivers, most likely those that lack a > hardware vblank counter, will care about the disable delay timer. > Perhaps this could be added later...when the need actually arises? That's what I'm thinking. That said, one possible solution would be for the core to wait for the same number of interrupts as the duration of the last wait before disabling the interrupt. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Michel Dänzer wrote: > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote: >> On Monday, June 11, 2007 11:36:10 Keith Packard wrote: >>> ick. just read the registers and return the value here. We should place >>> wrap-detection in the core code by reporting the range of the register >>> values; with the offset suggested above, that would result in a single >>> addition to convert from raw to cooked frame counts. >> Ok, here's an updated version: >> - move wraparound logic to the core >> - add pre/post modeset ioctls (per-driver right now, making them core would >> mean lots more DDX changes I think), > > Shouldn't really matter, DDX drivers can call driver independent ioctls. > >> hope I got this right >> - add vblank_get/put calls so interrupts are enabled at the right time >> >> I haven't implemented Ville's suggestion of adding a short timer before >> disabling interrupts again, but it should be easy now that the get/put >> routines are in place and we think it's worth it (might make vblank >> calls a little cheaper, but it would probably be hard to detect). > > Yeah, I'm doubtful. Ville, can you explain some use cases you're > thinking of? If an app is running with swap buffers synchronized to vblank, won't it go through the following: - - Render scene. - - Start to wait for vblank. - - Enable vblank int. - - Wait. - - Disable vblank int. - - Do swap. - - Repeat. Isn't there some cost associated with the extra enable / disable of the vblank interrupt? In addition, the "fake" wall clock based vblank counter might have accuracy problems with periods of only one or two vblanks. Unfortunately, these are the cases where apps will care the most about accuracy. My guess is that only some drivers, most likely those that lack a hardware vblank counter, will care about the disable delay timer. Perhaps this could be added later...when the need actually arises? -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.7 (GNU/Linux) iD8DBQFGbrNZX1gOwKyEAw8RAoMtAJ98JrYEVtMNXremOJ0tjUeL+fEyuQCeJraF w3NxVjD7vcdymP8aRgS98lI= =mnG1 -END PGP SIGNATURE- - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote:
> On Monday, June 11, 2007 11:36:10 Keith Packard wrote:
> > ick. just read the registers and return the value here. We should place
> > wrap-detection in the core code by reporting the range of the register
> > values; with the offset suggested above, that would result in a single
> > addition to convert from raw to cooked frame counts.
>
> Ok, here's an updated version:
> - move wraparound logic to the core
> - add pre/post modeset ioctls (per-driver right now, making them core would
> mean lots more DDX changes I think),
Shouldn't really matter, DDX drivers can call driver independent ioctls.
> hope I got this right
> - add vblank_get/put calls so interrupts are enabled at the right time
>
> I haven't implemented Ville's suggestion of adding a short timer before
> disabling interrupts again, but it should be easy now that the get/put
> routines are in place and we think it's worth it (might make vblank
> calls a little cheaper, but it would probably be hard to detect).
Yeah, I'm doubtful. Ville, can you explain some use cases you're
thinking of?
> Any more thoughts? If it looks good, I'll go ahead and test it, clean it up
> a bit,
> and convert my old radeon patch over to this new scheme (other drivers will
> need
> work too though).
The general direction looks good.
> @@ -265,13 +301,13 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
> }
>
> flags = vblwait.request.type & _DRM_VBLANK_FLAGS_MASK;
> + crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
>
> if (!drm_core_check_feature(dev, (flags & _DRM_VBLANK_SECONDARY) ?
> DRIVER_IRQ_VBL2 : DRIVER_IRQ_VBL))
> return -EINVAL;
>
> - seq = atomic_read((flags & _DRM_VBLANK_SECONDARY) ? &dev->vbl_received2
> - : &dev->vbl_received);
> + seq = atomic_read(&dev->vblank_count[crtc]);
>
> switch (vblwait.request.type & _DRM_VBLANK_TYPES_MASK) {
> case _DRM_VBLANK_RELATIVE:
This value is used as the basis for relative waits, so you need to make
sure it's up to date.
> @@ -311,15 +347,15 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
> }
> }
>
> - if (dev->vbl_pending >= 100) {
> + if (atomic_read(&dev->vbl_pending[crtc]) >= 100) {
> spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> return -EBUSY;
> }
Previously, dev->vbl_pending was only used to make sure userspace can't
exhaust memory by scheduling an infinite number of signals. I don't
think this is necessary for blocking calls (as every process can only do
one such call at a time), is it?
> @@ -313,14 +313,14 @@ irqreturn_t
> i915_driver_irq_handler(DRM_IRQ_ARGS)
>(DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B))
> == (DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B)) {
> if (temp & VSYNC_PIPEA_FLAG)
> - atomic_inc(&dev->vbl_received);
> + atomic_inc(&dev->vblank_count[0]);
> if (temp & VSYNC_PIPEB_FLAG)
> - atomic_inc(&dev->vbl_received2);
> + atomic_inc(&dev->vblank_count[1]);
> } else if (((temp & VSYNC_PIPEA_FLAG) &&
> (vblank_pipe & DRM_I915_VBLANK_PIPE_A)) ||
> ((temp & VSYNC_PIPEB_FLAG) &&
> (vblank_pipe & DRM_I915_VBLANK_PIPE_B)))
> - atomic_inc(&dev->vbl_received);
> + atomic_inc(&dev->vblank_count[0]);
>
> DRM_WAKEUP(&dev->vbl_queue);
> drm_vbl_send_signals(dev);
Maybe this should use i915_get_vblank_counter() instead of just
incrementing, in case e.g. an interrupt is missed.
> @@ -489,15 +458,116 @@ int i915_irq_wait(DRM_IOCTL_ARGS)
> return i915_wait_irq(dev, irqwait.irq_seq);
> }
>
> -static void i915_enable_interrupt (drm_device_t *dev)
> +void i915_enable_vblank(drm_device_t *dev, int crtc)
> {
> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>
> - dev_priv->irq_enable_reg = USER_INT_FLAG;
> - if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_A)
> + switch (crtc) {
> + case 0:
> dev_priv->irq_enable_reg |= VSYNC_PIPEA_FLAG;
> - if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_B)
> + break;
> + case 1:
> dev_priv->irq_enable_reg |= VSYNC_PIPEB_FLAG;
> + break;
> + default:
> + DRM_ERROR("tried to enable vblank on non-existent crtc %d\n",
> + crtc);
> + break;
> + }
> +
> + I915_WRITE16(I915REG_INT_ENABLE_R, dev_priv->irq_enable_reg);
> +}
Does this need to check that the X server enabled the vblank interrupt
for the pipe? What would happen if this ended up enabling it
Re: [RFC] update DRM core vblank code to be more power friendly
On Mon, 2007-06-11 at 11:47 -0700, Keith Packard wrote: > On Mon, 2007-06-11 at 11:14 -0700, Ian Romanick wrote: > > > The problem is that a few of the GLX extensions (e.g., > > GLX_SGI_video_sync and GLX_OML_sync_control) allow applications to query > > the vblank counter directly. I don't know of other hardware that > > maintains an actual counter. I know that MGA doesn't, and I'm pretty > > sure that Via doesn't either. > > Radeon and Intel do keep an internal counter. I posted an outline for > keeping the counter reasonably smoothly monotonic as seen by > applications which should make it work across mode switches too. > > There is a huge power savings advantage to using the chip-resident > counters as we don't have to wake up the CPU every frame to bump a > counter. For chips which do not provide a counter, the driver is welcome > to synthesize one in software. I was thinking that this could even be > done by estimating based on wall clock time, which should be sufficient > in most cases by enabling interrupts periodically to resync to the frame > counter edge. Going from 16ms wakeups to 2000ms wakeups is a huge > potential power savings. Also, for the time being they can just leave the vblank interrupt(s) enabled and keep more or less the same code they have now. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Monday, June 11, 2007 7:32:34 Jesse Barnes wrote: > On Monday, June 11, 2007 7:13:04 Jesse Barnes wrote: > > On Monday, June 11, 2007 6:42:09 Keith Packard wrote: > > > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote: > > > > +static u32 last_vblank; /* protected by dev->vbl_lock */ > > > > > > uh. per crtc? > > > > Oh, hm yeah. I guess it'll have to go in drm_device. > > Actually with the lock there I think it's safe. But if I move it to > drm_device I can get rid of the locking, so I guess I'll do that (that way > we can call drm_vblank_get under the vbl_lock). Ok ignore me. I've been looking at this too long. Need per-crtc last counts *and* the lock, otherwise multiple clients could hose one another's wraparound logic (though it's fairly unlikely given the reference counting). Jesse - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Monday, June 11, 2007 7:13:04 Jesse Barnes wrote: > On Monday, June 11, 2007 6:42:09 Keith Packard wrote: > > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote: > > > +static u32 last_vblank; /* protected by dev->vbl_lock */ > > > > uh. per crtc? > > Oh, hm yeah. I guess it'll have to go in drm_device. Actually with the lock there I think it's safe. But if I move it to drm_device I can get rid of the locking, so I guess I'll do that (that way we can call drm_vblank_get under the vbl_lock). Jesse - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Monday, June 11, 2007 6:42:09 Keith Packard wrote:
> On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote:
> > +static u32 last_vblank; /* protected by dev->vbl_lock */
>
> uh. per crtc?
Oh, hm yeah. I guess it'll have to go in drm_device.
> > + atomic_add(diff, &dev->vblank_count[crtc]);
>
> Ok, I think this is reasonable, although different from what I
> suggested.
This is just wraparound handling.
> > + seq = atomic_read(&dev->vblank_count[crtc]);
>
> Isn't this way too early? Seems like you'd want to get a reasonably
> up-to-date value here; should this be after drm_vblank_get?
Oh you're right about that, otherwise we'll end up with lots of events in the
past.
>
> > +u32 i915_get_vblank_counter(drm_device_t *dev, int crtc)
> > +{
> > + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > + return i915_vblank_counter(dev, crtc) + dev_priv->vblank_offset[crtc];
> > +}
>
> I thought you were doing the offset stuff in core?
I thought about it, but if I did, it would require lots of DDX changes I
think? Dave convinced me it should actually be a driver specific SETPARAM,
so after posting this patch I converted to that interface.
> > +
> > +int i915_premodeset(DRM_IOCTL_ARGS)
> > +{
> > + DRM_DEVICE;
> > + drm_i915_private_t *dev_priv = dev->dev_private;
> > +
> > + int crtc = data;
> > +
> > + if (crtc > 1) {
> > + DRM_ERROR("bad crtc\n");
> > + return -EINVAL;
> > + }
> > +
> > + dev_priv->vblank_premodeset[crtc] = i915_vblank_counter(dev, crtc);
> > + return 0;
> > +}
> > +
> > +int i915_postmodeset(DRM_IOCTL_ARGS)
> > +{
> > + DRM_DEVICE;
> > + drm_i915_private_t *dev_priv = dev->dev_private;
> > + u32 new;
> > + int crtc = data;
> > +
> > + if (crtc > 1) {
> > + DRM_ERROR("bad crtc\n");
> > + return -EINVAL;
> > + }
> > +
> > + new = i915_vblank_counter(dev, crtc);
> > + dev_priv->vblank_offset[crtc] = dev_priv->vblank_premodeset[crtc] -
> > new; + return 0;
> > +}
>
> Ah, ok, offsets in driver.
Since most drivers don't do randr-1.2 yet, poking around in their modesetting
code to make a generic DRM call seemed like overkill. I think a per-driver
setparam will get away from this, though either way we'll be stuck with a
dead interface once kernel modesetting is ready.
Jesse
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote:
> +static u32 last_vblank; /* protected by dev->vbl_lock */
uh. per crtc?
> + atomic_add(diff, &dev->vblank_count[crtc]);
Ok, I think this is reasonable, although different from what I
suggested.
> + seq = atomic_read(&dev->vblank_count[crtc]);
Isn't this way too early? Seems like you'd want to get a reasonably
up-to-date value here; should this be after drm_vblank_get?
> +u32 i915_get_vblank_counter(drm_device_t *dev, int crtc)
> +{
> + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> + return i915_vblank_counter(dev, crtc) + dev_priv->vblank_offset[crtc];
> +}
I thought you were doing the offset stuff in core?
> +
> +int i915_premodeset(DRM_IOCTL_ARGS)
> +{
> + DRM_DEVICE;
> + drm_i915_private_t *dev_priv = dev->dev_private;
> +
> + int crtc = data;
> +
> + if (crtc > 1) {
> + DRM_ERROR("bad crtc\n");
> + return -EINVAL;
> + }
> +
> + dev_priv->vblank_premodeset[crtc] = i915_vblank_counter(dev, crtc);
> + return 0;
> +}
> +
> +int i915_postmodeset(DRM_IOCTL_ARGS)
> +{
> + DRM_DEVICE;
> + drm_i915_private_t *dev_priv = dev->dev_private;
> + u32 new;
> + int crtc = data;
> +
> + if (crtc > 1) {
> + DRM_ERROR("bad crtc\n");
> + return -EINVAL;
> + }
> +
> + new = i915_vblank_counter(dev, crtc);
> + dev_priv->vblank_offset[crtc] = dev_priv->vblank_premodeset[crtc] - new;
> + return 0;
> +}
Ah, ok, offsets in driver.
--
[EMAIL PROTECTED]
signature.asc
Description: This is a digitally signed message part
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Monday, June 11, 2007 11:36:10 Keith Packard wrote:
> ick. just read the registers and return the value here. We should place
> wrap-detection in the core code by reporting the range of the register
> values; with the offset suggested above, that would result in a single
> addition to convert from raw to cooked frame counts.
Ok, here's an updated version:
- move wraparound logic to the core
- add pre/post modeset ioctls (per-driver right now, making them core would
mean lots more DDX changes I think), hope I got this right
- add vblank_get/put calls so interrupts are enabled at the right time
I haven't implemented Ville's suggestion of adding a short timer before
disabling interrupts again, but it should be easy now that the get/put
routines are in place and we think it's worth it (might make vblank
calls a little cheaper, but it would probably be hard to detect).
Any more thoughts? If it looks good, I'll go ahead and test it, clean it up a
bit,
and convert my old radeon patch over to this new scheme (other drivers will need
work too though).
Thanks,
Jesse
diff --git a/linux-core/drmP.h b/linux-core/drmP.h
index dd3a69d..ed0e0b7 100644
--- a/linux-core/drmP.h
+++ b/linux-core/drmP.h
@@ -627,8 +627,9 @@ struct drm_driver {
int (*kernel_context_switch) (struct drm_device * dev, int old,
int new);
void (*kernel_context_switch_unlock) (struct drm_device * dev);
- int (*vblank_wait) (struct drm_device * dev, unsigned int *sequence);
- int (*vblank_wait2) (struct drm_device * dev, unsigned int *sequence);
+ u32 (*get_vblank_counter) (struct drm_device *dev, int crtc);
+ void (*enable_vblank) (struct drm_device *dev, int crtc);
+ void (*disable_vblank) (struct drm_device *dev, int crtc);
int (*dri_library_name) (struct drm_device * dev, char * buf);
/**
@@ -783,12 +784,12 @@ typedef struct drm_device {
/[EMAIL PROTECTED] */
wait_queue_head_t vbl_queue;/**< VBLANK wait queue */
- atomic_t vbl_received;
- atomic_t vbl_received2; /**< number of secondary VBLANK
interrupts */
+ atomic_t *vblank_count; /**< number of VBLANK interrupts
(driver must alloc the right number of counters) */
spinlock_t vbl_lock;
struct list_head vbl_sigs; /**< signal list to send on
VBLANK */
struct list_head vbl_sigs2; /**< signals to send on secondary
VBLANK */
- unsigned int vbl_pending;
+ atomic_t *vbl_pending;
+ unsigned long max_vblank_count; /**< size of vblank counter register */
spinlock_t tasklet_lock;/**< For drm_locked_tasklet */
void (*locked_tasklet_func)(struct drm_device *dev);
diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
index 8871671..d9c396a 100644
--- a/linux-core/drm_irq.c
+++ b/linux-core/drm_irq.c
@@ -221,6 +221,42 @@ int drm_control(struct inode *inode, struct file *filp,
}
}
+static u32 last_vblank; /* protected by dev->vbl_lock */
+static void drm_vblank_get(drm_device_t *dev, int crtc)
+{
+ unsigned long irqflags;
+ u32 cur_vblank, diff;
+
+ if (atomic_add_return(1, &dev->vbl_pending[crtc]) != 1)
+ return;
+
+ /*
+* Interrpts were disabled prior to this call, so deal with counter
+* wrap if needed.
+*/
+ cur_vblank = dev->driver->get_vblank_counter(dev, crtc);
+ spin_lock_irqsave(&dev->vbl_lock, irqflags);
+ if (cur_vblank < last_vblank) {
+ diff = dev->max_vblank_count -
+ last_vblank;
+ diff += cur_vblank;
+ } else {
+ diff = cur_vblank - last_vblank;
+ }
+ last_vblank = cur_vblank;
+ spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+
+ atomic_add(diff, &dev->vblank_count[crtc]);
+ dev->driver->enable_vblank(dev, crtc);
+}
+
+static void drm_vblank_put(drm_device_t *dev, int crtc)
+{
+ if (atomic_dec_and_test(&dev->vbl_pending[crtc]))
+ dev->driver->disable_vblank(dev, crtc);
+}
+
+
/**
* Wait for VBLANK.
*
@@ -248,7 +284,7 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
drm_wait_vblank_t vblwait;
struct timeval now;
int ret = 0;
- unsigned int flags, seq;
+ unsigned int flags, seq, crtc;
if ((!dev->irq) || (!dev->irq_enabled))
return -EINVAL;
@@ -265,13 +301,13 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
}
flags = vblwait.request.type & _DRM_VBLANK_FLAGS_MASK;
+ crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
if (!drm_core_check_feature(dev, (flags & _DRM_VBLANK_SECONDARY) ?
DRIVER_IRQ_VBL2 : DRIVER_IRQ_VBL))
return -EINVAL;
- seq = atomic_read((flags & _DRM_VBLANK_SECONDARY) ? &dev->vbl_received2
- : &dev->vbl_received);
+ seq = atomic_read(&dev->vblank_cou
Re: [RFC] update DRM core vblank code to be more power friendly
On Mon, Jun 11, 2007 at 11:34:00AM -0700, Jesse Barnes wrote: > On Monday, June 11, 2007 11:14:45 Ian Romanick wrote: > > Jesse Barnes wrote: > > > We've had some IRC and off-list discussions about how to improve the > > > DRM's vblank code to be a bit more power friendly. The core requirement > > > is that we only enable vblank interrupts when a client is actually > > > waiting for a vblank event (be it a signal or a wakeup). > > > > > > This patch updates the DRM core, requiring drivers to provide vblank > > > enable and disable hooks, as well as a counter updater, and adds some > > > i915 code to use it. > > > > > > When the DRM vblank code is called, the core will update the counter, > > > add the desired sequence value to it, and either setup a signal or > > > wait for the desired sequence number to be hit, enabling vblanks around > > > the operation. Once complete, vblank interrupts will again be disabled > > > to save power. > > > > > > The patch doesn't yet deal with two obvious cases (and probably more > > > that I'm missing, it's untested yet): > > > - the hardware counter resets on mode switch, we need to rebase > > > the appropriate last_counter at that point so it's not treated as > > > a counter wrap > > > - a client interested in signals but also blocked on a vblank event > > > may cause vblanks to be disabled if it received signal at the wrong > > > time > > > > > > I'll be happy to fix it up and/or restructure as requested. I think the > > > basic approach should be fairly sound (even devices that don't support a > > > counter register could fake it using total time/vrefresh or similar), but > > > if not I'd love to hear about it. :) > > > > The problem is that a few of the GLX extensions (e.g., > > GLX_SGI_video_sync and GLX_OML_sync_control) allow applications to query > > the vblank counter directly. I don't know of other hardware that > > maintains an actual counter. I know that MGA doesn't, and I'm pretty > > sure that Via doesn't either. > > Right, we still have to expose the counter. But that just means calling the > update_vblank_counter hook before returning it to userspace. > > And of course, another option for devices that don't have vblank count > registers (aside from the 'fake it based on time' mentioned above) would be > to just leave interrupts enabled and do the counting there as usual. That > would make the enable/disable hooks no-ops, and the update_vblank_counter > into a simple return of the latest value. Rather than immediately disabling the interrupt what about keeping it enabled for a few seconds. In that case it would be never disabled if an application should need it constantly. That should provide accurate results for short intervals. If the interrupt has been disabled before the counter is queried again the driver could resort to approximation. -- Ville Syrjälä [EMAIL PROTECTED] http://www.sci.fi/~syrjala/ - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Monday, June 11, 2007 11:36:10 Keith Packard wrote:
> On Mon, 2007-06-11 at 10:59 -0700, Jesse Barnes wrote:
> > The patch doesn't yet deal with two obvious cases (and probably more
> > that I'm missing, it's untested yet):
> > - the hardware counter resets on mode switch, we need to rebase
> > the appropriate last_counter at that point so it's not treated as
> > a counter wrap
>
> I think this should be handled in the core by having an offset that
> relates the hardware counter to the software visible value. Mode setting
> would then 'rebase' the offset to make the user-visible value change
> smoothly:
>
> pre_mode_set
> old = current raw hardware counter
> post_mode_set
> new = current raw hardware counter
> offset += old - new
>
> now create a 'cooked' hardware counter:
>
> cooked hardware counter = offset + raw hardware counter
Yep, sounds reasonable.
>
> > - a client interested in signals but also blocked on a vblank event
> > may cause vblanks to be disabled if it received signal at the wrong
> > time
>
> the core should track a per-crtc enable/disable count and call the
> driver when it transitions through zero.
Yeah, I think there's even a 'vbl_pending' counter I can use for this, though
I'll have to make it atomic.
> > + unsigned long last_vblank_count[2];
> > + atomic_t vblank_count[2];
>
> For a first implementation, I suggest that the driver never store this
> value and just read from the registers every time it fetches the frame
> counter. You could 'optimize' reading when the interrupt was running by
> caching the last value, but this will be race-prone.
I'm using last_vblank_count to deal with wrapping (as you see below), and
vblank_count is the total vblank value (including all wraps).
>
> > + /*
> > +* Now update the appropriate counter.
> > +* Assume we haven't missed a full 24 bits of vblank
> > +* events, or that it won't matter if they're not accounted
> > +* for when we adjust for wrapping.
> > +* FIXME: if count was zero'd due to modeset, need to rebase
> > +*/
> > + spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > + if (count < dev_priv->last_vblank_count[crtc]) {
> > + diff = 0xff - dev_priv->last_vblank_count[crtc];
> > + diff += count;
> > + } else {
> > + diff = count - dev_priv->last_vblank_count[crtc];
> > + }
> > + dev_priv->last_vblank_count[crtc] = count;
> > + spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > + atomic_add(diff, &dev_priv->vblank_count[crtc]);
> > + return atomic_read(&dev_priv->vblank_count[crtc]);
>
> ick. just read the registers and return the value here. We should place
> wrap-detection in the core code by reporting the range of the register
> values; with the offset suggested above, that would result in a single
> addition to convert from raw to cooked frame counts.
Hm, yeah I guess it might be nicer in the core. I was thinking that this
would give the driver maximum flexibility and avoid the need for a 'max
value' callback or field, but that may be a net win.
Thanks,
Jesse
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Mon, 2007-06-11 at 11:14 -0700, Ian Romanick wrote: > The problem is that a few of the GLX extensions (e.g., > GLX_SGI_video_sync and GLX_OML_sync_control) allow applications to query > the vblank counter directly. I don't know of other hardware that > maintains an actual counter. I know that MGA doesn't, and I'm pretty > sure that Via doesn't either. Radeon and Intel do keep an internal counter. I posted an outline for keeping the counter reasonably smoothly monotonic as seen by applications which should make it work across mode switches too. There is a huge power savings advantage to using the chip-resident counters as we don't have to wake up the CPU every frame to bump a counter. For chips which do not provide a counter, the driver is welcome to synthesize one in software. I was thinking that this could even be done by estimating based on wall clock time, which should be sufficient in most cases by enabling interrupts periodically to resync to the frame counter edge. Going from 16ms wakeups to 2000ms wakeups is a huge potential power savings. -- [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Mon, 2007-06-11 at 10:59 -0700, Jesse Barnes wrote:
> The patch doesn't yet deal with two obvious cases (and probably more
> that I'm missing, it's untested yet):
> - the hardware counter resets on mode switch, we need to rebase
> the appropriate last_counter at that point so it's not treated as
> a counter wrap
I think this should be handled in the core by having an offset that
relates the hardware counter to the software visible value. Mode setting
would then 'rebase' the offset to make the user-visible value change
smoothly:
pre_mode_set
old = current raw hardware counter
post_mode_set
new = current raw hardware counter
offset += old - new
now create a 'cooked' hardware counter:
cooked hardware counter = offset + raw hardware counter
> - a client interested in signals but also blocked on a vblank event
> may cause vblanks to be disabled if it received signal at the wrong
> time
the core should track a per-crtc enable/disable count and call the
driver when it transitions through zero.
> + unsigned long last_vblank_count[2];
> + atomic_t vblank_count[2];
For a first implementation, I suggest that the driver never store this
value and just read from the registers every time it fetches the frame
counter. You could 'optimize' reading when the interrupt was running by
caching the last value, but this will be race-prone.
> + /*
> + * Now update the appropriate counter.
> + * Assume we haven't missed a full 24 bits of vblank
> + * events, or that it won't matter if they're not accounted
> + * for when we adjust for wrapping.
> + * FIXME: if count was zero'd due to modeset, need to rebase
> + */
> + spin_lock_irqsave(&dev->vbl_lock, irqflags);
> + if (count < dev_priv->last_vblank_count[crtc]) {
> + diff = 0xff - dev_priv->last_vblank_count[crtc];
> + diff += count;
> + } else {
> + diff = count - dev_priv->last_vblank_count[crtc];
> + }
> + dev_priv->last_vblank_count[crtc] = count;
> + spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> + atomic_add(diff, &dev_priv->vblank_count[crtc]);
> + return atomic_read(&dev_priv->vblank_count[crtc]);
ick. just read the registers and return the value here. We should place
wrap-detection in the core code by reporting the range of the register
values; with the offset suggested above, that would result in a single
addition to convert from raw to cooked frame counts.
--
[EMAIL PROTECTED]
signature.asc
Description: This is a digitally signed message part
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
On Monday, June 11, 2007 11:14:45 Ian Romanick wrote: > Jesse Barnes wrote: > > We've had some IRC and off-list discussions about how to improve the > > DRM's vblank code to be a bit more power friendly. The core requirement > > is that we only enable vblank interrupts when a client is actually > > waiting for a vblank event (be it a signal or a wakeup). > > > > This patch updates the DRM core, requiring drivers to provide vblank > > enable and disable hooks, as well as a counter updater, and adds some > > i915 code to use it. > > > > When the DRM vblank code is called, the core will update the counter, > > add the desired sequence value to it, and either setup a signal or > > wait for the desired sequence number to be hit, enabling vblanks around > > the operation. Once complete, vblank interrupts will again be disabled > > to save power. > > > > The patch doesn't yet deal with two obvious cases (and probably more > > that I'm missing, it's untested yet): > > - the hardware counter resets on mode switch, we need to rebase > > the appropriate last_counter at that point so it's not treated as > > a counter wrap > > - a client interested in signals but also blocked on a vblank event > > may cause vblanks to be disabled if it received signal at the wrong > > time > > > > I'll be happy to fix it up and/or restructure as requested. I think the > > basic approach should be fairly sound (even devices that don't support a > > counter register could fake it using total time/vrefresh or similar), but > > if not I'd love to hear about it. :) > > The problem is that a few of the GLX extensions (e.g., > GLX_SGI_video_sync and GLX_OML_sync_control) allow applications to query > the vblank counter directly. I don't know of other hardware that > maintains an actual counter. I know that MGA doesn't, and I'm pretty > sure that Via doesn't either. Right, we still have to expose the counter. But that just means calling the update_vblank_counter hook before returning it to userspace. And of course, another option for devices that don't have vblank count registers (aside from the 'fake it based on time' mentioned above) would be to just leave interrupts enabled and do the counting there as usual. That would make the enable/disable hooks no-ops, and the update_vblank_counter into a simple return of the latest value. Jesse - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [RFC] update DRM core vblank code to be more power friendly
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Jesse Barnes wrote: > We've had some IRC and off-list discussions about how to improve the > DRM's vblank code to be a bit more power friendly. The core requirement > is that we only enable vblank interrupts when a client is actually waiting > for a vblank event (be it a signal or a wakeup). > > This patch updates the DRM core, requiring drivers to provide vblank > enable and disable hooks, as well as a counter updater, and adds some > i915 code to use it. > > When the DRM vblank code is called, the core will update the counter, > add the desired sequence value to it, and either setup a signal or > wait for the desired sequence number to be hit, enabling vblanks around > the operation. Once complete, vblank interrupts will again be disabled > to save power. > > The patch doesn't yet deal with two obvious cases (and probably more > that I'm missing, it's untested yet): > - the hardware counter resets on mode switch, we need to rebase > the appropriate last_counter at that point so it's not treated as > a counter wrap > - a client interested in signals but also blocked on a vblank event > may cause vblanks to be disabled if it received signal at the wrong > time > > I'll be happy to fix it up and/or restructure as requested. I think the > basic approach should be fairly sound (even devices that don't support a > counter register could fake it using total time/vrefresh or similar), but > if not I'd love to hear about it. :) The problem is that a few of the GLX extensions (e.g., GLX_SGI_video_sync and GLX_OML_sync_control) allow applications to query the vblank counter directly. I don't know of other hardware that maintains an actual counter. I know that MGA doesn't, and I'm pretty sure that Via doesn't either. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.7 (GNU/Linux) iD8DBQFGbZEVX1gOwKyEAw8RAhh/AJwJYPPFgeYeZ/1MaJg2aw4MT9kvmQCfU5+N IhDQvXWW/4s270aXLm+KzRU= =cdp9 -END PGP SIGNATURE- - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
