Performance improvement to vga arbitration
henry.zhao at oracle.com
Wed Jun 16 19:56:01 PDT 2010
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:
>>> 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:
>> 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
>> (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
>> 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
[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
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
* 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:
> 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,
> 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
More information about the xorg-devel