Re: [PATCH libX11] _XDefaultError: set XlibDisplayIOError flag before calling exit

[Jamey Sharp]

On Feb 1, 2017 9:23 AM, "Arthur Huillet" <arthur.huil...@free.fr> wrote:

On Wed, 1 Feb 2017 09:12:47 -0800
Jamey Sharp <ja...@minilop.net> wrote:
> I'm a little confused by the notes in the commit message about whether
this
> is legal. I don't know why it should be considered legal to use a Display
> from a _fini or atexit handler… although this has been a source of bugs
for
> C++ apps in the past, so clearly people do it. I guess "there exist apps
in
> the wild that do this" is as good a definition of what's legal in Xlib as
> any. But if you have a more specific reference for why it's legal, it'd be
> nice to have that.

I am surprised that the notion that it is legal would require discussion.
It's not forbidden anywhere (that I'm aware of), therefore, it's allowed.
Doesn't that reasoning work?


I guess that's kind of a philosophical question, made more confusing by
Xlib's long history.

In my opinion, and speaking about software in general: no, I don't think
that's sound reasoning. :-) It leads to brittle software that relies on
accidents of implementation which may change without notice.

The story for Xlib is more complicated, because in practice, its
specification is not its documentation, but rather, "if it worked ten years
ago, it should probably work now." Of course this means Xlib is nearly
impossible to maintain. On a related note: many thanks to the people who
continue maintaining Xlib anyway. :-)

Yes, it sounds completely useless...


I never said that. :-) I just thought, from your use of the word "legal",
that you'd found explicit justification for this behavior in the
documentation. If you had, then there would be no question that a patch
like this is necessary, and that would be useful to call out in the commit
message.

As for clarification of the commit message, I thought that the paragraph
mentioning dlopen/dlclose was covering the problem.
What do you feel needs clarifying?


You said all that in the commit message but I didn't understand it. :-)
It's not super important I guess.

But: have you considered not using Xlib to issue requests, at least in your
_fini handler? I think it should always be safe to call XGetXCBConnection
and then issue your cleanup requests using XCB, which is carefully designed
to be safe in any context except signal handlers. That would make the
driver work on older Xlib installations too.

I'll also point out that I believe using a _fini handler to do X resource
cleanup is guaranteed to break if the application calls fork. Both the
parent and the child will attempt to clean up the resources, and one of
them will find the X connection is in the wrong state. If the other process
was still using that connection, it will probably hang next time it tries
to get a reply. Finding a different hook to do cleanup would be better.

I'll still offer a

Reviewed-by: Jamey Sharp <ja...@minilop.net>

because with Xlib, all we can do is harm reduction. If people insist on
doing things like this, we can at least try to minimize end-users'
suffering.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11] _XDefaultError: set XlibDisplayIOError flag before calling exit

[Jamey Sharp]

I don't love this either, but it's Xlib; there's little there to love. ;-)
I don't see anything fundamentally wrong with this approach.

I'm a little confused by the notes in the commit message about whether this
is legal. I don't know why it should be considered legal to use a Display
from a _fini or atexit handler… although this has been a source of bugs for
C++ apps in the past, so clearly people do it. I guess "there exist apps in
the wild that do this" is as good a definition of what's legal in Xlib as
any. But if you have a more specific reference for why it's legal, it'd be
nice to have that.

I also don't understand why OpenGL libraries specifically would need to
clean up X resources at disconnect, aside from the retain-permanent case.
Or did you mean that when the library is dlclosed you want to clean up when
the connection is not being closed? Perhaps you could clarify that in the
commit message.

Thanks,
Jamey

On Feb 1, 2017 6:02 AM,  wrote:

From: Arthur Huillet 

_XReply isn't reentrant, and it can lead to deadlocks when the default
error handler is called: _XDefaultError calls exit(1). It is called
indirectly by _XReply when a X protocol error comes in that isn't
filtered/handled by an extension or the application. This means that if the
application (or one of its loaded shared libraries such as the NVIDIA
OpenGL driver) has registered any _fini destructor, _fini will get called
while still on the call stack of _XReply. If the destructor interacts with
the X server and calls _XReply, it will hit a deadlock, looping on the
following in _XReply:
ConditionWait(dpy, dpy->xcb->reply_notify);

It is legal for an application to make Xlib calls during _fini, and that is
useful for an OpenGL driver to avoid resource leaks on the X server side,
for example in the dlopen/dlclose case. However, the driver can not readily
tell whether its _fini is being called because Xlib called exit, or for
another reason (dlclose), so it is hard to cleanly work around this issue
in the driver.

This change makes it so _XReply effectively becomes a no-op when called
after _XDefaultError was called, as though an XIOError had happened. The
dpy connection isn't broken at that point, but any call to _XReply is going
to hang. This is a bit of a kludge, because the more correct solution would
be to make _XReply reentrant, maybe by broadcasting the reply_notify
condition before calling the default error handler. However, such a change
would carry a grater risk of introducing regressions in Xlib.

This change will drop some valid requests on the floor, but this should not
matter, as it will only do so in the case where the application is dying: X
will clean up after it once exit() is done running. There is the case of
XSetCloseDownMode(RETAIN_PERMANENT), but an application using that and
wishing to clean up resources in _fini would currently be hitting a
deadlock, which is hardly a better situation.
---

I'm not in love with this change, to be honest, and if anyone has a better
suggestion I am eager to hear it. It is the best change I could think of,
in terms of making the problem go away without risking regressions.

Thanks
--
Arthur Huillet

 src/XlibInt.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/XlibInt.c b/src/XlibInt.c
index 9e67dff..1cb386d 100644
--- a/src/XlibInt.c
+++ b/src/XlibInt.c
@@ -1382,6 +1382,16 @@ int _XDefaultError(
XErrorEvent *event)
 {
 if (_XPrintDefaultError (dpy, event, stderr) == 0) return 0;
+
+/*
+ * Store in dpy flags that the client is exiting on an unhandled XError
+ * (pretend it is an IOError, since the application is dying anyway it
+ * does not make a difference).
+ * This is useful for _XReply not to hang if the application makes Xlib
+ * calls in _fini as part of process termination.
+ */
+dpy->flags |= XlibDisplayIOError;
+
 exit(1);
 /*NOTREACHED*/
 }
--
2.10.2
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: RFC: server-side XCB

[Jamey Sharp]

That's great! If Asalle and Jaya don't mind, I'd like to be CC'd on
progress reports and questions on these projects. I may be able to
help.

The length-checking/byte-swapping proposal sounds like what I had in
mind, and what Keith expressed interest in. I have some confusion
about some of the details, but I don't think they matter at this
stage; since we're talking about code generation, they'll be easy to
change later if necessary.

The task you've labeled server-side XCB is fairly ambitious, I think.
Sounds like that's roughly what Josh wants, details aside, and I was
going to complain about ambition at him too. :-) If Jaya is going to
do that, I think we should arrange an X.Org Extended Vacation of Code
grant for her.

Thanks for taking on mentorship of these tasks! Let me know how I can help.
Jamey

On Sun, Dec 21, 2014 at 12:36 AM, Christian Linhart
ch...@demorecorder.com wrote:
 There is already a project underway to do this.

 We would have sent the technical proposal in a few days.
 I will send the technical proposal as an extra message, soon.

 In any case we should avoid that two teams are working on that independently.

 We have two interns at Xorg who are working on this under my mentorship:
 * Asalle works as intern under the OPW-program ( 
 http://www.x.org/wiki/XorgOPW/ )
   She will do the length checking and byte swapping.

 * Jaya is an intern without any formal program behind it.
   She will implement server-side XCB in requests, replys etc,
   i.e. will replace the usage of the manually written proto-headers
   with using XCB.

 Currently, both are setting up their work environment and getting acquainted 
 with the code.
 Both already have gained some experience with XCB during their qualification 
 task etc.

 Me, I have 17 years experience with the X-Server source-code, especially with 
 protocol handling,
 from work in a commercial project.

 I have recently volunteered to become the maintainer of the XCB project.

 And I do understand the code of the XCB codegenerator. ;-)
 I have already made several changes to it, and more are in the pipeline.

 I also have made lots of changes of the XCB protocol definition of XInput, 
 with the goal of 100% coverage.
 I will also complete the protocol definitions of the XInput and XKeyboard 
 extensions'along with further changes in the XCB code generator.
 (actually already finished with this. Just need to prepare this for posting 
 on the XCB list.)

 Cheers,

 Chris


 On 12/20/14 21:27, Jamey Sharp wrote:

 We've talked about doing the xserver equivalent of XCB for years--that is, 
 auto-generating the protocol serialization and deserialization code from 
 XCB's machine-readable descriptions of the X protocol and extensions.

 Considering the recent CVEs in that code, I think it's time. So I want to 
 collect folks' thoughts on what server-side XCB should look like.

 At a high level, which code should we be generating? Off the top of my head, 
 I'm thinking the dispatch tables and all the swap procedures, as a first 
 target in order to get the codegen infrastructure merged and tested. Then 
 it'd be nice to generate code that validates arguments and returns 
 appropriate errors. Thoughts?

 XCB's code generator is implemented in Python. Can we make the same choice 
 in the xserver build process?

 Anything else I should be thinking about?
 Jamey



 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

RFC: server-side XCB

[Jamey Sharp]

We've talked about doing the xserver equivalent of XCB for years--that is,
auto-generating the protocol serialization and deserialization code from
XCB's machine-readable descriptions of the X protocol and extensions.

Considering the recent CVEs in that code, I think it's time. So I want to
collect folks' thoughts on what server-side XCB should look like.

At a high level, which code should we be generating? Off the top of my
head, I'm thinking the dispatch tables and all the swap procedures, as a
first target in order to get the codegen infrastructure merged and tested.
Then it'd be nice to generate code that validates arguments and returns
appropriate errors. Thoughts?

XCB's code generator is implemented in Python. Can we make the same choice
in the xserver build process?

Anything else I should be thinking about?
Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: Writing a new driver for nested Xorg servers based on Xephyr code

[Jamey Sharp]

On Sep 26, 2014 7:52 AM, Laércio de Sousa 
laercioso...@sme-mogidascruzes.sp.gov.br wrote:
 Now I've started playing with xf86-video-nested and Xephyr code, trying
to merge as much code as possible. It's still in an embrionary stage, so I
want to ask you some questions:

 * Is there an official position from X.Org development team about
deprecating Xephyr in favour of a driver based approach for nesting Xorg
server? Or at least let both approaches coexist (maybe moving common code
to a shared library, as I've asked you previously)?

Asking for an official position isn't especially effective in this
community, as we don't have anyone designated as a decision-maker. But
perhaps we can have a conversation about this question at the X.Org
Developers Conference in Bordeaux this week.

My personal opinion is that we might as well turn all the current X servers
into Xorg loadable drivers, but obviously we don't have a community-wide
consensus on that. ;-)

 * In case I decide to proceed with Xephyr code merging into a video
driver, should I do it in current project xf86-video-nested, or fork a new
one (e.g. xf86-video-ephyr)?

I would suggest doing it in the nested project, even if you wind up
rewriting it from scratch. There isn't much value in keeping the current
nested code if you get something Xephyr-like working.

I'm glad to see you picking this up!
Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: Running DRI3 on intel-gfx

[Jamey Sharp]

On Wed, Jun 04, 2014 at 06:58:47PM +0200, Sedat Dilek wrote:
 How do I see if DRI3 is enabled for mesa on my SandyBridge intel-gfxcard?
 
 glxinfo log-file attached...
 
 $ LC_ALL=C LIBGL_DEBUG=verbose glxinfo 2 /dev/null  /tmp/glxinfo.txt

You threw out the information you wanted by redirecting the debug output
to /dev/null. :-)

$ LC_ALL=C LIBGL_DEBUG=verbose glxinfo /dev/null
libGL: screen 0 does not appear to be DRI3 capable
libGL: pci id for fd 4: 8086:0d26, driver i965
...

I didn't realize that LIBGL_DEBUG=verbose would report that. It would
have been handy to know when I was trying to figure out the same thing a
few weeks ago.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] ephyr: Free damage structure at server reset time

[Jamey Sharp]

Xephyr could wrap CloseScreen after initializing kdrive, couldn't it? I
haven't looked carefully at that code, so I dunno.

Regardless:
Reviewed-by: Jamey Sharp ja...@minilop.net

On Wed, Jun 04, 2014 at 10:00:45PM -0700, Keith Packard wrote:
 The usual mechanism for freeing a damage structure when the pixmap is
 destroyed does not work for the screen pixmap as it isn't freed in the
 normal way.
 
 The existing driver cleanup function, scrfini, is called after the
 wrapped CloseScreen functions, including damageCloseScreen, are called
 and thus ephyr can't free the damage structure at that point.
 
 Deal with this by providing an early CloseScreen hook in KdCloseScreen
 which ephyr can use to free the damage structure before damage itself
 shuts down.
 
 Signed-off-by: Keith Packard kei...@keithp.com
 ---
  hw/kdrive/ephyr/ephyr.c | 6 ++
  hw/kdrive/ephyr/ephyr.h | 3 +++
  hw/kdrive/ephyr/ephyrinit.c | 2 ++
  hw/kdrive/src/kdrive.c  | 4 
  hw/kdrive/src/kdrive.h  | 1 +
  5 files changed, 16 insertions(+)
 
 diff --git a/hw/kdrive/ephyr/ephyr.c b/hw/kdrive/ephyr/ephyr.c
 index def50d8..49a4d2d 100644
 --- a/hw/kdrive/ephyr/ephyr.c
 +++ b/hw/kdrive/ephyr/ephyr.c
 @@ -756,6 +756,12 @@ ephyrScreenFini(KdScreenInfo * screen)
  }
  }
  
 +void
 +ephyrCloseScreen(ScreenPtr pScreen)
 +{
 +ephyrUnsetInternalDamage(pScreen);
 +}
 +
  /*  
   * Port of Mark McLoughlin's Xnest fix for focus in + modifier bug.
   * See https://bugs.freedesktop.org/show_bug.cgi?id=3030
 diff --git a/hw/kdrive/ephyr/ephyr.h b/hw/kdrive/ephyr/ephyr.h
 index 34ce460..e691780 100644
 --- a/hw/kdrive/ephyr/ephyr.h
 +++ b/hw/kdrive/ephyr/ephyr.h
 @@ -131,6 +131,9 @@ void
   ephyrScreenFini(KdScreenInfo * screen);
  
  void
 +ephyrCloseScreen(ScreenPtr pScreen);
 +
 +void
   ephyrCardFini(KdCardInfo * card);
  
  void
 diff --git a/hw/kdrive/ephyr/ephyrinit.c b/hw/kdrive/ephyr/ephyrinit.c
 index fac84cd..3f66e1c 100644
 --- a/hw/kdrive/ephyr/ephyrinit.c
 +++ b/hw/kdrive/ephyr/ephyrinit.c
 @@ -430,4 +430,6 @@ KdCardFuncs ephyrFuncs = {
  
  ephyrGetColors, /* getColors */
  ephyrPutColors, /* putColors */
 +
 +ephyrCloseScreen,   /* closeScreen */
  };
 diff --git a/hw/kdrive/src/kdrive.c b/hw/kdrive/src/kdrive.c
 index 9814fc6..b5b91c0 100644
 --- a/hw/kdrive/src/kdrive.c
 +++ b/hw/kdrive/src/kdrive.c
 @@ -621,8 +621,12 @@ KdCloseScreen(ScreenPtr pScreen)
  KdCardInfo *card = pScreenPriv-card;
  Bool ret;
  
 +if (card-cfuncs-closeScreen)
 +(*card-cfuncs-closeScreen)(pScreen);
 +
  pScreenPriv-closed = TRUE;
  pScreen-CloseScreen = pScreenPriv-CloseScreen;
 +
  if (pScreen-CloseScreen)
  ret = (*pScreen-CloseScreen) (pScreen);
  else
 diff --git a/hw/kdrive/src/kdrive.h b/hw/kdrive/src/kdrive.h
 index bec75cb..08b1681 100644
 --- a/hw/kdrive/src/kdrive.h
 +++ b/hw/kdrive/src/kdrive.h
 @@ -130,6 +130,7 @@ typedef struct _KdCardFuncs {
  void (*getColors) (ScreenPtr, int, xColorItem *);
  void (*putColors) (ScreenPtr, int, xColorItem *);
  
 +void (*closeScreen) (ScreenPtr);/* close ScreenRec */
  } KdCardFuncs;
  
  #define KD_MAX_PSEUDO_DEPTH 8
 -- 
 2.0.0.rc4
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: Alternative approach for a nested-xserver based video driver

[Jamey Sharp]

On Wed, Jun 04, 2014 at 09:07:22AM -0300, Laércio de Sousa wrote:
 Hello there!
 
 Some time ago I've wrotten asking for current status of xf86-video-nested
 development. I believe that, for a more robust single-card multiseat setup
 with systemd-logind, a real Xorg server with some kind of nested video
 driver works better than Xephyr, since it still lacks proper input
 hotplugging, for example.
 
 On the other hand, xf86-video-nested have received no relevant improvements
 for years, while Xephyr graphics support development is quite active.

Wow, I can't believe that capstone project was almost three years ago
already.

 So I'm thinking on rewriting xf86-video-nested driver based on latest
 Xephyr code. A more ambicious idea is to identify and move all video
 related code that could be useful for both Xephyr and nested driver to a
 shared library, namely libephyr, and link them against it. We could even
 rename xf86-video-nested to xf86-video-ephyr to reflect the new approach.
 
 I have absolutely no experience in writing video drivers for Xorg, but I'm
 open for learning. Any feedback from you will be welcome.

In my opinion, this would be great. One of my long-term goals is to have
only one X server implementation that anyone cares about, so being able
to replace both Xnest and Xephyr with Xorg+video-ephyr sounds good to
me. (And ideally, Xdmx would die in a fire.)

If I had my way, you wouldn't need to build a shared library, because
you'd just replace Xephyr. But practicality suggests that your shared
library plan is a better migration strategy.

I might suggest that you try hacking stuff into video-nested by
copy-paste before you try to figure out what shared library API you
need, though. It's much easier to make progress through incrementally
testable changes.

I hope this helps. :-)
Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: Alternative approach for a nested-xserver based video driver

[Jamey Sharp]

I'm almost entirely in support of this plan too :-) except I don't think
it will ever make sense to do things like video-nested or VNC as any
combination of video-modesetting and libinput.

But for everything else, yes, absolutely, throw that code away. :-)

Jamey

On Wed, Jun 04, 2014 at 12:19:41PM -0400, Jasper St. Pierre wrote:
 And on the other side, you have people like me who simply want to replace
 all video drivers with -modesetting, and all input drivers with libinput :)
 
 
 On Wed, Jun 4, 2014 at 12:16 PM, Jamey Sharp ja...@minilop.net wrote:
 
  On Wed, Jun 04, 2014 at 09:07:22AM -0300, Laércio de Sousa wrote:
   Hello there!
  
   Some time ago I've wrotten asking for current status of xf86-video-nested
   development. I believe that, for a more robust single-card multiseat
  setup
   with systemd-logind, a real Xorg server with some kind of nested video
   driver works better than Xephyr, since it still lacks proper input
   hotplugging, for example.
  
   On the other hand, xf86-video-nested have received no relevant
  improvements
   for years, while Xephyr graphics support development is quite active.
 
  Wow, I can't believe that capstone project was almost three years ago
  already.
 
   So I'm thinking on rewriting xf86-video-nested driver based on latest
   Xephyr code. A more ambicious idea is to identify and move all video
   related code that could be useful for both Xephyr and nested driver to a
   shared library, namely libephyr, and link them against it. We could
  even
   rename xf86-video-nested to xf86-video-ephyr to reflect the new approach.
  
   I have absolutely no experience in writing video drivers for Xorg, but
  I'm
   open for learning. Any feedback from you will be welcome.
 
  In my opinion, this would be great. One of my long-term goals is to have
  only one X server implementation that anyone cares about, so being able
  to replace both Xnest and Xephyr with Xorg+video-ephyr sounds good to
  me. (And ideally, Xdmx would die in a fire.)
 
  If I had my way, you wouldn't need to build a shared library, because
  you'd just replace Xephyr. But practicality suggests that your shared
  library plan is a better migration strategy.
 
  I might suggest that you try hacking stuff into video-nested by
  copy-paste before you try to figure out what shared library API you
  need, though. It's much easier to make progress through incrementally
  testable changes.
 
  I hope this helps. :-)
  Jamey
 
  ___
  xorg-devel@lists.x.org: X.Org development
  Archives: http://lists.x.org/archives/xorg-devel
  Info: http://lists.x.org/mailman/listinfo/xorg-devel
 
 
 
 
 -- 
   Jasper


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

OML_sync_control semantics

[Jamey Sharp]

[We should probably have moved this conversation to the piglit list when
it shifted to discussing these new OML_sync_control tests.]

On Wed, May 14, 2014 at 04:12:32PM +0100, Chris Wilson wrote:
 On Wed, May 14, 2014 at 06:49:10AM -0700, Jamey Sharp wrote:
  On Wed, May 14, 2014 at 01:31:14PM +0100, Chris Wilson wrote:
   On Tue, May 13, 2014 at 09:20:45AM -0700, Jamey Sharp wrote:
   Yeah, we have new tests that haven't quite been merged yet as we're
   addressing Eric's review comments. Our current version, revised 
yesterday,
   is available here:

   [2]https://github.com/ThirteenFish/piglit

   Make sure you do a full 'piglit-run.py -t OML_sync_control ...' as 
we run
   the new tests in a variety of configurations, and the full-screen 
ones are
   particularly interesting here. Though IIRC, SNA fails some of the 
others
   as well.
   
   Looks like there are quite a few bugs remaining in the test cases.
  
  Thanks for reviewing them, Chris!
  
   glXGetSyncValuesOML returns the current hardware frame counter and time
   of last frame update, not the last client swap frame or swap time. You
   make an assumption in the timing loop that the calls to SwapBuffers
   and GetSyncValues are instantaneous and do not account for a vblank
   interrupt that can happen in between updating the hardware counters.
  
  We do make that assumption, but if the assumption is violated we only
  emit a warning.
 
 It's a FAIL currently.

Hmm, right. I've rewritten those checks to just verify that alternating
between calls to WaitForSbc and GetSyncValues always have monotonically
increasing values for both UST and MSC. That's clearly required by the
spec, and still catches the driver bugs I was concerned about.

Regarding the behavior of divisor==0: I had to think very hard about
what you were saying, what the spec says, what the X.Org implementation
does, whether SwapInterval affects any of the above, etc. (In case
anyone wondered, I did not enjoy that.) Theo and I have concluded that
you're right, regardless of what SwapInterval is set to. (It was
confusing since a non-zero SwapInterval would in some cases make
divisor==0 sync to vblank almost as if you'd set divisor==swap_interval,
but not in all cases.)

As a result, we've changed timing.c to never use divisor==0, because we
want to validate that the driver syncs to vblank when required, and as
you say, the spec only requires that if divisor0. Instead, when testing
in -msc-delta mode we set divisor=1, to specify that if the swap misses
our target_msc, it should wait until the next vblank.

We're also retracting the repeat-swapbuffers test. There may still be
utility in a test like it but it isn't testing anything we actually care
about and we don't want to think about it further.

You can review/test these changes and others, currently in separate
patches, at https://github.com/ThirteenFish/piglit. We'll rebase them
into the series when we send it out for review again.

   The computation of interframe jitter is wonky, thanks to using
   (new_timestamp - -1) on the first pass.
  
  Oops, we broke that while addressing Eric's review comments on Monday.
  We'll fix it.

Now fixed in the above git repo.

Thanks,
Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] DRI2: By default, throttle after two pending swaps.

[Jamey Sharp]

On Wed, May 14, 2014 at 01:31:14PM +0100, Chris Wilson wrote:
 On Tue, May 13, 2014 at 09:20:45AM -0700, Jamey Sharp wrote:
 Yeah, we have new tests that haven't quite been merged yet as we're
 addressing Eric's review comments. Our current version, revised 
  yesterday,
 is available here:
  
 [2]https://github.com/ThirteenFish/piglit
  
 Make sure you do a full 'piglit-run.py -t OML_sync_control ...' as we run
 the new tests in a variety of configurations, and the full-screen ones 
  are
 particularly interesting here. Though IIRC, SNA fails some of the others
 as well.
 
 Looks like there are quite a few bugs remaining in the test cases.

Thanks for reviewing them, Chris!

 glXGetSyncValuesOML returns the current hardware frame counter and time
 of last frame update, not the last client swap frame or swap time. You
 make an assumption in the timing loop that the calls to SwapBuffers
 and GetSyncValues are instantaneous and do not account for a vblank
 interrupt that can happen in between updating the hardware counters.

We do make that assumption, but if the assumption is violated we only
emit a warning.

And it would be very surprising if a correctly-functioning driver
violated that assumption, because WaitForMsc/Sbc are required to return
as soon as possible once the swap completes. So we should have something
like 16ms to issue the follow-up GetSyncValues before the values will
change.

We included that warning because drivers call DRI2SwapComplete with
zeroes for UST and MSC, which is clearly always wrong, and this also
lets us continue on to test whether they're at least syncing to vblank.

 The computation of interframe jitter is wonky, thanks to using
 (new_timestamp - -1) on the first pass.

Oops, we broke that while addressing Eric's review comments on Monday.
We'll fix it.

(Theo, if I don't get to it first: both the if and else starting on line
182 need to be guarded with (last_timestamp = 0). We'll have to deal
with the 80-column limit some other way.)

 glXSwapBuffersMscOML: If divisor = 0,
 the swap will occur when MSC becomes greater than or equal to
 target_msc.

Yes, except:

If there are multiple outstanding swaps for the same window, at most
one such swap can be satisfied per increment of MSC.

 The minimum expected_msc is therefore the target_msc in many cases and
 no time may pass at all during the timing loop if msc-delta=0.

Note that timing.c's main() prohibits using WaitForMsc without also
setting either msc-delta or divisor, because that combination would fail
in the way you describe. However, because of the above-quoted sentence
in the spec, we believe that SwapBuffersMsc must behave as if we'd set
msc-delta non-zero.

In the open-source implementations, swap_interval defaults to 1 and so
it does behave this way. If you override swap_interval to 0, you get
non-conforming behavior, but I don't believe that's a test bug.

 In repeat-swapbuffers.c, you query the current hw counters before the
 swap may have completed,

That's because that test's purpose is, as the file comment says,
Verifies that glXSwapBuffersMscOML does not increment SBC more often
than MSC. In that test we don't want to wait for any swaps to complete.

 and runs afoul of the divisor==0 logic above.

Since that test only uses SwapBuffersMsc, the same reasoning applies.

Again, thanks for looking this over! Did you catch anything else when
you ran the new tests?

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] DRI2: By default, throttle after two pending swaps.

[Jamey Sharp]

On Tue, May 13, 2014 at 12:01 AM, Chris Wilson ch...@chris-wilson.co.uk wrote:
 On Mon, May 12, 2014 at 06:30:47PM -0700, Jamey Sharp wrote:
 According to Chris Wilson, the Intel driver has a horrifying kludge to
 get triple-buffering despite the swap_limit being set to only allow
 double-buffering by default. This kludge makes OML_sync_control unusable
 if the Intel driver goes into page-flipping mode.

 Since triple-buffering is preferred for most applications, let's make it
 the default in the server and allow the drivers to quit lying to the X
 server about when swaps complete.

 No, it is not that simple. GetBuffers() can only be allowed to proceed
 after the driver has arranged for the call to return the right buffers
 to the client. In the event that a client scheduled a future swap, most
 drivers will defer the buffer exchange until the swap is actually
 performed - limiting the client to double buffering to avoid
 complications in reporting the wrong buffers.

