[resend,for,CI,2/2] drm/i915: Use shorter route to dev_private where possible
diff mbox

Message ID 1458219850-21007-2-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin March 17, 2016, 1:04 p.m. UTC
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
)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c         | 2 +-
 drivers/gpu/drm/i915/i915_gem_context.c | 4 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.c     | 2 +-
 drivers/gpu/drm/i915/intel_lrc.c        | 2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

Comments

Chris Wilson March 17, 2016, 1:17 p.m. UTC | #1
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
Tvrtko Ursulin March 17, 2016, 1:47 p.m. UTC | #2
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
Chris Wilson March 17, 2016, 1:55 p.m. UTC | #3
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
Daniel Vetter March 21, 2016, 9:25 a.m. UTC | #4
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

Patch
diff mbox

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);