* [PATCH 1/3] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers
2017-12-20 9:35 [PATCH 0/3] drm: Plug framebuffer leaks Maarten Lankhorst
@ 2017-12-20 9:35 ` Maarten Lankhorst
2017-12-20 10:23 ` Daniel Vetter
2017-12-20 9:35 ` [PATCH 2/3] drm/framebuffer: Print task that allocated the fb in debug info Maarten Lankhorst
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Maarten Lankhorst @ 2017-12-20 9:35 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers
2017-12-20 9:35 ` [PATCH 1/3] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers Maarten Lankhorst
@ 2017-12-20 10:23 ` Daniel Vetter
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2017-12-20 10:23 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] drm/framebuffer: Print task that allocated the fb in debug info.
2017-12-20 9:35 [PATCH 0/3] drm: Plug framebuffer leaks Maarten Lankhorst
2017-12-20 9:35 ` [PATCH 1/3] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers Maarten Lankhorst
@ 2017-12-20 9:35 ` Maarten Lankhorst
2017-12-20 10:26 ` Daniel Vetter
2017-12-20 9:35 ` [PATCH 3/3] drm/i915: Disable all planes for load detection, v2 Maarten Lankhorst
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Maarten Lankhorst @ 2017-12-20 9:35 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] drm/framebuffer: Print task that allocated the fb in debug info.
2017-12-20 9:35 ` [PATCH 2/3] drm/framebuffer: Print task that allocated the fb in debug info Maarten Lankhorst
@ 2017-12-20 10:26 ` Daniel Vetter
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2017-12-20 10:26 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] drm/i915: Disable all planes for load detection, v2.
2017-12-20 9:35 [PATCH 0/3] drm: Plug framebuffer leaks Maarten Lankhorst
2017-12-20 9:35 ` [PATCH 1/3] drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers Maarten Lankhorst
2017-12-20 9:35 ` [PATCH 2/3] drm/framebuffer: Print task that allocated the fb in debug info Maarten Lankhorst
@ 2017-12-20 9:35 ` Maarten Lankhorst
2017-12-20 10:28 ` Daniel Vetter
2017-12-20 10:15 ` ✓ Fi.CI.BAT: success for drm: Plug framebuffer leaks Patchwork
2017-12-20 11:30 ` ✓ Fi.CI.IGT: " Patchwork
4 siblings, 1 reply; 10+ messages in thread
From: Maarten Lankhorst @ 2017-12-20 9:35 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] drm/i915: Disable all planes for load detection, v2.
2017-12-20 9:35 ` [PATCH 3/3] drm/i915: Disable all planes for load detection, v2 Maarten Lankhorst
@ 2017-12-20 10:28 ` Daniel Vetter
2017-12-21 9:58 ` Maarten Lankhorst
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2017-12-20 10:28 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] drm/i915: Disable all planes for load detection, v2.
2017-12-20 10:28 ` Daniel Vetter
@ 2017-12-21 9:58 ` Maarten Lankhorst
0 siblings, 0 replies; 10+ messages in thread
From: Maarten Lankhorst @ 2017-12-21 9:58 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for drm: Plug framebuffer leaks.
2017-12-20 9:35 [PATCH 0/3] drm: Plug framebuffer leaks Maarten Lankhorst
` (2 preceding siblings ...)
2017-12-20 9:35 ` [PATCH 3/3] drm/i915: Disable all planes for load detection, v2 Maarten Lankhorst
@ 2017-12-20 10:15 ` Patchwork
2017-12-20 11:30 ` ✓ Fi.CI.IGT: " Patchwork
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2017-12-20 10:15 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm: Plug framebuffer leaks.
URL : https://patchwork.freedesktop.org/series/35615/
State : success
== Summary ==
Series 35615v1 drm: Plug framebuffer leaks.
https://patchwork.freedesktop.org/api/1.0/series/35615/revisions/1/mbox/
Test debugfs_test:
Subgroup read_all_entries:
dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989 +2
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
dmesg-warn -> PASS (fi-kbl-r) fdo#104172
Test kms_psr_sink_crc:
Subgroup psr_basic:
dmesg-warn -> PASS (fi-skl-6700hq) fdo#101144
Test drv_module_reload:
Subgroup basic-reload:
dmesg-warn -> PASS (fi-gdg-551) fdo#102707
fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:435s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:440s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:380s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:493s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:275s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:489s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:497s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:473s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:464s
fi-elk-e7500 total:229 pass:167 dwarn:15 dfail:0 fail:0 skip:46
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:263s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:530s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:405s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:411s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:390s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:471s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:430s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:476s
fi-kbl-7560u total:288 pass:268 dwarn:1 dfail:0 fail:0 skip:19 time:519s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:471s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:524s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:575s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:439s
fi-skl-6600u total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:530s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:556s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:509s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:486s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:449s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:549s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:409s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:586s
fi-cnl-y total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:645s
94dca9e73099883228b4b7ad6a7077a7992f4061 drm-tip: 2017y-12m-20d-09h-26m-52s UTC integration manifest
0cd8655d5322 drm/i915: Disable all planes for load detection, v2.
10ae07c4090e drm/framebuffer: Print task that allocated the fb in debug info.
64719c3703a6 drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7547/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* ✓ Fi.CI.IGT: success for drm: Plug framebuffer leaks.
2017-12-20 9:35 [PATCH 0/3] drm: Plug framebuffer leaks Maarten Lankhorst
` (3 preceding siblings ...)
2017-12-20 10:15 ` ✓ Fi.CI.BAT: success for drm: Plug framebuffer leaks Patchwork
@ 2017-12-20 11:30 ` Patchwork
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2017-12-20 11:30 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm: Plug framebuffer leaks.
URL : https://patchwork.freedesktop.org/series/35615/
State : success
== Summary ==
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-pri-indfb-multidraw:
fail -> PASS (shard-snb) fdo#103167
Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
fail -> PASS (shard-snb) fdo#101623
Test kms_draw_crc:
Subgroup draw-method-xrgb2101010-mmap-gtt-xtiled:
skip -> PASS (shard-hsw)
Test gem_exec_suspend:
Subgroup basic-s4-devices:
pass -> INCOMPLETE (shard-hsw) fdo#103990
Test kms_flip:
Subgroup flip-vs-fences:
incomplete -> PASS (shard-hsw)
Subgroup vblank-vs-modeset-suspend:
pass -> SKIP (shard-snb) fdo#102365
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103990 https://bugs.freedesktop.org/show_bug.cgi?id=103990
fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
shard-hsw total:2678 pass:1521 dwarn:1 dfail:0 fail:10 skip:1145 time:9276s
shard-snb total:2712 pass:1308 dwarn:1 dfail:0 fail:11 skip:1392 time:8058s
Blacklisted hosts:
shard-apl total:2640 pass:1646 dwarn:1 dfail:0 fail:25 skip:967 time:13367s
shard-kbl total:2640 pass:1767 dwarn:1 dfail:0 fail:25 skip:846 time:10902s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7547/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread