[Intel-gfx] [PATCH] drm/fbdev: Update legacy plane->fb refcounting for atomic restore

[Daniel Vetter]

From: Matt Roper 

Starting with commit

commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
Author: Rob Clark 
Date:   Tue Aug 25 15:36:00 2015 -0400

drm/i915: enable atomic fb-helper

I've been seeing some panics on i915 when the DRM master shuts down that appear
to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
dropping our initial FB's reference count to 0 and freeing it, which causes a
crash when we try to restore it later).  Digging deeper, the state FB
refcounting is working as expected, but we seem to be missing proper
refcounting on the legacy plane->fb pointers in the new atomic fbdev code.

Tracking plane->old_fb and then doing a ref/unref at the end of the
fbdev restore like we do in the legacy ioctl's ensures we don't miscount
references on plane->fb and avoids the panics.

v2 from Daniel:

Really do what the atomic ioctl does:
- Also update plane->fb and plane->crtc.
- Clear out plane->old_fb on failures too.

Cc: Rob Clark 
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Matt Roper  (v1)
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_fb_helper.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 64fc5ca8fda2..1b564a69f561 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -352,6 +352,8 @@ retry:
drm_for_each_plane(plane, dev) {
struct drm_plane_state *plane_state;
 
+   plane->old_fb = plane->fb;
+
plane_state = drm_atomic_get_plane_state(state, plane);
if (IS_ERR(plane_state)) {
ret = PTR_ERR(plane_state);
@@ -382,6 +384,20 @@ retry:
}
 
ret = drm_atomic_commit(state);
+
+   drm_for_each_plane(plane, dev) {
+   if (ret == 0) {
+   struct drm_framebuffer *new_fb = plane->state->fb;
+   if (new_fb)
+   drm_framebuffer_reference(new_fb);
+   plane->fb = new_fb;
+   plane->crtc = plane->state->crtc;
+
+   if (plane->old_fb)
+   drm_framebuffer_unreference(plane->old_fb);
+   }
+   }
+
if (ret != 0)
goto fail;
 
-- 
2.5.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/fbdev: Update legacy plane->fb refcounting for atomic restore

[David Herrmann]

Hi

On Tue, Sep 22, 2015 at 12:56 PM, Daniel Vetter  wrote:
> From: Matt Roper 
>
> Starting with commit
>
> commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
> Author: Rob Clark 
> Date:   Tue Aug 25 15:36:00 2015 -0400
>
> drm/i915: enable atomic fb-helper
>
> I've been seeing some panics on i915 when the DRM master shuts down that 
> appear
> to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
> dropping our initial FB's reference count to 0 and freeing it, which causes a
> crash when we try to restore it later).  Digging deeper, the state FB
> refcounting is working as expected, but we seem to be missing proper
> refcounting on the legacy plane->fb pointers in the new atomic fbdev code.
>
> Tracking plane->old_fb and then doing a ref/unref at the end of the
> fbdev restore like we do in the legacy ioctl's ensures we don't miscount
> references on plane->fb and avoids the panics.
>
> v2 from Daniel:
>
> Really do what the atomic ioctl does:
> - Also update plane->fb and plane->crtc.
> - Clear out plane->old_fb on failures too.
>
> v3: git add everything. Oops.
>
> Cc: Rob Clark 
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Matt Roper  (v1)
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 17 +
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 64fc5ca8fda2..8af522afdfc1 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -352,6 +352,8 @@ retry:
> drm_for_each_plane(plane, dev) {
> struct drm_plane_state *plane_state;
>
> +   plane->old_fb = plane->fb;
> +
> plane_state = drm_atomic_get_plane_state(state, plane);
> if (IS_ERR(plane_state)) {
> ret = PTR_ERR(plane_state);
> @@ -382,6 +384,21 @@ retry:
> }
>
> ret = drm_atomic_commit(state);
> +
> +   drm_for_each_plane(plane, dev) {
> +   if (ret == 0) {
> +   struct drm_framebuffer *new_fb = plane->state->fb;
> +   if (new_fb)
> +   drm_framebuffer_reference(new_fb);
> +   plane->fb = new_fb;
> +   plane->crtc = plane->state->crtc;
> +
> +   if (plane->old_fb)
> +   drm_framebuffer_unreference(plane->old_fb);
> +   }
> +   plane->old_fb = NULL;

You still leak "old_fb" if something jumps to "fail:" before
drm_atomic_commit() is called. But I don't mind:

Reviewed-by: David Herrmann 

Thanks
David

> +   }
> +
> if (ret != 0)
> goto fail;
>
> --
> 2.5.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/fbdev: Update legacy plane->fb refcounting for atomic restore

[Maarten Lankhorst]

Hey,

Op 22-09-15 om 11:55 schreef Daniel Vetter:
> From: Matt Roper 
>
> Starting with commit
>
> commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
> Author: Rob Clark 
> Date:   Tue Aug 25 15:36:00 2015 -0400
>
> drm/i915: enable atomic fb-helper
>
> I've been seeing some panics on i915 when the DRM master shuts down that 
> appear
> to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
> dropping our initial FB's reference count to 0 and freeing it, which causes a
> crash when we try to restore it later).  Digging deeper, the state FB
> refcounting is working as expected, but we seem to be missing proper
> refcounting on the legacy plane->fb pointers in the new atomic fbdev code.
>
> Tracking plane->old_fb and then doing a ref/unref at the end of the
> fbdev restore like we do in the legacy ioctl's ensures we don't miscount
> references on plane->fb and avoids the panics.
>
> v2 from Daniel:
>
> Really do what the atomic ioctl does:
> - Also update plane->fb and plane->crtc.
> - Clear out plane->old_fb on failures too.
>
> Cc: Rob Clark 
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Matt Roper  (v1)
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 16 
>  1 file changed, 16 insertions(+)

Looks sane, but I see a lot of this boilerplate for the plane->fb updates, and 
we often get it wrong. Like for example in drm_mode_atomic_ioctl.

Could all the plane->fb and old_fb updates be done by drm_atomic_commit or 
async_commit instead?

> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 64fc5ca8fda2..1b564a69f561 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -352,6 +352,8 @@ retry:
>   drm_for_each_plane(plane, dev) {
>   struct drm_plane_state *plane_state;
>  
> + plane->old_fb = plane->fb;
> +
>   plane_state = drm_atomic_get_plane_state(state, plane);
>   if (IS_ERR(plane_state)) {
>   ret = PTR_ERR(plane_state);
> @@ -382,6 +384,20 @@ retry:
>   }
>  
>   ret = drm_atomic_commit(state);
> +
> + drm_for_each_plane(plane, dev) {
> + if (ret == 0) {
> + struct drm_framebuffer *new_fb = plane->state->fb;
> + if (new_fb)
> + drm_framebuffer_reference(new_fb);
> + plane->fb = new_fb;
> + plane->crtc = plane->state->crtc;
> +
> + if (plane->old_fb)
> + drm_framebuffer_unreference(plane->old_fb);
> + }
> + }
> +
>   if (ret != 0)
>   goto fail;
>  

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/fbdev: Update legacy plane->fb refcounting for atomic restore

[David Herrmann]

Hi

On Tue, Sep 22, 2015 at 12:33 PM, Maarten Lankhorst
 wrote:
> Hey,
>
> Op 22-09-15 om 11:55 schreef Daniel Vetter:
>> From: Matt Roper 
>>
>> Starting with commit
>>
>> commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
>> Author: Rob Clark 
>> Date:   Tue Aug 25 15:36:00 2015 -0400
>>
>> drm/i915: enable atomic fb-helper
>>
>> I've been seeing some panics on i915 when the DRM master shuts down that 
>> appear
>> to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
>> dropping our initial FB's reference count to 0 and freeing it, which causes a
>> crash when we try to restore it later).  Digging deeper, the state FB
>> refcounting is working as expected, but we seem to be missing proper
>> refcounting on the legacy plane->fb pointers in the new atomic fbdev code.
>>
>> Tracking plane->old_fb and then doing a ref/unref at the end of the
>> fbdev restore like we do in the legacy ioctl's ensures we don't miscount
>> references on plane->fb and avoids the panics.
>>
>> v2 from Daniel:
>>
>> Really do what the atomic ioctl does:
>> - Also update plane->fb and plane->crtc.
>> - Clear out plane->old_fb on failures too.
>>
>> Cc: Rob Clark 
>> Cc: intel-gfx@lists.freedesktop.org
>> Signed-off-by: Matt Roper  (v1)
>> Signed-off-by: Daniel Vetter 
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c | 16 
>>  1 file changed, 16 insertions(+)
>
> Looks sane, but I see a lot of this boilerplate for the plane->fb updates, 
> and we often get it wrong. Like for example in drm_mode_atomic_ioctl.
>
> Could all the plane->fb and old_fb updates be done by drm_atomic_commit or 
> async_commit instead?

If we decide to not care for stale "old_fb" pointers, I agree we
should make the commit-helpers do it.

Thanks
David
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/fbdev: Update legacy plane->fb refcounting for atomic restore

[David Herrmann]

Hi

On Tue, Sep 22, 2015 at 11:55 AM, Daniel Vetter  wrote:
> From: Matt Roper 
>
> Starting with commit
>
> commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
> Author: Rob Clark 
> Date:   Tue Aug 25 15:36:00 2015 -0400
>
> drm/i915: enable atomic fb-helper
>
> I've been seeing some panics on i915 when the DRM master shuts down that 
> appear
> to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
> dropping our initial FB's reference count to 0 and freeing it, which causes a
> crash when we try to restore it later).  Digging deeper, the state FB
> refcounting is working as expected, but we seem to be missing proper
> refcounting on the legacy plane->fb pointers in the new atomic fbdev code.
>
> Tracking plane->old_fb and then doing a ref/unref at the end of the
> fbdev restore like we do in the legacy ioctl's ensures we don't miscount
> references on plane->fb and avoids the panics.
>
> v2 from Daniel:
>
> Really do what the atomic ioctl does:
> - Also update plane->fb and plane->crtc.
> - Clear out plane->old_fb on failures too.
>
> Cc: Rob Clark 
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Matt Roper  (v1)
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 64fc5ca8fda2..1b564a69f561 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -352,6 +352,8 @@ retry:
> drm_for_each_plane(plane, dev) {
> struct drm_plane_state *plane_state;
>
> +   plane->old_fb = plane->fb;
> +
> plane_state = drm_atomic_get_plane_state(state, plane);
> if (IS_ERR(plane_state)) {
> ret = PTR_ERR(plane_state);
> @@ -382,6 +384,20 @@ retry:
> }
>
> ret = drm_atomic_commit(state);
> +
> +   drm_for_each_plane(plane, dev) {
> +   if (ret == 0) {
> +   struct drm_framebuffer *new_fb = plane->state->fb;
> +   if (new_fb)
> +   drm_framebuffer_reference(new_fb);
> +   plane->fb = new_fb;
> +   plane->crtc = plane->state->crtc;
> +
> +   if (plane->old_fb)
> +   drm_framebuffer_unreference(plane->old_fb);
> +   }
> +   }
> +
> if (ret != 0)
> goto fail;

Why move the fixup _before_ the error-check? drm_atomic_ioctl() does
this to make sure it never leaves stale "old_fb" pointers, but this is
clearly not what you do here. So either move it after the error-check
and drop the then redundant "if (ret ==0)" from the loop, or move it
into the "fail:" condition (maybe rename it) and properly take care of
"old_fb" and not-freeing "state".

Thanks
David
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/fbdev: Update legacy plane->fb refcounting for atomic restore

[Daniel Vetter]

From: Matt Roper 

Starting with commit

commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
Author: Rob Clark 
Date:   Tue Aug 25 15:36:00 2015 -0400

drm/i915: enable atomic fb-helper

I've been seeing some panics on i915 when the DRM master shuts down that appear
to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
dropping our initial FB's reference count to 0 and freeing it, which causes a
crash when we try to restore it later).  Digging deeper, the state FB
refcounting is working as expected, but we seem to be missing proper
refcounting on the legacy plane->fb pointers in the new atomic fbdev code.

Tracking plane->old_fb and then doing a ref/unref at the end of the
fbdev restore like we do in the legacy ioctl's ensures we don't miscount
references on plane->fb and avoids the panics.

v2 from Daniel:

Really do what the atomic ioctl does:
- Also update plane->fb and plane->crtc.
- Clear out plane->old_fb on failures too.

v3: git add everything. Oops.

Cc: Rob Clark 
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Matt Roper  (v1)
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_fb_helper.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 64fc5ca8fda2..8af522afdfc1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -352,6 +352,8 @@ retry:
drm_for_each_plane(plane, dev) {
struct drm_plane_state *plane_state;
 
+   plane->old_fb = plane->fb;
+
plane_state = drm_atomic_get_plane_state(state, plane);
if (IS_ERR(plane_state)) {
ret = PTR_ERR(plane_state);
@@ -382,6 +384,21 @@ retry:
}
 
ret = drm_atomic_commit(state);
+
+   drm_for_each_plane(plane, dev) {
+   if (ret == 0) {
+   struct drm_framebuffer *new_fb = plane->state->fb;
+   if (new_fb)
+   drm_framebuffer_reference(new_fb);
+   plane->fb = new_fb;
+   plane->crtc = plane->state->crtc;
+
+   if (plane->old_fb)
+   drm_framebuffer_unreference(plane->old_fb);
+   }
+   plane->old_fb = NULL;
+   }
+
if (ret != 0)
goto fail;
 
-- 
2.5.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/fbdev: Update legacy plane->fb refcounting for atomic restore

[Matt Roper]

Starting with commit

commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
Author: Rob Clark 
Date:   Tue Aug 25 15:36:00 2015 -0400

drm/i915: enable atomic fb-helper

I've been seeing some panics on i915 when the DRM master shuts down that appear
to be caused by using an already-freed framebuffer (i.e., we're unexpectedly
dropping our initial FB's reference count to 0 and freeing it, which causes a
crash when we try to restore it later).  Digging deeper, the state FB
refcounting is working as expected, but we seem to be missing proper
refcounting on the legacy plane->fb pointers in the new atomic fbdev code.

Tracking plane->old_fb and then doing a ref/unref at the end of the
fbdev restore like we do in the legacy ioctl's ensures we don't miscount
references on plane->fb and avoids the panics.

Cc: Rob Clark 
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Matt Roper 
---
I don't see any existing bugzilla's (or mailing list complaints) for this,
which surprises me a bit since it seems to be very easy to reproduce for me on
latest drm-intel-nightly running on IVB...boot the system, load X, kill X,
panic.

 drivers/gpu/drm/drm_fb_helper.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 64fc5ca..2149ac7 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -352,6 +352,8 @@ retry:
drm_for_each_plane(plane, dev) {
struct drm_plane_state *plane_state;
 
+   plane->old_fb = plane->fb;
+
plane_state = drm_atomic_get_plane_state(state, plane);
if (IS_ERR(plane_state)) {
ret = PTR_ERR(plane_state);
@@ -385,6 +387,14 @@ retry:
if (ret != 0)
goto fail;
 
+   drm_for_each_plane(plane, dev) {
+   if (plane->fb)
+   drm_framebuffer_reference(plane->fb);
+   if (plane->old_fb)
+   drm_framebuffer_unreference(plane->old_fb);
+   plane->old_fb = NULL;
+   }
+
return 0;
 
 fail:
-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx