date:20160811

Re: [PATCH xserver] mi: Remove spurious call to OsReleaseSignals from mieqGrowQueue

2016-08-11 Thread Keith Packard

Peter Hutterer  writes:

> On Thu, Aug 11, 2016 at 09:35:35PM -0700, Keith Packard wrote:
>> This call wasn't converted to 'input_unlock()' when the SIGIO code was
>> removed from the server, and so when the queue growing was reworked to
>> be done from the input thread, it got left sitting here. As the caller
>> now manages the lock, we don't need to switch this to input_unlock at
>> this point.
>> 
>> Signed-off-by: Keith Packard 
>
> Reviewed-by: Peter Hutterer 
> without the configure.ac change, which I generally approve of too
> though.

Yeah, that's been sent separately; perhaps eric will see it tomorrow :-)

Thanks.

-- 
-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] mi: Remove spurious call to OsReleaseSignals from mieqGrowQueue

2016-08-11 Thread Peter Hutterer

On Thu, Aug 11, 2016 at 09:35:35PM -0700, Keith Packard wrote:
> This call wasn't converted to 'input_unlock()' when the SIGIO code was
> removed from the server, and so when the queue growing was reworked to
> be done from the input thread, it got left sitting here. As the caller
> now manages the lock, we don't need to switch this to input_unlock at
> this point.
> 
> Signed-off-by: Keith Packard 

Reviewed-by: Peter Hutterer 
without the configure.ac change, which I generally approve of too though.

Cheers,
   Peter

> ---
>  configure.ac | 17 +++--
>  mi/mieq.c|  1 -
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 690035a..e206e0f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -654,7 +654,7 @@ AC_ARG_ENABLE(xquartz,
> AS_HELP_STRING([--enable-xquartz], [Build Xquartz
>  AC_ARG_ENABLE(xwayland,   AS_HELP_STRING([--enable-xwayland], [Build 
> Xwayland server (default: auto)]), [XWAYLAND=$enableval], [XWAYLAND=auto])
>  AC_ARG_ENABLE(standalone-xpbproxy, 
> AS_HELP_STRING([--enable-standalone-xpbproxy], [Build a standalone xpbproxy 
> (in addition to the one integrated into Xquartz as a separate thread) 
> (default: no)]), [STANDALONE_XPBPROXY=$enableval], [STANDALONE_XPBPROXY=no])
>  AC_ARG_ENABLE(xwin,AS_HELP_STRING([--enable-xwin], [Build 
> XWin server (default: auto)]), [XWIN=$enableval], [XWIN=auto])
> -AC_ARG_ENABLE(glamor, AS_HELP_STRING([--enable-glamor], [Build 
> glamor dix module (default: yes)]), [GLAMOR=$enableval], [GLAMOR=yes])
> +AC_ARG_ENABLE(glamor, AS_HELP_STRING([--enable-glamor], [Build 
> glamor dix module (default: auto)]), [GLAMOR=$enableval], [GLAMOR=auto])
>  dnl kdrive and its subsystems
>  AC_ARG_ENABLE(kdrive, AS_HELP_STRING([--enable-kdrive], [Build 
> kdrive servers (default: no)]), [KDRIVE=$enableval], [KDRIVE=no])
>  AC_ARG_ENABLE(xephyr, AS_HELP_STRING([--enable-xephyr], [Build the 
> kdrive Xephyr server (default: auto)]), [XEPHYR=$enableval], [XEPHYR=auto])
> @@ -2150,7 +2150,17 @@ AM_CONDITIONAL([XORG_BUS_PLATFORM], [test 
> "x$CONFIG_UDEV_KMS" = xyes])
>  AM_CONDITIONAL([XORG_DRIVER_MODESETTING], [test "x$XORG_DRIVER_MODESETTING" 
> = xyes])
>  
>  dnl glamor
> +if test "x$GLAMOR" = xauto; then
> + if test "x$XORG" = xyes; then
> + GLAMOR=yes
> + fi
> + if test "x$XEPHYR" = xyes; then
> + GLAMOR=yes
> + fi
> +fi
> +
>  AM_CONDITIONAL([GLAMOR], [test "x$GLAMOR" = xyes])
> +
>  if test "x$GLAMOR" = xyes; then
>   AC_DEFINE(GLAMOR, 1, [Build glamor])
>   PKG_CHECK_MODULES([GLAMOR], [epoxy])
> @@ -2163,8 +2173,11 @@ if test "x$GLAMOR" = xyes; then
>   [AC_DEFINE(GLAMOR_HAS_GBM_LINEAR, 1, [Have 
> GBM_BO_USE_LINEAR])], [],
>   [#include 
>#include ])
> + else
> + if test "x$XORG" = xyes; then
> + AC_MSG_ERROR([Glamor for Xorg requires $LIBGBM])
> + fi
>   fi
> -
>  fi
>  AM_CONDITIONAL([GLAMOR_EGL], [test "x$GBM" = xyes])
>  
> diff --git a/mi/mieq.c b/mi/mieq.c
> index 05447d6..e31e4f1 100644
> --- a/mi/mieq.c
> +++ b/mi/mieq.c
> @@ -160,7 +160,6 @@ mieqGrowQueue(EventQueuePtr eventQueue, size_t 
> new_nevents)
>  for (j = 0; j < i; j++)
>  FreeEventList(new_events[j].events, 1);
>  free(new_events);
> -OsReleaseSignals();
>  return FALSE;
>  }
>  new_events[i].events = evlist;
> -- 
> 2.8.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
> 
___
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] xfree86: fix unbalanced input_lock/unlock in xf86NewInputDevice()

2016-08-11 Thread Peter Hutterer

On Thu, Aug 11, 2016 at 09:27:29PM -0700, Keith Packard wrote:
> Peter Hutterer  writes:
> 
> > diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
> > index 42d0f32..054eb3e 100644
> > --- a/hw/xfree86/common/xf86Xinput.c
> > +++ b/hw/xfree86/common/xf86Xinput.c
> > @@ -961,6 +961,7 @@ xf86NewInputDevice(InputInfoPtr pInfo, DeviceIntPtr 
> > *pdev, BOOL enable)
> >  xf86Msg(X_ERROR, "Couldn't init device \"%s\"\n", pInfo->name);
> >  RemoveDevice(dev, TRUE);
> >  rval = BadMatch;
> > +input_unlock();
> >  goto unwind;
> >  }
> >  /* send enter/leave event, update sprite window */
> > -- 
> > 2.7.4
> >
> 
> The remaining OsReleaseSignals call should have been a clue. That should
> be removed in this same patch. I think we could put the input_unlock right 
> where
> the OsReleaseSignals call is, but it is safer to place it where you
> have.
> 
> With the call to OsReleaseSignals removed, this is
> 
> Reviewed-by: Keith Packard 

I added a comment on why input_unlock() is later than OsReleaseSignals() and
pushed. thanks for the review.

Cheers,
   Peter



___
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] Build glamor when Xorg or Xephyr are built.