Alright. How do you believe the Intel driver's buggy and unusable
implementation of OML_sync_control should be fixed?

I'd also be delighted to see patches to make SNA pass the new Piglit
tests. I tried to fix that before I turned to UXA but I got lost in
too many different DRI2SwapComplete calls with spec-violating
parameters before I even got to checking whether it was reliably
syncing to vertical retrace. I'd be happy to send along the one patch
I could figure out if you want it though.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] DRI2: By default, throttle after two pending swaps.

[Jamey Sharp]

On May 13, 2014 12:57 AM, Chris Wilson ch...@chris-wilson.co.uk wrote:
 =0 ickle:/usr/src/piglit$ ./bin/glx-oml-sync-control-getmscrate
 PIGLIT: {'result': 'pass' }
 =0 ickle:/usr/src/piglit$ ./bin/glx-oml-sync-control-waitformsc
 PIGLIT: {'result': 'pass' }
 =0 ickle:/usr/src/piglit$
./bin/glx-oml-sync-control-swapbuffersmsc-divisor-zero
 PIGLIT: {'result': 'pass' }
 =0 ickle:/usr/src/piglit$ ./bin/glx-oml-sync-control-swapbuffersmsc-return
 Testing with default swap interval
 PIGLIT: {'result': 'pass' }

 Am I missing something?

Yeah, we have new tests that haven't quite been merged yet as we're
addressing Eric's review comments. Our current version, revised yesterday,
is available here:

https://github.com/ThirteenFish/piglit

Make sure you do a full 'piglit-run.py -t OML_sync_control ...' as we run
the new tests in a variety of configurations, and the full-screen ones are
particularly interesting here. Though IIRC, SNA fails some of the others as
well.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] DRI2: By default, throttle after two pending swaps.

[Jamey Sharp]

According to Chris Wilson, the Intel driver has a horrifying kludge to
get triple-buffering despite the swap_limit being set to only allow
double-buffering by default. This kludge makes OML_sync_control unusable
if the Intel driver goes into page-flipping mode.

Since triple-buffering is preferred for most applications, let's make it
the default in the server and allow the drivers to quit lying to the X
server about when swaps complete.

I suggest this patch be back-ported to stable releases. Since the Intel
driver, at least, is already forcing applications to use triple
buffering regardless of their SwapLimit setting, it must be safe.

Signed-off-by: Jamey Sharp ja...@minilop.net
Signed-off-by: Theo Hill theo0...@gmail.com
Cc: Chris Wilson ch...@chris-wilson.co.uk
Cc: Eric Anholt e...@anholt.net
Cc: Matt Dew mar...@osource.org
---
 hw/xfree86/dri2/dri2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 61bf85e..bff41dc 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -217,7 +217,7 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
 if (!ds-GetMSC || !(*ds-GetMSC) (pDraw, ust, pPriv-last_swap_target))
 pPriv-last_swap_target = 0;
 
-pPriv-swap_limit = 1;  /* default to double buffering */
+pPriv-swap_limit = 2;  /* default to triple buffering */
 pPriv-last_swap_msc = 0;
 pPriv-last_swap_ust = 0;
 xorg_list_init(pPriv-reference_list);
-- 
1.9.2

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Fix XNextRequest() after direct usage of XCB

[Jamey Sharp]

On Sat, May 10, 2014 at 02:55:11PM +0200, Uli Schlachter wrote:
 On 10.05.2014 00:21, otay...@redhat.com wrote:
  From: Owen W. Taylor otay...@fishsoup.net
  
  When XCB owns the X socket, dpy-request is not updated, so
  NextRequest() and XNextRequest() return the wrong value. There's
  nothing we can do to fix NextRequest() while retaining ABI compat,
  but change XNextRequest() to grab the socket back from XCB,
  updating dpy-request.
  
  Signed-off-by: Owen W. Taylor otay...@fishsoup.net
 
 Reviewed-by: Uli Schlachter psyc...@znc.in
 
 (And this also makes this function thread safe on 32bit archs were reading a
 64bit value isn't atomic)

I don't have any issues with the patch, aside from possibly making
people think that using NextRequest is a good idea. :-)

But this comment isn't true; neither NextRequest nor XNextRequest can
ever be made thread-safe in any implementation, as you could always race
with another thread that's issuing a request, which invalidates
NextRequest's post-conditions.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] Warn on DRI2SwapComplete with constant UST/MSC.

[Jamey Sharp]

And same for DRI2WaitMSCComplete.

OML_sync_control requires glXWaitForSbcOML and glXWaitForMscOML to
return the correct values of UST and MSC as of the time that the wait
completed. Those values cannot possibly be compile-time constants, but
drivers frequently pass 0 for one or both values.

This patch uses GCC's __builtin_constant_p to test at compile time
whether either of those counters is being provided invalid constant
values at any call site.

Based on a quick Google search, I believe the same syntax is provided by
both clang and Sun Studio, if anyone wants to support this validation on
those compilers. However, this is a tool to help driver developers find
bugs in their drivers, not code required for correctness or performance,
so it's not terrible if it doesn't get ported to other compilers.

If any caller does pass constants to these functions, then this patch
inserts a call to _DRI2WarnConstantUSTOrMSC before the real call, but
only calls it once per call site to avoid log spam. That function logs
the file, line, function, and the bogus UST/MSC values used, so we can
tell from users' logs which wrong paths they're actually hitting. These
log entries look something like:

(EE) intel(0): [DRI2] sna_dri.c:2200:sna_dri_schedule_swap: driver incorrectly 
provided a constant UST (0) or MSC (0)

I believe the introduction of _DRI2WarnConstantUSTOrMSC doesn't count as
an ABI break under current xserver rules, because drivers compiled
against a version of the server headers without this patch will run fine
against a newer xserver. However, the reverse isn't true.

Finally, _DRI2WarnConstantUSTOrMSC is declared with GCC's warning
attribute, to provide a custom warning message at compile time for each
bogus call. (I'd have made it a compile-time error but I prefer not to
be made an example of by angry users and distros.) I can't tell
whether any other compiler supports this attribute, so it might be that
only the run-time log entries can be made to work on other compilers.

Signed-off-by: Jamey Sharp ja...@minilop.net
Cc: Theo Hill theo0...@gmail.com
---
 hw/xfree86/dri2/dri2.c | 21 -
 hw/xfree86/dri2/dri2.h | 43 +++
 2 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 76708ca..61bf85e 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -957,8 +957,8 @@ DRI2CanExchange(DrawablePtr pDraw)
 }
 
 void
-DRI2WaitMSCComplete(ClientPtr client, DrawablePtr pDraw, int frame,
-unsigned int tv_sec, unsigned int tv_usec)
+(DRI2WaitMSCComplete)(ClientPtr client, DrawablePtr pDraw, int frame,
+  unsigned int tv_sec, unsigned int tv_usec)
 {
 DRI2DrawablePtr pPriv;
 
@@ -1015,9 +1015,9 @@ DRI2WakeClient(ClientPtr client, DrawablePtr pDraw, int 
frame,
 }
 
 void
-DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, int frame,
- unsigned int tv_sec, unsigned int tv_usec, int type,
- DRI2SwapEventPtr swap_complete, void *swap_data)
+(DRI2SwapComplete)(ClientPtr client, DrawablePtr pDraw, int frame,
+   unsigned int tv_sec, unsigned int tv_usec, int type,
+   DRI2SwapEventPtr swap_complete, void *swap_data)
 {
 ScreenPtr pScreen = pDraw-pScreen;
 DRI2DrawablePtr pPriv;
@@ -1179,6 +1179,17 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, 
CARD64 target_msc,
 }
 
 void
+_DRI2WarnConstantUSTOrMSC(const char *file, int line, const char *func,
+  DrawablePtr pDraw, int frame,
+  unsigned int tv_sec, unsigned int tv_usec)
+{
+CARD64 ust = ((CARD64) tv_sec * 100) + tv_usec;
+xf86DrvMsg(pDraw-pScreen-myNum, X_ERROR,
+   [DRI2] %s:%d:%s: driver incorrectly provided a constant UST 
(%lu) or MSC (%d)\n,
+   file, line, func, (unsigned long) ust, frame);
+}
+
+void
 DRI2SwapInterval(DrawablePtr pDrawable, int interval)
 {
 ScreenPtr pScreen = pDrawable-pScreen;
diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
index 1e7afdd..b547852 100644
--- a/hw/xfree86/dri2/dri2.h
+++ b/hw/xfree86/dri2/dri2.h
@@ -352,6 +352,49 @@ extern _X_EXPORT void DRI2WaitMSCComplete(ClientPtr 
client, DrawablePtr pDraw,
   int frame, unsigned int tv_sec,
   unsigned int tv_usec);
 
+#if defined(__GNUC__)
+/* Validate OML_sync_control UST and MSC values. */
+#define _DRI2CheckUSTAndMSC(pDraw, frame, tv_sec, tv_usec) \
+do { \
+if (__builtin_constant_p(frame) || (__builtin_constant_p(tv_sec)  
__builtin_constant_p(tv_usec))) { \
+static int _already_warned; \
+if (!_already_warned) { \
+_already_warned = 1; \
+_DRI2WarnConstantUSTOrMSC(__FILE__, __LINE__, __func__, pDraw, 
frame, tv_sec, tv_usec); \
+} \
+} \
+} while(0)
+
+#define

[PATCH 1/2] DRI2SwapBuffers: Don't reuse swap_target variable.

[Jamey Sharp]

swap_target is an out-parameter that needs to be set to the value that
SBC will take on after this SwapBuffers request completes.

However, it was also being used as a temporary variable to hold the MSC
at which the SwapBuffers request got scheduled to occur. This confusion
makes it harder to reason about whether swap_target is being set
correctly for its out-parameter usage. (Hint: It isn't.)

For the latter use, it makes more sense to use the existing target_msc
variable, which already has the right value unless target_msc, divisor,
and remainder are all 0, in which case we can set it using swap_interval
as usual.

Signed-off-by: Jamey Sharp ja...@minilop.net
Signed-off-by: Theo Hill theo0...@gmail.com
---
 hw/xfree86/dri2/dri2.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 729a323..962f40c 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -1149,17 +1149,13 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, 
CARD64 target_msc,
  * we have to account for the current swap count, interval, and the
  * number of pending swaps.
  */
-*swap_target = pPriv-last_swap_target + pPriv-swap_interval;
+target_msc = pPriv-last_swap_target + pPriv-swap_interval;
 
 }
-else {
-/* glXSwapBuffersMscOML could have a 0 target_msc, honor it */
-*swap_target = target_msc;
-}
 
 pPriv-swapsPending++;
 ret = (*ds-ScheduleSwap) (client, pDraw, pDestBuffer, pSrcBuffer,
-   swap_target, divisor, remainder, func, data);
+   target_msc, divisor, remainder, func, data);
 if (!ret) {
 pPriv-swapsPending--;  /* didn't schedule */
 xf86DrvMsg(pScreen-myNum, X_ERROR,
@@ -1167,7 +1163,7 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, 
CARD64 target_msc,
 return BadDrawable;
 }
 
-pPriv-last_swap_target = *swap_target;
+pPriv-last_swap_target = target_msc;
 
 /* According to spec, return expected swapbuffers count SBC after this swap
  * will complete.
-- 
1.8.5.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 0/2] Fix uninitialized reply in DRI2SwapBuffers

[Jamey Sharp]

These two patches together clean up commit b180e439, which has been in
xserver releases since 1.9. That commit was attempting to fix an
OML_sync_control spec compliance bug; these finish the job.

I'd like to suggest that, once these patches are merged to master, they
also be included in any future stable point releases.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 2/2] DRI2SwapBuffers: Fix uninitialized target SBC.

[Jamey Sharp]

Fixes Piglit test swapbuffersmsc-return swap_interval 0.

Ensure that *swap_target gets initialized on any 'return Success' path,
even if the swap request can't be completed by the driver and the server
falls back to a simple blit. That path can also be triggered by setting
swap_interval to 0, which disables sync to vertical retrace.

We originally found this bug because for some reason SDL2 automatically
sets swap_interval to 0, when we were trying to test OML_sync_control in
an SDL2 test application. We then discovered that the above-mentioned
Piglit test has been failing for the same reason since it was
introduced.

Signed-off-by: Jamey Sharp ja...@minilop.net
Signed-off-by: Theo Hill theo0...@gmail.com
---
 hw/xfree86/dri2/dri2.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 962f40c..76708ca 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -1092,6 +1092,14 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, 
CARD64 target_msc,
 return BadDrawable;
 }
 
+/* According to spec, return expected swapbuffers count SBC after this swap
+ * will complete. This is ignored unless we return Success, but it must be
+ * initialized on every path where we return Success or the caller will 
send
+ * an uninitialized value off the stack to the client. So let's initialize
+ * it as early as possible, just to be sure.
+ */
+*swap_target = pPriv-swap_count + pPriv-swapsPending + 1;
+
 for (i = 0; i  pPriv-bufferCount; i++) {
 if (pPriv-buffers[i]-attachment == DRI2BufferFrontLeft)
 pDestBuffer = (DRI2BufferPtr) pPriv-buffers[i];
@@ -1165,11 +1173,6 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, 
CARD64 target_msc,
 
 pPriv-last_swap_target = target_msc;
 
-/* According to spec, return expected swapbuffers count SBC after this swap
- * will complete.
- */
-*swap_target = pPriv-swap_count + pPriv-swapsPending;
-
 DRI2InvalidateDrawableAll(pDraw);
 
 return Success;
-- 
1.8.5.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 0/8] Fix bugs found with -D_FORTIFY_SOURCE=2

[Jamey Sharp]

For the series, with the exception of patch 1/8:

Reviewed-by: Jamey Sharp ja...@minilop.net

Patch 1 uses assert incorrectly, as we discussed on IRC.
Jamey

On Fri, Apr 18, 2014 at 3:21 PM, Keith Packard kei...@keithp.com wrote:
 Here's a set of 8 tiny patches which eliminate warnings that are
 printed when the X server is compiled with -D_FORTIFY_SOURCE=2.

  [PATCH 1/8] test: Validate server log reading more carefully in

 Actual bug fix in the test code.

  [PATCH 2/8] kdrive: Explicitly ignore errors from the -switchCmd
  [PATCH 3/8] kdrive: Ignore failure to chown console tty to current

 The compiler now makes us work harder to ignore return values that we
 don't care about.

  [PATCH 4/8] os: Clear the -displayfd option after closing the file

 An actual bug fix -- if you use the -displayfd option and ran the
 server for more than one generation, you'd end up attempting to write
 the display number to a random fd after reset.

  [PATCH 5/8] os: FatalError if -displayfd writes fail

 If you pass -displayfd, then we assume you actually require the
 display number for the system to work right; a failure here should be
 fatal instead of being silently ignored.

  [PATCH 6/8] os: Make sure that writing our pid to the lock file

 We fatal error when create fails, but ignore write failures?

  [PATCH 7/8] os: Ignore log file write failures

 The compiler now makes us work harder to ignore return values that we
 don't care about.

  [PATCH 8/8] xkb: Verify reads of compiled keymap header and TOC

 If the compiled keymap file was truncated, we would silently ignore
 the errors.

 -keith
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] test: [v2] Validate server log reading more carefully in signal-logging test

[Jamey Sharp]

Reviewed-by: Jamey Sharp ja...@minilop.net

Jamey

On Fri, Apr 18, 2014 at 4:19 PM, Keith Packard kei...@keithp.com wrote:
 Check return value from fgets and strchr instead of assuming they
 worked.

 [v2]

 Don't do any necessary work inside the assert call.
 Also make sure the return value was long enough.

 Signed-off-by: Keith Packard kei...@keithp.com
 ---

 Responding to IRC review comments from Jamey

  test/signal-logging.c | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)

 diff --git a/test/signal-logging.c b/test/signal-logging.c
 index 88b37c1..a72675f 100644
 --- a/test/signal-logging.c
 +++ b/test/signal-logging.c
 @@ -178,9 +178,14 @@ static void logging_format(void)
  LogInit(log_file_path, NULL);
  assert(f = fopen(log_file_path, r));

 -#define read_log_msg(msg) \
 -fgets(read_buf, sizeof(read_buf), f); \
 -msg = strchr(read_buf, ']') + 2; /* advance past [time.stamp] */
 +#define read_log_msg(msg) do {  \
 +msg = fgets(read_buf, sizeof(read_buf), f); \
 +assert (msg != NULL);   \
 +msg = strchr(read_buf, ']');\
 +assert(msg != NULL);\
 +assert(strlen(msg)  2);\
 +msg = msg + 2; /* advance past [time.stamp] */  \
 +} while (0)

  /* boring test message */
  LogMessageVerbSigSafe(X_ERROR, -1, test message\n);
 --
 1.9.2

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

bug in glamor on ShmPutImage for XY images

[Jamey Sharp]

I haven't understood enough of the implementation to see how to fix
this, but I can reliably crash Xephyr this way:

hw/kdrive/ephyr/Xephyr :1 -screen 1024x768 -glamor
DISPLAY=:1 x11perf -shmputxy10

Without the -glamor option to Xephyr, the x11perf command runs
successfully. I'm testing on current git master.

As far as I can tell, fbPutXYImage is getting called with a pDrawable
for which the pixmap's devPrivate.ptr is NULL. It's setting FbBits
*dst to that NULL and passing it down to fbBltOne, which then
segfaults.

I guess ShmPutImage works fine for ZPixmaps and core PutImage works
for XYPixmaps; I haven't seen anything fail except when the two are
put together.

I kind of wanted to try the performance comparison exercise that
keithp described in http://keithp.com/blogs/glamor-hacking/ but this
got in the way.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v3] xnest: Ignore GetImage() error in xnestGetImage()

[Jamey Sharp]

Looks good to me!

Reviewed-by: Jamey Sharp ja...@minilop.net
On Oct 5, 2013 5:05 AM, Egbert Eich e...@freedesktop.org wrote:

 From: Radek Doulik r...@novell.com

 When an Xnest instance is not viewable it will crash when a client in
 that instance calls GetImage. This is because the Xnest server will
 itself receives a BadMatch error.
 This patch ignores the error. The application which has requested the
 image will receive garbage - this however is fully legal according
 to the specs as obscured areas will always contain garbage if there
 isn't some sort of backing store as discussed in
 https://bugs.freedesktop.org/show_bug.cgi?id=9488
 The applied patch is a version from Dadek Doulik.

 v2: Call XSync() before changing error handlers as suggested by
 Daniel Stone dan...@fooishbar.org.
 v3: Don't call Xsync before restoring error handler as any errors
 generated by XGetImage() should be processed when this call
 returns as suggested by Jamey Sharp ja...@minilop.net

 Signed-off-by: Egbert Eich e...@freedesktop.org
 ---
  hw/xnest/GCOps.c | 13 +
  1 file changed, 13 insertions(+)

 diff --git a/hw/xnest/GCOps.c b/hw/xnest/GCOps.c
 index e26a136..d00511d 100644
 --- a/hw/xnest/GCOps.c
 +++ b/hw/xnest/GCOps.c
 @@ -94,15 +94,28 @@ xnestPutImage(DrawablePtr pDrawable, GCPtr pGC, int
 depth, int x, int y,
  }
  }

 +static int
 +xnestIgnoreErrorHandler (Display *display,
 + XErrorEvent *event)
 +{
 +return False; /* return value is ignored */
 +}
 +
  void
  xnestGetImage(DrawablePtr pDrawable, int x, int y, int w, int h,
unsigned int format, unsigned long planeMask, char *pImage)
  {
  XImage *ximage;
  int length;
 +int (*old_handler)(Display*, XErrorEvent*);
 +
 +/* we may get BadMatch error when xnest window is minimized */
 +XSync(xnestDisplay, False);
 +old_handler = XSetErrorHandler (xnestIgnoreErrorHandler);

  ximage = XGetImage(xnestDisplay, xnestDrawable(pDrawable),
 x, y, w, h, planeMask, format);
 +XSetErrorHandler(old_handler);

  if (ximage) {
  length = ximage-bytes_per_line * ximage-height;
 --
 1.8.1.4


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11] Always initialise thread support

[Jamey Sharp]

Hmm. XInitThreads wasn't designed to be used this way. For instance,
initializing thread support isn't thread-safe, for fairly obvious
reasons.

This patch might mask more bugs than it causes, and thereby be a net
win. But it seems equally likely to turn out the other way.

I'd suggest an awful lot of caution here.

Jamey

On Fri, Jul 12, 2013 at 10:25:38PM +0100, Daniel Stone wrote:
 Make XOpenDisplay always call XInitThreads when opening a display, thus
 guarding it against possible disaster scenarios like calling
 XOpenDisplay without XInitThreads, then passing the Display pointer into
 an EGL library which uses threads.  Or any of the other five similar
 failure scenarios I've seen just this week.
 
 Signed-off-by: Daniel Stone dan...@fooishbar.org
 ---
  src/OpenDis.c | 2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/src/OpenDis.c b/src/OpenDis.c
 index fc67d1a..2104845 100644
 --- a/src/OpenDis.c
 +++ b/src/OpenDis.c
 @@ -87,6 +87,8 @@ XOpenDisplay (
   long int conn_buf_size;
   char *xlib_buffer_size;
  
 +XInitThreads();
 +
   /*
* If the display specifier string supplied as an argument to this
* routine is NULL or a pointer to NULL, read the DISPLAY variable.
 -- 
 1.8.3.1
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11] Always initialise thread support

[Jamey Sharp]

On Fri, Jul 12, 2013 at 11:16:16PM +0100, Daniel Stone wrote:
 On 12 July 2013 22:40, Jamey Sharp ja...@minilop.net wrote:
  Hmm. XInitThreads wasn't designed to be used this way. For instance,
  initializing thread support isn't thread-safe, for fairly obvious
  reasons.
 
  This patch might mask more bugs than it causes, and thereby be a net
  win. But it seems equally likely to turn out the other way.
 
  I'd suggest an awful lot of caution here.
 
 Hmm.  So the failure mode here would be n threads both calling
 XOpenDisplay simultaneously, all using the Displays independently
 (i.e. never mixing threads) and not having called XInitThreads.  If
 XInitThreads gets entered simultaneously between the test and
 assignment of _Xglobal_lock, then we'll leak n - 1 copies of all the
 mutexes.  The real disaster is if pthread_mutex_init isn't thread-safe
 (non-atomic pointer stores?), in which case the mutex pointers are
 going to be half of one and half of the other.
 
 So there's a theoretical API break for that case, but that can also be
 fixed by calling XInitThreads beforehand, which I think is acceptable.

I mostly agree with your reasoning, except the subtleties of memory
ordering semantics make this even more murky. Calling XInitThreads first
only helps if it's followed by a write barrier, and the later calls are
preceded by read barriers.

I think pthread_create should have the right barrier semantics in the
parent and child threads, so if you call XInitThreads before
pthread_create, it should be fine--but POSIX apparently doesn't specify
anything about that.

 The immediate provocation was the Mali GLES/EGL implementation, which
 uses multiple threads itself, and thus relies on XInitThreads having
 been called somewhere; so if you ever use that specific
 implementation, every app has to call XInitThreads first to ensure it
 doesn't die horribly.

Yeah, I won't argue that XInitThreads was ever a good idea... :-/ For
that matter, I'm not certain why --disable-xthreads is still allowed.

In fact, I don't really mean to argue against the patch. I'm just
proposing caution.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11] Always initialise thread support

[Jamey Sharp]

On Fri, Jul 12, 2013 at 05:09:00PM -0700, Alan Coopersmith wrote:
 On 07/12/13 03:16 PM, Daniel Stone wrote:
 The immediate provocation was the Mali GLES/EGL implementation, which
 uses multiple threads itself, and thus relies on XInitThreads having
 been called somewhere; so if you ever use that specific
 implementation, every app has to call XInitThreads first to ensure it
 doesn't die horribly.
 
 I don't know that it's any saner, but the patches we carried in the Solaris
 libX11 for years to allow libraries to be thread safe even when calling
 apps are not are posted, along with description, at:
 
 http://people.freedesktop.org/~alanc/thread-fixes/
 
 Unfortunately, I never got around to trying to figure out if these
 would be good to push upstream or not.

I think the first one looks like a reasonable idea, though I'd have to
think very hard to decide if it's correct.

http://people.freedesktop.org/~alanc/thread-fixes/#1234757

But as the second patch demonstrates, the first one could be opening up
a wide variety of other issues.

http://people.freedesktop.org/~alanc/thread-fixes/#4010755

Patch three looks crazy at first glance, and is probably superceded by
the fix for #1182, as you noted.

http://people.freedesktop.org/~alanc/thread-fixes/#4041914

Patch four seems unrelated to the current discussion, but sounds like a
plausible patch based on the description.

http://people.freedesktop.org/~alanc/thread-fixes/#4614834

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libX11 6/8] Check for symbol existence with #ifdef, not #if.

[Jamey Sharp]

Since every other XTHREADS check in that earlier patch uses #ifdef, it
looks like it was just a mistake.

For this fix:
Reviewed-by: Jamey Sharp ja...@minilop.net

Jamey
On 06/ 2/13 11:49 AM, Thomas Klausner wrote:

 ---
   src/XlibInt.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/XlibInt.c b/src/XlibInt.c
 index b06e57b..92a4340 100644
 --- a/src/XlibInt.c
 +++ b/src/XlibInt.c
 @@ -239,7 +239,7 @@ void _XSeqSyncFunction(
   static int
   _XPrivSyncFunction (Display *dpy)
   {
 -#if XTHREADS
 +#ifdef XTHREADS
   assert(!dpy-lock_fns);
   #endif
   assert(dpy-synchandler == _XPrivSyncFunction);


The comment right before that function is:
/* NOTE: only called if !XTHREADS, or when XInitThreads wasn't called. */
which makes this sound intentional, but since even in the old monolith
I only see XTHREADS being defined or undefined, not defined with a false
value, I'm not sure when that would be hit.

Since Jamey just added this in 2010, we can ask him and see if he remembers
why he did used #if instead of #ifdef:

http://cgit.freedesktop.org/**xorg/lib/libX11/commit/?id=**
a6d974dc59f2722b36e2df9d4f07ae**ee4f83ce43http://cgit.freedesktop.org/xorg/lib/libX11/commit/?id=a6d974dc59f2722b36e2df9d4f07aeee4f83ce43

-- 
-Alan Coopersmith-  alan.coopersm...@oracle.com
 Oracle Solaris Engineering - http://blogs.oracle.com/alanc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libX11] specs/libX11: correct prototype for XListPixmapFormats/XImageByteOrder

[Jamey Sharp]

Reviewed-by: Jamey Sharp ja...@minilop.net

On Fri, Jun 07, 2013 at 09:32:31AM -0700, Alan Coopersmith wrote:
 The XListPixmapFormats arguments was being shown with XImageByteOrder's
 name and return types.   Appears to have been a glitch in the nroff -
 docbook conversion.
 
 Reported-by: ZHANG Zhaolong zhangzl2...@126.com
 Signed-off-by: Alan Coopersmith alan.coopersm...@oracle.com
 ---
  specs/libX11/CH02.xml |9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)
 
 diff --git a/specs/libX11/CH02.xml b/specs/libX11/CH02.xml
 index 12df68e..5e0e611 100644
 --- a/specs/libX11/CH02.xml
 +++ b/specs/libX11/CH02.xml
 @@ -1511,12 +1511,9 @@ To obtain the pixmap format information for a given 
 display, use
  indexterm 
 significance=preferredprimaryXListPixmapFormats/primary/indexterm
  !-- .sM --
  /para
 -para
 -ImageByteOrder(emphasis remap='I'display/emphasis)
 -/para
 -funcsynopsis id='XImageByteOrder'
 +funcsynopsis id='XListPixmapFormats'
  funcprototype
 -  funcdefint functionXImageByteOrder/function/funcdef
 +  funcdefXPixmapFormatValues 
 *functionXListPixmapFormats/function/funcdef
paramdefDisplayparameter *display/parameter/paramdef
paramdefintparameter *count_return/parameter/paramdef
  /funcprototype
 @@ -1577,7 +1574,7 @@ These are often used by toolkits as well as by simple 
 applications.
  para
  ImageByteOrder(emphasis remap='I'display/emphasis)
  /para
 -funcsynopsis
 +funcsynopsis id='XImageByteOrder'
  funcprototype
funcdefint functionXImageByteOrder/function/funcdef
paramdefDisplayparameter *display/parameter/paramdef
 -- 
 1.7.9.2
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2] libX11: check size of GetReqExtra after XFlush

[Jamey Sharp]

It seems to me that the change to GetReqExtra should indeed be merged. I
think now we're just debating what to do with any possibly-hazardous
callers. Kees, perhaps you could split the patch?

Regarding the rest, I like Alan's comments, and would add:

On Jun 6, 2013 9:17 PM, Alan Coopersmith alan.coopersm...@oracle.com
wrote:
 On 06/ 6/13 10:40 AM, Kees Cook wrote:
 Two users of GetReqExtra pass arbitrarily sized allocations from the
 caller (ModMap and Host). Adjust _XGetRequest() (called by the
GetReqExtra
 macro) to double-check the requested length and invalidate req when
this
 happens. Users of GetReqExtra passing lengths greater than the Xlib
buffer
 size (normally 16K) now check req and fail if something has gone wrong.

 An alternative would be to convert them to use Data instead of
GetReqExtra()
 as the requests like PutImage do that truly send arbitrarily large data to
 the server, but I'm not sure it's worth it in these cases.

Since the original bug report was about xhost failing on long hostnames,
maybe it actually is worthwhile to make the Host routines use Data?

 diff --git a/src/ModMap.c b/src/ModMap.c
 index 5c5b426..f66e559 100644
 --- a/src/ModMap.c
 +++ b/src/ModMap.c
 @@ -80,6 +80,10 @@ XSetModifierMapping(

   LockDisplay(dpy);
   GetReqExtra(SetModifierMapping, mapSize, req);
 +if (!req) {
 +   UnlockDisplay(dpy);
 +   return 2;

 Style nit, please return MappingFailed instead of the raw value.  (I see
 the comment above the function uses the raw values instead of the symbolic
 names used in the man pages and server side code, which is unfortunate.)


 +}

   req-numKeyPerModifier = modifier_map-max_keypermod;

 Since the protocol defines that as a single byte, we should probably
reject
 at the top of the function any modifier_map-max_keypermod  255, as
otherwise
 we send a packet to the X server with more data than expected, which it
will
 reject - and that limit would also keep the mapSize within the normal Xlib
 buffer size.   That doesn't need to be this patch though.

I like this plan. Is MappingFailed the right return value for an
out-of-range value?

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xts] Write results directly to results directory

[Jamey Sharp]

Seems like a smart idea and solid patch. I have a couple suggestions you
can do with as you please:

On Mon, Jun 03, 2013 at 04:33:35PM -0400, Peter Harris wrote:
 If an installed xtest is run, the user does not normally have write
 access to the test directory. Writing intermediate files and results
 directly to the results directory avoids a number of spurious FAILs and
 UNRESOLVEDs.
...
 +const char *
 +outfile(const char *fn)
 +{
 + char *out = (char *)fn;

This assignment to out is dead; I'd delete the initializer.

 + const char *path;
 + char *resfile = getenv(TET_RESFILE);
 + if (!resfile)
 + return fn;
 +
 + resfile = strdup(resfile);
 + if (!resfile)
 + return fn;
 +
 + path = dirname(resfile);
 + if (!path)
 + goto done;
 +
 + out = malloc(strlen(path) + strlen(fn) + 2);
 + if (!out) {
 + out = (char *)fn;
 + goto done;
 + }
 + regid(NULL, (union regtypes *)out, REG_MALLOC);

I don't know the XTS codebase (and don't want to, but...): Does regid
register a resource to be freed at the end of the current test?

- If so, this patch seems clearly correct and a good idea. A comment at
  the declaration of outfile() about the lifetime of the result would be
  nice though.

- If not, meh, it won't leak so much it's a serious problem.

Either way:
Reviewed-by: Jamey Sharp ja...@minilop.net

 + sprintf(out, %s/%s, path, fn);
 +
 +done:
 + free(resfile);
 + return out;
 +}

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC][PATCH] Make GetXIDRange O(1) instead of O(N^2)

[Jamey Sharp]

I can't give this a full review, but off-hand it seems like a good idea
to me!

Changing the max hash size should probably be a separate commit with a
commit message justifying it.

There's some precedent for putting common data structures in shared code
in xserver, notably include/list.h. (Although namespace pollution is a
challenge, as commit ca64912c02bdff486fee420a49b11f54f8f5ba08 proved.) I
think this patch would be more clear with the generic data structure
code kept in separate files.

I hope someone will explain the XID reuse behavior. I've been told that
the X protocol spec permits the same XID to be used for multiple
resource types, as long as they don't conflict (like WINDOW and PIXMAP
do, through DRAWABLE). I've never been clear on how much that's really
supported or relied upon, though, or what the resource-free behavior is
supposed to be. It would be nice to have an example of a client that
does this, actually.

You'll want to be careful about the indentation in your patch when you
put the final version together for review. This version has several
different indentation levels throughout.

I hope those quick pseudo-review comments are helpful, despite not
really addressing the meat of your proposal...
Jamey

On Mon, May 27, 2013 at 11:56:06AM +0200, Roberto Ragusa wrote:
 Hi,
 
 I would like feedback about this patch (do not consider it ready to merge!)
 
 The GetXIDRange function in dix/resource.c can show incredibly bad performance
 in some cases; it can degenerate to O(N*2) behavior and make the system
 completely unusable.
 (see http://lists.x.org/archives/xorg-devel/2012-November/034555.html
 for a description of one manifestation of the problem)
 
 The basic problem is that when a client (xcb?) asks for free ids, the
 code makes a lot of inefficient guesses and spends an awful amount of time
 in identifying range of elements NOT present in its data structures (hashes).
 
 This patch, in addition to raising the max hash size from 2^11 to 2^16 (2^11 
 was
 decided in 1994, or maybe even 1987), adds explicit tracking of free ids.
 The data structure I've chosen is an rb-tree of free extents.
 
 At the start, free resources are 0-0x7ff (one extent).
 After you allocate 5 and 8, the tree contains three extents (0-4, 6-7, 
 9-0x7ff).
 After you free 5, the tree contains two extents (0-7, 9-0x7ff).
 
 The good part is that when GetXIDRange is invoked, we just take a range
 from the tree and return that, instantly.
 
 The patch has been running on my machine for some months with great 
 performance
 and no strange effects (there is a problem I'm investigating which could be an
 unrelated kwin bug).
 
 I need comments from experts about one strange thing I've discovered:
 
 - clients sometime add resources with already used IDs; it looks like the 
 newer
 resource is returned by get, and that all of them are deleted by free;
 is this a bug in the applications? I decided to not change the external 
 behavior of the
 code in these circumstances (see the functions containing maybe in their 
 names)
 
 As regards the practical implementation:
 
 - the rbtree code comes from
 
 http://en.literateprograms.org/index.php?title=Special:DownloadCode/Red-black_tree_%28C%29oldid=18555
   please check if the license is acceptable
 
 - the rbtree code could be in different files, I've just pasted it in 
 resource.c to avoid
   makefile changes
 
 - the code is big but there is a lot of useless stuff (tree verification, 
 statistics, debug
   facilities)
 
 - the code runs with debug off; but if you touch /tmp/xdbg it enables debug 
 after a few seconds,
   logs everything for a few seconds, then turns it off again (removes 
 /tmp/xdbg); this is
   my way to explore how it is working on a real everyday-used system
 
 Any comment will be really appreciated.
 Thank you.
 
 (patch is against xorg-server-1.12.4)
 
 --- resource.c_ORIG 2012-05-17 19:09:02.0 +0200
 +++ resource.c  2013-05-25 16:48:40.424770241 +0200
 @@ -148,6 +148,672 @@
 
  #include Xserver-dtrace.h
 
 +
 +
 +
 +static int dbg_on=0;
 +#define DBG(fmt,...) do{if(dbg_on){LogMessage(X_INFO,fmt,##__VA_ARGS__);}} 
 while(0)
 +
 +typedef struct XIDextent_t{
 +  XID from;
 +  XID to;
 +} XIDextent;
 +
 +
 +#include assert.h
 +#include stdio.h
 +#include stdlib.h
 +
 +   #include sys/types.h
 +   #include sys/stat.h
 +   #include unistd.h
 +
 +enum rbtree_node_color { RED, BLACK };
 +
 +typedef struct rbtree_node_t {
 +XIDextent extent;
 +struct rbtree_node_t* left;
 +struct rbtree_node_t* right;
 +struct rbtree_node_t* parent;
 +enum rbtree_node_color color;
 +} *rbtree_node;
 +
 +typedef struct rbtree_t {
 +rbtree_node root;
 +int stat_inserts;
 +int stat_deletes;
 +int stat_lookups;
 +int stat_lookup_evals;
 +int stat_mark_used;
 +int stat_mark_used_omit;
 +int stat_mark_unused;
 +int stat_maybe_mark_unused;
 +} *rbtree;
 +
 +typedef int 

Re: where does CARD18 come from?

[Jamey Sharp]

They're defined in the X11 core protocol specification:

http://www.x.org/releases/X11R7.7/doc/xproto/x11protocol.html#Common_Types

I believe CARD is short for cardinal, as in cardinal numbers.

Regarding X vs. Lisp: I've been told that the X design decision to make
XIDs 29 bits wide was because (at least some) Lisp implementations of
the day had 29-bit integers; the other bits in each word were used to
tag pointers vs. unboxed integers, I guess. So they're not so completely
unrelated as you might think.

Jamey

On Mon, May 20, 2013 at 07:12:44AM +0200, wemp...@gmail.com wrote:
 Hello,
 
 I was wondering where the names of various data types listed here may
 come from:
 http://www.x.org/wiki/XSessionManagementProtocol#Data_Types
 
 Most of them are clear, but these ones picked my interest:
 
  CARD8
  a one-byte unsigned integer
 
  CARD16
  a two-byte unsigned integer
 
  CARD32
  a four-byte unsigned integer 
 
 Do you know what is the etymology of these type names? Do they mean
 character..., but what does d stand for? In Lisp car is a
 function that returns the first element of the list but I think Xorg
 has nothing to do with Lisp.
 
 History is interesting ;)
 -- 
 wemp...@gmail.com
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:xman 2/2] configure: check for groff and enable groff extensions if found

[Jamey Sharp]

I confess I'm missing the context here, but it sounds like this should be a
runtime check, not build-time... shouldn't it?

Jamey
On Apr 21, 2013 10:19 AM, Alan Coopersmith alan.coopersm...@oracle.com
wrote:

 On 04/20/13 11:28 AM, Gaetan Nadon wrote:

 On 13-04-20 01:08 PM, Alan Coopersmith wrote:

 Check for groff never got translated from imake to autoconf

 Signed-off-by: Alan Coopersmith alan.coopersm...@oracle.com
 ---
   configure.ac |5 +
   defs.h   |4 
   2 files changed, 9 insertions(+)

 diff --git a/configure.ac b/configure.ac
 index 74a6fc8..b9920bd 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -37,6 +37,11 @@ AC_CONFIG_HEADERS([config.h])

   AC_CANONICAL_HOST

 +AC_CHECK_PROG([GROFF], [groff], [found], [missing])
 +if test x$GROFF = xfound ; then
 +   AC_DEFINE([HAS_GROFF], 1, [Define to 1 if you have the groff
 package.])
 +fi
 +
   AC_CHECK_FUNCS([mkstemp])

   AC_ARG_WITH(helpdir,
 diff --git a/defs.h b/defs.h
 index b4cd434..fe09b6b 100644
 --- a/defs.h
 +++ b/defs.h
 @@ -34,6 +34,10 @@ from the X Consortium.
* Created:   October 22, 1987
*/

 +#ifdef HAVE_CONFIG_H
 +# include config.h
 +#endif
 +
   #ifndef HELPFILE
   #define HELPFILE /usr/lib/X11/xman.help  /* name of the default
 helpfile. */
   #endif

 Alternatively, you can use XORG_WITH_GROFF macro and benefit from all
 the features. This was used in many docs before the move to DocBook. In
 any case you probably want to use HAVE_xx to follow the convention used
 by Automake elsewhere.


 Thanks - I'd forgotten about that macro.   It's tempting to use it for the
 --with-groff flag as Dan mentioned, though I wonder if people are expecting
 that to only control generating documentation at build time, not making the
 resulting code call groff.

 For cross-compilers especially, this would be the difference between groff
 in the build host vs. the target environment, so if we did have a flag, it
 should probably distinguish between the two cases somehow.

 --
 -Alan Coopersmith-  alan.coopersm...@oracle.com
  Oracle Solaris Engineering - http://blogs.oracle.com/alanc
 __**_
 xorg-devel@lists.x.org: X.Org development
 Archives: 
 http://lists.x.org/archives/**xorg-develhttp://lists.x.org/archives/xorg-devel
 Info: 
 http://lists.x.org/mailman/**listinfo/xorg-develhttp://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] list.h: Make xorg_list_init inline

[Jamey Sharp]

Seems like a good plan to me.

Reviewed-by: Jamey Sharp ja...@minilop.net

On Tue, Mar 12, 2013 at 09:40:16AM -0700, Robert Morell wrote:
 Otherwise this file is emitted in every unit that includes it.
 
 Signed-off-by: Robert Morell rmor...@nvidia.com
 ---
 
 This reduces the size of a debug Xorg binary on my system by just under 1%.
 
  include/list.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/include/list.h b/include/list.h
 index 067c679..11de7c5 100644
 --- a/include/list.h
 +++ b/include/list.h
 @@ -119,7 +119,7 @@ struct xorg_list {
   *
   * @param The list to initialized.
   */
 -static void
 +static inline void
  xorg_list_init(struct xorg_list *list)
  {
  list-next = list-prev = list;
 -- 
 1.8.1.5
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: RFC: xserver commit process changes

[Jamey Sharp]

On Thu, Feb 28, 2013 at 09:06:07AM +1000, Peter Hutterer wrote:
 * leave the current window of 3/2/1-ish months for the different
 devel stages
 * leave the requirement for a reviewed-by
 * one RM, calling the shots for when releases are made and generally
 being the reviewer of last resort and arbiter where needed
 * 3-5 people with commit access during the devel and general bugfix
 windows. They scoop up pull requests and commit them, if the patches
 have rev-by tags [3]
 * 2 people during the last bugfix window (emergency fixes only). The
 second person as backup to the RM to make sure we don't see delays.

Seems like a well-reasoned solution that keeps the advantages of having
few people with commit bits.

I am so not volunteering for a commit bit--as if there were any danger
of that. ;-) I support the proposal though.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: add FatalErrorSigSafe for signal-safe logging

[Jamey Sharp]

On Fri, Feb 15, 2013 at 09:35:00AM +1000, Peter Hutterer wrote:
 diff --git a/include/os.h b/include/os.h
 index c7108a5..2bdbf95 100644
 --- a/include/os.h
 +++ b/include/os.h
 @@ -655,6 +655,12 @@ FatalError(const char *f, ...)
  _X_ATTRIBUTE_PRINTF(1, 2)
  _X_NORETURN;
  
 +/* XXX: DO NOT USE! Temporary */
 +extern _X_EXPORT void
 +FatalErrorSigSafe(const char *f, ...)
 +_X_ATTRIBUTE_PRINTF(1, 2)
 +_X_NORETURN;
 +

Out of curiosity, do you really need to _X_EXPORT it if you only want it
used elsewhere in os/?

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: support for HW_SKIP_CONSOLE breaks use by blind people

[Jamey Sharp]

On Tue, Jan 08, 2013 at 09:53:45AM -0500, Timothy Meade wrote:
 Maybe I'm missing something, auto detecting if VT switch is needed means
 introducing an inconsistencty. Dummy and evdev can both work without
 allocating a VT, and there are cases where that may be useful, such as
 debugging a server or input driver.

For what it's worth: The reason I introduced HW_SKIP_CONSOLE originally
was to allow the server to run without root privileges when it doesn't
need them. That turns out to be useful for debugging too, but wasn't my
motivation.

I would very much like it if people could continue to run Xorg in
Xnest/Xvfb-like situations without needing extra privileges or magic
flags.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Jamey Sharp]

On Tue, Apr 3, 2012 at 4:41 AM, Jon TURNEY jon.tur...@dronecode.org.uk wrote:
 On 27/03/2012 05:09, Jamey Sharp wrote:
 However, our capstone project that just finished was a start toward
 replacing the XQuartz DDX with a stock Xorg server and a special
 client, and I'm hoping that XWin can go the same direction.

 This is certainly the approach I'd like to take, as well, rather than stuffing
 all the WM integration code into a client thread inside the X server.

 However, replacing the Win32 native XWin (as opposed to an XWin built to use
 the cygwin layer) with the Xorg DDX seems like it would involve a non-trivial
 amount of work, in effectively porting the Xorg DDX to the Win32 API.

I'd be really interested to know how much of the Xorg DDX relies on
POSIX and how much is straight C. The module loader is an obvious
candidate for pain, but once you turn off stuff like the PCI layer
that doesn't matter on Windows, how much non-portable stuff is left?

I'm going to propose another student capstone project in a couple of
weeks, and I haven't actually decided what to ask the students for
yet. If you can sort out pretty soon how much work is really involved
here, we could try getting a team of five or six students to do it
over the next six months. If we go that direction, I'd also need one
or two XWin developers to co-sponsor the project with me, committing
to being available for the platform-specific questions I can't answer.
It'd help if you could get to Portland at least once during the
project, too...

 http://cgit.freedesktop.org/xorg/lib/libxcwm/

 Thanks.  I know you have mentioned this project previously and I'll try to
 make the time to take a look at it.

Much appreciated!

 Is there a way to support hardware-accelerated GL in that arrangement?

I hope so. :-) First, write Mesa and Xorg video driver bits that use
the platform-native accelerated rendering APIs, but only allocate
off-screen video memory, even for Window objects. If you can do
DRI-like 3D rendering on Windows, then at least the really expensive
part of 3D rendering can be accelerated, even if the libxcwm-based
client still uses GetImage to read back the results.

(For extra bonus points, use the glamor library in this driver to
hardware-accelerate 2D rendering as well. Wheee!)

After that, if you can also provide an OpenGL texture-from-pixmap
extension, then libxcwm can replace the slow GetImage path with a
hardware-accelerated blit from the back-buffers the server owns to the
native windows created by the libxcwm-based client. Combined with the
Composite extension's NameWindowPixmap request, this is just like a GL
compositor such as Compiz. That gets you fast updates of any
rendering, 2D or 3D, onto the real display.

Implementing texture-from-pixmap is the tricky part, because it
requires that processes in different address spaces be permitted to
share textures. Linux DRI can do that; Jeremy tells me that given the
public APIs on OS X, it's tricky at best. Does Windows allow it?

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Jamey Sharp]

On 3/26/12, Alan Coopersmith alan.coopersm...@oracle.com wrote:
 On 03/26/12 09:07 PM, Jamey Sharp wrote:
 Maybe I have it right this time: On Debian, there's no problem,
 because /usr/bin/X is a trivial suid wrapper and /usr/bin/Xorg is not
 installed suid. Solaris and other Unixes could take the same approach,
 right?

 However, if the suid wrapper allows non-root users to specify arbitrary files
 to -config, then it's a dangerous security hole we can't allow (and since the
 Debian people aren't stupid, I assume it does not).  If it doesn't allow
 -config through, then I don't see how it would help here.

The key is to have a *non*-suid copy of the server available for those
who don't need root privs for their configuration. In that mode all
options can be processed without the server performing security
checks, and if you try to subvert system security the OS will stop
you.

Systems that still need to allow non-root users to run the server with
root privileges (hopefully a dwindling set over time) can either ship
a suid wrapper, or ship a second copy of the server that has the suid
bit set, whichever makes more sense to the packagers.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Jamey Sharp]

On 3/27/12, Mark Kettenis mark.kette...@xs4all.nl wrote:
 Date: Tue, 27 Mar 2012 06:03:03 -0700
 From: Jamey Sharp ja...@minilop.net

 The key is to have a *non*-suid copy of the server available for those
 who don't need root privs for their configuration. In that mode all
 options can be processed without the server performing security
 checks, and if you try to subvert system security the OS will stop
 you.

 This is based on the (false) assumption that a suid Xorg is making
 things less secure.  It is perhaps somewhat non-intuitive, but a
 suid-root binary can use its powers to drop priviliges and become less
 priviliged than a normal user.  So a *non*-suid Xorg should not be a
 goal in itself.

Um... I can't say I'm convinced.

1) Why would you want to engineer the complexity of dropping privs,
with plenty of opportunities to get it wrong in ways that lead to root
holes? And since every OS has different rules about process
permissions, even getting it right on one kernel tells you nothing
about whether it's right anywhere else.

2) You aren't offering any advantage to an administrator--if a user's
X server is prohibited from doing some things the user is permitted to
do, you haven't stopped the user from doing them. So you must be
trying to support users who don't trust their X server and want to
sandbox it, right? But if they don't trust the X server, they
certainly shouldn't rely on it to sandbox itself, especially if it has
to have root permissions to do so.

By contrast, I think setting the server suid directly to an
unprivileged user would be fine, because it doesn't involve any
security-sensitive code in the server. Or using a dedicated program
that sets up restricted privileges, chroot, unshare, whatever
sandboxing you want, and then execs a plain copy of the X server,
because the code you have to audit then is small.

But no, I'm pretty sure keeping the X server away from root privs is
always the right thing, and we're just in a transition period where it
isn't always feasible yet.

Discussion of privilege is straying a bit far from the original topic,
but, well:
http://xkcd.com/386/

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Jamey Sharp]

2012/3/26 Sérgio Basto ser...@serjux.com:
 in other day I saw /usr/bin/xvfb-run is in use on building
 miro-4.0.6-2.fc16

 how we test X in a server without displays ?

Use xf86-video-dummy with the Xorg DDX.

 I use
 /usr/bin/Xvfb

 Xvfb :1 -fbdir /var/tmp/
 export DISPLAY=:1
 sdl-gnash  --screenshot 0 --screenshot-file $IMG.tmp -r1 -t1 $SWF

 kind of a flash ripper .

xf86-video-dummy doesn't have an equivalent to -fbdir, but it looks
like you aren't actually making use of the mmap'd framebuffer in
/var/tmp/. So this case should work just fine with xf86-video-dummy
today as well.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Jamey Sharp]

On Mon, Mar 26, 2012 at 4:31 PM, Keith Packard kei...@keithp.com wrote:
 On Mon, 26 Mar 2012 16:13:46 -0700, Jeremy Huddleston jerem...@apple.com 
 wrote:
 Most functionality of these servers can be provide by Xorg with either
 the nested or dummy video driver.

+1 for deleting these obsolete DDXes. I'd suggest deleting Xdmx as well.

 I'm all for deleting the code. I would like to have some idea of what
 you mean by 'most' here -- is there any significant functionality which
 isn't provided by the xf86-video drivers?

I don't have a complete list, but here are some things.

Timothy Meade thinks there may be some bugs in xf86-video-fbdev
compared to Xfbdev, and had other complaints when I tried to remove
Xfbdev a year ago:
http://lists.x.org/archives/xorg-devel/2011-May/022177.html

Xvfb has -fbdir and -shmem options to allocate the framebuffer in a
mmap'd file or a shared memory segment, respectively. Surely nobody
cares? If somebody does, patches copying the code from Xvfb to
xf86-video-dummy should be easy to write.

Xnest has options for configuring the created window. Again, I doubt
anyone cares, but it's easy functionality to re-introduce in
xf86-video-nested if someone wants to.

I think Xephyr has support for some extensions that may not be
supported when using xf86-video-nested. Perhaps Xv and DRI? Or maybe
DRI support was in patches that never got merged, like the Xephyr XCB
patches.

Xdmx has a complicated GLX proxy to support indirect GL spanning
multiple ScreenRecs under Xinerama. It might be nice to find a way to
merge that into DIX or the xfree86 DDX. Bonus points if Mesa had a
multihead libGL that could delegate direct rendering for each
ScreenRec to an appropriate libGL. :-)

Generally, I think Xdmx may have support for some extensions that Xorg
doesn't support when Xinerama is turned on, and it'd be nice to copy
that over.

It might be nice to have backwards-compatibility shell scripts for the
replaced programs that convert their command-line arguments into a
suitable xorg.conf and then run the real server.

That's everything I can think of. I'm not convinced any of those are
worth waiting for before deleting these DDXes.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Jamey Sharp]

On Mon, Mar 26, 2012 at 5:09 PM, Chase Douglas
chase.doug...@canonical.com wrote:
 On 03/26/2012 04:55 PM, Jamey Sharp wrote:
 xf86-video-dummy doesn't have an equivalent to -fbdir, but it looks
 like you aren't actually making use of the mmap'd framebuffer in
 /var/tmp/. So this case should work just fine with xf86-video-dummy
 today as well.

 I haven't looked into screen scraping much, but I thought it wasn't
 supported by the dummy video driver. I'm glad to see it is.

 What does it take to get a screen capture out of the dummy driver? Is it
 something you would recommend we encapsulate inside of xorg-gtest?

Sure, GetImage on the root window works just like it does on any other
video driver. Jeremy, Josh, and I sponsored a recently-completed
capstone project where the students produced quite a bit of code
relying on that:

http://cgit.freedesktop.org/xorg/lib/libxcwm/

I haven't paid too much attention to xorg-gtest; I assume it's a
framework for building X server test suites? I'd certainly recommend
using xf86-video-dummy if you're kicking off an X server during any
automated test procedure. Sometimes people might appreciate a
configuration option to use xf86-video-nested instead, so they can see
the test output. I'm not sure there's anything else you'd need to
encapsulate in xorg-gtest that's specific to the dummy driver though.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Jamey Sharp]

On Mon, Mar 26, 2012 at 6:01 PM, Alan Coopersmith
alan.coopersm...@oracle.com wrote:
 On 03/26/12 04:13 PM, Jeremy Huddleston wrote:
 These need to die.  This removes 30K lines of code from xorg-server.  It 
 must be good!

 Most functionality of these servers can be provide by Xorg with either the 
 nested or dummy video driver.  If someone really misses functionality, we 
 should fix that deficiency in hw/xfree86, xf86-video-dummy, or 
 xf86-video-nested.  Also, there's nothing stopping anyone from using older 
 server versions if they still need these DDXs.

 The giant blocker from my point of view is that by just deleting them, you've
 made it impossible for non-root users to run them, since Xorg only reads 
 config
 files from system directories when run as a root user.

I'm glad to hear you have no objections: this is all fixed in 1.12. See

ead968a4300c0adeff89b9886e888b6d284c75cc

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC] server-private and public header separation

[Jamey Sharp]

On 3/22/12, Chase Douglas chase.doug...@canonical.com wrote:
 On 03/22/2012 09:25 PM, Peter Hutterer wrote:
 This patch introduces a new define __XSERVER__ that we can use in the sdk
 headers. Obviously the real integration will be a tad harder as there are
 other headers that are installed outside of include/. But the gist is that
 anything between ifdef __XSERVER__ disappears on install, (in)effectively
 hiding it.

 This implementation would make the struct size look different between
 the server and the client. I think that would likely end up causing some
 bad memory corruption bugs sometime down the line.

These aren't protocol-visible structs, so clients aren't affected.
Unless you meant drivers? Clearly any struct that drivers allocate
directly, or that drivers do pointer math on such as array indexing,
can't get this treatment. Those are the only bad cases though, I
think?

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC] server-private and public header separation

[Jamey Sharp]

Well, I'm in favor, for whatever that's worth. :-)

Jamey

On Thu, Mar 22, 2012 at 9:25 PM, Peter Hutterer
peter.hutte...@who-t.net wrote:
 This is just a quickfire patch to show the principle, it has not been tested
 much. Plus, it's more of an idea right now, not sure if I'll find the time
 to do it.

 Right now, we export virtually everything including the gory bits of every
 struct. Which causes us to break the ABI whenever we so much as look at a
 struct (see the per-device-idlecounters for example).

 This patch introduces a new define __XSERVER__ that we can use in the sdk
 headers. Obviously the real integration will be a tad harder as there are
 other headers that are installed outside of include/. But the gist is that
 anything between ifdef __XSERVER__ disappears on install, (in)effectively
 hiding it.

 It's going to be painful defining what goes in and what doesn't, but maybe,
 just maybe, it'll be useful in the long term.

 Is that something we want?
 ---
  include/Makefile.am     |    8 
  include/dix-config.h.in |    3 +++
  include/inputstr.h      |   27 +++
  3 files changed, 26 insertions(+), 12 deletions(-)

 diff --git a/include/Makefile.am b/include/Makefile.am
 index 972f403..3e183db 100644
 --- a/include/Makefile.am
 +++ b/include/Makefile.am
 @@ -71,3 +71,11 @@ EXTRA_DIST =         \
        eventconvert.h eventstr.h inpututils.h \
        protocol-versions.h \
        xsha1.h
 +
 +
 +unifdef_headers: $(sdk_HEADERS)
 +       for file in $(sdk_HEADERS); do \
 +           (cd $(DESTDIR)$(sdkdir)  unifdef -U__XSERVER__ -o $$file.tmp 
 $$file; mv $$file.tmp $$file) \
 +       done
 +
 +install-data-hook: unifdef_headers
 diff --git a/include/dix-config.h.in b/include/dix-config.h.in
 index 3fb6413..9b362c6 100644
 --- a/include/dix-config.h.in
 +++ b/include/dix-config.h.in
 @@ -3,6 +3,9 @@
  #ifndef _DIX_CONFIG_H_
  #define _DIX_CONFIG_H_

 +/* XServer-internal */
 +#define __XSERVER__ 1
 +
  /* Support BigRequests extension */
  #undef BIGREQS

 diff --git a/include/inputstr.h b/include/inputstr.h
 index 5a38924..a4f549c 100644
 --- a/include/inputstr.h
 +++ b/include/inputstr.h
 @@ -529,19 +529,8 @@ typedef struct _SpriteInfoRec {
  typedef struct _DeviceIntRec {
     DeviceRec public;
     DeviceIntPtr next;
 -    Bool startup;               /* true if needs to be turned on at
 -                                   server initialization time */
 -    DeviceProc deviceProc;      /* proc(DevicePtr, DEVICE_xx). It is
 -                                   used to initialize, turn on, or
 -                                   turn off the device */
 -    Bool inited;                /* TRUE if INIT returns Success */
 -    Bool enabled;               /* TRUE if ON returns Success */
 -    Bool coreEvents;            /* TRUE if device also sends core */
 -    GrabInfoRec deviceGrab;     /* grab on the device */
 -    int type;                   /* MASTER_POINTER, MASTER_KEYBOARD, SLAVE */
 -    Atom xinput_type;
 -    char *name;
     int id;
 +    char *name;
     KeyClassPtr key;
     ValuatorClassPtr valuator;
     TouchClassPtr touch;
 @@ -554,6 +543,19 @@ typedef struct _DeviceIntRec {
     StringFeedbackPtr stringfeed;
     BellFeedbackPtr bell;
     LedFeedbackPtr leds;
 +
 +#ifdef __XSERVER__
 +    Bool startup;               /* true if needs to be turned on at
 +                                   server initialization time */
 +    DeviceProc deviceProc;      /* proc(DevicePtr, DEVICE_xx). It is
 +                                   used to initialize, turn on, or
 +                                   turn off the device */
 +    Bool inited;                /* TRUE if INIT returns Success */
 +    Bool enabled;               /* TRUE if ON returns Success */
 +    Bool coreEvents;            /* TRUE if device also sends core */
 +    GrabInfoRec deviceGrab;     /* grab on the device */
 +    int type;                   /* MASTER_POINTER, MASTER_KEYBOARD, SLAVE */
 +    Atom xinput_type;
     struct _XkbInterest *xkb_interest;
     char *config_info;          /* used by the hotplug layer */
     ClassesPtr unused_classes;  /* for master devices */
 @@ -593,6 +595,7 @@ typedef struct _DeviceIntRec {
     int xtest_master_id;

     struct _SyncCounter *idle_counter;
 +#endif
  } DeviceIntRec;

  typedef struct {
 --
 1.7.7.6

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] randr: Silence valgrind after server regen with panning enabled

[Jamey Sharp]

On Fri, Mar 16, 2012 at 4:39 AM, Chris Wilson ch...@chris-wilson.co.uk wrote:
 If when bug hunting you trigger a server regeneration after enabling a
 pnning mode without moving the nice, you cause a flurry of valgrind
 warnings:

I really wanted to read that as pwning. I take it you meant
panning and mice?

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2 6/9] Xext: store the bracket values for idle counters in the private

[Jamey Sharp]

A small detail:

On 3/14/12, Peter Hutterer peter.hutte...@who-t.net wrote:
  static void
  IdleTimeWakeupHandler (pointer pCounter, int rc, pointer LastSelectMask)
  {
 -SyncCounter *counter = IdleTimeCounter;
 +SyncCounter *counter = pCounter;
...
 @@ -2814,7 +2817,7 @@ IdleTimeWakeupHandler (pointer pCounter, int rc,
 pointer LastSelectMask)
  if ((greater  XSyncValueGreaterOrEqual (idle, *greater)) ||
  (less  XSyncValueLessOrEqual (idle, *less)))
  {
 - SyncChangeCounter (counter, idle);
 + SyncChangeCounter ((SyncCounter*)pCounter, idle);
  }
  }

You have a properly-typed counter in scope now, so you can drop this hunk.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2 9/9] Xext: Add per-device SyncCounters

[Jamey Sharp]

For the series v2: Looks good!

Reviewed-by: Jamey Sharp ja...@minilop.net

Although I'd suggest dropping the unnecessary cast I pointed out in patch 6.

Jamey

On 3/14/12, Peter Hutterer peter.hutte...@who-t.net wrote:
 Previously, we only had one idle alarm that was triggered for all devices,
 whenever the user used any device, came back from suspend, etc.

 Add system SyncCounters for each device (named DEVICEIDLETIME x, with x
 being the device id) that trigger on that device only. This allows for
 enabling/disabling devices based on interaction with other devices.

 Popular use-case: disable the touchpad when the keyboard just above the
 touchpad stops being idle.

 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 Reviewed-by: Jeremy Huddleston jerem...@apple.com
 ---
 Changes to v1:
 - adapt to SysCounterGetPrivate
 - adapt to SysCounterList being a linked list

  Xext/sync.c|   48 +++-
  Xext/syncsrv.h |3 +++
  dix/devices.c  |8 
  include/inputstr.h |2 ++
  4 files changed, 56 insertions(+), 5 deletions(-)

 diff --git a/Xext/sync.c b/Xext/sync.c
 index 5479381..c2f6573 100644
 --- a/Xext/sync.c
 +++ b/Xext/sync.c
 @@ -70,6 +70,7 @@ PERFORMANCE OF THIS SOFTWARE.
  #include syncsrv.h
  #include syncsdk.h
  #include protocol-versions.h
 +#include inputstr.h

  #include stdio.h
  #if !defined(WIN32)
 @@ -2715,12 +2716,22 @@ SyncInitServerTime(void)
  typedef struct {
  XSyncValue *value_less;
  XSyncValue *value_greater;
 +int deviceid;
  } IdleCounterPriv;

  static void
  IdleTimeQueryValue (pointer pCounter, CARD64 *pValue_return)
  {
 -CARD32 idle = GetTimeInMillis() -
 lastDeviceEventTime[XIAllDevices].milliseconds;
 +int deviceid;
 +CARD32 idle;
 +
 +if (pCounter) {
 +SyncCounter *counter = pCounter;
 +IdleCounterPriv *priv = SysCounterGetPrivate(counter);
 +deviceid = priv-deviceid;
 +} else
 +deviceid = XIAllDevices;
 +idle = GetTimeInMillis() - lastDeviceEventTime[deviceid].milliseconds;
  XSyncIntsToValue (pValue_return, idle, 0);
  }

 @@ -2813,7 +2824,7 @@ IdleTimeWakeupHandler (pointer pCounter, int rc,
 pointer LastSelectMask)
  if (!less  !greater)
   return;

 -IdleTimeQueryValue (NULL, idle);
 +IdleTimeQueryValue (pCounter, idle);

  if ((greater  XSyncValueGreaterOrEqual (idle, *greater)) ||
  (less  XSyncValueLessOrEqual (idle, *less)))
 @@ -2849,8 +2860,8 @@ IdleTimeBracketValues (pointer pCounter, CARD64
 *pbracket_less,
  priv-value_less= pbracket_less;
  }

 -static void
 -SyncInitIdleTime (void)
 +static SyncCounter*
 +init_system_idle_counter(const char *name, int deviceid)
  {
  CARD64 resolution;
  XSyncValue idle;
 @@ -2860,12 +2871,39 @@ SyncInitIdleTime (void)
  IdleTimeQueryValue (NULL, idle);
  XSyncIntToValue (resolution, 4);

 -idle_time_counter = SyncCreateSystemCounter(IDLETIME, idle,
 resolution,
 +idle_time_counter = SyncCreateSystemCounter(name, idle, resolution,
   XSyncCounterUnrestricted,
   IdleTimeQueryValue,
   IdleTimeBracketValues);

 +priv-deviceid = deviceid;
  priv-value_less = priv-value_greater = NULL;

  idle_time_counter-pSysCounterInfo-private = priv;
 +
 +return idle_time_counter;
 +}
 +
 +static void
 +SyncInitIdleTime (void)
 +{
 +init_system_idle_counter(IDLETIME, XIAllDevices);
  }
 +
 +SyncCounter*
 +SyncInitDeviceIdleTime(DeviceIntPtr dev)
 +{
 +char timer_name[64];
 +sprintf(timer_name, DEVICEIDLETIME %d, dev-id);
 +
 +return init_system_idle_counter(timer_name, dev-id);
 +}
 +
 +void SyncRemoveDeviceIdleTime(SyncCounter *counter)
 +{
 +/* ResetProc frees the list before devices are shutdown and try to
 + * delete their counters again */
 +if (!xorg_list_is_empty(SysCounterList))
 +xorg_list_del(counter-pSysCounterInfo-entry);
 +}
 +
 diff --git a/Xext/syncsrv.h b/Xext/syncsrv.h
 index 27b533c..84c7aee 100644
 --- a/Xext/syncsrv.h
 +++ b/Xext/syncsrv.h
 @@ -139,4 +139,7 @@ extern void SyncDestroySystemCounter(
  );

  extern void SyncExtensionInit(void);
 +
 +extern SyncCounter *SyncInitDeviceIdleTime(DeviceIntPtr dev);
 +extern void SyncRemoveDeviceIdleTime(SyncCounter *counter);
  #endif /* _SYNCSRV_H_ */
 diff --git a/dix/devices.c b/dix/devices.c
 index 7478ad6..a488d41 100644
 --- a/dix/devices.c
 +++ b/dix/devices.c
 @@ -87,6 +87,7 @@ SOFTWARE.
  #include enterleave.h /* for EnterWindow() */
  #include xserver-properties.h
  #include xichangehierarchy.h /* For XISendDeviceHierarchyEvent */
 +#include syncsrv.h

  /** @file
   * This file handles input device-related stuff.
 @@ -419,9 +420,13 @@ EnableDevice(DeviceIntPtr dev, BOOL sendevent)

  RecalculateMasterButtons(dev);

 +/* initialise an idle timer for this device

Re: [PATCH 0/9] per-device idle counters

[Jamey Sharp]

This series seems like a good idea to me! I have a few review comments though:

Patch 1 seems strange. Shouldn't the counters themselves get freed at
reset? If they are, shouldn't that lead to the number of counters
reaching 0?

I guess the input ABI should get bumped for the patch that stores
per-device idle times. We decided to bump ABI more aggressively,
right?

I haven't checked: is it possible to build xserver without SYNC, or to
disable it at runtime? If so, the final patch needs some conditionals,
I assume.

Here's a request you can ignore if you like: Please consider making
SYSCOUNTERPRIV a static inline instead of a macro, and removing the
cast from inside it. That'd be different from the usual idiom in the X
server, but the usual idiom is dumb. :-) I'd prefer to see all the
callers assign their pointer pCounters to an appropriately-typed
local once right at the beginning of the function, which also serves
as documentation for what type you're supposed to pass in there.

The series looks fine generally; these are just minor details.

Jamey

On 3/13/12, Peter Hutterer peter.hutte...@who-t.net wrote:

 We currently have one global idlecounter. This patch series adds additional
 counters named DEVICEIDLETIME n (where n is the device id) that apply to
 that device only.

 One use-case here is as syndaemon replacement. A client can simply put an
 idlecounter on the keyboard above the touchpad. When that keyboard goes out
 of idle, disable the touchpad, then re-enable when the keyboard goes idle
 again.

 Cheers,
   Peter
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 0/9] per-device idle counters

[Jamey Sharp]

On Wed, Mar 14, 2012 at 4:30 PM, Peter Hutterer
peter.hutte...@who-t.net wrote:
 On Wed, Mar 14, 2012 at 09:01:13AM -0700, Jamey Sharp wrote:
 This series seems like a good idea to me! I have a few review comments 
 though:

 Patch 1 seems strange. Shouldn't the counters themselves get freed at
 reset? If they are, shouldn't that lead to the number of counters
 reaching 0?

 you're right but the order is a tad weird. The ResetProc is called before
 the counters are cleaned up, so we free the container list but the number is
 still correct. Later, the number goes down to 0 when the counters are
 actually freed. Technically correct, but weird. I think the diff below would
 make sense to merge into patch 1/9. This way, we remove all knowledge of
 system counters in the ResetProc and don't get any weird effects.

 diff --git a/Xext/sync.c b/Xext/sync.c
 index 6c8c564..bf47c93 100644
 --- a/Xext/sync.c
 +++ b/Xext/sync.c
 @@ -1200,8 +1200,8 @@ FreeCounter(void *env, XID id)
                    SysCounterList[i] = SysCounterList[i+1];
                }
            }
 +            SyncNumSystemCounters--;
        }
 -       SyncNumSystemCounters--;
     }
     free(pCounter);
     return Success;

This seems like increasing the fragility of the code...

I ran out of time to actually test, but the attached alternative patch
compiles, at least, and the diffstat makes me happy. What say you?

 in configure.ac:1312, it's always defined, so no conditionals are necessary.
 AC_DEFINE(XSYNC, 1, [Support XSync extension])

Good. I assume disabling it at runtime doesn't hurt anything either?
If that's possible...

 Here's a request you can ignore if you like: Please consider making
 SYSCOUNTERPRIV a static inline instead of a macro, and removing the
 cast from inside it. That'd be different from the usual idiom in the X
 server, but the usual idiom is dumb. :-) I'd prefer to see all the
 callers assign their pointer pCounters to an appropriately-typed
 local once right at the beginning of the function, which also serves
 as documentation for what type you're supposed to pass in there.

 How does this one look then? I'd squash this in but it's easier to review as
 a diff on top.

Looks perfect, except I'm a bit baffled at the NULL check: I'm hoping
the pCounters can't actually ever be null, but you're making me
wonder...?

 From cc02b58e95822a50658ef7fe13e862c3f569ee22 Mon Sep 17 00:00:00 2001
 From: Peter Hutterer peter.hutte...@who-t.net
 Date: Thu, 15 Mar 2012 09:27:24 +1000
 Subject: [PATCH] Xext: replace the macro with a static inline function.

 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
  Xext/sync.c    |   24 ++--
  Xext/syncsrv.h |    2 --
  2 files changed, 18 insertions(+), 8 deletions(-)

 diff --git a/Xext/sync.c b/Xext/sync.c
 index 6c8c564..5f74dab 100644
 --- a/Xext/sync.c
 +++ b/Xext/sync.c
 @@ -115,6 +115,15 @@ static void SyncInitServerTime(void);

  static void SyncInitIdleTime(void);

 +static inline void*
 +SysCounterGetPrivate(SyncCounter *counter)
 +{
 +    BUG_WARN(!IsSystemCounter(counter));
 +
 +    return counter-pSysCounterInfo ? counter-pSysCounterInfo-private : 
 NULL;
 +}
 +
 +
  static Bool
  SyncCheckWarnIsCounter(const SyncObject* pSync, const char *warning)
  {
 @@ -2747,7 +2756,8 @@ IdleTimeQueryValue (pointer pCounter, CARD64 
 *pValue_return)
     CARD32 idle;

     if (pCounter) {
 -        IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter);
 +        SyncCounter *counter = pCounter;
 +        IdleCounterPriv *priv = SysCounterGetPrivate(counter);
         deviceid = priv-deviceid;
     } else
         deviceid = XIAllDevices;
 @@ -2758,8 +2768,8 @@ IdleTimeQueryValue (pointer pCounter, CARD64 
 *pValue_return)
  static void
  IdleTimeBlockHandler(pointer pCounter, struct timeval **wt, pointer 
 LastSelectMask)
  {
 -    IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter);
     SyncCounter *counter = pCounter;
 +    IdleCounterPriv *priv = SysCounterGetPrivate(counter);
     XSyncValue *less = priv-value_less,
               *greater = priv-value_greater;
     XSyncValue idle, old_idle;
 @@ -2835,7 +2845,8 @@ IdleTimeBlockHandler(pointer pCounter, struct timeval 
 **wt, pointer LastSelectMa
  static void
  IdleTimeWakeupHandler (pointer pCounter, int rc, pointer LastSelectMask)
  {
 -    IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter);
 +    SyncCounter *counter = pCounter;
 +    IdleCounterPriv *priv = SysCounterGetPrivate(counter);
     XSyncValue *less = priv-value_less,
               *greater = priv-value_greater;
     XSyncValue idle;
 @@ -2856,7 +2867,8 @@ static void
  IdleTimeBracketValues (pointer pCounter, CARD64 *pbracket_less,
                        CARD64 *pbracket_greater)
  {
 -    IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter);
 +    SyncCounter *counter = pCounter;
 +    IdleCounterPriv *priv = SysCounterGetPrivate(counter);
     XSyncValue *less = priv-value_less,
               *greater = priv-value_greater;
     Bool registered

Re: [PATCH] include: add an X_DEBUG message type

[Jamey Sharp]

The #ifndef in log.c looks wrong, but otherwise, sure, why not? With that fixed:

Reviewed-by: Jamey Sharp ja...@minilop.net

On 3/13/12, Peter Hutterer peter.hutte...@who-t.net wrote:
 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
 Just a nice-to-have, makes grep a tad easier.

  include/os.h |1 +
  os/log.c |5 +
  2 files changed, 6 insertions(+), 0 deletions(-)

 diff --git a/include/os.h b/include/os.h
 index 48ce329..1e90cc5 100644
 --- a/include/os.h
 +++ b/include/os.h
 @@ -516,6 +516,7 @@ typedef enum {
  X_INFO,  /* Informational message */
  X_NONE,  /* No prefix */
  X_NOT_IMPLEMENTED,   /* Not implemented */
 +X_DEBUG, /* Debug message */
  X_UNKNOWN = -1   /* unknown -- this must always be last */
  } MessageType;

 diff --git a/os/log.c b/os/log.c
 index 671a01b..5a2e5a8 100644
 --- a/os/log.c
 +++ b/os/log.c
 @@ -164,6 +164,9 @@ asm (.desc ___crashreporter_info__, 0x10);
  #ifndef X_NOT_IMPLEMENTED_STRING
  #define X_NOT_IMPLEMENTED_STRING (NI)
  #endif
 +#ifndef X_DEBUG
 +#define X_DEBUG_STRING  (DB)
 +#endif
  #ifndef X_NONE_STRING
  #define X_NONE_STRING   
  #endif
 @@ -361,6 +364,8 @@ LogMessageTypeVerbString(MessageType type, int verb)
   return X_UNKNOWN_STRING;
  case X_NONE:
   return X_NONE_STRING;
 +case X_DEBUG:
 + return X_DEBUG_STRING;
  default:
   return X_UNKNOWN_STRING;
  }
 --
 1.7.7.6

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Cannot wait for user lock in sync_while_locked

[Jamey Sharp]

For both Block for other threads in _XUserLockDisplay and Restore
Xlib semantics of blocking on user lock from _XReadEvents:

Reviewed-by: Jamey Sharp ja...@minilop.net

Although, in Restore Xlib semantics, it would be nice if you'd fix
up the It appears that classic Xlib respected user locks comment. I
think it suffices to delete the final sentence, So we'll choose to
let the thread that got in first consume events, despite the later
thread's user locks.

Thanks for the careful review and detailed summary!
Jamey

On Fri, Mar 9, 2012 at 4:31 PM, Keith Packard kei...@keithp.com wrote:
 #part sign=pgpmime
 On Sun, 04 Mar 2012 02:04:27 -0800, Keith Packard kei...@keithp.com wrote:

 diff --git a/src/locking.c b/src/locking.c
 index 4f9a40f..b3dfb3b 100644
 --- a/src/locking.c
 +++ b/src/locking.c
 @@ -486,6 +486,8 @@ static void _XInternalLockDisplay(
  static void _XUserLockDisplay(
      register Display* dpy)
  {
 +    _XDisplayLockWait(dpy);
 +
      if (++dpy-lock-locking_level == 1) {
         dpy-lock-lock_wait = _XDisplayLockWait;
         dpy-lock-locking_thread = xthread_self();

 I made Bart think about this over lunch today and I think this is
 the right fix for this problem.

 Here's a lengthy discussion of the code. It took me a long time to
 study the code, and I'm sorry that it will take you a long time to just
 read this message...

 Recall that the problem occurs when a thread acquires the Display mutex
 without waiting for locking_level to go to zero. If it then enters
 XUserLockDisplay, it will increment locking_level, at which point we
 effectively have two separate threads owning the 'user lock' and chaos
 ensues.

 The 'user lock' is a recursive mutex which is implemented with the
 following two values:

        1) locking_level
                This counts the number of acquisitions of the lock, the
                lock is held until the count returns to zero.

        2) locking_thread
                This marks which thread took the lock the first time. This
                thread will not block in _XDisplayLockWait, which is how
                the mutex can recurse.

 The implementation is a bit weird -- the usual blocking part of mutex
 acquisition is separated from the increment of the count. That's because
 of the weird semantics of XLockDisplay/XUnlockDisplay. Those functions
 implement a mutex that affects *all* threads using Xlib, not just those
 sharing in the XLockDisplay/XUnlockDisplay fun.

 When a thread calls XLockDisplay, every other thread using Xlib is
 locked out of some Xlib activities. This is done by blocking every other
 thread in LockDisplay while the locking_level remains positive, while
 letting the thread holding the lock through by comparing its thread-id
 with locking_thread. Simple, and allows for recursive locks as
 locking_level can be incremented more than once.

 However, it doesn't allow for more than one thread to hold the 'user
 lock'. The current implementation of _XUserLockDisplay must therefore
 assume that any thread calling it has already ensured that locking_level
 is zero, or that the calling thread is the locking_thread. This is
 always true after _XLockDisplay is invoked -- that always calls
 _XDisplayLockWait to block execution until locking_level is zero. It is
 *not* always true after _XInternalLockDisplay is called; when 'wskip' is
 non-zero, locking_level may be non-zero, and locking_thread may well be
 another thread.

 *** Obvious Fix #1: 

 An obvious fix would seem to be to just make _XInternalLockDisplay
 always wait for a thread holding the 'user lock' to release it (forcing
 'wskip' to zero). That doesn't work in one important case -- in
 _XReadEvents. In that function, an event is read from xcb, then the
 mutex re-acquired and the event posted to the display. Only one thread
 is allowed to wait in xcb for the event. Other event readers will pend
 on the 'event_notify' condition variable. Once the event is read from
 xcb, the mutex is re-acquired, the event posted to Xlib and threads
 waiting on the condition variable awoken with a broadcast.

 Consider the sequence:

        Thread A                        Thread B
        XNextEvent
          LockDisplay
          _XReadEvents
            UnlockDisplay
            xcb_wait_for_event
                                        XLockDisplay
                                          LockDisplay
                                          _XUserLockDisplay    (locking level 
 1)
                                          UnlockDisplay
                                        XNextEvent
                                          LockDisplay
                                          _XReadEvents
                                            ConditionWait
                                              UnlockDisplay
            InternalLockDisplay                                (waiting for 
 locking_level to go to zero?)

 If the 'InternalLockDisplay' call were

Re: [PATCH] Cannot wait for user lock in sync_while_locked

[Jamey Sharp]

On Sat, Mar 10, 2012 at 4:04 PM, Keith Packard kei...@keithp.com wrote:
 On Sat, 10 Mar 2012 14:31:52 -0800, Jamey Sharp ja...@minilop.net wrote:
 Although, in Restore Xlib semantics, it would be nice if you'd fix
 up the It appears that classic Xlib respected user locks comment. I
 think it suffices to delete the final sentence, So we'll choose to
 let the thread that got in first consume events, despite the later
 thread's user locks.

 something like:

 -                        * deadlock. So we'll choose to let the thread
 -                        * that got in first consume events, despite the
 -                        * later thread's user locks. */
 +                        * deadlock. So we let the thread got in first read
 +                        * events, then signal other waiting threads and go
 +                        * back to check for a user lock */

Looks great; just insert let the thread [that] got in first and push. :-)

 Thanks for the careful review and detailed summary!

 Xlib locking code is hard. Let's go rocketing!

OMG rocket science is so much easier than Xlib surgery.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xfree86: always switch console to raw mode on vt switch (#41146)

[Jamey Sharp]

On Wed, Mar 7, 2012 at 8:21 PM, Peter Hutterer peter.hutte...@who-t.net wrote:
 Alt+SysReq+R sets the console to raw mode. Re-enable raw mode when
 VT-switching back to the server.

This commit message is confusing me. When VT-switching to the server,
you want to set the console to the same mode it might have been set to
with a sysrq key?

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libxcb] Fallback to TCP if no protocol is specified and the UNIX connection fails

[Jamey Sharp]

I've pushed this patch; thanks!

I'll let you and Jeremy sort out the libX11 side.

Jamey

On Sun, Feb 19, 2012 at 01:43:54PM +0100, Julien Cristau wrote:
 Signed-off-by: Julien Cristau jcris...@debian.org
 ---
  src/xcb_util.c |5 +
  1 files changed, 5 insertions(+), 0 deletions(-)
 
 diff --git a/src/xcb_util.c b/src/xcb_util.c
 index fde4f85..6d9efe0 100644
 --- a/src/xcb_util.c
 +++ b/src/xcb_util.c
 @@ -234,6 +234,11 @@ static int _xcb_open(const char *host, char *protocol, 
 const int display)
  fd = _xcb_open_unix(protocol, file);
  free(file);
  
 +if (fd  0  !protocol  *host == '\0') {
 + unsigned short port = X_TCP_PORT + display;
 + fd = _xcb_open_tcp(host, protocol, port);
 +}
 +
  return fd;
  #endif /* !_WIN32 */
  return -1; /* if control reaches here then something has gone wrong */
 -- 
 1.7.9
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH lib/libX11] Don't use caddr_t casts

[Jamey Sharp]

On Sat, Feb 18, 2012 at 05:21:24PM +, Jon TURNEY wrote:
 Casting a (const char *) to (caddr_t) to assign to iovec.io_base
 seems pointless. caddr_t isn't used anywhere else in xcb or libX11

According to the libxcb git history, I replaced (caddr_t) with (char *)
in 2006 to help DragonFly and Solaris. I'd be fine with this patch if
it explicitly cast to (char *), which I believe suppresses the constness
warning.

 Note: there's a warning about dropping constness here, but that's
 going to be unfixable as long as xcb_writev() takes a non-const struct
 iovec as a parameter.

C's rules regarding const always confuse me, but I'm pretty sure even
that wouldn't help. If the struct iovec is declared const, that just
means XCB won't change where the iov_base pointers point. It doesn't
mean XCB promises to refrain from writing through those pointers.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xorg-gtest 2/3] Install configuration file in /etc/X11 instead of /ush/share/xorg/gtest

[Jamey Sharp]

This should be fixed by commit ead968a4300c0adeff89b9886e888b6d284c75cc,
already on master--right?

Jamey

On Mon, Jan 30, 2012 at 03:32:19PM -0200, Daniel d'Andrada wrote:
 This is another enabler for making it possible to run Xorg without
 root privileges.
 
 The -config parameter in Xorg doesn't accept files in paths outside
 Xorg's standard search path for config files when run as non-root.
 See -config in man Xorg for more info.
 
 You have to run configure with --sysconfdir=/etc so that our conf
 file lands in Xorg's standard path for configuration files, namely /etc/X11
 
 Signed-off-by: Daniel d'Andrada daniel.dandr...@canonical.com
 
 diff --git a/Makefile.am b/Makefile.am
 index d10bca8..24f6f7b 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -20,13 +20,12 @@ library_include_HEADERS = \
  include/xorg/gtest/process.h \
  include/xorg/gtest/test.h
 
 -library_datadir = $(datadir)/xorg/gtest
 -library_data_DATA = conf/dummy.conf
 +library_datadir = $(sysconfdir)/X11
 +library_data_DATA = conf/gtest-dummy.conf
 
  libxorg_gtest_main_la_CPPFLAGS = \
  $(AM_CPPFLAGS) \
 -$(GTEST_CPPFLAGS) \
 --DDUMMY_CONF_PATH=\$(library_datadir)/dummy.conf\
 +$(GTEST_CPPFLAGS)
 
  libxorg_gtest_la_LDFLAGS = $(X11_LIBS)
  libxorg_gtest_main_la_LDFLAGS = $(X11_LIBS)
 diff --git a/configure.ac b/configure.ac
 index 9d7b36b..d7d1682 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -36,7 +36,7 @@ AS_IF([test x$ac_cv_lib_gtest_main != xyes],
 
  AC_SUBST([GTEST_CPPFLAGS])
 
 -AC_SUBST(DUMMY_CONF_PATH, $datadir/xorg/gtest/dummy.conf)
 +AC_SUBST(DUMMY_CONF_PATH, $sysconfdir/X11/gtest-dummy.conf)
 
  AC_CONFIG_FILES([Makefile
   xorg-gtest.pc])
 diff --git a/src/main.cpp b/src/main.cpp
 index eb6e728..2d45468 100644
 --- a/src/main.cpp
 +++ b/src/main.cpp
 @@ -26,6 +26,7 @@
  #include xorg/gtest/environment.h
 
  #define DEFAULT_XORG_LOGFILE /tmp/Xorg.GTest.log
 +#define DEFAULT_XORG_CONFIG gtest-dummy.conf
 
  namespace {
 
 @@ -50,7 +51,7 @@ const struct option longopts[] = {
 
  int main(int argc, char *argv[]) {
/* Default Xorg dummy conf path. */
 -  std::string xorg_conf_path(DUMMY_CONF_PATH);
 +  std::string xorg_conf_path(DEFAULT_XORG_CONFIG);
 
std::string xorg_logfile_path(DEFAULT_XORG_LOGFILE);
 
 @@ -102,7 +103,8 @@ int main(int argc, char *argv[]) {
  std::cout  \nAdditional options:\n;
  std::cout  --no-dummy-server: Use the currently running
 X server 
  for testing\n;
 -std::cout  --xorg-conf: Path to xorg dummy configuration
 file\n;
 +std::cout  --xorg-conf: Path to xorg dummy configuration
 file. See -config in \man Xorg\.\n
 +  Its default value is
 DEFAULT_XORG_CONFIG.\n;
  std::cout  --server: Path to X server executable\n;
  std::cout  --xorg-display: xorg dummy display port\n;
  std::cout  --xorg-logfile: xorg logfile filename. See
 -logfile in \man Xorg\.\n
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Remove XAA

[Jamey Sharp]

On Wed, Jan 18, 2012 at 03:05:41PM +1100, Daniel Stone wrote:
 commit 3811d1b85b71c1f8bd0b3c8269e0f7d388a51c2b
 Author: Daniel Stone dan...@fooishbar.org
 Date:   Wed Jan 18 15:01:09 2012 +1100
 
 Remove XAA
 
 It hasn't worked for over four years.  No-one's even come close to
 fixing it.

I love this. It invalidates some of my outstanding patches in the best
possible way. But... could you explain (preferably in the commit
message) what evidence you have that it hasn't worked? I thought
people were still using XAA.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Remove XAA

[Jamey Sharp]

On Thu, Jan 19, 2012 at 03:43:39PM +1100, Daniel Stone wrote:
 On 19 January 2012 04:03, Jamey Sharp ja...@minilop.net wrote:
  I love this. It invalidates some of my outstanding patches in the best
  possible way. But... could you explain (preferably in the commit
  message) what evidence you have that it hasn't worked? I thought
  people were still using XAA.
 
 Ah, fair point.  I guess it wasn't _completely_ non-functional as
 such, but almost entirely so; the only time it'd be useful is if you
 were playing xtank in a non-composited environment.  Here's the
 updated commit message:

Much better, thanks! Makes sense to me.

I seem to recall Nvidia was loading the XAA module because it was the
only entry point they could use to get at some symbol, but they don't
actually use the acceleration architecture, because that would be crazy.
If I have that right, I imagine we'll get some discussion about that
when Aaron notices this thread.

I haven't reviewed the patch, but I approve in principle. Is that an
acked-by? Feel free to add one, or not.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH GL 1/1] XQuartz: GL: Buildfix for recent GLX changes

[Jamey Sharp]

I sure don't see any reason not to merge that.

Reviewed-by: Jamey Sharp ja...@minilop.net

On Tue, Dec 20, 2011 at 07:49:47PM -0800, Jeremy Huddleston wrote:
 dispatch.h was leftover from an earlier implementation and is no longer
 needed, so remove it since including it causes a build failure due to
 conflicts between GL/gl.h and OpenGL/gl.h
 
 Signed-off-by: Jeremy Huddleston jerem...@apple.com
 ---
  hw/xquartz/GL/indirect.c |3 ---
  1 files changed, 0 insertions(+), 3 deletions(-)
 
 diff --git a/hw/xquartz/GL/indirect.c b/hw/xquartz/GL/indirect.c
 index 27d6dae..e6ff376 100644
 --- a/hw/xquartz/GL/indirect.c
 +++ b/hw/xquartz/GL/indirect.c
 @@ -48,9 +48,6 @@
  #include glxserver.h
  #include glxutil.h
  
 -typedef unsigned long long GLuint64EXT;
 -typedef long long GLint64EXT;
 -#include dispatch.h
  #include glapi.h
  
  #include x-hash.h
 -- 
 1.7.7.4
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xrandr-utils 0/6] Refactor xrandr internals into libxrandr-utils

[Jamey Sharp]

On Wed, Dec 21, 2011 at 06:47:07PM -0800, Bryce Harrington wrote:
 Begin refactoring the xrandr command line tool's internals into a
 utility library.  This is to provide a higher level abstraction for
 configuring multiple monitors.
 
 With the coming complexities of hybrid graphics, 2 heads, and so on, it
 will hopefully save headaches all around to have a central place for the
 configuration logic, rather than duplicated across Qt, gtk, and other
 client-side tools.

I'm entirely in favor of this goal. But I'd really like to see
newly-developed X library APIs exposed as pure XCB APIs at this point.
Pretty much everyone is shipping Xlib/XCB now, so it's no hardship for
higher-layer Xlib-based code to call XCB-only libraries, and doing it
now saves us having to migrate callers later.

Anyway, I heard Qt5 will ship with XCB as the default implementation, so
I expect more demand for this sort of porting from them.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: adding RandR support to dummy (was adding a custom modeline with xrandr..)

[Jamey Sharp]

On Mon, Dec 19, 2011 at 03:17:54PM +0700, Antoine Martin wrote:
 On 10/24/2011 06:26 PM, Jamey Sharp wrote:
  Off-hand, I'd guess nobody has implemented RANDR support in 
  xf86-video-dummy. But since I don't know what drivers need to do
  for that, for all I know there could be some generic support in the
  core that's failing here, instead.
 
 ...
 So I would like to add better RandR support to dummy which should be
 relatively straightforward, but I cannot find any reference
 implementation or documentation on which methods need to be
 implemented and how.. I found that drivers include xorg/xf86RandR12.h
 Calling xf86RandR12Init / PreInit only got me segfaults so far..
 Are there any useful pointers to help me get started?

I would like to know the answer to this question too. :-) Sooner or
later I want RANDR support in xf86-video-nested as well.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/2] configure.ac: Make Xephyr dependency error message more informative

[Jamey Sharp]

Reviewed-by: Jamey Sharp ja...@minilop.net

On 12/17/11, Jeremy Huddleston jerem...@apple.com wrote:
 Signed-off-by: Jeremy Huddleston jerem...@apple.com
 ---
  configure.ac |8 +++-
  1 files changed, 3 insertions(+), 5 deletions(-)

 diff --git a/configure.ac b/configure.ac
 index 518eb06..eb99c70 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -2020,12 +2020,10 @@ if test $KDRIVE = yes; then
  XEPHYR_REQUIRED_LIBS=$XEPHYR_REQUIRED_LIBS $LIBGL libdrm
  fi

 -PKG_CHECK_MODULES(XEPHYR, $XEPHYR_REQUIRED_LIBS, [xephyr=yes],
 [xephyr=no])
  if test x$XEPHYR = xauto; then
 -XEPHYR=$xephyr
 -fi
 -if test x$XEPHYR = xyes  test x$xephyr = xno; then 
 -AC_MSG_ERROR([Xephyr dependencies missing])
 +PKG_CHECK_MODULES(XEPHYR, $XEPHYR_REQUIRED_LIBS, [XEPHYR=yes],
 [XEPHYR=no])
 +elif test x$XEPHYR = xyes ; then
 +PKG_CHECK_MODULES(XEPHYR, $XEPHYR_REQUIRED_LIBS)
  fi

  # Xephyr needs nanosleep() which is in librt on Solaris
 --
 1.7.7.4

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2 02/42] dix: split positionSprite into scale_to_desktop and positionSprite

[Jamey Sharp]

Welp, I'm happy!

Reviewed-by: Jamey Sharp ja...@minilop.net

On Fri, Dec 16, 2011 at 10:28:31AM +1000, Peter Hutterer wrote:
 For future touch points, we need positionSprite to calculate the coordinates
 but we don't want to actually change the cursor position for non-emulating
 touches.
 
 No functional changes at this point.
 
 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
 Changes to v1:
 - instead of an update argument to positionSprite to exit early (before the
   miPointerSetPosition call), split into two functions.
 
 This implies a change to [PATCH 19/42 dix: generate touchpoints from
 driver-submitted data] which changed from
 
 scr = positionSprite(pDev, emulate_pointer, )
 to
 scr = scale_to_desktop(...)
 if (emulate_pointer)
scr = positionSprite(...)
 
 I'll skip that patch to keep noise low on the list.
 
  dix/getevents.c |   72 +++---
  1 files changed, 52 insertions(+), 20 deletions(-)
 
 diff --git a/dix/getevents.c b/dix/getevents.c
 index 57d8c17..ea62ca8 100644
 --- a/dix/getevents.c
 +++ b/dix/getevents.c
 @@ -786,36 +786,28 @@ scale_from_screen(DeviceIntPtr dev, ValuatorMask *mask)
  
  
  /**
 - * If we have HW cursors, this actually moves the visible sprite. If not, we
 - * just do all the screen crossing, etc.
 - *
 - * We scale from device to screen coordinates here, call
 - * miPointerSetPosition() and then scale back into device coordinates (if
 - * needed). miPSP will change x/y if the screen was crossed.
 + * Scale from (absolute) device to screen coordinates here,
   *
 - * The coordinates provided are always absolute. The parameter mode
 - * specifies whether it was relative or absolute movement that landed us at
 - * those coordinates. see fill_pointer_events for information on coordinate
 - * systems.
 + * The coordinates provided are always absolute. see fill_pointer_events for
 + * information on coordinate systems.
   *
   * @param dev The device to be moved.
 - * @param mode Movement mode (Absolute or Relative)
 - * @param[in,out] mask Mask of axis values for this event, returns the
 - * per-screen device coordinates after confinement
 + * @param mask Mask of axis values for this event
   * @param[out] devx x desktop-wide coordinate in device coordinate system
   * @param[out] devy y desktop-wide coordinate in device coordinate system
   * @param[out] screenx x coordinate in desktop coordinate system
   * @param[out] screeny y coordinate in desktop coordinate system
   */
  static ScreenPtr
 -positionSprite(DeviceIntPtr dev, int mode, ValuatorMask *mask,
 -   double *devx, double *devy,
 -   double *screenx, double *screeny)
 +scale_to_desktop(DeviceIntPtr dev, ValuatorMask *mask,
 + double *devx, double *devy,
 + double *screenx, double *screeny)
  {
 -double x, y;
 -double tmpx, tmpy;
  ScreenPtr scr = miPointerGetScreen(dev);
 +double x, y;
  
 +BUG_WARN(!dev-valuator);
 +BUG_WARN(dev-valuator-numAxes  2);
  if (!dev-valuator || dev-valuator-numAxes  2)
  return scr;
  
 @@ -834,11 +826,48 @@ positionSprite(DeviceIntPtr dev, int mode, ValuatorMask 
 *mask,
  *screeny = rescaleValuatorAxis(y, dev-valuator-axes + 1, NULL,
 screenInfo.y, screenInfo.height);
  
 -tmpx = *screenx;
 -tmpy = *screeny;
  *devx = x;
  *devy = y;
  
 +return scr;
 +}
 +
 +/**
 + * If we have HW cursors, this actually moves the visible sprite. If not, we
 + * just do all the screen crossing, etc.
 + *
 + * We use the screen coordinates here, call miPointerSetPosition() and then
 + * scale back into device coordinates (if needed). miPSP will change x/y if
 + * the screen was crossed.
 + *
 + * The coordinates provided are always absolute. The parameter mode
 + * specifies whether it was relative or absolute movement that landed us at
 + * those coordinates. see fill_pointer_events for information on coordinate
 + * systems.
 + *
 + * @param dev The device to be moved.
 + * @param mode Movement mode (Absolute or Relative)
 + * @param[out] mask Mask of axis values for this event, returns the
 + * per-screen device coordinates after confinement
 + * @param[in,out] devx x desktop-wide coordinate in device coordinate system
 + * @param[in,out] devy y desktop-wide coordinate in device coordinate system
 + * @param[in,out] screenx x coordinate in desktop coordinate system
 + * @param[in,out] screeny y coordinate in desktop coordinate system
 + */
 +static ScreenPtr
 +positionSprite(DeviceIntPtr dev, int mode, ValuatorMask *mask,
 +   double *devx, double *devy,
 +   double *screenx, double *screeny)
 +{
 +ScreenPtr scr = miPointerGetScreen(dev);
 +double tmpx, tmpy;
 +
 +if (!dev-valuator || dev-valuator-numAxes  2)
 +return scr;
 +
 +tmpx = *screenx;
 +tmpy = *screeny;
 +
  /* miPointerSetPosition takes care of crossing

Re: [PATCH 0/4] Move GC clip management to ScreenRec

[Jamey Sharp]

Could you have a look at my patches to do this, from October? I believe
Keith just wanted an ABI bump before merge, but I'd run out of time by
then, and now I assume it's too late for this cycle.

They're still sitting on my reviewed branch:

http://cgit.freedesktop.org/~jamey/xserver/log/?h=reviewed

Here's part of the discussion thread:

http://lists.x.org/archives/xorg-devel/2011-October/025978.html

Jamey

On Mon, Dec 05, 2011 at 01:56:13PM -0500, Adam Jackson wrote:
 None of the GC wrappers do anything particularly interesting with clip
 management.  I think in theory this was meant to allow the driver to
 flatten the clip to a bitmap and/or keep track of a window ID, but the
 only example I can find of anything remotely like that in open code is
 Xprint's PostScript backend, which, well.
 
 I've not tested this in any real sense.  It would want at least an xts5
 run against Xnest and Xvfb before committing.  The CopyClip change might
 want to be a screen func as well for symmetry.
 
 Longer term I don't think there's any reason to have GCFuncs at all, and
 that the whole thing could be embedded in the ScreenRec.
 
 Dead code diff:
 
text  data bss dec hex filename
 1844275 52216   64384 1960875  1debab Xorg-clip-gc
 1842034 52120   64384 1958538  1de28a Xorg-clip-screen
 
 - ajax
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PULL] GC clipping cleanup

[Jamey Sharp]

On Mon, Oct 17, 2011 at 02:06:55PM -0700, Keith Packard wrote:
 On Wed, 5 Oct 2011 08:30:56 -0700, Jamey Sharp ja...@minilop.net wrote:
  Alright, the pixmap hooks have weird undocumented constraints, and
  maybe we'll figure out something clever someday. Meanwhile, would you
  please merge the other three commits?
 
 For other ABI changes like this, we've provided compile-time tests you
 can use to switch the code without using the X server ABI version
 information. That way, it's easy to make video drivers compile across
 the change.
 
 The hardest part will be picking a suitable name which makes the
 difference reasonably clear.
 
 Otherwise, these changes look good to me; fixing the driver to handle
 the change was straightforward.

Oh. The merge window isn't closed yet? Huh.

I've added a video ABI bump to the second of the two commits. That will
do, I hope?

The following changes since commit f7ea7a324520844beaac8be7503ac50e20da5ab1:

  configure: split the required modules up (2011-12-14 11:48:14 -0800)

are available in the git repository at:
  git://anongit.freedesktop.org/~jamey/xserver cleanup-gcclip

Jamey Sharp (2):
  Quit wrapping ChangeClip, CopyClip, and DestroyClip in GCFuncs.
  Make GC clientClip always be a region, and bump video ABI to 13.

 Xext/panoramiX.c   |   32 --
 dix/gc.c   |   64 ++---
 doc/Xserver-spec.xml   |   76 +--
 exa/exa.c  |   48 -
 exa/exa_accel.c|   18 ++--
 exa/exa_priv.h |6 +-
 exa/exa_unaccel.c  |6 +-
 fb/fbgc.c  |3 -
 hw/dmx/dmxgc.c |  115 +++---
 hw/dmx/dmxgc.h |3 -
 hw/kdrive/src/kxv.c|2 +-
 hw/xfree86/common/xf86Module.h |2 +-
 hw/xfree86/common/xf86VGAarbiter.c |   31 +--
 hw/xfree86/common/xf86VGAarbiterPriv.h |4 -
 hw/xfree86/common/xf86xv.c |2 +-
 hw/xfree86/shadowfb/shadow.c   |   35 ---
 hw/xfree86/xaa/xaaBitBlt.c |4 +-
 hw/xfree86/xaa/xaaGC.c |   31 --
 hw/xnest/GC.c  |  170 +---
 hw/xnest/XNGC.h|4 -
 hw/xwin/wingc.c|   42 
 include/gc.h   |2 +-
 include/gcstruct.h |   25 ++---
 mi/mibitblt.c  |4 +-
 mi/micopy.c|4 +-
 mi/miexpose.c  |2 +-
 mi/migc.c  |   74 +--
 mi/migc.h  |   16 ---
 mi/mioverlay.c |2 +-
 miext/cw/cw.c  |   52 +-
 miext/damage/damage.c  |   31 --
 miext/rootless/rootlessGC.c|   29 --
 render/mirect.c|2 +-
 xfixes/region.c|   27 ++
 34 files changed, 164 insertions(+), 804 deletions(-)


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 02/42] dix: add an update argument to positionSprite

[Jamey Sharp]

This patch obviously doesn't change any behavior, but wouldn't it be
more clear to just factor out the beginning of positionSprite to a
computeSpritePosition or something?

I suppose there's a later patch that justifies this approach, but I
can't help thinking that returning early by adding another argument to
a 7-argument function with 5 out-parameters, a return value, and side
effects, is going to mean pain maintaining invariants.

Jamey

On 12/14/11, Peter Hutterer peter.hutte...@who-t.net wrote:
 For future touch points, we need positionSprite to calculate the coordinates
 but we don't want to actually change the cursor position for non-emulating
 touches.

 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
  dix/getevents.c |7 +--
  1 files changed, 5 insertions(+), 2 deletions(-)

 diff --git a/dix/getevents.c b/dix/getevents.c
 index 57d8c17..3b7b626 100644
 --- a/dix/getevents.c
 +++ b/dix/getevents.c
 @@ -808,7 +808,7 @@ scale_from_screen(DeviceIntPtr dev, ValuatorMask *mask)
   * @param[out] screeny y coordinate in desktop coordinate system
   */
  static ScreenPtr
 -positionSprite(DeviceIntPtr dev, int mode, ValuatorMask *mask,
 +positionSprite(DeviceIntPtr dev, Bool update, int mode, ValuatorMask *mask,
 double *devx, double *devy,
 double *screenx, double *screeny)
  {
 @@ -839,6 +839,9 @@ positionSprite(DeviceIntPtr dev, int mode, ValuatorMask
 *mask,
  *devx = x;
  *devy = y;

 +if (!update)
 +return scr;
 +
  /* miPointerSetPosition takes care of crossing screens for us, as well
 as
   * clipping to the current screen. Coordinates returned are in desktop
   * coord system */
 @@ -1251,7 +1254,7 @@ fill_pointer_events(InternalEvent *events,
 DeviceIntPtr pDev, int type,
  if ((flags  POINTER_NORAW) == 0)
  set_raw_valuators(raw, mask, raw-valuators.data);

 -scr = positionSprite(pDev, (flags  POINTER_ABSOLUTE) ? Absolute :
 Relative,
 +scr = positionSprite(pDev, TRUE, (flags  POINTER_ABSOLUTE) ? Absolute
 : Relative,
   mask, devx, devy, screenx, screeny);

  /* screenx, screeny are in desktop coordinates,
 --
 1.7.7.1

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: moving rootless out of the server

[Jamey Sharp]

On 12/14/11, Antoine Martin anto...@nagafix.co.uk wrote:
 On 11/11/2011 08:59 AM, Jamey Sharp wrote:
 Or use a modern implementation like x11vnc that uses COMPOSITE so it
 doesn't need any server-side code at all. :-) That's the approach
 we're trying to adopt in this new project.
 This approach sounds very similar to the one used by Xpra [1]: FYI it
 registers as a compositing window manager to capture the windows' pixels
 which it then forwards to potentially remote clients.

You and I definitely have related interests, although for different
reasons. I'm mostly interested in deleting code, but you're actually
doing useful new things. :-)

 Have you considered supporting this over the network?

Sure, remote clients have to work in order to fully replace XQuartz,
and you might as well allow the server to be remote too. Might even
get better performance if the server is on the same machine as the
clients instead of the machine with the display hardware. Keithp's old
papers on LBX and X network performance are probably insightful here.

 Wouldn't this be very useful in the context of Wayland?

I hope so! Once this project is done, I hope people reuse the code to
replace XWin and XWayland, and also to support rootless nested X.

 Are you also going to forward the bell, custom cursors, etc?
 (those still fit within X11 whereas supporting things like dbus /
 notifications is much more tricky)

Cursors are fairly important. I kind of hope the X bell works
eventually. It's possible that neither will be done by this team, but
we'll see!

 I have added memory mapped pixel transfers to Xpra recently [2]
 (obviously, this only works when the client and server are on the same
 machine) which improved performance significantly, although there is
 still too much memory copying going on. I suspect your solution will
 target zero-copy where possible?

Someday I hope to use DRI to share buffer objects directly between the
dummy X server and the magic rootless client. That requires a
DRI-capable version of video-dummy that only allocates off-screen
buffers, which is a little odd. And I don't know what AppleDRI
supports, for the OS X implementation.

In any case, that's probably out of scope for the current project. I
may push the team to use MIT-SHM if that looks easy enough, and that's
probably it.

 In any case, I am interested in your code as it may provide a
 better (and probably faster too) solution to some of the problems Xpra
 has to deal with. Please keep me posted.

Will do!

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 00/25] Noise abatement and eyesore rehabilitation

[Jamey Sharp]

For the series:

Reviewed-by: Jamey Sharp ja...@minilop.net

Nice work. :-)

On Sun, Dec 11, 2011 at 10:16:13AM -0800, Alan Coopersmith wrote:
 Removes 422 lines of warning messages from the xserver build when
 using gcc 4.5.2 on Solaris 11 with the latest xorg-macros set of
 warning flags, and does not introduce any new warnings.
 
 There's still plenty more to fix after this batch of course.
 
 Alan Coopersmith (25):
   CheckForEmptyMask does not need to declare int n twice
   Fix deconstifying cast warning in xi2_get_type
   Use const cast in BitIsOn macro to avoid angering gcc
   verify_internal_event: preserve constness of data pointer
   constify strings in resource name registry
   xres.c: Preserve constness of string returned by LookupResourceName
   os/access.c: replace acmp  acopy macros with memcmp  memcpy calls
   Constify string for authorization protocol names
   Constify the reason string throughout the authorization check
 framework
   OsInit: store /dev/null in a const char *
   WriteToClient: preserve constness of buf while extracting length
 value
   LockServer: store path to LOCKDIR literal string in a const char *
   xdmcp.c: fix three small const warnings
   CompareISOLatin1Lowered: constify arguments
   x86emu: constify debug strings
   DoShowOptions: preserve constness of options list as we walk it
   Convert KdDoSwitchCmd to use asprintf instead of malloc/strcat/etc.
   KdParseFindNext: Constify delim argument
   XkbFindSrvLedInfo: remove extraneous name-clashing sli variable
   _XkbFilterDeviceBtn: move variable declarations to match usage scope
   Remove duplicate declaration of xf86ValidateModesFlags in xf86Modes.h
   Remove duplicate declarations of KdAdd*Driver in kdrive.h
   xf86Priv.h: Add some noreturn attributes suggested by gcc
   Add some printf format attributes suggested by gcc
   xf86 parser: convert Error to a varargs macro to clear gcc format
 warnings
 
  Xext/security.c   |5 +-
  Xext/xres.c   |4 +-
  dix/cursor.c  |2 +-
  dix/dispatch.c|5 +-
  dix/dixutils.c|4 +-
  dix/events.c  |2 +-
  dix/inpututils.c  |2 +-
  dix/registry.c|5 +-
  dix/resource.c|2 +-
  hw/kdrive/src/kdrive.c|   15 ++
  hw/kdrive/src/kdrive.h|8 +---
  hw/xfree86/common/xf86.h  |2 +-
  hw/xfree86/common/xf86Config.c|2 +-
  hw/xfree86/common/xf86Configure.c |5 +-
  hw/xfree86/common/xf86Priv.h  |6 +--
  hw/xfree86/common/xf86Xinput.h|2 +-
  hw/xfree86/modes/xf86Modes.h  |4 --
  hw/xfree86/parser/Configint.h |4 +-
  hw/xfree86/parser/DRI.c   |4 +-
  hw/xfree86/parser/Device.c|4 +-
  hw/xfree86/parser/Extensions.c|2 +-
  hw/xfree86/parser/Files.c |2 +-
  hw/xfree86/parser/Flags.c |4 +-
  hw/xfree86/parser/Input.c |4 +-
  hw/xfree86/parser/InputClass.c|4 +-
  hw/xfree86/parser/Layout.c|   28 ++--
  hw/xfree86/parser/Module.c|4 +-
  hw/xfree86/parser/Monitor.c   |   84 ++--
  hw/xfree86/parser/Pointer.c   |8 ++--
  hw/xfree86/parser/Screen.c|   36 
  hw/xfree86/parser/Vendor.c|6 +-
  hw/xfree86/parser/Video.c |6 +-
  hw/xfree86/parser/configProcs.h   |4 +-
  hw/xfree86/x86emu/debug.c |4 +-
  hw/xfree86/x86emu/fpu.c   |   16 
  hw/xfree86/x86emu/ops2.c  |4 +-
  hw/xfree86/x86emu/x86emu/debug.h  |4 +-
  include/dix.h |4 +-
  include/inputstr.h|2 +-
  include/os.h  |6 +-
  include/registry.h|2 +-
  include/resource.h|2 +-
  os/access.c   |   30 ++---
  os/auth.c |6 +-
  os/connection.c   |4 +-
  os/io.c   |2 +-
  os/mitauth.c  |2 +-
  os/osdep.h|2 +-
  os/osinit.c   |2 +-
  os/rpcauth.c  |2 +-
  os/utils.c|4 +-
  os/xdmauth.c  |4 +-
  os/xdmcp.c|6 +-
  render/filter.c   |3 +-
  xkb/xkbActions.c  |9 ++--
  xkb/xkbLEDs.c |6 +--
  56 files changed, 194 insertions(+), 211 deletions(-)
 
 -- 
 1.7.3.2
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org

Re: [PATCH] Limit the number of screens Xvfb will attempt to allocate memory for

[Jamey Sharp]

Sure, that makes sense.

Reviewed-by: Jamey Sharp ja...@minilop.net

On 11/23/11, Alan Coopersmith alan.coopersm...@oracle.com wrote:
 Commit f9e3a2955d2ca7 removing the MAXSCREEN limit left the screen
 number too unlimited, and allowed any positive int for a screen number:

 Xvfb :1 -screen 2147483647 1024x1024x8

 Fatal server error:
 Not enough memory for screen 2147483647

 Found by Parfait 0.3.7:
 Error: Integer overflow (CWE 190)
Integer parameter of memory allocation function realloc() may overflow
 due to multiplication with constant value 1112
 at line 293 of hw/vfb/InitOutput.c in function 'ddxProcessArgument'.

 Since the X11 connection setup only has a CARD8 for number of SCREENS,
 limit to 255 screens, which is also low enough to avoid overflow on the
 sizeof(*vfbScreens) * (screenNum + 1) calculation for realloc.

 Signed-off-by: Alan Coopersmith alan.coopersm...@oracle.com
 ---
  hw/vfb/InitOutput.c |4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)

 diff --git a/hw/vfb/InitOutput.c b/hw/vfb/InitOutput.c
 index 3e5d051..9a9905d 100644
 --- a/hw/vfb/InitOutput.c
 +++ b/hw/vfb/InitOutput.c
 @@ -280,7 +280,9 @@ ddxProcessArgument(int argc, char *argv[], int i)
   int screenNum;
   CHECK_FOR_REQUIRED_ARGUMENTS(2);
   screenNum = atoi(argv[i+1]);
 - if (screenNum  0)
 + /* The protocol only has a CARD8 for number of screens in the
 +connection setup block, so don't allow more than that. */
 + if ((screenNum  0) || (screenNum = 255))
   {
   ErrorF(Invalid screen number %d\n, screenNum);
   UseMsg();
 --
 1.7.3.2

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 0/3] dix/window.c cleanups and death to server regen

[Jamey Sharp]

I'm tired of dealing with server regen. Here's a patch to kill it while
still meeting the requirements in the X protocol specification; and two
straightforward cleanup patches that I think are worth applying even if
the reset-rework patch is NAKed.

The following changes since commit f0d50cc6651dce3a8a3cd3fb84210aa92b139763:

  Fix vesa's VBE PanelID interpretation (2011-11-06 16:41:44 -0800)

are available in the git repository at:
  git://anongit.freedesktop.org/~jamey/xserver no-regen

Jamey Sharp (3):
  InitRootWindow: Use ChangeWindowAttributes instead of duplicating it.
  Factor out duplicated WindowOptPtr initialization.
  Reset only what the protocol requires, instead of full regeneration.

 dix/atom.c   |   30 
 dix/devices.c|   22 +++
 dix/globals.c|1 -
 dix/main.c   |  372 +++---
 dix/property.c   |   21 +++
 dix/resource.c   |3 +-
 dix/window.c |  125 +++--
 include/dix.h|7 +
 include/misc.h   |2 +-
 include/property.h   |2 +
 include/propertyst.h |1 +
 include/window.h |3 +
 12 files changed, 339 insertions(+), 250 deletions(-)
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 1/3] InitRootWindow: Use ChangeWindowAttributes instead of duplicating it.

[Jamey Sharp]

The protocol requires ChangeWindowAttributes to be able to set the root
window's background and cursor to their defaults. So let's just use that
when InitRootWindow needs to set up the defaults in the first place.

This guarantees that using ChangeWindowAttributes to reset the root
window really does restore the startup state, which wasn't quite true
before:

- If both party_like_its_1989 and bgNoneRoot were set, then the root
  background was initially the root weave, but ChangeWindowAttributes
  would reset it to background None.

- InitRootWindow checked whether the screen supported background None,
  while ChangeWindowAttributes unconditionally trusted bgNoneRoot.

Signed-off-by: Jamey Sharp ja...@minilop.net
---
 dix/window.c |   44 +---
 1 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/dix/window.c b/dix/window.c
index 1953f02..44bfa18 100644
--- a/dix/window.c
+++ b/dix/window.c
@@ -551,38 +551,13 @@ void
 InitRootWindow(WindowPtr pWin)
 {
 ScreenPtr pScreen = pWin-drawable.pScreen;
-int backFlag = CWBorderPixel | CWCursor | CWBackingStore;
+int mask = CWBackPixmap | CWBackingStore | CWCursor;
+XID attrs[] = { None, defaultBackingStore, None };
 
 if (!(*pScreen-CreateWindow)(pWin))
return; /* XXX */
 (*pScreen-PositionWindow)(pWin, 0, 0);
-
-pWin-cursorIsNone = FALSE;
-pWin-optional-cursor = rootCursor;
-rootCursor-refcnt++;
-
-
-if (party_like_its_1989) {
-MakeRootTile(pWin);
-backFlag |= CWBackPixmap;
-} else if (pScreen-canDoBGNoneRoot  bgNoneRoot) {
-pWin-backgroundState = XaceBackgroundNoneState(pWin);
-pWin-background.pixel = pScreen-whitePixel;
-backFlag |= CWBackPixmap;
-} else {
-pWin-backgroundState = BackgroundPixel;
-   if (whiteRoot)
-pWin-background.pixel = pScreen-whitePixel;
-else
-pWin-background.pixel = pScreen-blackPixel;
-backFlag |= CWBackPixel;
-} 
-
-pWin-backingStore = defaultBackingStore;
-pWin-forcedBS = (defaultBackingStore != NotUseful);
-/* We SHOULD check for an error value here XXX */
-(*pScreen-ChangeWindowAttributes)(pWin, backFlag);
-
+ChangeWindowAttributes(pWin, mask, attrs, serverClient);
 MapWindow(pWin, serverClient);
 }
 
@@ -1062,14 +1037,13 @@ SetRootWindowBackground(WindowPtr pWin, ScreenPtr 
pScreen, Mask *index2)
 {
 /* following the protocol: Changing the background of a root window to
  * None or ParentRelative restores the default background pixmap */
-if (bgNoneRoot) {
+if (party_like_its_1989)
+   MakeRootTile(pWin);
+else if (pScreen-canDoBGNoneRoot  bgNoneRoot) {
pWin-backgroundState = XaceBackgroundNoneState(pWin);
pWin-background.pixel = pScreen-whitePixel;
-}
-else if (party_like_its_1989)
-   MakeRootTile(pWin);
-else {
-pWin-backgroundState = BackgroundPixel;
+} else {
+   pWin-backgroundState = BackgroundPixel;
if (whiteRoot)
pWin-background.pixel = pScreen-whitePixel;
else
@@ -1278,7 +1252,7 @@ ChangeWindowAttributes(WindowPtr pWin, Mask vmask, XID 
*vlist, ClientPtr client)
goto PatchUp;
}
pWin-backingStore = val;
-   pWin-forcedBS = FALSE;
+   pWin-forcedBS = (client == serverClient  pWin-backingStore != 
NotUseful);
break;
  case CWBackingPlanes:
if (pWin-optional || ((CARD32)*pVlist != (CARD32)~0L)) {
-- 
1.7.5.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 3/3] Reset only what the protocol requires, instead of full regeneration.

[Jamey Sharp]

It's difficult to get regeneration correct in all the drivers,
extensions, and everywhere else it's currently required--and there's no
significant advantage to doing full regeneration. So instead, make DIX
reset exactly the things required by the protocol specification, and set
serverGeneration to a constant 1.

Eliminating all the ugly code that's conditional on serverGeneration is
left as a later exercise. This patch should simply improve reliability
when the server resets.

Signed-off-by: Jamey Sharp ja...@minilop.net
---
This patch is generated with `diff -w`, so the patch's indentation is
all wrong in places. I believe the real changes are easier to review
this way, because I had to re-indent most of dix/main.c. If you want to
review the version with correct indentation, please review it by cloning
my public xserver repo or by browsing it in cgit:

http://cgit.freedesktop.org/~jamey/xserver/commit/?h=no-regenid=8fd86ed43310863d7c3321c007d9cf6419b8c120

 dix/atom.c   |   30 +
 dix/devices.c|   22 
 dix/globals.c|1 -
 dix/main.c   |   88 +-
 dix/property.c   |   21 
 dix/resource.c   |3 +-
 dix/window.c |   13 +--
 include/dix.h|7 
 include/misc.h   |2 +-
 include/property.h   |2 +
 include/propertyst.h |1 +
 include/window.h |3 ++
 12 files changed, 156 insertions(+), 37 deletions(-)

diff --git a/dix/atom.c b/dix/atom.c
index 88b40db..5eb1d55 100644
--- a/dix/atom.c
+++ b/dix/atom.c
@@ -68,6 +68,7 @@ typedef struct _Node {
 } NodeRec, *NodePtr;
 
 static Atom lastAtom = None;
+static Atom lastServerAtom = None;
 static NodePtr atomRoot = NULL;
 static unsigned long tableLength;
 static NodePtr *nodeTable;
@@ -201,6 +202,7 @@ FreeAllAtoms(void)
 free(nodeTable);
 nodeTable = NULL;
 lastAtom = None;
+lastServerAtom = None;
 }
 
 void
@@ -216,3 +218,31 @@ InitAtoms(void)
 if (lastAtom != XA_LAST_PREDEFINED)
AtomError();
 }
+
+void
+SaveServerAtoms(void)
+{
+lastServerAtom = lastAtom;
+}
+
+static void
+ResetAtom(NodePtr *patom)
+{
+if (!*patom)
+   return;
+if ((*patom)-a  lastServerAtom)
+{
+   FreeAtom(*patom);
+   *patom = NULL;
+   return;
+}
+ResetAtom((*patom)-left);
+ResetAtom((*patom)-right);
+}
+
+void
+ResetAtoms(void)
+{
+lastAtom = lastServerAtom;
+ResetAtom(atomRoot);
+}
diff --git a/dix/devices.c b/dix/devices.c
index da817a8..7d91416 100644
--- a/dix/devices.c
+++ b/dix/devices.c
@@ -2645,3 +2645,25 @@ void valuator_set_mode(DeviceIntPtr dev, int axis, int 
mode)
 dev-valuator-axes[i].mode = mode;
 }
 }
+
+void ResetInput(void)
+{
+BITS32 mask = KBKeyClickPercent | KBBellPercent | KBBellPitch | 
KBBellDuration | KBAutoRepeatMode;
+XID attrs[] = { -1, -1, -1, -1, AutoRepeatModeDefault };
+DeviceIntPtr dev;
+for (dev = inputInfo.devices; dev; dev = dev-next)
+{
+   /* TODO: reset keyboard and pointer mappings */
+   if (dev-focus)
+   {
+   dev-focus-win = PointerRootWin;
+   dev-focus-revert = None;
+   dev-focus-time = currentTime;
+   dev-focus-traceGood = 0;
+   }
+   if (dev-kbdfeed)
+   DoChangeKeyboardControl(serverClient, dev, attrs, mask);
+   if (dev-ptrfeed)
+   dev-ptrfeed-ctrl = defaultPointerControl;
+}
+}
diff --git a/dix/globals.c b/dix/globals.c
index 0a6b170..28f9dbd 100644
--- a/dix/globals.c
+++ b/dix/globals.c
@@ -84,7 +84,6 @@ int  currentMaxClients;   /* current size of clients array */
 long maxBigRequestSize = MAX_BIG_REQUEST_SIZE;
 
 unsigned long globalSerialNumber = 0;
-unsigned long serverGeneration = 0;
 
 /* these next four are initialized in main.c */
 CARD32 ScreenSaverTime;
diff --git a/dix/main.c b/dix/main.c
index 16575ce..745fe05 100644
--- a/dix/main.c
+++ b/dix/main.c
@@ -149,9 +149,7 @@ int main(int argc, char *argv[], char *envp[])
 
 alwaysCheckForInput[0] = 0;
 alwaysCheckForInput[1] = 1;
-while(1)
-{
-   serverGeneration++;
+
ScreenSaverTime = defaultScreenSaverTime;
ScreenSaverInterval = defaultScreenSaverInterval;
ScreenSaverBlanking = defaultScreenSaverBlanking;
@@ -164,18 +162,14 @@ int main(int argc, char *argv[], char *envp[])
InitBlockAndWakeupHandlers();
/* Perform any operating system dependent initializations you'd like */
OsInit();
-   if(serverGeneration == 1)
-   {
CreateWellKnownSockets();
+
for (i=1; iMAXCLIENTS; i++)
clients[i] = NullClient;
serverClient = calloc(sizeof(ClientRec), 1);
if (!serverClient)
FatalError(couldn't create server client);
InitClient(serverClient, 0, (pointer)NULL);
-   }
-   else
-   ResetWellKnownSockets ();
clients[0] = serverClient

[PATCH 2/3] Factor out duplicated WindowOptPtr initialization.

[Jamey Sharp]

By using the same code for both MakeWindowOptional and CreateRootWindow,
we ensure that new WindowOptPtrs are always fully initialized.

Previously, CreateRootWindow did not initialize optional-cursor. It
relied on InitRootWindow to overwrite it later, which seems like a wild
pointer dereference just waiting to happen.

Signed-off-by: Jamey Sharp ja...@minilop.net
---
 dix/window.c |   74 +++--
 1 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/dix/window.c b/dix/window.c
index 44bfa18..c87020d 100644
--- a/dix/window.c
+++ b/dix/window.c
@@ -152,6 +152,12 @@ static unsigned char _back_msb[4] = {0x11, 0x44, 0x22, 
0x88};
 static Bool WindowParentHasDeviceCursor(WindowPtr pWin, 
 DeviceIntPtr pDev, 
 CursorPtr pCurs);
+static WindowOptPtr
+AllocateWindowOptional(CursorPtr cursor,
+   VisualID visual,
+   Colormap colormap,
+   Mask dontPropagateMask);
+
 static Bool 
 WindowSeekDeviceCursor(WindowPtr pWin, 
DeviceIntPtr pDev, 
@@ -481,25 +487,11 @@ CreateRootWindow(ScreenPtr pScreen)
 pWin-parent = NullWindow;
 SetWindowToDefaults(pWin);
 
-pWin-optional = malloc(sizeof (WindowOptRec));
+pWin-optional = AllocateWindowOptional(None,
+pScreen-rootVisual, pScreen-defColormap, 0);
 if (!pWin-optional)
 return FALSE;
 
-pWin-optional-dontPropagateMask = 0;
-pWin-optional-otherEventMasks = 0;
-pWin-optional-otherClients = NULL;
-pWin-optional-passiveGrabs = NULL;
-pWin-optional-userProps = NULL;
-pWin-optional-backingBitPlanes = ~0L;
-pWin-optional-backingPixel = 0;
-pWin-optional-boundingShape = NULL;
-pWin-optional-clipShape = NULL;
-pWin-optional-inputShape = NULL;
-pWin-optional-inputMasks = NULL;
-pWin-optional-deviceCursors = NULL;
-pWin-optional-colormap = pScreen-defColormap;
-pWin-optional-visual = pScreen-rootVisual;
-
 pWin-nextSib = NullWindow;
 
 pWin-drawable.id = FakeClientID(0);
@@ -3500,18 +3492,21 @@ CheckWindowOptionalNeed (WindowPtr w)
  * values.
  */
 
-Bool
-MakeWindowOptional (WindowPtr pWin)
+static WindowOptPtr
+AllocateWindowOptional (CursorPtr cursor,
+VisualID visual,
+Colormap colormap,
+Mask dontPropagateMask)
 {
-WindowOptPtr optional;
-WindowOptPtr parentOptional;
-
-if (pWin-optional)
-   return TRUE;
-optional = malloc(sizeof (WindowOptRec));
+WindowOptPtr optional = malloc(sizeof (WindowOptRec));
 if (!optional)
-   return FALSE;
-optional-dontPropagateMask = DontPropagateMasks[pWin-dontPropagate];
+   return NULL;
+optional-cursor = cursor;
+if (cursor)
+   cursor-refcnt++;
+optional-visual = visual;
+optional-colormap = colormap;
+optional-dontPropagateMask = dontPropagateMask;
 optional-otherEventMasks = 0;
 optional-otherClients = NULL;
 optional-passiveGrabs = NULL;
@@ -3523,21 +3518,22 @@ MakeWindowOptional (WindowPtr pWin)
 optional-inputShape = NULL;
 optional-inputMasks = NULL;
 optional-deviceCursors = NULL;
+return optional;
+}
 
+Bool
+MakeWindowOptional (WindowPtr pWin)
+{
+WindowOptPtr parentOptional;
+
+if (pWin-optional)
+   return TRUE;
 parentOptional = FindWindowWithOptional(pWin)-optional;
-optional-visual = parentOptional-visual;
-if (!pWin-cursorIsNone)
-{
-   optional-cursor = parentOptional-cursor;
-   optional-cursor-refcnt++;
-}
-else
-{
-   optional-cursor = None;
-}
-optional-colormap = parentOptional-colormap;
-pWin-optional = optional;
-return TRUE;
+pWin-optional = AllocateWindowOptional(
+   pWin-cursorIsNone ? None : parentOptional-cursor,
+   parentOptional-visual, parentOptional-colormap,
+   DontPropagateMasks[pWin-dontPropagate]);
+return pWin-optional != NULL;
 }
 
 /*
-- 
1.7.5.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libdri2] dri2video support

[Jamey Sharp]

On Wed, Nov 16, 2011 at 02:36:45PM +0100, walter harms wrote:
 Am 15.11.2011 23:49, schrieb Rob Clark:
  +nformats = (rep.length / sizeof(*formats)) * 4;
  +formats = malloc(nformats * sizeof(*formats));
 
 i do not understand the 4 here. do you mean:
   (rep.length / sizeof(*formats))*sizeof(*formats)

In the X protocol, the length field is always measured in 4-byte units.

I'm guessing instead that you meant:
nformats = rep.length * 4 / sizeof(*formats);

I think doing the division first will get the wrong answer any time the
number of formats is not divisible by 4, right?

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Save major/minor opcodes in ClientRec for RecordAReply

[Jamey Sharp]

Reviewed-by: Jamey Sharp ja...@minilop.net

On Mon, Nov 14, 2011 at 09:04:18AM -0800, Keith Packard wrote:
 On Tue, 8 Nov 2011 10:47:44 -0800, Jamey Sharp ja...@minilop.net wrote:
 
  And can I persuade you to just inline this directly into its only call
  site, below, in Dispatch? I don't think factoring it out enhances
  clarity in this case, especially since this way you need to look up the
  major opcode in both places.
 
 Yeah, that's looks a lot simpler overall. Here's an updated version
 
 From 9b54a88f8e17a2ab720bb1aa3eb2640b201be4d8 Mon Sep 17 00:00:00 2001
 From: Keith Packard kei...@keithp.com
 Date: Tue, 8 Nov 2011 10:13:15 -0800
 Subject: [PATCH] Save major/minor opcodes in ClientRec for RecordAReply
 
 The record extension needs the major and minor opcodes in the reply
 hook, but the request buffer may have been freed by the time the hook
 is invoked. Saving the request major and minor codes as the request is
 executed avoids fetching from the defunct request buffer.
 
 This patch also eliminates the public MinorOpcodeOfRequest function,
 inlining it into Dispatch. Usages of that function have been replaced
 with direct access to the new ClientRec field.
 
 Signed-off-by: Keith Packard kei...@keithp.com
 ---
  Xext/security.c   |4 +---
  Xext/xselinux_hooks.c |4 ++--
  dix/dispatch.c|   23 +--
  dix/extension.c   |   14 --
  include/dixstruct.h   |1 +
  include/extension.h   |2 --
  record/record.c   |8 +++-
  7 files changed, 20 insertions(+), 36 deletions(-)
 
 diff --git a/Xext/security.c b/Xext/security.c
 index 08d8158..b0d82ab 100644
 --- a/Xext/security.c
 +++ b/Xext/security.c
 @@ -148,9 +148,7 @@ SecurityLabelInitial(void)
  static _X_INLINE const char *
  SecurityLookupRequestName(ClientPtr client)
  {
 -int major = ((xReq *)client-requestBuffer)-reqType;
 -int minor = MinorOpcodeOfRequest(client);
 -return LookupRequestName(major, minor);
 +return LookupRequestName(client-majorOp, client-minorOp);
  }
  
  
 diff --git a/Xext/xselinux_hooks.c b/Xext/xselinux_hooks.c
 index f1d8e5d..0d4c9ab 100644
 --- a/Xext/xselinux_hooks.c
 +++ b/Xext/xselinux_hooks.c
 @@ -263,8 +263,8 @@ SELinuxAudit(void *auditdata,
  if (client) {
   REQUEST(xReq);
   if (stuff) {
 - major = stuff-reqType;
 - minor = MinorOpcodeOfRequest(client);
 + major = client-majorOp;
 + minor = client-minorOp;
   }
  }
  if (audit-id)
 diff --git a/dix/dispatch.c b/dix/dispatch.c
 index 6e33615..b39271f 100644
 --- a/dix/dispatch.c
 +++ b/dix/dispatch.c
 @@ -337,8 +337,6 @@ DisableLimitedSchedulingLatency(void)
   SmartScheduleLatencyLimited = 0;
  }
  
 -#define MAJOROP ((xReq *)client-requestBuffer)-reqType
 -
  void
  Dispatch(void)
  {
 @@ -419,21 +417,28 @@ Dispatch(void)
   }
  
   client-sequence++;
 + client-majorOp = ((xReq *)client-requestBuffer)-reqType;
 + client-minorOp = 0;
 + if (client-majorOp = EXTENSION_BASE) {
 + ExtensionEntry *ext = GetExtensionEntry(client-majorOp);
 + if (ext)
 + client-minorOp = ext-MinorOpcode(client);
 + }
  #ifdef XSERVER_DTRACE
 - XSERVER_REQUEST_START(LookupMajorName(MAJOROP), MAJOROP,
 + XSERVER_REQUEST_START(LookupMajorName(client-majorOp), 
 client-majorOp,
 ((xReq *)client-requestBuffer)-length,
 client-index, client-requestBuffer);
  #endif
   if (result  (maxBigRequestSize  2))
   result = BadLength;
   else {
 - result = XaceHookDispatch(client, MAJOROP);
 + result = XaceHookDispatch(client, client-majorOp);
   if (result == Success)
 - result = (* client-requestVector[MAJOROP])(client);
 + result = (* 
 client-requestVector[client-majorOp])(client);
   XaceHookAuditEnd(client, result);
   }
  #ifdef XSERVER_DTRACE
 - XSERVER_REQUEST_DONE(LookupMajorName(MAJOROP), MAJOROP,
 + XSERVER_REQUEST_DONE(LookupMajorName(client-majorOp), 
 client-majorOp,
 client-sequence, client-index, result);
  #endif
  
 @@ -444,8 +449,8 @@ Dispatch(void)
   }
   else if (result != Success)
   {
 - SendErrorToClient(client, MAJOROP,
 -   MinorOpcodeOfRequest(client),
 + SendErrorToClient(client, client-majorOp,
 +   client-minorOp,
 client-errorValue, result);
   break;
   }
 @@ -466,8 +471,6 @@ Dispatch(void)
  SmartScheduleLatencyLimited = 0;
  }
  
 -#undef MAJOROP
 -
  static int  VendorRelease = VENDOR_RELEASE;
  static char

Re: Changes causes application

[Jamey Sharp]

On 11/11/11, Ratin rat...@gmail.com wrote:
 On Thu, Nov 10, 2011 at 12:57 PM, Jamey Sharp ja...@minilop.net wrote:
 What application is it? Does it make X requests from more than one
 thread?

 Hi Jamey, Thanks for the insightful question. Its a bit complex video
 wall application ... [interesting details snipped]

 As far as multiple threads operating on one window - I would say yes,
 since the window content gets updated by the hardware decoder API
 (VDPAU presentation queue - I would consider this to be from a thread)
 and we have another thread waiting for some x input events for on that
 window and operating on the same window if such input occurs (such as
 mouse click). But this   input event handler is not absolutely
 necessary.  Do you suggest taking this second thread out of the
 picture and see if the problem persists?

I'm still not entirely clear whether the app is calling Xlib from more
than one thread, but it sounds similar enough to a known bug that I
would indeed suggest that experiment, of removing the event handling
thread. (I can't look up the relevant bug report just now, sorry.)

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

moving rootless out of the server

[Jamey Sharp]

Just to let you all know: Getting students to build xf86-video-nested
worked out well, so we're starting another X-related PSU Capstone
project. This time Jeremy is joining Josh and me as project sponsors.

The goal this time around is to replace the XQuartz DDX. We'll run the
stock xfree86 DDX (though Jeremy's patches to make it run in OS X aren't
all merged yet, I think?) with a video-dummy driver.

The students' task is to replace the functionality currently provided by
the server's generic rootless layer and the input and rendering support
provided by XQuartz, which we're asking them to do by writing a
compositing window manager that sucks window contents out of the server
and blits them into native windows. Similarly, for the initial
implementation they'll use XTest to inject input from native sources
into the server.

We hope that the resulting client code can be shared with other
platforms, so future work may include replacing XWin with a Windows
version of the same client. It may also be useful to build versions on
Weyland or another X server. The latter would be different from Xnest
because it's rootless, which we don't currently have for X nested in X.

The students are just starting to gather requirements this week. If all
goes well, their project should complete in late March.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: moving rootless out of the server

[Jamey Sharp]

On 11/10/11, Alan Hourihane al...@fairlite.co.uk wrote:
 On 11/10/11 23:40, Peter Hutterer wrote:
 I wonder how much effort it would be to write a xf86-video-vnc that runs
 on xfree86 ddx.

That would be interesting to try, and there's some library out there
for quickly building a VNC server into an arbitrary app. Or Alan's
suggestion;

 xf4vnc already does. Just use the xf86-video-dummy driver and you have a
 standalone setup.

Or use a modern implementation like x11vnc that uses COMPOSITE so it
doesn't need any server-side code at all. :-) That's the approach
we're trying to adopt in this new project.

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Save major/minor opcodes in ClientRec for RecordAReply

[Jamey Sharp]

You're welcome to throw my reviewed-by on this patch, with the caveat
that not only have I not tested it, but I haven't investigated the issue
it's trying to address, either. But it seems like an improvement to the
code regardless of any bugs it might fix.

I'd suggest a couple minor edits, though:

On Tue, Nov 08, 2011 at 10:22:13AM -0800, Keith Packard wrote:
 diff --git a/Xext/xselinux_hooks.c b/Xext/xselinux_hooks.c
 index f1d8e5d..0d4c9ab 100644
 --- a/Xext/xselinux_hooks.c
 +++ b/Xext/xselinux_hooks.c
 @@ -263,8 +263,8 @@ SELinuxAudit(void *auditdata,
  if (client) {
   REQUEST(xReq);
   if (stuff) {

I feel like this should turn into if (client  client-requestBuffer)
rather than hiding the real condition inside the REQUEST macro.

 - major = stuff-reqType;
 - minor = MinorOpcodeOfRequest(client);
 + major = client-majorOp;
 + minor = client-minorOp;
   }
  }
  if (audit-id)
 diff --git a/dix/dispatch.c b/dix/dispatch.c
 index 6e33615..3600acd 100644
 --- a/dix/dispatch.c
 +++ b/dix/dispatch.c
 @@ -337,7 +337,20 @@ DisableLimitedSchedulingLatency(void)
   SmartScheduleLatencyLimited = 0;
  }
  
 -#define MAJOROP ((xReq *)client-requestBuffer)-reqType
 +static inline unsigned short
 +MinorOpcodeOfRequest(ClientPtr client)
 +{
 +unsigned char major;
 +ExtensionEntry *ext;
 +
 +major = ((xReq *)client-requestBuffer)-reqType;
 +if (major  EXTENSION_BASE)
 + return 0;

GetExtensionEntry already checks that major is at least EXTENSION_BASE,
so this test is redundant.

 +ext = GetExtensionEntry(major);
 +if (!ext)
 + return 0;
 +return ext-MinorOpcode (client);
 +}

And can I persuade you to just inline this directly into its only call
site, below, in Dispatch? I don't think factoring it out enhances
clarity in this case, especially since this way you need to look up the
major opcode in both places.

  void
  Dispatch(void)
 @@ -419,21 +432,23 @@ Dispatch(void)
   }
  
   client-sequence++;
 + client-majorOp = ((xReq *)client-requestBuffer)-reqType;
 + client-minorOp = MinorOpcodeOfRequest(client);

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11 v2 1/4] Add _XGetRequest as substitute for GetReq/GetReqExtra

[Jamey Sharp]

On Thu, Nov 03, 2011 at 07:30:52AM +1000, Peter Hutterer wrote:
 On Wed, Nov 02, 2011 at 11:23:55AM -0700, Jamey Sharp wrote:
  The best option, of course, is to ensure that no bug can occur. Peter,
  what about making the length argument take 4-byte units, dividing in the
  callers, and multiplying inside _XGetRequest? 
 
 I had that in the first version but we cannot change GetReqExtra and that
 takes bytes, not 4-byte units. And having two ways to get a request with a
 specific size that take different units seemed more error-prone.
 SIZEOF() returns bytes as well, so imo switching to a different unit to help
 those potential future developers that don't read the documentation for the
 macros isn't overly useful. It's not like the 4-byte alignment requirement
 of the protocol is a secred either :)

Well, I'm convinced. As far as I'm concerned, push when you're ready. I
believe I already provided r-b tags for the series?

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11 v2 1/4] Add _XGetRequest as substitute for GetReq/GetReqExtra

[Jamey Sharp]

On Wed, Nov 02, 2011 at 05:32:24AM -0700, Dan Nicholson wrote:
 On Oct 27, 2011 4:26 AM, Peter Hutterer peter.hutte...@who-t.net wrote:
  On Thu, Oct 27, 2011 at 10:41:06AM +0200, walter harms wrote:
   Am 27.10.2011 09:55, schrieb Jamey Sharp:
On Thu, Oct 27, 2011 at 09:15:54AM +0200, walter harms wrote:
Does it make sense to continue here ?
perhaps you want a add a return NULL ?
   
It doesn't make sense to continue, but there's no way to report the
error that any caller can handle. If you return NULL here, the caller is
guaranteed to segfault.
  
   what is bad with segfault ? the macro version would more or less make
   sure that len % 4 is 0 by using sizeof. Everybody else has a serious 
   problem.
   Better you stop here (by returning NULL) than at a random place.
 
  all the existing code still uses the macros so the same assumptions are
  true. Only new code could use _XGetRequest directly and you'd hope that
  whoever writes that code runs it at least once to see the error message.
 
 Drive by reviewing ...
 What about an assert? Than the user's incorrect program will at least
 stop with a usefulish message. Seems better then a segfault.

That's a quite sensible-sounding answer, which I've tried in libX11 in
the past. You or Walter are welcome to commit a follow-on patch that
asserts or returns NULL, whichever you prefer, as long as you take
responsibility for any bug reports that follow. :-) Keep in mind that
distros on the whole are amazingly bad about forwarding bug reports
upstream, so you'll need to monitor multiple distros' bug trackers
yourself. Also be prepared for users you've never met cursing your name.
Bleh. :-/

I really do agree that it's better to catch this sort of bug.
Unfortunately, asserts and segfaults mean that the people who are most
likely to encounter bugs are the least capable of dealing with them.

The best option, of course, is to ensure that no bug can occur. Peter,
what about making the length argument take 4-byte units, dividing in the
callers, and multiplying inside _XGetRequest? The division will be
constant-folded away, so there's no code-size cost in the callers. We
could even round up to the nearest four-byte boundary for free, although
that means that buggy extensions magically work when built with new
headers but fail when built with old headers. Or throw something like
the Linux kernel BUILD_BUG_ON macro into the GetReq macros...

Of course there's the further problem that _XGetRequest doesn't work for
requests bigger than either the core maximum request size or the Xlib
output queue size, whichever is smaller. Which raises the same questions
of printf or assert or return NULL all over again. But I don't think
this bike-shedding should block merging the patches.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11 v2 1/4] Add _XGetRequest as substitute for GetReq/GetReqExtra

[Jamey Sharp]

For the series:

Reviewed-by: Jamey Sharp ja...@minilop.net

Thanks!

On Thu, Oct 27, 2011 at 02:21:25PM +1000, Peter Hutterer wrote:
 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
 Changes to v1:
 - a few comment fixes, whitespace fixes to better align the code with the
   rest of the file
 - Warning added to _XGetRequest
 - typecast added to _XGetRequest usage from macros
 
  include/X11/Xlibint.h |   49 
 -
  src/XlibInt.c |   31 +++
  2 files changed, 47 insertions(+), 33 deletions(-)
 
 diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h
 index 2ce356d..43d1f2a 100644
 --- a/include/X11/Xlibint.h
 +++ b/include/X11/Xlibint.h
 @@ -420,6 +420,18 @@ extern LockInfoPtr _Xglobal_lock;
  #define WORD64ALIGN
  #endif /* WORD64 */
  
 +/**
 + * Return a len-sized request buffer for the request type. This function may
 + * flush the output queue.
 + *
 + * @param dpy The display connection
 + * @param type The request type
 + * @param len Length of the request in bytes
 + *
 + * @returns A pointer to the request buffer with a few default values
 + * initialized.
 + */
 +extern void *_XGetRequest(Display *dpy, CARD8 type, size_t len);
  
  /*
   * GetReq - Get the next available X request packet in the buffer and
 @@ -432,25 +444,10 @@ extern LockInfoPtr _Xglobal_lock;
  
  #if !defined(UNIXCPP) || defined(ANSICPP)
  #define GetReq(name, req) \
 -WORD64ALIGN\
 - if ((dpy-bufptr + SIZEOF(x##name##Req))  dpy-bufmax)\
 - _XFlush(dpy);\
 - req = (x##name##Req *)(dpy-last_req = dpy-bufptr);\
 - req-reqType = X_##name;\
 - req-length = (SIZEOF(x##name##Req))2;\
 - dpy-bufptr += SIZEOF(x##name##Req);\
 - dpy-request++
 -
 + req = (x##name##Req *) _XGetRequest(dpy, X_##name, SIZEOF(x##name##Req))
  #else  /* non-ANSI C uses empty comment instead of ## for token 
 concatenation */
  #define GetReq(name, req) \
 -WORD64ALIGN\
 - if ((dpy-bufptr + SIZEOF(x/**/name/**/Req))  dpy-bufmax)\
 - _XFlush(dpy);\
 - req = (x/**/name/**/Req *)(dpy-last_req = dpy-bufptr);\
 - req-reqType = X_/**/name;\
 - req-length = (SIZEOF(x/**/name/**/Req))2;\
 - dpy-bufptr += SIZEOF(x/**/name/**/Req);\
 - dpy-request++
 + req = (x/**/name/**/Req *) _XGetRequest(dpy, X_/**/name, 
 SIZEOF(x/**/name/**/Req))
  #endif
  
  /* GetReqExtra is the same as GetReq, but allocates n additional
 @@ -458,24 +455,10 @@ extern LockInfoPtr _Xglobal_lock;
  
  #if !defined(UNIXCPP) || defined(ANSICPP)
  #define GetReqExtra(name, n, req) \
 -WORD64ALIGN\
 - if ((dpy-bufptr + SIZEOF(x##name##Req) + n)  dpy-bufmax)\
 - _XFlush(dpy);\
 - req = (x##name##Req *)(dpy-last_req = dpy-bufptr);\
 - req-reqType = X_##name;\
 - req-length = (SIZEOF(x##name##Req) + n)2;\
 - dpy-bufptr += SIZEOF(x##name##Req) + n;\
 - dpy-request++
 + req = (x##name##Req *) _XGetRequest(dpy, X_##name, SIZEOF(x##name##Req) 
 + n)
  #else
  #define GetReqExtra(name, n, req) \
 -WORD64ALIGN\
 - if ((dpy-bufptr + SIZEOF(x/**/name/**/Req) + n)  dpy-bufmax)\
 - _XFlush(dpy);\
 - req = (x/**/name/**/Req *)(dpy-last_req = dpy-bufptr);\
 - req-reqType = X_/**/name;\
 - req-length = (SIZEOF(x/**/name/**/Req) + n)2;\
 - dpy-bufptr += SIZEOF(x/**/name/**/Req) + n;\
 - dpy-request++
 + req = (x/**/name/**/Req *) _XGetRequest(dpy, X_/**/name, 
 SIZEOF(x/**/name/**/Req) + n)
  #endif
  
  
 diff --git a/src/XlibInt.c b/src/XlibInt.c
 index 3db151e..a8f5d08 100644
 --- a/src/XlibInt.c
 +++ b/src/XlibInt.c
 @@ -1956,6 +1956,37 @@ Screen *_XScreenOfWindow(Display *dpy, Window w)
  }
  
  
 +/*
 + * WARNING: This implementation's pre-conditions and post-conditions
 + * must remain compatible with the old macro-based implementations of
 + * GetReq, GetReqExtra, GetResReq, and GetEmptyReq. The portions of the
 + * Display structure affected by those macros are part of libX11's
 + * ABI.
 + */
 +void *_XGetRequest(Display *dpy, CARD8 type, size_t len)
 +{
 +xReq *req;
 +
 +WORD64ALIGN
 +
 +if (dpy-bufptr + len  dpy-bufmax)
 + _XFlush(dpy);
 +
 +if (len % 4)
 + fprintf(stderr,
 + Xlib: request %d length %zd not a multiple of 4.\n,
 + type, len);
 +
 +dpy-last_req = dpy-bufptr;
 +
 +req = (xReq*)dpy-bufptr;
 +req-reqType = type;
 +req-length = len / 4;
 +dpy-bufptr += len;
 +dpy-request++;
 +return req;
 +}
 +
  #if defined(WIN32)
  
  /*
 -- 
 1.7.7
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http

Re: [PATCH libX11 v2 1/4] Add _XGetRequest as substitute for GetReq/GetReqExtra

[Jamey Sharp]

On Thu, Oct 27, 2011 at 09:15:54AM +0200, walter harms wrote:
 Am 27.10.2011 06:21, schrieb Peter Hutterer:
  +void *_XGetRequest(Display *dpy, CARD8 type, size_t len)
  +{
  +xReq *req;
  +
  +WORD64ALIGN
  +
  +if (dpy-bufptr + len  dpy-bufmax)
  +   _XFlush(dpy);
  +
  +if (len % 4)
  +   fprintf(stderr,
  +   Xlib: request %d length %zd not a multiple of 4.\n,
  +   type, len);
 
 Does it make sense to continue here ?
 perhaps you want a add a return NULL ?

It doesn't make sense to continue, but there's no way to report the
error that any caller can handle. If you return NULL here, the caller is
guaranteed to segfault.

Since these errors are already possible today, but aren't being even
noticed, I think Peter's choice of a printf is the best we can do. At
least it allows the possibility of somebody noticing the bug.

It'd be nice if we could get more information than the minor opcode for
extension requests, but nothing else is immediately obvious to me here.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11] Add _XGetRequest as substitute for GetReq/GetReqExtra

[Jamey Sharp]

I pretty much approve whole-heartedly of this patch, but I have a couple
nits to ask that you fix.

On Tue, Oct 25, 2011 at 01:37:56PM +1000, Peter Hutterer wrote:
 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
  include/X11/Xlibint.h |   51 
  src/XlibInt.c |   26 +
  2 files changed, 43 insertions(+), 34 deletions(-)
 
 diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h
 index 083498f..2074e96 100644
 --- a/include/X11/Xlibint.h
 +++ b/include/X11/Xlibint.h
 @@ -420,6 +420,19 @@ extern LockInfoPtr _Xglobal_lock;
  #define WORD64ALIGN
  #endif /* WORD64 */
  
 +/**
 + * Return a len-sized request buffer for the request type. This function may
 + * flush the buffer.

Can I suggest may flush the output queue? Otherwise it sounds like it
does something to the returned buffer.

 + *
 + * @param dpy The display connection
 + * @param type The request qtype

request type, right?

 + * @param len Length of the request in bytes
 + *
 + * @returns A pointer to the request buffer with a few default values
 + * initialized.
 + */
 +extern void*
 +_XGetRequest(Display *dpy, CARD8 type, size_t len);

There's almost no precedent in libX11 to derive style guidelines for
void * vs. void*, so there's no local-consistency argument to make;
but I'd prefer a space, both in the declaration here and the function
definition in XlibInt.c.

  /*
   * GetReq - Get the next available X request packet in the buffer and
 @@ -432,25 +445,10 @@ extern LockInfoPtr _Xglobal_lock;
  
  #if !defined(UNIXCPP) || defined(ANSICPP)
  #define GetReq(name, req) \
 -WORD64ALIGN\
 - if ((dpy-bufptr + SIZEOF(x##name##Req))  dpy-bufmax)\
 - _XFlush(dpy);\
 - req = (x##name##Req *)(dpy-last_req = dpy-bufptr);\
 - req-reqType = X_##name;\
 - req-length = (SIZEOF(x##name##Req))2;\
 - dpy-bufptr += SIZEOF(x##name##Req);\
 - dpy-request++
 -
 + req = _XGetRequest(dpy, X_##name, SIZEOF(x##name##Req))
  #else  /* non-ANSI C uses empty comment instead of ## for token 
 concatenation */
  #define GetReq(name, req) \
 -WORD64ALIGN\
 - if ((dpy-bufptr + SIZEOF(x/**/name/**/Req))  dpy-bufmax)\
 - _XFlush(dpy);\
 - req = (x/**/name/**/Req *)(dpy-last_req = dpy-bufptr);\
 - req-reqType = X_/**/name;\
 - req-length = (SIZEOF(x/**/name/**/Req))2;\
 - dpy-bufptr += SIZEOF(x/**/name/**/Req);\
 - dpy-request++
 + req = _XGetRequest(dpy, X_/**/name, SIZEOF(x/**/name/**/Req))
  #endif
  
  /* GetReqExtra is the same as GetReq, but allocates n additional
 @@ -458,24 +456,10 @@ extern LockInfoPtr _Xglobal_lock;
  
  #if !defined(UNIXCPP) || defined(ANSICPP)
  #define GetReqExtra(name, n, req) \
 -WORD64ALIGN\
 - if ((dpy-bufptr + SIZEOF(x##name##Req) + n)  dpy-bufmax)\
 - _XFlush(dpy);\
 - req = (x##name##Req *)(dpy-last_req = dpy-bufptr);\
 - req-reqType = X_##name;\
 - req-length = (SIZEOF(x##name##Req) + n)2;\
 - dpy-bufptr += SIZEOF(x##name##Req) + n;\
 - dpy-request++
 + req = _XGetRequest(dpy, X_##name, SIZEOF(x##name##Req) + n)
  #else
  #define GetReqExtra(name, n, req) \
 -WORD64ALIGN\
 - if ((dpy-bufptr + SIZEOF(x/**/name/**/Req) + n)  dpy-bufmax)\
 - _XFlush(dpy);\
 - req = (x/**/name/**/Req *)(dpy-last_req = dpy-bufptr);\
 - req-reqType = X_/**/name;\
 - req-length = (SIZEOF(x/**/name/**/Req) + n)2;\
 - dpy-bufptr += SIZEOF(x/**/name/**/Req) + n;\
 - dpy-request++
 + req = _XGetRequest(dpy, X_/**/name, SIZEOF(x/**/name/**/Req) + n)
  #endif
  
  /* GetReqSized is the same as GetReq but allows the caller to specify the

These implementations of GetReq and GetReqExtra look to me like huge
improvements in every way except one: casting to (x##name##Req *) before
assigning to req enabled the compiler to check that the argument to
GetReq matched the type of the declared request pointer. I'd put that
cast back on each macro-ized call to _XGetRequest.

GetEmptyReq should also be easy to replace:
req = (xReq *) _XGetRequest(dpy, X_##name, SIZEOF(xReq))

And I think GetResReq isn't too bad either:
req = (xResourceReq *) _XGetRequest(dpy, X_##name, 
SIZEOF(xResourceReq)); \
req-id = (rid)

I'd like to see all four variants fixed up.

 @@ -503,7 +487,6 @@ extern LockInfoPtr _Xglobal_lock;
  #endif
  
  
 -
  /*
   * GetResReq is for those requests that have a resource ID
   * (Window, Pixmap, GContext, etc.) as their single argument.

This hunk seems a little frivolous. :-)

 diff --git a/src/XlibInt.c b/src/XlibInt.c
 index 3db151e..a143dc4 100644
 --- a/src/XlibInt.c
 +++ b/src/XlibInt.c
 @@ -1956,6 +1956,32 @@ Screen *_XScreenOfWindow(Display *dpy, Window w)
  }
  
  
 +
 +void*
 +_XGetRequest(Display *dpy, CARD8 type, size_t len)

I'd like a big warning comment in here. Something like:

/*
 * WARNING: This implementation's pre-conditions and 

Re: adding a custom modeline with xrandr (dummy driver)

[Jamey Sharp]

On Mon, Oct 24, 2011 at 05:20:29PM +0700, Antoine Martin wrote:
 Whenever an xpra client connects to a server I want to resize the dummy
 xserver to match the client's resolution exactly.
 ...
 Failed to change the screen configuration!

Off-hand, I'd guess nobody has implemented RANDR support in
xf86-video-dummy. But since I don't know what drivers need to do for
that, for all I know there could be some generic support in the core
that's failing here, instead.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: Glamor - next step

[Jamey Sharp]

On Fri, Oct 21, 2011 at 11:00:20AM +0800, Zhigang Gong wrote:
 Hi folks,
 
 Thanks for your valuable advices. To incrementally merge glamor with current
 video driver is indeed a more reasonable and less risk solution than merge it
 directly into xorg.
 
 The idea to bind BOs to a textures works fine. Actually, I already did so 
 when I 
 try to get DRI2 work with glamor.
 
 Now, it become clear for next step.
 
 1. Extract the device independent part of glamor into an independent library 
 which
   could be linked by other ddx driver.
 2. Incrementally change current intel-video driver's rendering path to 
 GL-based
   rendering path.
 
 If there is still any misunderstanding, please help to point it out.
 Thanks.

That looks perfect. I'm hoping you will eventually port the Xephyr bits
into xf86-video-nested, too, but demonstrating on one driver would be a
great start, and helps illustrate what to do if somebody else wants to
try it with another driver.

This plan also has the advantage that you only need your co-workers'
approval to merge your work, not the rest of us on xorg-devel. :-)

Jamey

 -- zhigang
 
  -Original Message-
  From: xorg-devel-bounces+zhigang.gong=linux.intel@lists.x.org
  [mailto:xorg-devel-bounces+zhigang.gong=linux.intel@lists.x.org] On
  Behalf Of Eric Anholt
  Sent: Thursday, October 20, 2011 11:32 PM
  To: Jamey Sharp; Zhigang Gong; Jeremy Huddleston
  Cc: xorg-devel@lists.x.org
  Subject: Re: [Pull v2] Glamor - fixed build failure, fixed coding style
  problem.
  
  On Thu, 20 Oct 2011 09:54:29 +0200, Jamey Sharp ja...@minilop.net
  wrote:
  Non-text part: multipart/mixed
  Non-text part: multipart/signed
   This is also like Xgl, as I recall, in that Xgl was a large
   development effort happening inside a company (Novell, in that case)
   and then being dropped on the X.Org community. I'd say that had a lot
   to do with it eventually failing.
  
   A bunch of this work has Eric Anholt's and Kristian Høgsberg's names
   on it, and I'd have expected them to remember that debacle.
  
  I didn't get very far in the work before deciding that getting DRI2 working
  was going to be hard and that further work wasn't useful until I had some
  sort of plan for it.
  
  My intention back when I was working on it was to do incremental merging
  once DRI2 was figured out -- there's an obvious merge point where
  correct all-software-fallbacks are possible, then another when the first
  couple of blit paths are in, then Render accel.
  
  But now, in order to get DRI2 working with GL-based 2D, we're thinking of
  flipping the whole thing on its head and using our current driver, binding
  its BOs into GL, and doing incremental migration of our current rendering
  to GL rendering that way.  We've burned ourselves switching to new AAs
  both times we've done it, so any plan that involves incremental changes
  has a lot recommending it.
 


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] pci: Restore pciTag as a deprecated function

[Jamey Sharp]

On Wed, Oct 19, 2011 at 10:47:17AM -0700, Jeremy Huddleston wrote:
 Actually, I retract this.  I didn't intend to do it originally, but
 now that it's done, meh.

If you change your mind:

Reviewed-by: Jamey Sharp ja...@minilop.net

But I, for one, am perfectly happy with meh. :-)

 I think we coddled the drivers enough with the current set of
 just-deprecations.  This is too trivial.  I'll just go through and
 make sure nobody is actually using it because drivers still build
 successfully due to us not using
 -Werror=implicit-function-declaration
 
 Perhaps we should add -Werror=implicit-function-declaration to all the
 drivers...

Any reason not to add it to xorg-macros, and get the equivalents for Sun
Studio and clang in too? I think it should be safe to say that every
instance of an implicit function declaration anywhere in X.Org is a bug.

Jamey

 On Oct 19, 2011, at 12:20 AM, Jeremy Huddleston wrote:
 
  
  This partially reverts b3d56d06ef840bbbe16ec3c37e170078b7f98b04 to allow
  driver developers time to adjust.
  
  Signed-off-by: Jeremy Huddleston jerem...@apple.com
  ---
  The commit that deprecated the PCITAG type had these leftovers that I 
  didn't mean to get rid of just yet.
  
  hw/xfree86/os-support/bus/Pci.c |6 ++
  hw/xfree86/os-support/bus/xf86Pci.h |1 +
  2 files changed, 7 insertions(+), 0 deletions(-)
  
  diff --git a/hw/xfree86/os-support/bus/Pci.c 
  b/hw/xfree86/os-support/bus/Pci.c
  index 0362a00..f1dbfc2 100644
  --- a/hw/xfree86/os-support/bus/Pci.c
  +++ b/hw/xfree86/os-support/bus/Pci.c
  @@ -126,6 +126,12 @@
  
  #include Pci.h
  
  +PCITAG
  +pciTag(int busnum, int devnum, int funcnum)
  +{
  +   return(PCI_MAKE_TAG(busnum,devnum,funcnum));
  +}
  +
  Bool
  xf86scanpci(void)
  {
  diff --git a/hw/xfree86/os-support/bus/xf86Pci.h 
  b/hw/xfree86/os-support/bus/xf86Pci.h
  index 74ead20..55e631c 100644
  --- a/hw/xfree86/os-support/bus/xf86Pci.h
  +++ b/hw/xfree86/os-support/bus/xf86Pci.h
  @@ -251,6 +251,7 @@ typedef enum {
  
  
  /* Public PCI access functions */
  +extern _X_EXPORT _X_DEPRECATED PCITAG pciTag(int busnum, int devnum, int 
  funcnum);
  extern _X_EXPORT Bool xf86scanpci(void);
  
  /* Domain access functions.  Some of these probably shouldn't be public */
  -- 
  1.7.7
  
  
  ___
  xorg-devel@lists.x.org: X.Org development
  Archives: http://lists.x.org/archives/xorg-devel
  Info: http://lists.x.org/mailman/listinfo/xorg-devel
  
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Pull v2] Glamor - fixed build failure, fixed coding style problem.

