[PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC

[PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC

[Grazvydas Ignotas]

FBIO_WAITFORVSYNC is a stardard ioctl for waiting vsync, already
used by some userspace, so add it as an alias for OMAPFB_WAITFORVSYNC.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
Tomi,

I know I already sent this earlier, but as now FBIO_WAITFORVSYNC is a
standard ioctl, so why don't we support it? I know you might not like
that omapfb_ioctl will have one standard ioctl handler between all OMAP
specific ones, but that's something plenty of other drivers do.

 drivers/video/omap2/omapfb/omapfb-ioctl.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c b/drivers/video/omap2/omapfb/omapfb-ioctl.c
index 9c73618..32fab5a 100644
--- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
+++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
@@ -490,6 +490,7 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg)
 		struct omapfb_vram_info		vram_info;
 		struct omapfb_tearsync_info	tearsync_info;
 		struct omapfb_display_info	display_info;
+		u32				crt;
 	} p;
 
 	int r = 0;
@@ -648,6 +649,17 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg)
 			r = -EFAULT;
 		break;
 
+	case FBIO_WAITFORVSYNC:
+		if (get_user(p.crt, (__u32 __user *)arg)) {
+			r = -EFAULT;
+			break;
+		}
+		if (p.crt != 0) {
+			r = -ENODEV;
+			break;
+		}
+		/* FALLTHROUGH */
+
 	case OMAPFB_WAITFORVSYNC:
 		DBG("ioctl WAITFORVSYNC\n");
 		if (!display) {
-- 
1.6.3.3

RE: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Grazvydas Ignotas [mailto:notasas@gmail.com]
> Sent: Saturday, July 03, 2010 2:25 AM
> To: linux-fbdev@vger.kernel.org
> Cc: linux-omap@vger.kernel.org; Tomi Valkeinen; Hiremath, Vaibhav; Grazvydas
> Ignotas
> Subject: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC
> 
> FBIO_WAITFORVSYNC is a stardard ioctl for waiting vsync, already
> used by some userspace, so add it as an alias for OMAPFB_WAITFORVSYNC.
> 
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
> Tomi,
> 
> I know I already sent this earlier, but as now FBIO_WAITFORVSYNC is a
> standard ioctl, so why don't we support it? I know you might not like
> that omapfb_ioctl will have one standard ioctl handler between all OMAP
> specific ones, but that's something plenty of other drivers do.
> 
[Hiremath, Vaibhav] We should always use standard interfaces wherever possible and I think Tomi will also be aligned on this.

>  drivers/video/omap2/omapfb/omapfb-ioctl.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c
> b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> index 9c73618..32fab5a 100644
> --- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
> +++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> @@ -490,6 +490,7 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd,
> unsigned long arg)
>  		struct omapfb_vram_info		vram_info;
>  		struct omapfb_tearsync_info	tearsync_info;
>  		struct omapfb_display_info	display_info;
> +		u32				crt;
>  	} p;
> 
>  	int r = 0;
> @@ -648,6 +649,17 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd,
> unsigned long arg)
>  			r = -EFAULT;
>  		break;
> 
> +	case FBIO_WAITFORVSYNC:
> +		if (get_user(p.crt, (__u32 __user *)arg)) {
> +			r = -EFAULT;
> +			break;
> +		}
> +		if (p.crt != 0) {
> +			r = -ENODEV;
> +			break;
> +		}
> +		/* FALLTHROUGH */
> +
>  	case OMAPFB_WAITFORVSYNC:
[Hiremath, Vaibhav] I don't see any reason why we should still keep old custom IOCTL support here. 

Thanks,
Vaibhav
>  		DBG("ioctl WAITFORVSYNC\n");
>  		if (!display) {
> --
> 1.6.3.3

Re: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC

[Ville Syrjälä]

On Tue, Jul 06, 2010 at 08:08:14AM +0200, ext Hiremath, Vaibhav wrote:
> > @@ -648,6 +649,17 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd,
> > unsigned long arg)
> >  			r = -EFAULT;
> >  		break;
> > 
> > +	case FBIO_WAITFORVSYNC:
> > +		if (get_user(p.crt, (__u32 __user *)arg)) {
> > +			r = -EFAULT;
> > +			break;
> > +		}
> > +		if (p.crt != 0) {
> > +			r = -ENODEV;
> > +			break;
> > +		}
> > +		/* FALLTHROUGH */
> > +
> >  	case OMAPFB_WAITFORVSYNC:
> [Hiremath, Vaibhav] I don't see any reason why we should still keep old custom IOCTL support here. 

It can already be used so it should not be removed.

-- 
Ville Syrjälä

RE: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@nokia.com]
> Sent: Tuesday, July 06, 2010 3:36 PM
> To: Hiremath, Vaibhav
> Cc: Grazvydas Ignotas; linux-fbdev@vger.kernel.org; linux-
> omap@vger.kernel.org; Valkeinen Tomi (Nokia-MS/Helsinki)
> Subject: Re: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC
> 
> On Tue, Jul 06, 2010 at 08:08:14AM +0200, ext Hiremath, Vaibhav wrote:
> > > @@ -648,6 +649,17 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int
> cmd,
> > > unsigned long arg)
> > >  			r = -EFAULT;
> > >  		break;
> > >
> > > +	case FBIO_WAITFORVSYNC:
> > > +		if (get_user(p.crt, (__u32 __user *)arg)) {
> > > +			r = -EFAULT;
> > > +			break;
> > > +		}
> > > +		if (p.crt != 0) {
> > > +			r = -ENODEV;
> > > +			break;
> > > +		}
> > > +		/* FALLTHROUGH */
> > > +
> > >  	case OMAPFB_WAITFORVSYNC:
> > [Hiremath, Vaibhav] I don't see any reason why we should still keep old
> custom IOCTL support here.
> 
> It can already be used so it should not be removed.
> 
[Hiremath, Vaibhav] I am not in favor of this, if we have standard interface then we should encourage people to use it. Don't you think we will have different interface for OMAP and different for non-omap device.

Thanks,
Vaibhav
> --
> Ville Syrjälä

Re: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC

[Ville Syrjälä]

On Tue, Jul 06, 2010 at 01:26:28PM +0200, ext Hiremath, Vaibhav wrote:
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@nokia.com]
> > Sent: Tuesday, July 06, 2010 3:36 PM
> > To: Hiremath, Vaibhav
> > Cc: Grazvydas Ignotas; linux-fbdev@vger.kernel.org; linux-
> > omap@vger.kernel.org; Valkeinen Tomi (Nokia-MS/Helsinki)
> > Subject: Re: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC
> > 
> > On Tue, Jul 06, 2010 at 08:08:14AM +0200, ext Hiremath, Vaibhav wrote:
> > > > @@ -648,6 +649,17 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int
> > cmd,
> > > > unsigned long arg)
> > > >  			r = -EFAULT;
> > > >  		break;
> > > >
> > > > +	case FBIO_WAITFORVSYNC:
> > > > +		if (get_user(p.crt, (__u32 __user *)arg)) {
> > > > +			r = -EFAULT;
> > > > +			break;
> > > > +		}
> > > > +		if (p.crt != 0) {
> > > > +			r = -ENODEV;
> > > > +			break;
> > > > +		}
> > > > +		/* FALLTHROUGH */
> > > > +
> > > >  	case OMAPFB_WAITFORVSYNC:
> > > [Hiremath, Vaibhav] I don't see any reason why we should still keep old
> > custom IOCTL support here.
> > 
> > It can already be used so it should not be removed.
> > 
> [Hiremath, Vaibhav] I am not in favor of this, if we have standard interface then we should encourage people to use it. Don't you think we will have different interface for OMAP and different for non-omap device.

What anyone thinks that apps should do doesn't matter. Removing the
ioctl will break the ABI and that is not an acceptable thing to do in
kernel development usually.

-- 
Ville Syrjälä

Re: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC

[Tomi Valkeinen]

On Fri, 2010-07-02 at 22:54 +0200, ext Grazvydas Ignotas wrote:
> FBIO_WAITFORVSYNC is a stardard ioctl for waiting vsync, already
> used by some userspace, so add it as an alias for OMAPFB_WAITFORVSYNC.
> 
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
> Tomi,
> 
> I know I already sent this earlier, but as now FBIO_WAITFORVSYNC is a
> standard ioctl, so why don't we support it? I know you might not like
> that omapfb_ioctl will have one standard ioctl handler between all OMAP
> specific ones, but that's something plenty of other drivers do.

One could ask that if FBIO_WAITFORSYNC is a standard ioctl, why do we
need to handle it in omapfb's custom ioctl function =).

But I think this is fine, I'll apply.

 Tomi

RE: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@nokia.com]
> Sent: Tuesday, August 03, 2010 5:32 PM
> To: ext Grazvydas Ignotas
> Cc: linux-fbdev@vger.kernel.org; linux-omap@vger.kernel.org; Hiremath,
> Vaibhav
> Subject: Re: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC
> 
> On Fri, 2010-07-02 at 22:54 +0200, ext Grazvydas Ignotas wrote:
> > FBIO_WAITFORVSYNC is a stardard ioctl for waiting vsync, already
> > used by some userspace, so add it as an alias for OMAPFB_WAITFORVSYNC.
> >
> > Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> > ---
> > Tomi,
> >
> > I know I already sent this earlier, but as now FBIO_WAITFORVSYNC is a
> > standard ioctl, so why don't we support it? I know you might not like
> > that omapfb_ioctl will have one standard ioctl handler between all OMAP
> > specific ones, but that's something plenty of other drivers do.
> 
> One could ask that if FBIO_WAITFORSYNC is a standard ioctl, why do we
> need to handle it in omapfb's custom ioctl function =).
> 
> But I think this is fine, I'll apply.
[Hiremath, Vaibhav] I still think we should mark the old (custom) interface as deprecated and add new standard ioctl interface here, so that in the future we can remove custom one.

Thanks,
Vaibhav

> 
>  Tomi
>