From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gustavo Padovan Subject: Re: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file Date: Tue, 28 Jun 2016 12:27:01 -0300 Message-ID: <20160628152701.GM2508@joana> References: <1467055762-25881-1-git-send-email-gustavo@padovan.org> <1467055762-25881-4-git-send-email-gustavo@padovan.org> <20160628080221.GB25424@nuc-i3427.alporthouse.com> <20160628142500.GK2508@joana> <20160628150952.GE28577@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <20160628150952.GE28577@nuc-i3427.alporthouse.com> Sender: linux-kernel-owner@vger.kernel.org To: Chris Wilson , Gustavo Padovan , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Stone , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , laurent.pinchart@ideasonboard.com, seanpaul@google.com, marcheu@google.com, m.chehab@samsung.com, Sumit Semwal , Maarten Lankhorst , Christian =?iso-8859-1?Q?K=F6nig?= List-Id: dri-devel@lists.freedesktop.org 2016-06-28 Chris Wilson : > On Tue, Jun 28, 2016 at 11:25:00AM -0300, Gustavo Padovan wrote: > > 2016-06-28 Chris Wilson : > >=20 > > > On Mon, Jun 27, 2016 at 04:29:22PM -0300, Gustavo Padovan wrote: > > > > From: Gustavo Padovan > > > >=20 > > > > Create sync_file->fence to abstract the type of fence we are us= ing for > > > > each sync_file. If only one fence is present we use a normal st= ruct fence > > > > but if there is more fences to be added to the sync_file a fenc= e_array > > > > is created. > > > >=20 > > > > This change cleans up sync_file a bit. We don't need to have sy= nc_file_cb > > > > array anymore. Instead, as we always have one fence, only one = fence > > > > callback is registered per sync_file. > > > >=20 > > > > Cc: Chris Wilson > > > > Cc: Christian K=F6nig > > > > Signed-off-by: Gustavo Padovan > > > > --- > > > > @@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct f= ence *fence) > > > > { > > > > struct sync_file *sync_file; > > > > =20 > > > > - sync_file =3D sync_file_alloc(offsetof(struct sync_file, cbs[= 1])); > > > > + sync_file =3D sync_file_alloc(); > > > > if (!sync_file) > > > > return NULL; > > > > =20 > > > > - sync_file->num_fences =3D 1; > > > > + sync_file->fence =3D fence; > > > > + > > > > atomic_set(&sync_file->status, 1); > > >=20 > > > sync_file->status =3D> fence_is_signaled(sync_file->fence); > > >=20 > > > Both should just be an atomic read, except fence_is_signaled() wi= ll then > > > do a secondary poll. > >=20 > > Not sure I follow. I set it to 1 here, but below when we call > > fence_add_callback() and the fence is already signalled atomic_dec = sets > > sync_file->status to 0. >=20 > I'm just saying that usage sync_file->status is equivalent to > fence_is_signaled(), i.e. we reduce the amount of bookkeeping local t= o > sync_file. Indeed, that makes sense, I'll remove status from sync_file. >=20 > > > > snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu= -%d", > > > > fence->ops->get_driver_name(fence), > > > > fence->ops->get_timeline_name(fence), fence->context, > > > > fence->seqno); > > > > =20 > > > > - sync_file->cbs[0].fence =3D fence; > > > > - sync_file->cbs[0].sync_file =3D sync_file; > > > > - if (fence_add_callback(fence, &sync_file->cbs[0].cb, > > > > - fence_check_cb_func)) > > > > + if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_= func)) > > > > atomic_dec(&sync_file->status); > > > > =20 > > > > return sync_file; > > > > @@ -121,14 +119,42 @@ err: > > > > return NULL; > > > > } > > > > =20 > > > > -static void sync_file_add_pt(struct sync_file *sync_file, int = *i, > > > > - struct fence *fence) > > > > +static int sync_file_set_fence(struct sync_file *sync_file, > > > > + struct fence **fences, int num_fences) > > > > { > > > > - sync_file->cbs[*i].fence =3D fence; > > > > - sync_file->cbs[*i].sync_file =3D sync_file; > > > > + struct fence_array *array; > > > > + > > > > + if (num_fences =3D=3D 1) { > > > > + sync_file->fence =3D fences[0]; > > >=20 > > > This steals the references. > > >=20 > > > > + } else { > > > > + array =3D fence_array_create(num_fences, fences, > > > > + fence_context_alloc(1), 1, false); > > >=20 > > > This creates a reference. > > >=20 > > > When we call fence_put(sync_fence->fence) we release a reference = we > > > never owned if num_fences =3D=3D 1. > >=20 > > No, sync_file_merge() gets a new reference for each fence it is goi= ng to > > add to the new fence. So for num_fences =3D=3D 1 when sync_file->fe= nce is > > set we already hold a reference to it, so no matter if it is a fenc= e or > > a array we own a reference. >=20 > Ugh. Root cause appears to be that fence_array_create() does not beha= ve > how I would expect, in that it borrows references to the fences and > not own a reference to the fences in its array. I beg for a comment a= s > this function is very counter-intuitive for me. There is this in fence_array_create(): * The caller should allocte the fences array with num_fences size = =20 * and fill it with the fences it wants to add to the object. Ownership= of this=20 * array is take and fence_put() is used on each fence on release. =20 Gustavo