| Message ID | 20170927122642.18628-1-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 27, 2017 at 08:26:42PM +0800, Jeffy Chen wrote: > Free the drm_atomic_state allocated by drm_atomic_helper_suspend(). > > Fixes: 5a5873830972 ("drm/rockchip: Use atomic PM helpers") > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > index 76d63de5921d..80235b672deb 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > @@ -15,6 +15,7 @@ > */ > > #include <drm/drmP.h> > +#include <drm/drm_atomic.h> > #include <drm/drm_crtc_helper.h> > #include <drm/drm_fb_helper.h> > #include <drm/drm_gem_cma_helper.h> > @@ -299,6 +300,7 @@ static int rockchip_drm_sys_resume(struct device *dev) > > priv = drm->dev_private; > drm_atomic_helper_resume(drm, priv->state); > + drm_atomic_state_put(priv->state); Won't this be freed for you eventually in commit_tail()? Sean > rockchip_drm_fb_resume(drm); > drm_kms_helper_poll_enable(drm); > > -- > 2.11.0 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Sean, On 09/28/2017 04:27 AM, Sean Paul wrote: >> >@@ -299,6 +300,7 @@ static int rockchip_drm_sys_resume(struct device *dev) >> > >> > priv = drm->dev_private; >> > drm_atomic_helper_resume(drm, priv->state); >> >+ drm_atomic_state_put(priv->state); > Won't this be freed for you eventually in commit_tail()? > i think the drm_atomic_state_put in commit_tail is paired to the drm_atomic_state_get in drm_atomic_helper_commit. and the kmemleak shows(after a few suspend/resume): unreferenced object 0xffffffc0ce0fa400 (size 256): ... backtrace: [<ffffffc00034a9a8>] __save_stack_trace+0x48/0x6c [<ffffffc00034b088>] create_object+0x138/0x254 [<ffffffc0009d5504>] kmemleak_alloc+0x58/0x8c [<ffffffc000345f78>] __kmalloc+0x1d4/0x2a0 [<ffffffc0006765bc>] usb_alloc_urb+0x30/0x60 [<ffffffbffc128598>] alloc_ctrl_urb+0x38/0x120 [btusb] [<ffffffbffc129e7c>] btusb_send_frame+0x64/0xf8 [btusb] checking the current code, i saw only i915/intel_display.c has this drm_atomic_state_put for the state allocated by drm_atomic_helper_suspend(), there're many drivers missing that(or maybe they free it in some other way?) > Sean >
Hi Sean, On 09/28/2017 11:16 AM, jeffy wrote: > Hi Sean, > > On 09/28/2017 04:27 AM, Sean Paul wrote: >>> >@@ -299,6 +300,7 @@ static int rockchip_drm_sys_resume(struct device >>> *dev) >>> > >>> > priv = drm->dev_private; >>> > drm_atomic_helper_resume(drm, priv->state); >>> >+ drm_atomic_state_put(priv->state); >> Won't this be freed for you eventually in commit_tail()? >> > i think the drm_atomic_state_put in commit_tail is paired to the > drm_atomic_state_get in drm_atomic_helper_commit. > > and the kmemleak shows(after a few suspend/resume): > > unreferenced object 0xffffffc0ce0fa400 (size 256): > ... > backtrace: > [<ffffffc00034a9a8>] __save_stack_trace+0x48/0x6c > [<ffffffc00034b088>] create_object+0x138/0x254 > [<ffffffc0009d5504>] kmemleak_alloc+0x58/0x8c > [<ffffffc000345f78>] __kmalloc+0x1d4/0x2a0 > [<ffffffc0006765bc>] usb_alloc_urb+0x30/0x60 > [<ffffffbffc128598>] alloc_ctrl_urb+0x38/0x120 [btusb] > [<ffffffbffc129e7c>] btusb_send_frame+0x64/0xf8 [btusb] oops, the log is wrong... it should be: unreferenced object 0xffffffc0c75fe600 (size 256): comm "bash", pid 168, jiffies 4294748032 (age 557.932s) hex dump (first 32 bytes): e0 4a 02 ce c0 ff ff ff 00 98 f2 c8 c0 ff ff ff .J.............. 00 98 f2 c8 c0 ff ff ff 00 45 9f cb c0 ff ff ff .........E...... backtrace: [<ffffff9008c76ba0>] kmemleak_alloc+0x58/0x8c [<ffffff900828fe70>] __kmalloc+0x1dc/0x2d4 [<ffffff9008647698>] drm_atomic_state_init+0xb8/0x114 [<ffffff9008649ed8>] drm_atomic_state_alloc+0x70/0xa4 [<ffffff90086209f8>] drm_atomic_helper_duplicate_state+0x94/0x210 [<ffffff9008621184>] drm_atomic_helper_suspend+0xb4/0x160 [<ffffff9008666f18>] rockchip_drm_sys_suspend+0x5c/0xa0 [<ffffff9008697f18>] platform_pm_suspend+0x58/0x74 [<ffffff90086a7f98>] dpm_run_callback+0x13c/0x2f0 [<ffffff90086a9064>] __device_suspend+0x28c/0x3fc [<ffffff90086ab9ec>] dpm_suspend+0x1cc/0x50c [<ffffff90086ac5a4>] dpm_suspend_start+0x8c/0xa4 [<ffffff90081226a0>] suspend_devices_and_enter+0x12c/0xd84 [<ffffff9008123d30>] pm_suspend+0xa38/0xb98 [<ffffff9008120f94>] state_store+0xe0/0x108 [<ffffff9008c63e7c>] kobj_attr_store+0x48/0x58 since it's allocated in the drm_atomic_helper_suspend(), maybe it would make more sense to unref it in drm_atomic_helper_resume(), i'll send a v2 patch for it :) > > > > > checking the current code, i saw only i915/intel_display.c has this > drm_atomic_state_put for the state allocated by > drm_atomic_helper_suspend(), there're many drivers missing that(or maybe > they free it in some other way?) > >> Sean >> >
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index 76d63de5921d..80235b672deb 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -15,6 +15,7 @@ */ #include <drm/drmP.h> +#include <drm/drm_atomic.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> #include <drm/drm_gem_cma_helper.h> @@ -299,6 +300,7 @@ static int rockchip_drm_sys_resume(struct device *dev) priv = drm->dev_private; drm_atomic_helper_resume(drm, priv->state); + drm_atomic_state_put(priv->state); rockchip_drm_fb_resume(drm); drm_kms_helper_poll_enable(drm);
Free the drm_atomic_state allocated by drm_atomic_helper_suspend(). Fixes: 5a5873830972 ("drm/rockchip: Use atomic PM helpers") Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 ++ 1 file changed, 2 insertions(+)