Re: [PATCH 2/3] modesetting: add dynamic connector hotplug support (MST) (v2)
On 04/19/2015 08:52 PM, Dave Airlie wrote: On 14 April 2015 at 13:12, Keith Packard wrote: Dave Airlie writes: So we hot unplug, we remove the output XID from the server, in parallel the client does an operation with the output XID it has gotten already, and still believe is valid, and it gets BadMatch, and since hardly anyone handles X errors it falls over. Yeah, I was curious if you'd found an actual race condition that couldn't be solved by correctly written applications. Sounds like it's just 'buggy' clients. Given that there just aren't that many applications which deal with RandR objects, it's pretty tempting to let them experience reality for a year or so and expect that they'll get fixed. As I said, the alternative would be to amend the spec to say that outputs may get added, but will never go away. That would enshrine their behavior as compliant, and ensure that we'd never break them in the future. I can implement that. Frankly, I think I'd probably be OK with either plan, the only one I'm not up for is supporting both modes with a configuration parameter that no-one in their right mind would enable. This is what we have today in the nvidia driver. In practice, I don't think I've ever heard of anyone enabling the option to delete outputs, but it's also not a huge amount of code so no one has felt the need to rip it out yet. I'm sort of happy to just enshrine the outputs cannot be destroyed behaviour, Me too. But I do wonder how many things would need fixing, it might just be mutter and a few things like that, though if gtk falls over then we should probably leave things alone. The big one was gnome-settings-daemon on RHEL 5, IIRC. We have customers who run old versions of GNOME with very recent drivers and hardware, so unless someone invents a time machine, I don't think that configuration can be fixed. Aaron, any ideas on how prevalent the issue is, or if nvidia care enough, since I took the option from you! I don't have a strong preference either way. We'll probably keep the option in our implementation until we have a reason to delete it, but I'd be kind of surprised if anyone ever actually needed it. If RandR starts mandating that outputs never go away, I'll probably delete the option just in the interests of spec compliance. Dave. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] modesetting: add dynamic connector hotplug support (MST) (v2)
On 14 April 2015 at 13:12, Keith Packard wrote: > Dave Airlie writes: > >> So we hot unplug, we remove the output XID from the server, in >> parallel the client does an operation with the output XID it has >> gotten already, and still believe is valid, and it gets BadMatch, and >> since hardly anyone handles X errors it falls over. > > Yeah, I was curious if you'd found an actual race condition that > couldn't be solved by correctly written applications. Sounds like it's > just 'buggy' clients. > > Given that there just aren't that many applications which deal with > RandR objects, it's pretty tempting to let them experience reality for a > year or so and expect that they'll get fixed. > > As I said, the alternative would be to amend the spec to say that > outputs may get added, but will never go away. That would enshrine their > behavior as compliant, and ensure that we'd never break them in the > future. > > Frankly, I think I'd probably be OK with either plan, the only one I'm > not up for is supporting both modes with a configuration parameter that > no-one in their right mind would enable. I'm sort of happy to just enshrine the outputs cannot be destroyed behaviour, But I do wonder how many things would need fixing, it might just be mutter and a few things like that, though if gtk falls over then we should probably leave things alone. Aaron, any ideas on how prevalent the issue is, or if nvidia care enough, since I took the option from you! Dave. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] modesetting: add dynamic connector hotplug support (MST) (v2)
Dave Airlie writes: > So we hot unplug, we remove the output XID from the server, in > parallel the client does an operation with the output XID it has > gotten already, and still believe is valid, and it gets BadMatch, and > since hardly anyone handles X errors it falls over. Yeah, I was curious if you'd found an actual race condition that couldn't be solved by correctly written applications. Sounds like it's just 'buggy' clients. Given that there just aren't that many applications which deal with RandR objects, it's pretty tempting to let them experience reality for a year or so and expect that they'll get fixed. As I said, the alternative would be to amend the spec to say that outputs may get added, but will never go away. That would enshrine their behavior as compliant, and ensure that we'd never break them in the future. Frankly, I think I'd probably be OK with either plan, the only one I'm not up for is supporting both modes with a configuration parameter that no-one in their right mind would enable. -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] modesetting: add dynamic connector hotplug support (MST) (v2)
On 14 April 2015 at 00:51, Keith Packard wrote: > Dave Airlie writes: > >> It uses the same option name as nvidia and the other DDXes to >> disable tearing down outputs as it is hard to avoid racing with >> clients. > > I don't think having an option here is useful -- either we figure out > some way to make this reliable for clients, or we abandon the notion of > ever deleting the outputs and fix the protocol spec. > > Can someone describe the race condition seen with clients? If it's just > a bug in the clients, it seems like this is a fine opportunity to > encourage such bugs to be fixed. I don't think its really fixable, at least not in all the clients. The problem is racing between the hot unplug and the client doing something, So we hot unplug, we remove the output XID from the server , in parallel the client does an operation with the output XID it has gotten already, and still believe is valid, and it gets BadMatch, and since hardly anyone handles X errors it falls over. Dave. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/3] modesetting: add dynamic connector hotplug support (MST) (v2)
Dave Airlie writes: > It uses the same option name as nvidia and the other DDXes to > disable tearing down outputs as it is hard to avoid racing with > clients. I don't think having an option here is useful -- either we figure out some way to make this reliable for clients, or we abandon the notion of ever deleting the outputs and fix the protocol spec. Can someone describe the race condition seen with clients? If it's just a bug in the clients, it seems like this is a fine opportunity to encourage such bugs to be fixed. -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/3] modesetting: add dynamic connector hotplug support (MST) (v2)
From: Dave Airlie
This is ported from the same code in the ati and intel drivers,
It uses the same option name as nvidia and the other DDXes to
disable tearing down outputs as it is hard to avoid racing with clients.
v2: address two issues with DeleteUnusedDP12 enabled, reported
by Daniel Martin,
a) check we have a mode_output before destroying it
b) only delete *unused* displays (thanks Aaron for clarifying)
so we check if the output has a crtc and if it does we don't
delete it.
Signed-off-by: Dave Airlie
---
hw/xfree86/drivers/modesetting/driver.c | 6 +
hw/xfree86/drivers/modesetting/drmmode_display.c | 221 +--
hw/xfree86/drivers/modesetting/drmmode_display.h | 1 +
3 files changed, 212 insertions(+), 16 deletions(-)
diff --git a/hw/xfree86/drivers/modesetting/driver.c
b/hw/xfree86/drivers/modesetting/driver.c
index e2f3846..27612fa 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -123,6 +123,7 @@ typedef enum {
OPTION_DEVICE_PATH,
OPTION_SHADOW_FB,
OPTION_ACCEL_METHOD,
+OPTION_DELETE_DP_12_DISP,
} modesettingOpts;
static const OptionInfoRec Options[] = {
@@ -130,6 +131,7 @@ static const OptionInfoRec Options[] = {
{OPTION_DEVICE_PATH, "kmsdev", OPTV_STRING, {0}, FALSE},
{OPTION_SHADOW_FB, "ShadowFB", OPTV_BOOLEAN, {0}, FALSE},
{OPTION_ACCEL_METHOD, "AccelMethod", OPTV_STRING, {0}, FALSE},
+{OPTION_DELETE_DP_12_DISP, "DeleteUnusedDP12Displays", OPTV_BOOLEAN, {0},
FALSE},
{-1, NULL, OPTV_NONE, {0}, FALSE}
};
@@ -768,6 +770,10 @@ PreInit(ScrnInfoPtr pScrn, int flags)
ms->drmmode.sw_cursor = TRUE;
}
+if (xf86ReturnOptValBool(ms->Options, OPTION_DELETE_DP_12_DISP, FALSE)) {
+ms->drmmode.delete_dp_12_displays = TRUE;
+}
+
ms->cursor_width = 64;
ms->cursor_height = 64;
ret = drmGetCap(ms->fd, DRM_CAP_CURSOR_WIDTH, &value);
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 8dc6b32..c7cb453 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -326,6 +326,8 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr
mode,
continue;
drmmode_output = output->driver_private;
+if (drmmode_output->output_id == -1)
+continue;
output_ids[output_count] =
drmmode_output->mode_output->connector_id;
output_count++;
@@ -366,10 +368,14 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr
mode,
/* go through all the outputs and force DPMS them back on? */
for (i = 0; i < xf86_config->num_output; i++) {
xf86OutputPtr output = xf86_config->output[i];
+drmmode_output_private_ptr drmmode_output;
if (output->crtc != crtc)
continue;
+drmmode_output = output->driver_private;
+if (drmmode_output->output_id == -1)
+continue;
output->funcs->dpms(output, DPMSModeOn);
}
}
@@ -712,6 +718,9 @@ drmmode_output_detect(xf86OutputPtr output)
drmmode_ptr drmmode = drmmode_output->drmmode;
xf86OutputStatus status;
+if (drmmode_output->output_id == -1)
+return XF86OutputStatusDisconnected;
+
drmModeFreeConnector(drmmode_output->mode_output);
drmmode_output->mode_output =
@@ -873,11 +882,13 @@ drmmode_output_destroy(xf86OutputPtr output)
free(drmmode_output->props[i].atoms);
}
free(drmmode_output->props);
-for (i = 0; i < drmmode_output->mode_output->count_encoders; i++) {
-drmModeFreeEncoder(drmmode_output->mode_encoders[i]);
+if (drmmode_output->mode_output) {
+for (i = 0; i < drmmode_output->mode_output->count_encoders; i++) {
+drmModeFreeEncoder(drmmode_output->mode_encoders[i]);
+}
+drmModeFreeConnector(drmmode_output->mode_output);
}
free(drmmode_output->mode_encoders);
-drmModeFreeConnector(drmmode_output->mode_output);
free(drmmode_output);
output->driver_private = NULL;
}
@@ -,22 +1122,133 @@ static const char *const output_names[] = {
"DSI",
};
+static xf86OutputPtr find_output(ScrnInfoPtr pScrn, int id)
+{
+xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
+int i;
+for (i = 0; i < xf86_config->num_output; i++) {
+xf86OutputPtr output = xf86_config->output[i];
+drmmode_output_private_ptr drmmode_output;
+
+drmmode_output = output->driver_private;
+if (drmmode_output->output_id == id)
+return output;
+}
+return NULL;
+}
+
+static int parse_path_blob(drmModePropertyBlobPtr path_blob, int
*conn_base_id, char **path)
+{
+char *conn;
+char conn_id[5];
+int id, len;
+char *blob_data;
+
+if (!path_blob)
+return -1;
+
+blob
