[PATCH] omapfb vsync support

[PATCH] omapfb vsync support

[Brian Swetland]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]


Here's a patch that provides the FBIO_WAITFORVSYNC ioctl
(same one used by the ati and nvidia drivers) to the omapfb
driver.  We could use the omap-specific ioctl, but it seemed
to make more sense to use an existing one.  Opinions?

Brian


[-- Attachment #2: omapfb_vsync.patch --]
[-- Type: text/plain, Size: 4475 bytes --]

diff -u linux-omap-2.6/drivers/video/omap/lcdc.c linux-omap-work/drivers/video/omap/lcdc.c
--- linux-omap-2.6/drivers/video/omap/lcdc.c	2006-02-27 12:07:17.000000000 -0800
+++ linux-omap-work/drivers/video/omap/lcdc.c	2006-02-27 12:39:25.000000000 -0800
@@ -294,6 +294,12 @@
 		}
 	}
 
+	if (status & OMAP_LCDC_STAT_VSYNC) {
+		struct omapfb_device* fbdev = (struct omapfb_device*)dev_id;
+		fbdev->vblank.count++;
+		wake_up_interruptible(&fbdev->vblank.wait);
+	}
+	
 	/* Clear these interrupt status bits.
 	 * Sync_lost, FUF bits were cleared by disabling the LCD controller
 	 * LOADED_PALETTE can be cleared this way only in palette only
@@ -899,6 +905,22 @@
 	clk_put(omap_lcdc.lcd_ck);
 }
 
+static void omap_lcdc_enable_vsync_irq(void)
+{
+	if (!(omap_lcdc.irq_mask & OMAP_LCDC_IRQ_VSYNC)) {
+		enable_irqs(OMAP_LCDC_IRQ_VSYNC);
+		enable_controller();
+	}
+}
+
+static void omap_lcdc_disable_vsync_irq(void)
+{
+	if (omap_lcdc.irq_mask & OMAP_LCDC_IRQ_VSYNC) {
+		disable_irqs(OMAP_LCDC_IRQ_VSYNC);
+		enable_controller();
+	}
+}
+
 struct lcd_ctrl omap1_int_ctrl = {
 	.name			= "internal",
 	.init			= omap_lcdc_init,
@@ -913,6 +935,9 @@
 	.setup_plane		= omap_lcdc_setup_plane,
 	.enable_plane		= omap_lcdc_enable_plane,
 	.setcolreg		= omap_lcdc_setcolreg,
+	.set_color_key		= NULL,
+	.enable_vsync_irq	= omap_lcdc_enable_vsync_irq,
+	.disable_vsync_irq	= omap_lcdc_disable_vsync_irq
 };
 
 MODULE_DESCRIPTION("TI OMAP LCDC controller");
diff -u linux-omap-2.6/drivers/video/omap/omapfb_main.c linux-omap-work/drivers/video/omap/omapfb_main.c
--- linux-omap-2.6/drivers/video/omap/omapfb_main.c	2006-02-27 12:07:17.000000000 -0800
+++ linux-omap-work/drivers/video/omap/omapfb_main.c	2006-02-27 12:39:25.000000000 -0800
@@ -119,6 +119,10 @@
 	up(&fbdev->rqueue_sema);
 }
 
+#ifndef FBIO_WAITFORVSYNC
+#define FBIO_WAITFORVSYNC _IOW('F', 0x20, __u32)
+#endif
+
 /*
  * ---------------------------------------------------------------------------
  * LCD controller and LCD DMA
@@ -547,6 +551,34 @@
 	return r;
 }
 
+static int omapfb_wait_for_vsync(struct omapfb_device *fbdev, unsigned int crtc)
+{
+	struct omap_interrupt *vbl;
+	struct lcd_ctrl *ctrl;
+	unsigned int count;
+	int r;
+
+	if (crtc != 0)
+		return -ENODEV;
+
+	vbl = &fbdev->vblank;
+	ctrl = fbdev->ctrl;
+
+	if (ctrl->enable_vsync_irq == NULL) 
+		return -ENODEV;
+	
+	count = vbl->count;
+
+	/* enable the vsync irq the first time */
+	ctrl->enable_vsync_irq();
+		
+	r = wait_event_interruptible_timeout(vbl->wait, count != vbl->count, HZ/10);
+	if (r == 0)
+		r = -ETIMEDOUT;
+
+	return r;
+}
+
 /* Check values in var, try to adjust them in case of out of bound values if
  * possible, or return error.
  */
@@ -797,6 +829,7 @@
 		enum omapfb_update_mode		update_mode;
 		unsigned long		caps;
 		unsigned int		mirror;
+		unsigned int		crtc;
 	} p;
 	int r = 0;
 
@@ -804,6 +837,13 @@
 	DBGPRINT(2, "cmd=%010x\n", cmd);
 	switch (cmd)
 	{
+	case FBIO_WAITFORVSYNC:
+		if (get_user(p.crtc, (unsigned int __user *)arg))
+			r = -EFAULT;
+		else 
+			r = omapfb_wait_for_vsync(fbdev, p.crtc);
+		break;
+
 	case OMAPFB_MIRROR:
 		if (get_user(p.mirror, (int __user *)arg))
 			r = -EFAULT;
@@ -1275,6 +1315,9 @@
 
 	init_MUTEX(&fbdev->rqueue_sema);
 
+	init_waitqueue_head(&fbdev->vblank.wait);
+	fbdev->vblank.count = 0;
+
 #ifdef CONFIG_ARCH_OMAP1
 	fbdev->int_ctrl = &omap1_int_ctrl;
 #ifdef CONFIG_FB_OMAP_LCDC_EXTERNAL
diff -u linux-omap-2.6/include/asm-arm/arch-omap/omapfb.h linux-omap-work/include/asm-arm/arch-omap/omapfb.h
--- linux-omap-2.6/include/asm-arm/arch-omap/omapfb.h	2006-02-27 12:06:29.000000000 -0800
+++ linux-omap-work/include/asm-arm/arch-omap/omapfb.h	2006-02-27 12:39:26.000000000 -0800
@@ -139,6 +139,7 @@
 #define OMAP_LCDC_SIGNAL_MASK		0x003f
 
 #define OMAP_LCDC_PANEL_TFT		0x0100
+#define OMAP_LCDC_PANEL_INIT_AFTER_LCDC 0x0200
 
 #ifdef CONFIG_ARCH_OMAP1
 #define OMAPFB_PLANE_NUM		1
@@ -257,6 +258,8 @@
 					   int update_hw_mem);
 	int		(*set_color_key)  (struct omapfb_color_key *ck);
 
+	void		(*enable_vsync_irq)(void);
+	void		(*disable_vsync_irq)(void);
 };
 
 enum omapfb_state {
@@ -265,6 +268,11 @@
 	OMAPFB_ACTIVE	= 100
 };
 
+struct omap_interrupt {
+	wait_queue_head_t	wait;
+	unsigned int		count;
+};
+
 struct omapfb_device {
 	int			state;
 	int                     ext_lcdc;               /* Using external
@@ -287,6 +295,8 @@
 							   interface */
 	struct fb_info		*fb_info;
 
+	struct omap_interrupt vblank;
+
 	struct device		*dev;
 };
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

RE: [PATCH] omapfb vsync support

[Woodruff, Richard]

How come there is no disable of the interrupt?

Seems like at ioctrl the caller needs to be tracked so when he does a
close the interrupt can be disabled.

This will cause a VST enabled kernel to wake up for no good reason if
there is no application requesting it anymore.  Someone pointed out this
fact to me recently while doing power measurements.  If you have a low
power screen saver mode this can mess up expected power savings.

Regards,
Richard W.

> -----Original Message-----
> From: linux-omap-open-source-bounces@linux.omap.com
[mailto:linux-omap-
> open-source-bounces@linux.omap.com] On Behalf Of Brian Swetland
> Sent: Monday, February 27, 2006 3:59 PM
> To: linux-omap-open-source@linux.omap.com
> Subject: [PATCH] omapfb vsync support
> 
> 
> Here's a patch that provides the FBIO_WAITFORVSYNC ioctl
> (same one used by the ati and nvidia drivers) to the omapfb
> driver.  We could use the omap-specific ioctl, but it seemed
> to make more sense to use an existing one.  Opinions?
> 
> Brian

Re: [PATCH] omapfb vsync support

[Brian Swetland]

["Woodruff, Richard" <r-woodruff2@ti.com>]
> How come there is no disable of the interrupt?
> 
> Seems like at ioctrl the caller needs to be tracked so when he does a
> close the interrupt can be disabled.
> 
> This will cause a VST enabled kernel to wake up for no good reason if
> there is no application requesting it anymore.  Someone pointed out this
> fact to me recently while doing power measurements.  If you have a low
> power screen saver mode this can mess up expected power savings.

Hmm.  I wonder if there's some happy medium between disabling the 
interrupt every time the ioctl returns and having to close and reopen
the driver when going into and returning from low power mode.

Brian

RE: [PATCH] omapfb vsync support

[Woodruff, Richard]

> > This will cause a VST enabled kernel to wake up for no good reason
if
> > there is no application requesting it anymore.  Someone pointed out
this
> > fact to me recently while doing power measurements.  If you have a
low
> > power screen saver mode this can mess up expected power savings.
> 
> Hmm.  I wonder if there's some happy medium between disabling the
> interrupt every time the ioctl returns and having to close and reopen
> the driver when going into and returning from low power mode.

Two bits, one is I don't see the disable being called in the code when
you wake the process up.  I see you have the function, at interrupt wake
up time it should likely be disabled.

Second goes to your comment... One would think an application should let
go of this accounting if it doesn't need it.  Perhaps that is two
optimistic... I would think something like sending a D2/D3 message to
the driver could trigger it to lock out processes until a specified wake
up event.  The policy manager upon feeling its correct, would send such
a call to the devices, to match the wake up state.  An activity time out
like which now drives blanking might be enough to trigger the lockout,
but probably not quite enough.

Regards,
Richard W.

Re: [PATCH] omapfb vsync support

[Mathias Agopian]

Hello,

I'm the one to blame for the omapfb vsync patch :-)

> ["Woodruff, Richard" <r-woodruff2@ti.com>]
>> How come there is no disable of the interrupt?
>>
>> Seems like at ioctrl the caller needs to be tracked so when he does a
>> close the interrupt can be disabled.

I decided not to bother too much about tracking the clients because  
the omapfb driver is not thread-safe in a lot of ways, which means it  
cannot really support more than one client anyway (or in very basic  
situations). In other words, I assumed there would only always be one  
client.

Nevertheless, we wouldn't really need to track the clients because I  
think it would be fine to turn the IRQ off when any client closes the  
driver (provided that no other client is currently waiting on it),  
since it is re-enabled when a client calls the FBIO_WAITFORSYNC ioctl.

>> This will cause a VST enabled kernel to wake up for no good reason if
>> there is no application requesting it anymore.  Someone pointed  
>> out this
>> fact to me recently while doing power measurements.  If you have a  
>> low
>> power screen saver mode this can mess up expected power savings.
>
> Hmm.  I wonder if there's some happy medium between disabling the
> interrupt every time the ioctl returns and having to close and reopen
> the driver when going into and returning from low power mode.
>
> Brian

Maybe we could simply disable the IRQ from the IRQ handler after a  
second or so if no client has requested a FBIO_WAITFORSYNC ioctl?

That, combined with disabling the interrupt when a client closes the  
driver should take care of most cases.

Alternatively, we could disable the IRQ each time, if it's not a too  
heavy operation?

On Feb 27, 2006, at 3:10 PM, Woodruff, Richard wrote:

> Two bits, one is I don't see the disable being called in the code when
> you wake the process up.  I see you have the function, at interrupt  
> wake
> up time it should likely be disabled.

Yes, I added the function for completeness, I thought it might be  
useful at some point (I thought the issue of not disabling it each  
time would come up ;-) ).


Mathias

RE: [PATCH] omapfb vsync support

[Woodruff, Richard]

> Maybe we could simply disable the IRQ from the IRQ handler after a
> second or so if no client has requested a FBIO_WAITFORSYNC ioctl?
> 
> That, combined with disabling the interrupt when a client closes the
> driver should take care of most cases.
> 
> Alternatively, we could disable the IRQ each time, if it's not a too
> heavy operation?

Maybe just have a use count and decrement it in omapfb_wait_for_vsync()
at wake up time... If you know how many users are in your queue you
should know how many to decrement it by.  If a process is killed I'm not
sure how that plays out, the kernel likely just closes paths and
probably silently de-queues a process where ever it happens to be stuck.

Regards,
Richard W.

Re: [PATCH] omapfb vsync support

[Imre Deak]

On Mon, 2006-02-27 at 15:27 -0800, ext Mathias Agopian wrote:
> Hello,
> 
> I'm the one to blame for the omapfb vsync patch :-)
> 
> > ["Woodruff, Richard" <r-woodruff2@ti.com>]
> >> How come there is no disable of the interrupt?
> >>
> >> Seems like at ioctrl the caller needs to be tracked so when he does a
> >> close the interrupt can be disabled.
> 
> I decided not to bother too much about tracking the clients because  
> the omapfb driver is not thread-safe in a lot of ways, which means it  
> cannot really support more than one client anyway (or in very basic  
> situations). In other words, I assumed there would only always be one  
> client.

Please be more specific. Then we could fix those places.

> 
> Nevertheless, we wouldn't really need to track the clients because I  
> think it would be fine to turn the IRQ off when any client closes the  
> driver (provided that no other client is currently waiting on it),  
> since it is re-enabled when a client calls the FBIO_WAITFORSYNC ioctl.
> 
> >> This will cause a VST enabled kernel to wake up for no good reason if
> >> there is no application requesting it anymore.  Someone pointed  
> >> out this
> >> fact to me recently while doing power measurements.  If you have a  
> >> low
> >> power screen saver mode this can mess up expected power savings.
> >
> > Hmm.  I wonder if there's some happy medium between disabling the
> > interrupt every time the ioctl returns and having to close and reopen
> > the driver when going into and returning from low power mode.
> >
> > Brian
> 
> Maybe we could simply disable the IRQ from the IRQ handler after a  
> second or so if no client has requested a FBIO_WAITFORSYNC ioctl?
> 
> That, combined with disabling the interrupt when a client closes the  
> driver should take care of most cases.
> 
> Alternatively, we could disable the IRQ each time, if it's not a too  
> heavy operation?

How about taking the lock for the IOCTL and handling the vsync
completely in the controller:

static int omapfb_wait_for_vsync(struct omapfb_device *fbdev)
{
	omapfb_rqueue_lock(fbdev);
	ctrl->vsync();
	omapfb_rqueue_unlock(fbdev);
}

And the controller could enable the IRQ wait for it then disable the
IRQ.

--Imre

> 
> On Feb 27, 2006, at 3:10 PM, Woodruff, Richard wrote:
> 
> > Two bits, one is I don't see the disable being called in the code when
> > you wake the process up.  I see you have the function, at interrupt  
> > wake
> > up time it should likely be disabled.
> 
> Yes, I added the function for completeness, I thought it might be  
> useful at some point (I thought the issue of not disabling it each  
> time would come up ;-) ).
> 
> 
> Mathias
> 
> 
> _______________________________________________
> Linux-omap-open-source mailing list
> Linux-omap-open-source@linux.omap.com
> http://linux.omap.com/mailman/listinfo/linux-omap-open-source

RE: [PATCH] omapfb vsync support

[Woodruff, Richard]

> > Alternatively, we could disable the IRQ each time, if it's not a too
> > heavy operation?
> 
> Maybe just have a use count and decrement it in
omapfb_wait_for_vsync()
> at wake up time... If you know how many users are in your queue you
> should know how many to decrement it by.  If a process is killed I'm
not
> sure how that plays out, the kernel likely just closes paths and
> probably silently de-queues a process where ever it happens to be
stuck.

Actually, a lock and an enable flag are probably enough.  Everyone in
your queued gets woke up upon wake_up anyway, counting probably doesn't
add anything but overhead.  Having a suspend lockout on D2/D3 is
probably still worth having.  However, that becomes a bit more coupled
with power management and that seems to be a bit of a moving target.

Regards,
Richard W.