Re: [PATCH xserver] modesetting: Allow a DRM fd to be passed on command line with -masterfd

2018-06-28 Thread Lyude Paul 
On Thu, 2018-06-28 at 12:39 -0700, Keith Packard wrote:
> 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.

Makes sense!

Reviewed-by: Lyude Paul 
> 
> > > +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!
> 
> ___
> 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
-- 
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

Re: [PATCH xserver] modesetting: Allow a DRM fd to be passed on command line with -masterfd

2018-06-28 Thread 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)
 xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "drmSetMaster failed: %s\n",
@@ -1754,7 +1777,8 @@ LeaveVT(ScrnInfoPtr pScrn)
 return;
 #endif
 
-drmDropMaster(ms->fd);
+if (!ms->fd_passed)
+

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

2018-06-28 Thread 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

2018-06-28 Thread 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

RE: 4K@60 YCbCr420 missing mode in usermode

2018-06-28 Thread Zuo, Jerry 
Hi Mike:

 I’ve checked amd-staging-drm-next branch with the commit point 
“35d594ef218696edc92cf6a1175c1a4a9c7c2aa6” (4.18.0-rc1). Please checkout that 
commit, and apply the attached patch. ASUS PA328 display could only give 4K@30, 
and attached Xorg log shows YCbCr420 only 4K@60 modes are getting filtered out. 
The usermode I am using now is 607399. Thanks for your help.

Regards,
Jerry

From: Mike Lothian [mailto:m...@fireburn.co.uk]
Sent: June 27, 2018 11:36 AM
To: Zuo, Jerry 
Cc: Michel Dänzer ; Emil Velikov 
; xorg-devel@lists.x.org; Lipski, Mikita 
; Wentland, Harry 
Subject: Re: 4K@60 YCbCr420 missing mode in usermode

Hi

I'm using https://cgit.freedesktop.org/~agd5f/linux/log/?h=drm-next-4.19-wip

Cheers

Mike

On Wed, 27 Jun 2018 at 15:27 Zuo, Jerry 
mailto:jerry@amd.com>> wrote:
Hi Mike:

 The build I am using for debug is based on amd-staging-dal-drm-next branch 
with commit 624fa8daa6e10af0b7f74dc31a66c26c4fbb2926. Please let me know which 
build you are using. Thanks.

Regards,
Jerry

From: Mike Lothian [mailto:m...@fireburn.co.uk]
Sent: June 26, 2018 7:09 PM
To: Zuo, Jerry mailto:jerry@amd.com>>
Cc: Michel Dänzer mailto:mic...@daenzer.net>>; Emil Velikov 
mailto:emil.l.veli...@gmail.com>>; 
xorg-devel@lists.x.org; Lipski, Mikita 
mailto:mikita.lip...@amd.com>>; Wentland, Harry 
mailto:harry.wentl...@amd.com>>

Subject: Re: 4K@60 YCbCr420 missing mode in usermode

With the modesetting driver I would see this in my dmesg if it helps:

Jun 27 00:00:38 quark kernel: [ cut here ]
Jun 27 00:00:38 quark kernel: kernel BUG at 
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4797!
Jun 27 00:00:38 quark kernel: invalid opcode:  [#1] PREEMPT SMP NOPTI
Jun 27 00:00:38 quark kernel: CPU: 2 PID: 335 Comm: Xorg Tainted: GW
 4.18.0-rc1-agd5f+ #189
Jun 27 00:00:38 quark kernel: Hardware name: System manufacturer System Product 
Name/ROG STRIX X470-I GAMING, BIOS 0601 04/19/2018
Jun 27 00:00:38 quark kernel: RIP: 0010:dm_update_crtcs_state+0x45b/0x4e0
Jun 27 00:00:38 quark kernel: Code: ff ff 48 85 db 0f 84 ea fe ff ff 48 c7 44 
24 18 00 00 00 00 48 c7 44 24 08 00 00 00 00 48 c7 04 24 00 00 00 00 e9 34 fe 
ff ff <0f> 0b 48 83 c4 30 b8 ea ff ff ff 5b 5d 41 5c 41 5d 41 5e 41 5f c3
Jun 27 00:00:38 quark kernel: RSP: 0018:c94fbb08 EFLAGS: 00010246
Jun 27 00:00:38 quark kernel: RAX: 880818642401 RBX: 88081b354000 RCX: 

Jun 27 00:00:38 quark kernel: RDX: 22d2 RSI: 22d2 RDI: 
880818641800
Jun 27 00:00:38 quark kernel: RBP: 880806659f80 R08: 00022ac0 R09: 
815bf644
Jun 27 00:00:38 quark kernel: R10: ea0020619000 R11: 88083e806e80 R12: 

Jun 27 00:00:38 quark kernel: R13: 880818642000 R14: 880818646800 R15: 
88081b360800
Jun 27 00:00:38 quark kernel: FS:  7f5529f3df80() 
GS:88083ec8() knlGS:
Jun 27 00:00:38 quark kernel: CS:  0010 DS:  ES:  CR0: 80050033
Jun 27 00:00:38 quark kernel: CR2: 7fb043f26610 CR3: 000811394000 CR4: 
003406a0
Jun 27 00:00:38 quark kernel: Call Trace:
Jun 27 00:00:38 quark kernel:  ? amdgpu_dm_atomic_check+0x1b7/0x3d0
Jun 27 00:00:38 quark kernel:  ? drm_atomic_check_only+0x44d/0x640
Jun 27 00:00:38 quark kernel:  ? drm_atomic_set_property+0x6a/0x680
Jun 27 00:00:38 quark kernel:  ? drm_mode_atomic_ioctl+0x64c/0x9f0
Jun 27 00:00:38 quark kernel:  ? drm_atomic_set_property+0x680/0x680
Jun 27 00:00:38 quark kernel:  ? drm_ioctl_kernel+0x9c/0xe0
Jun 27 00:00:38 quark kernel:  ? drm_ioctl+0x1e1/0x380
Jun 27 00:00:38 quark kernel:  ? drm_atomic_set_property+0x680/0x680
Jun 27 00:00:38 quark kernel:  ? __set_current_blocked+0x38/0x50
Jun 27 00:00:38 quark kernel:  ? _raw_spin_unlock_irq+0xe/0x20
Jun 27 00:00:38 quark kernel:  ? amdgpu_drm_ioctl+0x44/0x80
Jun 27 00:00:38 quark kernel:  ? do_vfs_ioctl+0x9f/0x610
Jun 27 00:00:38 quark kernel:  ? recalc_sigpending+0x11/0x40
Jun 27 00:00:38 quark kernel:  ? _copy_from_user+0x37/0x60
Jun 27 00:00:38 quark kernel:  ? ksys_ioctl+0x35/0x70
Jun 27 00:00:38 quark kernel:  ? __x64_sys_ioctl+0x11/0x20
Jun 27 00:00:38 quark kernel:  ? do_syscall_64+0x43/0xf0
Jun 27 00:00:38 quark kernel:  ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
Jun 27 00:00:38 quark kernel: Modules linked in:
Jun 27 00:00:38 quark kernel: ---[ end trace c37f846a2d1ee561 ]---
Jun 27 00:00:38 quark kernel: RIP: 0010:dm_update_crtcs_state+0x45b/0x4e0
Jun 27 00:00:38 quark kernel: Code: ff ff 48 85 db 0f 84 ea fe ff ff 48 c7 44 
24 18 00 00 00 00 48 c7 44 24 08 00 00 00 00 48 c7 04 24 00 00 00 00 e9 34 fe 
ff ff <0f> 0b 48 83 c4 30 b8 ea ff ff ff 5b 5d 41 5c 41 5d 41 5e 41 5f c3
Jun 27 00:00:38 quark kernel: RSP: 0018:c94fbb08 EFLAGS: 00010246
Jun 27 00:00:38 quark kernel: RAX: 880818642401 RBX: 88081b354000 RCX: 

Jun 27 00:00:38 quark kernel: RDX: