Re: [PATCH 1/6] Revert "drm/atomic-helper: Fix leak in disable_all"

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 21, 2018 at 09:25:06AM +0100, Daniel Vetter wrote:
> On Tue, Mar 20, 2018 at 09:17:52PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we're leaking fbs on load detect on account of nothing setting
> > up plane->old_fb for the drm_atomic_clean_old_fb() call in
> > drm_atomic_helper_commit_duplicated_state(). Removing the
> > drm_atomic_clean_old_fb() call seems like the right call to me here.
> > This does mean we end up leaking something via
> > drm_atomic_helper_shutdown() though, but we'll fix that up in a
> > different way.
> > 
> > This reverts commit 49d70aeaeca8f62b72b7712ecd1e29619a445866.
> 
> So the reason I went this way and not what you're suggesting in this patch
> series is that i915 is the one and only driver noodling around with load
> detect and gpu reset that needs this.
> 
> Imo it'd be better to fix up our load detect code to also set the old_fb
> stuff up correctly,

Feels a bit silly to just set plane->old_fb = plane->fb and immediately
follow it up with inc(plane->fb)+dec(plane->old_fb) and old_fb=NULL.

> and add at least a long-term task to todo.rst to get
> rid of all this.

I have a series that stops using plane->fb/old_fb entirely on atomic
drivers. That removes the entire clean_old_fbs() thing. Not sure all
drivers are ready for that though. I think I'll post it as an rfc
anyway.

> Inflicting a new parameter on all the other drives (like
> you do in patch 2) is imo the wrong way round - we have 31 other atomic
> drivers than i915.ko.

None of them use disable_all() directly.

But if we want to avoid the new parameter being visible too drivers I
could just squash in:

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 76424dd961d7..cdf10b9e5177 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2881,33 +2881,9 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 	return 0;
 }
 
-/**
- * drm_atomic_helper_disable_all - disable all currently active outputs
- * @dev: DRM device
- * @ctx: lock acquisition context
- * @clean_old_fbs: update the plane->fb/old_fb pointers?
- *
- * Loops through all connectors, finding those that aren't turned off and then
- * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
- * that they are connected to.
- *
- * This is used for example in suspend/resume to disable all currently active
- * functions when suspending. If you just want to shut down everything at e.g.
- * driver unload, look at drm_atomic_helper_shutdown().
- *
- * Note that if callers haven't already acquired all modeset locks this might
- * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
- *
- * Returns:
- * 0 on success or a negative error code on failure.
- *
- * See also:
- * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
- * drm_atomic_helper_shutdown().
- */
-int drm_atomic_helper_disable_all(struct drm_device *dev,
-				  struct drm_modeset_acquire_ctx *ctx,
-				  bool clean_old_fbs)
+static int __drm_atomic_helper_disable_all(struct drm_device *dev,
+					   struct drm_modeset_acquire_ctx *ctx,
+					   bool clean_old_fbs)
 {
 	struct drm_atomic_state *state;
 	struct drm_connector_state *conn_state;
@@ -2973,7 +2949,34 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 	drm_atomic_state_put(state);
 	return ret;
 }
-
+/**
+ * drm_atomic_helper_disable_all - disable all currently active outputs
+ * @dev: DRM device
+ * @ctx: lock acquisition context
+ *
+ * Loops through all connectors, finding those that aren't turned off and then
+ * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
+ * that they are connected to.
+ *
+ * This is used for example in suspend/resume to disable all currently active
+ * functions when suspending. If you just want to shut down everything at e.g.
+ * driver unload, look at drm_atomic_helper_shutdown().
+ *
+ * Note that if callers haven't already acquired all modeset locks this might
+ * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ *
+ * See also:
+ * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
+ * drm_atomic_helper_shutdown().
+ */
+int drm_atomic_helper_disable_all(struct drm_device *dev,
+				  struct drm_modeset_acquire_ctx *ctx)
+{
+	return __drm_atomic_helper_disable_all(dev, ctx, false);
+}
 EXPORT_SYMBOL(drm_atomic_helper_disable_all);
 
 /**
@@ -2996,7 +2999,7 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
 	while (1) {
 		ret = drm_modeset_lock_all_ctx(dev, &ctx);
 		if (!ret)
-			ret = drm_atomic_helper_disable_all(dev, &ctx, true);
+			ret = __drm_atomic_helper_disable_all(dev, &ctx, true);
 
 		if (ret != -EDEADLK)
 			break;
@@ -3074,7 +3077,7 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
 		}
 	}
 
-	err = drm_atomic_helper_disable_all(dev, &ctx, false);
+	err = drm_atomic_helper_disable_all(dev, &ctx);
 	if (err < 0) {
 		drm_atomic_state_put(state);
 		state = ERR_PTR(err);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 008fe6903c8c..34231b9c78af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3721,7 +3721,7 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv)
 		return;
 	}
 
-	ret = drm_atomic_helper_disable_all(dev, ctx, false);
+	ret = drm_atomic_helper_disable_all(dev, ctx);
 	if (ret) {
 		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
 		drm_atomic_state_put(state);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 10e952951d2f..26aaba58d6ce 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -122,8 +122,7 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 		struct drm_atomic_state *state);
 
 int drm_atomic_helper_disable_all(struct drm_device *dev,
-				  struct drm_modeset_acquire_ctx *ctx,
-				  bool clean_old_fbs);
+				  struct drm_modeset_acquire_ctx *ctx);
 void drm_atomic_helper_shutdown(struct drm_device *dev);
 struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
 int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx