Re: [Linux-fbdev-users] [PATCH] cfbimgblt: Allow generic BitBLT functions to work with swapped pixel order in bytes.

[Pavel P. <pi...@cm...>]

Hello Antonino,

thanks for fast reply.

On Friday 31 August 2007 02:43, Antonino A. Daplas wrote:
> >
> > +#define FB_SHIFTED_PIXELS_MASK(index, bswapmask) ({ \
> > +		u32 _m; \
> > +		u32 _indx = (index); \
> > +		if (!bswapmask) { \
> > +			_m = FB_SHIFT_HIGH(~(u32)0, _indx); \
> > +		} else { \
> > +			_m = 0xff << FB_LEFT_POS(8); \
> > +			_m = FB_SHIFT_LOW(_m, _indx & (bswapmask)) & _m; \
> > +			_m = FB_SHIFT_HIGH(_m, _indx & ~(bswapmask)); \
> > +			_m |= FB_SHIFT_HIGH(~(u32)0, (_indx + bswapmask) & ~(bswapmask)); \
> > +		} \
> > +		_m; \
> > +	})
> > +
>
> If you can convert the above to to an inline function instead of a
> macro, that would be best.

I would prefer that as inline too. I have not been sure only,
if all GCC versions would optimize that equally well in such case.
When bswapmask is constantly zero, the else part should be eliminated.

> > +
> > +#ifdef CONFIG_FB_CFB_REV_PIXELS_IN_BYTE
> > +	if (bpp < 8) {
> > +		/*
> > +		 * Reversed order of pixel layout in bytes
> > +		 * works only for 1, 2 and 4 bpp
> > +		 */
> > +		bswapmask = 7 - bpp + 1;
> > +	}
> > +#endif
>
> And this too, if you can create a small inline function and move out the
> #ifdef out of the function, that would be nice also.

I am not sure there if this is not better documenting what is going
about there. But if you prefer to solve that as inline I change it.

Even if swapping support is enabled, I would like to allow control
actual swapping through some field in mode information and propagated
to
   struct fb_info->var or fix 
        struct fb_var_screeninfo
        struct fb_fix_screeninfo

have you idea for name and correct place for this SWAP_PIXELS_IN_BYTE
bit/flag.

> > If this is acceptable, I try to add support for rectangle
> > and copy in future, but this do not seems to be critical for
> > console. Normal fonts widths are multiples of 8 so problems
> > are not seen.
>
> BTW, if this driver will support only multiples of 8 font widths at less
> than 8bpp, you might want to advertise this limitation to the console,
> so it won't accidentally load an incompatible font.  The pertinent
> fields and function would be info->pixmap.blit_x and fb_get_caps().
>
> An example would be s3fb.  In tileblitting/text mode, it can only handle
> certain fonts (specifically 8x16 fonts)
>
> See s3fb.c and svgalib.c in drivers/video.

The extended cfbimgblt functions should fully handle all previous modes
+ 1, 2, 4 bit bpp modes with swapped pixels in byte. There should not be
any problem for arbitrarily images/fonts sizes. It cannot support 12
or 6 bit bpp with swapped pixels in bytes. I think, that LE to BE
conversions are only solutions for that.

The notice has been about rectangle fill and copy, which are in different
files. I am not sure if they are used for console such way that it would
could mean problems. The screen scroll should not be problem with
swapped pixels in bytes. Any operation on whole lines should not be problem
too. Only problem is if they are used for real moves with odd/unaligned
X coordinates. May it be - this would make some not so nice effects
for cursor when some strange font width is used.

Best wishes

            Pavel