| Message ID | 20170330112115.120240-3-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 30, 2017 at 11:21:12AM +0000, Michal Wajdeczko wrote: > Some of the DRM_NOTE messages are just using "uC" without specifying > which uc they are related to. We can be more user friendly. > > v2: moved to the header (Joonas) > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_uc.c | 6 ++++-- > drivers/gpu/drm/i915/intel_uc.h | 14 ++++++++++++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index c767dc3..e259cae 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -182,10 +182,12 @@ static void fetch_uc_fw(struct drm_i915_private *dev_priv, > } > > if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) { > - DRM_NOTE("Skipping uC firmware version check\n"); > + DRM_NOTE("Skipping %s firmware version check\n", > + intel_uc_fw_type_repr(uc_fw->type)); > } else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || > uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { > - DRM_NOTE("uC firmware version %d.%d, required %d.%d\n", > + DRM_NOTE("%s firmware version %d.%d, required %d.%d\n", > + intel_uc_fw_type_repr(uc_fw->type), > uc_fw->major_ver_found, uc_fw->minor_ver_found, > uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted); > err = -ENOEXEC; > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index f524387..7139e31 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -125,6 +125,20 @@ enum intel_uc_fw_type { > INTEL_UC_FW_TYPE_HUC > }; > > +/* User-friendly representation of an enum */ > +static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type) > +{ > + switch (type) { > + case INTEL_UC_FW_TYPE_GUC: > + return "GuC"; > + case INTEL_UC_FW_TYPE_HUC: > + return "HuC"; > + default: > + MISSING_CASE(type); > + return "<invalid>"; > + } Do we want to write these (when we have a clear enum type): switch (type) { case INTEL_UC_FW_TYPE_GUC: return "GuC"; case INTEL_UC_FW_TYPE_HUC: return "HuC"; } MISSING_CASE(type); return "uC"; The hope being that the compiler will give us a warning before we encounter the runtime WARN. -Chris
On Thu, Mar 30, 2017 at 12:27:45PM +0100, Chris Wilson wrote: > On Thu, Mar 30, 2017 at 11:21:12AM +0000, Michal Wajdeczko wrote: > > Some of the DRM_NOTE messages are just using "uC" without specifying > > which uc they are related to. We can be more user friendly. > > > > v2: moved to the header (Joonas) > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_uc.c | 6 ++++-- > > drivers/gpu/drm/i915/intel_uc.h | 14 ++++++++++++++ > > 2 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > > index c767dc3..e259cae 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -182,10 +182,12 @@ static void fetch_uc_fw(struct drm_i915_private *dev_priv, > > } > > > > if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) { > > - DRM_NOTE("Skipping uC firmware version check\n"); > > + DRM_NOTE("Skipping %s firmware version check\n", > > + intel_uc_fw_type_repr(uc_fw->type)); > > } else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || > > uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { > > - DRM_NOTE("uC firmware version %d.%d, required %d.%d\n", > > + DRM_NOTE("%s firmware version %d.%d, required %d.%d\n", > > + intel_uc_fw_type_repr(uc_fw->type), > > uc_fw->major_ver_found, uc_fw->minor_ver_found, > > uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted); > > err = -ENOEXEC; > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > > index f524387..7139e31 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.h > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > @@ -125,6 +125,20 @@ enum intel_uc_fw_type { > > INTEL_UC_FW_TYPE_HUC > > }; > > > > +/* User-friendly representation of an enum */ > > +static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type) > > +{ > > + switch (type) { > > + case INTEL_UC_FW_TYPE_GUC: > > + return "GuC"; > > + case INTEL_UC_FW_TYPE_HUC: > > + return "HuC"; > > + default: > > + MISSING_CASE(type); > > + return "<invalid>"; > > + } > > Do we want to write these (when we have a clear enum type): > > switch (type) { > case INTEL_UC_FW_TYPE_GUC: return "GuC"; > case INTEL_UC_FW_TYPE_HUC: return "HuC"; > } > > MISSING_CASE(type); > return "uC"; > > The hope being that the compiler will give us a warning before we > encounter the runtime WARN. I was already proposing this option to rely on compiler, but there was a single nack from Anusha, now overbid ;) -Michal
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index c767dc3..e259cae 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -182,10 +182,12 @@ static void fetch_uc_fw(struct drm_i915_private *dev_priv, } if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) { - DRM_NOTE("Skipping uC firmware version check\n"); + DRM_NOTE("Skipping %s firmware version check\n", + intel_uc_fw_type_repr(uc_fw->type)); } else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { - DRM_NOTE("uC firmware version %d.%d, required %d.%d\n", + DRM_NOTE("%s firmware version %d.%d, required %d.%d\n", + intel_uc_fw_type_repr(uc_fw->type), uc_fw->major_ver_found, uc_fw->minor_ver_found, uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted); err = -ENOEXEC; diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index f524387..7139e31 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -125,6 +125,20 @@ enum intel_uc_fw_type { INTEL_UC_FW_TYPE_HUC }; +/* User-friendly representation of an enum */ +static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type) +{ + switch (type) { + case INTEL_UC_FW_TYPE_GUC: + return "GuC"; + case INTEL_UC_FW_TYPE_HUC: + return "HuC"; + default: + MISSING_CASE(type); + return "<invalid>"; + } +} + /* * This structure encapsulates all the data needed during the process * of fetching, caching, and loading the firmware image into the GuC.