Re: [PATCH 16/18] drm/i915: Enable multiple timelines

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2016-10-20 at 13:49 +0100, Chris Wilson wrote:
> On Mon, Sep 19, 2016 at 06:52:13PM +0300, Joonas Lahtinen wrote:
> > 
> > On ke, 2016-09-14 at 07:52 +0100, Chris Wilson wrote:
> > > 
> > > @@ -315,17 +304,42 @@ submit_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
> > >  {
> > >  	struct drm_i915_gem_request *request =
> > >  		container_of(fence, typeof(*request), submit);
> > > +	struct intel_timeline *timeline;
> > > +	struct intel_engine_cs *engine = request->engine;
> > > +	unsigned long flags;
> > > +	u32 seqno;
> > >  
> > >  	/* Will be called from irq-context when using foreign DMA fences */
> > >  
> > > -	switch (state) {
> > > -	case FENCE_COMPLETE:
> > > -		request->engine->submit_request(request);
> > > -		break;
> > > +	if (state != FENCE_COMPLETE)
> > > +		return NOTIFY_DONE;
> > >  
> > > -	case FENCE_FREE:
> > > -		break;
> > > -	}
> > > +	timeline = engine->timeline;
> > > +	GEM_BUG_ON(timeline == request->timeline);
> > 
> > Umm, why this BUG_ON?
> 
> To document that the intent here is to move from the per-context
> timeline onto the global per-engine timeline. If the request was already
> on the engine->timeline bad things would happen, at the very simplest a
> deadlock here.

I see. I would have lifted the assignment and the BUG_ON before actual
code, like elsewhere. But I can live with this too.

The new patch seems to have got rid of the emit_request weirdness too,
so I'll add by R-b there.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx