lkml.org 
[lkml]   [2005]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/1: 2.6.15-rc5-git3] Fixed and updated CyblaFB
    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-12-14 16:10    [W:0.063 / U:32.760 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site