2016-08-11 Thread Keith Packard

Requires gbm when building Xorg so that xf86-video-modesetting will
work.

Signed-off-by: Keith Packard 
---
 configure.ac | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 2b93a4a..e206e0f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -654,7 +654,7 @@ AC_ARG_ENABLE(xquartz,
AS_HELP_STRING([--enable-xquartz], [Build Xquartz
 AC_ARG_ENABLE(xwayland,   AS_HELP_STRING([--enable-xwayland], [Build 
Xwayland server (default: auto)]), [XWAYLAND=$enableval], [XWAYLAND=auto])
 AC_ARG_ENABLE(standalone-xpbproxy, 
AS_HELP_STRING([--enable-standalone-xpbproxy], [Build a standalone xpbproxy (in 
addition to the one integrated into Xquartz as a separate thread) (default: 
no)]), [STANDALONE_XPBPROXY=$enableval], [STANDALONE_XPBPROXY=no])
 AC_ARG_ENABLE(xwin,  AS_HELP_STRING([--enable-xwin], [Build 
XWin server (default: auto)]), [XWIN=$enableval], [XWIN=auto])
-AC_ARG_ENABLE(glamor, AS_HELP_STRING([--enable-glamor], [Build glamor 
dix module (default: no)]), [GLAMOR=$enableval], [GLAMOR=no])
+AC_ARG_ENABLE(glamor, AS_HELP_STRING([--enable-glamor], [Build glamor 
dix module (default: auto)]), [GLAMOR=$enableval], [GLAMOR=auto])
 dnl kdrive and its subsystems
 AC_ARG_ENABLE(kdrive, AS_HELP_STRING([--enable-kdrive], [Build kdrive 
servers (default: no)]), [KDRIVE=$enableval], [KDRIVE=no])
 AC_ARG_ENABLE(xephyr, AS_HELP_STRING([--enable-xephyr], [Build the 
kdrive Xephyr server (default: auto)]), [XEPHYR=$enableval], [XEPHYR=auto])
@@ -2150,7 +2150,17 @@ AM_CONDITIONAL([XORG_BUS_PLATFORM], [test 
"x$CONFIG_UDEV_KMS" = xyes])
 AM_CONDITIONAL([XORG_DRIVER_MODESETTING], [test "x$XORG_DRIVER_MODESETTING" = 
xyes])
 
 dnl glamor
+if test "x$GLAMOR" = xauto; then
+   if test "x$XORG" = xyes; then
+   GLAMOR=yes
+   fi
+   if test "x$XEPHYR" = xyes; then
+   GLAMOR=yes
+   fi
+fi
+
 AM_CONDITIONAL([GLAMOR], [test "x$GLAMOR" = xyes])
+
 if test "x$GLAMOR" = xyes; then
AC_DEFINE(GLAMOR, 1, [Build glamor])
PKG_CHECK_MODULES([GLAMOR], [epoxy])
@@ -2163,8 +2173,11 @@ if test "x$GLAMOR" = xyes; then
[AC_DEFINE(GLAMOR_HAS_GBM_LINEAR, 1, [Have 
GBM_BO_USE_LINEAR])], [],
[#include 
 #include ])
+   else
+   if test "x$XORG" = xyes; then
+   AC_MSG_ERROR([Glamor for Xorg requires $LIBGBM])
+   fi
fi
-
 fi
 AM_CONDITIONAL([GLAMOR_EGL], [test "x$GBM" = xyes])
 
-- 
2.8.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] mi: Remove spurious call to OsReleaseSignals from mieqGrowQueue

2016-08-11 Thread Keith Packard

This call wasn't converted to 'input_unlock()' when the SIGIO code was
removed from the server, and so when the queue growing was reworked to
be done from the input thread, it got left sitting here. As the caller
now manages the lock, we don't need to switch this to input_unlock at
this point.

Signed-off-by: Keith Packard 
---
 configure.ac | 17 +++--
 mi/mieq.c|  1 -
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 690035a..e206e0f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -654,7 +654,7 @@ AC_ARG_ENABLE(xquartz,
AS_HELP_STRING([--enable-xquartz], [Build Xquartz
 AC_ARG_ENABLE(xwayland,   AS_HELP_STRING([--enable-xwayland], [Build 
Xwayland server (default: auto)]), [XWAYLAND=$enableval], [XWAYLAND=auto])
 AC_ARG_ENABLE(standalone-xpbproxy, 
AS_HELP_STRING([--enable-standalone-xpbproxy], [Build a standalone xpbproxy (in 
addition to the one integrated into Xquartz as a separate thread) (default: 
no)]), [STANDALONE_XPBPROXY=$enableval], [STANDALONE_XPBPROXY=no])
 AC_ARG_ENABLE(xwin,  AS_HELP_STRING([--enable-xwin], [Build 
XWin server (default: auto)]), [XWIN=$enableval], [XWIN=auto])
-AC_ARG_ENABLE(glamor, AS_HELP_STRING([--enable-glamor], [Build glamor 
dix module (default: yes)]), [GLAMOR=$enableval], [GLAMOR=yes])
+AC_ARG_ENABLE(glamor, AS_HELP_STRING([--enable-glamor], [Build glamor 
dix module (default: auto)]), [GLAMOR=$enableval], [GLAMOR=auto])
 dnl kdrive and its subsystems
 AC_ARG_ENABLE(kdrive, AS_HELP_STRING([--enable-kdrive], [Build kdrive 
servers (default: no)]), [KDRIVE=$enableval], [KDRIVE=no])
 AC_ARG_ENABLE(xephyr, AS_HELP_STRING([--enable-xephyr], [Build the 
kdrive Xephyr server (default: auto)]), [XEPHYR=$enableval], [XEPHYR=auto])
@@ -2150,7 +2150,17 @@ AM_CONDITIONAL([XORG_BUS_PLATFORM], [test 
"x$CONFIG_UDEV_KMS" = xyes])
 AM_CONDITIONAL([XORG_DRIVER_MODESETTING], [test "x$XORG_DRIVER_MODESETTING" = 
xyes])
 
 dnl glamor
+if test "x$GLAMOR" = xauto; then
+   if test "x$XORG" = xyes; then
+   GLAMOR=yes
+   fi
+   if test "x$XEPHYR" = xyes; then
+   GLAMOR=yes
+   fi
+fi
+
 AM_CONDITIONAL([GLAMOR], [test "x$GLAMOR" = xyes])
+
 if test "x$GLAMOR" = xyes; then
AC_DEFINE(GLAMOR, 1, [Build glamor])
PKG_CHECK_MODULES([GLAMOR], [epoxy])
@@ -2163,8 +2173,11 @@ if test "x$GLAMOR" = xyes; then
[AC_DEFINE(GLAMOR_HAS_GBM_LINEAR, 1, [Have 
GBM_BO_USE_LINEAR])], [],
[#include 
 #include ])
+   else
+   if test "x$XORG" = xyes; then
+   AC_MSG_ERROR([Glamor for Xorg requires $LIBGBM])
+   fi
fi
-
 fi
 AM_CONDITIONAL([GLAMOR_EGL], [test "x$GBM" = xyes])
 
diff --git a/mi/mieq.c b/mi/mieq.c
index 05447d6..e31e4f1 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -160,7 +160,6 @@ mieqGrowQueue(EventQueuePtr eventQueue, size_t new_nevents)
 for (j = 0; j < i; j++)
 FreeEventList(new_events[j].events, 1);
 free(new_events);
-OsReleaseSignals();
 return FALSE;
 }
 new_events[i].events = evlist;
-- 
2.8.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] xfree86: lock input during PreInit

2016-08-11 Thread Keith Packard

Peter Hutterer  writes:

> Signed-off-by: Peter Hutterer 

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] xfree86: fix unbalanced input_lock/unlock in xf86NewInputDevice()

2016-08-11 Thread Keith Packard

Peter Hutterer  writes:

> diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
> index 42d0f32..054eb3e 100644
> --- a/hw/xfree86/common/xf86Xinput.c
> +++ b/hw/xfree86/common/xf86Xinput.c
> @@ -961,6 +961,7 @@ xf86NewInputDevice(InputInfoPtr pInfo, DeviceIntPtr 
> *pdev, BOOL enable)
>  xf86Msg(X_ERROR, "Couldn't init device \"%s\"\n", pInfo->name);
>  RemoveDevice(dev, TRUE);
>  rval = BadMatch;
> +input_unlock();
>  goto unwind;
>  }
>  /* send enter/leave event, update sprite window */
> -- 
> 2.7.4
>

The remaining OsReleaseSignals call should have been a clue. That should
be removed in this same patch. I think we could put the input_unlock right where
the OsReleaseSignals call is, but it is safer to place it where you
have.

With the call to OsReleaseSignals removed, this is

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

[PATCH xserver] xfree86: lock input during PreInit

2016-08-11 Thread Peter Hutterer

This is a problem for the libinput driver that uses the same context across
multiple devices. The driver may be halfway through setting up an input device
(and the only way to do so is to add it to libinput) when the input thread
comes in an reads events. This then causes mayhem when data is dereferenced
that hasn't been set up yet.

In my case the cause was the call to libinput_path_remove_device() inside
preinit racing with evdev_dispatch_device() handling of ENODEV. The sequence
was:
- thread 2 gets an event and calls evdev_dispatch_device()
- thread 1 calls libinput_path_remove_device() which sets the device->source
  to NULL
- thread 2 reads from the fd, gets ENODEV and now removes the device->source,
  dereferencing the null-pointer

This is the one I could reproduce the most, but there are other potential
pitfalls that affect any driver that uses the same fd for multiple devices.
Avoid all this and wrap PreInit into the lock.

Signed-off-by: Peter Hutterer 
---
 hw/xfree86/common/xf86Xinput.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
index 054eb3e..6359e37 100644
--- a/hw/xfree86/common/xf86Xinput.c
+++ b/hw/xfree86/common/xf86Xinput.c
@@ -926,7 +926,9 @@ xf86NewInputDevice(InputInfoPtr pInfo, DeviceIntPtr *pdev, 
BOOL enable)
 
 xf86AddInput(drv, pInfo);
 
+input_lock();
 rval = drv->PreInit(drv, pInfo, 0);
+input_unlock();
 
 if (rval != Success) {
 xf86Msg(X_ERROR, "PreInit returned %d for \"%s\"\n", rval, 
pInfo->name);
-- 
2.7.4

___
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: fix unbalanced input_lock/unlock in xf86NewInputDevice()

2016-08-11 Thread Peter Hutterer

If a device couldn't be enabled we left the lock hanging.

Signed-off-by: Peter Hutterer 
---
 hw/xfree86/common/xf86Xinput.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
index 42d0f32..054eb3e 100644
--- a/hw/xfree86/common/xf86Xinput.c
+++ b/hw/xfree86/common/xf86Xinput.c
@@ -961,6 +961,7 @@ xf86NewInputDevice(InputInfoPtr pInfo, DeviceIntPtr *pdev, 
BOOL enable)
 xf86Msg(X_ERROR, "Couldn't init device \"%s\"\n", pInfo->name);
 RemoveDevice(dev, TRUE);
 rval = BadMatch;
+input_unlock();
 goto unwind;
 }
 /* send enter/leave event, update sprite window */
-- 
2.7.4

___
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: don't re-remove an already removed fd

2016-08-11 Thread Peter Hutterer

On Thu, Aug 11, 2016 at 12:39:46PM -0700, Keith Packard wrote:
> Peter Hutterer  writes:
> 
> > that's the plan for the driver, but for now the current behaviour is a
> > change to what used to work. And it's a bit confusing too -
> > xf86AddEnabledDevice() takes a pInfo, not just an fd. So it looks like it
> > works on a per-device level and we should emulate that behaviour as best as
> > we can. Right now you can pass two different pInfos in and then you get to
> > bet whether you remove the same device twice or not.
> 
> I went and looked at the previous version of xf86AddEnabledDevice, and
> it supports multiple calls with the same fd by replacing the callback
> and args. We can do the same thing with InputThreadRegisterDev and make
> it actually compatible with the old API. Seems like a more sane plan
> than having duplicate fds in the input device list, only one of which
> actually gets used.

thanks, that does seem like the better plan.
and it didn't crash across lots of wacom replugs.

pushed2df2815..bf31d6f  master -> master

Cheers,
   Peter


> From 0ed596f16bde0f6df151db657c11973c457f4eb4 Mon Sep 17 00:00:00 2001
> From: Keith Packard 
> Date: Thu, 11 Aug 2016 12:34:54 -0700
> Subject: [PATCH xserver] os: Allow re-registering fd with
>  InputThreadRegisterDev
> 
> Calling InputThreadRegisterDev twice with the same fd should replace
> the existing function and args instead of creating a new entry with
> the same fd.
> 
> Signed-off-by: Keith Packard 
> ---
>  os/inputthread.c | 36 +---
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/os/inputthread.c b/os/inputthread.c
> index cb3af06..1cd1c2a 100644
> --- a/os/inputthread.c
> +++ b/os/inputthread.c
> @@ -188,24 +188,38 @@ InputThreadRegisterDev(int fd,
> NotifyFdProcPtr readInputProc,
> void *readInputArgs)
>  {
> -InputThreadDevice *dev;
> +InputThreadDevice *dev, *old;
>  
>  if (!inputThreadInfo)
>  return SetNotifyFd(fd, readInputProc, X_NOTIFY_READ, readInputArgs);
>  
> -dev = calloc(1, sizeof(InputThreadDevice));
> -if (dev == NULL) {
> -DebugF("input-thread: could not register device\n");
> -return 0;
> +input_lock();
> +
> +dev = NULL;
> +xorg_list_for_each_entry(old, >devs, node) {
> +if (old->fd == fd) {
> +dev = old;
> +break;
> +}
>  }
>  
> -dev->fd = fd;
> -dev->readInputProc = readInputProc;
> -dev->readInputArgs = readInputArgs;
> -dev->state = device_state_added;
> +if (dev) {
> +dev->readInputProc = readInputProc;
> +dev->readInputArgs = readInputArgs;
> +} else {
> +dev = calloc(1, sizeof(InputThreadDevice));
> +if (dev == NULL) {
> +DebugF("input-thread: could not register device\n");
> +input_unlock();
> +return 0;
> +}
>  
> -input_lock();
> -xorg_list_append(>node, >devs);
> +dev->fd = fd;
> +dev->readInputProc = readInputProc;
> +dev->readInputArgs = readInputArgs;
> +dev->state = device_state_added;
> +xorg_list_append(>node, >devs);
> +}
>  
>  inputThreadInfo->changed = TRUE;
>  
> -- 
> 2.8.1
> 

> 
> -- 
> -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

Re: [PATCH libpciaccess] Support for 32-bit domains

2016-08-11 Thread Keith Busch

On Thu, Aug 11, 2016 at 12:03:30PM -0700, Eric Anholt wrote:
> Given that libpciaccess allocates the struct, and the struct isn't
> embedded in any other public structs, I think you could just tack a
> "uint16_t domain_high;" at the end of the struct and fill that with the
> high bits.

Hmm, then we have to change every usage of domain to combine the two,
and every usage thereafter must do the same.

How about tacking 'uint32_t domain' to the end for the full domain,
rename the existing 'uint16_t domain' to 'domain_low', and then just
copy the lower bits from the full domain into there? That should satisfy
legacy usage, and everywhere else will automatically use the full value.

Does something like this look more reasonable?

---
diff --git a/include/pciaccess.h b/include/pciaccess.h
index 1d7aa4b..e095dfb 100644
--- a/include/pciaccess.h
+++ b/include/pciaccess.h
@@ -321,7 +321,7 @@ struct pci_device {
  * the domain will always be zero.
  */
 /*@{*/
-uint16_tdomain;
+uint16_tdomain_low;
 uint8_t bus;
 uint8_t dev;
 uint8_t func;
@@ -385,6 +385,12 @@ struct pci_device {
   * Used by the VGA arbiter. Type of resource decoded by the device
   * and
   * the file descriptor (/dev/vga_arbiter). */
 int vgaarb_rsrc;
+
+/**
+ * The 32-bit pci domain. The lower 16 bits are copied into the
+ * domain_low field for ABI compatibility.
+ */
+uint32_tdomain;
 };


diff --git a/src/linux_sysfs.c b/src/linux_sysfs.c
index 6367b11..c640f41 100644
--- a/src/linux_sysfs.c
+++ b/src/linux_sysfs.c
@@ -159,10 +159,11 @@ populate_entries( struct pci_system * p )
(struct pci_device_private *) >devices[i];


-   sscanf(devices[i]->d_name, "%04x:%02x:%02x.%1u",
+   sscanf(devices[i]->d_name, "%x:%02x:%02x.%1u",
   & dom, & bus, & dev, & func);

device->base.domain = dom;
+   device->base.domain_low = dom;
device->base.bus = bus;
device->base.dev = dev;
device->base.func = func;
--
___
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: don't re-remove an already removed fd

2016-08-11 Thread Keith Packard

Peter Hutterer  writes:

> that's the plan for the driver, but for now the current behaviour is a
> change to what used to work. And it's a bit confusing too -
> xf86AddEnabledDevice() takes a pInfo, not just an fd. So it looks like it
> works on a per-device level and we should emulate that behaviour as best as
> we can. Right now you can pass two different pInfos in and then you get to
> bet whether you remove the same device twice or not.

I went and looked at the previous version of xf86AddEnabledDevice, and
it supports multiple calls with the same fd by replacing the callback
and args. We can do the same thing with InputThreadRegisterDev and make
it actually compatible with the old API. Seems like a more sane plan
than having duplicate fds in the input device list, only one of which
actually gets used.

From 0ed596f16bde0f6df151db657c11973c457f4eb4 Mon Sep 17 00:00:00 2001
From: Keith Packard 
Date: Thu, 11 Aug 2016 12:34:54 -0700
Subject: [PATCH xserver] os: Allow re-registering fd with
 InputThreadRegisterDev

Calling InputThreadRegisterDev twice with the same fd should replace
the existing function and args instead of creating a new entry with
the same fd.

Signed-off-by: Keith Packard 
---
 os/inputthread.c | 36 +---
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/os/inputthread.c b/os/inputthread.c
index cb3af06..1cd1c2a 100644
--- a/os/inputthread.c
+++ b/os/inputthread.c
@@ -188,24 +188,38 @@ InputThreadRegisterDev(int fd,
NotifyFdProcPtr readInputProc,
void *readInputArgs)
 {
-InputThreadDevice *dev;
+InputThreadDevice *dev, *old;
 
 if (!inputThreadInfo)
 return SetNotifyFd(fd, readInputProc, X_NOTIFY_READ, readInputArgs);
 
-dev = calloc(1, sizeof(InputThreadDevice));
-if (dev == NULL) {
-DebugF("input-thread: could not register device\n");
-return 0;
+input_lock();
+
+dev = NULL;
+xorg_list_for_each_entry(old, >devs, node) {
+if (old->fd == fd) {
+dev = old;
+break;
+}
 }
 
-dev->fd = fd;
-dev->readInputProc = readInputProc;
-dev->readInputArgs = readInputArgs;
-dev->state = device_state_added;
+if (dev) {
+dev->readInputProc = readInputProc;
+dev->readInputArgs = readInputArgs;
+} else {
+dev = calloc(1, sizeof(InputThreadDevice));
+if (dev == NULL) {
+DebugF("input-thread: could not register device\n");
+input_unlock();
+return 0;
+}
 
-input_lock();
-xorg_list_append(>node, >devs);
+dev->fd = fd;
+dev->readInputProc = readInputProc;
+dev->readInputArgs = readInputArgs;
+dev->state = device_state_added;
+xorg_list_append(>node, >devs);
+}
 
 inputThreadInfo->changed = TRUE;
 
-- 
2.8.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

Re: [PATCH libpciaccess] Support for 32-bit domains

2016-08-11 Thread Eric Anholt

Michel Dänzer  writes:

> On 10/08/16 06:39 AM, Keith Busch wrote:
>> A pci "domain" is a purely software construct, and need not be limited
>> to the 16-bit ACPI defined segment. The Linux kernel currently supports
>> 32-bit domains, so this patch matches up with those capabilities to make
>> it usable on systems exporting such domains.
>> 
>> Reported-by: Pawel Baldysiak 
>> Signed-off-by: Keith Busch 
>> ---
>>  include/pciaccess.h | 2 +-
>>  src/linux_sysfs.c   | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/pciaccess.h b/include/pciaccess.h
>> index 1d7aa4b..93ed76f 100644
>> --- a/include/pciaccess.h
>> +++ b/include/pciaccess.h
>> @@ -321,7 +321,7 @@ struct pci_device {
>>   * the domain will always be zero.
>>   */
>>  /*@{*/
>> -uint16_tdomain;
>> +uint32_tdomain;
>
> This change breaks ABI, so as is it would require bumping the library
> SONAME (which is painful and thus generally discouraged from a
> downstream POV).

Given that libpciaccess allocates the struct, and the struct isn't
embedded in any other public structs, I think you could just tack a
"uint16_t domain_high;" at the end of the struct and fill that with the
high bits.


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 libpciaccess] Support for 32-bit domains

2016-08-11 Thread Mark Kettenis

> Date: Wed, 10 Aug 2016 22:58:34 +
> From: Keith Busch 
> 
> On Thu, Aug 11, 2016 at 12:17:48AM +0200, Mark Kettenis wrote:
> > > From: Keith Busch 
> > > Date: Tue,  9 Aug 2016 15:39:35 -0600
> > > 
> > > A pci "domain" is a purely software construct, and need not be limited
> > > to the 16-bit ACPI defined segment. The Linux kernel currently supports
> > > 32-bit domains, so this patch matches up with those capabilities to make
> > > it usable on systems exporting such domains.
> > 
> > Well, yes, and no.  PCI domains really are a hardware property.  There
> > are systems out there that have multiple PCI host bridges, each with
> > their own separate config/mem/io address spaces and bus numbers
> > starting with 0 for the root of the PCI bus hierarchy.  Pretty much
> > any 64-bit SPARC system falls into this category, and I've seen
> > PA-RISC and POWER systems with such a hardware configuration as well.
> > And given that HP's Itanium line developed from their PA-RISC hardware
> > I expect them to be in the same boat.  There is no domain numering
> > scheme that is implied by the hardware though, do domain numbers are
> > indeed purely a software construct.  On OpenBSD we simply number the
> > domains sequentially.  So 16 bits are more than enough.
> > 
> > The Linux kernel could do the same with ACPI segments (which may or
> > may not map onto true PCI domains).  That would remove the need to
> > change te libpciaccess ABI.  Although I can see that having a 1:1
> > mapping of ACPI segments to domains is something that is nice to have.
> 
> I can give a little more background on where this is coming from. The
> Intel x86 Skylake E-Series has an option to provide a number of additional
> "host bridges". The "vmd" driver in the Linux mainline kernel supports
> this hardware.
> 
> For better or worse, Linux does match the segment number to the
> domain. The "vmd" hardware is not a segment though, and decoupling _SEG
> from domain numbers in the Linux kernel proved difficult and unpopular
> with the devs. To avoid the potential clash from letting vmd hardware
> occupy the same range that an ACPI _SEG could define, we let VMD start
> at 0x1.
> 
> I've already patched pci-utils (provides lspci, setpci) to allow
> this, but I missed this library at the time (my dev machines are all
> no-graphics). Right now, a system with VMD segfaults startx. I believe
> it's down to the error handling that frees the pci devices and sets
> pci_system->devices to NULL. It looks like this is dereferenced later,
> but I'm very unfamiliar with the code base and not sure which repo to
> look into.
> 
> If preserving libpciaccess ABI is of high importance, I think the only
> other option is to just ignore domains requiring 32-bits.  That should
> be okay for us since X should not need the devices in these domains
> anyway. I'll send a patch for consideration.

To be honest, bumping the shared library major is perfectly fine with
me.  The current "thou shalt never bump the shared library major"
mantra that seems to has taken hold of the Linux community makes no
sense.  Why have a shared library major at all if you can never bump
it?

In any case the impact of bumping the libpciaccess shared library
should be fairly limited as it's not widely used outside of X.  But I
fear it does affect the driver API.

> > However, I do believe that ACPI segments are actually encoded as
> > 64-bit integers.  So such a 1:1 mapping may not be achievable.
> 
> True, though only 16 bits are used (ACPI 6, section 6.5.6):
> 
>   "
>   The lower 16 bits of _SEG returned integer is the PCI Segment Group number.
>   Other bits are reserved.
>   "

At least whoever designed the libpciaccess interface had some reason
to pick a 16-bit integer for the domain.

___
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 libpciaccess] Support for 32-bit domains

2016-08-11 Thread Keith Busch

On Wed, Aug 10, 2016 at 10:09:14AM +0900, Michel Dänzer wrote:
> This change breaks ABI, so as is it would require bumping the library
> SONAME (which is painful and thus generally discouraged from a
> downstream POV).

Well, the current situation is such systems are unbootable without this
fix, so the pain from bumping for ABI sounds pretty tame in comparison. :)

I suspect, though, the boot halting segfault is probably a bug that can
be fixed elsewhere without supporting 32-bit domain devices. I'll see
if I can confirm that to be the case.
___
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: don't re-remove an already removed fd

2016-08-11 Thread Peter Hutterer

On Wed, Aug 10, 2016 at 10:52:03PM -0700, Keith Packard wrote:
> Peter Hutterer  writes:
> 
> > wacom does. and yes, it needs to get fixed in the driver now that it
> > doesn't work anymore but meanwhile not crashing is still good.
> 
> Right, wondering if simply not registering the duplicate FD would also work?

that's the plan for the driver, but for now the current behaviour is a
change to what used to work. And it's a bit confusing too -
xf86AddEnabledDevice() takes a pInfo, not just an fd. So it looks like it
works on a per-device level and we should emulate that behaviour as best as
we can. Right now you can pass two different pInfos in and then you get to
bet whether you remove the same device twice or not.

Cheers,
   Peter
___
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] mi: Remove spurious call to OsReleaseSignals from mieqGrowQueue

Re: [PATCH xserver] mi: Remove spurious call to OsReleaseSignals from mieqGrowQueue

Re: [PATCH xserver] xfree86: fix unbalanced input_lock/unlock in xf86NewInputDevice()

[PATCH xserver] Build glamor when Xorg or Xephyr are built.

[PATCH xserver] mi: Remove spurious call to OsReleaseSignals from mieqGrowQueue

Re: [PATCH xserver] xfree86: lock input during PreInit

Re: [PATCH xserver] xfree86: fix unbalanced input_lock/unlock in xf86NewInputDevice()

[PATCH xserver] xfree86: lock input during PreInit

[PATCH xserver] xfree86: fix unbalanced input_lock/unlock in xf86NewInputDevice()

Re: [PATCH xserver] os: don't re-remove an already removed fd

Re: [PATCH libpciaccess] Support for 32-bit domains

Re: [PATCH xserver] os: don't re-remove an already removed fd

Re: [PATCH libpciaccess] Support for 32-bit domains

Re: [PATCH libpciaccess] Support for 32-bit domains

Re: [PATCH libpciaccess] Support for 32-bit domains

Re: [PATCH xserver] os: don't re-remove an already removed fd

16 matches

Site Navigation

Mail list logo

Footer information