Re: [PATCH 14/19] drm/i915: Move context initialisation to first-use

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 14/19] drm/i915: Move context initialisation to first-use
Date: Thu, 21 Apr 2016 08:56:38 +0100	[thread overview]
Message-ID: <20160421075638.GI15733@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1461224850.4381.26.camel@linux.intel.com>

On Thu, Apr 21, 2016 at 10:47:30AM +0300, Joonas Lahtinen wrote:
> On to, 2016-04-21 at 08:08 +0100, Chris Wilson wrote:
> > On Thu, Apr 21, 2016 at 09:57:03AM +0300, Joonas Lahtinen wrote:
> > > 
> > > On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote:
> > > > 
> > > > +	if (!request->ctx->engine[engine->id].initialised) {
> > > > +		ret = engine->init_context(request);
> > > > +		if (ret) {
> > > > +			intel_lr_context_unpin(request->ctx, engine);
> > > I prefer the goto teardown path, it's easy to read and modify later on.
> 
> Meant something like this which is functionally the same but less
> nesting;
> 
> 	if (request->ctx->engine[engine->id].initialised)
> 		return 0;
> 
> 	ret = engine->init_context(request);
> 	if (ret)
> 		goto out_unpin;
> 
> 	request->ctx->engine[engine->id].initialised = true;
> 
> 	return 0;
> 
> out_unpin:
> 	intel_lr_context_unpin(request->ctx, engine);
> 
> 	return ret;

Yes, I am arguing that this leaves the next person thinking it will be
safe to extend the unwind. And so by not conforming to the idiom, it
should be more of a danger signal.

Compromise?

        if (!request->ctx->engine[engine->id].initialised) {
                ret = engine->init_context(request);
                if (ret)
                        goto err_unpin;

                request->ctx->engine[engine->id].initialised = true;
        }

        /* Note that after this point, we have committed to using
         * this request as it is being used to both track the
         * state of engine initialisation and liveness of the
         * golden renderstate above. Think twice before you try
         * to cancel/unwind this request now.
         */

        return 0;

err_unpin:
        intel_lr_context_unpin(request->ctx, engine);
        return ret;
}

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx