[PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Ali Gholami Rudi]

Hi,

Is there any reason for not adding these ioctls to fbdev?  I searched
the net and couldn't any.  Anyway, these patches simply implement those
ioctls.

Ali Gholami Rudi (2):
  fbdev: add FBIOFILLRECT ioctl
  fbdev: add FBIOCOPYAREA ioctl

 drivers/video/fbmem.c |   50 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fb.h    |    2 +
 2 files changed, 52 insertions(+), 0 deletions(-)

[PATCH 1/2] fbdev: add FBIOFILLRECT ioctl

[Ali Gholami Rudi]

Signed-off-by: Ali Gholami Rudi <ali@rudi.ir>
---
 drivers/video/fbmem.c |   25 +++++++++++++++++++++++++
 include/linux/fb.h    |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index a85c818..a90cd0f 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1016,6 +1016,25 @@ fb_blank(struct fb_info *info, int blank)
  	return ret;
 }
 
+static int fb_fillrect_user(struct fb_info *info,
+			    struct fb_fillrect *fill)
+{
+	int ret = 0;
+	if (fill->rop != ROP_COPY && fill->rop != ROP_XOR)
+		return -EINVAL;
+	if (!lock_fb_info(info))
+		return -ENODEV;
+	if (fill->dx + fill->width > info->var.xres ||
+	    fill->dy + fill->height > info->var.yres) {
+		ret = -EINVAL;
+		goto out;
+	}
+	info->fbops->fb_fillrect(info, fill);
+out:
+	unlock_fb_info(info);
+	return ret;
+}
+
 static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			unsigned long arg)
 {
@@ -1026,6 +1045,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 	struct fb_cmap cmap_from;
 	struct fb_cmap_user cmap;
 	struct fb_event event;
+	struct fb_fillrect fill;
 	void __user *argp = (void __user *)arg;
 	long ret = 0;
 
@@ -1133,6 +1153,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 		release_console_sem();
 		unlock_fb_info(info);
 		break;
+	case FBIOFILLRECT:
+		if (copy_from_user(&fill, argp, sizeof(fill)))
+			return -EFAULT;
+		ret = fb_fillrect_user(info, &fill);
+		break;
 	default:
 		if (!lock_fb_info(info))
 			return -ENODEV;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index f847df9..40cc99a 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -37,6 +37,7 @@ struct dentry;
 #define FBIOGET_HWCINFO         0x4616
 #define FBIOPUT_MODEINFO        0x4617
 #define FBIOGET_DISPINFO        0x4618
+#define FBIOFILLRECT		0x4619
 
 
 #define FB_TYPE_PACKED_PIXELS		0	/* Packed Pixels	*/

[PATCH 2/2] fbdev: add FBIOCOPYAREA ioctl

[Ali Gholami Rudi]

Signed-off-by: Ali Gholami Rudi <ali@rudi.ir>
---
 drivers/video/fbmem.c |   25 +++++++++++++++++++++++++
 include/linux/fb.h    |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index a90cd0f..518119a 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1035,6 +1035,25 @@ out:
 	return ret;
 }
 
+static int fb_copyarea_user(struct fb_info *info,
+			    struct fb_copyarea *copy)
+{
+	int ret = 0;
+	if (!lock_fb_info(info))
+		return -ENODEV;
+	if (copy->dx + copy->width > info->var.xres ||
+	    copy->sx + copy->width > info->var.xres ||
+	    copy->dy + copy->height > info->var.yres ||
+	    copy->sy + copy->height > info->var.yres) {
+		ret = -EINVAL;
+		goto out;
+	}
+	info->fbops->fb_copyarea(info, copy);
+out:
+	unlock_fb_info(info);
+	return ret;
+}
+
 static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			unsigned long arg)
 {
@@ -1046,6 +1065,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 	struct fb_cmap_user cmap;
 	struct fb_event event;
 	struct fb_fillrect fill;
+	struct fb_copyarea copy;
 	void __user *argp = (void __user *)arg;
 	long ret = 0;
 
@@ -1158,6 +1178,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			return -EFAULT;
 		ret = fb_fillrect_user(info, &fill);
 		break;
+	case FBIOCOPYAREA:
+		if (copy_from_user(&copy, argp, sizeof(copy)))
+			return -EFAULT;
+		ret = fb_copyarea_user(info, &copy);
+		break;
 	default:
 		if (!lock_fb_info(info))
 			return -ENODEV;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 40cc99a..f1cf8df 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -38,6 +38,7 @@ struct dentry;
 #define FBIOPUT_MODEINFO        0x4617
 #define FBIOGET_DISPINFO        0x4618
 #define FBIOFILLRECT		0x4619
+#define FBIOCOPYAREA		0x461A
 
 
 #define FB_TYPE_PACKED_PIXELS		0	/* Packed Pixels	*/

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Arjan van de Ven]

On Mon, 13 Jul 2009 19:47:09 +0430
Ali Gholami Rudi <ali@rudi.ir> wrote:

> Hi,
> 
> Is there any reason for not adding these ioctls to fbdev?  I searched
> the net and couldn't any.  Anyway, these patches simply implement
> those ioctls.
> 

can we turn this around, is there a reason to add them?
or in other words, how / where would these be used ?

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Ali Gholami Rudi]

Arjan van de Ven <arjan@infradead.org> wrote:
> On Mon, 13 Jul 2009 19:47:09 +0430
> Ali Gholami Rudi <ali@rudi.ir> wrote:
> > Is there any reason for not adding these ioctls to fbdev?  I searched
> > the net and couldn't any.  Anyway, these patches simply implement
> > those ioctls.
> > 
> 
> can we turn this around, is there a reason to add them?
> or in other words, how / where would these be used ?

User-space programs that use framebuffer directly can use them.  I was
writing a simple framebuffer virtual terminal (using libfreetype for
fonts; like fbterm); scrolling and painting boxes would be faster if
there was someway of using hardware accelerated operations.  I think
other similar programs can benefit, too.

	Ali

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Dave Airlie]

On Tue, Jul 14, 2009 at 2:25 AM, Ali Gholami Rudi<ali@rudi.ir> wrote:
> Arjan van de Ven <arjan@infradead.org> wrote:
>> On Mon, 13 Jul 2009 19:47:09 +0430
>> Ali Gholami Rudi <ali@rudi.ir> wrote:
>> > Is there any reason for not adding these ioctls to fbdev?  I searched
>> > the net and couldn't any.  Anyway, these patches simply implement
>> > those ioctls.
>> >
>>
>> can we turn this around, is there a reason to add them?
>> or in other words, how / where would these be used ?
>
> User-space programs that use framebuffer directly can use them.  I was
> writing a simple framebuffer virtual terminal (using libfreetype for
> fonts; like fbterm); scrolling and painting boxes would be faster if
> there was someway of using hardware accelerated operations.  I think
> other similar programs can benefit, too.

The general opinion is we should keep acceleration in userspace if at
all possible.

Not all hw can implement these usefully in the kernel, directfb
already does some
things for this.

Dave.

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Ali Gholami Rudi]

Dave Airlie <airlied@gmail.com> wrote:
> On Tue, Jul 14, 2009 at 2:25 AM, Ali Gholami Rudi<ali@rudi.ir> wrote:
> > Arjan van de Ven <arjan@infradead.org> wrote:
> >> can we turn this around, is there a reason to add them?
> >> or in other words, how / where would these be used ?
> >
> > User-space programs that use framebuffer directly can use them.  I was
> > writing a simple framebuffer virtual terminal (using libfreetype for
> > fonts; like fbterm); scrolling and painting boxes would be faster if
> > there was someway of using hardware accelerated operations.  I think
> > other similar programs can benefit, too.
> 
> The general opinion is we should keep acceleration in userspace if at
> all possible.

I see.  The line between user- and kernel-space for graphic applications
is very blurred to me :-)

> Not all hw can implement these usefully in the kernel, directfb

AFAICT, many major ones like intelfb, radeonfb and nv implement them and
those that can't, use a software implementation.  You mean they are
unreliable or that there is little performance improvement because of
the way those operations are implemented in the kernel?

> already does some
> things for this.

Sometime ago I did try running directfb on a radeon r300 and it failed.
I didn't try hard to see what's wrong but I got the impression that
userspace apps are not good at using the hardware directly.

	Ali

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Dave Airlie]

On Tue, Jul 14, 2009 at 1:43 PM, Ali Gholami Rudi<ali@rudi.ir> wrote:
> Dave Airlie <airlied@gmail.com> wrote:
>> On Tue, Jul 14, 2009 at 2:25 AM, Ali Gholami Rudi<ali@rudi.ir> wrote:
>> > Arjan van de Ven <arjan@infradead.org> wrote:
>> >> can we turn this around, is there a reason to add them?
>> >> or in other words, how / where would these be used ?
>> >
>> > User-space programs that use framebuffer directly can use them.  I was
>> > writing a simple framebuffer virtual terminal (using libfreetype for
>> > fonts; like fbterm); scrolling and painting boxes would be faster if
>> > there was someway of using hardware accelerated operations.  I think
>> > other similar programs can benefit, too.
>>
>> The general opinion is we should keep acceleration in userspace if at
>> all possible.
>
> I see.  The line between user- and kernel-space for graphic applications
> is very blurred to me :-)
>
>> Not all hw can implement these usefully in the kernel, directfb
>
> AFAICT, many major ones like intelfb, radeonfb and nv implement them and
> those that can't, use a software implementation.  You mean they are
> unreliable or that there is little performance improvement because of
> the way those operations are implemented in the kernel?

