Hey,
Op 22-09-15 om 11:55 schreef Daniel Vetter:
> From: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> Starting with commit
>
> commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
> Author: Rob Clark <robdclark@xxxxxxxxx>
> 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 <robdclark@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
