Performance improvement to vga arbitration

[Henry Zhao]

On 06/14/10 10:53, Vignatti Tiago (Nokia-D/Helsinki) wrote:
> Hi Henry,
>
> On Sat, Jun 12, 2010 at 04:38:14AM +0200, ext Henry Zhao wrote:
>    
>> On 06/11/10 04:56, Tiago Vignatti wrote:
>>      
>>> http://people.freedesktop.org/~vignatti/tmp/0001-xfree86-vgaarb-disable-VGA-decoding-after-POST.patch
>>>
>>> At this point on the X server, we already POSTed all cards and we could be
>>> optimistic, letting the drivers say when we actually need VGA legacy. Right
>>> now, as you said, we're assuming legacy access as default whenever the system
>>> has more than one card, not driven by DRI (DRI and VGA legacy conflict with
>>> the current design. Dave preferred to let this away).
>>>        
>> The patch didn't go to git master yet ?
>>      
> not yet. Do you mind to do a proper review and insert your review tag there?
>
> For the X, we have a strict and rigorous development process now, trying to
> minimize the amount of errors being pushed upstream trees. For this we needed
> to set up a way get patches reviewed by stamping a review tag. In general it's
> not so different from what linux kernel is being doing. You may want to take a
> look on these wiki pages:
>
>      http://www.x.org/wiki/XServer
>      http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches
>    
>> We still need arbitration once a
>> device gets "graphics mode".  Say devices "a" and "b" are working in
>> "graphics mode", then
>>
>> (1) A new device "c" may start, which does legacy accesses during
>> initialization;
>> (2) While "b" is still running, device "a" may exit, which also involves
>> legacy accesses.
>>
>> Therefore we need to use another lock ("locks2"), in addition to the
>> current "locks"
>> (for legacy accesses), for non-legacy accesses such that "locks2" and
>> "locks" are in
>> conflict, but "locks2"s themselves are not.
>>
>> Besides, we cannot rule out possibilities that some drivers may use legacy
>> access in some operations in "graphics mode" in certain cases, therefore
>> we need
>> to give drivers a final say on kind of accesses they use for the
>> operations, although
>> the default is non-legacy access.
>>      
> right, I was missing the hotplug case. It makes perfect sense for me. And yes,
> I'd prefer to keep VGA decoding disabled by default after the session is up,
> exactly as my patch is doing.
>
>
>    
>>> Also, I posted two patches to fix the userspace decoding interface on the
>>> mailing list some days ago. If you're carrying about remove cards from
>>> arbitration and stop decoding then definitely you'll want to take a look on
>>> those:
>>>
>>> http://lists.x.org/archives/xorg-devel/2010-June/009559.html
>>>
>>>        
>> I had this fixed in my patch.
>>      
> can you please put your reviewing tags please?
>    

I noticed you have two patches lately:

[PATCH pciaccess 2/2] vgaarb: read back vga count when setting new 
decoding <http://lists.x.org/archives/xorg-devel/2010-June/009558.html>
[PATCH pciaccess resent 1/2] vgaarb: decode should send new information 
to the kernel <http://lists.x.org/archives/xorg-devel/2010-June/009559.html>

Can you send me them in email so that I can review them ?

>
>    
>> 3 patches are posted.
>>      
>
> Your explanation was good enough and clear for me. However, in all of the three
> patches, you're inserting more modifications than you described here. It makes
> the review difficult and tough.
>    

Actually the important change is in-kernel vgaarb driver where an 
arbitration
on locks2 (for non-legacy access) is introduced.  The other two patches,
though with a lot of code changes, only deal with the interface changes
that add requesting resource (rsrc) as an argument to the lock/unlock
functions such that:

* In libpciaccess layer,  we will have

     pci_device_vgaarb_lock(int rsrc);
     pci_device_vgaarb_trylock(int rsrc);
     pci_device_vgaarb_unlock(int rsrc);

* In X server, VGAGet(x),  VGAPut(x), and etc, will pass the right
     operation resource mask for each wrap function. This operation
     resource mask is used for lock/unlock before/after the execution
     of the function. The default is non-legacy access, but drivers can
     call xf86VGAarbiterDeviceOpsMask() to redefine.

So should be straightforward.


> A good habit is to split one in a series in which each patch has a different
> semantical meaning - one for cleaning up only, another introducing a hook
> function, another for the actual changes and so on. If possible, if each patch
> is independent from the other (able to compile) then it would also ease the
> search for some possible errors (using git-bisect)
>
> I'd please ask you to take a look on this page and re-send the patches, using
> a proper git-format-patch style:
>
>      http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches
>
>
> I have to admit that there are some parts of your kernel patch that simply
> goes being the scope of my knowledge. I'm pretty sure if you follow this tips
> on the wiki we're gonna be able to bring other guys from the community to do
> the review.
>    

Thank you for pointing that, I will do it.
>
> Thank you,
>
>               Tiago
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>