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
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
