[01/13] drm/fb-helper: Push down modeset lock into FB helpers
diff mbox

Message ID 20170627145936.18983-2-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter June 27, 2017, 2:59 p.m. UTC
From: Thierry Reding <treding@nvidia.com>

Move the modeset locking from drivers into FB helpers.

v2: Also handle intel_connector_add_to_fbdev.

Tested-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
 drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
 3 files changed, 34 insertions(+), 19 deletions(-)

Comments

Maarten Lankhorst June 29, 2017, 9:10 a.m. UTC | #1
Op 27-06-17 om 16:59 schreef Daniel Vetter:
> From: Thierry Reding <treding@nvidia.com>
>
> Move the modeset locking from drivers into FB helpers.
>
> v2: Also handle intel_connector_add_to_fbdev.
>
> Tested-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>  3 files changed, 34 insertions(+), 19 deletions(-)
I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..

best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.
Daniel Vetter June 29, 2017, 9:23 a.m. UTC | #2
On Thu, Jun 29, 2017 at 11:10 AM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 27-06-17 om 16:59 schreef Daniel Vetter:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> Move the modeset locking from drivers into FB helpers.
>>
>> v2: Also handle intel_connector_add_to_fbdev.
>>
>> Tested-by: John Stultz <john.stultz@linaro.org>
>> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>>  3 files changed, 34 insertions(+), 19 deletions(-)
> I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..
>
> best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.

Hm, but this did race even before that already ... Should I just wrap
the mst_port = NULL in the connection_mutex? With a big warning that
we should have proper refcounting for this instead (both connectors
and all the mst things are refcounted already).
-Daniel
Maarten Lankhorst June 29, 2017, 9:33 a.m. UTC | #3
Op 29-06-17 om 11:23 schreef Daniel Vetter:
> On Thu, Jun 29, 2017 at 11:10 AM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 27-06-17 om 16:59 schreef Daniel Vetter:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Move the modeset locking from drivers into FB helpers.
>>>
>>> v2: Also handle intel_connector_add_to_fbdev.
>>>
>>> Tested-by: John Stultz <john.stultz@linaro.org>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>>>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>>>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>>>  3 files changed, 34 insertions(+), 19 deletions(-)
>> I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..
>>
>> best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.
> Hm, but this did race even before that already ... Should I just wrap
> the mst_port = NULL in the connection_mutex? With a big warning that
> we should have proper refcounting for this instead (both connectors
> and all the mst things are refcounted already).
> -Daniel

I think leave open the race, the DP-MST modeset code itself doesn't use the connector->mst_port, just the detect callbacks do. In case of nonblocking modeset mst_port might already fall away so it's not like the race is any worse now.

I don't even know how to solve it, except by not unplugging. :)

~Maarten
Daniel Vetter June 29, 2017, 9:44 a.m. UTC | #4
On Thu, Jun 29, 2017 at 11:33 AM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 29-06-17 om 11:23 schreef Daniel Vetter:
>> On Thu, Jun 29, 2017 at 11:10 AM, Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com> wrote:
>>> Op 27-06-17 om 16:59 schreef Daniel Vetter:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> Move the modeset locking from drivers into FB helpers.
>>>>
>>>> v2: Also handle intel_connector_add_to_fbdev.
>>>>
>>>> Tested-by: John Stultz <john.stultz@linaro.org>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>>>>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>>>>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>>>>  3 files changed, 34 insertions(+), 19 deletions(-)
>>> I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..
>>>
>>> best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.
>> Hm, but this did race even before that already ... Should I just wrap
>> the mst_port = NULL in the connection_mutex? With a big warning that
>> we should have proper refcounting for this instead (both connectors
>> and all the mst things are refcounted already).
>> -Daniel
>
> I think leave open the race, the DP-MST modeset code itself doesn't use the connector->mst_port, just the detect callbacks do. In case of nonblocking modeset mst_port might already fall away so it's not like the race is any worse now.
>
> I don't even know how to solve it, except by not unplugging. :)

