Re: [PATCH 8/8] drm/i915: remove early fb allocation dependency on CONFIG_FB v2

Daniel Vetter
2014-03-09T19:27:53Z

On Sun, Mar 9, 2014 at 6:33 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Sat, 8 Mar 2014 11:33:15 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Fri, Mar 07, 2014 at 08:57:55AM -0800, Jesse Barnes wrote:
>> > By stuffing the fb allocation into the crtc, we get mode set lifetime
>> > refcounting for free, but have to handle the initial pin & fence
>> > slightly differently. It also means we can move the shared fb handling
>> > into the core rather than leaving it out in the fbdev code.
>> >
>> > v2: null out crtc->fb on error (Daniel)
>> > take fbdev fb ref and remove unused error path (Daniel)
>> >
>> > Requested-by: Daniel Vetter <daniel@ffwll.ch>
>> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>
>> Ok, I've merged patches 1-4 and this one here from the series. Not
>> terribly happy that you didn't squash in this fixup, since now people
>> might stumble over the strange lifetime rules of the previous patches. But
>> meh ;-)
>
> Yeah even though there's a handoff from the core to the fbdev side, I
> still think they're correct as far as fbs go. The underlying obj ref
> may not be correct though; I think that was papering over an earlier
> bug.
>
> Either way those should manifest as a leaked object (the stolen fb will
> stick around forever) rather than a crash or something. Whereas this
> last patch is more likely to cause serious trouble I think since it's a
> bit more invasive and relies on some other bits more...

Yeah I agree that the refcounting before this patch was just a bit
leaky - otherwise I would have protested much louder ;-)

Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch