| Message ID | 20180314183715.28808-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 14, 2018 at 06:37:15PM +0000, Michal Wajdeczko wrote: > We should avoid using guc_log prefix for functions that don't > operate on GuC log, but rather request action from the GuC. > Better to use guc_action prefix. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_log.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index b9c7bd7..457168a 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -39,7 +39,7 @@ > * registers value. > */ > > -static int guc_log_flush_complete(struct intel_guc *guc) > +static int guc_action_flush_log_complete(struct intel_guc *guc) > { > u32 action[] = { > INTEL_GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE > @@ -48,7 +48,7 @@ static int guc_log_flush_complete(struct intel_guc *guc) > return intel_guc_send(guc, action, ARRAY_SIZE(action)); > } > > -static int guc_log_flush(struct intel_guc *guc) > +static int guc_action_flush_log(struct intel_guc *guc) > { > u32 action[] = { > INTEL_GUC_ACTION_FORCE_LOG_BUFFER_FLUSH, > @@ -58,7 +58,8 @@ static int guc_log_flush(struct intel_guc *guc) > return intel_guc_send(guc, action, ARRAY_SIZE(action)); > } > > -static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity) > +static int guc_action_enable_log(struct intel_guc *guc, bool enable, > + u32 verbosity) Let's hide the fact that the actual action is called "ENABLE_LOGGING", and stick with guc_action_log_control, especially since we're using guc_log_control union, and the action itself is also used for verbosity (and default log... more than just enable/disable switch). With that: Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> -Michał > { > union guc_log_control control_val = { > { > @@ -525,7 +526,7 @@ static void guc_log_capture_logs(struct intel_guc *guc) > * time, so get/put should be really quick. > */ > intel_runtime_pm_get(dev_priv); > - guc_log_flush_complete(guc); > + guc_action_flush_log_complete(guc); > intel_runtime_pm_put(dev_priv); > } > > @@ -541,7 +542,7 @@ static void guc_flush_logs(struct intel_guc *guc) > > /* Ask GuC to update the log buffer state */ > intel_runtime_pm_get(dev_priv); > - guc_log_flush(guc); > + guc_action_flush_log(guc); > intel_runtime_pm_put(dev_priv); > > /* GuC would have updated log buffer by now, so capture it */ > @@ -639,10 +640,11 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) > } > > intel_runtime_pm_get(dev_priv); > - ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val)); > + ret = guc_action_enable_log(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val)); > intel_runtime_pm_put(dev_priv); > if (ret) { > - DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret); > + DRM_DEBUG_DRIVER("GuC action to %s log failed (%d)\n", > + enabled ? "enable" : "disable", ret); > goto out_unlock; > } > > -- > 1.9.1 >
On Thu, 15 Mar 2018 16:57:26 +0100, Michał Winiarski <michal.winiarski@intel.com> wrote: > On Wed, Mar 14, 2018 at 06:37:15PM +0000, Michal Wajdeczko wrote: >> We should avoid using guc_log prefix for functions that don't >> operate on GuC log, but rather request action from the GuC. >> Better to use guc_action prefix. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michal Winiarski <michal.winiarski@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc_log.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c >> b/drivers/gpu/drm/i915/intel_guc_log.c >> index b9c7bd7..457168a 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >> @@ -39,7 +39,7 @@ >> * registers value. >> */ >> >> -static int guc_log_flush_complete(struct intel_guc *guc) >> +static int guc_action_flush_log_complete(struct intel_guc *guc) >> { >> u32 action[] = { >> INTEL_GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE >> @@ -48,7 +48,7 @@ static int guc_log_flush_complete(struct intel_guc >> *guc) >> return intel_guc_send(guc, action, ARRAY_SIZE(action)); >> } >> >> -static int guc_log_flush(struct intel_guc *guc) >> +static int guc_action_flush_log(struct intel_guc *guc) >> { >> u32 action[] = { >> INTEL_GUC_ACTION_FORCE_LOG_BUFFER_FLUSH, >> @@ -58,7 +58,8 @@ static int guc_log_flush(struct intel_guc *guc) >> return intel_guc_send(guc, action, ARRAY_SIZE(action)); >> } >> >> -static int guc_log_control(struct intel_guc *guc, bool enable, u32 >> verbosity) >> +static int guc_action_enable_log(struct intel_guc *guc, bool enable, >> + u32 verbosity) > > Let's hide the fact that the actual action is called "ENABLE_LOGGING", > and stick > with guc_action_log_control, especially since we're using > guc_log_control union, > and the action itself is also used for verbosity (and default log... > more than > just enable/disable switch). Hmm, I think that using action name as base for function is right thing. If in your opinion action name is not correct, we should start with action rename first. And I would rather prefer to drop definition of union guc_log_control and replace it with set of SHIFT/MASK macros as we do for other bitfields. Also using actual action name as base for new function name, we could avoid having yet another [log|control|log] function name permutation. But I'm flexible ;) > > With that: > > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > > -Michał > >> { >> union guc_log_control control_val = { >> { >> @@ -525,7 +526,7 @@ static void guc_log_capture_logs(struct intel_guc >> *guc) >> * time, so get/put should be really quick. >> */ >> intel_runtime_pm_get(dev_priv); >> - guc_log_flush_complete(guc); >> + guc_action_flush_log_complete(guc); >> intel_runtime_pm_put(dev_priv); >> } >> >> @@ -541,7 +542,7 @@ static void guc_flush_logs(struct intel_guc *guc) >> >> /* Ask GuC to update the log buffer state */ >> intel_runtime_pm_get(dev_priv); >> - guc_log_flush(guc); >> + guc_action_flush_log(guc); >> intel_runtime_pm_put(dev_priv); >> >> /* GuC would have updated log buffer by now, so capture it */ >> @@ -639,10 +640,11 @@ int intel_guc_log_control_set(struct intel_guc >> *guc, u64 val) >> } >> >> intel_runtime_pm_get(dev_priv); >> - ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val)); >> + ret = guc_action_enable_log(guc, enabled, >> LOG_LEVEL_TO_VERBOSITY(val)); >> intel_runtime_pm_put(dev_priv); >> if (ret) { >> - DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret); >> + DRM_DEBUG_DRIVER("GuC action to %s log failed (%d)\n", >> + enabled ? "enable" : "disable", ret); >> goto out_unlock; >> } >> >> -- >> 1.9.1
On Thu, Mar 15, 2018 at 05:19:27PM +0100, Michal Wajdeczko wrote: > On Thu, 15 Mar 2018 16:57:26 +0100, Michał Winiarski > <michal.winiarski@intel.com> wrote: > > > On Wed, Mar 14, 2018 at 06:37:15PM +0000, Michal Wajdeczko wrote: > > > We should avoid using guc_log prefix for functions that don't > > > operate on GuC log, but rather request action from the GuC. > > > Better to use guc_action prefix. > > > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Cc: Michal Winiarski <michal.winiarski@intel.com> > > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_guc_log.c | 16 +++++++++------- > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c > > > b/drivers/gpu/drm/i915/intel_guc_log.c > > > index b9c7bd7..457168a 100644 > > > --- a/drivers/gpu/drm/i915/intel_guc_log.c > > > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > > > @@ -39,7 +39,7 @@ > > > * registers value. > > > */ > > > > > > -static int guc_log_flush_complete(struct intel_guc *guc) > > > +static int guc_action_flush_log_complete(struct intel_guc *guc) > > > { > > > u32 action[] = { > > > INTEL_GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE > > > @@ -48,7 +48,7 @@ static int guc_log_flush_complete(struct intel_guc > > > *guc) > > > return intel_guc_send(guc, action, ARRAY_SIZE(action)); > > > } > > > > > > -static int guc_log_flush(struct intel_guc *guc) > > > +static int guc_action_flush_log(struct intel_guc *guc) > > > { > > > u32 action[] = { > > > INTEL_GUC_ACTION_FORCE_LOG_BUFFER_FLUSH, > > > @@ -58,7 +58,8 @@ static int guc_log_flush(struct intel_guc *guc) > > > return intel_guc_send(guc, action, ARRAY_SIZE(action)); > > > } > > > > > > -static int guc_log_control(struct intel_guc *guc, bool enable, u32 > > > verbosity) > > > +static int guc_action_enable_log(struct intel_guc *guc, bool enable, > > > + u32 verbosity) > > > > Let's hide the fact that the actual action is called "ENABLE_LOGGING", > > and stick > > with guc_action_log_control, especially since we're using > > guc_log_control union, > > and the action itself is also used for verbosity (and default log... > > more than > > just enable/disable switch). > > Hmm, I think that using action name as base for function is right thing. > If in your opinion action name is not correct, we should start with action > rename first. Nooo, then we're going to have i915/GuC mismatch :/ > And I would rather prefer to drop definition of union guc_log_control > and replace it with set of SHIFT/MASK macros as we do for other bitfields. Sure - why not. > Also using actual action name as base for new function name, we could > avoid having yet another [log|control|log] function name permutation. We're not consistent with maching action/function name, and I think 4 arguments "enable" function is going to be really confusing. But I don't have a strong opinion here, it's going to be used in a single place, and it has "guc_action_*" warning sign now ;) -Michał > But I'm flexible ;) > > > > > With that: > > > > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > > > > -Michał > > > > > > > > /* GuC would have updated log buffer by now, so capture it */ > > > @@ -639,10 +640,11 @@ int intel_guc_log_control_set(struct intel_guc > > > *guc, u64 val) > > > } > > > > > > intel_runtime_pm_get(dev_priv); > > > - ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val)); > > > + ret = guc_action_enable_log(guc, enabled, > > > LOG_LEVEL_TO_VERBOSITY(val)); > > > intel_runtime_pm_put(dev_priv); > > > if (ret) { > > > - DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret); > > > + DRM_DEBUG_DRIVER("GuC action to %s log failed (%d)\n", > > > + enabled ? "enable" : "disable", ret); > > > goto out_unlock; > > > } > > > > > > -- > > > 1.9.1
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index b9c7bd7..457168a 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -39,7 +39,7 @@ * registers value. */ -static int guc_log_flush_complete(struct intel_guc *guc) +static int guc_action_flush_log_complete(struct intel_guc *guc) { u32 action[] = { INTEL_GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE @@ -48,7 +48,7 @@ static int guc_log_flush_complete(struct intel_guc *guc) return intel_guc_send(guc, action, ARRAY_SIZE(action)); } -static int guc_log_flush(struct intel_guc *guc) +static int guc_action_flush_log(struct intel_guc *guc) { u32 action[] = { INTEL_GUC_ACTION_FORCE_LOG_BUFFER_FLUSH, @@ -58,7 +58,8 @@ static int guc_log_flush(struct intel_guc *guc) return intel_guc_send(guc, action, ARRAY_SIZE(action)); } -static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity) +static int guc_action_enable_log(struct intel_guc *guc, bool enable, + u32 verbosity) { union guc_log_control control_val = { { @@ -525,7 +526,7 @@ static void guc_log_capture_logs(struct intel_guc *guc) * time, so get/put should be really quick. */ intel_runtime_pm_get(dev_priv); - guc_log_flush_complete(guc); + guc_action_flush_log_complete(guc); intel_runtime_pm_put(dev_priv); } @@ -541,7 +542,7 @@ static void guc_flush_logs(struct intel_guc *guc) /* Ask GuC to update the log buffer state */ intel_runtime_pm_get(dev_priv); - guc_log_flush(guc); + guc_action_flush_log(guc); intel_runtime_pm_put(dev_priv); /* GuC would have updated log buffer by now, so capture it */ @@ -639,10 +640,11 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) } intel_runtime_pm_get(dev_priv); - ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val)); + ret = guc_action_enable_log(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val)); intel_runtime_pm_put(dev_priv); if (ret) { - DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret); + DRM_DEBUG_DRIVER("GuC action to %s log failed (%d)\n", + enabled ? "enable" : "disable", ret); goto out_unlock; }
We should avoid using guc_log prefix for functions that don't operate on GuC log, but rather request action from the GuC. Better to use guc_action prefix. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_guc_log.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)