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