[Jamey Sharp]

On Wed, Oct 19, 2011 at 11:20:54AM -0700, Jeremy Huddleston wrote:
 On Oct 19, 2011, at 1:02 AM, Zhigang Gong wrote:
  Just as I said above. Your thought do make sense. Currently glamor
  implements some GL based rendering functions. And in the long term,
  we may merge those functions into cairo or pixman, and let glamor
  call them directly. Just as the relation between libfb(which belongs
  to X server) and pixman.
  
  To achieve that goal, I think we still need more time. And now I
  want to open glamor at this point, and move to that direction step
  by step. And eventually, there will be a thinner glamor module. 

I completely understand your desire to get this merged. Until that
happens you can't have a clear idea of how to make further progress.
Unfortunately, that's exactly why open source communities need to hear
about projects like this earlier in the development cycle. Earlier
notice lets us tell you about problems or concerns before you do so much
work, and gets you community support for your project while you work on
it.

Since that didn't happen this time, we have a bigger challenge: how can
we work with you to make sure your effort isn't wasted? Although I'm
very excited to see effort toward implementing 2D acceleration with 3D
drivers, I'm sorry to say that I don't think this method of integrating
it should be merged. I do have an alternative proposal, though, below.

 What existing problem does your solution solve?  Can you give me an
 example of a configuration which doesn't have 2D acceleration now
 which will with glamor or a case where 2D performance has
 significantly improved by switching to your codepath?