Why does grabbing the lock not fix the race? ->detect eventually calls
drm_dp_get_validated_port_ref, which will bail on a zombie mst port..
-Daniel
Maarten Lankhorst June 29, 2017, 11:13 a.m. UTC | #5
Op 29-06-17 om 11:44 schreef Daniel Vetter:
> On Thu, Jun 29, 2017 at 11:33 AM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 29-06-17 om 11:23 schreef Daniel Vetter:
>>> On Thu, Jun 29, 2017 at 11:10 AM, Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>> Op 27-06-17 om 16:59 schreef Daniel Vetter:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Move the modeset locking from drivers into FB helpers.
>>>>>
>>>>> v2: Also handle intel_connector_add_to_fbdev.
>>>>>
>>>>> Tested-by: John Stultz <john.stultz@linaro.org>
>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>>>>>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>>>>>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>>>>>  3 files changed, 34 insertions(+), 19 deletions(-)
>>>> I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..
>>>>
>>>> best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.
>>> Hm, but this did race even before that already ... Should I just wrap
>>> the mst_port = NULL in the connection_mutex? With a big warning that
>>> we should have proper refcounting for this instead (both connectors
>>> and all the mst things are refcounted already).
>>> -Daniel
>> I think leave open the race, the DP-MST modeset code itself doesn't use the connector->mst_port, just the detect callbacks do. In case of nonblocking modeset mst_port might already fall away so it's not like the race is any worse now.
>>
>> I don't even know how to solve it, except by not unplugging. :)
> Why does grabbing the lock not fix the race? ->detect eventually calls
> drm_dp_get_validated_port_ref, which will bail on a zombie mst port..
Oh ok. But there will always be a race. If DP-MST fails immediately after drm_dp_get_validated_port_ref,
you'd still not be able to do anything about it. :)
Daniel Vetter June 29, 2017, 12:38 p.m. UTC | #6
On Thu, Jun 29, 2017 at 1:13 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Oh ok. But there will always be a race. If DP-MST fails immediately after drm_dp_get_validated_port_ref,
> you'd still not be able to do anything about it. :)

This is just about not oopsing, any transactions to the sink will
still go nowhere an fail.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 574af01d3ce9..5e73cc9f51e3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -109,8 +109,8 @@  static DEFINE_MUTEX(kernel_fb_helper_lock);
 	for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
 	     i__ = 0; i__ < (fbh)->connector_count; i__++)
 
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
-				    struct drm_connector *connector)
+static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+					     struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector *fb_conn;
 	struct drm_fb_helper_connector **temp;
@@ -141,8 +141,23 @@  int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	drm_connector_get(connector);
 	fb_conn->connector = connector;
 	fb_helper->connector_info[fb_helper->connector_count++] = fb_conn;
+
 	return 0;
 }
+
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+				    struct drm_connector *connector)
+{
+	int err;
+
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+	err = __drm_fb_helper_add_one_connector(fb_helper, connector);
+
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	return err;
+}
 EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
 
 /**
@@ -172,8 +187,7 @@  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	mutex_lock(&dev->mode_config.mutex);
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
-		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
-
+		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
 		if (ret)
 			goto fail;
 	}
@@ -198,8 +212,8 @@  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
 
-int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
-				       struct drm_connector *connector)
+static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+						struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector *fb_helper_connector;
 	int i, j;
@@ -227,6 +241,20 @@  int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 
 	return 0;
 }
+
+int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+				       struct drm_connector *connector)
+{
+	int err;
+
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
+
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	return err;
+}
 EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
 
 static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 2cf046beae0f..5b78165882ce 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -501,11 +501,8 @@  static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 static void intel_dp_register_mst_connector(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
 
-	drm_modeset_lock_all(dev);
 	intel_connector_add_to_fbdev(intel_connector);
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_register(&intel_connector->base);
 }
@@ -514,15 +511,12 @@  static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
 
 	drm_connector_unregister(connector);
 
 	/* need to nuke the connector */
-	drm_modeset_lock_all(dev);
 	intel_connector_remove_from_fbdev(intel_connector);
 	intel_connector->mst_port = NULL;
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_unreference(&intel_connector->base);
 	DRM_DEBUG_KMS("\n");
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 6598306dca9b..ebdf1b859cb6 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -300,9 +300,7 @@  static void radeon_dp_register_mst_connector(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct radeon_device *rdev = dev->dev_private;
 
-	drm_modeset_lock_all(dev);
 	radeon_fb_add_connector(rdev, connector);
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_register(connector);
 }
@@ -315,13 +313,8 @@  static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct radeon_device *rdev = dev->dev_private;
 
 	drm_connector_unregister(connector);
-	/* need to nuke the connector */
-	drm_modeset_lock_all(dev);
-	/* dpms off */
 	radeon_fb_remove_connector(rdev, connector);
-
 	drm_connector_cleanup(connector);
-	drm_modeset_unlock_all(dev);
 
 	kfree(connector);
 	DRM_DEBUG_KMS("\n");