[RFC PATCH xserver] os: move tempfiles.d/x11.conf from systemd to here

[Peter Hutterer]

Let's not rely on some other package to set up and clean up after our
tempfiles.

Signed-off-by: Peter Hutterer 
---
Not all of these are created by the server, but moving them to the
respective libs etc. makes even less sense.

 configure.ac   | 13 +
 os/Makefile.am |  7 ++-
 os/x11.conf| 11 +++
 3 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 os/x11.conf

diff --git a/configure.ac b/configure.ac
index ec98f52c0..54d71bbbe 100644
--- a/configure.ac
+++ b/configure.ac
@@ -851,6 +851,19 @@ if test "x$WITH_SYSTEMD_DAEMON" = "xyes" -o 
"x$WITH_SYSTEMD_DAEMON" = "xauto" ;
 fi
 AM_CONDITIONAL([HAVE_SYSTEMD_DAEMON], [test "x$HAVE_SYSTEMD_DAEMON" = "xyes"])
 
+dnl systemd tmpfiles.d directory
+PKG_CHECK_MODULES([SYSTEMD], [systemd],
+  [tmpfilesdir=`$PKG_CONFIG --variable=tmpfilesdir systemd`],
+  [tmpfilesdir=no])
+AC_ARG_WITH([tmpfiles-dir],
+AS_HELP_STRING([--with-tmpfiles-dir],
+   [Install the tmpfiles into the given directory 
(default: auto)]),
+[TMPFILES_DIR=$withval], [TMPFILES_DIR=$tmpfilesdir])
+if test "x$TMPFILES_DIR" != "xno"; then
+   AC_SUBST(TMPFILES_DIR, "$TMPFILES_DIR")
+fi
+AM_CONDITIONAL(HAVE_TMPFILES_DIR, test "x$TMPFILES_DIR" != "xno")
+
 if test "x$CONFIG_UDEV" = xyes && test "x$CONFIG_HAL" = xyes; then
AC_MSG_ERROR([Hotplugging through both libudev and hal not allowed])
 fi
diff --git a/os/Makefile.am b/os/Makefile.am
index c6e78cb99..437e91431 100644
--- a/os/Makefile.am
+++ b/os/Makefile.am
@@ -54,7 +54,12 @@ if BUSFAULT
 libos_la_SOURCES += $(BUSFAULT_SRCS)
 endif
 
-EXTRA_DIST = $(SECURERPC_SRCS) $(XDMCP_SRCS)
+if HAVE_TMPFILES_DIR
+tmpfilesdir = $(TMPFILES_DIR)
+tmpfiles_DATA = x11.conf
+endif
+
+EXTRA_DIST = $(SECURERPC_SRCS) $(XDMCP_SRCS) x11.conf
 
 if SPECIAL_DTRACE_OBJECTS
 # Generate dtrace object code for probes in libos & libdix
diff --git a/os/x11.conf b/os/x11.conf
new file mode 100644
index 0..eb2d67d72
--- /dev/null
+++ b/os/x11.conf
@@ -0,0 +1,11 @@
+# See tmpfiles.d(5) for details
+
+# Make sure these are created by default so that nobody else can
+d /tmp/.X11-unix 1777 root root 10d
+d /tmp/.ICE-unix 1777 root root 10d
+d /tmp/.XIM-unix 1777 root root 10d
+d /tmp/.font-unix 1777 root root 10d
+d /tmp/.Test-unix 1777 root root 10d
+
+# Unlink the X11 lock files
+r! /tmp/.X[0-9]*-lock
-- 
2.13.6

___
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 3/3] ramdac: Handle master and slave cursors independently

[Alex Goins]

Thanks both for the quick feedback. Replies inline.

On Wed, 8 Nov 2017, Hans de Goede wrote:

> > https://lists.x.org/archives/xorg-devel/2016-September/050973.html implies
> > that
> > xf86CursorScreenPtr is part of the ABI. Mark xf86CursorPriv.h as such.
> 
> I'm not sure if exporting this is a good idea. I assume you are just
> after xf86CursorScreenPtr->SWCursor ?  If so it is probably a better
> idea to add a helper function taking pScreen which gets that and add
> that function to the ABI.

That works for me, this is just based on the past advice for drivers to lookup
xf86CursorScreenPtr to tell if we are using a SW cursor, which sets a precedent
of it being considered ABI.

> > +Bool
> > +xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr
> > infoPtr)
> > +{
> > +Bool use_hw_cursor = FALSE;
> > +
> > +input_lock();
> > +
> > +if (xf86CheckSlavesHWCursor_locked(pScreen, cursor)) {
> > +use_hw_cursor = TRUE;
> > +goto unlock;
> 
> This goto seems wrong, what if the secondary GPU has outputs which
> can do a hw-cursor, but the primary GPU also has active video outputs
> and the primary GPU does not support a hw-cursor ?
> 
> I think you always need to check the master supports a hw-cursor.
> 
> > +/* if there are no active slaves that support HW cursor, or using the
> > HW
> > + * cursor on them failed, use the master */
> > +if (ScreenPriv->SlaveHWCursor) {
> > +xf86SetSlavesCursor_locked(pScreen, NullCursor, x, y);
> > +ScreenPriv->SlaveHWCursor = FALSE;
> > +}
> > +ret = xf86ScreenSetCursor(pScreen, pCurs, x, y);
> >  out:
> >   input_unlock();
> 
> 
> Again what if both the primary and secondary GPU have active video outputs,
> then you need to update the cursor on both. You seem te be thinking about
> something like the XPS15 here, used with the nvidia driver forcing the
> nvidia GPU to become the primary GPU, so the primary will not have any
> video-outputs. But when using for example a Latitude E6430 in optimus
> mode with nouveau, the intel GPU will be the primary, driving the LCD
> panel and the nvidia GPU will be the secondary driving the HDMI output,
> so you need to set the cursor on both.

You're right, there is a possibility of combined native and PRIME outputs. In
fact, I typically test on a desktop system with iGPU Multi-Monitor, where this
possibility is especially clear.

I overlooked the case of native + PRIME where the PRIME slave supports HW
cursor. Native + PRIME with non-HW cursor slave works, but the former doesn't.

On Wed, 8 Nov 2017, Michel Dänzer wrote:

> > Change 7b634067 added HW cursor support for PRIME by removing the
> > pixmap_dirty_list check from xf86CursorSetCursor() and making the requisite
> > cursor functions set/check the cursor both on master and slave.
> > 
> > However, before this change, drivers that did not make use of 
> > pixmap_dirty_list
> > to implement PRIME master were able to pass this check. That may have been a
> > bug, but in effect it allowed hardware cursor to be enabled with PRIME, 
> > where
> > the master composites the cursor into the shared pixmap.
> > 
> > Naturally, the slave driving an actual hardware cursor is preferable to the
> > master compositing a cursor into the shared pixmap, but there are certain
> > situations where the slave cannot drive a hardware cursor (certain DRM 
> > drivers,
> > or when used with a transform). In these cases the master may still be 
> > capable
> > of compositing a cursor,
> 
> How and where exactly would this "compositing the cursor into the shared
> pixmap" happen? Looks like this is just left for the master screen
> driver to handle? If so, there probably needs to be a mechanism for the
> master screen driver to opt into this.

Yes, it's just left for the master screen the handle as if it were a hardware
cursor.

Prior to change 7b634067 this was the existing behavior of the NVIDIA driver +
PRIME outputs, due to the fact that the check to exclude HW cursor support on
PRIME outputs relied on pScreen->pixmap_dirty_list not being empty. With NVIDIA
as the master, X would successfully set a cursor on the master despite PRIME
being in use, and the master would use that to composite its own cursor into the
shared pixmap.

Change 7b634067 had the side effect of preventing this configuration, and the
intent here is to re-enable it as an alternative fallback to the server's SW
cursor, while maintaining the possibility of slave-driven HW cursor in
configurations that support it.

> > and that would be preferable to using the server's software cursor (due
> > to the fact that it's unsynchronzied by OpenGL rendering to the root
> > window, it can cause corruption with certain compositors).
> 
> Frankly, that sounds like an issue with your direct rendering
> infrastructure. We used to have the same issue with DRI1, but no more
> with DRI2/DRI3 (we still have an intermittent cursor flickering issue
> though, but not 

[PATCH font-util] ucs2any: Fix parser crash on 32 bit

[Tobias Stoeckmann]

It is possible to crash ucs2any or provoke successful return value even
though the processing was not successful.

The problem lies within a possible integer overflow when adding elements
with a key which is too large.

You can trigger the issue this way on a 32 bit system:

$ cat > source.bdf << "EOF"
STARTFONT source
CHARS 1
ENCODING 1073741823
EOF
$ ucs2any source.bdf
Segmentation fault
$ _

Another possibility would be to add "ENCODING 1" right after the CHARS
line. In that case, realloc will allocate 0 bytes afterwards which is a
success but might return NULL, e.g. on Linux/glibc systems. Such a
result value is handled as an error and errno is evaluated and returned,
even though there was no error:

$ cat > source.bdf << "EOF"
STARTFONT source
CHARS 1
ENCODING 1
ENCODING 1073741823
EOF
$ ucs2any source.bdf
ucs2any: Success
$ echo $?
0
$ _

Signed-off-by: Tobias Stoeckmann 
---
 ucs2any.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/ucs2any.c b/ucs2any.c
index 10bb029..1f575d1 100644
--- a/ucs2any.c
+++ b/ucs2any.c
@@ -45,6 +45,7 @@
 #endif
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -220,6 +221,11 @@ da_add(da_t *da, int key, void *value)
 {
int i = da->size;
if (key >= 0) {
+   if ((size_t)key >= SIZE_MAX / sizeof(void *)) {
+   fprintf(stderr, "%s: Illegal key '%d' encountered!\n",
+   my_name, key);
+   exit(1);
+   }
if (key >= da->size) {
da->size = key + 1;
da->values = zrealloc(da->values,
-- 
2.15.0

___
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 3/3] ramdac: Handle master and slave cursors independently

[Michel Dänzer]

On 08/11/17 05:15 AM, Alex Goins wrote:
> Change 7b634067 added HW cursor support for PRIME by removing the
> pixmap_dirty_list check from xf86CursorSetCursor() and making the requisite
> cursor functions set/check the cursor both on master and slave.
> 
> However, before this change, drivers that did not make use of 
> pixmap_dirty_list
> to implement PRIME master were able to pass this check. That may have been a
> bug, but in effect it allowed hardware cursor to be enabled with PRIME, where
> the master composites the cursor into the shared pixmap.
> 
> Naturally, the slave driving an actual hardware cursor is preferable to the
> master compositing a cursor into the shared pixmap, but there are certain
> situations where the slave cannot drive a hardware cursor (certain DRM 
> drivers,
> or when used with a transform). In these cases the master may still be capable
> of compositing a cursor,

How and where exactly would this "compositing the cursor into the shared
pixmap" happen? Looks like this is just left for the master screen
driver to handle? If so, there probably needs to be a mechanism for the
master screen driver to opt into this.


> and that would be preferable to using the server's software cursor (due
> to the fact that it's unsynchronzied by OpenGL rendering to the root
> window, it can cause corruption with certain compositors).

Frankly, that sounds like an issue with your direct rendering
infrastructure. We used to have the same issue with DRI1, but no more
with DRI2/DRI3 (we still have an intermittent cursor flickering issue
though, but not corruption).


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
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] modesetting: fail PreInit() if the device has zero connectors

[Hans de Goede]

Hi,

On 03-11-17 17:56, Giuseppe Bilotta wrote:

On Fri, Nov 3, 2017 at 10:09 AM, Hans de Goede  wrote:

Weird, on my XPS15 9550 where the nvidia GPU does not have/drives any
outputs
I do get 2 devices in xrandr --listproviders as expected. You may want to
start
with figuring out why the normal setup where you load nouveau normally is
not
working. Perhaps once that works, it will also powerdown the GPU properly.


I have an XPS15 9350 with a similar situation. You get two devices if
nouveau is loaded before Xorg starts.

If I understand Tobias' mail, his situation is with nouveau modprobed
_while X is running already_, not if it's loaded before X starts. I've
tried it in the past and I got the same segfaults that he is getting.
I have AutoAddGPU set to false specifically to avoid this segfault,
since I use a similar setup (no driver loaded, modprobe as needed),
although in my case it's more out of a need to be able to switch to
nvidia's proprietary as-needed.

In my case I have verified that without loading nouveau or nvidia the
GPU is powered up, which is supoptimal battery-wise, and I'm not sure
nvidia powers down the GPU while not in use (nouveau does, byt to me
is mostly useless, because the performance it achieves on the dGPU is
less than the one I get on Intel's iGP, and I still need the nvidia
one for CUDA), but as Tobias mentioned, that's a separate issue from
the segfault that comes from runtime nouveau modprobing.


Right, so the thing to fix here is pin down where the segfault from
runtime nouveau probing comes from and fix that, so that once
modprobed things work the same as they do when nouveau is loaded
before X starts.

Regards,

Hans
___
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 3/3] ramdac: Handle master and slave cursors independently

[Hans de Goede]

Hi,

On 08-11-17 05:15, Alex Goins wrote:

Change 7b634067 added HW cursor support for PRIME by removing the
pixmap_dirty_list check from xf86CursorSetCursor() and making the requisite
cursor functions set/check the cursor both on master and slave.

However, before this change, drivers that did not make use of pixmap_dirty_list
to implement PRIME master were able to pass this check. That may have been a
bug, but in effect it allowed hardware cursor to be enabled with PRIME, where
the master composites the cursor into the shared pixmap.

Naturally, the slave driving an actual hardware cursor is preferable to the
master compositing a cursor into the shared pixmap, but there are certain
situations where the slave cannot drive a hardware cursor (certain DRM drivers,
or when used with a transform). In these cases the master may still be capable
of compositing a cursor, and that would be preferable to using the server's
software cursor (due to the fact that it's unsynchronzied by OpenGL rendering to
the root window, it can cause corruption with certain compositors).

The PRIME HW cursor change works by checking both master and slave HW cursor
capabilities, and setting the cursor on both. For masters such as modesetting,
the master cursor capabilities check will pass but the cursor set will be a
NOOP, effectively driving a hardware cursor only on the slave while still
passing the check. However, if two different drivers with different sets of
capabilities are used, HW cursor ends up only being used if the intersection of
capabilities are supported. For example, if the master can drive a cursor with a
transform, and the slave can't, checking capabilities on both means we
unnecessarily fall back to SW cursor.

To alleviate this issue while still prioritizing HW cursor on the slave, this
change restructures the HW cursor code such that the slave aspects of these
functions are tried first, falling back to the master if they fail.

SlaveHWCursor, a flag for querying if the hardware cursor is being driven by
slaves, is added to xf86CursorScreenRec, resulting in an ABI break.

Signed-off-by: Alex Goins 
---
  hw/xfree86/ramdac/xf86CursorPriv.h |   3 +
  hw/xfree86/ramdac/xf86HWCurs.c | 114 +++--
  2 files changed, 88 insertions(+), 29 deletions(-)

diff --git a/hw/xfree86/ramdac/xf86CursorPriv.h 
b/hw/xfree86/ramdac/xf86CursorPriv.h
index 397d2a1..83ea379 100644
--- a/hw/xfree86/ramdac/xf86CursorPriv.h
+++ b/hw/xfree86/ramdac/xf86CursorPriv.h
@@ -35,6 +35,9 @@ typedef struct {
  Bool HWCursorForced;
  
  void *transparentData;

+
+/* If hardware cursor is being driven by slaves */
+Bool SlaveHWCursor;
  } xf86CursorScreenRec, *xf86CursorScreenPtr;
  
  Bool xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y);

diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c
index 15b1cd7..786ed54 100644
--- a/hw/xfree86/ramdac/xf86HWCurs.c
+++ b/hw/xfree86/ramdac/xf86HWCurs.c
@@ -136,18 +136,13 @@ xf86ScreenCheckHWCursor(ScreenPtr pScreen, CursorPtr 
cursor, xf86CursorInfoPtr i
   (!infoPtr->UseHWCursor || infoPtr->UseHWCursor(pScreen, cursor)));
  }
  
-Bool

-xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr 
infoPtr)
+static Bool
+xf86CheckSlavesHWCursor_locked(ScreenPtr pScreen, CursorPtr cursor)
  {
  ScreenPtr pSlave;
-Bool use_hw_cursor = TRUE;
-
-input_lock();
  
-if (!xf86ScreenCheckHWCursor(pScreen, cursor, infoPtr)) {

-use_hw_cursor = FALSE;
-goto unlock;
-}
+Bool use_hw_cursor = FALSE;
+Bool slave_consuming_pixmap = FALSE;
  
  /* ask each driver consuming a pixmap if it can support HW cursor */

  xorg_list_for_each_entry(pSlave, >slave_list, slave_head) {
@@ -156,6 +151,11 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, 
xf86CursorInfoPtr infoPtr
  if (!RRHasScanoutPixmap(pSlave))
  continue;
  
+if (!slave_consuming_pixmap) {

+slave_consuming_pixmap = TRUE;
+use_hw_cursor = TRUE;
+}
+
  sPriv = dixLookupPrivate(>devPrivates, xf86CursorScreenKey);
  if (!sPriv) { /* NULL if Option "SWCursor", possibly other conditions 
*/
  use_hw_cursor = FALSE;
@@ -169,6 +169,26 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, 
xf86CursorInfoPtr infoPtr
  }
  }
  
+/* there are active slaves and they all support hw cursor */

+return (slave_consuming_pixmap && use_hw_cursor);
+}
+
+Bool
+xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr 
infoPtr)
+{
+Bool use_hw_cursor = FALSE;
+
+input_lock();
+
+if (xf86CheckSlavesHWCursor_locked(pScreen, cursor)) {
+use_hw_cursor = TRUE;
+goto unlock;


This goto seems wrong, what if the secondary GPU has outputs which
can do a hw-cursor, but the primary GPU also has active video outputs
and the primary GPU does not support a 

Re: [PATCH xserver 2/3] ramdac: Add xf86CursorPriv.h to ABI

[Hans de Goede]

Hi,

On 08-11-17 05:15, Alex Goins wrote:

https://lists.x.org/archives/xorg-devel/2016-September/050973.html implies that
xf86CursorScreenPtr is part of the ABI. Mark xf86CursorPriv.h as such.


I'm not sure if exporting this is a good idea. I assume you are just
after xf86CursorScreenPtr->SWCursor ?  If so it is probably a better
idea to add a helper function taking pScreen which gets that and add
that function to the ABI.

Regards,

Hans





Signed-off-by: Alex Goins 
---
  hw/xfree86/ramdac/Makefile.am | 4 ++--
  hw/xfree86/sdksyms.sh | 1 +
  2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/xfree86/ramdac/Makefile.am b/hw/xfree86/ramdac/Makefile.am
index 59e0996..89aab61 100644
--- a/hw/xfree86/ramdac/Makefile.am
+++ b/hw/xfree86/ramdac/Makefile.am
@@ -3,9 +3,9 @@ noinst_LTLIBRARIES = libramdac.la
  libramdac_la_SOURCES = xf86RamDac.c xf86RamDacCmap.c \
 xf86CursorRD.c xf86HWCurs.c IBM.c BT.c TI.c
  
-sdk_HEADERS = BT.h IBM.h TI.h xf86Cursor.h xf86RamDac.h

+sdk_HEADERS = BT.h IBM.h TI.h xf86Cursor.h xf86CursorPriv.h xf86RamDac.h
  
-EXTRA_DIST = BTPriv.h IBMPriv.h TIPriv.h xf86CursorPriv.h xf86RamDacPriv.h \

+EXTRA_DIST = BTPriv.h IBMPriv.h TIPriv.h xf86RamDacPriv.h \
CURSOR.NOTES
  
  AM_CFLAGS = $(DIX_CFLAGS) $(XORG_CFLAGS)

diff --git a/hw/xfree86/sdksyms.sh b/hw/xfree86/sdksyms.sh
index 9aa1eec..56e3fab 100755
--- a/hw/xfree86/sdksyms.sh
+++ b/hw/xfree86/sdksyms.sh
@@ -147,6 +147,7 @@ cat > sdksyms.c << EOF
  #include "IBM.h"
  #include "TI.h"
  #include "xf86Cursor.h"
+#include "xf86CursorPriv.h"
  #include "xf86RamDac.h"
  
  


___
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/3] ramdac: Fix formatting in xf86CheckHWCursor()

[Hans de Goede]

Hi,

On 08-11-17 05:15, Alex Goins wrote:

xf86CheckHWCursor() has spacing that is inconsistent with the rest of the file.
Correct this in preparation for subsequent changes.

Signed-off-by: Alex Goins 


LGTM:

Acked-by: Hans de Goede 

Regards,

Hans



---
  hw/xfree86/ramdac/xf86HWCurs.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c
index 366837c..15b1cd7 100644
--- a/hw/xfree86/ramdac/xf86HWCurs.c
+++ b/hw/xfree86/ramdac/xf86HWCurs.c
@@ -146,7 +146,7 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, 
xf86CursorInfoPtr infoPtr
  
  if (!xf86ScreenCheckHWCursor(pScreen, cursor, infoPtr)) {

  use_hw_cursor = FALSE;
-   goto unlock;
+goto unlock;
  }
  
  /* ask each driver consuming a pixmap if it can support HW cursor */

@@ -159,14 +159,14 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, 
xf86CursorInfoPtr infoPtr
  sPriv = dixLookupPrivate(>devPrivates, xf86CursorScreenKey);
  if (!sPriv) { /* NULL if Option "SWCursor", possibly other conditions 
*/
  use_hw_cursor = FALSE;
-   break;
-   }
+break;
+}
  
  /* FALSE if HWCursor not supported by slave */

  if (!xf86ScreenCheckHWCursor(pSlave, cursor, sPriv->CursorInfoPtr)) {
  use_hw_cursor = FALSE;
-   break;
-   }
+break;
+}
  }
  
  unlock:



___
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] xkb: small clarification

[Eric Engestrom]

On Tuesday, 2017-11-07 18:37:11 +0100, Giuseppe Bilotta wrote:
> ---
>  xkb/xkbUtils.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> On Tue, Nov 7, 2017 at 10:55 AM, Eric Engestrom  wrote:
> 
> > I think this patch is good, because it explicitly shows the NoSymbol
> > value that is tested later on. The implicit 0s are fine, but I think adding
> > a one-sentence explanation to the commit message would be good.
> 
> Oops, looks like it was pushed already.
> 
> Not sure if it's worth it anymore, but here's a one-line clarification comment
> in-code, just in case.

Meh, I wouldn't bother. Thanks though :)

> 
> -- 
> Giuseppe "Oblomov" Bilotta
> 
> diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
> index 8975ade8d..023902be5 100644
> --- a/xkb/xkbUtils.c
> +++ b/xkb/xkbUtils.c
> @@ -222,7 +222,9 @@ XkbUpdateKeyTypesFromCore(DeviceIntPtr pXDev,
>  XkbDescPtr xkb;
>  unsigned key, nG, explicit;
>  int types[XkbNumKbdGroups];
> -KeySym tsyms[XkbMaxSymsPerKey] = {NoSymbol}, *syms;
> +/* Initialize tsym with NoSymbol, which conveniently has value 0 */
> +KeySym tsyms[XkbMaxSymsPerKey] = { NoSymbol };
> +KeySym *syms;
>  XkbMapChangesPtr mc;
>  
>  xkb = pXDev->key->xkbInfo->desc;
> -- 
> 2.14.1.439.g647b9b4702
> 
___
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/11] modesetting: Simplify mst path blob parsing

