From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934317AbcATPDN (ORCPT ); Wed, 20 Jan 2016 10:03:13 -0500 Received: from mga01.intel.com ([192.55.52.88]:31712 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933718AbcATPDL (ORCPT ); Wed, 20 Jan 2016 10:03:11 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,321,1449561600"; d="scan'208";a="897376012" Subject: Re: [RFC 00/29] De-stage android's sync framework To: Gustavo Padovan , Greg Kroah-Hartman , devel@driverdev.osuosl.org, daniels@collabora.com, Daniel Vetter , Riley Andrews , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= , Gustavo Padovan , John Harrison References: <1452869739-3304-1-git-send-email-gustavo@padovan.org> <569F614A.1070807@linux.intel.com> <20160120143250.GF8217@joana> From: Maarten Lankhorst Message-ID: <569FA187.2060601@linux.intel.com> Date: Wed, 20 Jan 2016 16:02:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20160120143250.GF8217@joana> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Op 20-01-16 om 15:32 schreef Gustavo Padovan: > 2016-01-20 Maarten Lankhorst : > >> Hey, >> >> Op 15-01-16 om 15:55 schreef Gustavo Padovan: >>> From: Gustavo Padovan >>> >>> This patch series de-stage the sync framework, and in order to accomplish that >>> a bunch of cleanups/improvements on the sync and fence were made. >>> >>> The sync framework contained some abstractions around struct fence and those >>> were removed in the de-staging process among other changes: >>> >>> Userspace visible changes >>> ------------------------- >>> >>> * The sw_sync file was moved from /dev/sw_sync to /sync/sw_sync. No >>> other change. >>> >>> Kernel API changes >>> ------------------ >>> >>> * struct sync_timeline is now struct fence_timeline >>> * sync_timeline_ops is now fence_timeline_ops and they now carry struct >>> fence as parameter instead of struct sync_pt >>> * a .cleanup() fence op was added to allow sync_fence to run a cleanup when >>> the fence_timeline is destroyed >>> * added fence_add_used_data() to pass a private point to struct fence. This >>> pointer is sent back on the .cleanup op. >>> * The sync timeline function were moved to be fence_timeline functions: >>> - sync_timeline_create() -> fence_timeline_create() >>> - sync_timeline_get() -> fence_timeline_get() >>> - sync_timeline_put() -> fence_timeline_put() >>> - sync_timeline_destroy() -> fence_timeline_destroy() >>> - sync_timeline_signal() -> fence_timeline_signal() >>> >>> * sync_pt_create() was replaced be fence_create_on_timeline() >>> >>> Internal changes >>> ---------------- >>> >>> * fence_timeline_ops was removed in favor of direct use fence_ops >>> * fence default functions were created for fence_ops >>> * removed structs sync_pt, sw_sync_timeline and sw_sync_pt >>> >>> Gustavo Padovan (29): >>> staging/android: fix sync framework documentation >>> staging/android: fix checkpatch warning >>> staging/android: rename sync_fence_release >>> staging/android: rename 'android_fence' to 'sync_fence' >>> staging/android: remove not used sync_timeline ops >>> staging/android: create a 'sync' dir for debugfs information >>> staging/android: move sw_sync file to debugfs file >>> staging/android: Remove WARN_ON_ONCE when releasing sync_fence >>> staging/android: rename struct sync_fence's variables to 'sync_fence' >>> staging/android: rename 'sync_pt' to 'fence' in struct sync_fence_cb >>> dma-buf/fence: move sync_timeline to fence_timeline >>> staging/android: remove struct sync_pt >>> dma-buf/fence: create fence_default_enable_signaling() >>> dma-buf/fence: create fence_default_release() >>> dma-buf/fence: create fence_default_get_driver_name() >>> dma-buf/fence: create fence_default_timeline_name() >> This is misleading. I think timeline_fence prefix would be more appropriate here. > Why? These fence_default_.. functions are fence_ops and not related to > fence_timeline in any way. Because they're using fence_parent, which should probably be renamed to fence_to_timeline() The name makes it sound as if they could apply to all type of fences, I don't think this is the case. >> I also believe this should be done in multiple series. First series should de-stage the userspace fence framework. The next series should fix up android_fence and maybe rename it to timeline_fence since sync_fence is already used for the userspace fd, which would add more confusion? > Sure. I've been thinking on how to split this properly. I'm trying to > add a bunch of clean up/renaming first, eg the sync_fence rename to > sync_file that Daniel Vetter and I discussed. > > Next my plan would be move sync_timeline to fence_timeline, add the > fence_default.. fence_ops, clean up sw_sync and finally merge > fence_context and fence_timeline. > > Looking at how sync and fence It looks easier to me to de-stage sync_timeline first than userspace > fence. There's already code to add a sync_fence_create_dma export [1][2]. So if you want to de-stage it then there will be users for it. sync_pt otoh has no upstream in-kernel users. It was a wrapper to keep android drivers api compatible with the fence api. [1] https://patchwork.freedesktop.org/patch/67845/ [2] https://patchwork.freedesktop.org/patch/67846/