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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
