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

Knut Petersen wrote:

Antonino Daplas wrote:

quoted

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

You can always divide the patch into bug fix and feature addition.
The bug fix goes to mainline, the feature addition goes into trial.

It's also easier to spot regressions if you divide the patch into
logical parts. For example, the recent atyfb update was composed of
10 different patches, each doing 1 logical thing.

quoted

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

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.

Well yes.  But this is a fast path that will affect performance.  And the
check will tend to hide bugs than reveal them (though you do have a printk
function).  And thirdly, cfb_imageblit does not support scan_align != 1.

quoted

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.

It's the same code whether you use scan_align of 1, 2, 4, 8, 16, etc. So,
I still don't see any benefit. You even confused akpm et al at what it does,
why you need to cast with u32. (It's because the accelerator needs u32
values, but supporting scan_align 1 and 2 means the bitmap becomes aligned
at u8 and u16 boundaries).

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?

No, it's the property of the accelerator.

quoted

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.

That has always been the (not often followed) rule - Round up if you can,
return -EINVAL if you can't.  Because with most user apps, finding the best
mode involves many iterations of FB_ACTIVATE_TEST, so that particular method
will be speeded up if you give working values rather than -EINVAL.  And yes,
properly working fb apps will check the contents of fb_var_screeninfo even if
the check_var returns success.

If your user will set something crazy like 799x600, might as well give him
an 800x600. As long as you use values that are higher than requested,
it won't hurt. 

Tony


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