clipping

clipping

[Geert Uytterhoeven]

cfb_imageblit() takes care of clipping, but forgets to update fb_image.data if
fb_image.d[xy] was changed.

BTW, do we really need clipping in fb_ops.fb_{fillrect,copyarea,imageblit}()?

If these calls are accessible from user space through ioctl() (if anyone really
wants to do that?!?), I guess the answer is yes. For calls from kernel space,
I'd say it's not necessary since calling them with out-of-range parameters is a
bug anyway.
So we could move the clipping to the generic ioctl() handling in fbmem.c, if we
ever want to provide that functionality. Or is there a too high speed penalty
for graphics chips that do implement clipping in hardware?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: clipping

[Antonino Daplas]

On Mon, 2002-12-30 at 05:21, Geert Uytterhoeven wrote:
> 
> cfb_imageblit() takes care of clipping, but forgets to update fb_image.data if
> fb_image.d[xy] was changed.
> 
> BTW, do we really need clipping in fb_ops.fb_{fillrect,copyarea,imageblit}()?
Personally, I don't think we need clipping.  I tried removing it before
(circa linux-2.5.30+), but the console segfaults whenever I decrease
var->yres_virtual.  I haven't tried this again with the newest
framebuffer framework though.

Tony





-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: clipping

[Geert Uytterhoeven]

On 3 Jan 2003, Antonino Daplas wrote:
> On Mon, 2002-12-30 at 05:21, Geert Uytterhoeven wrote:
> > cfb_imageblit() takes care of clipping, but forgets to update fb_image.data if
> > fb_image.d[xy] was changed.
> > 
> > BTW, do we really need clipping in fb_ops.fb_{fillrect,copyarea,imageblit}()?
> Personally, I don't think we need clipping.  I tried removing it before
> (circa linux-2.5.30+), but the console segfaults whenever I decrease
> var->yres_virtual.  I haven't tried this again with the newest
> framebuffer framework though.

Any definitive answer on this?

What happens if you resize the VT to such a large size that columns*fontwidth >
xres_virtual or rows*fontheight > yres_virtual? I guess clipping prevents a
crash there?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[James Simmons]

> > > BTW, do we really need clipping in fb_ops.fb_{fillrect,copyarea,imageblit}()?
> > Personally, I don't think we need clipping.  I tried removing it before
> > (circa linux-2.5.30+), but the console segfaults whenever I decrease
> > var->yres_virtual.  I haven't tried this again with the newest
> > framebuffer framework though.
> 
> Any definitive answer on this?
> 
> What happens if you resize the VT to such a large size that columns*fontwidth >
> xres_virtual or rows*fontheight > yres_virtual? I guess clipping prevents a
> crash there?

Correct. Actually fbcon_resize checks to make sure the user doesn't do 
something stupid like this. So we might not needed. Hm.




-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Geert Uytterhoeven]

On Tue, 7 Jan 2003, James Simmons wrote:
> > > > BTW, do we really need clipping in fb_ops.fb_{fillrect,copyarea,imageblit}()?
> > > Personally, I don't think we need clipping.  I tried removing it before
> > > (circa linux-2.5.30+), but the console segfaults whenever I decrease
> > > var->yres_virtual.  I haven't tried this again with the newest
> > > framebuffer framework though.
> > 
> > Any definitive answer on this?
> > 
> > What happens if you resize the VT to such a large size that columns*fontwidth >
> > xres_virtual or rows*fontheight > yres_virtual? I guess clipping prevents a
> > crash there?
> 
> Correct. Actually fbcon_resize checks to make sure the user doesn't do 
> something stupid like this. So we might not needed. Hm.

And what if you use fbset to reduce the resolution below what's needed to
accomodate the current console size?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Antonino Daplas]

