Knut Petersen wrote:
> With kernel 2.6.15-rc5-git3 all patches needed by this new version
> of cyblafb reached the main line kernel. So here it is.
>
> Main advantages:
> ============
> - vxres > xres support
> - ywrap support
> - much faster for almost all modes (e.g. 1280x1024-16bpp
> draws more than 41 full screens of text instead of about 25
> full screens of text per second on authors Epia 5000)
> - module init/exit code fixed
> - bugs triggered by console rotation fixed
> - lots of minor improvements
> - startup modes suitable for high performance scrolling
> in all directions
>
> As only one single graphics core is affected, there should be no
> reason not to include it in 2.6.15. No side effects are possible.
>
But current users of cyblafb will be affected if your patch
does have a problem.
> @@ -240,8 +247,8 @@ static void cyblafb_setup_GE(int pitch,i
>
> static int cyblafb_sync(struct fb_info *info)
> {
> - int status, i=100000;
> - while( ((status=in32(GE20)) & 0xFA800000) && i != 0)
> + int status,i=100000;
> + while( ((status=in32(GE20)) & 0xFe800000) && i != 0)
It's easier to read if you change your whitespace to something
like this...
int status, i = 100000;
while (((status = in32(GE20)) & 0xFE800000) && i != 0)
space after commas, before and after an operator. No space
here, in32(GE20), but put a space here, while (...)
> + s1 = point(ca->sx,0);
> + s2 = point(ca->sx+ca->width-1,ca->height-1);
> + d1 = point(ca->dx,0);
> + d2 = point(ca->dx+ca->width-1,ca->height-1);
> +
> + if ((ca->sy > ca->dy) || ((ca->sy == ca->dy) && (ca->sx >
> ca->dx)))
> + direction = 0;
> + else
> + direction = 2;
> +
> + out32(GEB8,basestride | ((ca->dy * info->var.xres_virtual *
> + info->var.bits_per_pixel) >> 6));
> + out32(GEC8,basestride | ((ca->sy * info->var.xres_virtual *
> + info->var.bits_per_pixel) >> 6 ));
> + out32(GE44,0xa0000000|1<<19|1<<2|direction);
> + out32(GE00,direction?s2:s1);
> + out32(GE04,direction?s1:s2);
> + out32(GE08,direction?d2:d1);
> + out32(GE0C,direction?d1:d2);
You replaced the tabs here with spaces.
>
> }
>
> @@ -355,26 +364,37 @@ static void cyblafb_copyarea(struct fb_i
> //
> // Cyberblade specific imageblit
> //
> -// Accelerated for the most usual case, blitting 1-bit deep character
> -// character images. Everything else is passed to the generic imageblit.
> +// Accelerated for the most usual case, blitting 1-bit deep
> +// character images. Everything else is passed to the generic imageblit
> +// unless it is so insane that it is better to printk an alert.
> //
> //=======================================================================
>
> static void cyblafb_imageblit(struct fb_info *info,
> const struct fb_image *image)
> {
> + u32 fgcol, bgcol, *pd = (u32 *) image->data;
>
> - u32 fgcol, bgcol;
> -
> - int i;
> int bpp = info->var.bits_per_pixel;
> - int index = 0;
> - int index_end=image->height * image->width / 8;
> - int width_dds=image->width / 32;
> - int width_dbs=image->width % 32;
>
> - if (image->depth != 1 || bpp < 8 || bpp > 32 || bpp % 8 != 0 ||
> - image->width % 8 != 0 || image->width == 0 || image->height ==
> 0) {
> + // Used only for drawing the penguine (image->depth > 1)
> + if (image->depth != 1) {
> + if (verbosity > 1)
> + output("imageblit depth = %d, dimen = %d x %d\n",
> + image->depth, image->width, image->height);
> + cfb_imageblit(info,image);
> + return;
> + }
> +
> + // That should never happen, but it would be fatal
It won't :-)
> + if (image->width == 0 || image->height == 0) {
> + output("imageblit: width/height 0 detected\n");
> + return;
> + }
> +
> + if (bpp < 8 || bpp > 32 || bpp % 8 != 0 ||
> + info->pixmap.scan_align > 4 ) {
Why this paranoid check? The check_var() function already
guaranteed that these conditions will not happen.
> + output("imageblit: invalid bpp or pixmap alignement\n");
> cfb_imageblit(info,image);
> return;
> }
> @@ -401,32 +421,32 @@ static void cyblafb_imageblit(struct fb_
> break;
> }
>
> - cyblafb_sync(info);
> -
> + out32(GEB8,basestride |
> + ((image->dy * info->var.xres_virtual * bpp) >> 6));
> out32(GE60,fgcol);
> out32(GE64,bgcol);
> out32(GE44,0xa0000000 | 1<<20 | 1<<19);
> - out32(GE08,point(image->dx,image->dy));
> - out32(GE0C,point(image->dx+image->width-1,image->dy+image->height-1));
> + out32(GE08,point(image->dx,0));
> + out32(GE0C,point(image->dx+image->width-1,image->height-1));
>
> - while(index < index_end) {
> - const char *p = image->data + index;
> - for(i=0;i<width_dds;i++) {
> - out32(GE9C,*(u32*)p);
> - p+=4;
> - index+=4;
> - }
> - switch(width_dbs) {
> - case 0: break;
> - case 8: out32(GE9C,*(u8*)p);
> - index+=1;
> - break;
> - case 16: out32(GE9C,*(u16*)p);
> - index+=2;
> - break;
> - case 24: out32(GE9C,*(u16*)p | *(u8*)(p+2)<<16);
> - index+=3;
> - break;
> + if (likely(info->pixmap.scan_align == 4)) {
> + int dds = ((image->width + 31) >> 5) * image->height;
> + while (dds--)
> + out32(GE9C,*pd++);
> + } else {
> + int i, j, dds = image->width / 32, bits = image->width % 32;
> + for (i=0; i<image->height; i++) {
> + for (j=0; j<dds; j++)
> + out32(GE9C, *pd++);
> + if(bits != 0) {
> + out32(GE9C, *pd);
> + if (info->pixmap.scan_align == 2)
> + pd = (u32*)((u32) pd +
> + (((bits + 15) >> 4) << 1) );
> + else
> + pd = (u32*)((u32) pd +
> + ((bits + 7) >> 3));
> + }
Do you really have to support scan_align 1 and 2? Why not just stick
with scan_align of 4, the code is so much easier to understand? I can't
find anything useful with this, even for debugging.
> + // try to be smart about (x|y)res(_virtual) problems.
> + //
> + if (var->xres % 8 != 0)
> return -EINVAL;
Isn't this too much? Why not var->xres = (var->xres + 7) & ~7?
> + if (var->xres > var->xres_virtual)
> + var->xres_virtual = var->xres;
> + if (var->yres > var->yres_virtual)
> + var->yres_virtual = var->yres;
> + if (bpp == 8 || bpp == 16) {
> + if (var->xres_virtual > 4088)
> + var->xres_virtual = 4088;
> + } else {
> + if (var->xres_virtual > 2040)
> + var->xres_virtual = 2040;
> + }
> + if (var->xres_virtual % 8 != 0)
> + var->xres_virtual &= ~7;
Or just var->xres_virtual &= ~7 without the if (...)
> @@ -1336,18 +1442,21 @@ static int __devinit cybla_pci_probe(str
> errout_findmode:
> iounmap(info->screen_base);
> errout_smem_remap:
> - release_mem_region(info->fix.smem_start,
> - info->fix.smem_len);
> - errout_smem_req:
> + if (!(vesafb & 4))
Wrong boolean check? Should be if (vesafb & 4). Or might as
well get rid of this check, it's redundant.
> + release_mem_region(info->fix.smem_start,
> + info->fix.smem_len);
> iounmap(io_virt);
> errout_mmio_remap:
> release_mem_region(info->fix.mmio_start,
> info->fix.mmio_len);
> errout_mmio_reqmem:
> -// release_region(0x3c0,32);
> + if (!(vesafb & 1))
> + release_region(0x3c0,32);
> errout_enable:
> + kfree(info->pixmap.addr);
> + errout_alloc_pixmap:
> framebuffer_release(info);
> - errout_alloc:
> + errout_alloc_info:
> output("CyblaFB version %s aborting init.\n",VERSION);
> return -ENODEV;
> }
> @@ -1359,9 +1468,15 @@ static void __devexit cybla_pci_remove(s
> unregister_framebuffer(info);
> iounmap(io_virt);
> iounmap(info->screen_base);
> - release_mem_region(info->fix.smem_start,info->fix.smem_len);
> + if (!(vesafb & 4))
Shouldn't this be if (vesafb & 4)?
> + release_mem_region(info->fix.smem_start,info->fix.smem_len);
> release_mem_region(info->fix.mmio_start,info->fix.mmio_len);
> fb_dealloc_cmap(&info->cmap);
> + if (!(vesafb & 2))
and this...
> + release_region(GEBase,0x100);
> + if (!(vesafb & 1))
and this...?
Tony
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
