Looks good! One nitpick I'm not 100% sure about:
On Thu, 2018-03-01 at 15:38 -0800, Keith Packard wrote:
> 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
>
> Signed-off-by: Keith Packard <kei...@keithp.com>
> ---
> 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 | 22 ++++++++++++++++++++++
> 5 files changed, 51 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 ec2aa9a27..1d84e113d 100644
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -36,6 +36,7 @@
> #include <unistd.h>
> #include <fcntl.h>
> #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, 0);
> else {
> @@ -822,6 +836,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) {
> if (pEnt->location.id.plat->flags & XF86_PDEV_SERVER_FD)
> @@ -1495,6 +1515,9 @@ SetMaster(ScrnInfoPtr pScrn)
> (ms->pEnt->location.id.plat->flags & XF86_PDEV_SERVER_FD))
> return TRUE;
>
> + if (ms->fd_passed)
> + return TRUE;
> +
> ret = drmSetMaster(ms->fd);
> if (ret)
> xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "drmSetMaster failed: %s\n",
> @@ -1746,7 +1769,8 @@ LeaveVT(ScrnInfoPtr pScrn)
> (ms->pEnt->location.id.plat->flags & XF86_PDEV_SERVER_FD))
> return;
>
> - drmDropMaster(ms->fd);
> + if (!ms->fd_passed)
> + drmDropMaster(ms->fd);
> }
>
> /*
> diff --git a/hw/xfree86/drivers/modesetting/driver.h
> b/hw/xfree86/drivers/modesetting/driver.h
> index fe835918b..6be51e01b 100644
> --- a/hw/xfree86/drivers/modesetting/driver.h
> +++ b/hw/xfree86/drivers/modesetting/driver.h
> @@ -84,6 +84,7 @@ struct ms_drm_queue {
>
> typedef struct _modesettingRec {
> int fd;
> + Bool fd_passed;
>
> int Chipset;
> EntityInfoPtr pEnt;
> diff --git a/hw/xfree86/os-support/linux/lnx_init.c b/hw/xfree86/os-
> support/linux/lnx_init.c
> index 9e5ddcd50..b654f7618 100644
> --- a/hw/xfree86/os-support/linux/lnx_init.c
> +++ b/hw/xfree86/os-support/linux/lnx_init.c
> @@ -356,6 +356,13 @@ xf86CloseConsole(void)
> close(xf86Info.consoleFd); /* make the vt-manager happy */
> }
>
> +#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?
> \
> + }
> +
> int
> xf86ProcessArgument(int argc, char *argv[], int i)
> {
> @@ -376,6 +383,19 @@ xf86ProcessArgument(int argc, char *argv[], int i)
> }
> return 1;
> }
> +
> + if (!strcmp(argv[i], "-masterfd")) {
> + CHECK_FOR_REQUIRED_ARGUMENT();
> + if (xf86PrivsElevated())
> + FatalError("\nCannot specify -masterfd when server is
> setuid/setgid\n");
> + if (sscanf(argv[++i], "%d", &xf86DRMMasterFd) != 1) {
> + UseMsg();
> + xf86DRMMasterFd = -1;
> + return 0;
> + }
> + return 2;
> + }
> +
> return 0;
> }
>
> @@ -385,4 +405,6 @@ xf86UseMsg(void)
> ErrorF("vtXX use the specified VT number\n");
> ErrorF("-keeptty ");
> ErrorF("don't detach controlling tty (for debugging only)\n");
> + if (!xf86PrivsElevated())
> + ErrorF("-masterfd <fd> 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
> }
--
Cheers,
Lyude Paul
_______________________________________________
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
