From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [Nouveau] [PATCH 09/17] drm/radeon: use common fence implementation for fences Date: Tue, 22 Jul 2014 17:18:39 +0200 Message-ID: <53CE80CF.2060709@canonical.com> References: <20140709093124.11354.3774.stgit@patser> <20140709122953.11354.46381.stgit@patser> <53CE2421.5040906@amd.com> <20140722114607.GL15237@phenom.ffwll.local> <20140722115737.GN15237@phenom.ffwll.local> <53CE56ED.4040109@vodafone.de> <20140722132652.GO15237@phenom.ffwll.local> <53CE6AFA.1060807@vodafone.de> <53CE78C0.9030408@canonical.com> <53CE7D06.2010109@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <53CE7D06.2010109@amd.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: =?ISO-8859-1?Q?Christian_K=F6nig?= , =?ISO-8859-1?Q?Christian_K=F6nig?= , Dave Airlie , Thomas Hellstrom , nouveau , LKML , dri-devel , Ben Skeggs , "Deucher, Alexander" List-Id: nouveau.vger.kernel.org Hey, op 22-07-14 17:02, Christian K=F6nig schreef: > Am 22.07.2014 16:44, schrieb Maarten Lankhorst: >> op 22-07-14 15:45, Christian K=F6nig schreef: >>> Am 22.07.2014 15:26, schrieb Daniel Vetter: >>>> On Tue, Jul 22, 2014 at 02:19:57PM +0200, Christian K=F6nig wrote: >>>>> Am 22.07.2014 13:57, schrieb Daniel Vetter: >>>>>> On Tue, Jul 22, 2014 at 01:46:07PM +0200, Daniel Vetter wrote: >>>>>>> On Tue, Jul 22, 2014 at 10:43:13AM +0200, Christian K=F6nig wrote: >>>>>>>> Am 22.07.2014 06:05, schrieb Dave Airlie: >>>>>>>>> On 9 July 2014 22:29, Maarten Lankhorst wrote: >>>>>>>>>> Signed-off-by: Maarten Lankhorst >>>>>>>>>> --- >>>>>>>>>> drivers/gpu/drm/radeon/radeon.h | 15 +- >>>>>>>>>> drivers/gpu/drm/radeon/radeon_device.c | 60 ++++++++- >>>>>>>>>> drivers/gpu/drm/radeon/radeon_fence.c | 223 +++++++++++++++= +++++++++++------ >>>>>>>>>> 3 files changed, 248 insertions(+), 50 deletions(-) >>>>>>>>>> >>>>>>>>> From what I can see this is still suffering from the problem th= at we >>>>>>>>> need to find a proper solution to, >>>>>>>>> >>>>>>>>> My summary of the issues after talking to Jerome and Ben and >>>>>>>>> re-reading things is: >>>>>>>>> >>>>>>>>> We really need to work out a better interface into the drivers to= be >>>>>>>>> able to avoid random atomic entrypoints, >>>>>>>> Which is exactly what I criticized from the very first beginning. = Good to >>>>>>>> know that I'm not the only one thinking that this isn't such a goo= d idea. >>>>>>> I guess I've lost context a bit, but which atomic entry point are we >>>>>>> talking about? Afaics the only one that's mandatory is the is >>>>>>> fence->signaled callback to check whether a fence really has been >>>>>>> signalled. It's used internally by the fence code to avoid spurious >>>>>>> wakeups. Afaik that should be doable already on any hardware. If th= at's >>>>>>> not the case then we can always track the signalled state in softwa= re and >>>>>>> double-check in a worker thread before updating the sw state. And w= rap >>>>>>> this all up into a special fence class if there's more than one dri= ver >>>>>>> needing this. >>>>>> One thing I've forgotten: The i915 scheduler that's floating around = runs >>>>>> its bottom half from irq context. So I really want to be able to che= ck >>>>>> fence state from irq context and I also want to make it possible >>>>>> (possible! not mandatory) to register callbacks which are run from a= ny >>>>>> context asap after the fence is signalled. >>>>> NAK, that's just the bad design I've talked about. Checking fence sta= te >>>>> inside the same driver from interrupt context is OK, because it's the >>>>> drivers interrupt that we are talking about here. >>>>> >>>>> Checking fence status from another drivers interrupt context is what = really >>>>> concerns me here, cause your driver doesn't have the slightest idea i= f the >>>>> called driver is really capable of checking the fence right now. >>>> I guess my mail hasn't been clear then. If you don't like it we could = add >>>> a bit of glue to insulate the madness and bad design i915 might do from >>>> radeon. That imo doesn't invalidate the overall fence interfaces. >>>> >>>> So what about the following: >>>> - fence->enabling_signaling is restricted to be called from process >>>> context. We don't use any different yet, so would boild down to ad= ding a >>>> WARN_ON(in_interrupt) or so to fence_enable_sw_signalling. >>>> >>>> - Make fence->signaled optional (already the case) and don't implement= it >>>> in readon (i.e. reduce this patch here). Only downside is that rad= eon >>>> needs to correctly (i.e. without races or so) call fence_signal. A= nd the >>>> cross-driver synchronization might be a bit less efficient. Note t= hat >>>> you can call fence_signal from wherever you want to, so hopefully = that >>>> doesn't restrict your implementation. >>>> >>>> End result: No one calls into radeon from interrupt context, and this = is >>>> guaranteed. >>>> >>>> Would that be something you can agree to? >>> No, the whole enable_signaling stuff should go away. No callback from t= he driver into the fence code, only the other way around. >>> >>> fence->signaled as well as fence->wait should become mandatory and only= called from process context without holding any locks, neither atomic nor = any mutex/semaphore (rcu might be ok). >> fence->wait is mandatory, and already requires sleeping. >> >> If .signaled is not implemented there is no guarantee the fence will be >> signaled sometime soon, this is also why enable_signaling exists, to >> allow the driver to flush. I get it that it doesn't apply to radeon and = nouveau, >> but for other drivers that could be necessary, like vmwgfx. >> >> Ironically that is also a part of the ttm fence, except it was called fl= ush there. > > Then call it flush again and make it optional like in TTM. You've posted a lot of concerns, but I haven't seen you come up with any sc= enario that could create a lockup and lockdep wouldn't warn about. >> I would also like to note that ttm_bo_wait currently is also a function = that currently uses is_signaled from atomic_context... > > I know, but TTM is only called from inside a single driver, no inter driv= er needs here. We currently even call the internal fence implementation fro= m interrupt context as well and at more than one occasion assume that TTM o= nly uses radeon fences. This is no longer true when you start synchronizing with other drivers. The= TTM core will see the intel fences and treat them no differently. That's t= he entire reason for this conversion. It's also needed to remove the need t= o pin dma-buf when exporting. ~Maarten.