Re: [Linux-fbdev-devel] [PATCH] Tile Blitting

[Antonino D. <ad...@po...>]

On Thu, 2003-02-27 at 09:18, James Simmons wrote:
> > As Geert and DaveM has
> > mentioned to me, the current implementation might not be thread-safe
> > (although I see more of a concurrency problem between CPU and GPU).
> 
> I agree I see more of a problem with CPU GPU syncing issue. I do have a 
> fix in BK with allocating and deallocating continuely but it is the wrong
> approach.
> 

We can avoid concurrency problems by assigning different sections of the
buffer per call to fb_imageblit(). There is really no need to map/unmap
or allocate/deallocate buffers unless you do not trust the drivers to
behave :-).  The buffer will not be exposed to userland, unlike DRM
which has to implement "map->user access->unmap->pass to hardware".
 
> > Thus, the restriction that the buffer must be completely copied by the
> > driver before returning.  And because of this restriction, an extra copy
> > which might be unnecessary cannot be avoided (this was noted by Petr).
> > 
> > Treating the buffer as a ringbuffer, we eliminate these restrictions.
> 
> I didn't realize that the below was a ringbuffer implementation. The name
> threw me off. 

Well, it's not strictly a ringbuffer implementation.  This would require
a head and tail pointer where fbcon will adjust the tail and the
driver/hardware will adjust the head.  This will be very difficult to
implement in a device independent manner.  So we just cheat by issuing
an fb_sync() per loop to flush all pending commands.

> 
> Do you still have the original patch?
> 

Here's a revised one. Driver's can choose to fill up the following
structure, or leave it empty:

#define FB_PIXMAP_DEFAULT 1     /* used internally by fbcon */
#define FB_PIXMAP_SYSTEM  2     /* memory is in system RAM  */
#define FB_PIXMAP_IO      4     /* memory is iomapped       */
#define FB_PIXMAP_SYNC    256   /* set if GPU can DMA       */

struct fb_pixmap {
        __u8  *addr; 
	__u32 size;  
	__u32 offset;
	__u32 buf_align; 
	__u32 scan_align; 
	__u32 flags; 
	void (*outbuf)(u8 dst, u8 *addr); 
	u8   (*inbuf) (u8 *addr);
	unsigned long lock_flags;
	spinlock_t lock; 
};

The buffer can be anywhere, system or io.  If it's in special memory
(ie, offscreen graphics), access methods must be specified (outbuf,
inbuf).  If the buffer is DMA'able by the GPU, then FB_PIXMAP_SYNC must
be set (it issues an fb_sync()), otherwise leave it cleared (ie soft
accels). The buf_align and scan_align are hardware specific.  This will
let fbcon format the bitmap into a form acceptable by the hardware.  The
modified rivafb sets the alignment according to its needs, which greatly
simplified the rivafb_imageblit() function.

The spinlock may be necessary because fbcon_cursor, which is called via
timer or interrupt, might also use fb_imageblit().  You can change it to
a more appropriate locking method if you want.

The patch should work without driver breakage.  

Diff is against linux-2.5.61 + your fbdev.diff.gz + my accel_putcs
optimization patch + Geert's logo updates.  I know you already applied
them all in your tree.

Tony