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

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 ...

quoted

+    // 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.

quoted

+    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?

quoted

+    // 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.

quoted

+    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


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click