From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932280AbcEWOAr (ORCPT ); Mon, 23 May 2016 10:00:47 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:34517 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932155AbcEWOAq (ORCPT ); Mon, 23 May 2016 10:00:46 -0400 Date: Mon, 23 May 2016 16:00:45 +0200 From: Daniel Vetter To: Christian =?iso-8859-1?Q?K=F6nig?= Cc: Gustavo Padovan , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] dma-buf/fence: add fence_array fences v4 Message-ID: <20160523140045.GC27098@phenom.ffwll.local> Mail-Followup-To: Christian =?iso-8859-1?Q?K=F6nig?= , Gustavo Padovan , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <1463752571-28688-1-git-send-email-deathsimple@vodafone.de> <1463752571-28688-2-git-send-email-deathsimple@vodafone.de> <20160520144728.GE15150@joana> <20160523074149.GW27098@phenom.ffwll.local> <5742E987.3030609@vodafone.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5742E987.3030609@vodafone.de> X-Operating-System: Linux phenom 4.6.0-rc5+ User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 23, 2016 at 01:29:11PM +0200, Christian König wrote: > Am 23.05.2016 um 09:41 schrieb Daniel Vetter: > >On Fri, May 20, 2016 at 11:47:28AM -0300, Gustavo Padovan wrote: > >>2016-05-20 Christian König : > >> > >>>From: Gustavo Padovan > >>> > >>>struct fence_collection inherits from struct fence and carries a > >>>collection of fences that needs to be waited together. > >>> > >>>It is useful to translate a sync_file to a fence to remove the complexity > >>>of dealing with sync_files on DRM drivers. So even if there are many > >>>fences in the sync_file that needs to waited for a commit to happen, > >>>they all get added to the fence_collection and passed for DRM use as > >>>a standard struct fence. > >>> > >>>That means that no changes needed to any driver besides supporting fences. > >>> > >>>fence_collection's fence doesn't belong to any timeline context, so > >>>fence_is_later() and fence_later() are not meant to be called with > >>>fence_collections fences. > >>> > >>>v2: Comments by Daniel Vetter: > >>> - merge fence_collection_init() and fence_collection_add() > >>> - only add callbacks at ->enable_signalling() > >>> - remove fence_collection_put() > >>> - check for type on to_fence_collection() > >>> - adjust fence_is_later() and fence_later() to WARN_ON() if they > >>> are used with collection fences. > >>> > >>>v3: - Initialize fence_cb.node at fence init. > >>> > >>> Comments by Chris Wilson: > >>> - return "unbound" on fence_collection_get_timeline_name() > >>> - don't stop adding callbacks if one fails > >>> - remove redundant !! on fence_collection_enable_signaling() > >>> - remove redundant () on fence_collection_signaled > >>> - use fence_default_wait() instead > >>> > >>>v4 (chk): Rework, simplification and cleanup: > >>> - Drop FENCE_NO_CONTEXT handling, always allocate a context. > >>> - Rename to fence_array. > >>> - Return fixed driver name. > >>> - Register only one callback at a time. > >>> - Document that create function takes ownership of array. > >>This looks good to me. Dropping NO_CONTEXT was a good idea, also > >>registering only one callback makes it looks better. > >This will make it even harder to eventually add a real fence_context > >structure for tracking and debugging. I know you don't care for amdgpu > >since you have amdgpu-specific debug files, and there's some lifetime fun > >that makes it not immediately obvious how to resolve it. > > Completely independent of my work on amdgpu I still think that it's not such > a good idea to use a complex structure for the fence context. > > Especially on SoCs and small embedded systems you probably don't want to > overhead associated with that only for debugging purposes in a production > environment. At least in all the drivers I've seen you have to allocate a little bit of stuff _anyway_ to store that context id, plus a bunch of hw state. So the allocation itsel shouldn't be a problem at all, since that can be handled by embedding. If it's the atomic inc/dec for refcounting you're worried about, then that could be made dependent on CONFIG_FENCE_DEBUGGING. And android didn't have that Kconfig knob even, and people seemingly didn't care about the overhead on arm socs. > >But on "lots of > >shitty little drivers" systems aka SoCs generic debugging information is > >crucial I think. Not liking too much where this is going. > > Yeah I agree that generic debugging information is usually crucial, but the > lifetime issues indeed can't be solved without reference counting and a hole > bunch of overhead. > > How about V5 of the patch I've just send out? Apart from fixing a few issues > I've made the context and sequence number parameters of the fence_array > object. > > This way you don't need to always allocate a new context for each object, > but just enough to keep your timelines straight. > > E.g. you don't get a lot of contexts only used once. This is at least > sufficient for my amdgpu use case. Well my idea behind NO_CONTEXT was that this way there'd be only one special context for merged fences, which could have a linked list of all fences ever (with debugging stuff). That way there's no need to allocate one context per fence_array. It has the downside of a slightly leaky abstraction, as in you must use the provided interface functions to figure out whether a fence is on the same timeline, and if so, which one is later. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch