| Message ID | 20171208095436.63626-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 08, 2017 at 10:54:36AM +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> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Please push to drm-misc-fixes as soon as CI approves it fully (which is probably next Monday, no rush). -Daniel > --- > 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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Patchwork (2017-12-08 11:35:05) > == Series Details == > > Series: drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers > URL : https://patchwork.freedesktop.org/series/35077/ > State : success > > == Summary == > > Series 35077v1 drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers > https://patchwork.freedesktop.org/api/1.0/series/35077/revisions/1/mbox/ > > Test debugfs_test: > Subgroup read_all_entries: > dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989 > dmesg-warn -> PASS (fi-bdw-gvtdvm) fdo#103938 +1 > > fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989 > fdo#103938 https://bugs.freedesktop.org/show_bug.cgi?id=103938 gdg still defeats us: <7>[ 279.565692] [IGT] drv_module_reload: starting subtest basic-reload <7>[ 279.618846] [drm:intel_disable_pipe [i915]] disabling pipe A <7>[ 279.639614] [drm:i9xx_get_fifo_size [i915]] FIFO size - (0x00001d9c) A: 28 <7>[ 279.639663] [drm:i9xx_get_fifo_size [i915]] FIFO size - (0x00001d9c) B: 31 <7>[ 279.639707] [drm:i9xx_update_wm [i915]] FIFO watermarks - A: 26, B: 29 <7>[ 279.639753] [drm:i9xx_update_wm [i915]] Setting FIFO watermarks - A: 26, B: 29, C: 2, SR 1 <7>[ 279.639820] [drm:intel_atomic_commit_tail [i915]] [ENCODER:39:CRT] <7>[ 279.639882] [drm:intel_atomic_commit_tail [i915]] [ENCODER:40:SDVO B] <7>[ 279.639940] [drm:verify_connector_state.isra.77 [i915]] [CONNECTOR:38:VGA-1] <7>[ 279.640095] [drm:intel_atomic_commit_tail [i915]] [CRTC:32:pipe A] <4>[ 279.647674] WARNING: CPU: 0 PID: 2462 at drivers/gpu/drm/drm_mode_config.c:470 drm_mode_config_cleanup+0x28d/0x2b0 <4>[ 279.647680] Modules linked in: vgem i915(-) intel_powerclamp lpc_ich tg3 ptp pps_core prime_numbers [last unloaded: vgem] <4>[ 279.647718] CPU: 0 PID: 2462 Comm: drv_module_relo Tainted: G U 4.15.0-rc2-CI-Patchwork_7451+ #1 <4>[ 279.647722] Hardware name: Dell Inc. OptiPlex GX280 /0G8310, BIOS A04 02/09/2005 <4>[ 279.647726] task: 00000000134e8d08 task.stack: 000000005a052046 <4>[ 279.647731] RIP: 0010:drm_mode_config_cleanup+0x28d/0x2b0 <4>[ 279.647735] RSP: 0018:ffffc9000029bdb8 EFLAGS: 00010287 <4>[ 279.647742] RAX: ffff8800341f71c0 RBX: ffff880030690950 RCX: 0000000000000001 <4>[ 279.647746] RDX: ffff880030690978 RSI: 0000000000000000 RDI: ffff880030690950 <4>[ 279.647749] RBP: ffff880030690000 R08: 0000000000000000 R09: 0000000000000000 <4>[ 279.647752] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880030690748 <4>[ 279.647756] R13: ffffffffa0233750 R14: ffff88003d1625a8 R15: ffffffffa02337c8 <4>[ 279.647760] FS: 00007f7e93c6a8c0(0000) GS:ffff88003f400000(0000) knlGS:0000000000000000 <4>[ 279.647764] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4>[ 279.647768] CR2: 00007f78efc3b000 CR3: 000000003b4f7000 CR4: 00000000000006f0 <4>[ 279.647772] Call Trace: <4>[ 279.647875] intel_modeset_cleanup+0x89/0xd0 [i915] <4>[ 279.647918] i915_driver_unload+0x98/0x180 [i915] <4>[ 279.647961] i915_pci_remove+0x10/0x20 [i915] <4>[ 279.647969] pci_device_remove+0x31/0xa0 <4>[ 279.647977] device_release_driver_internal+0x15b/0x230 <4>[ 279.647985] driver_detach+0x35/0x70 <4>[ 279.647993] bus_remove_driver+0x53/0xd0 <4>[ 279.647999] pci_unregister_driver+0x39/0xa0 <4>[ 279.648010] SyS_delete_module+0x180/0x1d0 <4>[ 279.648021] entry_SYSCALL_64_fastpath+0x1c/0x89 <4>[ 279.648026] RIP: 0033:0x7f7e92182287 <4>[ 279.648029] RSP: 002b:00007fff782ed478 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0 <4>[ 279.648036] RAX: ffffffffffffffda RBX: 00005558d2c0bfe0 RCX: 00007f7e92182287 <4>[ 279.648040] RDX: 0000000000000001 RSI: 0000000000000800 RDI: 00005558d2c0b878 <4>[ 279.648044] RBP: 00007f7e92437400 R08: 0000000000000000 R09: 0000000000000080 <4>[ 279.648047] R10: 0000000000000885 R11: 0000000000000206 R12: 0000000000000000 <4>[ 279.648050] R13: 00005558d2c08410 R14: 00005558d2c0bfe0 R15: 00007fff782ec448 <4>[ 279.648068] Code: 48 89 da 31 f6 48 c7 c7 2e c6 c6 81 e8 5d f5 ff ff 48 89 e7 e8 d5 a7 ff ff 48 85 c0 75 de 48 89 e7 e8 a8 a8 ff ff e9 0b fe ff ff <0f> ff e9 f6 fe ff ff 0f ff 48 83 c4 28 5b 5d 41 5c 41 5d 41 5e <4>[ 279.648257] ---[ end trace b879a808700d8111 ]--- <7>[ 279.648264] [leaked fb] framebuffer[49]: <7>[ 279.648268] [leaked fb] refcount=63 <7>[ 279.648275] [leaked fb] format=XR24 little-endian (0x34325258) <7>[ 279.648280] [leaked fb] modifier=0x0 <7>[ 279.648284] [leaked fb] size=1024x768 <7>[ 279.648288] [leaked fb] layers: <7>[ 279.648292] [leaked fb] size[0]=1024x768 <7>[ 279.648296] [leaked fb] pitch[0]=4096 <7>[ 279.648300] [leaked fb] offset[0]=0 <7>[ 279.648304] [leaked fb] obj[0]:(null)
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) {
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(-)