[PATCH 0/6] fb refcount and crtc helper patches

[PATCH 0/6] fb refcount and crtc helper patches

[Daniel Vetter]

Hi all,

Russell found a refcount bug in my fb refcounting conversion, but to also fix
i915.ko I've opted for a slightly different approach.

While at it I've thought it would be good to backport some of the semantic
changes we've implented in i915's ->set_config callback since we've forked our
own modeset infrastructure.

The intel code receive lots more refactorings and cleanups (most of them right
after the code fork) which would be simple to backport, but I'm kinda swamped
(hint, hint, ...). But these few patches are the important bits.

Cheers, Daniel

Daniel Vetter (6):
  drm/crtc-helpers: Enforce sane set_config api
  drm/crtc-helper: no need to check for fb->depth/bpp
  drm/crtc-helper: explicit DPMS on after modeset
  drm/crtc-helper: don't disable disconnected outputs
  drm: check that ->set_config properly updates the fb
  drm: fix fb leak in setcrtc

 drivers/gpu/drm/drm_crtc.c        |   25 +++++++++++++++-----
 drivers/gpu/drm/drm_crtc_helper.c |   47 +++++++++++++++++--------------------
 include/drm/drm_crtc.h            |    4 ++++
 3 files changed, 45 insertions(+), 31 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] drm/crtc-helpers: Enforce sane set_config api

[Daniel Vetter]

There's no point in trying to clean up after driver-bugs, so just blow
up. Furthermore it's an interface abuse to set no mode but have an fb
and aslo to try to set an fb without enough connectors. These two
spefici cases of interface abuse have been committed by the fb helper,
but that's been fixed meanwhile in

commit 7e53f3a423146745a4e4bb93362d488dfad502a8
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Mon Jan 21 10:52:17 2013 +0100

    drm/fb-helper: fixup set_config semantics

The i915 driver has been shipping since a while with these BUGs with
no reports, so should be save.

Note that this drops an ugly case where we clear crtc->fb behind the
upper levels back and so cause a refcounting mayhem, which Russell
Kins spotted while trying to hunt down a drm framebuffer leak.

Reported-by: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index f554516..e57cfec 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -565,14 +565,13 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 
 	DRM_DEBUG_KMS("\n");
 
-	if (!set)
-		return -EINVAL;
+	BUG_ON(!set);
+	BUG_ON(!set->crtc);
+	BUG_ON(!set->crtc->helper_private);
 
-	if (!set->crtc)
-		return -EINVAL;
-
-	if (!set->crtc->helper_private)
-		return -EINVAL;
+	/* Enforce sane interface api - has been abused by the fb helper. */
+	BUG_ON(!set->mode && set->fb);
+	BUG_ON(set->fb && set->num_connectors == 0);
 
 	crtc_funcs = set->crtc->helper_private;
 
-- 
1.7.10.4

[PATCH 2/6] drm/crtc-helper: no need to check for fb->depth/bpp

[Daniel Vetter]

... since we already check for fb->pixel_format, which encodes all
this. The other two fields are only for backwards compat of older
drivers (and we might want to look into eventually just killing them).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index e57cfec..1ace9c1 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -645,11 +645,6 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 			mode_changed = true;
 		} else if (set->fb == NULL) {
 			mode_changed = true;
-		} else if (set->fb->depth != set->crtc->fb->depth) {
-			mode_changed = true;
-		} else if (set->fb->bits_per_pixel !=
-			   set->crtc->fb->bits_per_pixel) {
-			mode_changed = true;
 		} else if (set->fb->pixel_format !=
 			   set->crtc->fb->pixel_format) {
 			mode_changed = true;
-- 
1.7.10.4

[PATCH 3/6] drm/crtc-helper: explicit DPMS on after modeset

[Daniel Vetter]

Atm the crtc helper implementation of set_config has really
inconsisten semantics: If just an fb update is good enough, dpms state
will be left as-is, but if we do a full modeset we force everything to
dpms on.

This change has already been applied to the i915 modeset code in

commit e3de42b68478a8c95dd27520e9adead2af9477a5
Author: Imre Deak <imre.deak@intel.com>
Date:   Fri May 3 19:44:07 2013 +0200

    drm/i915: force full modeset if the connector is in DPMS OFF mode

which according to Greg KH seems to aim for a new record in most
Bugzilla: links in a commit message.

The history of this dpms forcing is pretty interesting. This patch
here is an almost-revert of

commit 811aaa55ba21ab37407018cfc01770d6b037d3fb
Author: Keith Packard <keithp@keithp.com>
Date:   Thu Feb 3 16:57:28 2011 -0800

    drm: Only set DPMS ON when actually configuring a mode

which fixed the bug of trying to dpms on disabled outputs, but
introduced the new discrepancy between an fb update only and full
modesets. The actual introduction of this goes back to

commit bf9dc102e284a5aa78c73fc9d72e11d5ccd8669f
Author: Keith Packard <keithp@keithp.com>
Date:   Fri Nov 26 10:45:58 2010 -0800

    drm: Set connector DPMS status to ON in drm_crtc_helper_set_config

And if you'd dig around in the i915 driver code there's even more fun
around forcing dpms on and losing our heads and temper of the
resulting inconsistencies. Especially the DP re-training code had tons
of funny stuff in it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 1ace9c1..738a429 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -754,12 +754,6 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 				ret = -EINVAL;
 				goto fail;
 			}
-			DRM_DEBUG_KMS("Setting connector DPMS state to on\n");
-			for (i = 0; i < set->num_connectors; i++) {
-				DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id,
-					      drm_get_connector_name(set->connectors[i]));
-				set->connectors[i]->funcs->dpms(set->connectors[i], DRM_MODE_DPMS_ON);
-			}
 		}
 		drm_helper_disable_unused_functions(dev);
 	} else if (fb_changed) {
@@ -777,6 +771,22 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 		}
 	}
 
+	/*
+	 * crtc set_config helpers implicit set the crtc and all connected
+	 * encoders to DPMS on for a full mode set. But for just an fb update it
+	 * doesn't do that. To not confuse userspace, do an explicit DPMS_ON
+	 * unconditionally. This will also ensure driver internal dpms state is
+	 * consistent again.
+	 */
+	if (set->crtc->enabled) {
+		DRM_DEBUG_KMS("Setting connector DPMS state to on\n");
+		for (i = 0; i < set->num_connectors; i++) {
+			DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id,
+				      drm_get_connector_name(set->connectors[i]));
+			set->connectors[i]->funcs->dpms(set->connectors[i], DRM_MODE_DPMS_ON);
+		}
+	}
+
 	kfree(save_connectors);
 	kfree(save_encoders);
 	kfree(save_crtcs);
-- 
1.7.10.4

[PATCH 4/6] drm/crtc-helper: don't disable disconnected outputs

[Daniel Vetter]

This has originally been added in

commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93
Author: Dave Airlie <airlied@redhat.com>
Date:   Mon Aug 31 15:16:30 2009 +1000

    drm/kms: add explicit encoder disable function and detach harder.

I think it's the wrong thing to do for a few reasons:
- It's policy in the kernel. Userspace gets hotplug events when an
  output disconnects, and it can inquire about all the routing that is
  already set up. If userspace wants to disable a disconnected output
  it can simply do so itself.
- The reason for adding it given in the commit message was to improve
  use of shared encoders. But the disable_unused_functions call only
  happens after the modeset has been completed, so it won't help too
  much in making the modest succeed.
- We (at least in drm/i915, but iirc all other drivers work the same)
  ensure that if the user accidentally yanks the cable and then
  replugs, the same configuration will keep on working without any
  userspace actions required. For most outputs that's the case by
  default, and DP can have the same semantics with a simple re-train
  handler fired off from the hpd replug event. This breaks it since if
  the user is unlucky and hits the mouse, resulting in a panning of
  e.g. the integrated panel that modeset might kill the accidentally
  yanked output. Which isn't too great.

i915 has applied this behaviour change as part of the bit modeset
review, thus far without resulting in an angry crowd with pitchforks:

commit b0a2658acb5bf9ca86b4aab011b7106de3af0add
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Dec 18 09:37:54 2012 +0100

    drm/i915: don't disable disconnected outputs

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c |    7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 738a429..917c803 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -279,13 +279,6 @@ void drm_helper_disable_unused_functions(struct drm_device *dev)
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (!connector->encoder)
-			continue;
-		if (connector->status == connector_status_disconnected)
-			connector->encoder = NULL;
-	}
-
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
 		if (!drm_helper_encoder_in_use(encoder)) {
 			drm_encoder_disable(encoder);
-- 
1.7.10.4

[PATCH 5/6] drm: check that ->set_config properly updates the fb

[Daniel Vetter]

Historically drm lacked fb refcounting, so the updating of crtc->fb
was done by the lower levels at a point convenient to get their own
refcounting (e.g. refcounts for the underlying gem bo, pinning
refcounts) right. With the introduction of refcounted fbs the drm core
handled the fb refcounts, but still relied on drivers to update the
crtc->fb pointer (this approach required the least invasive changes in
drivers).

Enforce this contract with a WARN_ON.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index baee575..bcee25a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1939,6 +1939,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
 
 	ret = crtc->funcs->set_config(set);
 	if (ret == 0) {
+		/* crtc->fb must be updated by ->set_config, enforces this. */
+		WARN_ON(fb != crtc->fb);
+
 		if (old_fb)
 			drm_framebuffer_unreference(old_fb);
 		if (fb)
-- 
1.7.10.4

[PATCH 6/6] drm: fix fb leak in setcrtc

[Daniel Vetter]

Drivers are allowed (actually have to) disable unrelated crtcs in
their ->set_config callback (when we steal all the connectors from
that crtc). If they do that they'll clear crtc->fb to NULL.

Which results in a refcount leak, since the drm core is keeping track
of that reference.

To fix this track the old fb of all crtcs and adjust references for
all of them. Of course, since we only hold an additional reference for
the fb for the current crtc we need to increase refcounts before we
drop the old one.

This approach has the benefit that it inches us a bit closer to an
atomic modeset world, where we want to update the config of all crtcs
in one step.

This regression has been introduce in the framebuffer refcount
conversion, specifically in

commit b0d1232589df5575c5971224ac4cb30e7e525884
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Dec 11 01:07:12 2012 +0100

    drm: refcounting for crtc framebuffers

Reported-by: Russell King <linux@arm.linux.org.uk>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c |   22 ++++++++++++++++------
 include/drm/drm_crtc.h     |    4 ++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index bcee25a..3486a00 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1931,21 +1931,31 @@ out:
 int drm_mode_set_config_internal(struct drm_mode_set *set)
 {
 	struct drm_crtc *crtc = set->crtc;
-	struct drm_framebuffer *fb, *old_fb;
+	struct drm_framebuffer *fb;
+	struct drm_crtc *tmp;
 	int ret;
 
-	old_fb = crtc->fb;
+	/*
+	 * NOTE: ->set_config can also disable other crtcs (if we steal all
+	 * connectors from it), hence we need to refcount the fbs across all
+	 * crtcs. Atomic modeset will have saner semantics ...
+	 */
+	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
+		tmp->old_fb = tmp->fb;
+
 	fb = set->fb;
 
 	ret = crtc->funcs->set_config(set);
 	if (ret == 0) {
 		/* crtc->fb must be updated by ->set_config, enforces this. */
 		WARN_ON(fb != crtc->fb);
+	}
 
-		if (old_fb)
-			drm_framebuffer_unreference(old_fb);
-		if (fb)
-			drm_framebuffer_reference(fb);
+	list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
+		if (tmp->fb)
+			drm_framebuffer_reference(tmp->fb);
+		if (tmp->old_fb)
+			drm_framebuffer_unreference(tmp->old_fb);
 	}
 
 	return ret;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 53c33e2..07d1dbbb 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -409,6 +409,10 @@ struct drm_crtc {
 	/* framebuffer the connector is currently bound to */
 	struct drm_framebuffer *fb;
 
+	/* Temporary tracking of the old fb while a modeset is ongoing. Used
+	 * by drm_mode_set_config_internal to implement correct refcounting. */
+	struct drm_framebuffer *old_fb;
+
 	bool enabled;
 
 	/* Requested mode from modesetting. */
-- 
1.7.10.4

[PATCH] drm/crtc-helper: explicit DPMS on after modeset

[Daniel Vetter]

Atm the crtc helper implementation of set_config has really
inconsisten semantics: If just an fb update is good enough, dpms state
will be left as-is, but if we do a full modeset we force everything to
dpms on.

This change has already been applied to the i915 modeset code in

commit e3de42b68478a8c95dd27520e9adead2af9477a5
Author: Imre Deak <imre.deak@intel.com>
Date:   Fri May 3 19:44:07 2013 +0200

    drm/i915: force full modeset if the connector is in DPMS OFF mode

which according to Greg KH seems to aim for a new record in most
Bugzilla: links in a commit message.

The history of this dpms forcing is pretty interesting. This patch
here is an almost-revert of

commit 811aaa55ba21ab37407018cfc01770d6b037d3fb
Author: Keith Packard <keithp@keithp.com>
Date:   Thu Feb 3 16:57:28 2011 -0800

    drm: Only set DPMS ON when actually configuring a mode

which fixed the bug of trying to dpms on disabled outputs, but
introduced the new discrepancy between an fb update only and full
modesets. The actual introduction of this goes back to

commit bf9dc102e284a5aa78c73fc9d72e11d5ccd8669f
Author: Keith Packard <keithp@keithp.com>
Date:   Fri Nov 26 10:45:58 2010 -0800

    drm: Set connector DPMS status to ON in drm_crtc_helper_set_config

And if you'd dig around in the i915 driver code there's even more fun
around forcing dpms on and losing our heads and temper of the
resulting inconsistencies. Especially the DP re-training code had tons
of funny stuff in it.

v2: So v1 totally blew up on resume on my radeon system here. After
much head-scraching I've figured out that the radeon resume functions
resumes the console system _before_ it actually restores all the
modeset state. And resuming the console systems means that fbdev doeas
an immediate ->set_par call.

Now up to this patch that ->set_par did absolutely nothing: All the
old sw state from pre-suspend was still around (since the modeset
reset wasn't done yet), which means that the set_config calls done as
a result of the ->set_par where all treated as no-ops (despite the the
real hw state was obviously something completely else).

Since v1 of this patch just added a bunch of ->dpms calls if the crtc
was enabled, those set_config calls suddenly stopped being no-ops. But
because the hw state wasn't restored the ->dpms callbacks resulted in
decent amounts of hilarity and eventual full hangs.

Sinc I can't review all kms drivers for such tricky ordering
constraints v2 opts of a different approach and forces a full modeset
if the connector dpms state isnt' DPMS_ON. Since the ->dpms callbacks
implemented by the modeset helpers update the connector->dpms property
we have the same effect of ensure that the pipe is ultimately turned
on, even if we just end up updating the fb. This is the same approache
we ended up using in the intel driver.

Note that besides i915.ko only all other drivers eventually call
drm_helper_connector_dpms with the exception of vmwgfx, which does not
support dmps at all.

Cc: Alex Deucher <alexdeucher@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 72282af..eb51dc4 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -669,6 +669,11 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 					/* don't break so fail path works correct */
 					fail = 1;
 				break;
+
+				if (connector->dpms != DRM_MODE_DPMS_ON) {
+					DRM_DEBUG_KMS("connector dpms not on, full mode switch\n");
+					mode_changed = true;
+				}
 			}
 		}
 
-- 
1.7.10.4

[PATCH] drm/crtc-helper: don't disable disconnected outputs

[Daniel Vetter]

This has originally been added in

commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93
Author: Dave Airlie <airlied@redhat.com>
Date:   Mon Aug 31 15:16:30 2009 +1000

    drm/kms: add explicit encoder disable function and detach harder.

I think it's the wrong thing to do for a few reasons:
- It's policy in the kernel. Userspace gets hotplug events when an
  output disconnects, and it can inquire about all the routing that is
  already set up. If userspace wants to disable a disconnected output
  it can simply do so itself.
- The reason for adding it given in the commit message was to improve
  use of shared encoders. But the disable_unused_functions call only
  happens after the modeset has been completed, so it won't help too
  much in making the modest succeed.
- We (at least in drm/i915, but iirc all other drivers work the same)
  ensure that if the user accidentally yanks the cable and then
  replugs, the same configuration will keep on working without any
  userspace actions required. For most outputs that's the case by
  default, and DP can have the same semantics with a simple re-train
  handler fired off from the hpd replug event. This breaks it since if
  the user is unlucky and hits the mouse, resulting in a panning of
  e.g. the integrated panel that modeset might kill the accidentally
  yanked output. Which isn't too great.

i915 has applied this behaviour change as part of the bit modeset
review, thus far without resulting in an angry crowd with pitchforks:

commit b0a2658acb5bf9ca86b4aab011b7106de3af0add
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Dec 18 09:37:54 2012 +0100

    drm/i915: don't disable disconnected outputs

v2: Remove unused variable that I've missed.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc_helper.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 1ace9c1..72282af 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -276,16 +276,8 @@ drm_encoder_disable(struct drm_encoder *encoder)
 void drm_helper_disable_unused_functions(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
-	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (!connector->encoder)
-			continue;
-		if (connector->status == connector_status_disconnected)
-			connector->encoder = NULL;
-	}
-
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
 		if (!drm_helper_encoder_in_use(encoder)) {
 			drm_encoder_disable(encoder);
-- 
1.7.10.4

Re: [PATCH] drm/crtc-helper: don't disable disconnected outputs

[Dave Airlie]

On Sun, Jun 16, 2013 at 6:41 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This has originally been added in
>
> commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93
> Author: Dave Airlie <airlied@redhat.com>
> Date:   Mon Aug 31 15:16:30 2009 +1000
>
>     drm/kms: add explicit encoder disable function and detach harder.
>
> I think it's the wrong thing to do for a few reasons:
> - It's policy in the kernel. Userspace gets hotplug events when an
>   output disconnects, and it can inquire about all the routing that is
>   already set up. If userspace wants to disable a disconnected output
>   it can simply do so itself.
> - The reason for adding it given in the commit message was to improve
>   use of shared encoders. But the disable_unused_functions call only
>   happens after the modeset has been completed, so it won't help too
>   much in making the modest succeed.
> - We (at least in drm/i915, but iirc all other drivers work the same)
>   ensure that if the user accidentally yanks the cable and then
>   replugs, the same configuration will keep on working without any
>   userspace actions required. For most outputs that's the case by
>   default, and DP can have the same semantics with a simple re-train
>   handler fired off from the hpd replug event. This breaks it since if
>   the user is unlucky and hits the mouse, resulting in a panning of
>   e.g. the integrated panel that modeset might kill the accidentally
>   yanked output. Which isn't too great.
>
> i915 has applied this behaviour change as part of the bit modeset
> review, thus far without resulting in an angry crowd with pitchforks:
>

The reason is that i915 hardware is nearly impossible to ever set
something up that could trigger this on. You don't have
shared encoders in places that could affect this from what I can see.

I know this will break the radeon card I have that I wrote this for
originally, where if you alternate hotplugging a tv-out
and vga that share the same encoder it won't work. I can probably drag
that card out of whatever hole if fell into,
but the given justifications have give me no confidence that this
patch just isn't reverting the original commit without
fixing anything else.

I suspect I was testing hotplugging at fbcon not using X also.

Dave.

Re: [PATCH] drm/crtc-helper: don't disable disconnected outputs

[Daniel Vetter]

On Tue, Jun 25, 2013 at 3:09 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Sun, Jun 16, 2013 at 6:41 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> This has originally been added in
>>
>> commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93
>> Author: Dave Airlie <airlied@redhat.com>
>> Date:   Mon Aug 31 15:16:30 2009 +1000
>>
>>     drm/kms: add explicit encoder disable function and detach harder.
>>
>> I think it's the wrong thing to do for a few reasons:
>> - It's policy in the kernel. Userspace gets hotplug events when an
>>   output disconnects, and it can inquire about all the routing that is
>>   already set up. If userspace wants to disable a disconnected output
>>   it can simply do so itself.
>> - The reason for adding it given in the commit message was to improve
>>   use of shared encoders. But the disable_unused_functions call only
>>   happens after the modeset has been completed, so it won't help too
>>   much in making the modest succeed.
>> - We (at least in drm/i915, but iirc all other drivers work the same)
>>   ensure that if the user accidentally yanks the cable and then
>>   replugs, the same configuration will keep on working without any
>>   userspace actions required. For most outputs that's the case by
>>   default, and DP can have the same semantics with a simple re-train
>>   handler fired off from the hpd replug event. This breaks it since if
>>   the user is unlucky and hits the mouse, resulting in a panning of
>>   e.g. the integrated panel that modeset might kill the accidentally
>>   yanked output. Which isn't too great.
>>
>> i915 has applied this behaviour change as part of the bit modeset
>> review, thus far without resulting in an angry crowd with pitchforks:
>>
>
> The reason is that i915 hardware is nearly impossible to ever set
> something up that could trigger this on. You don't have
> shared encoders in places that could affect this from what I can see.

Actually we do have shared encoders, and on second consideration I
guess we could hit this. We get away with it though since we don't
really implement the connector modeset correctly for shared encoders:
We simply pick what has been last detected as connected ...

Of course that's a bit broken, and we have the bug reports to prove it
(and since the modeset rework they include noisy dmesg spam from the
checker about the inconsistencies even). I guess I should fix that up
first ;-)

> I know this will break the radeon card I have that I wrote this for
> originally, where if you alternate hotplugging a tv-out
> and vga that share the same encoder it won't work. I can probably drag
> that card out of whatever hole if fell into,
> but the given justifications have give me no confidence that this
> patch just isn't reverting the original commit without
> fixing anything else.
>
> I suspect I was testing hotplugging at fbcon not using X also.

Oh, that makes a lot of sense. fbcon kinda wants an atomic modeset
interface, but since it doesn't have one it expects the backend to
paper over any intermediate inconsistencies. I guess we should fix
that first.

For userspace I still think that it makes much more sense to obey
userspace's request and not silently kill outputs. We do that already
if userspace steals the crtc. But that's something userspace can
predict, wheras the exact connector->encoder relationship is more
elusive. So I think the right thing would be to fail such a modeset.

Anyway I agree that this patch needs a bit more prep work.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch