fb_write change in your tree

fb_write change in your tree

[Benjamin Herrenschmidt]

Hi James !

What is this change to fb_write() in your tree ? It looks very
wrong to me. You are removing the ability for the driver to
hook it's own fb_write function, which can be a problem. You
basically assume copy_from_user() can be used straight with
the framebuffer as a destination. This is broken. On ppc, for
example, copy_from_user() may do dcbz's cache instruction on
the destination. However, the fb is mapped uncacheable, that
will result in a lot of exceptions during the copy, which is
plain wrong (or possibly the copy failing completely).

What about drivers that are taking great care _NOT_ to map
the framebuffer (or at least not all of it) to avoid having
to ioremap hundreds of megabytes in kernel space ?

A driver should be able to never fill scree_base provided
that it has filled all the accel hooks and provides it's
own fb_read/write functions (which can be slow as they might
have to do mapping/unmapping of fb portions on demand, but
then, mmap is the preferred way for userland to access the
framebuffer content when that is possible).


Ben.




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: fb_write change in your tree

[James Simmons]

> Hi James !
> 
> What is this change to fb_write() in your tree ? It looks very
> wrong to me. You are removing the ability for the driver to
> hook it's own fb_write function, which can be a problem. You
> basically assume copy_from_user() can be used straight with
> the framebuffer as a destination. 

Look again. The idea was to grab the userland data and create struct 
fb_image then use xxxfb_imageblit to draw for you. We should never 
allow fb_write and fb_read to directly access the framebuffer.

Speaking of. We really need to cleanup the fb_write and crap.
The program flow should be:

fb_write -> xxxfb_imageblit -> fb_pixmap.outbuf();

The outbuf and inbuf commands should replace the fb_writeX and fb_readX 
stuff. This way driver developers can still use the cfb_xxx functions for 
strange hardware. a good example is the Epson 1335 chip. You can only 
access the framebuffer 16 bits at a time. Using 32 bit transfers will drop 
have the data. Why epson did this will never be know.

   The other bonus with using the imageblit method with fb_write is 
that we can treat planar modes a packed pixel modes :-)








-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: Re: fb_write change in your tree

[Geert Uytterhoeven]

On Tue, 23 Mar 2004, James Simmons wrote:
> > What is this change to fb_write() in your tree ? It looks very
> > wrong to me. You are removing the ability for the driver to
> > hook it's own fb_write function, which can be a problem. You
> > basically assume copy_from_user() can be used straight with
> > the framebuffer as a destination.
>
> Look again. The idea was to grab the userland data and create struct
> fb_image then use xxxfb_imageblit to draw for you. We should never
> allow fb_write and fb_read to directly access the framebuffer.
>
> Speaking of. We really need to cleanup the fb_write and crap.
> The program flow should be:
>
> fb_write -> xxxfb_imageblit -> fb_pixmap.outbuf();
>
> The outbuf and inbuf commands should replace the fb_writeX and fb_readX
> stuff. This way driver developers can still use the cfb_xxx functions for
> strange hardware. a good example is the Epson 1335 chip. You can only
> access the framebuffer 16 bits at a time. Using 32 bit transfers will drop
> have the data. Why epson did this will never be know.
>
>    The other bonus with using the imageblit method with fb_write is
> that we can treat planar modes a packed pixel modes :-)

Never thought about that one...

So if your application doesn't support the frame buffer format, it can use
write() instead of mmap() and just use packed pixels in a shadow frame buffer.

But what about read()? There's no imageget() method.

Do we want to support writev() as well? That would be useful for drawing
rectangular images.

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: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: Re: fb_write change in your tree

[James Simmons]

> > Look again. The idea was to grab the userland data and create struct
> > fb_image then use xxxfb_imageblit to draw for you. We should never
> > allow fb_write and fb_read to directly access the framebuffer.
> >
> > Speaking of. We really need to cleanup the fb_write and crap.
> > The program flow should be:
> >
> > fb_write -> xxxfb_imageblit -> fb_pixmap.outbuf();
> >
> > The outbuf and inbuf commands should replace the fb_writeX and fb_readX
> > stuff. This way driver developers can still use the cfb_xxx functions for
> > strange hardware. a good example is the Epson 1335 chip. You can only
> > access the framebuffer 16 bits at a time. Using 32 bit transfers will drop
> > have the data. Why epson did this will never be know.
> >
> >    The other bonus with using the imageblit method with fb_write is
> > that we can treat planar modes a packed pixel modes :-)
> 
> Never thought about that one...
> 
> So if your application doesn't support the frame buffer format, it can use
> write() instead of mmap() and just use packed pixels in a shadow frame buffer.

Yes!!!

> But what about read()? There's no imageget() method.

I have been thinking about that one. The best solution avoiding break the 
current api is to allow a struct fb_image into xxxfb_imageblit that lacks 
a data pointer. If it doesn't have a data pointer we then allocate memory
and transfer data from the framebuffer into the data buffer we have and 
then sent back to the user. 

> Do we want to support writev() as well? That would be useful for drawing
> rectangular images.

If we go with using imageblit for thsi we could. 

Note!!! The vga16fb and amiga driver would need to have be writing to read 
data from the framebuffer. I don't have a amiga to test it but I can do 
this for vga16fb. The only thing is I don't knowhow to do that for vga16. 
Anyone here no how?




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: Re: fb_write change in your tree

[Geert Uytterhoeven]

On Tue, 23 Mar 2004, James Simmons wrote:
> > >    The other bonus with using the imageblit method with fb_write is
> > > that we can treat planar modes a packed pixel modes :-)
> >
> > Never thought about that one...
> >
> > So if your application doesn't support the frame buffer format, it can use
> > write() instead of mmap() and just use packed pixels in a shadow frame buffer.
>
> Yes!!!
>
> > But what about read()? There's no imageget() method.

(sidenote: of course most applications using shadow frame buffers don't need to
 read from the real frame buffer)

> Note!!! The vga16fb and amiga driver would need to have be writing to read
> data from the framebuffer. I don't have a amiga to test it but I can do
> this for vga16fb. The only thing is I don't knowhow to do that for vga16.
> Anyone here no how?

As usual, I can take care of the Amiga driver.

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: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: fb_write change in your tree

[Benjamin Herrenschmidt]

> Look again. The idea was to grab the userland data and create struct 
> fb_image then use xxxfb_imageblit to draw for you. We should never 
> allow fb_write and fb_read to directly access the framebuffer.

Right. Note that fb_read is still using copy_to_user() with the
fb as a source, and that is hairy too.

But I would still like to allow the fb to just overwrite the whole
fb_read/write implementation. Also, your patch may break things if
userland did fb_write for an amount that covers several lines but
ends up in a middle of one afaik, it converts to images that are
rectangles, and doesn't properly deal with the possible first line
and last lines not beeing complete

> Speaking of. We really need to cleanup the fb_write and crap.
> The program flow should be:
> 
> fb_write -> xxxfb_imageblit -> fb_pixmap.outbuf();
> 
> The outbuf and inbuf commands should replace the fb_writeX and fb_readX 
> stuff. This way driver developers can still use the cfb_xxx functions for 
> strange hardware. a good example is the Epson 1335 chip. You can only 
> access the framebuffer 16 bits at a time. Using 32 bit transfers will drop 
> have the data. Why epson did this will never be know.
> 
>    The other bonus with using the imageblit method with fb_write is 
> that we can treat planar modes a packed pixel modes :-)
> 
> 
> 
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>



-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: fb_write change in your tree

[James Simmons]

> Right. Note that fb_read is still using copy_to_user() with the
> fb as a source, and that is hairy too.

yes. I haven't go to do that.

> But I would still like to allow the fb to just overwrite the whole
> fb_read/write implementation. 

I will think about keeping it. for now I'm experimenting.

> Also, your patch may break things if
> userland did fb_write for an amount that covers several lines but
> ends up in a middle of one afaik, it converts to images that are
> rectangles, and doesn't properly deal with the possible first line
> and last lines not beeing complete

Its not done properly :-( I was busy pushing patches mainline instead of 
perfecting it.




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: fb_write change in your tree

[Benjamin Herrenschmidt]

> Its not done properly :-( I was busy pushing patches mainline instead of 
> perfecting it.

Yup, and that's good :) It's more important to finish getting the
cursor stuff and drivers like rivafb & atyfb up to date in mainline
than doign that right now ;)

Ben.




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click