Re: [ PATCH-LINK ] fb accel capabilities - take 2

Re: [ PATCH-LINK ] fb accel capabilities - take 2

[Thomas Winischhofer]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: Re: [Linux-fbdev-devel] [ PATCH-LINK ] fb accel capabilities - take 2 --]
[-- Type: message/rfc822, Size: 3753 bytes --]

From: Thomas Winischhofer <thomas@winischhofer.net>
To: David Eger <eger@theboonies.us>
Subject: Re: [Linux-fbdev-devel] [ PATCH-LINK ] fb accel capabilities - take 2
Date: Fri, 28 May 2004 13:27:33 +0200
Message-ID: <40B72225.4090901@winischhofer.net>

David Eger wrote:
> Thomas Winischhofer eloquently put:
> 
>>David Eger wrote:
>> > If the patch looks good to you (James, Geert, BenH), I'd like to
>> > forward it on to Linus followed by the individual driver updates
>> > (that'll take a little longer -- I need to revise how drivers do
>> > things like remember noaccel, etc. and update them).
>>
>>How about properly documenting what you actually do and let the driver
>>maintainer handle the implementation?
> 
> 
> see the update to skeletonfb.c

I saw that but no explanation on eg. the new PAN flags; what has hw 
accel with panning to do? Or do these just mean that the driver CAN pan?

>>General question (not to you only): How about considering the 2.6 series
>>as "stable" as everybody else does and keep it to fixing bugs instead of
>>change the driver API the Xth time?
> 
> 
> 
> If you want to handle the fix for sisfb, please do, I'll skip over that one ;-)

Yes please. However, you may very well do the changes yourself, but 
please mail them to me as the maintainer and not directly to the kernel 
maintainer.

> It's all part of fixing bugs ;-)  When my "accelerated" fb driver is still
> super-slow, i consider that a bug.  And it's a bug of the framebuffer
> subsystem, not really of the drivers.  Your lovely
> SiS310SubsequentScreenToScreenCopy() and friends (do I smell xfree86 code here?
> ;-) ) 

Yes, you do ;) (I wrote that, too, so it would have been a waste not 
recycling it)

> would probably work great if they were ever called.  Currently, they're
> not :-P

Erm, I wasn't aware of this. Interesting... Personally, since sisfb does 
y-panning I didn't see any speed regressions compared with 2.4 yet.

> The patch (a later version mailed to lkml against mainline) fixes an issue
> discussed in a large thread previously.  The Linux 2.6 fbdriver/kernel
> interface is opaque as to what hardware acceleration a video card *actually*
> provides.  There are a bunch of entry-points, but they're often done by
> software.  This patch fixes the framebuffer interface so that fbcon (currently
> the only app that really lives on top of the fb subsystem) can sanely choose
> its method of rendering.

I thought the acceleration features are all used opaquely (?) through 
the fb_ops structure... what's that one good for if I also need to set a 
flag for each supported function? The NULLs in the structure are to be 
filled out by the layer above using the non-accelerated functions.... Me 
thought.

Allright, keep on doing what you do. But please be more verbose in the 
include files as to what the flags actually mean.

Are these to set/cleared exclusively by the driver or must the driver 
expect them to change? (I mean for example that fbset in 2.4 could be 
used to disable acceleration, which is important to DirectFB)

Is there any driver already finished so that I can look it up somewhere?

Will an old driver (status as of now) work at all with the new flags not 
being touched? Will it compile? (That *should* work IMHO)

Thomas

-- 
Thomas Winischhofer
Vienna/Austria
thomas AT winischhofer DOT net          http://www.winischhofer.net/
twini AT xfree86 DOT org

Re: [ PATCH-LINK ] fb accel capabilities - take 2

[David Eger]

Quoting Thomas Winischhofer <thomas@winischhofer.net>:

> David Eger wrote:
> > Thomas Winischhofer eloquently put:
> >>How about properly documenting what you actually do and let the driver
> >>maintainer handle the implementation?
> >
> > see the update to skeletonfb.c
>
> I saw that but no explanation on eg. the new PAN flags; what has hw
> accel with panning to do? Or do these just mean that the driver CAN pan?

I'll add some comments to the patch :)

The intent for the FBINFO_HWACCEL_fnName flags is to say that the hardware will
be smart about it (i.e., it ought to be fast).  New fb drivers have been
filling in copyarea() with cfb_copyarea() (that is, the software version) or
more opaquely filling in copyarea() with their own mydriver_copyarea(), and
then conditionally calling cfb_copyarea() depending on the noaccel flag.

With the patch, there are a bunch of these "is X really hardware accelerated?"
flags in fbinfo->flags, as well as FBINFO_HWACCEL_DISABLED which should tell
the driver to not use its acceleration engine --- and use unaccelerated
versions of copyarea(), fillrect(), imageblit(); and FBINFO_MODULE (is the
driver loaded as a module?)

> > If you want to handle the fix for sisfb, please do, I'll skip over that one
> ;-)
>
> Yes please. However, you may very well do the changes yourself, but
> please mail them to me as the maintainer and not directly to the kernel
> maintainer.

Can do.  The one patch that touched everyone's driver just kept the new
flags from breaking things, as I changed FBINFO_FLAG_MODULE to FBINFO_MODULE.
I can leave a backwards-compatibility macro in fb.h, but it's sort of ugly, and
I'd just as soon touch all the drivers and get rid of the old #define 
completely :-/

> > SiS310SubsequentScreenToScreenCopy() and friends
> > would probably work great if they were ever called.  Currently, they're
> > not :-P
>
> Erm, I wasn't aware of this. Interesting... Personally, since sisfb does
> y-panning I didn't see any speed regressions compared with 2.4 yet.

So with my patch, you'd see a regression in speed until you flag that your
driver can do hardware panning.  I thought it would be good for our small
brains to put all the flags for this in one place instead of testing the
(x/y)panstep variables for zero...

> Allright, keep on doing what you do. But please be more verbose in the
> include files as to what the flags actually mean.
Will do in the next version of the patch (no one seems to have merged it yet, so
that's OK).  It seems to be getting good discussion, anyways ;-)

> Are these to set/cleared exclusively by the driver or must the driver
> expect them to change? (I mean for example that fbset in 2.4 could be
> used to disable acceleration, which is important to DirectFB)
*really*?  I was wondering about that.  I'll have to go see how that works...

> Is there any driver already finished so that I can look it up somewhere?
There's a patch for radeonfb (my video card ;-) )

> Will an old driver (status as of now) work at all with the new flags not
> being touched? Will it compile? (That *should* work IMHO)

/me nods.  It should. I tried compiling all of the drivers, and most of them
compiled.  The few drivers that didn't seemed to be pure 2.4 API code that no
one has loved for a long time :-/  Ah well..

-dte



-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click

fbset noaccel

[David Eger]

[ reads through fbset code ]

So it looks like fbset turns off acceleration by setting up the accel flag in
var (that FB_ACCELF_TEXT oddity).

While it makes sense that changing acceleration is a mode-switch item (quiescing
video engine and such), it seems more like it should be an ioctl (or sysfs!)
thing now...  Changing certain bits of var and hoping that set_var() will guess
in the correct way seems generally b0rken...

comments?

-dte


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click

Re: fbset noaccel

[Antonino A. Daplas]

On Saturday 29 May 2004 20:14, David Eger wrote:
> [ reads through fbset code ]
>
> So it looks like fbset turns off acceleration by setting up the accel flag
> in var (that FB_ACCELF_TEXT oddity).
>
> While it makes sense that changing acceleration is a mode-switch item
> (quiescing video engine and such), it seems more like it should be an ioctl
> (or sysfs!) thing now...  Changing certain bits of var and hoping that
> set_var() will guess in the correct way seems generally b0rken...
>
> comments?
>

It's not as bad as it seems.  As long as an accelerated driver does not ignore 
this flag and properly flushes->syncs->disables the accel engine on a 
set_var, it should work.  Plus, a lot of apps clear/depend on this flag 
(XFree86-fbdev, fbset, DirectFB, etc).

I agree though that turning off/syncing/flushing the accel engine deserves 
it's own ioctl/method so as to completely separate it from mode switching 
which is a different process in itself.

Tony




-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click

Re: [ PATCH-LINK ] fb accel capabilities - take 2

[Thomas Winischhofer]

David Eger wrote:
> I'll add some comments to the patch :)
> 
> The intent for the FBINFO_HWACCEL_fnName flags is to say that the hardware will
> be smart about it (i.e., it ought to be fast).  New fb drivers have been
> filling in copyarea() with cfb_copyarea() (that is, the software version) or
> more opaquely filling in copyarea() with their own mydriver_copyarea(), and
> then conditionally calling cfb_copyarea() depending on the noaccel flag.

Evidently.

> With the patch, there are a bunch of these "is X really hardware accelerated?"
> flags in fbinfo->flags, as well as FBINFO_HWACCEL_DISABLED which should tell
> the driver to not use its acceleration engine --- and use unaccelerated
> versions of copyarea(), fillrect(), imageblit(); and FBINFO_MODULE (is the
> driver loaded as a module?)

So far so good. Can't see a reason for partly disabling acceleration, 
but ok.

>>Erm, I wasn't aware of this. Interesting... Personally, since sisfb does
>>y-panning I didn't see any speed regressions compared with 2.4 yet.
> 
> 
> So with my patch, you'd see a regression in speed until you flag that your
> driver can do hardware panning.  I thought it would be good for our small
> brains to put all the flags for this in one place instead of testing the
> (x/y)panstep variables for zero...

There is another issue with panning. How does the new fbcon code decide 
how big the virtual screen shall be (if it cares at all)? Will there be 
a change in behavior?

Right now, this is more or less a hack: sisfb, unless explicitly told 
not to do so (nomax or noypan), will maximize the virtual y resolution 
on all var's it receives. DirectFB disables this "auto-maximizing" 
feature with a respective ioctl before changing the mode etc. Works 
fine. Dunno if other drivers do the same. Advantage is that users get 
the fastest possible console by default (without using fbset or the like 
to set the virtual screen size.)

Do I have to expect this mechanism to fail? IOW: (No, not io-write, in 
other words ;)) Will the panning stuff change in the fbcon layer 
breaking this?


>>Are these to set/cleared exclusively by the driver or must the driver
>>expect them to change? (I mean for example that fbset in 2.4 could be
>>used to disable acceleration, which is important to DirectFB)
> 
> *really*?  I was wondering about that.  I'll have to go see how that works...


Assumingly simple. Set/clears FB_ACCELF_TEXT. DirectFB uses the same 
technique. Your new flags will break both.


>>Is there any driver already finished so that I can look it up somewhere?
> 
> There's a patch for radeonfb (my video card ;-) )


I already saw that. Will take a closer look.


>>Will an old driver (status as of now) work at all with the new flags not
>>being touched? Will it compile? (That *should* work IMHO)
> 
> 
> /me nods.  It should. I tried compiling all of the drivers, and most of them
> compiled.  The few drivers that didn't seemed to be pure 2.4 API code that no
> one has loved for a long time :-/  Ah well..

(Assume you mean 2.5 API...?)

If they should compile you will need the mentioned compat-#define's in 
the long run...

Thomas

-- 
Thomas Winischhofer
Vienna/Austria
thomas AT winischhofer DOT net          http://www.winischhofer.net/
twini AT xfree86 DOT org


-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click

Re: [ PATCH-LINK ] fb accel capabilities - take 2

[Antonino A. Daplas]

On Sunday 30 May 2004 19:44, Thomas Winischhofer wrote:
> David Eger wrote:
> > I'll add some comments to the patch :)
> >
> > The intent for the FBINFO_HWACCEL_fnName flags is to say that the
> > hardware will be smart about it (i.e., it ought to be fast).  New fb
> > drivers have been filling in copyarea() with cfb_copyarea() (that is, the
> > software version) or more opaquely filling in copyarea() with their own
> > mydriver_copyarea(), and then conditionally calling cfb_copyarea()
> > depending on the noaccel flag.
>
> Evidently.
>
> > With the patch, there are a bunch of these "is X really hardware
> > accelerated?" flags in fbinfo->flags, as well as FBINFO_HWACCEL_DISABLED
> > which should tell the driver to not use its acceleration engine --- and
> > use unaccelerated versions of copyarea(), fillrect(), imageblit(); and
> > FBINFO_MODULE (is the driver loaded as a module?)
>
> So far so good. Can't see a reason for partly disabling acceleration,
> but ok.
>
> >>Erm, I wasn't aware of this. Interesting... Personally, since sisfb does
> >>y-panning I didn't see any speed regressions compared with 2.4 yet.
> >
> > So with my patch, you'd see a regression in speed until you flag that
> > your driver can do hardware panning.  I thought it would be good for our
> > small brains to put all the flags for this in one place instead of
> > testing the (x/y)panstep variables for zero...
>
> There is another issue with panning. How does the new fbcon code decide
> how big the virtual screen shall be (if it cares at all)? Will there be
> a change in behavior?

There shouldn't be a change in behavior.  The console virtual screen 
(display->vrows) is still based on var->yres_virtual, which the driver should 
set.  fbcon also checks if var->ypanstep != 0 if the driver can do panning or 
not.  The main intent of the capability flags is choosing the best scrolling 
mode.  So in pseudo_code:

if (cannot_pan && fb_read_is_very_slow)
	no_pan + redraw;
else if (cannot_pan && fb_read_is_fast)
	no_pan + move;
else if (can_pan && fb_read_is_very_slow)
	pan + redraw;
else /* pan + fb_read_is_fast */
	pan + move;

/* an accelerated copyarea == fb_read is fast */
/* With PCI cards, fb_read is magnitudes slower than fb_write */

Of course, pan+redraw is still not supported by fbcon, so this resolves to 
no_pan + redraw.

>
> Right now, this is more or less a hack: sisfb, unless explicitly told
> not to do so (nomax or noypan), will maximize the virtual y resolution
> on all var's it receives. DirectFB disables this "auto-maximizing"
> feature with a respective ioctl before changing the mode etc. Works
> fine. Dunno if other drivers do the same. Advantage is that users get

I think other drivers do the same.  But syntax might be different, ie (if 
var->yres_virtual == 0, then maximize var->yres_virtual, or something like 
that).

> the fastest possible console by default (without using fbset or the like
> to set the virtual screen size.)
>
> Do I have to expect this mechanism to fail? IOW: (No, not io-write, in
> other words ;)) Will the panning stuff change in the fbcon layer
> breaking this?
>
> >>Are these to set/cleared exclusively by the driver or must the driver
> >>expect them to change? (I mean for example that fbset in 2.4 could be
> >>used to disable acceleration, which is important to DirectFB)
> >

The accel capability bits should be set/cleared exclusively by the driver 
depending on it's capability (duh) or if var->accel_flags is set or not.  
 
> > *really*?  I was wondering about that.  I'll have to go see how that
> > works...
>
> Assumingly simple. Set/clears FB_ACCELF_TEXT. DirectFB uses the same
> technique. Your new flags will break both.
>

The new flags should not break DirectFB if the driver correspondingly sets/
clears the capability bits depending on the setting of var->accel_flags.

> >>Is there any driver already finished so that I can look it up somewhere?
> >
> > There's a patch for radeonfb (my video card ;-) )
>
> I already saw that. Will take a closer look.
>
> >>Will an old driver (status as of now) work at all with the new flags not
> >>being touched? Will it compile? (That *should* work IMHO)
> >
> > /me nods.  It should. I tried compiling all of the drivers, and most of
> > them compiled.  The few drivers that didn't seemed to be pure 2.4 API
> > code that no one has loved for a long time :-/  Ah well..
>
> (Assume you mean 2.5 API...?)
>
> If they should compile you will need the mentioned compat-#define's in
> the long run...
>
> Thomas




-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click