[Intel-gfx] [PATCH 4/6] drm/i915: Use to_i915() instead of guc_to_i915()

[Chris Wilson]

On Tue, Mar 22, 2016 at 10:55:40AM +0000, Dave Gordon wrote:
> On 18/03/16 21:16, Chris Wilson wrote:
> >-	for_each_engine(engine, dev_priv, i) {
> >+	for_each_engine(engine, guc, i) {
> 
> ... but not this (see earlier mail), although, the objection is less
> here because the GuC is singular and associated with all engines, so
> there isn't much else that we could expect to iterate over.
> 
> OTOH this may actually be less efficient, because the conversion of
> the "struct intel_guc" to the thing(s) actually needed for the
> iteration will (or at least may) occur on each iteration of the
> loop. Generally I'd prefer to pull all such conversions out to the
> head of the function, as the original code did.

It's an init func, the question is simply which is more readable.

> >  		struct guc_execlist_context *lrc = &desc.lrc[engine->guc_id];
> >  		struct drm_i915_gem_object *obj;
> >  		uint64_t ctx_desc;
> >@@ -772,7 +771,6 @@ err:
> >
> >  static void guc_create_log(struct intel_guc *guc)
> >  {
> >-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> >  	struct drm_i915_gem_object *obj;
> >  	unsigned long offset;
> >  	uint32_t size, flags;
> >@@ -791,7 +789,7 @@ static void guc_create_log(struct intel_guc *guc)
> >
> >  	obj = guc->log_obj;
> >  	if (!obj) {
> >-		obj = gem_allocate_guc_obj(dev_priv->dev, size);
> >+		obj = gem_allocate_guc_obj(to_i915(guc)->dev, size);
> 
> Should we have to_dev(any) as well as to_i915()?
> 
> >  		if (!obj) {
> >  			/* logging will be off */
> >  			i915.guc_log_level = -1;
> >@@ -835,7 +833,6 @@ static void init_guc_policies(struct guc_policies *policies)
> >
> >  static void guc_create_ads(struct intel_guc *guc)
> >  {
> >-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> 
> This dev_priv is used more than once (in fact, it's used in a
> for_each_engine() loop below), so I'd think it worth keeping -- and
> therefore none of the changes below would be applicable.

There's a later change to fix that since these functions are attrocious,
in terms of layering name and paramter abuse.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre