Performance improvement to vga arbitration

[Vignatti Tiago (Nokia-D/Helsinki)]

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?


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

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,

             Tiago