Generally newer consumer HW can't implement a simple blit without using the
3D engine which requires a large amount of state to be sent to the device.
Keeping the state encoding and stuff out of the kernel and in userspace is
generally seen as the right thing to do.

>> already does some
>> things for this.
>
> Sometime ago I did try running directfb on a radeon r300 and it failed.
> I didn't try hard to see what's wrong but I got the impression that
> userspace apps are not good at using the hardware directly.

directfb has hw accel for different cards, really porting the code out of
the kernel for the cards you are interested in would probably benefit
more.

Dave.

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Greg KH]

On Mon, Jul 13, 2009 at 07:47:09PM +0430, Ali Gholami Rudi wrote:
> Hi,
> 
> Is there any reason for not adding these ioctls to fbdev?  I searched
> the net and couldn't any.  Anyway, these patches simply implement those
> ioctls.

Have you also added the needed 32 vs. 64 bit handlers for these ioctls?

thanks,

greg k-h

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Arnd Bergmann]

On Monday 13 July 2009, Greg KH wrote:
> On Mon, Jul 13, 2009 at 07:47:09PM +0430, Ali Gholami Rudi wrote:
> > Hi,
> > 
> > Is there any reason for not adding these ioctls to fbdev?  I searched
> > the net and couldn't any.  Anyway, these patches simply implement those
> > ioctls.
> 
> Have you also added the needed 32 vs. 64 bit handlers for these ioctls?
> 

They are not there, but could be trivially added with

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index f8a09bf..ce107c4 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1285,6 +1285,8 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
 	case FBIOPAN_DISPLAY:
 	case FBIOGET_CON2FBMAP:
 	case FBIOPUT_CON2FBMAP:
+	case FBIOCOPYAREA:
+	case FBIOFILLRECT:
 		arg = (unsigned long) compat_ptr(arg);
 	case FBIOBLANK:
 		ret = do_fb_ioctl(info, cmd, arg);


Unfortunately, it would not be as easy to add an FBIOIMAGE, which sounds
equally useful.

	Arnd <><

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Ali Gholami Rudi]

Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 13 July 2009, Greg KH wrote:
> > On Mon, Jul 13, 2009 at 07:47:09PM +0430, Ali Gholami Rudi wrote:
> > > Is there any reason for not adding these ioctls to fbdev?  I searched
> > > the net and couldn't any.  Anyway, these patches simply implement those
> > > ioctls.
> > 
> > Have you also added the needed 32 vs. 64 bit handlers for these ioctls?
> > 
> 
> They are not there, but could be trivially added with
> 
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index f8a09bf..ce107c4 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1285,6 +1285,8 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
>  	case FBIOPAN_DISPLAY:
>  	case FBIOGET_CON2FBMAP:
>  	case FBIOPUT_CON2FBMAP:
> +	case FBIOCOPYAREA:
> +	case FBIOFILLRECT:
>  		arg = (unsigned long) compat_ptr(arg);
>  	case FBIOBLANK:
>  		ret = do_fb_ioctl(info, cmd, arg);

Thanks, I'll update the patches.

	Ali

Re: [PATCH 0/2] fbdev: add fillrect and copyarea ioctls

[Ali Gholami Rudi]

Greg KH <gregkh@suse.de> wrote:
> On Mon, Jul 13, 2009 at 07:47:09PM +0430, Ali Gholami Rudi wrote:
> > Is there any reason for not adding these ioctls to fbdev?  I searched
> > the net and couldn't any.  Anyway, these patches simply implement those
> > ioctls.
> 
> Have you also added the needed 32 vs. 64 bit handlers for these ioctls?

I guess Arnd's patch will do.

By the way, it should check for overflows, too?  I mean should I change
this:

> +       if (copy->dx + copy->width > info->var.xres ||
> +           copy->sx + copy->width > info->var.xres ||
> +           copy->dy + copy->height > info->var.yres ||
> +           copy->sy + copy->height > info->var.yres) {

#define ISSUMLESSTHAN(a, b, s)	(((a) <= (s)) && ((b) <= (s)) && \
				 ((a) + (b) <= (s)))

if (!ISSUMLESSTHAN(copy->dx, copy->width, info->var.xres) || ...

	Ali