On Wed, 2003-01-08 at 17:54, Geert Uytterhoeven wrote:
> On Tue, 7 Jan 2003, James Simmons wrote:
> > > > > BTW, do we really need clipping in fb_ops.fb_{fillrect,copyarea,imageblit}()?
> > > > Personally, I don't think we need clipping.  I tried removing it before
> > > > (circa linux-2.5.30+), but the console segfaults whenever I decrease
> > > > var->yres_virtual.  I haven't tried this again with the newest
> > > > framebuffer framework though.
> > > 
> > > Any definitive answer on this?
> > > 
> > > What happens if you resize the VT to such a large size that columns*fontwidth >
> > > xres_virtual or rows*fontheight > yres_virtual? I guess clipping prevents a
> > > crash there?
> > 
> > Correct. Actually fbcon_resize checks to make sure the user doesn't do 
> > something stupid like this. So we might not needed. Hm.
> 
> And what if you use fbset to reduce the resolution below what's needed to
> accomodate the current console size?
> 
Then we do need clipping :-)  I think that happened to me, I just did
not recognize it.  I accidentally lowered the window size using fbset,
and suddenly I had a console where the tail end of each row of text
wraps to the next row.  I wasn't using cfb_imageblit then so this was
done by hardware.  If I was using cfb_imageblit, this will be clipped.

So, I think we do need clipping, but instead of implementing it in
fb_{fillrect,copyarea,imageblit}, we implement it higher up, in
accel_putcs() and family.  This should also protect drivers using 
hardware-based drawing. 

So perhaps, a function something like this:

void fb_clip(struct fb_fillrect *region, struct fb_fillrect *clip);

This should not be difficult to implement, and I'll code it if everyone
agrees.

The other option (which I don't like) is just to check the passed
fb_var_screeninfo in the put_var ioctl against the current console
window size.  But this is not foolproof as we will not be sure of the
resulting window size _after_ the fb_set_var() call.

Tony




-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[James Simmons]

> > > What happens if you resize the VT to such a large size that columns*fontwidth >
> > > xres_virtual or rows*fontheight > yres_virtual? I guess clipping prevents a
> > > crash there?
> > 
> > Correct. Actually fbcon_resize checks to make sure the user doesn't do 
> > something stupid like this. So we might not needed. Hm.
> 
> And what if you use fbset to reduce the resolution below what's needed to
> accomodate the current console size?

I haven't fixed that issue yet. One the last close of /dev/fb the tty 
should be set back to its original state before /dev/fb was first open.



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[James Simmons]

> So perhaps, a function something like this:
> 
> void fb_clip(struct fb_fillrect *region, struct fb_fillrect *clip);
> 
> This should not be difficult to implement, and I'll code it if everyone
> agrees.
 
I thought about this. Originally I did have this function but removed it.
WHat do you think Geert?

> The other option (which I don't like) is just to check the passed
> fb_var_screeninfo in the put_var ioctl against the current console
> window size.  But this is not foolproof as we will not be sure of the
> resulting window size _after_ the fb_set_var() call.

Yuck!!! The other way is better.



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Geert Uytterhoeven]

On Thu, 9 Jan 2003, James Simmons wrote:
> > So perhaps, a function something like this:
> > 
> > void fb_clip(struct fb_fillrect *region, struct fb_fillrect *clip);
> > 
> > This should not be difficult to implement, and I'll code it if everyone
> > agrees.
>  
> I thought about this. Originally I did have this function but removed it.
> WHat do you think Geert?

Yes, that's OK.

But I was most concerned about fb_ops.fb_imageblit(), since clipping impacts
fb_image.data as well. IIRC, all (most?) current clipping implementations
modify fb_image.{dx,dy,width,height} only, without updating fb_image.data.

> > The other option (which I don't like) is just to check the passed
> > fb_var_screeninfo in the put_var ioctl against the current console
> > window size.  But this is not foolproof as we will not be sure of the
> > resulting window size _after_ the fb_set_var() call.
> 
> Yuck!!! The other way is better.

Well, in between the calls to fb_ops.fb_check_var() and fb_ops.set_par() you
can still perform that check. But it's indeed ugly.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Antonino Daplas]

On Fri, 2003-01-10 at 05:24, James Simmons wrote:
> 
> > So perhaps, a function something like this:
> > 
> > void fb_clip(struct fb_fillrect *region, struct fb_fillrect *clip);
> > 
> > This should not be difficult to implement, and I'll code it if everyone
> > agrees.
>  
> I thought about this. Originally I did have this function but removed it.
> WHat do you think Geert?
> 
> > The other option (which I don't like) is just to check the passed
> > fb_var_screeninfo in the put_var ioctl against the current console
> > window size.  But this is not foolproof as we will not be sure of the
> > resulting window size _after_ the fb_set_var() call.
> 
> Yuck!!! The other way is better.
> 

Yes, I thought so too :-).

I think one scenario why we need clipping is changing bits_per_pixel,
ie, changing from 8->16 will drop your vyres by half and it's difficult
to determine this unless you do another check before the set_par(), as
Geert mentioned.  Worse still, there is little if no valid memory past
vyres but the console thinks there is.  That's where you will get a
segfault.

Whereas explicitly doing fbset -vyres 768 will not cause a segfault,
because you still have valid framebuffer memory past y=768.

Tony



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Geert Uytterhoeven]

On 10 Jan 2003, Antonino Daplas wrote:
> On Fri, 2003-01-10 at 05:24, James Simmons wrote:
> > > So perhaps, a function something like this:
> > > 
> > > void fb_clip(struct fb_fillrect *region, struct fb_fillrect *clip);
> > > 
> > > This should not be difficult to implement, and I'll code it if everyone
> > > agrees.
> >  
> > I thought about this. Originally I did have this function but removed it.
> > WHat do you think Geert?
> > 
> > > The other option (which I don't like) is just to check the passed
> > > fb_var_screeninfo in the put_var ioctl against the current console
> > > window size.  But this is not foolproof as we will not be sure of the
> > > resulting window size _after_ the fb_set_var() call.
> > 
> > Yuck!!! The other way is better.
> > 
> 
> Yes, I thought so too :-).
> 
> I think one scenario why we need clipping is changing bits_per_pixel,
> ie, changing from 8->16 will drop your vyres by half and it's difficult
> to determine this unless you do another check before the set_par(), as
> Geert mentioned.  Worse still, there is little if no valid memory past
> vyres but the console thinks there is.  That's where you will get a
> segfault.
> 
> Whereas explicitly doing fbset -vyres 768 will not cause a segfault,
> because you still have valid framebuffer memory past y=768.

That depends. On unified memory architectures or if the graphics memory is
shared by two heads, you may still corrupt someone's memory.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Geert Uytterhoeven]

On 10 Jan 2003, Antonino Daplas wrote:
> For putcs and putc, I just made it mandatory that when one character is
> to be out of bounds, it just won't get drawn.  A cleaner implementation
> is, of course, to clip the destination window + the passed bitmap.

Except for the geek value, why would it be cleaner to draw half of a character
near the border?

> Do we need to clip the fb_set_logo() too?

I don't think so. The current code already draws only the number of logos that
really fit on the screen. Unless you want to see half penguins as well :-)

> The patch may be a bit difficult to swallow because it is pretty much
> invasive and not so clean :-(, so it's okay if you don't take it.
> But it has the advantage of clipping the coordinates for hardware that 
> does not support clipping (like the i810fb).  For hardware that does 
> support clipping, or wants to implement its own clipping code, we require 
> a 'caps' (capabilities) field in fb_info or fb_fix_screeninfo.  This is 
> checked first and if set, the default clipping code is bypassed.

I prefer fb_info, since user space doesn't need to know.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds




-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[James Simmons]

> > I thought about this. Originally I did have this function but removed it.
> > WHat do you think Geert?
> 
> Yes, that's OK.
> 
> But I was most concerned about fb_ops.fb_imageblit(), since clipping impacts
> fb_image.data as well. IIRC, all (most?) current clipping implementations
> modify fb_image.{dx,dy,width,height} only, without updating fb_image.data.

Good point. Of course we could ignore pieces of data in that image instead 
of altering the data. Altering the entire data would be expensive.

> > > The other option (which I don't like) is just to check the passed
> > > fb_var_screeninfo in the put_var ioctl against the current console
> > > window size.  But this is not foolproof as we will not be sure of the
> > > resulting window size _after_ the fb_set_var() call.
> > 
> > Yuck!!! The other way is better.
> 
> Well, in between the calls to fb_ops.fb_check_var() and fb_ops.set_par() you
> can still perform that check. But it's indeed ugly.

Altering to the framebuffer via /dev/fb that would effect the tty should 
not remain after the final close of /dev/fb. Instead the console should 
reset to its previous state before we first opened /dev/fb.



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Geert Uytterhoeven]

On Fri, 10 Jan 2003, James Simmons wrote:
> > > I thought about this. Originally I did have this function but removed it.
> > > WHat do you think Geert?
> > 
> > Yes, that's OK.
> > 
> > But I was most concerned about fb_ops.fb_imageblit(), since clipping impacts
> > fb_image.data as well. IIRC, all (most?) current clipping implementations
> > modify fb_image.{dx,dy,width,height} only, without updating fb_image.data.
> 
> Good point. Of course we could ignore pieces of data in that image instead 
> of altering the data. Altering the entire data would be expensive.

Actually I thought about modifying the fb_image.data pointer only. But indeed,
for monochrome expansion you may have to modify the data, since pixel
boundaries in a bitmap don't correspond to byte boundaries.

Alternatively, if fb_image would countain sx and sy fields (cfr. fb_copyarea),
we can just alter (a copy of) sx and sy when clipping.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Antonino Daplas]

On Sat, 2003-01-11 at 03:39, James Simmons wrote:
> 
> > > I thought about this. Originally I did have this function but removed it.
> > > WHat do you think Geert?
> > 
> > Yes, that's OK.
> > 
> > But I was most concerned about fb_ops.fb_imageblit(), since clipping impacts
> > fb_image.data as well. IIRC, all (most?) current clipping implementations
> > modify fb_image.{dx,dy,width,height} only, without updating fb_image.data.
> 
> Good point. Of course we could ignore pieces of data in that image instead 
> of altering the data. Altering the entire data would be expensive.
> 
> > > > The other option (which I don't like) is just to check the passed
> > > > fb_var_screeninfo in the put_var ioctl against the current console
> > > > window size.  But this is not foolproof as we will not be sure of the
> > > > resulting window size _after_ the fb_set_var() call.
> > > 
> > > Yuck!!! The other way is better.
> > 
> > Well, in between the calls to fb_ops.fb_check_var() and fb_ops.set_par() you
> > can still perform that check. But it's indeed ugly.
> 
> Altering to the framebuffer via /dev/fb that would effect the tty should 
> not remain after the final close of /dev/fb. Instead the console should 
> reset to its previous state before we first opened /dev/fb.
> 

Right :-) I believe this is nearer the correct solution.  Clipping is
typically used to maximize graphics/video bandwidth, but fbcon is using
it to "cover up" possible bugs in the code.

If we can have a correctly working fbcon, without the clipping code, it
will be proof to its robustness.

So, basically, on the first open we save xres, yres, xres_virtual,
yres_virtual and bits_per_pixel, and restore them on the last close, is
this correct?  Any other parameters that will affect the actual and
virtual window size?

Tony





-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: clipping

[Antonino Daplas]

On Sat, 2003-01-11 at 04:31, James Simmons wrote:
> 
> > > > Except for the geek value, why would it be cleaner to draw half of a character
> > > > near the border?
> > > 
> > > This is one of the reasons why I moved from fbset to setting the console 
> > > resolution via the console layer. You can't end up with half characters. 
> > > Via fbset you could but fbcon had extra code to prevent such foolishness. 
> > > But it still looked silly to have half line on the bottom of the screen 
> > > that nothing ever drawed in.
> > 
> > What else can you do? My notebook has a fixed LCD of 1024x768. With the 12x22
> > font, I have a text console of 85x34, using only 1020x748 pixels. Shrinking the
> > resolution to 1020x748 is not an option.
> 
> Ug. I guess we handle it the best way we can. Also we can try BenH 
> suggesting of incorpating scaling. 
> 

Or we can modify fbcon_resize() so it just checks if the resulting
resolution is enough to accomodate the console window size...

static int fbcon_resize(struct vc_data *vc, unsigned int width, 
			unsigned int height)
{
	struct display *p = &fb_display[vc->vc_num];
	struct fb_info *info = p->fb_info;
	struct fb_var_screeninfo var = info->var;
	int err, x, y;

	var.xres = x = width * vc->vc_font.width;
	var.yres = y = height * vc->vc_font.height;
	var.activate = FB_ACTIVATE_TEST;
	err = fb_set_var(&var, info);
	if (err || var.xres < x || var.yres < y)
		return -EINVAL;

	var.activate = FB_ACTIVATE_NOW;
	err = fb_set_var(&var, info);
	p->vrows = info->var.yres_virtual/vc->vc_font.height;
	return err; 
}

...which implies that we also have to modify accel_clear_margins() so it
clears more than just a fraction of a character's width and height...

void accel_clear_margins(struct vc_data *vc, struct display *p,
				int bottom_only)
{
	struct fb_info *info = p->fb_info;
	unsigned int cw = vc->vc_font.width;
	unsigned int ch = vc->vc_font.height;
	unsigned int rw = info->var.xres - vc->vc_cols * cw;
	unsigned int bh = info->var.yres - vc->vc_rows * ch;
	unsigned int rs = info->var.xres - rw;
	unsigned int bs = info->var.yres - bh;
	struct fb_fillrect region;

	region.color = attr_bgcol_ec(p, vc);
	region.rop = ROP_COPY;

	if (rw & !bottom_only) {
		region.dx = info->var.xoffset + rs;
		region.dy = 0;
		region.width = rw;
		region.height = info->var.yres_virtual;
		info->fbops->fb_fillrect(info, &region);
	}

	if (bh) {
		region.dx = info->var.xoffset;
		region.dy = info->var.yoffset + bs;
		region.width = rs;
		region.height = bh;
		info->fbops->fb_fillrect(info, &region);
	}	
}	

... which exposed a bug in vc_resize() in vt.c.  In vc_resize(), the
global variables:

	video_num_lines;
	video_num_columns;
	video_size_row;
	screenbuf_size;

are modified before calling consw->con_resize() which, if it returns
with an error, are left with invalid numbers. So attached is a patch.

It behaves correctly now.  When the resulting resolution is bigger than
the console window size, it's left with a small window at the upper left
corner.  So stty must be called so rows and cols are enough but no
bigger than the supported resolution.

This works pretty well if panning is disabled.  With panning, I get a
cursor "dissynchronous" with the display and where the virtual screen is
randomly placed anywhere within the display window.

We might need to fix for scrolling + panning here.

Tony

diff -Naur linux-2.5.54/drivers/char/vt.c linux/drivers/char/vt.c
--- linux-2.5.54/drivers/char/vt.c	2003-01-11 04:47:02.000000000 +0000
+++ linux/drivers/char/vt.c	2003-01-11 04:47:49.000000000 +0000
@@ -732,24 +732,24 @@
 	if (new_cols == video_num_columns && new_rows == video_num_lines)
 		return 0;
 
-	newscreen = (unsigned short *) kmalloc(new_screen_size, GFP_USER);
-	if (!newscreen)
-		return -ENOMEM;
-
 	old_rows = video_num_lines;
 	old_cols = video_num_columns;
 	old_row_size = video_size_row;
 	old_screen_size = screenbuf_size;
 
+	err = resize_screen(currcons, new_cols, new_rows);
+	if (err) 
+		return err;
+
+	newscreen = (unsigned short *) kmalloc(new_screen_size, GFP_USER);
+	if (!newscreen)
+		return -ENOMEM;
+
 	video_num_lines = new_rows;
 	video_num_columns = new_cols;
 	video_size_row = new_row_size;
 	screenbuf_size = new_screen_size;
 
-	err = resize_screen(currcons, new_cols, new_rows);
-	if (err)
-		return err;
-
 	rlth = min(old_row_size, new_row_size);
 	rrem = new_row_size - rlth;
 	old_origin = origin;





-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com