Re: [PATCH 01/15] drm/i915/guc: Tidy guc_log_control

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 02, 2018 at 04:39:38PM +0530, Sagar Arun Kamble wrote:
> 
> 
> On 2/27/2018 6:22 PM, Michał Winiarski wrote:
> > We plan to decouple log runtime (mapping + relay) from verbosity control.
> > Let's tidy the code now to reduce the churn in the following patches.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_debugfs.c  | 11 ++----
> >   drivers/gpu/drm/i915/intel_guc_log.c | 75 +++++++++++++++++++-----------------
> >   drivers/gpu/drm/i915/intel_guc_log.h |  3 +-
> >   3 files changed, 46 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 33fbf3965309..58983cafaece 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2500,13 +2500,10 @@ static int i915_guc_log_control_get(void *data, u64 *val)
> Should we name this i915_guc_log_level_get instead? and other related
> functions too?

I chose symmetry here, note that the debugfs file is still named
i915_guc_log_control at this point. This changes later in the series though.

> >   {
> >   	struct drm_i915_private *dev_priv = data;
> > -	if (!HAS_GUC(dev_priv))
> > +	if (!USES_GUC(dev_priv))
> >   		return -ENODEV;
> > -	if (!dev_priv->guc.log.vma)
> > -		return -EINVAL;
> > -
> > -	*val = i915_modparams.guc_log_level;
> > +	*val = intel_guc_log_control_get(&dev_priv->guc);
> >   	return 0;
> >   }
> > @@ -2515,10 +2512,10 @@ static int i915_guc_log_control_set(void *data, u64 val)
> >   {
> >   	struct drm_i915_private *dev_priv = data;
> > -	if (!HAS_GUC(dev_priv))
> > +	if (!USES_GUC(dev_priv))
> >   		return -ENODEV;
> > -	return intel_guc_log_control(&dev_priv->guc, val);
> > +	return intel_guc_log_control_set(&dev_priv->guc, val);
> >   }
> >   DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
> > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> > index 7b5074e2120c..22a05320817b 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_log.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> > @@ -657,52 +657,55 @@ void intel_guc_log_destroy(struct intel_guc *guc)
> >   	i915_vma_unpin_and_release(&guc->log.vma);
> >   }
> > -int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
> > +int intel_guc_log_control_get(struct intel_guc *guc)
> Should we be passing guc_log as parameter and implement guc_log_to_guc()
> function.

This is the top-level interface exported for GuC users. In other words - callers
of this function shouldn't have to know about struct guc_log (and the fact that
it's located inside struct intel_guc).

> > +{
> > +	GEM_BUG_ON(!guc->log.vma);
> > +	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> > +
> > +	return i915_modparams.guc_log_level;
> > +}
> > +
> > +#define GUC_LOG_IS_ENABLED(x)		(x > 0)
> > +#define GUC_LOG_LEVEL_TO_VERBOSITY(x)	(GUC_LOG_IS_ENABLED(x) ? x - 1 : 0)
> This is bit misleading, can we make this macro return -1 if logging is to be
> disabled. That way guc_log_control can be invoked with
> single signed 32bit parameter.

Note that guc_log_control is the function operating directly on GuC interface.
This Host2GuC action really takes 3 arguments (2 parameters here) - enable,
default_logging_enable, verbosity.
As a consequence, I'd like to avoid placing any logic there. The macros are
taking care of translation from guc_log_level modparam to values understood by
GuC (host2guc params).

I agree that the naming is confusing here.
I'll go with LOG_LEVEL_TO_ENABLED(x) and LOG_LEVEL_TO_VERBOSITY(x) in second
spin as suggested by Michał.

> > +int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
> >   {
> >   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > -	bool enable_logging = control_val > 0;
> > -	u32 verbosity;
> >   	int ret;
> > -	if (!guc->log.vma)
> > -		return -ENODEV;
> > +	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
> > +	GEM_BUG_ON(!guc->log.vma);
> > +	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> > -	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN);
> > -	if (control_val > 1 + GUC_LOG_VERBOSITY_MAX)
> > +	/*
> > +	 * GuC is recognizing log levels starting from 0 to max, we're using 0
> > +	 * as indication that logging should be disablded.
> > +	 */
> > +	if (GUC_LOG_LEVEL_TO_VERBOSITY(val) < GUC_LOG_VERBOSITY_MIN ||
> This check seems unnecessary as we currently don't have negative output for
> G_L_L_T_V macro.
> If we add negative value there, will need to remove this check.

Yeah, agree. That's an error on my part, I wanted to do input validation here.
This should probably be something more like:
if (val < VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MIN) ||
    val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))

-Michał

> > +	    GUC_LOG_LEVEL_TO_VERBOSITY(val) > GUC_LOG_VERBOSITY_MAX)
> >   		return -EINVAL;
> > -	/* This combination doesn't make sense & won't have any effect */
> > -	if (!enable_logging && !i915_modparams.guc_log_level)
> > -		return 0;
> > +	mutex_lock(&dev_priv->drm.struct_mutex);
> > -	verbosity = enable_logging ? control_val - 1 : 0;
> > +	if (i915_modparams.guc_log_level == val) {
> > +		ret = 0;
> > +		goto out_unlock;
> > +	}
> > -	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> > -	if (ret)
> > -		return ret;
> >   	intel_runtime_pm_get(dev_priv);
> > -	ret = guc_log_control(guc, enable_logging, verbosity);
> > +	ret = guc_log_control(guc, GUC_LOG_IS_ENABLED(val),
> > +			      GUC_LOG_LEVEL_TO_VERBOSITY(val));
> >   	intel_runtime_pm_put(dev_priv);
> > -	mutex_unlock(&dev_priv->drm.struct_mutex);
> > +	if (ret)
> > +		goto out_unlock;
> > -	if (ret < 0) {
> > -		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
> > -		return ret;
> > -	}
> > +	i915_modparams.guc_log_level = val;
> > -	if (enable_logging) {
> > -		i915_modparams.guc_log_level = 1 + verbosity;
> > +	mutex_unlock(&dev_priv->drm.struct_mutex);
> > -		/*
> > -		 * If log was disabled at boot time, then the relay channel file
> > -		 * wouldn't have been created by now and interrupts also would
> > -		 * not have been enabled. Try again now, just in case.
> > -		 */
> > +	if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) {
> >   		ret = guc_log_late_setup(guc);
> > -		if (ret < 0) {
> > -			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
> > -			return ret;
> > -		}
> > +		if (ret)
> > +			goto out;
> >   		/* GuC logging is currently the only user of Guc2Host interrupts */
> >   		mutex_lock(&dev_priv->drm.struct_mutex);
> > @@ -710,7 +713,7 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
> >   		gen9_enable_guc_interrupts(dev_priv);
> >   		intel_runtime_pm_put(dev_priv);
> >   		mutex_unlock(&dev_priv->drm.struct_mutex);
> > -	} else {
> > +	} else if (!GUC_LOG_IS_ENABLED(val) && guc_log_has_runtime(guc)) {
> >   		/*
> >   		 * Once logging is disabled, GuC won't generate logs & send an
> >   		 * interrupt. But there could be some data in the log buffer
> > @@ -718,11 +721,13 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
> >   		 * buffer state and then collect the left over logs.
> >   		 */
> >   		guc_flush_logs(guc);
> > -
> > -		/* As logging is disabled, update log level to reflect that */
> > -		i915_modparams.guc_log_level = 0;
> >   	}
> > +	return 0;
> > +
> > +out_unlock:
> > +	mutex_unlock(&dev_priv->drm.struct_mutex);
> > +out:
> >   	return ret;
> >   }
> > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> > index dab0e949567a..141ce9ca22ce 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_log.h
> > +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> > @@ -64,7 +64,8 @@ void intel_guc_log_destroy(struct intel_guc *guc);
> >   void intel_guc_log_init_early(struct intel_guc *guc);
> >   int intel_guc_log_relay_create(struct intel_guc *guc);
> >   void intel_guc_log_relay_destroy(struct intel_guc *guc);
> > -int intel_guc_log_control(struct intel_guc *guc, u64 control_val);
> > +int intel_guc_log_control_get(struct intel_guc *guc);
> > +int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
> >   void i915_guc_log_register(struct drm_i915_private *dev_priv);
> >   void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> 
> -- 
> Thanks,
> Sagar
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux