Re: [Linux-fbdev-devel] Re: [PATCH 1/1: 2.6.15-rc5-git3] Fixed and updated CyblaFB

[Knut Petersen <Knut_Petersen@t-online.de>]

Antonino Daplas wrote:

> But current users of cyblafb will be affected if your patch
> does have a problem.
>
>  
They definitely will be affected when they lock their system
while trying rotation options ...

> > +    // That should never happen, but it would be fatal
> >    
>
> It won't :-)
>
>  
The graphics engine would not react kindly, and it does not really hurt.

> > +    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.
>
>  
Yes, I am a bit paranoid. That paranoia led to the discovery of some bugs
nobody knew or cared about. But you are right, this check might be a bit too
paranoid.

> 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.
>
>  
Well, you are shure that there is really not a single bug left in the 
bitmap construction
code? And that the code will never be touched again because it already 
is optimal? I
think support for all alignment possibilities will be handy in the near 
future, and
although it could be hidden by an #ifdef or stay a private patch, I 
prefer to include it.

Currently bitmap construction takes longer than blitting the image to 
the screen with
cyblafb, and I think I will have a very close look at that code soon.

BTW, something fundamental: Isn´t the pixelmap alignment really a 
property of the
image bitmap like the depth of the image data?

> > +    // 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?
>
>  
Do you really think that this is a good idea? I would like to ease the 
use of
e.g. fbset in scripts by returning -EINVAL when something as fundamental as
the selected xres is not acceptable. Ok, it´s always possible to parse 
the output
of fbset -s  in those cases.

> > +    if (var->xres_virtual % 8 != 0)
> > +        var->xres_virtual &= ~7;
> >    
>
> Or just var->xres_virtual &= ~7 without the if (...)
>  
Yes. That saves a few bytes.

> Wrong boolean check?  Should be if (vesafb & 4). Or might as
> well get rid of this check, it's redundant.
>
> Shouldn't this be if (vesafb & 4)?
>  
>
> and this...
>
>  
>
> and this...?
>  
No, no, no, no.

cu,
Knut
-
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/