To partially answer these questions: glamor is a great idea in principle
for at least two cases.

I've been listening to driver developers moan for years about having to
write a 2D driver, then turn around and write a separate 3D driver,
every time a new piece of hardware comes out. Glamor may enable writing
only a kernel modesetting driver and a Mesa 3D driver, and using the
generic xf86-video-modesetting userspace driver for 2D X.

For nested X (Xnest/Xephyr/Xdmx/xf86-video-nested), glamor could reduce
or eliminate the software-render-and-PutImage cycle, instead letting the
backend X server use suitably accelerated 3D. In the case where the
backend server is on the same machine as the nested server, the nested
server should wind up direct-rendering straight to the video hardware.

Based on public statements from Intel developers I'd speculate that
they're interested in both cases. I don't know what will happen with
Tizen, but the Meego development environment used Xephyr as the fake
phone screen, and apparently its lack of rendering performance was
painful. And of course like any good developers, the graphics team is
lazy and would like to spend less effort on writing drivers. :-)

Jeremy, if glamor works out, OS X should benefit eventually too. Ideally
you'll have a variant of xf86-video-dummy that direct-renders to
offscreen video memory, which you'd composite into native windows using
GL handles. Glamor ought to enable 2D offscreen acceleration there too.

 We want to minimize the API boundary between the server and its
 drivers.  Rather than provide a server module which will present
 glamor API exposed by the Xserver for drivers to use, I'd prefer you
 provide this as a library that drivers (or even non-Xorg DDX) can link
 against.

Just so this is very clear: The *reason* we want to minimize the
server's API boundary is because anything that we expose in the server's
API, we have to continue to support. That ongoing support burden is why
Jeremy is pushing you to expose this functionality in a different way.

Here's my proposal: extract what you have in xserver/glamor as a
library, then use that library in xf86-video-modesetting (instead of the
glamor DDX) and xf86-video-nested (instead of Xephyr). Dave Airlie has
put a lot of work into the modesetting driver over the past few weeks,
and Jeremy and I have been working on the nested driver, so hopefully
you can get community support for any issues you run into in those
drivers.

 XGL tried to solve this same problem ... how do we know this won't
 flop?

This is also like Xgl, as I recall, in that Xgl was a large development
effort happening inside a company (Novell, in that case) and then being
dropped on the X.Org community. I'd say that had a lot to do with it
eventually failing.

A bunch of this work has Eric Anholt's and Kristian Høgsberg's names on
it, and I'd have expected them to remember that debacle.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 3/5] Xi: avoid overrun of callback array.

[Jamey Sharp]

On Wed, Oct 19, 2011 at 05:01:45PM +0100, Dave Airlie wrote:
 From: Dave Airlie airl...@redhat.com
 
 This code had an off-by-one and would allow writing one past the end of
 the callbacks array.

I think you mean reading one past the end? I don't see any bad pointer
writes here, though I'm only looking at the patch context.

Jamey

 Pointed out by coverity.
 
 Signed-off-by: Dave Airlie airl...@redhat.com
 ---
  Xi/extinit.c |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/Xi/extinit.c b/Xi/extinit.c
 index 7724f5f..1fbe0a2 100644
 --- a/Xi/extinit.c
 +++ b/Xi/extinit.c
 @@ -409,7 +409,7 @@ static int
  ProcIDispatch(ClientPtr client)
  {
  REQUEST(xReq);
 -if (stuff-data  ARRAY_SIZE(ProcIVector) || !ProcIVector[stuff-data])
 +if (stuff-data = ARRAY_SIZE(ProcIVector) || !ProcIVector[stuff-data])
  return BadRequest;
  
  return (*ProcIVector[stuff-data])(client);
 @@ -428,7 +428,7 @@ static int
  SProcIDispatch(ClientPtr client)
  {
  REQUEST(xReq);
 -if (stuff-data  ARRAY_SIZE(SProcIVector) || !SProcIVector[stuff-data])
 +if (stuff-data = ARRAY_SIZE(SProcIVector) || 
 !SProcIVector[stuff-data])
  return BadRequest;
  
  return (*SProcIVector[stuff-data])(client);
 -- 
 1.7.6.4
 
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/5] fbdevhw: iterate over all modes that match a mode. (v2)

[Jamey Sharp]

Reviewed-by: Jamey Sharp ja...@minilop.net

I haven't tested it, nor do I have mind-reading powers, but I think
this logic means what you think it means. Thanks for the revisions.

Jamey

On 10/19/11, Dave Airlie airl...@gmail.com wrote:
 From: Adam Jackson a...@redhat.com

 So on RHEL5 anaconda sets an xorg.conf with a fixed 800x600 mode in it,
 we run radeonfb and fbdev since ati won't work in userspace due to domain
 issues in the older codebase.

 On certain pseries blades the built-in KVM can't accept an 800x600-43 mode,
 it requires the 800x600-60 mode, so we have to have the kernel radeonfb
 driver reject the 800x600-43 mode when it sees it. However then fbdev
 doesn't try any of the other 800x600 modes in the modelist, and we end up
 getting a default 640x480 mode we don't want.

 This patch changes the mode validation loop to continue on with the other
 modes
 that match to find one that works.

 v2: move code around to avoid extra loop, after comment from Jamey.

 Signed-off-by: Dave Airlie airl...@redhat.com
 ---
  hw/xfree86/fbdevhw/fbdevhw.c |   21 +
  1 files changed, 13 insertions(+), 8 deletions(-)

 diff --git a/hw/xfree86/fbdevhw/fbdevhw.c b/hw/xfree86/fbdevhw/fbdevhw.c
 index dee731b..7bd409e 100644
 --- a/hw/xfree86/fbdevhw/fbdevhw.c
 +++ b/hw/xfree86/fbdevhw/fbdevhw.c
 @@ -509,20 +509,25 @@ fbdevHWSetVideoModes(ScrnInfoPtr pScrn)
   pScrn-virtualY = pScrn-display-virtualY;

   for (modename = pScrn-display-modes; *modename != NULL; modename++) {
 - for (mode = pScrn-monitor-Modes; mode != NULL; mode = 
 mode-next)
 - if (0 == strcmp(mode-name,*modename))
 - break;
 +
 + mode = pScrn-monitor-Modes;
 +
 + for (; mode != NULL; mode = mode-next) {
 + if (0 == strcmp(mode-name,*modename)) {
 + if (fbdevHWSetMode(pScrn, mode, TRUE))
 + break;
 +
 + xf86DrvMsg(pScrn-scrnIndex, X_INFO,
 +\tmode \%s\ test failed\n, 
 *modename);
 + }
 + }
 +
   if (NULL == mode) {
   xf86DrvMsg(pScrn-scrnIndex, X_INFO,
  \tmode \%s\ not found\n, *modename);
   continue;
   }

 - if (!fbdevHWSetMode(pScrn, mode, TRUE)) {
 - xf86DrvMsg(pScrn-scrnIndex, X_INFO,
 -\tmode \%s\ test failed\n, *modename);
 - continue;
 - }
   xf86DrvMsg(pScrn-scrnIndex, X_INFO,
  \tmode \%s\ ok\n, *modename);

 --
 1.7.6.4

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11] include: Add GetReqSized() for request buffers of specific size

[Jamey Sharp]

On Mon, Oct 17, 2011 at 09:19:02AM +1000, Peter Hutterer wrote:
 On Fri, Oct 14, 2011 at 06:42:02PM +0200, Jamey Sharp wrote:
  The GetReq family of macros is so evil. I'm not going to go so far as
  to NAK this patch or anything, but please consider whether you can
  implement this as a proper function or something. Perhaps:
  
  void *XGetReq(CARD8 reqType, size_t sz);
  
  (I think the other macros could be re-implemented on top of that
  function, so long as the function continues to behave in a manner
  compatible with code compiled using the current macros.)
  
  Also, do you care about big-requests here? I can't remember how Xlib
  handles that extension.
 
 fwiw, GetReqSized is more-or-less a copy of GetReqExtra with one parameter
 replaced. And GetReqExtra in turn is a copy of GetReq.

I certainly recognized the pattern. It's an awful pattern and I want it
to stop. :-)

 From a quick glance neither of them handle big-req directly.

If I had looked at the source, I'd have noticed that SetReqLen is
apparently the standard way to fix up a request to be a big-request,
after GetReq returns a standard one.

 I'll have a look at getting this to a function instead of a macro.

Thanks!

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH V4] xserver: check for elevated privileges not uid=0

[Jamey Sharp]

Reviewed-by: Jamey Sharp ja...@minilop.net

Since this has gone through so many changes I tried to carefully
re-review the whole thing, and it still looks good to me. Thanks,
Antoine!

We don't actually put xserver: in the commit message for a server
patch, though. It wouldn't be wrong to tag this xfree86: even though
it touches autoconf-ery because it's really all for the benefit of the
xfree86 DDX, as written; but it would be OK to leave off the tag
entirely, too.

The next trick is to get Keith to merge this patch, and/or get more
reviewed-by/tested-by tags.

Jamey

On Tue, Oct 11, 2011 at 03:02:50PM +0700, Antoine Martin wrote:
 This allows us to run the server as a normal user whilst still
 being able to use the -modulepath, -logfile and -config switches
 We define a xf86PrivsElevated which will do the checks and cache
 the result in case it is called more than once.
 Also renamed the paths #defines to match their new meaning.
 Original discussion which led to this patch can be found here:
 http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html
 
 Signed-off-by: Antoine Martin anto...@nagafix.co.uk
 ---
 
 Changes from the previous version of this patch:
 * use FatalError() rather than exit() (thx to Julien Cristau)
 * remove comment superseded by FatalError message (thx to Tormod Volden)
 * more defensive code: print warning and assume privs are elevated
 if getresuid or getresgid fail, ensure all codepaths set
 privsElevated explicitly and make it default to TRUE (in case we
 ever miss one)
 * email title includes patch version in the right location
 (btw, there are changes in xserver's autoconf, not just hw/xfree86)
 
 Thanks
 Antoine
 
  configure.ac   |4 ++-
  hw/xfree86/common/xf86Config.c |   28 +++---
  hw/xfree86/common/xf86Init.c   |   76 
 +++-
  hw/xfree86/common/xf86Priv.h   |1 +
  include/xorg-config.h.in   |6 +++
  5 files changed, 91 insertions(+), 24 deletions(-)
 
 diff --git a/configure.ac b/configure.ac
 index 67a6836..a79e2be 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -208,9 +208,11 @@ AC_SUBST(DLOPEN_LIBS)
  
  dnl Checks for library functions.
  AC_FUNC_VPRINTF
 -AC_CHECK_FUNCS([geteuid getuid link memmove memset mkstemp strchr strrchr \
 +AC_CHECK_FUNCS([geteuid getuid issetugid getresuid \
 + link memmove memset mkstemp strchr strrchr \
   strtol getopt getopt_long vsnprintf walkcontext backtrace \
   getisax getzoneid shmctl64 strcasestr ffs vasprintf])
 +
  AC_FUNC_ALLOCA
  dnl Old HAS_* names used in os/*.c.
  AC_CHECK_FUNC([getdtablesize],
 diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
 index f8c1b65..8ccc52a 100644
 --- a/hw/xfree86/common/xf86Config.c
 +++ b/hw/xfree86/common/xf86Config.c
 @@ -72,8 +72,8 @@
   * These paths define the way the config file search is done.  The escape
   * sequences are documented in parser/scan.c.
   */
 -#ifndef ROOT_CONFIGPATH
 -#define ROOT_CONFIGPATH  %A, %R, \
 +#ifndef ALL_CONFIGPATH
 +#define ALL_CONFIGPATH   %A, %R, \
   /etc/X11/%R, %P/etc/X11/%R, \
   %E, %F, \
   /etc/X11/%F, %P/etc/X11/%F, \
 @@ -83,8 +83,8 @@
   %P/lib/X11/%X.%H, \
   %P/lib/X11/%X
  #endif
 -#ifndef USER_CONFIGPATH
 -#define USER_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \
 +#ifndef RESTRICTED_CONFIGPATH
 +#define RESTRICTED_CONFIGPATH/etc/X11/%S, %P/etc/X11/%S, \
   /etc/X11/%G, %P/etc/X11/%G, \
   /etc/X11/%X, /etc/%X, \
   %P/etc/X11/%X.%H, \
 @@ -92,14 +92,14 @@
   %P/lib/X11/%X.%H, \
   %P/lib/X11/%X
  #endif
 -#ifndef ROOT_CONFIGDIRPATH
 -#define ROOT_CONFIGDIRPATH   %A, %R, \
 +#ifndef ALL_CONFIGDIRPATH
 +#define ALL_CONFIGDIRPATH%A, %R, \
   /etc/X11/%R, %C/X11/%R, \
   /etc/X11/%X, %C/X11/%X
  #endif
 -#ifndef USER_CONFIGDIRPATH
 -#define USER_CONFIGDIRPATH   /etc/X11/%R, %C/X11/%R, \
 - /etc/X11/%X, %C/X11/%X
 +#ifndef RESTRICTED_CONFIGDIRPATH
 +#define RESTRICTED_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \
 + /etc/X11/%X, %C/X11/%X
  #endif
  #ifndef SYS_CONFIGDIRPATH
  #define SYS_CONFIGDIRPATH/usr/share/X11/%X, %D/X11/%X
 @@ -2302,12 +2302,12 @@ xf86HandleConfigFile(Bool autoconfig)
  Bool implicit_layout = FALSE;
  
  if (!autoconfig) {
 - if (getuid() == 0) {
 - filesearch = ROOT_CONFIGPATH;
 - dirsearch = ROOT_CONFIGDIRPATH;
 + if (!xf86PrivsElevated()) {
 + filesearch = ALL_CONFIGPATH;
 + dirsearch = ALL_CONFIGDIRPATH;
   } else {
 - filesearch = USER_CONFIGPATH;
 - dirsearch = USER_CONFIGDIRPATH;
 + filesearch = RESTRICTED_CONFIGPATH;
 + dirsearch

Re: [PULL] build fix, GC clipping cleanup

[Jamey Sharp]

On Mon, Oct 17, 2011 at 02:06:55PM -0700, Keith Packard wrote:
 On Wed, 5 Oct 2011 08:30:56 -0700, Jamey Sharp ja...@minilop.net wrote:
 
  Alright, the pixmap hooks have weird undocumented constraints, and
  maybe we'll figure out something clever someday. Meanwhile, would you
  please merge the other three commits?
 
 For other ABI changes like this, we've provided compile-time tests you
 can use to switch the code without using the X server ABI version
 information. That way, it's easy to make video drivers compile across
 the change.
 
 The hardest part will be picking a suitable name which makes the
 difference reasonably clear.
 
 Otherwise, these changes look good to me; fixing the driver to handle
 the change was straightforward.

Augh, another round on the patches I'm stalled waiting for. It's a fair
critique though.

Do you care if drivers still work in between the two patches? I'm not
keen on adding two separate #defines when the changes are conceptually
paired like this, but the second patch is only clearly correct after the
first is in place, so I hate to squash them. At least they should both
trigger compile-time errors in any code that isn't updated, making the
cause of a bisect failure somewhat obvious.

Could you at least merge the stupid trivial gitignore fix so I can get
something out of my tree?

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libX11] include: Add GetReqSized() for request buffers of specific size

[Jamey Sharp]

The GetReq family of macros is so evil. I'm not going to go so far as
to NAK this patch or anything, but please consider whether you can
implement this as a proper function or something. Perhaps:

void *XGetReq(CARD8 reqType, size_t sz);

(I think the other macros could be re-implemented on top of that
function, so long as the function continues to behave in a manner
compatible with code compiled using the current macros.)

Also, do you care about big-requests here? I can't remember how Xlib
handles that extension.

Jamey

On 10/14/11, Peter Hutterer peter.hutte...@who-t.net wrote:
 Some XI2 requests change in size over different versions and libXi would
 need to hack around GetReq and GetReqExtra. Add a new GetReqSized so the
 library can explicitly specify the size of the request in 4-byte units.

 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 ---
 Instead of hacking around in libXi with GetReq, let's just add a new macro.

  include/X11/Xlibint.h |   25 +
  1 files changed, 25 insertions(+), 0 deletions(-)

 diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h
 index 2ce356d..083498f 100644
 --- a/include/X11/Xlibint.h
 +++ b/include/X11/Xlibint.h
 @@ -478,6 +478,31 @@ extern LockInfoPtr _Xglobal_lock;
   dpy-request++
  #endif

 +/* GetReqSized is the same as GetReq but allows the caller to specify the
 + * size in 4-byte units */
 +#if !defined(UNIXCPP) || defined(ANSICPP)
 +#define GetReqSized(name, sz, req) \
 +WORD64ALIGN\
 + if ((dpy-bufptr + sz * 4)  dpy-bufmax)\
 + _XFlush(dpy);\
 + req = (x##name##Req *)(dpy-last_req = dpy-bufptr);\
 + req-reqType = X_##name;\
 + req-length = sz; \
 + dpy-bufptr += sz * 4;\
 + dpy-request++
 +#else
 +#define GetReqSized(name, sz, req) \
 +WORD64ALIGN\
 + if ((dpy-bufptr + sz * 4)  dpy-bufmax)\
 + _XFlush(dpy);\
 + req = (x/**/name/**/Req *)(dpy-last_req = dpy-bufptr);\
 + req-reqType = X_/**/name;\
 + req-length = sz; \
 + dpy-bufptr += sz * 4; \
 + dpy-request++
 +#endif
 +
 +

  /*
   * GetResReq is for those requests that have a resource ID
 --
 1.7.6.4

 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] check for elevated privileges rather than just euid=0

[Jamey Sharp]

On 10/6/11, Michal Suchanek hramr...@centrum.cz wrote:
 I would like to check this out but how do I tell this actually works?

 Xephyr and Xvfb already run as non-root, Xorg needs device access to
 run which will likely fail as non-root.

The motivation for Antoine's patch is the xf86-video-dummy driver, and
the same problems come up with xf86-video-nested. You could try either
of those. The thread that Antoine cited in the commit message has more
information on setting up an appropriate xorg.conf.

Hopefully, someday, device access won't require root either, and this
is a step in the right direction for that too. KMS drivers could
probably run unprivileged with little work...

Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL] build fix, GC clipping cleanup

[Jamey Sharp]

Alright, the pixmap hooks have weird undocumented constraints, and
maybe we'll figure out something clever someday. Meanwhile, would you
please merge the other three commits?

Thanks,
Jamey

On 10/3/11, Jamey Sharp ja...@minilop.net wrote:
 On Mon, Oct 03, 2011 at 12:01:23PM -0700, Keith Packard wrote:
 On Wed, 28 Sep 2011 13:57:27 -0700, Jamey Sharp ja...@minilop.net wrote:
  Hi Keith! Here's some reviewed code deletion (hooray!) and the fix to
  make kdrive build again after the last pull.

 There was a comment requesting that FreePixmap be renamed
 'UnreferencePixmap' or some such?

 Yeah, and I'm not opposed. (Although I'm hoping for a better name
 choice...) It's probably a good idea to change the names anyway so any
 unfixed callers are detected at compile time.

 Would you pull the rest of my reviewed patches in the meantime though? I
 have a lot of patches that depend on the clipping changes going in, and
 the missing .gitignore entry keeps bugging me.

 The following changes since commit 6378d0233d21088b6429755627b4253859892c72:

   Merge remote-tracking branch 'herrb/master' (2011-10-03 13:56:06 -0700)

 are available in the git repository at:

   git://anongit.freedesktop.org/~jamey/xserver reviewed

 Jamey Sharp (3):
   test: .gitignore the binary for xfree86.c.
   Quit wrapping ChangeClip, CopyClip, and DestroyClip in GCFuncs.
   Make GC clientClip always be a region.

  Xext/panoramiX.c   |   32 --
  dix/gc.c   |   64 ++---
  doc/Xserver-spec.xml   |   76 +--
  exa/exa.c  |   48 -
  exa/exa_accel.c|   18 ++--
  exa/exa_priv.h |6 +-
  exa/exa_unaccel.c  |6 +-
  fb/fbgc.c  |3 -
  hw/dmx/dmxgc.c |  115 +++---
  hw/dmx/dmxgc.h |3 -
  hw/kdrive/src/kxv.c|2 +-
  hw/xfree86/common/xf86VGAarbiter.c |   31 +--
  hw/xfree86/common/xf86VGAarbiterPriv.h |4 -
  hw/xfree86/common/xf86xv.c |2 +-
  hw/xfree86/shadowfb/shadow.c   |   35 ---
  hw/xfree86/xaa/xaaBitBlt.c |4 +-
  hw/xfree86/xaa/xaaGC.c |   31 --
  hw/xnest/GC.c  |  170
 +---
  hw/xnest/XNGC.h|4 -
  hw/xwin/wingc.c|   42 
  include/gc.h   |2 +-
  include/gcstruct.h |   25 ++---
  mi/mibitblt.c  |4 +-
  mi/micopy.c|4 +-
  mi/miexpose.c  |2 +-
  mi/migc.c  |   74 +--
  mi/migc.h  |   16 ---
  mi/mioverlay.c |2 +-
  miext/cw/cw.c  |   52 +-
  miext/damage/damage.c  |   31 --
  miext/rootless/rootlessGC.c|   29 --
  render/mirect.c|2 +-
  test/.gitignore|1 +
  xfixes/region.c|   27 ++
  34 files changed, 164 insertions(+), 803 deletions(-)

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL] build fix, GC clipping cleanup

[Jamey Sharp]

On Mon, Oct 03, 2011 at 03:13:09PM -0700, Jamey Sharp wrote:
 On Mon, Oct 03, 2011 at 12:01:23PM -0700, Keith Packard wrote:
  On Wed, 28 Sep 2011 13:57:27 -0700, Jamey Sharp ja...@minilop.net wrote:
   Hi Keith! Here's some reviewed code deletion (hooray!) and the fix to
   make kdrive build again after the last pull.
  
  There was a comment requesting that FreePixmap be renamed
  'UnreferencePixmap' or some such?
 
 Yeah, and I'm not opposed. (Although I'm hoping for a better name
 choice...) It's probably a good idea to change the names anyway so any
 unfixed callers are detected at compile time.
 
 Would you pull the rest of my reviewed patches in the meantime though? I
 have a lot of patches that depend on the clipping changes going in, and
 the missing .gitignore entry keeps bugging me.

And now, with that fixed and review tags updated, please pull the whole
thing:

The following changes since commit 6378d0233d21088b6429755627b4253859892c72:

  Merge remote-tracking branch 'herrb/master' (2011-10-03 13:56:06 -0700)

are available in the git repository at:

  git://anongit.freedesktop.org/~jamey/xserver reviewed

Jamey Sharp (6):
  test: .gitignore the binary for xfree86.c.
  Quit wrapping ChangeClip, CopyClip, and DestroyClip in GCFuncs.
  Make GC clientClip always be a region.
  Introduce ReleasePixmap to unreference, call screen hooks, and free.
  Introduce CreatePixmap, allocating a header and calling screen hooks.
  Move pixmap size limit checking to CreatePixmap, from screen hooks.

 Xext/panoramiX.c   |   32 --
 Xext/saver.c   |4 +-
 Xext/shm.c |   34 +++
 Xext/xvmain.c  |   10 +--
 composite/compalloc.c  |6 +-
 composite/compwindow.c |7 +-
 dbe/midbe.c|   42 +++-
 dix/dispatch.c |   12 +--
 dix/gc.c   |   89 -
 dix/glyphcurs.c|8 +-
 dix/pixmap.c   |   55 +++
 dix/window.c   |   21 ++--
 doc/Xserver-spec.xml   |  119 +++---
 exa/exa.c  |   48 -
 exa/exa_accel.c|   18 ++--
 exa/exa_classic.c  |   70 ++---
 exa/exa_driver.c   |   50 --
 exa/exa_glyphs.c   |   20 ++---
 exa/exa_mixed.c|   56 +--
 exa/exa_offscreen.c|4 +-
 exa/exa_priv.h |   27 +++---
 exa/exa_render.c   |7 +-
 exa/exa_unaccel.c  |6 +-
 fb/fb.h|   12 +--
 fb/fb24_32.c   |2 +-
 fb/fbgc.c  |7 +-
 fb/fboverlay.c |4 +-
 fb/fbpixmap.c  |   67 +++--
 fb/fbwindow.c  |2 +-
 glx/glxcmds.c  |3 +-
 hw/dmx/dmxgc.c |  115 +++---
 hw/dmx/dmxgc.h |3 -
 hw/dmx/dmxpixmap.c |   42 ++---
 hw/dmx/dmxpixmap.h |6 +-
 hw/dmx/glxProxy/glxext.c   |4 +-
 hw/kdrive/src/kxv.c|2 +-
 hw/xfree86/common/xf86DGA.c|6 +-
 hw/xfree86/common/xf86VGAarbiter.c |   42 ++---
 hw/xfree86/common/xf86VGAarbiterPriv.h |7 +-
 hw/xfree86/common/xf86xv.c |2 +-
 hw/xfree86/shadowfb/shadow.c   |   35 ---
 hw/xfree86/xaa/xaaBitBlt.c |4 +-
 hw/xfree86/xaa/xaaGC.c |   31 --
 hw/xfree86/xaa/xaaInit.c   |   99 +--
 hw/xnest/GC.c  |  170 +---
 hw/xnest/Pixmap.c  |   40 ++--
 hw/xnest/XNGC.h|4 -
 hw/xnest/XNPixmap.h|5 +-
 hw/xwin/win.h  |5 +-
 hw/xwin/wingc.c|   42 
 hw/xwin/winpixmap.c|   51 +++---
 include/gc.h   |2 +-
 include/gcstruct.h |   25 ++---
 include/pixmap.h   |9 +-
 include/scrnintstr.h   |   10 +--
 mi/miarc.c |9 +-
 mi/mibitblt.c  |   18 ++--
 mi/micopy.c|4 +-
 mi/midispcur.c |   26 +++---
 mi/miexpose.c  |2 +-
 mi/migc.c  |   76 +--
 mi/migc.h  |   16 ---
 mi/miglblt.c   |   10 +-
 mi/mioverlay.c |2 +-
 mi

Re: [PATCH 0/3] simplify pixmap create/free hooks

[Jamey Sharp]

On Tue, Oct 04, 2011 at 10:41:48AM +0100, Chris Wilson wrote:
 On Sat,  1 Oct 2011 23:08:45 -0700, Jamey Sharp ja...@minilop.net wrote:
  Jamey Sharp (3):
Have FreePixmap call screen hooks, not the other way around.
Introduce CreatePixmap, allocating a header and calling screen hooks.
Move pixmap size limit checking to CreatePixmap, from screen hooks.
 
 Where's the ABI bump for out-of-tree drivers?

Are we doing that for every change now? There's already an ABI bump in
somebody's pull request for 1.12. I forget whether it got merged yet.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel