| Message ID | 1458219850-21007-2-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 17, 2016 at 01:04:10PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Where we have a request we can use req->i915 directly instead > of going through the engine and device. Coccinelle script: > > @@ > function f; > identifier r; > @@ > f(..., struct drm_i915_gem_request *r, ...) > { > ... > - engine->dev->dev_private > + r->i915 > ... > } > @@ > struct drm_i915_gem_request *req; > @@ > ( > req-> > - engine->dev->dev_private > + i915 > ) struct intel_engine_cs *e; - e->dev->dev_private + e->i915 Pretty please? INTEL_INFO(engine->dev) (IS_GEN, HAS_ etc) if r: INTEL_INFO(r->i915) if e: INTEL_INFO(e->i915) ? -Chris
On 17/03/16 13:17, Chris Wilson wrote: > On Thu, Mar 17, 2016 at 01:04:10PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Where we have a request we can use req->i915 directly instead >> of going through the engine and device. Coccinelle script: >> >> @@ >> function f; >> identifier r; >> @@ >> f(..., struct drm_i915_gem_request *r, ...) >> { >> ... >> - engine->dev->dev_private >> + r->i915 >> ... >> } >> @@ >> struct drm_i915_gem_request *req; >> @@ >> ( >> req-> >> - engine->dev->dev_private >> + i915 >> ) > > struct intel_engine_cs *e; > - e->dev->dev_private > + e->i915 > > Pretty please? There is no e->i915 yet as far as I can see. > INTEL_INFO(engine->dev) (IS_GEN, HAS_ etc) > if r: INTEL_INFO(r->i915) > if e: INTEL_INFO(e->i915) > ? I have a patch to do: - dev->dev_private + to_i915(dev) But that is huge and to little gain. Maybe we should have: to_i915(req) to_i915(engine) to_i915(dev) And a smart macro which does the right thing at compile time? That way churn would be one time only and the magic happens in the macro definition, as shortcuts are added or (re)moved. Regards, Tvrtko
On Thu, Mar 17, 2016 at 01:47:31PM +0000, Tvrtko Ursulin wrote: > > On 17/03/16 13:17, Chris Wilson wrote: > >On Thu, Mar 17, 2016 at 01:04:10PM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Where we have a request we can use req->i915 directly instead > >>of going through the engine and device. Coccinelle script: > >> > >>@@ > >>function f; > >>identifier r; > >>@@ > >>f(..., struct drm_i915_gem_request *r, ...) > >>{ > >>... > >>- engine->dev->dev_private > >>+ r->i915 > >>... > >>} > >>@@ > >>struct drm_i915_gem_request *req; > >>@@ > >>( > >> req-> > >>- engine->dev->dev_private > >>+ i915 > >>) > > > >struct intel_engine_cs *e; > >- e->dev->dev_private > >+ e->i915 > > > >Pretty please? > > There is no e->i915 yet as far as I can see. Oh, you have seen it. It might be over there instead :| > >INTEL_INFO(engine->dev) (IS_GEN, HAS_ etc) > >if r: INTEL_INFO(r->i915) > >if e: INTEL_INFO(e->i915) > >? > > I have a patch to do: > > - dev->dev_private > + to_i915(dev) > > But that is huge and to little gain. > > Maybe we should have: > > to_i915(req) > to_i915(engine) > to_i915(dev) > > And a smart macro which does the right thing at compile time? Tempting. Should look nice and consistent and such magic would improve the look of many opening stanzas. > That way churn would be one time only and the magic happens in the > macro definition, as shortcuts are added or (re)moved. Aye. -Chris
On Thu, Mar 17, 2016 at 01:55:57PM +0000, Chris Wilson wrote: > On Thu, Mar 17, 2016 at 01:47:31PM +0000, Tvrtko Ursulin wrote: > > > > On 17/03/16 13:17, Chris Wilson wrote: > > >On Thu, Mar 17, 2016 at 01:04:10PM +0000, Tvrtko Ursulin wrote: > > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > >> > > >>Where we have a request we can use req->i915 directly instead > > >>of going through the engine and device. Coccinelle script: > > >> > > >>@@ > > >>function f; > > >>identifier r; > > >>@@ > > >>f(..., struct drm_i915_gem_request *r, ...) > > >>{ > > >>... > > >>- engine->dev->dev_private > > >>+ r->i915 > > >>... > > >>} > > >>@@ > > >>struct drm_i915_gem_request *req; > > >>@@ > > >>( > > >> req-> > > >>- engine->dev->dev_private > > >>+ i915 > > >>) > > > > > >struct intel_engine_cs *e; > > >- e->dev->dev_private > > >+ e->i915 > > > > > >Pretty please? > > > > There is no e->i915 yet as far as I can see. > > Oh, you have seen it. It might be over there instead :| > > > >INTEL_INFO(engine->dev) (IS_GEN, HAS_ etc) > > >if r: INTEL_INFO(r->i915) > > >if e: INTEL_INFO(e->i915) > > >? > > > > I have a patch to do: > > > > - dev->dev_private > > + to_i915(dev) > > > > But that is huge and to little gain. > > > > Maybe we should have: > > > > to_i915(req) > > to_i915(engine) > > to_i915(dev) > > > > And a smart macro which does the right thing at compile time? > > Tempting. Should look nice and consistent and such magic would improve > the look of many opening stanzas. This is imo a bit too much magic ... reg_to_i915, engine_to_i915 and to_i915 all sound like good ideas (we have lots of examples where we avoid the superclass, e.g. to_hdmi or similar). -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 9c4d9c186d91..042cb412e652 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2550,7 +2550,7 @@ void __i915_add_request(struct drm_i915_gem_request *request, return; engine = request->engine; - dev_priv = engine->dev->dev_private; + dev_priv = request->i915; ringbuf = request->ringbuf; /* diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 1993449ab7c5..6a3b4553119e 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -656,7 +656,7 @@ static int do_switch(struct drm_i915_gem_request *req) { struct intel_context *to = req->ctx; struct intel_engine_cs *engine = req->engine; - struct drm_i915_private *dev_priv = engine->dev->dev_private; + struct drm_i915_private *dev_priv = req->i915; struct intel_context *from = engine->last_context; u32 hw_flags = 0; bool uninitialized = false; @@ -829,7 +829,7 @@ unpin_out: int i915_switch_context(struct drm_i915_gem_request *req) { struct intel_engine_cs *engine = req->engine; - struct drm_i915_private *dev_priv = engine->dev->dev_private; + struct drm_i915_private *dev_priv = req->i915; WARN_ON(i915.enable_execlists); WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 9c752fe0f730..fb0f9637d46f 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2192,7 +2192,7 @@ int i915_ppgtt_init_hw(struct drm_device *dev) int i915_ppgtt_init_ring(struct drm_i915_gem_request *req) { - struct drm_i915_private *dev_priv = req->engine->dev->dev_private; + struct drm_i915_private *dev_priv = req->i915; struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; if (i915.enable_execlists) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index f72782200226..7c636b3db156 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -897,7 +897,7 @@ int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords) int ret; WARN_ON(req == NULL); - dev_priv = req->engine->dev->dev_private; + dev_priv = req->i915; ret = i915_gem_check_wedge(&dev_priv->gpu_error, dev_priv->mm.interruptible); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 015dc7db32b7..b7c8fc1a73a3 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2499,7 +2499,7 @@ int intel_ring_begin(struct drm_i915_gem_request *req, WARN_ON(req == NULL); engine = req->engine; - dev_priv = engine->dev->dev_private; + dev_priv = req->i915; ret = i915_gem_check_wedge(&dev_priv->gpu_error, dev_priv->mm.interruptible);