| Message ID | 1516766952-8231-4-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 24 Jan 2018 05:09:10 +0100, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > i915_guc_log_control is GuC interface and GuC APIs that are not user > facing should be named with "intel_guc" prefix hence we change name to > intel_guc_log_control. Also changed the parameter to intel_guc struct. > > Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> with small bikeshed below > drivers/gpu/drm/i915/i915_debugfs.c | 5 +++-- > drivers/gpu/drm/i915/intel_guc_log.c | 4 ++-- > drivers/gpu/drm/i915/intel_guc_log.h | 2 +- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index b45be0d..c10a9e8 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2467,14 +2467,15 @@ static int i915_guc_log_control_get(void *data, > u64 *val) > static int i915_guc_log_control_set(void *data, u64 val) > { > struct drm_i915_private *dev_priv = data; > + struct intel_guc *guc = &dev_priv->guc; > if (!HAS_GUC(dev_priv)) > return -ENODEV; > - if (!dev_priv->guc.log.vma) > + if (!guc->log.vma) > return -EINVAL; Hmm, as this looks like internal check, maybe better to move it into intel_guc_log_control() ? Also, I'm not sure that lack of internal vma should be reported as -EINVAL > - return i915_guc_log_control(dev_priv, val); > + return intel_guc_log_control(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 35de889..27eb545 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -637,9 +637,9 @@ void intel_guc_log_destroy(struct intel_guc *guc) > i915_vma_unpin_and_release(&guc->log.vma); > } > -int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 > control_val) > +int intel_guc_log_control(struct intel_guc *guc, u64 control_val) > { > - struct intel_guc *guc = &dev_priv->guc; > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > bool enable_logging = control_val > 0; > u32 verbosity; > int ret; > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h > b/drivers/gpu/drm/i915/intel_guc_log.h > index c638b9d..dab0e94 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.h > +++ b/drivers/gpu/drm/i915/intel_guc_log.h > @@ -64,7 +64,7 @@ struct intel_guc_log { > 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 i915_guc_log_control(struct drm_i915_private *dev_priv, u64 > control_val); > +int intel_guc_log_control(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);
Quoting Michal Wajdeczko (2018-01-24 10:07:12) > On Wed, 24 Jan 2018 05:09:10 +0100, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > > > i915_guc_log_control is GuC interface and GuC APIs that are not user > > facing should be named with "intel_guc" prefix hence we change name to > > intel_guc_log_control. Also changed the parameter to intel_guc struct. > > > > Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > with small bikeshed below > > > drivers/gpu/drm/i915/i915_debugfs.c | 5 +++-- > > drivers/gpu/drm/i915/intel_guc_log.c | 4 ++-- > > drivers/gpu/drm/i915/intel_guc_log.h | 2 +- > > 3 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index b45be0d..c10a9e8 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2467,14 +2467,15 @@ static int i915_guc_log_control_get(void *data, > > u64 *val) > > static int i915_guc_log_control_set(void *data, u64 val) > > { > > struct drm_i915_private *dev_priv = data; > > + struct intel_guc *guc = &dev_priv->guc; > > if (!HAS_GUC(dev_priv)) > > return -ENODEV; > > - if (!dev_priv->guc.log.vma) > > + if (!guc->log.vma) > > return -EINVAL; > > Hmm, as this looks like internal check, maybe better to move > it into intel_guc_log_control() ? > > Also, I'm not sure that lack of internal vma should be reported > as -EINVAL Right, it's not reporting that the user's parameter was wrong, just that the device wasn't initialised, so ENODEV seems appropriate. -Chris
On 1/24/2018 3:41 PM, Chris Wilson wrote: > Quoting Michal Wajdeczko (2018-01-24 10:07:12) >> On Wed, 24 Jan 2018 05:09:10 +0100, Sagar Arun Kamble >> <sagar.a.kamble@intel.com> wrote: >> >>> i915_guc_log_control is GuC interface and GuC APIs that are not user >>> facing should be named with "intel_guc" prefix hence we change name to >>> intel_guc_log_control. Also changed the parameter to intel_guc struct. >>> >>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> >> with small bikeshed below >> >>> drivers/gpu/drm/i915/i915_debugfs.c | 5 +++-- >>> drivers/gpu/drm/i915/intel_guc_log.c | 4 ++-- >>> drivers/gpu/drm/i915/intel_guc_log.h | 2 +- >>> 3 files changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>> b/drivers/gpu/drm/i915/i915_debugfs.c >>> index b45be0d..c10a9e8 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -2467,14 +2467,15 @@ static int i915_guc_log_control_get(void *data, >>> u64 *val) >>> static int i915_guc_log_control_set(void *data, u64 val) >>> { >>> struct drm_i915_private *dev_priv = data; >>> + struct intel_guc *guc = &dev_priv->guc; >>> if (!HAS_GUC(dev_priv)) >>> return -ENODEV; >>> - if (!dev_priv->guc.log.vma) >>> + if (!guc->log.vma) >>> return -EINVAL; >> Hmm, as this looks like internal check, maybe better to move >> it into intel_guc_log_control() ? >> >> Also, I'm not sure that lack of internal vma should be reported >> as -EINVAL > Right, it's not reporting that the user's parameter was wrong, just that > the device wasn't initialised, so ENODEV seems appropriate. Thanks for the review and inputs. > -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index b45be0d..c10a9e8 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2467,14 +2467,15 @@ static int i915_guc_log_control_get(void *data, u64 *val) static int i915_guc_log_control_set(void *data, u64 val) { struct drm_i915_private *dev_priv = data; + struct intel_guc *guc = &dev_priv->guc; if (!HAS_GUC(dev_priv)) return -ENODEV; - if (!dev_priv->guc.log.vma) + if (!guc->log.vma) return -EINVAL; - return i915_guc_log_control(dev_priv, val); + return intel_guc_log_control(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 35de889..27eb545 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -637,9 +637,9 @@ void intel_guc_log_destroy(struct intel_guc *guc) i915_vma_unpin_and_release(&guc->log.vma); } -int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) +int intel_guc_log_control(struct intel_guc *guc, u64 control_val) { - struct intel_guc *guc = &dev_priv->guc; + struct drm_i915_private *dev_priv = guc_to_i915(guc); bool enable_logging = control_val > 0; u32 verbosity; int ret; diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h index c638b9d..dab0e94 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.h +++ b/drivers/gpu/drm/i915/intel_guc_log.h @@ -64,7 +64,7 @@ struct intel_guc_log { 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 i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val); +int intel_guc_log_control(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);
i915_guc_log_control is GuC interface and GuC APIs that are not user facing should be named with "intel_guc" prefix hence we change name to intel_guc_log_control. Also changed the parameter to intel_guc struct. Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_debugfs.c | 5 +++-- drivers/gpu/drm/i915/intel_guc_log.c | 4 ++-- drivers/gpu/drm/i915/intel_guc_log.h | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-)