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

From: David Herrmann <dh.herrmann@gmail.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/fbdev: Update legacy plane->fb refcounting for atomic restore
Date: Tue, 22 Sep 2015 12:34:33 +0200	[thread overview]
Message-ID: <CANq1E4RdwTV9Q1dxW=mEhO-_AFmmugnAqtz5iNy1xbiej1acWw@mail.gmail.com> (raw)
In-Reply-To: <56012E69.5000903@linux.intel.com>

Hi

On Tue, Sep 22, 2015 at 12:33 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Hey,
>
> Op 22-09-15 om 11:55 schreef Daniel Vetter:
>> From: Matt Roper <matthew.d.roper@intel.com>
>>
>> Starting with commit
>>
>>         commit 28cc504e8d52248962f5b485bdc65f539e3fe21d
>>         Author: Rob Clark <robdclark@gmail.com>
>>         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@gmail.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> (v1)
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel