[Intel-gfx] [PATCH v2 08/15] drm/i915/guc: Split relay control and GuC log level

[Chris Wilson]

Quoting Michał Winiarski (2018-03-09 16:30:42)
> On Fri, Mar 09, 2018 at 12:00:06PM +0100, Michal Wajdeczko wrote:
> > On Thu, 08 Mar 2018 16:47:00 +0100, Michał Winiarski
> > <michal.winiarski at intel.com> wrote:
> > 
> > > Those two concepts are really separate. Since GuC is writing data into
> > > its own buffer and we even provide a way for userspace to read directly
> > > from it using i915_guc_log_dump debugfs, there's no real reason to tie
> > > log level with relay creation.
> > > Let's create a separate debugfs, giving userspace a way to create a
> > > relay on demand, when it wants to read a continuous log rather than a
> > > snapshot.
> > > 
> > > v2: Don't touch guc_log_level on relay creation error, adjust locking
> > >     after rebase, s/dev_priv/i915, pass guc to file->private_data (Sagar)
> > >     Use struct_mutex rather than runtime.lock for set_log_level
> > > 
> > > Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > > Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > ---
> > 
> > /snip/
> > 
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.c
> > > b/drivers/gpu/drm/i915/intel_uc.c
> > > index 90d2f38e22c9..abce0e38528a 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.c
> > > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > > @@ -219,28 +219,6 @@ static void guc_free_load_err_log(struct intel_guc
> > > *guc)
> > >             i915_gem_object_put(guc->load_err_log);
> > >  }
> > > -int intel_uc_register(struct drm_i915_private *i915)
> > > -{
> > > -   int ret = 0;
> > > -
> > > -   if (!USES_GUC(i915))
> > > -           return 0;
> > > -
> > > -   if (i915_modparams.guc_log_level)
> > > -           ret = intel_guc_log_register(&i915->guc);
> > > -
> > > -   return ret;
> > > -}
> > > -
> > > -void intel_uc_unregister(struct drm_i915_private *i915)
> > > -{
> > > -   if (!USES_GUC(i915))
> > > -           return;
> > > -
> > > -   if (i915_modparams.guc_log_level)
> > > -           intel_guc_log_unregister(&i915->guc);
> > > -}
> > > -
> > >  static int guc_enable_communication(struct intel_guc *guc)
> > >  {
> > >     struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.h
> > > b/drivers/gpu/drm/i915/intel_uc.h
> > > index d6af984cd789..f76d51d1ce70 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.h
> > > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > > @@ -31,8 +31,6 @@
> > >  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
> > >  void intel_uc_init_early(struct drm_i915_private *dev_priv);
> > >  void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
> > > -int intel_uc_register(struct drm_i915_private *dev_priv);
> > > -void intel_uc_unregister(struct drm_i915_private *dev_priv);
> > 
> > hmm, it looks that timelines of these two functions were very short
> > (from patch 2 till patch 8) - so maybe by doing some patch re-order
> > can we fully skip them ?
> 
> I could drop patch 2 entirely and go straight to the decoupling - but that means
> more code movement overall and way more content in this one.
> 
> I don't really see a better split (but since I'm the author I'm biased :) ).
> I'm open for suggestions.

I can settle the argument by grabbing the first 3 patches as code
movement, before we get into the details. Ok?
-Chris