On Thu, 7 Dec 2017 13:31:34 +0100 Stefan Wahren <stefan.wahren@xxxxxxxx> wrote: > Hi Daniel, > > > Am 24.11.2017 um 15:52 schrieb Daniel Vetter: > > On Thu, Nov 23, 2017 at 09:24:11AM +0100, Boris Brezillon wrote: > >> On Wed, 22 Nov 2017 16:13:31 -0800 > >> Eric Anholt <eric@xxxxxxxxxx> wrote: > >> > >>> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> writes: > >>> > >>>> On Wed, 22 Nov 2017 13:16:08 -0800 > >>>> Eric Anholt <eric@xxxxxxxxxx> wrote: > >>>> > >>>>> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> writes: > >>>>> > >>>>>> With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's > >>>>>> passed a refcount object that has its counter set to 0. In this driver, > >>>>>> this is a valid use case since we want to increment ->usecnt only when > >>>>>> the BO object starts to be used by real HW components and this is > >>>>>> definitely not the case when the BO is created. > >>>>>> > >>>>>> Fix the problem by using refcount_inc_not_zero() instead of > >>>>>> refcount_inc() and fallback to refcount_set(1) when > >>>>>> refcount_inc_not_zero() returns false. Note that this 2-steps operation > >>>>>> is not racy here because the whole section is protected by a mutex > >>>>>> which guarantees that the counter does not change between the > >>>>>> refcount_inc_not_zero() and refcount_set() calls. > >>>>> If we're not following the model, and protecting the refcount by a > >>>>> mutex, shouldn't we just be using addition and subtraction instead of > >>>>> refcount's atomics? > >>>> Actually, this mutex is protecting the bo->madv value which has to be > >>>> checked when the counter reaches 0 (when decrementing) or 1 (when > >>>> incrementing). We just benefit from this protection here, but ideally > >>>> it would be better to have an refcount_inc_allow_zero() as suggested by > >>>> Daniel. > >>> Let me restate this to see if it makes sense: The refcount is always >= > >>> 0, this is is the only path that increases the refcount from 0 to 1, and > >>> it's (incidentally) protected by a mutex, so there's no race between the > >>> attempted increase from nonzero and the set from nonzero to 1. > >> Yep. > >> > >>> This seems fine to me as a bugfix. > >> The discussion is going on in the other thread, let's wait a bit > >> before taking a decision. > > Let's not block the bugfix on reaching perfection imo. I'd merge this as > > the minimal fix, and then apply the pretty paint once we have a clear idea > > for that. > > -Daniel > > sorry but i can't find it in the DRI tree. Sorry for the delay. I applied it this morning [1] and it should appear in the next -rc. Regards, Boris [1]https://cgit.freedesktop.org/drm/drm-misc/log/?h=drm-misc-fixes _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
