* Re: [PATCH 01/02] sh: Add wait for vsync
2010-02-11 10:23 [PATCH 01/02] sh: Add wait for vsync Phil Edworthy
@ 2010-02-12 3:18 ` Paul Mundt
2010-02-12 8:18 ` Ian Armstrong
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Paul Mundt @ 2010-02-12 3:18 UTC (permalink / raw)
To: linux-fbdev
On Thu, Feb 11, 2010 at 10:23:50AM -0000, Phil Edworthy wrote:
> Added FBIO_WAITFORVSYNC ioctl for SH-Mobile devices. Tested on MS7724 board
> against 2.6.33-rc7
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
> @@ -19,6 +19,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/interrupt.h>
> #include <linux/vmalloc.h>
> +#include <linux/ioctl.h>
> #include <video/sh_mobile_lcdc.h>
> #include <asm/atomic.h>
>
> @@ -40,6 +41,10 @@
> #define _LDDWAR 0x900
> #define _LDDRAR 0x904
>
> +#ifndef FBIO_WAITFORVSYNC
> +#define FBIO_WAITFORVSYNC _IOW('F', 0x20, __u32)
> +#endif
> +
> /* shared registers and their order for context save/restore */
> static int lcdc_shared_regs[] = {
> _LDDCKR,
The ioctl definition should be placed in include/video/sh_mobile_lcdc.h,
and the ifndef is superfluous.
If the definition is placed in the header then that can be safely
exported to userspace after the header has been sanitized and thrown in
to include/video/Kbuild, which will make sure that the userspace
applications and kernel keep in sync with regards to the ioctl numbering.
> @@ -795,6 +844,8 @@ static struct fb_ops sh_mobile_lcdc_ops
> .fb_copyarea = sh_mobile_lcdc_copyarea,
> .fb_imageblit = sh_mobile_lcdc_imageblit,
> .fb_pan_display = sh_mobile_fb_pan_display,
> + .fb_ioctl = sh_mobile_ioctl,
> + .fb_compat_ioctl = sh_mobile_ioctl
> };
>
You shouldn't be touching fb_compat_ioctl here. While for this particular
ioctl it doesn't really matter, accesses to arg have the potential for
being bogus due to not having passed through compat_ptr(). This would be
a nasty surprise for anyone treating arg as a userspace pointer when
adding new ioctls in the future. In any event, as we don't presently
implement a 64-bit ABI the compat bits are never going to be used
anyways, so it's best just to not hook it up at all.
Beyond that, if these patches haven't broken the deferred I/O case, I'll
queue them up for 2.6.34.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 01/02] sh: Add wait for vsync
2010-02-11 10:23 [PATCH 01/02] sh: Add wait for vsync Phil Edworthy
2010-02-12 3:18 ` Paul Mundt
@ 2010-02-12 8:18 ` Ian Armstrong
2010-02-15 13:57 ` Phil Edworthy
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Ian Armstrong @ 2010-02-12 8:18 UTC (permalink / raw)
To: linux-fbdev
On Friday 12 February 2010, Paul Mundt wrote:
> On Thu, Feb 11, 2010 at 10:23:50AM -0000, Phil Edworthy wrote:
> > Added FBIO_WAITFORVSYNC ioctl for SH-Mobile devices. Tested on MS7724
> > board against 2.6.33-rc7
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > @@ -19,6 +19,7 @@
> > #include <linux/dma-mapping.h>
> > #include <linux/interrupt.h>
> > #include <linux/vmalloc.h>
> > +#include <linux/ioctl.h>
> > #include <video/sh_mobile_lcdc.h>
> > #include <asm/atomic.h>
> >
> > @@ -40,6 +41,10 @@
> > #define _LDDWAR 0x900
> > #define _LDDRAR 0x904
> >
> > +#ifndef FBIO_WAITFORVSYNC
> > +#define FBIO_WAITFORVSYNC _IOW('F', 0x20, __u32)
> > +#endif
> > +
> > /* shared registers and their order for context save/restore */
> > static int lcdc_shared_regs[] = {
> > _LDDCKR,
>
> The ioctl definition should be placed in include/video/sh_mobile_lcdc.h,
> and the ifndef is superfluous.
>
> If the definition is placed in the header then that can be safely
> exported to userspace after the header has been sanitized and thrown in
> to include/video/Kbuild, which will make sure that the userspace
> applications and kernel keep in sync with regards to the ioctl numbering.
I would question the original definition for FBIO_WAITFORVSYNC. I get the
impression that it originated with the matrox driver, but other drivers have
since implemented the same ioctl and each has an identical define. Would the
proper fix be to get the definition into fb.h and use it from there. It's
already defined in multiple places (ps3fb.h, intelfb.h, atyfb_base.c,
ivtvfb.h, matroxfb.h) and exported to userspace more than once.
--
Ian
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH 01/02] sh: Add wait for vsync
2010-02-11 10:23 [PATCH 01/02] sh: Add wait for vsync Phil Edworthy
2010-02-12 3:18 ` Paul Mundt
2010-02-12 8:18 ` Ian Armstrong
@ 2010-02-15 13:57 ` Phil Edworthy
2010-02-16 1:06 ` Paul Mundt
2010-02-16 1:30 ` Jaya Kumar
4 siblings, 0 replies; 6+ messages in thread
From: Phil Edworthy @ 2010-02-15 13:57 UTC (permalink / raw)
To: linux-fbdev
Hi Paul,
Thanks for your comments, here's the fixed patch (patch 2 can still be
applied after this).
Phil
Added FBIO_WAITFORVSYNC ioctl for SH-Mobile devices.
Tested on MS7724 and MigoR boards against 2.6.33-rc7.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
diff -ruNp a/drivers/video/sh_mobile_lcdcfb.c
b/drivers/video/sh_mobile_lcdcfb.c
--- a/drivers/video/sh_mobile_lcdcfb.c 2010-01-06 00:02:46.000000000 +0000
+++ b/drivers/video/sh_mobile_lcdcfb.c 2010-02-15 13:10:57.000000000 +0000
@@ -19,6 +19,7 @@
#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
#include <linux/vmalloc.h>
+#include <linux/ioctl.h>
#include <video/sh_mobile_lcdc.h>
#include <asm/atomic.h>
@@ -106,6 +107,7 @@ static unsigned long lcdc_offs_sublcd[NR
#define LDRCNTR_SRC 0x00010000
#define LDRCNTR_MRS 0x00000002
#define LDRCNTR_MRC 0x00000001
+#define LDSR_MRS 0x00000100
struct sh_mobile_lcdc_priv;
struct sh_mobile_lcdc_chan {
@@ -124,6 +126,7 @@ struct sh_mobile_lcdc_chan {
unsigned long pan_offset;
unsigned long new_pan_offset;
wait_queue_head_t frame_end_wait;
+ struct completion vsync_completion;
};
struct sh_mobile_lcdc_priv {
@@ -366,7 +369,8 @@ static irqreturn_t sh_mobile_lcdc_irq(in
}
/* VSYNC End */
- if (ldintr & LDINTR_VES) {
+ if ((ldintr & LDINTR_VES) &&
+ (ch->pan_offset != ch->new_pan_offset)) {
unsigned long ldrcntr = lcdc_read(priv, _LDRCNTR);
/* Set the source address for the next refresh */
lcdc_write_chan_mirror(ch, LDSA1R, ch->dma_handle +
@@ -379,6 +383,9 @@ static irqreturn_t sh_mobile_lcdc_irq(in
ldrcntr ^ LDRCNTR_MRS);
ch->pan_offset = ch->new_pan_offset;
}
+
+ if (ldintr & LDINTR_VES)
+ complete(&ch->vsync_completion);
}
return IRQ_HANDLED;
@@ -786,6 +793,44 @@ static int sh_mobile_fb_pan_display(stru
return 0;
}
+static int sh_mobile_wait_for_vsync(struct fb_info *info)
+{
+ struct sh_mobile_lcdc_chan *ch = info->par;
+ unsigned long ldintr;
+ int ret;
+
+ /* Enable VSync End interrupt */
+ ldintr = lcdc_read(ch->lcdc, _LDINTR);
+ ldintr |= LDINTR_VEE;
+ lcdc_write(ch->lcdc, _LDINTR, ldintr);
+
+ ret wait_for_completion_interruptible_timeout(&ch->vsync_completion,
+
msecs_to_jiffies(100));
+
+ if (!ret)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+static int sh_mobile_ioctl(struct fb_info *info, unsigned int cmd,
+ unsigned long arg)
+{
+ int retval;
+
+ switch (cmd) {
+ case FBIO_WAITFORVSYNC:
+ retval = sh_mobile_wait_for_vsync(info);
+ break;
+
+ default:
+ retval = -ENOIOCTLCMD;
+ break;
+ }
+ return retval;
+}
+
+
static struct fb_ops sh_mobile_lcdc_ops = {
.owner = THIS_MODULE,
.fb_setcolreg = sh_mobile_lcdc_setcolreg,
@@ -795,6 +840,7 @@ static struct fb_ops sh_mobile_lcdc_ops
.fb_copyarea = sh_mobile_lcdc_copyarea,
.fb_imageblit = sh_mobile_lcdc_imageblit,
.fb_pan_display = sh_mobile_fb_pan_display,
+ .fb_ioctl = sh_mobile_ioctl,
};
static int sh_mobile_lcdc_set_bpp(struct fb_var_screeninfo *var, int bpp)
@@ -962,6 +1008,7 @@ static int __init sh_mobile_lcdc_probe(s
goto err1;
}
init_waitqueue_head(&priv->ch[i].frame_end_wait);
+ init_completion(&priv->ch[i].vsync_completion);
priv->ch[j].pan_offset = 0;
priv->ch[j].new_pan_offset = 0;
diff -ruNp a/include/video/sh_mobile_lcdc.h b/include/video/sh_mobile_lcdc.h
--- a/include/video/sh_mobile_lcdc.h 2010-01-06 00:02:46.000000000 +0000
+++ b/include/video/sh_mobile_lcdc.h 2010-02-15 13:09:24.000000000 +0000
@@ -34,6 +34,8 @@ enum { LCDC_CLK_BUS, LCDC_CLK_PERIPHERAL
#define LCDC_FLAGS_HSCNT (1 << 3) /* Disable HSYNC during VBLANK */
#define LCDC_FLAGS_DWCNT (1 << 4) /* Disable dotclock during blanking */
+#define FBIO_WAITFORVSYNC _IOW('F', 0x20, __u32)
+
struct sh_mobile_lcdc_sys_bus_cfg {
unsigned long ldmt2r;
unsigned long ldmt3r;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 01/02] sh: Add wait for vsync
2010-02-11 10:23 [PATCH 01/02] sh: Add wait for vsync Phil Edworthy
` (2 preceding siblings ...)
2010-02-15 13:57 ` Phil Edworthy
@ 2010-02-16 1:06 ` Paul Mundt
2010-02-16 1:30 ` Jaya Kumar
4 siblings, 0 replies; 6+ messages in thread
From: Paul Mundt @ 2010-02-16 1:06 UTC (permalink / raw)
To: linux-fbdev
On Mon, Feb 15, 2010 at 01:57:49PM -0000, Phil Edworthy wrote:
> Thanks for your comments, here's the fixed patch (patch 2 can still be
> applied after this).
>
Looks fine, I'll roll them in for 2.6.34, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 01/02] sh: Add wait for vsync
2010-02-11 10:23 [PATCH 01/02] sh: Add wait for vsync Phil Edworthy
` (3 preceding siblings ...)
2010-02-16 1:06 ` Paul Mundt
@ 2010-02-16 1:30 ` Jaya Kumar
4 siblings, 0 replies; 6+ messages in thread
From: Jaya Kumar @ 2010-02-16 1:30 UTC (permalink / raw)
To: linux-fbdev
On Fri, Feb 12, 2010 at 4:18 PM, Ian Armstrong <mail01@iarmst.co.uk> wrote:
>
> I would question the original definition for FBIO_WAITFORVSYNC. I get the
> impression that it originated with the matrox driver, but other drivers have
> since implemented the same ioctl and each has an identical define. Would the
> proper fix be to get the definition into fb.h and use it from there. It's
> already defined in multiple places (ps3fb.h, intelfb.h, atyfb_base.c,
> ivtvfb.h, matroxfb.h) and exported to userspace more than once.
>
Hi Ian, fbdev folks,
Yes, I think the fbdev and vsync concept needs an update. I have an
API I would like to propose. It is still early and I would like to
make it sufficiently generic so that it can meet the needs of at least
the following types of hardware:
A- those that expose a vertical retrace counter
B- those that support generating an interrupt at the start of the
vertical blanking period
C- those controllers for displays that do not have a concept of
vertical retrace (ie: non-volatile/bistable displays)
The api would look like (from userspace):
fb_wait_vsync(int div, int rem, unsigned int *count);
Internally, that would lead to an ioctl and drivers would be able to
provide an fbops for their specific implementation. The implementation
would do something along the following lines for each hardware type:
A- put the calling process to sleep until (retrace_counter % div =
rem). This will let vissim apps achieve their desired level of
granularity.
B- put the calling process to sleep until the vblank interrupt is
received plus calculated offset using timing and pixclock, returning
the calculated value for the count.
C- put the calling process to sleep until the most recent display
update is completed, returning 0 for the count and ignoring div, rem.
Let me know if this concept looks satisfactory.
Thanks,
jaya
^ permalink raw reply [flat|nested] 6+ messages in thread