[PATCH 0/3] drm: Plug framebuffer leaks.

[PATCH 0/3] drm: Plug framebuffer leaks.

[Maarten Lankhorst]

There's a small leak excerbated by interruptible waiting in setplane_internal,
which was triggered by kms_atomic. Another leak is in load detection, and easily
plugged by just removing the fb handling.

Maarten Lankhorst (2):
  drm/plane: Make framebuffer refcounting the responsibility of
    setplane_internal callers
  drm/framebuffer: Print task that allocated the fb in debug info.

Ville Syrjälä (1):
  drm/i915: Disable all planes for load detection, v2.

 drivers/gpu/drm/drm_fb_helper.c      |   1 +
 drivers/gpu/drm/drm_framebuffer.c    |   2 +
 drivers/gpu/drm/drm_plane.c          |  42 +++++-----
 drivers/gpu/drm/i915/intel_display.c | 147 +++++------------------------------
 include/drm/drm_framebuffer.h        |   6 ++
 5 files changed, 47 insertions(+), 151 deletions(-)

-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers

[Maarten Lankhorst]

lock_all_ctx in setplane_internal may return -EINTR, and
__setplane_internal could return -EDEADLK. Making more
special cases for fb would make the code even harder to
read, so the easiest solution is not taking over the fb
refcount, and making callers responsible for dropping
the ref.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707
Fixes: 13736ba3b38b ("drm/legacy: Convert setplane ioctl locking to interruptible.")
Testcase: kms_atomic_interruptible
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 37a93cdffb4a..2c90519576a3 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -558,11 +558,10 @@ int drm_plane_check_pixel_format(const struct drm_plane *plane, u32 format)
 }
 
 /*
- * setplane_internal - setplane handler for internal callers
+ * __setplane_internal - setplane handler for internal callers
  *
- * Note that we assume an extra reference has already been taken on fb.  If the
- * update fails, this reference will be dropped before return; if it succeeds,
- * the previous framebuffer (if any) will be unreferenced instead.
+ * This function will take a reference on the new fb for the plane
+ * on success.
  *
  * src_{x,y,w,h} are provided in 16.16 fixed point format
  */
@@ -630,14 +629,12 @@ static int __setplane_internal(struct drm_plane *plane,
 	if (!ret) {
 		plane->crtc = crtc;
 		plane->fb = fb;
-		fb = NULL;
+		drm_framebuffer_get(plane->fb);
 	} else {
 		plane->old_fb = NULL;
 	}
 
 out:
-	if (fb)
-		drm_framebuffer_put(fb);
 	if (plane->old_fb)
 		drm_framebuffer_put(plane->old_fb);
 	plane->old_fb = NULL;
@@ -685,6 +682,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	struct drm_plane *plane;
 	struct drm_crtc *crtc = NULL;
 	struct drm_framebuffer *fb = NULL;
+	int ret;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -717,15 +715,16 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		}
 	}
 
-	/*
-	 * setplane_internal will take care of deref'ing either the old or new
-	 * framebuffer depending on success.
-	 */
-	return setplane_internal(plane, crtc, fb,
-				 plane_req->crtc_x, plane_req->crtc_y,
-				 plane_req->crtc_w, plane_req->crtc_h,
-				 plane_req->src_x, plane_req->src_y,
-				 plane_req->src_w, plane_req->src_h);
+	ret = setplane_internal(plane, crtc, fb,
+				plane_req->crtc_x, plane_req->crtc_y,
+				plane_req->crtc_w, plane_req->crtc_h,
+				plane_req->src_x, plane_req->src_y,
+				plane_req->src_w, plane_req->src_h);
+
+	if (fb)
+		drm_framebuffer_put(fb);
+
+	return ret;
 }
 
 static int drm_mode_cursor_universal(struct drm_crtc *crtc,
@@ -788,13 +787,12 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
 		src_h = fb->height << 16;
 	}
 
-	/*
-	 * setplane_internal will take care of deref'ing either the old or new
-	 * framebuffer depending on success.
-	 */
 	ret = __setplane_internal(crtc->cursor, crtc, fb,
-				crtc_x, crtc_y, crtc_w, crtc_h,
-				0, 0, src_w, src_h, ctx);
+				  crtc_x, crtc_y, crtc_w, crtc_h,
+				  0, 0, src_w, src_h, ctx);
+
+	if (fb)
+		drm_framebuffer_put(fb);
 
 	/* Update successful; save new cursor position, if necessary */
 	if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/framebuffer: Print task that allocated the fb in debug info.

[Maarten Lankhorst]

This is is very useful to finding sources of leaked framebufers.
The fbcon fb is annotated with [fbcon], to give it a better name
than kworker.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c   | 1 +
 drivers/gpu/drm/drm_framebuffer.c | 2 ++
 include/drm/drm_framebuffer.h     | 6 ++++++
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 04a3a5ce370a..d396d74a7dda 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1848,6 +1848,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	if (ret < 0)
 		return ret;
 
+	strcpy(fb_helper->fb->comm, "[fbcon]");
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index d63d4c2ac4c8..5a13ff29f4f0 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -664,6 +664,7 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	INIT_LIST_HEAD(&fb->filp_head);
 
 	fb->funcs = funcs;
+	strcpy(fb->comm, current->comm);
 
 	ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB,
 				    false, drm_framebuffer_free);
@@ -978,6 +979,7 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
 	struct drm_format_name_buf format_name;
 	unsigned int i;
 
+	drm_printf_indent(p, indent, "allocated by = %s\n", fb->comm);
 	drm_printf_indent(p, indent, "refcount=%u\n",
 			  drm_framebuffer_read_refcount(fb));
 	drm_printf_indent(p, indent, "format=%s\n",
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index dccb897951ba..c50502c656e5 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -121,6 +121,12 @@ struct drm_framebuffer {
 	 * @base: base modeset object structure, contains the reference count.
 	 */
 	struct drm_mode_object base;
+
+	/**
+	 * @comm: Name of the process allocating the fb, used for fb dumping.
+	 */
+	char comm[TASK_COMM_LEN];
+
 	/**
 	 * @format: framebuffer format information
 	 */
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Disable all planes for load detection, v2.

[Maarten Lankhorst]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We don't need any active planes during load detection, so just disable
them all. This saves us from having to come up with a suitable
framebuffer. And we also avoid leaving sprite/cursor planes on and
potentially presenting them at a peculiar location during the load
detection.

Changes since v1 (Maarten):
- Add missing call to add_all_affected_planes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707
---
 drivers/gpu/drm/i915/intel_display.c | 147 +++++------------------------------
 1 file changed, 18 insertions(+), 129 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e833268c9df..ef61f9a75c93 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9689,111 +9689,27 @@ intel_framebuffer_create(struct drm_i915_gem_object *obj,
 	return ERR_PTR(ret);
 }
 
-static u32
-intel_framebuffer_pitch_for_width(int width, int bpp)
-{
-	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
-	return ALIGN(pitch, 64);
-}
-
-static u32
-intel_framebuffer_size_for_mode(const struct drm_display_mode *mode, int bpp)
-{
-	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
-	return PAGE_ALIGN(pitch * mode->vdisplay);
-}
-
-static struct drm_framebuffer *
-intel_framebuffer_create_for_mode(struct drm_device *dev,
-				  const struct drm_display_mode *mode,
-				  int depth, int bpp)
-{
-	struct drm_framebuffer *fb;
-	struct drm_i915_gem_object *obj;
-	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
-
-	obj = i915_gem_object_create(to_i915(dev),
-				    intel_framebuffer_size_for_mode(mode, bpp));
-	if (IS_ERR(obj))
-		return ERR_CAST(obj);
-
-	mode_cmd.width = mode->hdisplay;
-	mode_cmd.height = mode->vdisplay;
-	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width,
-								bpp);
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
-
-	fb = intel_framebuffer_create(obj, &mode_cmd);
-	if (IS_ERR(fb))
-		i915_gem_object_put(obj);
-
-	return fb;
-}
-
-static struct drm_framebuffer *
-mode_fits_in_fbdev(struct drm_device *dev,
-		   const struct drm_display_mode *mode)
-{
-#ifdef CONFIG_DRM_FBDEV_EMULATION
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct drm_i915_gem_object *obj;
-	struct drm_framebuffer *fb;
-
-	if (!dev_priv->fbdev)
-		return NULL;
-
-	if (!dev_priv->fbdev->fb)
-		return NULL;
-
-	obj = dev_priv->fbdev->fb->obj;
-	BUG_ON(!obj);
-
-	fb = &dev_priv->fbdev->fb->base;
-	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
-							       fb->format->cpp[0] * 8))
-		return NULL;
-
-	if (obj->base.size < mode->vdisplay * fb->pitches[0])
-		return NULL;
-
-	drm_framebuffer_get(fb);
-	return fb;
-#else
-	return NULL;
-#endif
-}
-
-static int intel_modeset_setup_plane_state(struct drm_atomic_state *state,
-					   struct drm_crtc *crtc,
-					   const struct drm_display_mode *mode,
-					   struct drm_framebuffer *fb,
-					   int x, int y)
+static int intel_modeset_disable_planes(struct drm_atomic_state *state,
+					struct drm_crtc *crtc)
 {
+	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
-	int hdisplay, vdisplay;
-	int ret;
-
-	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
-	if (IS_ERR(plane_state))
-		return PTR_ERR(plane_state);
-
-	if (mode)
-		drm_mode_get_hv_timing(mode, &hdisplay, &vdisplay);
-	else
-		hdisplay = vdisplay = 0;
+	int ret, i;
 
-	ret = drm_atomic_set_crtc_for_plane(plane_state, fb ? crtc : NULL);
+	ret = drm_atomic_add_affected_planes(state, crtc);
 	if (ret)
 		return ret;
-	drm_atomic_set_fb_for_plane(plane_state, fb);
-	plane_state->crtc_x = 0;
-	plane_state->crtc_y = 0;
-	plane_state->crtc_w = hdisplay;
-	plane_state->crtc_h = vdisplay;
-	plane_state->src_x = x << 16;
-	plane_state->src_y = y << 16;
-	plane_state->src_w = hdisplay << 16;
-	plane_state->src_h = vdisplay << 16;
+
+	for_each_new_plane_in_state(state, plane, plane_state, i) {
+		if (plane_state->crtc != crtc)
+			continue;
+
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret)
+			return ret;
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+	}
 
 	return 0;
 }
@@ -9811,7 +9727,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct drm_framebuffer *fb;
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_atomic_state *state = NULL, *restore_state = NULL;
 	struct drm_connector_state *connector_state;
@@ -9879,10 +9794,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
 found:
 	intel_crtc = to_intel_crtc(crtc);
 
-	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
-	if (ret)
-		goto fail;
-
 	state = drm_atomic_state_alloc(dev);
 	restore_state = drm_atomic_state_alloc(dev);
 	if (!state || !restore_state) {
@@ -9914,39 +9825,17 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
 	if (!mode)
 		mode = &load_detect_mode;
 
-	/* We need a framebuffer large enough to accommodate all accesses
-	 * that the plane may generate whilst we perform load detection.
-	 * We can not rely on the fbcon either being present (we get called
-	 * during its initialisation to detect all boot displays, or it may
-	 * not even exist) or that it is large enough to satisfy the
-	 * requested mode.
-	 */
-	fb = mode_fits_in_fbdev(dev, mode);
-	if (fb == NULL) {
-		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
-		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
-	} else
-		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
-	if (IS_ERR(fb)) {
-		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
-		ret = PTR_ERR(fb);
-		goto fail;
-	}
-
-	ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0);
-	drm_framebuffer_put(fb);
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
 	if (ret)
 		goto fail;
 
-	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	ret = intel_modeset_disable_planes(state, crtc);
 	if (ret)
 		goto fail;
 
 	ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector));
 	if (!ret)
 		ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc));
-	if (!ret)
-		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary));
 	if (ret) {
 		DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret);
 		goto fail;
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers

[Daniel Vetter]

On Wed, Dec 20, 2017 at 10:35:43AM +0100, Maarten Lankhorst wrote:
> lock_all_ctx in setplane_internal may return -EINTR, and
> __setplane_internal could return -EDEADLK. Making more
> special cases for fb would make the code even harder to
> read, so the easiest solution is not taking over the fb
> refcount, and making callers responsible for dropping
> the ref.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707
> Fixes: 13736ba3b38b ("drm/legacy: Convert setplane ioctl locking to interruptible.")
> Testcase: kms_atomic_interruptible
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I thought I reviewed this one before already, but probably just a paste
somewhere on irc.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_plane.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 37a93cdffb4a..2c90519576a3 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -558,11 +558,10 @@ int drm_plane_check_pixel_format(const struct drm_plane *plane, u32 format)
>  }
>  
>  /*
> - * setplane_internal - setplane handler for internal callers
> + * __setplane_internal - setplane handler for internal callers
>   *
> - * Note that we assume an extra reference has already been taken on fb.  If the
> - * update fails, this reference will be dropped before return; if it succeeds,
> - * the previous framebuffer (if any) will be unreferenced instead.
> + * This function will take a reference on the new fb for the plane
> + * on success.
>   *
>   * src_{x,y,w,h} are provided in 16.16 fixed point format
>   */
> @@ -630,14 +629,12 @@ static int __setplane_internal(struct drm_plane *plane,
>  	if (!ret) {
>  		plane->crtc = crtc;
>  		plane->fb = fb;
> -		fb = NULL;
> +		drm_framebuffer_get(plane->fb);
>  	} else {
>  		plane->old_fb = NULL;
>  	}
>  
>  out:
> -	if (fb)
> -		drm_framebuffer_put(fb);
>  	if (plane->old_fb)
>  		drm_framebuffer_put(plane->old_fb);
>  	plane->old_fb = NULL;
> @@ -685,6 +682,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_framebuffer *fb = NULL;
> +	int ret;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -717,15 +715,16 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> -	/*
> -	 * setplane_internal will take care of deref'ing either the old or new
> -	 * framebuffer depending on success.
> -	 */
> -	return setplane_internal(plane, crtc, fb,
> -				 plane_req->crtc_x, plane_req->crtc_y,
> -				 plane_req->crtc_w, plane_req->crtc_h,
> -				 plane_req->src_x, plane_req->src_y,
> -				 plane_req->src_w, plane_req->src_h);
> +	ret = setplane_internal(plane, crtc, fb,
> +				plane_req->crtc_x, plane_req->crtc_y,
> +				plane_req->crtc_w, plane_req->crtc_h,
> +				plane_req->src_x, plane_req->src_y,
> +				plane_req->src_w, plane_req->src_h);
> +
> +	if (fb)
> +		drm_framebuffer_put(fb);
> +
> +	return ret;
>  }
>  
>  static int drm_mode_cursor_universal(struct drm_crtc *crtc,
> @@ -788,13 +787,12 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  		src_h = fb->height << 16;
>  	}
>  
> -	/*
> -	 * setplane_internal will take care of deref'ing either the old or new
> -	 * framebuffer depending on success.
> -	 */
>  	ret = __setplane_internal(crtc->cursor, crtc, fb,
> -				crtc_x, crtc_y, crtc_w, crtc_h,
> -				0, 0, src_w, src_h, ctx);
> +				  crtc_x, crtc_y, crtc_w, crtc_h,
> +				  0, 0, src_w, src_h, ctx);
> +
> +	if (fb)
> +		drm_framebuffer_put(fb);
>  
>  	/* Update successful; save new cursor position, if necessary */
>  	if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/framebuffer: Print task that allocated the fb in debug info.

[Daniel Vetter]

On Wed, Dec 20, 2017 at 10:35:44AM +0100, Maarten Lankhorst wrote:
> This is is very useful to finding sources of leaked framebufers.
> The fbcon fb is annotated with [fbcon], to give it a better name
> than kworker.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I think we can polish this more later on, it's a good start.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_fb_helper.c   | 1 +
>  drivers/gpu/drm/drm_framebuffer.c | 2 ++
>  include/drm/drm_framebuffer.h     | 6 ++++++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 04a3a5ce370a..d396d74a7dda 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1848,6 +1848,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  	if (ret < 0)
>  		return ret;
>  
> +	strcpy(fb_helper->fb->comm, "[fbcon]");
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index d63d4c2ac4c8..5a13ff29f4f0 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -664,6 +664,7 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  	INIT_LIST_HEAD(&fb->filp_head);
>  
>  	fb->funcs = funcs;
> +	strcpy(fb->comm, current->comm);
>  
>  	ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB,
>  				    false, drm_framebuffer_free);
> @@ -978,6 +979,7 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>  	struct drm_format_name_buf format_name;
>  	unsigned int i;
>  
> +	drm_printf_indent(p, indent, "allocated by = %s\n", fb->comm);
>  	drm_printf_indent(p, indent, "refcount=%u\n",
>  			  drm_framebuffer_read_refcount(fb));
>  	drm_printf_indent(p, indent, "format=%s\n",
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index dccb897951ba..c50502c656e5 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -121,6 +121,12 @@ struct drm_framebuffer {
>  	 * @base: base modeset object structure, contains the reference count.
>  	 */
>  	struct drm_mode_object base;
> +
> +	/**
> +	 * @comm: Name of the process allocating the fb, used for fb dumping.
> +	 */
> +	char comm[TASK_COMM_LEN];
> +
>  	/**
>  	 * @format: framebuffer format information
>  	 */
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/i915: Disable all planes for load detection, v2.

[Daniel Vetter]

On Wed, Dec 20, 2017 at 10:35:45AM +0100, Maarten Lankhorst wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We don't need any active planes during load detection, so just disable
> them all. This saves us from having to come up with a suitable
> framebuffer. And we also avoid leaving sprite/cursor planes on and
> potentially presenting them at a peculiar location during the load
> detection.
> 
> Changes since v1 (Maarten):
> - Add missing call to add_all_affected_planes.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707

Less code, I like. And I think we have enough load-detect machines (+ plus
the nasty igt to do it anywhere we still have native vga) to have
reasonable ensurance it actually works.

But maybe let's soak this in -next first for a while, then cherry-pick
over after a few weeks once it's solid.

With the missing Fixes: line added.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 147 +++++------------------------------
>  1 file changed, 18 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7e833268c9df..ef61f9a75c93 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9689,111 +9689,27 @@ intel_framebuffer_create(struct drm_i915_gem_object *obj,
>  	return ERR_PTR(ret);
>  }
>  
> -static u32
> -intel_framebuffer_pitch_for_width(int width, int bpp)
> -{
> -	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> -	return ALIGN(pitch, 64);
> -}
> -
> -static u32
> -intel_framebuffer_size_for_mode(const struct drm_display_mode *mode, int bpp)
> -{
> -	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
> -	return PAGE_ALIGN(pitch * mode->vdisplay);
> -}
> -
> -static struct drm_framebuffer *
> -intel_framebuffer_create_for_mode(struct drm_device *dev,
> -				  const struct drm_display_mode *mode,
> -				  int depth, int bpp)
> -{
> -	struct drm_framebuffer *fb;
> -	struct drm_i915_gem_object *obj;
> -	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> -
> -	obj = i915_gem_object_create(to_i915(dev),
> -				    intel_framebuffer_size_for_mode(mode, bpp));
> -	if (IS_ERR(obj))
> -		return ERR_CAST(obj);
> -
> -	mode_cmd.width = mode->hdisplay;
> -	mode_cmd.height = mode->vdisplay;
> -	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width,
> -								bpp);
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
> -
> -	fb = intel_framebuffer_create(obj, &mode_cmd);
> -	if (IS_ERR(fb))
> -		i915_gem_object_put(obj);
> -
> -	return fb;
> -}
> -
> -static struct drm_framebuffer *
> -mode_fits_in_fbdev(struct drm_device *dev,
> -		   const struct drm_display_mode *mode)
> -{
> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_i915_gem_object *obj;
> -	struct drm_framebuffer *fb;
> -
> -	if (!dev_priv->fbdev)
> -		return NULL;
> -
> -	if (!dev_priv->fbdev->fb)
> -		return NULL;
> -
> -	obj = dev_priv->fbdev->fb->obj;
> -	BUG_ON(!obj);
> -
> -	fb = &dev_priv->fbdev->fb->base;
> -	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
> -							       fb->format->cpp[0] * 8))
> -		return NULL;
> -
> -	if (obj->base.size < mode->vdisplay * fb->pitches[0])
> -		return NULL;
> -
> -	drm_framebuffer_get(fb);
> -	return fb;
> -#else
> -	return NULL;
> -#endif
> -}
> -
> -static int intel_modeset_setup_plane_state(struct drm_atomic_state *state,
> -					   struct drm_crtc *crtc,
> -					   const struct drm_display_mode *mode,
> -					   struct drm_framebuffer *fb,
> -					   int x, int y)
> +static int intel_modeset_disable_planes(struct drm_atomic_state *state,
> +					struct drm_crtc *crtc)
>  {
> +	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
> -	int hdisplay, vdisplay;
> -	int ret;
> -
> -	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
> -	if (IS_ERR(plane_state))
> -		return PTR_ERR(plane_state);
> -
> -	if (mode)
> -		drm_mode_get_hv_timing(mode, &hdisplay, &vdisplay);
> -	else
> -		hdisplay = vdisplay = 0;
> +	int ret, i;
>  
> -	ret = drm_atomic_set_crtc_for_plane(plane_state, fb ? crtc : NULL);
> +	ret = drm_atomic_add_affected_planes(state, crtc);
>  	if (ret)
>  		return ret;
> -	drm_atomic_set_fb_for_plane(plane_state, fb);
> -	plane_state->crtc_x = 0;
> -	plane_state->crtc_y = 0;
> -	plane_state->crtc_w = hdisplay;
> -	plane_state->crtc_h = vdisplay;
> -	plane_state->src_x = x << 16;
> -	plane_state->src_y = y << 16;
> -	plane_state->src_w = hdisplay << 16;
> -	plane_state->src_h = vdisplay << 16;
> +
> +	for_each_new_plane_in_state(state, plane, plane_state, i) {
> +		if (plane_state->crtc != crtc)
> +			continue;
> +
> +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +		if (ret)
> +			return ret;
> +
> +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> +	}
>  
>  	return 0;
>  }
> @@ -9811,7 +9727,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_framebuffer *fb;
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_atomic_state *state = NULL, *restore_state = NULL;
>  	struct drm_connector_state *connector_state;
> @@ -9879,10 +9794,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
>  found:
>  	intel_crtc = to_intel_crtc(crtc);
>  
> -	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> -	if (ret)
> -		goto fail;
> -
>  	state = drm_atomic_state_alloc(dev);
>  	restore_state = drm_atomic_state_alloc(dev);
>  	if (!state || !restore_state) {
> @@ -9914,39 +9825,17 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
>  	if (!mode)
>  		mode = &load_detect_mode;
>  
> -	/* We need a framebuffer large enough to accommodate all accesses
> -	 * that the plane may generate whilst we perform load detection.
> -	 * We can not rely on the fbcon either being present (we get called
> -	 * during its initialisation to detect all boot displays, or it may
> -	 * not even exist) or that it is large enough to satisfy the
> -	 * requested mode.
> -	 */
> -	fb = mode_fits_in_fbdev(dev, mode);
> -	if (fb == NULL) {
> -		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
> -		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
> -	} else
> -		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
> -	if (IS_ERR(fb)) {
> -		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
> -		ret = PTR_ERR(fb);
> -		goto fail;
> -	}
> -
> -	ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0);
> -	drm_framebuffer_put(fb);
> +	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
>  	if (ret)
>  		goto fail;
>  
> -	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
> +	ret = intel_modeset_disable_planes(state, crtc);
>  	if (ret)
>  		goto fail;
>  
>  	ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector));
>  	if (!ret)
>  		ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc));
> -	if (!ret)
> -		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary));
>  	if (ret) {
>  		DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret);
>  		goto fail;
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/i915: Disable all planes for load detection, v2.

[Maarten Lankhorst]

Op 20-12-17 om 11:28 schreef Daniel Vetter:
> On Wed, Dec 20, 2017 at 10:35:45AM +0100, Maarten Lankhorst wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> We don't need any active planes during load detection, so just disable
>> them all. This saves us from having to come up with a suitable
>> framebuffer. And we also avoid leaving sprite/cursor planes on and
>> potentially presenting them at a peculiar location during the load
>> detection.
>>
>> Changes since v1 (Maarten):
>> - Add missing call to add_all_affected_planes.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707
> Less code, I like. And I think we have enough load-detect machines (+ plus
> the nasty igt to do it anywhere we still have native vga) to have
> reasonable ensurance it actually works.
>
> But maybe let's soak this in -next first for a while, then cherry-pick
> over after a few weeks once it's solid.
>
> With the missing Fixes: line added.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Pushed, didn't add fixes since it's an ancient bug, for a piece of code that changed a lot.
Since in most cases the fbcon fb will be leaked, the impact is just a warning and not even
a real leak. :)

Thanks for review,

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel