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