| Message ID | 20170627145936.18983-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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. :)
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
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");