[Daniel Martin]

On 7 November 2017 at 14:06, Emil Velikov  wrote:
> On 7 November 2017 at 09:38, Daniel Martin  wrote:
>> The kernel guarantees that the MST path property blob of a connector
>> has a certain format and this property is immutable - can't be changed
>> from user space. With that in mind, reduce the parsing code to a minimum
>> matching the format criteria.
>>
>> (Preparation to fold the parsing into output name creation function.)
>>
>> Signed-off-by: Daniel Martin 
>> ---
>>  hw/xfree86/drivers/modesetting/drmmode_display.c | 41 
>> +---
>>  1 file changed, 15 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
>> b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> index 404bb1dc2..7ff55ef24 100644
>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> @@ -1636,48 +1636,37 @@ static xf86OutputPtr find_output(ScrnInfoPtr pScrn, 
>> int id)
>>  return NULL;
>>  }
>>
>> -static int parse_path_blob(drmModePropertyBlobPtr path_blob, int 
>> *conn_base_id, char **path)
>> +static Bool
>> +parse_path_blob(drmModePropertyBlobPtr path_blob,
>> +int *conn_base_id, char **extra_path)
>>  {
>> -char *conn;
>> -char conn_id[5];
>> -int id, len;
>> -char *blob_data;
>> +char *colon, *dash;
>>
>>  if (!path_blob)
>> -return -1;
>> +return FALSE;
>>
>> -blob_data = path_blob->data;
>> -/* we only handle MST paths for now */
>> -if (strncmp(blob_data, "mst:", 4))
>> -return -1;
>> +colon = strchr(path_blob->data, ':');
>> +dash = strchr(path_blob->data, '-');
>>
> Won't the removal of "mst" cause false positives - surely there are
> !MST blobs, right?
> If it's safe a comment will help a lot.

Yes, this property blob is always prefixed with "mst:" and I've added
a comment to the code.
(By skipping the prefix check, we are safe for the future, i.e. if the
prefix becomes mst3k. ;) )

>> -conn = strchr(blob_data + 4, '-');
>> -if (!conn)
>> -return -1;
>> -len = conn - (blob_data + 4);
>> -if (len + 1> 5)
>> -return -1;
>> -memcpy(conn_id, blob_data + 4, len);
>> -conn_id[len + 1] = '\0';
>> -id = strtoul(conn_id, NULL, 10);
>> +if (colon && dash) {
>> +*conn_base_id = strtoul(colon + 1, NULL, 10);
>> +*extra_path = dash + 1;
>>
>> -*conn_base_id = id;
>> +return TRUE;
>> +}
>>
>> -*path = conn + 1;
>> -return 0;
>> +return FALSE;
>>  }
>>
>>  static void
>>  drmmode_create_name(ScrnInfoPtr pScrn, drmModeConnectorPtr koutput, char 
>> *name,
>> -   drmModePropertyBlobPtr path_blob)
>> +drmModePropertyBlobPtr path_blob)
> Unrelated whitespace change.
>
>>  {
>> -int ret;
>>  char *extra_path;
>>  int conn_id;
>>  xf86OutputPtr output;
>>
>> -ret = parse_path_blob(path_blob, _id, _path);
>> -if (ret == -1)
>> +if (!parse_path_blob(path_blob, _id, _path))
> The function name doesn't imply a boolean return value, so I'd stick with int.

I've reverted the unnecessary changes.
___
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/11] xfree86: Fix set but not used warnings in lnx_platform

[Daniel Martin]

On 7 November 2017 at 13:57, Emil Velikov  wrote:
> On 7 November 2017 at 09:38, Daniel Martin  wrote:
>> ../hw/xfree86/os-support/linux/lnx_platform.c: In function ‘get_drm_info’:
>> ../hw/xfree86/os-support/linux/lnx_platform.c:29:16: warning: variable 
>> ‘minor’ set but not used [-Wunused-but-set-variable]
>>  int major, minor, fd;
>> ^
>> ../hw/xfree86/os-support/linux/lnx_platform.c:29:9: warning: variable 
>> ‘major’ set but not used [-Wunused-but-set-variable]
>>  int major, minor, fd;
>>  ^
>>
> It's not immediately obvious so I'd add some meat to the commit message.
>
> "When building w/o systemd, the functions ... are macros which don't
> use the A/B arguments.
> This leads to the following build warning:
>
>  warning messages ...
> "
>
> With that
> Reviewed-by: Emil Velikov 

Thanks, I've added the comments.
___
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