Re: [PATCH xserver] modesetting: Fix up some XXX from removing GLAMOR_HAS_DRM_*

[Daniel Stone]

Hi Mario,

On 29 March 2018 at 00:47, Mario Kleiner  wrote:
> On Thu, Mar 22, 2018 at 7:47 PM, Adam Jackson  wrote:
>> @@ -2244,18 +2247,19 @@ drmmode_output_dpms(xf86OutputPtr output, int mode)
>>  {
>>  drmmode_output_private_ptr drmmode_output = output->driver_private;
>>  xf86CrtcPtr crtc = output->crtc;
>> +modesettingPtr ms = modesettingPTR(crtc->scrn);
>
> --> This assignment above makes the X-Server crash, if output->crtc is
> NULL, and therefore crtc->scrn is a NULL ptr deref.
> output->crtc == NULL can happen from some call paths, e.g., whenever
> xf86DisableUnusedFunctions() gets called and decides to dpms off an
> output.

Ugh. It looks like we should be able to just directly use output->scrn
instead of crtc->scrn in that offending line; does that work for you?

Cheers,
Daniel
___
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: Fix up some XXX from removing GLAMOR_HAS_DRM_*

[Mario Kleiner]

A new crasher, this time while trying to play with the non-desktop property.

On Thu, Mar 22, 2018 at 7:47 PM, Adam Jackson  wrote:
> Signed-off-by: Adam Jackson 
> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 75 
> +---
>  hw/xfree86/drivers/modesetting/present.c |  1 -
>  2 files changed, 41 insertions(+), 35 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 6f636ba7bd..47c11adced 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c

 snip ...

> @@ -2244,18 +2247,19 @@ drmmode_output_dpms(xf86OutputPtr output, int mode)
>  {
>  drmmode_output_private_ptr drmmode_output = output->driver_private;
>  xf86CrtcPtr crtc = output->crtc;
> +modesettingPtr ms = modesettingPTR(crtc->scrn);

--> This assignment above makes the X-Server crash, if output->crtc is
NULL, and therefore crtc->scrn is a NULL ptr deref.
output->crtc == NULL can happen from some call paths, e.g., whenever
xf86DisableUnusedFunctions() gets called and decides to dpms off an
output.

[ 12732.278] (EE) Backtrace:
[ 12732.278] (EE) 0: /usr/bin/X (OsSigHandler+0x29) [0x591c19]
[ 12732.279] (EE) 1: /lib/x86_64-linux-gnu/libpthread.so.0
(__restore_rt+0x0) [0x7f3ecf20d38f]
[ 12732.280] (EE) 2:
/usr/local/lib/xorg/modules/drivers/modesetting_drv.so
(drmmode_output_dpms+0x10) [0x7f3ecb2bcc60]
[ 12732.280] (EE) 3: /usr/bin/X (xf86DisableUnusedFunctions+0x13c) [0x4b10dc]
[ 12732.280] (EE) 4: /usr/bin/X (xf86RandR12CrtcSet+0x4ca) [0x4b941a]
[ 12732.281] (EE) 5: /usr/bin/X (RRCrtcSet+0x2ce) [0x4f2eee]
[ 12732.281] (EE) 6: /usr/bin/X (ProcRRSetCrtcConfig+0x420) [0x4f4740]
[ 12732.282] (EE) 7: /usr/bin/X (Dispatch+0x28b) [0x43e41b]
[ 12732.282] (EE) 8: /usr/bin/X (dix_main+0x398) [0x4424f8]
[ 12732.283] (EE) 9: /lib/x86_64-linux-gnu/libc.so.6
(__libc_start_main+0xf0) [0x7f3ecee52830]
[ 12732.283] (EE) 10: /usr/bin/X (_start+0x29) [0x42c4e9]
[ 12732.284] (EE) 11: ? (?+0x0) [0x0]
[ 12732.284] (EE)
[ 12732.284] (EE) Segmentation fault at address 0x8

How i triggered this in my case:

1. Have a VR HMD connected during server startup, which gets set to
"Output disconnected" due to having the new "non-desktop" property set
to true,
therefore the HMD on HDMI-3 stays dark.

2. xrandr --output HDMI-3 --set 'non-desktop' 0because i want to
enable the HMD on my desktop to use it without a dedicated VR
compositor.
3. Boom!

thanks,
-mario
___
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: Fix up some XXX from removing GLAMOR_HAS_DRM_*

[Emil Velikov]

Reviewed-by: Emil Velikov 

Thanks!
Emil
___
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: Fix up some XXX from removing GLAMOR_HAS_DRM_*

[Adam Jackson]

Signed-off-by: Adam Jackson 
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 75 +---
 hw/xfree86/drivers/modesetting/present.c |  1 -
 2 files changed, 41 insertions(+), 35 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 6f636ba7bd..47c11adced 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -1632,8 +1632,11 @@ drmmode_crtc_destroy(xf86CrtcPtr crtc)
 {
 drmmode_mode_ptr iterator, next;
 drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+modesettingPtr ms = modesettingPTR(crtc->scrn);
+
+if (!ms->atomic_modeset)
+return;
 
-// XXX: if (!...->atomic_modeset) return;
 drmmode_prop_info_free(drmmode_crtc->props_plane, DRMMODE_PLANE__COUNT);
 xorg_list_for_each_entry_safe(iterator, next, _crtc->mode_list, 
entry) {
 drm_mode_destroy(crtc, iterator);
@@ -1872,7 +1875,6 @@ drmmode_crtc_create_planes(xf86CrtcPtr crtc, int num)
 drmmode_crtc->num_formats = best_kplane->count_formats;
 drmmode_crtc->formats = calloc(sizeof(drmmode_format_rec),
best_kplane->count_formats);
-// XXX: Runtime check modifiers_supported or it's implied by the 
successfull blob creation?
 if (blob_id) {
 populate_format_modifiers(crtc, best_kplane, blob_id);
 }
@@ -1894,6 +1896,7 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, 
drmModeResPtr mode_res
 xf86CrtcPtr crtc;
 drmmode_crtc_private_ptr drmmode_crtc;
 modesettingEntPtr ms_ent = ms_ent_priv(pScrn);
+modesettingPtr ms = modesettingPTR(pScrn);
 drmModeObjectPropertiesPtr props;
 static const drmmode_prop_info_rec crtc_props[] = {
 [DRMMODE_CRTC_ACTIVE] = { .name = "ACTIVE" },
@@ -1911,20 +1914,20 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_res
 drmmode_crtc->vblank_pipe = drmmode_crtc_vblank_pipe(num);
 xorg_list_init(_crtc->mode_list);
 
-// XXX: if (...->atomic_modeset) {
-props = drmModeObjectGetProperties(drmmode->fd, mode_res->crtcs[num],
-   DRM_MODE_OBJECT_CRTC);
-if (!props || !drmmode_prop_info_copy(drmmode_crtc->props, crtc_props,
-  DRMMODE_CRTC__COUNT, 0)) {
-xf86CrtcDestroy(crtc);
-return 0;
-}
+if (ms->atomic_modeset) {
+props = drmModeObjectGetProperties(drmmode->fd, mode_res->crtcs[num],
+   DRM_MODE_OBJECT_CRTC);
+if (!props || !drmmode_prop_info_copy(drmmode_crtc->props, crtc_props,
+  DRMMODE_CRTC__COUNT, 0)) {
+xf86CrtcDestroy(crtc);
+return 0;
+}
 
-drmmode_prop_info_update(drmmode, drmmode_crtc->props,
- DRMMODE_CRTC__COUNT, props);
-drmModeFreeObjectProperties(props);
-drmmode_crtc_create_planes(crtc, num);
-// XXX: }
+drmmode_prop_info_update(drmmode, drmmode_crtc->props,
+ DRMMODE_CRTC__COUNT, props);
+drmModeFreeObjectProperties(props);
+drmmode_crtc_create_planes(crtc, num);
+}
 
 /* Hide any cursors which may be active from previous users */
 drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, 0, 0, 0);
@@ -2244,18 +2247,19 @@ drmmode_output_dpms(xf86OutputPtr output, int mode)
 {
 drmmode_output_private_ptr drmmode_output = output->driver_private;
 xf86CrtcPtr crtc = output->crtc;
+modesettingPtr ms = modesettingPTR(crtc->scrn);
 drmModeConnectorPtr koutput = drmmode_output->mode_output;
 drmmode_ptr drmmode = drmmode_output->drmmode;
 
 if (!koutput)
 return;
 
-// XXX: if (...->atomic_modeset) {
-drmmode_output->dpms = mode;
-// XXX: } else {
-drmModeConnectorSetProperty(drmmode->fd, koutput->connector_id,
-drmmode_output->dpms_enum_id, mode);
-// XXX: }
+if (ms->atomic_modeset) {
+drmmode_output->dpms = mode;
+} else {
+drmModeConnectorSetProperty(drmmode->fd, koutput->connector_id,
+drmmode_output->dpms_enum_id, mode);
+}
 
 if (crtc) {
 drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
@@ -2596,6 +2600,7 @@ drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_r
 {
 xf86OutputPtr output;
 xf86CrtcConfigPtr   xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
+modesettingPtr ms = modesettingPTR(pScrn);
 drmModeConnectorPtr koutput;
 drmModeEncoderPtr *kencoders = NULL;
 drmmode_output_private_ptr drmmode_output;
@@ -2696,20 +2701,22 @@ drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_r
 /* work out the