Re: GetScratchPixmapHeader / FreeScratchPixmapHeader

[Keith Packard]

Jeremy Huddleston Sequoia  writes:

> Is there any real benefit to maintaining this released pixmap for
> future re-use on modern hardware?  It seems like it's a bunch of code
> complexity without much benefit.  I'm happy to nuke it completely
> unless there are objections...

Seems reasonable -- it's just a CPU time optimization that probably
doesn't matter much anymore. It is often used for PutImage, but that
probably has enough overhead elsewhere to swamp malloc/free costs.

-- 
-keith


signature.asc
Description: PGP signature

Re: Xlib contract for XEvent up/down-casting

[Keith Packard]

Jeremy Huddleston Sequoia  writes:

> So the problem is clear, but I'm not sure which side needs to change.

Probably both?

How about a simple hack in XPutBackEvent that uses _XEventToWire
followed by _XWireToEvent? That would also clear out any unused bits
from the event before inserting it into the queue and check to make sure
the event type was valid.

diff --git a/src/PutBEvent.c b/src/PutBEvent.c
index 0f9df342..f7b74b31 100644
--- a/src/PutBEvent.c
+++ b/src/PutBEvent.c
@@ -79,9 +79,22 @@ XPutBackEvent (
 register XEvent *event)
{
int ret;
+   xEvent wire = {0};
+   XEvent lib = {0};
+   Status (*fp)(Display *, XEvent *, xEvent *);
+   int type = event->type & 0177;
 
LockDisplay(dpy);
-   ret = _XPutBackEvent(dpy, event);
+   fp = dpy->wire_vec[type];
+   if (fp == NULL)
+   fp = _XEventToWire;
+   ret = (*fp)(dpy, event, );
+   if (ret)
+   {
+   ret = (*dpy->event_vec[type])(dpy, , );
+   if (ret)
+   ret = _XPutBackEvent(dpy, );
+   }
UnlockDisplay(dpy);
return ret;
}

> With a quick grep through our source code, I see some (XEvent *)
> casting in many other locations (xterm, cairo, libXt, ...).  So is the
> burden on the caller to ensure that their events are properly padded
> into an XEvent, or is the burden on the recipient of an XEvent to
> ensure that they determine the event type before reading the event?

Yeah, that's problematic behavior for sure; one should never cast to a
pointer of a type which is larger. But, we can't exactly fix the Xlib
API to make that unnecessary.

I guess the question is whether there are other places in Xlib which
take an XEvent * and access fields beyond those defined by the related
union member?

> Or is this unique to XPutBackEvent(), and freeglut is abusing
> XPutBackEvent() since XPutBackEvent() should be use to "push an event
> back onto the head of the display's event queue" (meaning the event
> should have previously been popped from the queue and thus be
> appropriately sized, not something created on the stack of arbitrary
> size?

The XPutBackEvent docs don't explicitly require that the event come from
the queue, it seems to use that only as an example.

-- 
-keith


signature.asc
Description: PGP signature

Re: XInitThreads in library constructor breaks Motif!

[Keith Packard]

Po Lu  writes:

> Besides, how to move it forward? If it works, wouldn't the obvious
> solution be to install the change? If all that is needed is for
> someone to try it out, I'd be happy to do it, but I'm pretty sure the
> better solution is just to remove the call to XInitThreads in the
> first place.

As currently described, the fix is untested. I'd encourage you to apply
it and see if it works; that would at least provide some evidence that
it helps.

-- 
-keith


signature.asc
Description: PGP signature

Re: Questions about XPresent

[Keith Packard]

Po Lu  writes:

> Does that make any sense?  I'm thinking of a control flow that looks
> like this, when the client draws to the window after a pixmap has been
> presented by the compositor:
>
> Client  Server  Compositing manager
> PresentPixmap > PresentRequest ---> actually takes the pixmap
> specified and composites
> it to display.

Yeah, I've toyed with this design several times and never came up with
any solid way to manage pixmap ownership across these boundaries, plus
it doesn't really fix the underlying latency issues caused by having
three processes involved in showing each frame.

I got started creating a 'semi-automatic compositing' design where the
compositing manager could tell the X server "Hey, just composite this
window to the screen in the 'obvious' way and don't bother me with the
details".

With that, the X server could be in a position to actually make stuff
work like a real window system (iOS, Android, Windows, Mac OS X all work
this way). You'd turn it off when there was any funny business going on,
but most of the time, things could go through this fast path.

-- 
-keith


signature.asc
Description: PGP signature

Re: xserver and splitting ultra-wide monitors

[Keith Packard]

Michael Wyraz  writes:

> For the second monitor, the output must be set to "none" which is 
> obviously wrong since it is connected to a device. The reason why it is 
> set to "none" is some code in xserver that removes an monitor if another 
> one is added to the same output:

That's actually required in the RandR spec:

For each output in 'info.outputs, each one is removed from all
pre-existing Monitors. If removing the output causes the list of
outputs for that Monitor to become empty, then that Monitor will
be deleted as if RRDeleteMonitor were called.

The notion of splitting one physical output into multiple virtual
monitors was not considered when this extension was defined, which is
why it doesn't work. I don't see any particular reason for *not*
supporting your use case.

However, there are subtleties here. We want to remove any automatically
created 'Monitor' objects when mapping user-specified monitors to
them, and we want to re-generate automatically generated 'Monitors' when
all virtual monitors associated with an output are removed.

I think what we want is:

 * If no user-specified Monitors map to a particular Output, then automatically
   create a Monitor for that Output

 * If any user-specified Monitors map to a particular Output, then
   remove the automatically generated Monitor for that Output.
   
In the current spec, there's no real separation between user-specified
and automatically-generated Monitors, I think that would be necessary to
make this work?

-- 
-keith


signature.asc
Description: PGP signature

Re: [Xcb] pthread stubs in libX11 vs. libxcb

[Keith Packard]

Alan Coopersmith  writes:

> I'm not familiar with the runtime linkers on all platforms, but I know at 
> least
> on Solaris, lazy loading won't load the library until the first reference to 
> it
> is made (or a search needs to be made for a reference that doesn't have direct
> binding information to record what library to load for it).

Ah. That makes sense. I have the feeling that lots of us are planning
for or actually using non-lazy loading as that lets the relocation
tables be marked read-only after startup (-z relro -z now in the gnu
linker).

> I also don't know if the ordering between library loading and init section
> execution is well defined.

That's a good point -- with lazy loading, if the other libraries aren't
even loaded until first used, then it seems unlikely that their init
sections are invoked before then.

-- 
-keith


signature.asc
Description: PGP signature

Re: pthread stubs in libX11 vs. libxcb

[Keith Packard]

Alan Coopersmith  writes:

> Does anyone disagree?

Yeah, synchronizing with xcb seems 'obviously right. The only question
I've got is that in 2022, should xcb ever use stubs for the thread functions?

-- 
-keith


signature.asc
Description: PGP signature

Re: XRenderComposite Tie To Vsync

[Keith Packard]

Ismael Luceno  writes:

> AFAIK XRender can't do vsync. You may want to use OpenGL for
> everything, or even just for the presentation step in order to do
> the vsync...

GL and Vulkan use the Present extension, which can also be used by other
X applications.


-- 
-keith


signature.asc
Description: PGP signature

Re: RFC: New libxcvt library

[Keith Packard]

Olivier Fourdan  writes:

> Can we consider moving “libxcvt” to the xorg/lib namespace in gitlab?

Seems like a good plan to me; I've had several requests to add CVT
support to xrandr and haven't done so because it would have involved
pulling in a ton of code or creating a library like this :-)

-- 
-keith


signature.asc
Description: PGP signature
___
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: Fwd: The importance of mutual authentication: Local Privilege Escalation in X11

[Keith Packard]

Roberto Ragusa  writes:

> Wouldn't this make it impossible to run processes under different users
> by using xauth and export DISPLAY=:0 ?

As long as the other user has access to the specified path, processes
will be able to connect to the server.

-- 
-keith


signature.asc
Description: PGP signature
___
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: AW: AW: AW: Preparing for libX11 1.7.0

[Keith Packard]

Walter Harms  writes:

> nobody expects this to become bug free. The point was to raise awareness that 
> the
> same class (heap-use-after-free) are still reported.

Yup, it's important to realize that while this class of bugs don't
*seem* important, they often lead to very difficult to diagnose problems
when encountered in actual applications.

-- 
-keith


signature.asc
Description: PGP signature
___
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: AW: AW: Preparing for libX11 1.7.0

[Keith Packard]

I've posted an MR for version 1.7.0 that includes release notes added to
the README.md. It might be useful to include links to bugs known to be
addressed with this release as well?

-- 
-keith


signature.asc
Description: PGP signature
___
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

Thinking about xprop-1.2.5

[Keith Packard]

Pierre-Loup Griffais helped his kid make some fun changes in xprop -- it
can now display icons in full color, and it checks the terminal width
when displaying icons. Plus there's a new '-help' option added by Jason
Nader.

Unless anyone else is thinking about doing development here, I figure we
might as well do a release so that these changes eventually wind their
way out to users.

Any objections to my just pushing out a release in a few days? If
someone else wants to review the changes since 1.2.4, that would be
awesome too.

-- 
-keith


signature.asc
Description: PGP signature
___
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: AW: Preparing for libX11 1.7.0

[Keith Packard]

Vittorio Zecca  writes:

> Even easier to reproduce running /usr/bin/wish if you have tk installed.

I run wish (for 'gitk') many times a day and haven't seen any thing
amiss, so it could be something in your environment causing trouble.

> If there is a fix to this issue plese let me know it so I can try it
> here.

The current master branch of libX11 has a likely fix for this issue.

https://gitlab.freedesktop.org/xorg/lib/libx11

-- 
-keith


signature.asc
Description: PGP signature
___
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: AW: Preparing for libX11 1.7.0

[Keith Packard]

Alan Coopersmith  writes:

> https://lists.x.org/archives/xorg/2020-November/060510.html

I've reviewed this message and believe that this issue has already been
fixed on Xlib master -- Jacek Caban provided a set of fixes over three
years ago which have been merged along with some small additional work I
did as well:

https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/56

This series gives up on ever freeing locale information due to Xlib API
design issues, so it's likely to avoid accessing something after it has
been freed.

-- 
-keith


signature.asc
Description: PGP signature
___
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: AW: Preparing for libX11 1.7.0

[Keith Packard]

Alan Coopersmith  writes:

> Unfortunately, it only seems to have been sent to the mailing list,
> not filed in the bug tracker:
>
> https://lists.x.org/archives/xorg/2020-November/060510.html

Thanks, Alan. Should we wait and see if more information about this bug
appears before finalizing a 1.7.0 release? It doesn't seem like there's
a huge urgency for the 1.7.0 release, but there are some useful fixes
and API additions that would be nice to get put to bed before we all
forget about them...

-- 
-keith


signature.asc
Description: PGP signature
___
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: AW: Preparing for libX11 1.7.0

[Keith Packard]

Walter Harms  writes:

> before the actions start,
> i would like to point out that Vittorio Zecca reported a
> use-after-free in LibX11. It is reproduceable and it is found with
> libtk.

Do you have any references to a bug report about this?

-- 
-keith


signature.asc
Description: PGP signature
___
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: Preparing for libX11 1.7.0

[Keith Packard]

Matthieu Herrb  writes:

> Since a new API was added, the shared lib version number in
> src/Makefie.am probably needs to be adjusted too.

Good point. I think we want 6:4:0 then? No changed or removed APIs, just
new ones, like this?

diff --git a/src/Makefile.am b/src/Makefile.am
index 7430bf41..634b75f9 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -364,7 +364,7 @@ if XKB
 USE_XKB_LIBS = $(XKB_LIBS)
 endif
 
-libX11_la_LDFLAGS = -version-number 6:3:0 -no-undefined
+libX11_la_LDFLAGS = -version-number 6:4:0 -no-undefined
 
 libX11_la_LIBADD = \
$(LTLIBOBJS) \

-- 
-keith


signature.asc
Description: PGP signature
___
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: Fwd: The importance of mutual authentication: Local Privilege Escalation in X11

[Keith Packard]

Alan Coopersmith  writes:

> Since this is now public, we can open up the discussion of how to fix it in
> public as well, and hope we can make more progress than the security list
> did during the embargo phase.

I've got a proposed fix for this issue in two merge requests, one for
xcb and the other for the X server:

https://gitlab.freedesktop.org/xorg/lib/libxcb/-/merge_requests/10

https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/546

These two changes enables code used on Mac OS X for all other platforms.
This code allows the X listen socket to be placed anywhere in the file
system. Systems which currently place that in /tmp are vulnerable to the
bug reported above. Placing this listen socket in a protected location
should prevent un-privileged applications from spoofing the X server for
the user.

Patches for ssh will be needed to close the security issue when
forwarding X connections through that.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Preparing for libX11 1.7.0

[Keith Packard]

Alan Coopersmith  writes:

> Since https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/15
> was merged, I was thinking the next release should be 1.7.0 since it has
> a new API people may want to check for with pkg-config version checks,
> but yes, a new release once things finish getting merged seems like a
> good idea.

Thanks for thinking about what the version number should be. I've
updated the subject line accordingly.

-- 
-keith


signature.asc
Description: PGP signature
___
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

Preparing for libX11 1.6.13

[Keith Packard]

Xlib has seen some useful changes get merged since 1.6.12 and I'm
wondering if others agree that we might want to do a 1.6.13 in the near
future.h?

I'd like to do a release in the next week or so, mostly to avoid having
the charge from recent activity leak away.

We've got one more important locking/threading fix which hasn't been
merged:

https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/53

This MR is a slight variant on another MR which addresses the same issue:

https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/34

At this point, the only thing I feel blocks these MRs is figuring out
how to assign credit appropriately (!34 represents the bulk of the work,
!53 makes the patch cleaner and easier to reason about for me, at least).

Aside from that, there's this MR:

https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/24

This adds some compose key entries for a new French keyboard layout. I
don't feel competent attempting to review that as it appears that it may
cause problems for existing users. So I'd suggest leaving that pending
until it has been carefully reviewed.

Here's the issues that I think are fixed in current master:

https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/121
_XReply deadlock

https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/95
Hang in XIQueryDevice -> _XReply

https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/93
Hanging on recursive _XReply() invocation

https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/25
Deadlock in _XReply when recursing through _XSeqSyncFunction

Here's the issues that I think will be resolved by a release that
includes MR !53 above:

https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/26

And here are links to more bugs that may be related to this:

https://gitlab.gnome.org/GNOME/gtk/-/issues/2767
https://github.com/geany/geany/issues/1962
https://bugs.launchpad.net/ubuntu/+source/pcmanfm/+bug/1782984
https://bugs.launchpad.net/ubuntu/+source/gtk+2.0/+bug/1808710

-- 
-keith


signature.asc
Description: PGP signature
___
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: xserver release process

[Keith Packard]

Adam Jackson  writes:

> In short, releases need to happen, and we have CI, so let's just pop a
> release out on scheduled dates assuming CI passes.

WFM

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] modesetting: typo in drmmode_display.c -- ',' instead of '; ' at end of line

[Keith Packard]

Keith Packard  writes:

> This seems like a simple typo to me; thanks to C it isn't caught by
> the compiler.
>
> Signed-off-by: Keith Packard 

Sorry, I didn't find this myself. It was found by Roman Gilg, who should
be listed as the author.

> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index cb48aa46b..aee374821 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -1868,7 +1868,7 @@ drmmode_shadow_create(xf86CrtcPtr crtc, void *data, int 
> width, int height)
>  }
>  
>  pPixData = drmmode_bo_map(drmmode, _crtc->rotate_bo);
> -rotate_pitch = drmmode_bo_get_pitch(_crtc->rotate_bo),
> +rotate_pitch = drmmode_bo_get_pitch(_crtc->rotate_bo);
>  
>  rotate_pixmap = drmmode_create_pixmap_header(scrn->pScreen,
>   width, height,
> -- 
> 2.20.1
>

-- 
-keith


signature.asc
Description: PGP signature
___
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

[PATCH xserver] modesetting: typo in drmmode_display.c -- ',' instead of '; ' at end of line

[Keith Packard]

This seems like a simple typo to me; thanks to C it isn't caught by
the compiler.

Signed-off-by: Keith Packard 
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index cb48aa46b..aee374821 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -1868,7 +1868,7 @@ drmmode_shadow_create(xf86CrtcPtr crtc, void *data, int 
width, int height)
 }
 
 pPixData = drmmode_bo_map(drmmode, _crtc->rotate_bo);
-rotate_pitch = drmmode_bo_get_pitch(_crtc->rotate_bo),
+rotate_pitch = drmmode_bo_get_pitch(_crtc->rotate_bo);
 
 rotate_pixmap = drmmode_create_pixmap_header(scrn->pScreen,
  width, height,
-- 
2.20.1

___
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] xfree86/modes: Add "NoOutputInitialSize" option

[Keith Packard]

Andy Ritger via xorg-devel  writes:

> Normally, the X server infers the initial screen size based on any
> connected outputs.  However, if no outputs are connected, the X server
> picks a default screen size of 1024 x 768.  This option overrides the
> default screen size to use when no outputs are connected.  In contrast
> to the "Virtual" Display SubSection entry, which applies unconditionally,
> "NoOutputInitialSize" is only used if no outputs are detected when the
> X server starts.
>
> Parse this option in the new exported helper function
> xf86AssignNoOutputInitialSize(), so that other XFree86 loadable drivers
> can use it, even if they don't use xf86InitialConfiguration().
>
> Signed-off-by: Andy Ritger 

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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: [Xcb] Migrating XCB to GitLab

[Keith Packard]

Daniel Stone  writes:

> I've done this now, including having moved the bugs over. \o/

Thanks much!

-- 
-keith


signature.asc
Description: PGP signature
___
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: [ANNOUNCE] X Server 1.19.7 maintenance release plan

[Keith Packard]

"Kevin Brace"  writes:

> Assuming everything goes right, I expect an official maintenance
> release around end of February 2019 to early March 2019.

Sounds good to me; thanks for stepping in. I'd be happy to help out, let
me know what I can do.

-- 
-keith


signature.asc
Description: PGP signature
___
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 app/xcursorgen v3] Update README for gitlab migration

[Keith Packard]

Alan Coopersmith  writes:

> On 11/13/18 05:13 PM, Ray Strode wrote:
>> hi,
>> 
>> On Tue, Nov 13, 2018, 7:57 PM Alan Coopersmith >  wrote:
>> 
>> Anyone have a preference?   Shipping README.md seems easier - will it 
>> cause
>> problems for any distros or packagers?
>> 
>> i think README.md is the right way to go.  presumably any packaging 
>> challenges 
>> have been ironed out by now considering how widespread gitlab/github is now.
>
> Since Keith objected to git committing symlinks, I'll go ahead with

Yeah, it just seems like a bad idea somehow...

>   EXTRA_DIST += README.md
> when checking these in and leave anything fancier for someone else to
> tackle in the future.

Sounds like a fine plan to me. Thanks for taking care of this.

-- 
-keith


signature.asc
Description: PGP signature
___
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 app/xcursorgen v3] Update README for gitlab migration

[Keith Packard]

Alan Coopersmith  writes:

> 3rd choice - this seems to work for me, shipping a file named README in the
> tarball without any Makefile modifications:

Yikes! Scary git-fu!

Maybe we can see if cmark-gfm is available? That's the github fork that
parses more useful common mark syntax files, which should be starting to
show up in distros (it's in Debian at least).

$ cmark-gfm --to plaintext README.md > README

if we actually want to generate a plaintext README file? Until then,
just copy it?

-- 
-keith


signature.asc
Description: PGP signature
___
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: DRM leases + X = SW only OpenGL acceleration on child X server

[Keith Packard]

Raimonds Cicans  writes:

> Some questions:
> 1) Which video drivers support DRM leases? Only amdgpu?

Both amdgpu and modesetting.

> 2) Which video driver must load child X server? modesetting? Same as
> master X server?

Only modesetting appears to use xf86DRMMasterFd at this point; adding
that to amdgpu should be straightforward though.

-- 
-keith


signature.asc
Description: PGP signature
___
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: DRM leases + X = SW only OpenGL acceleration on child X server

[Keith Packard]

Michel Dänzer  writes:

> Looks like drmAuthMagic returns an error for the leased FD in the Xorg
> process.
>
> Keith, was this working for you?

At one point, I'm nearly certain it was, but I haven't messed with this
in a while.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Sharing a KMS device

[Keith Packard]

Pekka Paalanen  writes:

> Hi Keith,
>
> sorry to bump in, but do you mean that a lessee doing drmDropMaster()
> will both succeed and drop master also for the lessor (and recursively
> through the whole leasing tree up and down as a consequence)?

If that call weren't disabled for lessees, then yes. And, that's why 
patch 761e05a702f5d537ffcca1ba933f9f0a968aa022 was added...

-- 
-keith


signature.asc
Description: PGP signature
___
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: Sharing a KMS device

[Keith Packard]

Troll Berserker  writes:

> Hmm. I see that drmDropMaster became conditional:
>
>  if (!ms->fd_passed)
>  drmDropMaster(ms->fd);

That's actually a fix which is necessary so that the lessee doesn't give
away control of the whole device.

> I was thinking that may be it is possible to implement a CUSE driver
> to expose leased descriptors as /dev/dri/card* devices so that it
> would be possible to run X server unmodified.

I suppose it could, but I'm not sure why you'd want to?

> But now I see that these leased FDs are somehow special so you can't perform 
> drmDropMaster on them.
> Why?

'drm master' is the overall controller of the whole device; when that is
dropped, all of the related lessees and lessor no longer have access to
the device.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Sharing a KMS device

[Keith Packard]

Troll Berserker  writes:

> Cool. In your talk you even mentions multiseating.
> AFAIU with this technique one doesn't get multiple /dev/dri/card* for
> a GPU out of the box, so it is not possible to use unmodified
> modesetting driver to run multiple X servers.

Correct. I have posted X server patches to pass the DRM file descriptor
in from an external program, and a sample program which creates a lease
for that purpose.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Sharing a KMS device

[Keith Packard]

Troll Berserker  writes:

> Is it theoretically possible to implement a driver (let's call it
> shared-modesetting) which will offload mode setting to a helper daemon
> (drmMaster) thus allowing to run multiple Xorg servers on one DRM
> device? (provided that there are enough CRTCs (?) available).

We've recently add a new technique in DRM called 'leasing', which allows
you to create a new DRM controller for a subset of the available resources.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Window scaling (aka owner sizes)

[Keith Packard]

Olivier Fourdan  writes:

> How do you plan to deal with this in your approach?

Well, that's why I'm trying to get some help here...

What we're trying to do here is replace some really ugly hacks that
Valve has made outside of the server to get applications that depend on
changing the video mode to make the application look reasonable. As
changing video modes isn't a good idea in 2018, Valve has written a
compositing manager which scales applications for output (which only
works for scaling up), and then does pretty ugly things to get mouse
input 'mostly working' -- grabbing the mouse and generating synthetic
events for the application. Yikes! Oh, and it captures attempts to set
video modes.

The proposed hacks are already *way* better than that and make these
applications work much better. There's still more to do, and I'll be
working on those bits too (like redirecting video mode settings
somehow).

The root window size shouldn't be all that interesting to applications;
they really need to know the monitor sizes and positions, and that
information is available via RandR and Xinerama. So, I'm not all that
concerned about root window size; applications relying on that are
already broken.

Ok, so to make this 'actually' work, I might have to scale not only the
window size, but the window position as well -- and then I might
actually have to scale *all* of the window sizes and positions for the
specified application.

Or, maybe we create a synthetic monitor and then pretend to applications
that their windows appear there instead.

> PS: When considering this for Xwayland, I ended up thinking that it
> would be actually easier to have different X11 screens (i.e. :0.0,
> :0.1, etc.) of different size for different scales for the X11 clients
> ended up being scaled differently (which led me to
> https://gitlab.freedesktop.org/ofourdan/xserver/tree/xwayland-multi-screen),
> but that approach introduces a lot of complexity (the CM has to play
> the proxy between all those X11 screens for things like copy/paste,
> DnD, etc.)

Yeah, X screens are probably not what you want. An X monitor might be
reasonable; those are visible in RandR and Xinerama, but leave
applications on the same screen.

I didn't want to change the owner position of the windows, but I now
wonder if that might not turn out ok?

-- 
-keith


signature.asc
Description: PGP signature
___
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: Window scaling (aka owner sizes)

[Keith Packard]

Giuseppe Bilotta  writes:

> By drawing “outside” of their window I don't mean using the window's
> own GC context —that would actually work correctly, since it's the one
> that gets scaled— but rather using a (grand)parent window such as the
> root or vroot window(s).

If they did that without IncludeInferiors, then they will be clipped
away from the current size of the window, unless you're using Manual
compositing, in which case of course you can paint all over that area.

> More in general, there could be issues with clients that interact with
> the geometry of other windows (the only examples I can think of are
> toys like AMOR or xsnow though, not sure if there's any “serious”
> program that could be affected by this).

Always possible, but my goal is to make this work like the 'normal' size
outside of the window and the 'owner' size within.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Window scaling (aka owner sizes)

[Keith Packard]

Daniel Stone  writes:

> It's the other way around. Wayland only has surface-relative
> co-ordinates, so we take those and then translate them back into the
> X11 global co-ordinate space.

Oh, so if we know how XWayland is scaling the output to the screen, then
we can translate the coordinates back to the correct root
coordinates. That seems like it might be useful in supporting per-window
scaling.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Window scaling (aka owner sizes)

[Keith Packard]

Robert Mader  writes:

> I can post my WIP work (will have to clean it up and rebase to current
> master).
> That said, I'm very interested to see this go forward and am very
> willing to help :)

I don't know how device coordinates are handled in XWayland, but this
mechanism *might* help if you pass screen event coordinates down to X
for it to then convert into window coordinates?

There are still lots of details to sort out to see if this might
work. In particular, applications often need to do screen-relative
layout of UI elements, such as menus. And many UI elements appear as
separate top-level windows, which would presumably also need scaling to
be usable?

-- 
-keith


signature.asc
Description: PGP signature
___
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: Window scaling (aka owner sizes)

[Keith Packard]

Giuseppe Bilotta  writes:

> Wonderful endeavor.

Thanks!

> I guess the only way in which this can fail is if a client tries to
> draw its own decorations (or something) “outside” of their own window,
> assuming that origin+width or origin+height actually brings them there
> in the root context (I mean as opposed to using XTranslateCoords,
> which can be handled from within the server).

They won't be able to draw 'outside' of their window -- all drawing to
the window is redirected to the composite pixmap and clipped to the
owner size. For applications with a border, the border also gets drawn
to the composite pixmap and then scaled to the current size, although
the border width is the same, so a tiled border may not look right.

> This sounds like an excellent solution to improve Hi- and most
> importantly mixed-DPI setups. I'm a bit swamped for the next month,
> but after that I can at the very least help test and debug the code,
> since I have multiple such setups. One thing that would definitely be
> useful for the mixed DPI case is some way to more or less
> automatically change the scaling factor based on window location (or
> at least Monitor it belongs to), at least in the case where the X
> server itself is doing the compositing.

Cool. As with any similar change, getting the details right is the hard
part, and that means experimenting in many environments.

-- 
-keith


signature.asc
Description: PGP signature
___
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

Window scaling (aka owner sizes)

["Keith Packard"]

I'm doing a bit of work to help support applications that want to run at
a small fixed size. The 'usual' way to make them visible on our modern
desktops is to flip video modes, but that means everything runs at low
resolution, plus you get the adventure of switching modes, which can
take a long time in complex monitor environments.

https://keithp.com/blogs/window-scaling/

The basic plan is to make the size-as-seen-by-the-owner different from
the size seen by the rest of the system, and then to either have the
server scale the output or get the compositing manager to do it.

This solves one of the main use cases for 'composite redirection' that I
talked about a long time ago and could never get to work. Restricting
the problem space to simple application scaling, instead of arbitrary
transformation, meant that I could place the input event scaling right
inside the X server, allowing it to be synchronous, instead of
attempting to have some external agent perform the transformation.

When there's no compositing manager running, the window is placed into
automatic compositing mode and the X server performs the necessary
output scaling.

When there is a compositing manager running, I've thought of three
possible ways to make it work:

 1. The compositing manager can be extended to handle the scaling
operation. This seems like the best plan; just a single copy from
the native application size to the scanout buffer.

 2. Alternatively, another window might be interposed between the scaled
client and the compositor to provide a place for the X server to do
the scaling; although that would naturally introduce a bunch more
expense and delay in the process.

 3. It would also be possible to have the X server provide a scaled
version of the window pixmap to the compositing manager until it
advertised support for built-in scaling. Same rendering delay as
with an interposed window, but no changes required in the
environment, although a bit of work to be done in the X server. That
might be a good transition plan so that things didn't break when you
started a compositing manager without the required support.

I'm wondering if there are others who would find this useful, and if
some of them might help get this into reasonable shape. Suggestions are
welcome!

-- 
-keith


signature.asc
Description: PGP signature
___
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: [PULL xserver] Remove old colormap special cases from xfree86

[Keith Packard]

Adam Jackson  writes:

> This makes xf86 colormap setup not as weird as other DDXes, and removes
> matching special-casing from the devPrivate system. The trident and
> xgixp drivers would be affected by this, losing the ability to set the
> overscan border color. Either user is encouraged to speak up.
>
> The following changes since commit 1fc20b985cc888345bc8c6fce7b43f10ce71fe43:
>
>   meson: Add detection of libsystemd-daemon. (2018-08-09 13:42:54 -0400)
>
> are available in the Git repository at:
>
>   ssh://g...@gitlab.freedesktop.org/ajax/xserver xf86cmap
>
> for you to fetch changes up to 5e01650d190fa046a53139a4955b0ef7aedcbaad:
>
>   xfree86: Remove the rest of ->SetOverscan (2018-08-20 14:55:35
>   -0400)

For the series:

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] randr: rrCheckPixmapBounding should only increase screen size

[Keith Packard]

Alex Goins  writes:

> Instead, this change simply
> makes it so that rrCheckPixmapBounding() will only resize the fb to be larger
> than it already is, preventing it from stepping on prior requests to increase
> the size of the fb.

Seems like a fine plan to me.

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] dix: check_modmap_change() returns Success, not true

[Keith Packard]

Peter Hutterer  writes:

> Not sure what if anything calls XSetDeviceModifierMapping() but this would've
> failed all the time. check_modmap_change() returns Successs but we were
> treating it like a boolean. Fix this.

Yeah, and check_modmap_change_slave *does* return a boolean; I see no
possibility for confusion here.

I saw that check_modmap_change returns -1 for BadValue and then checked
to make sure this odd value is fixed up in both
ProcXSetDeviceModifierMapping and ProcSetModifierMapping so that this
value doesn't get back to the client.

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 7/7] composite: Implement backing store's Always mode

[Keith Packard]

Adam Jackson  writes:

> +static Bool
> +backed(WindowPtr pWin)
> +{
> +for (; pWin; pWin = pWin->parent)
> +if (pWin->backStorage)
> +return TRUE;
> +
> +return FALSE;
> +}
> +

Do we need to stop if we find another redirection layer? I think that
will break the backing store which may exist above that point?

>  Bool
>  compUnrealizeWindow(WindowPtr pWin)
>  {
> @@ -284,12 +296,36 @@ compUnrealizeWindow(WindowPtr pWin)
>  compCheckRedirect(pWin);
>  if (!(*pScreen->UnrealizeWindow) (pWin))
>  ret = FALSE;
> +
> +/* UnrealizeTree walks from root to leaves, so only need to check parent 
> */
> +if (backed(pWin) && pWin->parent->paintable)
> +pWin->paintable = TRUE;
> +

Presumably paintable has been set to false in DIX? Having composite not
integrated into DIX is starting to suck too much?

> +void
> +compWindowExposures(WindowPtr pWin, RegionPtr reg)
> +{
> +ScreenPtr pScreen = pWin->drawable.pScreen;
> +CompScreenPtr cs = GetCompScreen(pScreen);
> +
> +pScreen->WindowExposures = cs->WindowExposures;
> +
> +if (pWin->backStorage) {
> +DamageDamageRegion(>drawable, reg);
> +RegionEmpty(reg);
> +}
> +
> +pScreen->WindowExposures(pWin, reg);
> +
> +cs->WindowExposures = pScreen->WindowExposures;
> +pScreen->WindowExposures = compWindowExposures;
> +}

Why is this needed? WindowExposures should get set correctly if we're
setting the regions in miValidateTree?

> +/*
> + * Take down bs explicitly, to get ->backStorage cleared
> + */
> +if (pWin->backingStore != NotUseful) {
> +pWin->backingStore = NotUseful;
> +pScreen->ChangeWindowAttributes(pWin, CWBackingStore);
> +}
> +

This seems like it shouldn't be necessary; destroying a window with
backStorage set should be cleaned up already? Or is it only ever cleared
in unmap at this point?

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 6/7] dix: Switch window unmap to mark normally instead of UnmapValData

[Keith Packard]

Adam Jackson  writes:

> ---
>  dix/window.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/dix/window.c b/dix/window.c
> index 55290577d9..34bed93d93 100644
> --- a/dix/window.c
> +++ b/dix/window.c
> @@ -2870,7 +2870,7 @@ UnmapWindow(WindowPtr pWin, Bool fromConfigure)
>  if (SubStrSend(pWin, pParent))
>  DeliverUnmapNotify(pWin, fromConfigure);
>  if (wasPaintable && !fromConfigure) {
> -pWin->valdata = UnmapValData;
> +(*pScreen->MarkWindow) (pWin);

Hrm. This drops a pretty significant optimization on the floor; perhaps
we no longer care though?

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 5/7] dix: Update window state based on paintable not viewable

[Keith Packard]

Adam Jackson  writes:

> Signed-off-by: Adam Jackson 

Yeah, this all looks reasonable.

> +if (wasPaintable && anyMarked) {
> +if (pLayerWin->parent == pWin)
> +(*pScreen->MarkWindow) (pWin);
> +else {
> +WindowPtr ptmp;

Argh. Please make the logic fix separate from this semantic change; I
don't want to try and figure out if the following code is different by
eye.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 4/7] mi: Shortcut miDoCopy/miCopyArea based on paintable not realized

[Keith Packard]

Adam Jackson  writes:

> Signed-off-by: Adam Jackson 

Should we just be checking for empty clip lists instead? It's not quite
as cheap, but we do catch FullyObscurred in the same check.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 2/7] mi: miValidateTree based on paintable not viewable

[Keith Packard]

Adam Jackson  writes:

Reading through this, I don't see what this patch ends up changing --
TreatAsTransparent will always return True for unmapped Always windows,
so every place you're checking paintable && !TreatAsTransparent, you
could equivalently be checking viewable && !TreatAsTrasparent without
having changed TreatAsTransparent.

What needs changing is that miComputeClips needs to be called for
paintable windows, not just viewable windows; everything else seems like
it doesn't need changing at all?

> +}
> +else if (pParent->paintable)
>  newVis = VisibilityFullyObscured;
> -break;
> +else {
> +newVis = VisibilityNotViewable;
>  }

This isn't right -- unmapped windows should always be NotViewable, as
seen through the protocol.

>  pParent->visibility = newVis;
> -if (oldVis != newVis &&
> +if (pParent->realized &&
> +oldVis != newVis &&
>  ((pParent->
>eventMask | wOtherEventMasks(pParent)) & VisibilityChangeMask))
>  SendVisibilityNotify(pParent);
> @@ -293,14 +303,13 @@ miComputeClips(WindowPtr pParent,
>   (oldVis == VisibilityUnobscured))) {
>  pChild = pParent;
>  while (1) {
> -if (pChild->viewable) {
> +if (pChild->paintable) {
>  if (pChild->visibility != VisibilityFullyObscured) {
>  RegionTranslate(>borderClip, dx, dy);
>  RegionTranslate(>clipList, dx, dy);

And this is going to end up wrong -- I think we need to ignore
visibility for purposes of computing clip values here, and just check
for empty clip lists? In any case, the use of visibility to optimize
clip computations is going to serve us poorly, especially if we ever
want to fix it with composite windows.

>  for (; pChild; pChild = pChild->nextSib) {
> -if (pChild->viewable && !TreatAsTransparent(pChild))
> +if (pChild->paintable && !TreatAsTransparent(pChild))
>  RegionAppend(, >borderSize);

This should be checking viewable, in which case you don't need to change
TreatAsTransparent
>  for (pChild = pParent->lastChild; pChild; pChild = 
> pChild->prevSib) {
> -if (pChild->viewable && !TreatAsTransparent(pChild))
> +if (pChild->paintable && !TreatAsTransparent(pChild))
>  RegionAppend(, >borderSize);

Same here.

>  }
>  }
>  RegionValidate(, );
>  
>  for (pChild = pParent->firstChild; pChild; pChild = pChild->nextSib) 
> {
> -if (pChild->viewable) {
> +if (pChild->paintable) {
>  /*
> - * If the child is viewable, we want to remove its extents
> + * If the child is paintable, we want to remove its extents
>   * from the current universe, but we only re-clip it if
>   * it's been marked.

I'm not sure you need to use paintable here -- unmapped paintable
windows will never want to be removed from their parent anyways, so this
is equivalent to viewable.

>   */
> @@ -564,7 +573,7 @@ miValidateTree(WindowPtr pParent,   /* Parent to 
> validate */
>  ScreenPtr pScreen;
>  WindowPtr pWin;
>  Bool overlap;
> -int viewvals;
> +int paintables = 0;

I think viewvals is correct here; the only windows affecting their
parent clip list are those which are viewable; unmapped paintable
windows have no impact on their parents.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] Xext: dynamically allocate the PanoramiXDepths[j].vids array

[Keith Packard]

Peter Hutterer  writes:

> Control flow is:
>PanoramiXMaybeAddDepth() allocates an array size 240 (pDepth->numVisuals)
>PanoramiXMaybeAddVisual() finds up to 270 matches (pScreen->numVisuals)
>and writes those into the previously allocated array.
>
> This caused invalid reads/writes followed by eventually a double-free abort.
>
> Reproduced with xorg-integration-tests server test
> XineramaTest.ScreenCrossing/* (and a bunch of others).
>
> Signed-off-by: Peter Hutterer 

Reviewed-by: Keith Packard 

(I'd complain about the lack of NULL check, but the original code didn't
bother either)

-- 
-keith


signature.asc
Description: PGP signature
___
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 xf86-video-amdgpu 0/3] Add RandR lease support [v2]

[Keith Packard]

Michel Dänzer  writes:

> On 2018-07-07 02:36 AM, Keith Packard wrote:

> Well, this is unfortunate timing. Last Friday, I finally got around to
> porting these myself, as I had promised I would, but ran out of time for
> sending them out before the weekend. Sent out now:

Not a problem at all -- it gave me a chance to review the code again
which made it easier to read over your version.

> As my patches are simpler (due to e.g. taking advantage of existing
> XF86_* defines), I'm inclined to go with them. Sorry for any time you've
> wasted. If you could review my patches and maybe test that the lease
> functionality is actually working, that would be much appreciated.

All done. Thanks for getting these ready for merging. It was pretty
funny that we both decided to work on this on the same day after letting
them sit for a few months.

-- 
-keith


signature.asc
Description: PGP signature
___
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

[PATCH xf86-video-amdgpu 3/3] Add RandR leases with modesetting driver support [v7]

[Keith Packard]

This adds support for RandR CRTC/Output leases through the modesetting
driver, creating a lease using new kernel infrastructure and returning
that to a client through an fd which will have access to only those
resources.

v2: Restore CRTC mode when leases terminate

When a lease terminates for a crtc we have saved data for, go
ahead and restore the saved mode.

v3: Report RR_Rotate_0 rotations for leased crtcs.

Ignore leased CRTCs when selecting screen size.

Stop leasing encoders, the kernel doesn't do that anymore.

Turn off crtc->enabled while leased so that modesetting
ignores them.

Check lease status before calling any driver mode functions

When starting a lease, mark leased CRTCs as disabled and hide
their cursors. Also, check to see if there are other
non-leased CRTCs which are driving leased Outputs and mark
them as disabled as well. Sometimes an application will lease
an idle crtc instead of the one already associated with the
leased output.

When terminating a lease, reset any CRTCs which are driving
outputs that are no longer leased so that they start working
again.

This required splitting the DIX level lease termination code
into two pieces, one to remove the lease from the system
(RRLeaseTerminated) and a new function that frees the lease
data structure (RRLeaseFree).

v4: Report RR_Rotate_0 rotation for leased crtcs.

v5: Terminate all leases on server reset.

Leases hang around after the associated client exits so that
the client doesn't need to occupy an X server client slot and
consume a file descriptor once it has gotten the output
resources necessary.

Any leases still hanging around when the X server resets or
shuts down need to be cleaned up by calling the kernel to
terminate the lease and freeing any DIX structures.

Note that we cannot simply use the existing
drmmode_terminate_lease function on each lease as that wants
to also reset the video mode, and during server shut down that

   modesetting: Validate leases on VT enter

The kernel doesn't allow any master ioctls to run when another
VT is active, including simple things like listing the active
leases. To deal with that, we check the list of leases
whenever the X server VT is activated.

   xfree86: hide disabled cursors when resetting after lease termination

The lessee may well have played with cursors and left one
active on our screen. Just tell the kernel to turn it off.

v6: Add meson build infrastructure

v7:
Make it build on X server 1.13
Remove drmmode_terminate_leases

Signed-off-by: Keith Packard 
Reviewed-by: Adam Jackson 
---
 configure.ac  |  18 ++
 src/drmmode_display.c | 137 +-
 src/drmmode_display.h |   5 ++
 3 files changed, 159 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 378b5aa..d56e519 100644
--- a/configure.ac
+++ b/configure.ac
@@ -54,6 +54,14 @@ if test "x$GCC" = "xyes"; then
CPPFLAGS="$CPPFLAGS -Wall"
 fi
 
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
+#include 
+#ifndef __GLIBC__
+#error not glibc
+#endif
+]], [])], [AC_DEFINE(_GNU_SOURCE, 1,
+   [ Enable GNU and other extensions to the C environment for glibc])])
+
 AH_TOP([#include "xorg-server.h"])
 
 # Define a configure option for an alternate module directory
@@ -170,6 +178,12 @@ AC_CHECK_DECL(RROutputSetNonDesktop,
  [#include 
   #include ])
 
+AC_CHECK_DECL(RRDeliverLeaseEvent,
+ [AC_DEFINE(HAVE_RRLEASE, 1,
+ [Have RandR lease support API])], [],
+ [#include 
+  #include ])
+
 AC_CHECK_DECL(fbGlyphs,
  [AC_DEFINE(HAVE_FBGLYPHS, 1, [Have fbGlyphs API])], [],
  [#include 
@@ -209,6 +223,8 @@ AC_CHECK_HEADERS([dri3.h], [], [],
 
 CPPFLAGS="$SAVE_CPPFLAGS"
 
+AC_CHECK_LIB([bsd], [arc4random_buf])
+
 # Checks for headers/macros for byte swapping
 # Known variants:
 #   bswap_16, bswap_32, bswap_64  (glibc)
@@ -219,6 +235,8 @@ CPPFLAGS="$SAVE_CPPFLAGS"
 # if  is found, assume it's the correct version
 AC_CHECK_HEADERS([byteswap.h])
 
+AC_REPLACE_FUNCS([reallocarray])
+
 # if  is found, have to check which version
 AC_CHECK_HEADER([sys/endian.h], [HAVE_SYS_ENDIAN_H="yes"], 
[HAVE_SYS_ENDIAN_H="no"])
 
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 914086d..84a355c 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2934,8 +2934,138 @@ fail:
return FALSE;
 }
 
+#ifdef HAVE_RRLEASE
+
+static void
+drmmode_validate_leases(ScrnInfoPtr scrn)
+{
+   ScreenPtr screen = scrn->pScreen;
+   rrScrPrivPtr

[PATCH xf86-video-amdgpu 1/3] xf86-video-modesetting: Record non-desktop kernel property at PreInit time [v2]

[Keith Packard]

Save any value of the kernel non-desktop property in the xf86Output
structure to avoid non-desktop outputs in the default configuration.

v2: Support X server 1.13

Signed-off-by: Keith Packard 
Reviewed-by: Adam Jackson 
---
 configure.ac  |  6 ++
 src/drmmode_display.c | 15 ++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 2593f52..378b5aa 100644
--- a/configure.ac
+++ b/configure.ac
@@ -164,6 +164,12 @@ AC_CHECK_DECL(RegionDuplicate,
  [#include 
   #include ])
 
+AC_CHECK_DECL(RROutputSetNonDesktop,
+ [AC_DEFINE(HAVE_NONDESKTOP, 1,
+ [Have non-desktop property support API])], [],
+ [#include 
+  #include ])
+
 AC_CHECK_DECL(fbGlyphs,
  [AC_DEFINE(HAVE_FBGLYPHS, 1, [Have fbGlyphs API])], [],
  [#include 
diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 9364a88..92cb947 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2597,6 +2597,9 @@ drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_r
drmModePropertyBlobPtr path_blob = NULL;
char name[32];
int i;
+#ifdef HAVE_NONDESKTOP
+   Bool nonDesktop = FALSE;
+#endif
const char *s;
 
koutput =
@@ -2606,6 +2609,11 @@ drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_r
return 0;
 
path_blob = koutput_get_prop_blob(pAMDGPUEnt->fd, koutput, "PATH");
+#ifdef HAVE_NONDESKTOP
+   i = koutput_get_prop_idx(pAMDGPUEnt->fd, koutput, DRM_MODE_PROP_RANGE, 
RR_PROPERTY_NON_DESKTOP);
+   if (i >= 0)
+   nonDesktop = koutput->prop_values[i] != 0;
+#endif
 
kencoders = calloc(sizeof(drmModeEncoderPtr), koutput->count_encoders);
if (!kencoders) {
@@ -2638,6 +2646,9 @@ drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_r
drmmode_output = output->driver_private;
drmmode_output->output_id = mode_res->connectors[num];
drmmode_output->mode_output = koutput;
+#ifdef HAVE_NONDESKTOP
+   output->non_desktop = nonDesktop;
+#endif
for (i = 0; i < koutput->count_encoders; i++) {
drmModeFreeEncoder(kencoders[i]);
}
@@ -2681,7 +2692,9 @@ drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_r
output->interlaceAllowed = TRUE;
output->doubleScanAllowed = TRUE;
output->driver_private = drmmode_output;
-
+#ifdef HAVE_NONDESKTOP
+   output->non_desktop = nonDesktop;
+#endif
output->possible_crtcs = 0x;
for (i = 0; i < koutput->count_encoders; i++) {
output->possible_crtcs &= kencoders[i]->possible_crtcs;
-- 
2.17.1

___
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

[PATCH xf86-video-amdgpu 0/3] Add RandR lease support [v2]

[Keith Packard]

When we last saw these patches, they required current X server bits to build,
while the project requires that the driver build back to X server 1.13. I've
added suitable checks and wrapped the new code in #ifdefs now.

If I don't hear any complaints in a few days (given that we're at the boundary
of a weekend), I'll go ahead and push these.

-keith


___
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

[PATCH xf86-video-amdgpu 2/3] xf86-video-modesetting: Create CONNECTOR_ID properties for outputs [v2]

[Keith Packard]

This lets a DRM client map between X outputs and kernel connectors.

v2:
Change CONNECTOR_ID to enum -- Adam Jackson 

Signed-off-by: Keith Packard 
Reviewed-by: Adam Jackson 
---
 src/drmmode_display.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 92cb947..914086d 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2229,6 +2229,29 @@ static void 
drmmode_output_create_resources(xf86OutputPtr output)
drmmode_output->tear_free = info->tear_free;
drmmode_output->num_props++;
 
+   /* Create CONNECTOR_ID property */
+   {
+   Atomname = MakeAtom("CONNECTOR_ID", 12, TRUE);
+   INT32   value = mode_output->connector_id;
+
+   if (name != BAD_RESOURCE) {
+   err = RRConfigureOutputProperty(output->randr_output, 
name,
+   FALSE, FALSE, TRUE,
+   1, );
+   if (err != 0) {
+   xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+  "RRConfigureOutputProperty error, 
%d\n", err);
+   }
+   err = RRChangeOutputProperty(output->randr_output, name,
+XA_INTEGER, 32, 
PropModeReplace, 1,
+, FALSE, FALSE);
+   if (err != 0) {
+   xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+  "RRChangeOutputProperty error, 
%d\n", err);
+   }
+   }
+   }
+
for (i = 0; i < drmmode_output->num_props; i++) {
drmmode_prop_ptr p = _output->props[i];
drmmode_prop = p->mode_prop;
-- 
2.17.1

___
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: gitlab migration

[Keith Packard]

Adam Jackson  writes:

> I don't really see a reason not to move the repos for everything. It's
> a pretty invisible change since the old URLs keep working, and I'm
> itchy to see CI wired up.

Sounds good to me; I was trying to not over-load poor Daniel. If he's up
for it, or can use help from us, let's do it.

-- 
-keith


signature.asc
Description: PGP signature
___
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: gitlab migration

[Keith Packard]

Adam Jackson  writes:

> I'd like us to start moving repos and bug tracking into gitlab.

I would also like to get to a merge-request model at some
point. However, I think we can take this in stages and start by moving
the git repos over to gitlab, and then move the bugs over, and finally
start doing patch review via the gitlab issue tracker.

For the first step, I'd like to propose moving the x server git
repository to gitlab in the coming week, with the switch-over happening
on the morning of July 9.

Daniel has already created accounts for everyone in the xorg group, but
I believe there is a process for each user to authenticate via email and
upload ssh keys. You can also set up two-factor authentication, which
seems like a good idea to me.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] modesetting: Allow a DRM fd to be passed on command line with -masterfd

[Keith Packard]

Lyude Paul  writes:

> Looks good! One nitpick I'm not 100% sure about:

>> +#define CHECK_FOR_REQUIRED_ARGUMENT() \
>> +if (((i + 1) >= argc) || (!argv[i + 1])) {  
>> \
>> +  ErrorF("Required argument to %s not specified\n", argv[i]);   \
>> +  UseMsg(); \
>> +  FatalError("Required argument to %s not specified\n", argv[i]);   
> Is the double printing of "Required argument to %s not specified" here
> intentional?

I copied CHECK_FOR_REQUIRED_ARGUMENT from xf86Init.c where it does the
same thing. I assume this is intended to make sure the user understands
what error caused the server to exit -- you can see it both before and
after the long usage message.

>> +if (!xf86PrivsElevated())
>> +ErrorF("-masterfd  use the specified fd as the DRM
>> master fd\n");
> I think it would be a better idea for us to show this argument description
> unconditionally, along with adding a note about setuid/setgid not being
> allowed with it

Sounds good. 

Here's an updated patch with the usage message change suggested. Thanks
for reviewing!

From f122451b7f5be985036cae29df7126a7f25cc891 Mon Sep 17 00:00:00 2001
From: Keith Packard 
Date: Thu, 18 Jan 2018 18:07:29 -0800
Subject: [PATCH xserver] modesetting: Allow a DRM fd to be passed on command
 line with -masterfd [v2]

This lets an application open a suitable DRM device and pass the file
descriptor to the mode setting driver through an X server command line
option, '-masterfd'.

There's a companion application, xlease, which creates a DRM master by
leasing an output from another X server. That is available at

	git clone git://people.freedesktop.org/~keithp/xlease

v2:
	Always print usage, but note that it can't be used if
	setuid/gid

	Suggested-by: Lyude Paul 

Signed-off-by: Keith Packard 
---
 hw/xfree86/common/xf86Globals.c |  2 ++
 hw/xfree86/common/xf86Priv.h|  1 +
 hw/xfree86/drivers/modesetting/driver.c | 26 -
 hw/xfree86/drivers/modesetting/driver.h |  1 +
 hw/xfree86/os-support/linux/lnx_init.c  | 21 
 5 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/hw/xfree86/common/xf86Globals.c b/hw/xfree86/common/xf86Globals.c
index e890f05c2..7cc7401a2 100644
--- a/hw/xfree86/common/xf86Globals.c
+++ b/hw/xfree86/common/xf86Globals.c
@@ -53,6 +53,8 @@ DevPrivateKeyRec xf86ScreenKeyRec;
 ScrnInfoPtr *xf86Screens = NULL;/* List of ScrnInfos */
 ScrnInfoPtr *xf86GPUScreens = NULL;/* List of ScrnInfos */
 
+int xf86DRMMasterFd = -1;  /* Command line argument for DRM master file descriptor */
+
 const unsigned char byte_reversed[256] = {
 0x00, 0x80, 0x40, 0xc0, 0x20, 0xa0, 0x60, 0xe0,
 0x10, 0x90, 0x50, 0xd0, 0x30, 0xb0, 0x70, 0xf0,
diff --git a/hw/xfree86/common/xf86Priv.h b/hw/xfree86/common/xf86Priv.h
index 4fe2b5f33..393af3900 100644
--- a/hw/xfree86/common/xf86Priv.h
+++ b/hw/xfree86/common/xf86Priv.h
@@ -93,6 +93,7 @@ extern _X_EXPORT int xf86LogVerbose;/* log file verbosity level */
 
 extern ScrnInfoPtr *xf86GPUScreens;  /* List of pointers to ScrnInfoRecs */
 extern int xf86NumGPUScreens;
+extern _X_EXPORT int xf86DRMMasterFd;  /* Command line argument for DRM master file descriptor */
 #ifndef DEFAULT_VERBOSE
 #define DEFAULT_VERBOSE		0
 #endif
diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
index 306541f33..6f4637254 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include "xf86.h"
+#include "xf86Priv.h"
 #include "xf86_OSproc.h"
 #include "compiler.h"
 #include "xf86Pci.h"
@@ -194,11 +195,24 @@ modesettingEntPtr ms_ent_priv(ScrnInfoPtr scrn)
 return pPriv->ptr;
 }
 
+static int
+get_passed_fd(void)
+{
+if (xf86DRMMasterFd >= 0) {
+xf86DrvMsg(-1, X_INFO, "Using passed DRM master file descriptor %d\n", xf86DRMMasterFd);
+return dup(xf86DRMMasterFd);
+}
+return -1;
+}
+
 static int
 open_hw(const char *dev)
 {
 int fd;
 
+if ((fd = get_passed_fd()) != -1)
+return fd;
+
 if (dev)
 fd = open(dev, O_RDWR | O_CLOEXEC, 0);
 else {
@@ -818,6 +832,12 @@ ms_get_drm_master_fd(ScrnInfoPtr pScrn)
 return TRUE;
 }
 
+ms->fd_passed = FALSE;
+if ((ms->fd = get_passed_fd()) >= 0) {
+ms->fd_passed = TRUE;
+return TRUE;
+}
+
 #ifdef XSERVER_PLATFORM_BUS
 if (pEnt->location.type == BUS_PLATFORM) {
 #ifdef XF86_PDEV_SERVER_FD
@@ -1502,6 +1522,9 @@ SetMaster(ScrnInfoPtr pScrn)
 return TRUE;
 #endif
 
+if (ms->fd_passed)
+return TRUE;
+
 ret = drmSetMaster(ms->fd);
 if (ret)
 

[PATCH xserver 2/2] xfree86: Wrap RRCrtcIsLeased and RROutputIsLeased to check for DIX structures

[Keith Packard]

Before DIX structures are allocated for crtcs and outputs, we don't
want to call DIX randr code with NULL pointers. This can happen if the
driver sets video modes early in server initialization, which Nouveau
does in zaphod mode.

Cc: thellst...@vmware.com
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=106772
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=106960
Signed-off-by: Keith Packard 
---
 hw/xfree86/modes/xf86Crtc.c | 42 ++---
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index 142ab1ebe..37a45bb3a 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -174,6 +174,32 @@ xf86CrtcInUse(xf86CrtcPtr crtc)
 return FALSE;
 }
 
+/**
+ * Return whether the crtc is leased by a client
+ */
+
+static Bool
+xf86CrtcIsLeased(xf86CrtcPtr crtc)
+{
+/* If the DIX structure hasn't been created, it can't have been leased */
+if (!crtc->randr_crtc)
+return FALSE;
+return RRCrtcIsLeased(crtc->randr_crtc);
+}
+
+/**
+ * Return whether the output is leased by a client
+ */
+
+static Bool
+xf86OutputIsLeased(xf86OutputPtr output)
+{
+/* If the DIX structure hasn't been created, it can't have been leased */
+if (!output->randr_output)
+return FALSE;
+return RROutputIsLeased(output->randr_output);
+}
+
 void
 xf86CrtcSetScreenSubpixelOrder(ScreenPtr pScreen)
 {
@@ -254,7 +280,7 @@ xf86CrtcSetModeTransform(xf86CrtcPtr crtc, DisplayModePtr 
mode,
 RRTransformRec saved_transform;
 Bool saved_transform_present;
 
-crtc->enabled = xf86CrtcInUse(crtc) && !RRCrtcIsLeased(crtc->randr_crtc);;
+crtc->enabled = xf86CrtcInUse(crtc) && !xf86CrtcIsLeased(crtc);
 
 /* We only hit this if someone explicitly sends a "disabled" modeset. */
 if (!crtc->enabled) {
@@ -412,7 +438,7 @@ xf86CrtcSetOrigin(xf86CrtcPtr crtc, int x, int y)
 crtc->x = x;
 crtc->y = y;
 
-if (RRCrtcIsLeased(crtc->randr_crtc))
+if (xf86CrtcIsLeased(crtc))
 return;
 
 if (crtc->funcs->set_origin) {
@@ -2662,7 +2688,7 @@ xf86InitialConfiguration(ScrnInfoPtr scrn, Bool canGrow)
 static void
 xf86DisableCrtc(xf86CrtcPtr crtc)
 {
-if (RRCrtcIsLeased(crtc->randr_crtc))
+if (xf86CrtcIsLeased(crtc))
 return;
 
 crtc->funcs->dpms(crtc, DPMSModeOff);
@@ -2683,7 +2709,7 @@ xf86PrepareOutputs(ScrnInfoPtr scrn)
 for (o = 0; o < config->num_output; o++) {
 xf86OutputPtr output = config->output[o];
 
-if (RROutputIsLeased(output->randr_output))
+if (xf86OutputIsLeased(output))
 continue;
 
 #if RANDR_GET_CRTC_INTERFACE
@@ -2709,7 +2735,7 @@ xf86PrepareCrtcs(ScrnInfoPtr scrn)
 uint32_t desired_outputs = 0, current_outputs = 0;
 int o;
 
-if (RRCrtcIsLeased(crtc->randr_crtc))
+if (xf86CrtcIsLeased(crtc))
 continue;
 
 for (o = 0; o < config->num_output; o++) {
@@ -2732,7 +2758,7 @@ xf86PrepareCrtcs(ScrnInfoPtr scrn)
 if (desired_outputs != current_outputs || !desired_outputs)
 xf86DisableCrtc(crtc);
 #else
-if (RRCrtcIsLeased(crtc->randr_crtc))
+if (xf86CrtcIsLeased(crtc))
 continue;
 
 xf86DisableCrtc(crtc);
@@ -2970,7 +2996,7 @@ xf86DPMSSet(ScrnInfoPtr scrn, int mode, int flags)
 for (i = 0; i < config->num_output; i++) {
 xf86OutputPtr output = config->output[i];
 
-if (!RROutputIsLeased(output->randr_output) && output->crtc != 
NULL)
+if (!xf86OutputIsLeased(output) && output->crtc != NULL)
 (*output->funcs->dpms) (output, mode);
 }
 }
@@ -2986,7 +3012,7 @@ xf86DPMSSet(ScrnInfoPtr scrn, int mode, int flags)
 for (i = 0; i < config->num_output; i++) {
 xf86OutputPtr output = config->output[i];
 
-if (!RROutputIsLeased(output->randr_output) && output->crtc != 
NULL)
+if (!xf86OutputIsLeased(output) && output->crtc != NULL)
 (*output->funcs->dpms) (output, mode);
 }
 }
-- 
2.17.1

___
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

[PATCH xserver 1/2] xfree86: Reset randr_crtc and randr_output early in xf86CrtcCloseScreen

[Keith Packard]

The DIX crtc and output structures are freed when their resources are
destroyed, which happens before CloseScreen is called. As a result, we
know these pointers are invalid and referencing them during any of the
remaining CloseScreen sequence will be bad.

Signed-off-by: Keith Packard 
Cc: thellst...@vmware.com
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=106960
---
 hw/xfree86/modes/xf86Crtc.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index 4aa77a244..142ab1ebe 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -734,14 +734,11 @@ xf86CrtcCloseScreen(ScreenPtr screen)
 xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
 int o, c;
 
-screen->CloseScreen = config->CloseScreen;
-
-xf86RotateCloseScreen(screen);
-
-xf86RandR12CloseScreen(screen);
-
-screen->CloseScreen(screen);
-
+/* The randr_output and randr_crtc pointers are already invalid as
+ * the DIX resources were freed when the associated resources were
+ * freed. Clear them now; referencing through them during the rest
+ * of the CloseScreen sequence will not end well.
+ */
 for (o = 0; o < config->num_output; o++) {
 xf86OutputPtr output = config->output[o];
 
@@ -752,6 +749,15 @@ xf86CrtcCloseScreen(ScreenPtr screen)
 
 crtc->randr_crtc = NULL;
 }
+
+screen->CloseScreen = config->CloseScreen;
+
+xf86RotateCloseScreen(screen);
+
+xf86RandR12CloseScreen(screen);
+
+screen->CloseScreen(screen);
+
 /* detach any providers */
 if (config->randr_provider) {
 RRProviderDestroy(config->randr_provider);
-- 
2.17.1

___
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

[PATCH xserver] xf86-video-modesetting: Lease planes as well if using atomic

[Keith Packard]

If we're using atomic modesetting, then we're also using universal
planes, and so the lease we create needs to include the plane.

Signed-off-by: Keith Packard 
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 859a21a9d..dbb885e8e 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -3251,6 +3251,9 @@ drmmode_create_lease(RRLeasePtr lease, int *fd)
 
 nobjects = ncrtc + noutput;
 
+if (ms->atomic_modeset)
+nobjects += ncrtc;
+
 if (nobjects == 0)
 return BadValue;
 
@@ -3267,12 +3270,14 @@ drmmode_create_lease(RRLeasePtr lease, int *fd)
 
 i = 0;
 
-/* Add CRTC ids */
+/* Add CRTC and plane ids */
 for (c = 0; c < ncrtc; c++) {
 xf86CrtcPtr crtc = lease->crtcs[c]->devPrivate;
 drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
 
 objects[i++] = drmmode_crtc->mode_crtc->crtc_id;
+if (ms->atomic_modeset)
+objects[i++] = drmmode_crtc->plane_id;
 }
 
 /* Add connector ids */
-- 
2.17.1

___
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: RR Leases use-after-free bug

[Keith Packard]

Thomas Hellstrom  writes:

> Hi!
>
> I'd like to draw your attention to bug 106960 where the new leases code 
> accesses freed memory.

Thanks. I've got a proposed fix which re-works where leases are taken
down during X server reset or termination.

> On xf86-video-vmware it causes a server segfault. On modesetting it 
> doesn't (yet) but can be seen with valgrind.

I've hacked up a client to take a lease and close the X connection so
that the lease hangs around for X server shutdown for testing; the
results were pretty spectacular. The leases held pointers to crtcs and
outputs; during termination, the crtcs and outputs were freed when their
associated XIDs were freed, which happens before CloseScreen. At
CloseScreen time, the leases still held pointers to those objects which
caused all sorts of issues.

The patch below terminates any lease referencing a CRTC or
output that is being destroyed; that makes the leases go away before
CloseScreen. Any remaining leases (which must have no outputs or CRTCs
at all) get terminated in RRCloseScreen.

Lease termination is no longer necessary in the driver now, so I've
removed it from the modesetting driver.

I also discovered that leasing is broken with current X server master;
patches to fix that have been sent to the list under a separate cover.

From 509d59ce4f9faa939c83a987aae7552293c8b624 Mon Sep 17 00:00:00 2001
From: Keith Packard 
Date: Tue, 26 Jun 2018 09:20:00 -0700
Subject: [PATCH xserver] During reset/shutdown, clean up leases in DIX instead
 of each driver

Instead of having every video driver loop over any pending leases to
free them during CloseScreen, do this up in the DIX layer by
terminating leases when a leased CRTC or Output is destroyed and
(just to make sure), also terminating leases in RRCloseScreen. The
latter should "never" get invoked as any lease should be associated
with a resource which was destroyed.

This is required as by the time the driver's CloseScreen function is
invoked, we've already freed all of the DIX randr structures and no
longer have any way to reference the leases

Signed-off-by: Keith Packard 
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=106960
Cc: Thomas Hellstrom 
---
 hw/xfree86/drivers/modesetting/driver.c |  2 --
 .../drivers/modesetting/drmmode_display.c   | 17 -
 .../drivers/modesetting/drmmode_display.h   |  2 --
 randr/randr.c   |  4 
 randr/randrstr.h|  3 +++
 randr/rrcrtc.c  | 11 +++
 randr/rrlease.c |  2 +-
 randr/rroutput.c| 11 +++
 8 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
index 66ed2b811..02cd0b4ac 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -1812,8 +1812,6 @@ CloseScreen(ScreenPtr pScreen)
 ms->drmmode.shadow_fb2 = NULL;
 }
 
-drmmode_terminate_leases(pScrn, >drmmode);
-
 drmmode_uevent_fini(pScrn, >drmmode);
 
 drmmode_free_bos(pScrn, >drmmode);
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
index dbb885e8e..fa0a46dc3 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -3325,23 +3325,6 @@ drmmode_terminate_lease(RRLeasePtr lease)
 }
 }
 
-void
-drmmode_terminate_leases(ScrnInfoPtr pScrn, drmmode_ptr drmmode)
-{
-ScreenPtr screen = xf86ScrnToScreen(pScrn);
-rrScrPrivPtr scr_priv = rrGetScrPriv(screen);
-RRLeasePtr lease, next;
-
-xorg_list_for_each_entry_safe(lease, next, _priv->leases, list) {
-drmmode_lease_private_ptr lease_private = lease->devPrivate;
-drmModeRevokeLease(drmmode->fd, lease_private->lessee_id);
-free(lease_private);
-lease->devPrivate = NULL;
-RRLeaseTerminated(lease);
-RRLeaseFree(lease);
-}
-}
-
 static const xf86CrtcConfigFuncsRec drmmode_xf86crtc_config_funcs = {
 .resize = drmmode_xf86crtc_resize,
 .create_lease = drmmode_create_lease,
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h
index e46a292d3..cde661450 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.h
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
@@ -272,8 +272,6 @@ extern Bool drmmode_setup_colormap(ScreenPtr pScreen, ScrnInfoPtr pScrn);
 extern void drmmode_uevent_init(ScrnInfoPtr scrn, drmmode_ptr drmmode);
 extern void drmmode_uevent_fini(ScrnInfoPtr scrn, drmmode_ptr drmmode);
 
-extern void drmmode_terminate_leases(ScrnInfoPtr scrn, drmmode_ptr drmmode);
-
 Bool drmmode_create_initial_bos(ScrnInfoPtr pScrn, drmmode_ptr drmmode);
 void *drmmode_map_front_bo(drmmode_ptr d

[PATCH xserver] xf86-video-modesetting: Don't enable UNIVERSAL_PLANES separately

[Keith Packard]

We don't want universal_planes unless we're using atomic APIs for
modesetting, and the kernel already enables universal_planes
automatically when atomic is enabled.

If we enable universal_planes when we're not using atomic, then we
won't have selected a plane for each crtc, and this will break lease
creation which requires planes for each output when universal_planes
is enabled.

Signed-off-by: Keith Packard 
---
 hw/xfree86/drivers/modesetting/driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index 140852acc..02cd0b4ac 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -1014,8 +1014,7 @@ PreInit(ScrnInfoPtr pScrn, int flags)
 #endif
 }
 
-ret = drmSetClientCap(ms->fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
-ret |= drmSetClientCap(ms->fd, DRM_CLIENT_CAP_ATOMIC, 1);
+ret = drmSetClientCap(ms->fd, DRM_CLIENT_CAP_ATOMIC, 1);
 ms->atomic_modeset = (ret == 0);
 
 ms->kms_has_modifiers = FALSE;
-- 
2.17.1

___
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: Fw: merging imake fork

[Keith Packard]

Alan Coopersmith  writes:

> and the most likely outcome will be many people recommending you move
> to anything but Imake, and maybe offering to make you the maintainer of 
> X.Org's
> version so we can stop paying attention to it too.

I don't think we even want imake in the X.org repository anymore. I'd
suggest that the CDE developers are likely the best candidates to own
that project at this point, unless they decide to switch to something
more modern as we have.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xorgproto 1/2] Remove the use of no-op B16 & B32 bitfield macros in headers

[Keith Packard]

Alan Coopersmith  writes:

> These have always done nothing on all platforms except CRAY.
> As https://bugs.freedesktop.org/show_bug.cgi?id=45202 points out
> we don't even detect when they've been wrong for decades.
>
> Performed via:
> find include -name '*.h' | grep -v md.h | xargs perl -i -p -e 's{\s+B\d+}{}g'
> followed by manual whitespace fixups to preserve visual alignment.
>
> The #defines for B16 & B32 are left in place to preserve compatibility
> in any code that used them outside the xorgproto repo.
>
> Signed-off-by: Alan Coopersmith 

Acked-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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 xorg-proto] Remove libdir from pc files.

[Keith Packard]

Jeremy Puhlman  writes:

> Currently the pc files define libdir, however they are installed into
> /usr/share, which means they should be architecture agnostic.

Makes sense to me; none of these install any libraries.

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] present/wnmd: Preserve window pixmap's screen_x/y on flip

[Keith Packard]

Michel Dänzer  writes:

> From: Michel Dänzer 
>
> The incorrect values could result in the new pixmap's contents
> getting corrupted down the line.
>
> Bugzilla: https://bugs.freedesktop.org/106841
> Fixes: 029608dd8020 "present: Add window flip mode"

Ah. Makes sense. Screen flips are always at 0,0...

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] glamor: Enable modifier support for xfree86 too

[Keith Packard]

Adam Jackson  writes:

> This was left disabled in 1.20.0, it's time to start being sure it
> works.
>
> Signed-off-by: Adam Jackson 

Yup.

Acked-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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

[PATCH xserver] xfree86: Wrap RRCrtcIsLeased and RROutputIsLeased to check for DIX structures

[Keith Packard]

Before DIX structures are allocated for crtcs and outputs, we don't
want to call DIX randr code with NULL pointers. This can happen if the
driver sets video modes early in server initialization, which Nouveau
does in zaphod mode.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=106772
Signed-off-by: Keith Packard 
---
 hw/xfree86/modes/xf86Crtc.c | 42 ++---
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index 4aa77a244..dd795f983 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -174,6 +174,32 @@ xf86CrtcInUse(xf86CrtcPtr crtc)
 return FALSE;
 }
 
+/**
+ * Return whether the crtc is leased by a client
+ */
+
+static Bool
+xf86CrtcIsLeased(xf86CrtcPtr crtc)
+{
+/* If the DIX structure hasn't been created, it can't have been leased */
+if (!crtc->randr_crtc)
+return FALSE;
+return RRCrtcIsLeased(crtc->randr_crtc);
+}
+
+/**
+ * Return whether the output is leased by a client
+ */
+
+static Bool
+xf86OutputIsLeased(xf86OutputPtr output)
+{
+/* If the DIX structure hasn't been created, it can't have been leased */
+if (!output->randr_output)
+return FALSE;
+return RROutputIsLeased(output->randr_output);
+}
+
 void
 xf86CrtcSetScreenSubpixelOrder(ScreenPtr pScreen)
 {
@@ -254,7 +280,7 @@ xf86CrtcSetModeTransform(xf86CrtcPtr crtc, DisplayModePtr 
mode,
 RRTransformRec saved_transform;
 Bool saved_transform_present;
 
-crtc->enabled = xf86CrtcInUse(crtc) && !RRCrtcIsLeased(crtc->randr_crtc);;
+crtc->enabled = xf86CrtcInUse(crtc) && !xf86CrtcIsLeased(crtc);
 
 /* We only hit this if someone explicitly sends a "disabled" modeset. */
 if (!crtc->enabled) {
@@ -412,7 +438,7 @@ xf86CrtcSetOrigin(xf86CrtcPtr crtc, int x, int y)
 crtc->x = x;
 crtc->y = y;
 
-if (RRCrtcIsLeased(crtc->randr_crtc))
+if (xf86CrtcIsLeased(crtc))
 return;
 
 if (crtc->funcs->set_origin) {
@@ -2656,7 +2682,7 @@ xf86InitialConfiguration(ScrnInfoPtr scrn, Bool canGrow)
 static void
 xf86DisableCrtc(xf86CrtcPtr crtc)
 {
-if (RRCrtcIsLeased(crtc->randr_crtc))
+if (xf86CrtcIsLeased(crtc))
 return;
 
 crtc->funcs->dpms(crtc, DPMSModeOff);
@@ -2677,7 +2703,7 @@ xf86PrepareOutputs(ScrnInfoPtr scrn)
 for (o = 0; o < config->num_output; o++) {
 xf86OutputPtr output = config->output[o];
 
-if (RROutputIsLeased(output->randr_output))
+if (xf86OutputIsLeased(output))
 continue;
 
 #if RANDR_GET_CRTC_INTERFACE
@@ -2703,7 +2729,7 @@ xf86PrepareCrtcs(ScrnInfoPtr scrn)
 uint32_t desired_outputs = 0, current_outputs = 0;
 int o;
 
-if (RRCrtcIsLeased(crtc->randr_crtc))
+if (xf86CrtcIsLeased(crtc))
 continue;
 
 for (o = 0; o < config->num_output; o++) {
@@ -2726,7 +2752,7 @@ xf86PrepareCrtcs(ScrnInfoPtr scrn)
 if (desired_outputs != current_outputs || !desired_outputs)
 xf86DisableCrtc(crtc);
 #else
-if (RRCrtcIsLeased(crtc->randr_crtc))
+if (xf86CrtcIsLeased(crtc))
 continue;
 
 xf86DisableCrtc(crtc);
@@ -2964,7 +2990,7 @@ xf86DPMSSet(ScrnInfoPtr scrn, int mode, int flags)
 for (i = 0; i < config->num_output; i++) {
 xf86OutputPtr output = config->output[i];
 
-if (!RROutputIsLeased(output->randr_output) && output->crtc != 
NULL)
+if (!xf86OutputIsLeased(output) && output->crtc != NULL)
 (*output->funcs->dpms) (output, mode);
 }
 }
@@ -2980,7 +3006,7 @@ xf86DPMSSet(ScrnInfoPtr scrn, int mode, int flags)
 for (i = 0; i < config->num_output; i++) {
 xf86OutputPtr output = config->output[i];
 
-if (!RROutputIsLeased(output->randr_output) && output->crtc != 
NULL)
+if (!xf86OutputIsLeased(output) && output->crtc != NULL)
 (*output->funcs->dpms) (output, mode);
 }
 }
-- 
2.17.1

___
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 xserver 1/2] xfree86: Fix O_CLOEXEC usage in lnx_platform

[Keith Packard]

Michel Dänzer <mic...@daenzer.net> writes:

> From: Michel Dänzer <michel.daen...@amd.com>
>
> It was passing O_CLOEXEC as permission bits instead of as a flag.

Both are

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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: Stepping back

[Keith Packard]

Alan Coopersmith  writes:

> While in person discussions can be efficient, I do wonder if limiting
> them to people who can travel to XDC is how we end up burning out the
> same folks over and over.

A good point. With corporate support for desktop computing seeming
fairly limited, I suspect we're going to need some creative ideas on how
to continue development.

> I don't think the world is yet ready to stop using X altogether, but
> it's getting harder and harder for us to keep X alive.

Adam and others have spent the last years reducing the code base. With
xf86-video-modesetting and xf86-input-libinput, we have at least reduced
the amount of work needed to just keep X running on new hardware.

But, there's always the general churn of bug fixes and minor
improvements, and those require management and testing to shepherd them
to a release.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Stepping back

[Keith Packard]

Adam Jackson  writes:

> tl;dr: I will not be release manager for 1.21, nor for anything
> thereafter either, and this time that's probably permanent.

I'd like to thank you for all of the work you have done and with you all
the best in your next adventures.

> As for what this means for tree management and future release plans,
> well, I can't answer that, that's sort of the point. There's a
> community discussion that needs to happen there, and my opinions can't 
> dominate that if I'm serious about stepping back.

We can start discussions now, and I think we should plan on holding a
discussion about this during XDC in September.

Again, thanks very much for the work you've done; it's been an awesome run.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] os: Call FlushClient() before sending FD-passing messages

[Keith Packard]

Alexander Volkov  writes:

> libxcb stores received file descriptors in the buffer of size 16 
> (XCB_MAX_PASS_FD).
> Whether it's possible that the X server will send more than 16 fds in a 
> single reply
> and overflow the libxcb's buffer?

It wouldn't be if the X server were careful in flushing things, but as
that seems 'hard', we should probably fix xcb. I don't think that's
terribly urgent as it would take a very strange situation to pass 16 fds
in a short amount of time, especially in such close proximity as to end
up not getting a reply that processes any of them in the middle of the
sequence.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 0/8] GCC8 warning fixes

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> gcc 8's buffer size logic got a bit pickier, in mostly reasonable ways,
> but -Wmaybe-uninitialized got a lot more speculative. Regardless I'm
> tired of seeing warnings in my build logs.

(some of these are truly ridiculous, but...)

Acked-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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

[PATCH xcb/libxcb] Don't close file descriptors received before associated replies

[Keith Packard]

The X server may send reply file descriptors at any point before the
reply itself as the buffering of fds in the socket is separate from
reply data. Instead of closing those received early, leave them around
in xcb and await the receipt of the associated reply.

Signed-off-by: Keith Packard <kei...@keithp.com>
---
 src/xcb_in.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/src/xcb_in.c b/src/xcb_in.c
index 73209e0..fc58622 100644
--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -1025,17 +1025,11 @@ int _xcb_in_read(xcb_connection_t *c)
 c->in.in_fd.nfd * sizeof (int));
 c->in.in_fd.ifd = 0;
 
-/* If we have any left-over file descriptors after emptying
- * the input buffer, then the server sent some that we weren't
- * expecting.  Close them and mark the connection as broken;
+/* Leave any remaining file descriptors around; the associated
+ * replies will be along shortly to pick them up. This way,
+ * the X server just needs to make sure the fds are sent no
+ * later than the reply, but can be sent at any earlier time.
  */
-if (c->in.queue_len == 0 && c->in.in_fd.nfd != 0) {
-int i;
-for (i = 0; i < c->in.in_fd.nfd; i++)
-close(c->in.in_fd.fd[i]);
-_xcb_conn_shutdown(c, XCB_CONN_CLOSED_FDPASSING_FAILED);
-return 0;
-}
 }
 #endif
 #ifndef _WIN32
-- 
2.16.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: [PATCH xserver] os: Call FlushClient() before sending FD-passing messages

[Keith Packard]

Alexander Volkov  writes:

> Yes, it would be easier to fix this in libxcb, but I believe that it
> would be more correct to do this in the X server. At least I want to
> try to fix the X server.

Hrm.

The problem is that there are two streams of data here -- the stream of
fds and the stream of replies. xcb currently insists that they remain
aligned so that any fds are associated with the reply data received in
the same recvmsg operation. This allows an X server to send fds which
the client is not expecting, with the client can silently closing
them.

If we let the two streams run un-aligned, then there will be a queue of
received fds that should (unless there's a bug) eventually get
associated with the correct reply. The requirement here is looser -- we
just need to make sure the fds arrive no later than the reply data. This
seems much easier to achieve, and in fact the current X server code does
this today.

Changing xcb to allow early fds involves removing code that closes fds
received before the associated reply.

Changing the X server to ensure that fds are delivered exactly with the
associated reply data involves adding code to queue the fds and insert
additional flushes to make sure the kernel writes the fds with the start
of the reply, and then making sure that xcb doesn't discard fds received
with a partial reply.

Given these two possible solutions, I'd like to suggest that we might
prefer the simpler one.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] os: Call FlushClient() before sending FD-passing messages

[Keith Packard]

Alexander Volkov  writes:

> Otherwise a client may receive data with an unrelated file
> descriptor after calling recvmsg() if its input buffer is not
> big enough. In libxcb it may lead to a situation when all
> received messages fit the buffer while a message related to
> the attached fd is not received yet. libxcb can't find the
> corresponding message and fails with XCB_CONN_CLOSED_FDPASSING_FAILED
> error.

Thanks for looking in to this; it looks like a problem to me as well
because xcb does this:

/* If we have any left-over file descriptors after emptying
 * the input buffer, then the server sent some that we weren't
 * expecting.  Close them and mark the connection as broken;
 */

However, fixing this in the server is harder than it appears...

> +if (oc->output && oc->output->count > 0)
> +(void) FlushClient(client, oc, (char *) NULL, 0);
> +

FlushClient doesn't empty the buffer when the kernel buffers are full,
so while calling it here may reduce the occurrence of the problem, it
won't eliminate it.

One way to fix this would be to have the X server OS layer *also* buffer
fds and ensure that they are presented to Xtrans just before the reply
which uses them. Alternatively, we could fix xcb so that it kept the fds
around for 'a while' instead of discarding them immediately. The latter
approach seems a lot easier to me?

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] meson: Add option to set default font path (v2)

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> +default_font_path = ','.join([
> +join_paths(fontrootdir, 'misc'),
> +join_paths(fontrootdir, 'TTF'),
> +join_paths(fontrootdir, 'OTF'),
> +join_paths(fontrootdir, 'Type1'),
> +join_paths(fontrootdir, '75dpi'),
> +join_paths(fontrootdir, '100dpi'),

So close! The current default sticks 100dpi ahead of 75dpi.

Do we care about using meson for OS X yet? There are some extra paths
added in that case (/Library/Fonts and /System/Library/Fonts).

With the bitmap fonts reversed, this is

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 08/12] meson: Build cvt and gtf

[Keith Packard]

Adam Jackson  writes:

> What'd be Real Nice is if xrandr knew how to generate modes for given
> sizes.

Yeah, I've started to add cvt to xrandr a couple of times but never
finished. Would also be nice to let you add a mode, associate with an
output and set that mode all in one command line.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 05/12] meson: Build Xorg suid wrapper

[Keith Packard]

Adam Jackson  writes:

> Probably this path should add a warning() to alert the builder?

Yeah, probably.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 12/12] meson: Fix install path for 10-quirks.conf

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> Signed-off-by: Adam Jackson <a...@redhat.com>

Almost got me with this one -- autotools uses XF86CONFIGDIR but offers
no way to configure it. Hardcoding the name seems just fine to me.

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 11/12] meson: Install xorg-server.m4

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> Signed-off-by: Adam Jackson <a...@redhat.com>

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 10/12] meson: Generate xorg-server.pc

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> Otherwise external drivers can't build against us.
>
> Signed-off-by: Adam Jackson <a...@redhat.com>

Acked-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 09/12] meson: Install the dmx utilities

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> And add the forgotten dmxrminput to the list.

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 08/12] meson: Build cvt and gtf

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> Signed-off-by: Adam Jackson <a...@redhat.com>

These shouldn't be in the X server sources, but...

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 07/12] meson: Install man pages

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> Signed-off-by: Adam Jackson <a...@redhat.com>

I have no idea if this works correctly, but we certainly need it if it
does, so:

Acked-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 06/12] meson: Add option to set default font path

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> +option('default_font_path', type: 'string',
> +   value: 'catalogue:/etc/X11/fontpath.d,built-ins')

This isn't the autotools default; seems like you should probably just
use that?

Otherwise,

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 05/12] meson: Build Xorg suid wrapper

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> Signed-off-by: Adam Jackson <a...@redhat.com>

> +if get_option('suid_wrapper')
> +executable('Xorg.wrap',
> +'xorg-wrapper.c',
> +include_directories: [inc, xorg_inc],
> +dependencies: xorg_deps,
> +c_args: xorg_c_args,
> +install: true,
> +install_dir: get_option('libexecdir'),
> +# install_mode: ['r-sr-xr-x', 0, 0],

I assume your package files fix the permissions? If I'm reading the
autotools version, there's a chown and chmod for that?

Otherwise, this is

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 04/12] meson: Fix installing protocol.txt

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> One fix the constructed path, two actually install it.
>
> Signed-off-by: Adam Jackson <a...@redhat.com>

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 03/12] meson: Add libdrm to hw/xfree86/common's dependencies

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> Signed-off-by: Adam Jackson <a...@redhat.com>

How did this ever work?

Acked-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 01/12] autotools: Stop caring about XORG_DATE

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> Signed-off-by: Adam Jackson <a...@redhat.com>

Acked-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver 02/12] man: s/__/@/g

[Keith Packard]

Adam Jackson <a...@redhat.com> writes:

> A cosmetic change for automake (though we have to replicate some of
> xorg-macros.m4 in manpages.am now), but meson's configure_file() wants
> @-delimited strings.
>
> Signed-off-by: Adam Jackson <a...@redhat.com>

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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:libXaw] Fix xload crashes if the window is wider than 2048 pixels

[Keith Packard]

Alan Coopersmith <alan.coopersm...@oracle.com> writes:

> https://bugs.freedesktop.org/show_bug.cgi?id=96670
>
> Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>

Tested-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] present: cap the version returned to the client

[Keith Packard]

Julien Cristau  writes:

> Doesn't this break when e.g. client supports 2.2 and server supports
> 1.4, where we'll return 2.2 instead of 1.4?  i.e. it seems to me this
> should be
>
> if (rep.majorVersion > stuff->majorVersion ||
> (rep.majorVersion == stuff->majorVersion &&
>  rep.minorVersion > stuff->minorVersion)) {
>  ...
> }

Yeah, probably. Of course, we will "never" have a major version change
as that would indicate an incompatible change in the protocol.

-- 
-keith


signature.asc
Description: PGP signature
___
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] Use flexible array member instead of fake size.

[Keith Packard]

Michal Srb <m...@suse.com> writes:

> The _XimCacheStruct structure is followed in memory by two strings containing
> fname and encoding. The memory was accessed using the last member of the
> structure `char fname[1]`. That is a lie, prohibits us from using sizeof and
> confuses checkers. Lets declare it properly as a flexible array, so compilers
> don't complain about writing past that array. As bonus we can replace the
> XOffsetOf with regular sizeof.

Thanks; this form is how we used to need to do this as C compilers
didn't support a flexible array at the end of a struct...

Reviewed-by: Keith Packard <kei...@keithp.com>

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] randr: Initialize RROuptutRec::nonDesktop

[Keith Packard]

Michel Dänzer <mic...@daenzer.net> writes:

> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>

Reviewed-by: Keith Packard <kei...@keithp.com

(I'd have been happy to see this swap malloc for calloc as well)

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] Xext/saver: Swap ScreenSaverSuspend 'suspend' field. Handle old XCB clients.

[Keith Packard]

Mihai Moldovan  writes:

> The actual changes (both server and proto) LGTM.

I'll take that as a 'Reviewed-by', unless you have some objection.

-- 
-keith


signature.asc
Description: PGP signature
___
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 xserver] dix: don't free() stack memory

[Keith Packard]

Eric Engestrom  writes:

> I'll look at the code more closely to figure out when the free is
> needed, but I just saw this warning and had a look, this isn't code I'm
> familiar with *at all*, so I might just end up giving up if I can't
> figure it out easily enough :/

There's no free of stack memory in this path, but the compiler can't
figure it out. The free path is only hit if the client had been put to
sleep to wait for the font, in which case the original stack structure
would have been copied to allocated memory.

Hrm. It might be easy to fix this by simply creating a new function to
pass to ClientSleep which does the free and leave doImageText out of
that part.

Alternatively, leave the code alone and stick some comments or
instructions for the compiler to not complain.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Missing swapl() in Xext/saver.c?

[Keith Packard]

Alan Coopersmith  writes:

> On 03/10/18 01:31 AM, Mihai Moldovan wrote:
>> int everywhere and includes the (ignored) B32 width specifier
>
> We could just delete those now, as they're just noise for the pre-processor
> to strip ever since we dropped support for Cray builds with "everything is
> a 64-bit int, using bitfields to emulate smaller sizes" back in 2013:

Sure, would just require a simple sed script run over the xorgproto repo
at this point.

-- 
-keith


signature.asc
Description: PGP signature
___
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: Missing swapl() in Xext/saver.c?

[Keith Packard]

Mihai Moldovan  writes:

> Sounds sane. Reverting to the initially intended BOOL type would cause 
> breakage,
> but using CARD32 + swapl() would leave the struct size consistent and fix the
> issue in newer versions.

I've posted patches for xcb/proto, xorgproto and the X server. For the X
server, I've also added a hack to keep old XCB clients on LSB machines
working by only using the low byte of the 4-byte value. There's not much
we can do for old XCB clients on MSB machines; the true value will be in
the high byte, whereas Xlib clients using the same version will be in
the low byte.

Of course, this changes the API for xcb, which used to use uint8_t and
now uses uint32_t. That doesn't change the ABI on any 32-bit int or
larger machine as the uint8_t would be passed in the same size object as
the uint32_t, and the X server will be ignoring the high three bytes.

Review is encouraged.

-- 
-keith


signature.asc
Description: PGP signature
___
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