| Message ID | 1458321994-12212-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/03/16 17:26, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > This anonymous struct was causing a good amount of overly > verbose code. Also, if we name it and cache the pointer locally > when there are multiple accesses to it, not only the code is > more readable, but the compiler manages to generate smaller > binary. > > Along the way I also shortened access to dev_priv and eliminated > some unused variables and cached some where I spotted the > opportunity. > > Name for the structure, intel_context_engine, and the local > variable name were borrowed from a similar patch by Chris Wilson. > > v2: Hate the engine->dev surprises, really do. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_lrc.c | 94 +++++++++++++++++++++------------------- > 2 files changed, 50 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 00c41a4bde2a..480639c39543 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -840,7 +840,7 @@ struct intel_context { > } legacy_hw_ctx; > > /* Execlists */ > - struct { > + struct intel_context_engine { Good idea, I had a version of this too, derived from Chris' patch [157/190] drm/i915: Tidy execlists by using intel_context_engine locals. The only thing to disagree with is the actual name; it should be "intel_engine_context" (or some abbreviation thereof), because in English (and German) the noun at the *end* of a compound noun-phrase is what it actually *is*, with all the others qualifying it. So a "railway bridge" is a type of bridge, not a type of railway, and "eine Straßenbahnhaltestelle" (street-train-stopping-place => tram stop) is not a street. [aside] My favourite in English is "Space Civilisation Power Struggle Game" (five nouns in a row!) describing a certain boxed game -- anyone recognise that? [/aside] I'll post a version following that naming convention shortly ... .Dave.
On Mon, Mar 21, 2016 at 03:23:57PM +0000, Dave Gordon wrote: > On 18/03/16 17:26, Tvrtko Ursulin wrote: > >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > >This anonymous struct was causing a good amount of overly > >verbose code. Also, if we name it and cache the pointer locally > >when there are multiple accesses to it, not only the code is > >more readable, but the compiler manages to generate smaller > >binary. > > > >Along the way I also shortened access to dev_priv and eliminated > >some unused variables and cached some where I spotted the > >opportunity. > > > >Name for the structure, intel_context_engine, and the local > >variable name were borrowed from a similar patch by Chris Wilson. > > > >v2: Hate the engine->dev surprises, really do. > > > >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Cc: Chris Wilson <chris@chris-wilson.co.uk> > >--- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_lrc.c | 94 +++++++++++++++++++++------------------- > > 2 files changed, 50 insertions(+), 46 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >index 00c41a4bde2a..480639c39543 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.h > >+++ b/drivers/gpu/drm/i915/i915_drv.h > >@@ -840,7 +840,7 @@ struct intel_context { > > } legacy_hw_ctx; > > > > /* Execlists */ > >- struct { > >+ struct intel_context_engine { > > Good idea, I had a version of this too, derived from Chris' patch > [157/190] drm/i915: Tidy execlists by using intel_context_engine locals. > > The only thing to disagree with is the actual name; it should be > "intel_engine_context" (or some abbreviation thereof), because in We have been using object_subobject. -Chris
On 22/03/16 09:28, Chris Wilson wrote: > On Mon, Mar 21, 2016 at 03:23:57PM +0000, Dave Gordon wrote: >> On 18/03/16 17:26, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> This anonymous struct was causing a good amount of overly >>> verbose code. Also, if we name it and cache the pointer locally >>> when there are multiple accesses to it, not only the code is >>> more readable, but the compiler manages to generate smaller >>> binary. >>> >>> Along the way I also shortened access to dev_priv and eliminated >>> some unused variables and cached some where I spotted the >>> opportunity. >>> >>> Name for the structure, intel_context_engine, and the local >>> variable name were borrowed from a similar patch by Chris Wilson. >>> >>> v2: Hate the engine->dev surprises, really do. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 2 +- >>> drivers/gpu/drm/i915/intel_lrc.c | 94 +++++++++++++++++++++------------------- >>> 2 files changed, 50 insertions(+), 46 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>> index 00c41a4bde2a..480639c39543 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -840,7 +840,7 @@ struct intel_context { >>> } legacy_hw_ctx; >>> >>> /* Execlists */ >>> - struct { >>> + struct intel_context_engine { >> >> Good idea, I had a version of this too, derived from Chris' patch >> [157/190] drm/i915: Tidy execlists by using intel_context_engine locals. >> >> The only thing to disagree with is the actual name; it should be >> "intel_engine_context" (or some abbreviation thereof), because in > > We have been using object_subobject. > -Chris No we haven't. Examples of the above convention for structs include: * "struct intel_device_info" - information about an intel device * "struct i915_ctx_hang_stats" - statistics about hangs, per context * "struct intel_uncore_funcs" - functions exported from uncore * "struct i915_power_well_ops" - operations on power wells * "struct i915_suspend_saved_registers" - registers saved at suspend and for instance names: * "struct list_head request_list" - a list of requests * "struct i915_vma *lrc_vma" - the VMA for an LRC * "uint32_t *lrc_reg_state" - the state of the registers in an LRC Indeed it's quite difficult to find any compound names that do *not* follow this convention. Maybe your version would be more natural in a language where adjectives (or possessives) normally follow the noun they describe? .Dave.
On 22/03/16 11:20, Dave Gordon wrote: > On 22/03/16 09:28, Chris Wilson wrote: >> On Mon, Mar 21, 2016 at 03:23:57PM +0000, Dave Gordon wrote: >>> On 18/03/16 17:26, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> This anonymous struct was causing a good amount of overly >>>> verbose code. Also, if we name it and cache the pointer locally >>>> when there are multiple accesses to it, not only the code is >>>> more readable, but the compiler manages to generate smaller >>>> binary. >>>> >>>> Along the way I also shortened access to dev_priv and eliminated >>>> some unused variables and cached some where I spotted the >>>> opportunity. >>>> >>>> Name for the structure, intel_context_engine, and the local >>>> variable name were borrowed from a similar patch by Chris Wilson. >>>> >>>> v2: Hate the engine->dev surprises, really do. >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 2 +- >>>> drivers/gpu/drm/i915/intel_lrc.c | 94 >>>> +++++++++++++++++++++------------------- >>>> 2 files changed, 50 insertions(+), 46 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>>> b/drivers/gpu/drm/i915/i915_drv.h >>>> index 00c41a4bde2a..480639c39543 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -840,7 +840,7 @@ struct intel_context { >>>> } legacy_hw_ctx; >>>> >>>> /* Execlists */ >>>> - struct { >>>> + struct intel_context_engine { >>> >>> Good idea, I had a version of this too, derived from Chris' patch >>> [157/190] drm/i915: Tidy execlists by using intel_context_engine locals. >>> >>> The only thing to disagree with is the actual name; it should be >>> "intel_engine_context" (or some abbreviation thereof), because in >> >> We have been using object_subobject. >> -Chris > > No we haven't. Examples of the above convention for structs include: > > * "struct intel_device_info" - information about an intel device > * "struct i915_ctx_hang_stats" - statistics about hangs, per context > * "struct intel_uncore_funcs" - functions exported from uncore > * "struct i915_power_well_ops" - operations on power wells > * "struct i915_suspend_saved_registers" - registers saved at suspend > > and for instance names: > > * "struct list_head request_list" - a list of requests > * "struct i915_vma *lrc_vma" - the VMA for an LRC > * "uint32_t *lrc_reg_state" - the state of the registers in an LRC > > Indeed it's quite difficult to find any compound names that do *not* > follow this convention. Maybe your version would be more natural in a > language where adjectives (or possessives) normally follow the noun they > describe? One example is maybe: struct drm_i915_error_state { struct drm_i915_error_ring So per-ring state of the total error state, which sounds equivalent to per-engine state of a context. Or: struct intel_fbc { struct intel_fbc_state_cache { More verbose version like "struct intel_context_engine_state"? "struct intel_engine_context" reads like a context of an engine to me, which sounds opposite to what it should be - state of an engine for a context. ? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 00c41a4bde2a..480639c39543 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -840,7 +840,7 @@ struct intel_context { } legacy_hw_ctx; /* Execlists */ - struct { + struct intel_context_engine { struct drm_i915_gem_object *state; struct intel_ringbuffer *ringbuf; int pin_count; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 3a23b9549f7b..e733795b57e0 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -316,16 +316,16 @@ static void intel_lr_context_descriptor_update(struct intel_context *ctx, struct intel_engine_cs *engine) { + struct intel_context_engine *ce = &ctx->engine[engine->id]; uint64_t lrca, desc; - lrca = ctx->engine[engine->id].lrc_vma->node.start + - LRC_PPHWSP_PN * PAGE_SIZE; + lrca = ce->lrc_vma->node.start + LRC_PPHWSP_PN * PAGE_SIZE; - desc = engine->ctx_desc_template; /* bits 0-11 */ + desc = engine->ctx_desc_template; /* bits 0-11 */ desc |= lrca; /* bits 12-31 */ desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */ - ctx->engine[engine->id].lrc_desc = desc; + ce->lrc_desc = desc; } uint64_t intel_lr_context_descriptor(struct intel_context *ctx, @@ -361,8 +361,7 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0, { struct intel_engine_cs *engine = rq0->engine; - struct drm_device *dev = engine->dev; - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_private *dev_priv = rq0->i915; uint64_t desc[2]; if (rq1) { @@ -670,7 +669,8 @@ static int logical_ring_invalidate_all_caches(struct drm_i915_gem_request *req) static int execlists_move_to_gpu(struct drm_i915_gem_request *req, struct list_head *vmas) { - const unsigned other_rings = ~intel_engine_flag(req->engine); + struct intel_engine_cs *engine = req->engine; + const unsigned other_rings = ~intel_engine_flag(engine); struct i915_vma *vma; uint32_t flush_domains = 0; bool flush_chipset = false; @@ -680,7 +680,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req, struct drm_i915_gem_object *obj = vma->obj; if (obj->active & other_rings) { - ret = i915_gem_object_sync(obj, req->engine, &req); + ret = i915_gem_object_sync(obj, engine, &req); if (ret) return ret; } @@ -782,6 +782,7 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) struct intel_ringbuffer *ringbuf = request->ringbuf; struct drm_i915_private *dev_priv = request->i915; struct intel_engine_cs *engine = request->engine; + struct intel_context *ctx = request->ctx; intel_logical_ring_advance(ringbuf); request->tail = ringbuf->tail; @@ -799,12 +800,12 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) if (intel_engine_stopped(engine)) return 0; - if (engine->last_context != request->ctx) { + if (engine->last_context != ctx) { if (engine->last_context) intel_lr_context_unpin(engine->last_context, engine); - if (request->ctx != request->i915->kernel_context) { - intel_lr_context_pin(request->ctx, engine); - engine->last_context = request->ctx; + if (ctx != dev_priv->kernel_context) { + intel_lr_context_pin(ctx, engine); + engine->last_context = ctx; } else { engine->last_context = NULL; } @@ -949,9 +950,8 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, struct drm_i915_gem_execbuffer2 *args, struct list_head *vmas) { - struct drm_device *dev = params->dev; struct intel_engine_cs *engine = params->engine; - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_private *dev_priv = to_i915(params->dev); struct intel_ringbuffer *ringbuf = params->ctx->engine[engine->id].ringbuf; u64 exec_start; int instp_mode; @@ -1092,17 +1092,18 @@ static int intel_lr_context_do_pin(struct intel_context *ctx, struct intel_engine_cs *engine) { struct drm_device *dev = engine->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state; - struct intel_ringbuffer *ringbuf = ctx->engine[engine->id].ringbuf; + struct drm_i915_private *dev_priv = ctx->i915; + struct intel_context_engine *ce = &ctx->engine[engine->id]; + struct drm_i915_gem_object *ctx_obj = ce->state; + struct intel_ringbuffer *ringbuf = ce->ringbuf; struct page *lrc_state_page; uint32_t *lrc_reg_state; int ret; - WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex)); + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN, - PIN_OFFSET_BIAS | GUC_WOPCM_TOP); + PIN_OFFSET_BIAS | GUC_WOPCM_TOP); if (ret) return ret; @@ -1112,15 +1113,15 @@ static int intel_lr_context_do_pin(struct intel_context *ctx, goto unpin_ctx_obj; } - ret = intel_pin_and_map_ringbuffer_obj(engine->dev, ringbuf); + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); if (ret) goto unpin_ctx_obj; - ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj); + ce->lrc_vma = i915_gem_obj_to_ggtt(ctx_obj); intel_lr_context_descriptor_update(ctx, engine); lrc_reg_state = kmap(lrc_state_page); lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start; - ctx->engine[engine->id].lrc_reg_state = lrc_reg_state; + ce->lrc_reg_state = lrc_reg_state; ctx_obj->dirty = true; /* Invalidate GuC TLB. */ @@ -1138,9 +1139,10 @@ unpin_ctx_obj: static int intel_lr_context_pin(struct intel_context *ctx, struct intel_engine_cs *engine) { + struct intel_context_engine *ce = &ctx->engine[engine->id]; int ret = 0; - if (ctx->engine[engine->id].pin_count++ == 0) { + if (ce->pin_count++ == 0) { ret = intel_lr_context_do_pin(ctx, engine); if (ret) goto reset_pin_count; @@ -1150,23 +1152,25 @@ static int intel_lr_context_pin(struct intel_context *ctx, return ret; reset_pin_count: - ctx->engine[engine->id].pin_count = 0; + ce->pin_count = 0; return ret; } void intel_lr_context_unpin(struct intel_context *ctx, struct intel_engine_cs *engine) { - struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state; + struct intel_context_engine *ce = &ctx->engine[engine->id]; + struct drm_i915_gem_object *ctx_obj = ce->state; WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex)); - if (--ctx->engine[engine->id].pin_count == 0) { - kunmap(kmap_to_page(ctx->engine[engine->id].lrc_reg_state)); - intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf); + + if (--ce->pin_count == 0) { + kunmap(kmap_to_page(ce->lrc_reg_state)); + intel_unpin_ringbuffer_obj(ce->ringbuf); i915_gem_object_ggtt_unpin(ctx_obj); - ctx->engine[engine->id].lrc_vma = NULL; - ctx->engine[engine->id].lrc_desc = 0; - ctx->engine[engine->id].lrc_reg_state = NULL; + ce->lrc_vma = NULL; + ce->lrc_desc = 0; + ce->lrc_reg_state = NULL; i915_gem_context_unreference(ctx); } @@ -1177,8 +1181,7 @@ static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req) int ret, i; struct intel_engine_cs *engine = req->engine; struct intel_ringbuffer *ringbuf = req->ringbuf; - struct drm_device *dev = engine->dev; - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_private *dev_priv = req->i915; struct i915_workarounds *w = &dev_priv->workarounds; if (w->count == 0) @@ -2513,8 +2516,9 @@ void intel_lr_context_free(struct intel_context *ctx) int i; for (i = I915_NUM_ENGINES; --i >= 0; ) { - struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf; - struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state; + struct intel_context_engine *ce = &ctx->engine[i]; + struct intel_ringbuffer *ringbuf = ce->ringbuf; + struct drm_i915_gem_object *ctx_obj = ce->state; if (!ctx_obj) continue; @@ -2524,7 +2528,7 @@ void intel_lr_context_free(struct intel_context *ctx) i915_gem_object_ggtt_unpin(ctx_obj); } - WARN_ON(ctx->engine[i].pin_count); + WARN_ON(ce->pin_count); intel_ringbuffer_free(ringbuf); drm_gem_object_unreference(&ctx_obj->base); } @@ -2604,13 +2608,14 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx, struct intel_engine_cs *engine) { struct drm_device *dev = engine->dev; + struct intel_context_engine *ce = &ctx->engine[engine->id]; struct drm_i915_gem_object *ctx_obj; uint32_t context_size; struct intel_ringbuffer *ringbuf; int ret; WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL); - WARN_ON(ctx->engine[engine->id].state); + WARN_ON(ce->state); context_size = round_up(intel_lr_context_size(engine), 4096); @@ -2635,8 +2640,8 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx, goto error_ringbuf; } - ctx->engine[engine->id].ringbuf = ringbuf; - ctx->engine[engine->id].state = ctx_obj; + ce->ringbuf = ringbuf; + ce->state = ctx_obj; if (ctx != ctx->i915->kernel_context && engine->init_context) { struct drm_i915_gem_request *req; @@ -2663,8 +2668,8 @@ error_ringbuf: intel_ringbuffer_free(ringbuf); error_deref_obj: drm_gem_object_unreference(&ctx_obj->base); - ctx->engine[engine->id].ringbuf = NULL; - ctx->engine[engine->id].state = NULL; + ce->ringbuf = NULL; + ce->state = NULL; return ret; } @@ -2676,10 +2681,9 @@ void intel_lr_context_reset(struct drm_device *dev, int i; for_each_engine(engine, dev_priv, i) { - struct drm_i915_gem_object *ctx_obj = - ctx->engine[engine->id].state; - struct intel_ringbuffer *ringbuf = - ctx->engine[engine->id].ringbuf; + struct intel_context_engine *ce = &ctx->engine[engine->id]; + struct drm_i915_gem_object *ctx_obj = ce->state; + struct intel_ringbuffer *ringbuf = ce->ringbuf; uint32_t *reg_state; struct page *page;