| Message ID | 1515082914-4111-5-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > guc_log_level parameter takes effect when GuC is loaded which is > controlled through enable_guc parameter. Add this relation info. ^^^ Extra "." > in parameter description and documentation. > Earlier, this patch was added to sanitize guc_log_level like old > GuC parameters enable_guc_loading/submission. With new parameter > enable_guc, sanitization of guc_log_level is no more needed. Hmm, I think we still need to sanitize log_level if it was wrongly enabled without enabling GuC first (in intel_uc_sanitize_options). > > v2: Added documentation to intel_guc_log.c and param description > about GuC loading dependency. (Michal Wajdeczko) > > v3: Removed sanitization of module parameter guc_log_level. > Previous review comments not applicable now. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2 > --- > drivers/gpu/drm/i915/i915_params.c | 3 ++- > drivers/gpu/drm/i915/intel_guc_log.c | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index b5f3eb4..a93a6ca 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = { > "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)"); > i915_param_named(guc_log_level, int, 0400, > - "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); > + "GuC firmware logging level. This takes effect only if GuC is to be " > + "loaded (depends on enable_guc) (-1:disabled (default), 0-3:enabled)"); Btw, I was planing to change above values to follow schema used in other modparams: -1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC) 0: disabled 1: enabled (legacy level 0) 2: enabled (legacy level 1) 3: enabled (legacy level 2) 4: enabled (legacy level 3) So now I'm not sure that I want your patch ;) > i915_param_named_unsafe(guc_firmware_path, charp, 0400, > "GuC firmware path to use instead of the default one"); > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c > b/drivers/gpu/drm/i915/intel_guc_log.c > index 59a9021..d0131bc 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -34,6 +34,7 @@ > * DOC: GuC firmware log > * > * Firmware log is enabled by setting i915.guc_log_level to > non-negative level. > + * This takes effect only if GuC is to be loaded based on enable_guc. ... based on i915.enable_guc modparam. > * Log data is printed out via reading debugfs i915_guc_log_dump. > Reading from > * i915_guc_load_status will print out firmware loading status and > scratch > * registers value.
On 1/4/2018 10:22 PM, Michal Wajdeczko wrote: > On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> guc_log_level parameter takes effect when GuC is loaded which is >> controlled through enable_guc parameter. Add this relation info. > ^^^ > Extra "." > >> in parameter description and documentation. >> Earlier, this patch was added to sanitize guc_log_level like old >> GuC parameters enable_guc_loading/submission. With new parameter >> enable_guc, sanitization of guc_log_level is no more needed. > > Hmm, I think we still need to sanitize log_level if it was wrongly > enabled without enabling GuC first (in intel_uc_sanitize_options). I think it would not be harmful as all decisions based on it will be gated by USES_GUC. > >> >> v2: Added documentation to intel_guc_log.c and param description >> about GuC loading dependency. (Michal Wajdeczko) >> >> v3: Removed sanitization of module parameter guc_log_level. >> Previous review comments not applicable now. >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2 >> --- >> drivers/gpu/drm/i915/i915_params.c | 3 ++- >> drivers/gpu/drm/i915/intel_guc_log.c | 1 + >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_params.c >> b/drivers/gpu/drm/i915/i915_params.c >> index b5f3eb4..a93a6ca 100644 >> --- a/drivers/gpu/drm/i915/i915_params.c >> +++ b/drivers/gpu/drm/i915/i915_params.c >> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = { >> "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)"); >> i915_param_named(guc_log_level, int, 0400, >> - "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); >> + "GuC firmware logging level. This takes effect only if GuC is to >> be " >> + "loaded (depends on enable_guc) (-1:disabled (default), >> 0-3:enabled)"); > > Btw, I was planing to change above values to follow schema used in > other modparams: > > -1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC) > 0: disabled > 1: enabled (legacy level 0) > 2: enabled (legacy level 1) > 3: enabled (legacy level 2) > 4: enabled (legacy level 3) > > So now I'm not sure that I want your patch ;) > Makes sense. Will drop this patch. Thanks Sagar >> i915_param_named_unsafe(guc_firmware_path, charp, 0400, >> "GuC firmware path to use instead of the default one"); >> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c >> b/drivers/gpu/drm/i915/intel_guc_log.c >> index 59a9021..d0131bc 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >> @@ -34,6 +34,7 @@ >> * DOC: GuC firmware log >> * >> * Firmware log is enabled by setting i915.guc_log_level to >> non-negative level. >> + * This takes effect only if GuC is to be loaded based on enable_guc. > > ... based on i915.enable_guc modparam. > >> * Log data is printed out via reading debugfs i915_guc_log_dump. >> Reading from >> * i915_guc_load_status will print out firmware loading status and >> scratch >> * registers value.
On 1/5/2018 10:24 AM, Sagar Arun Kamble wrote: > > > On 1/4/2018 10:22 PM, Michal Wajdeczko wrote: >> On Thu, 04 Jan 2018 17:21:46 +0100, Sagar Arun Kamble >> <sagar.a.kamble@intel.com> wrote: >> >>> guc_log_level parameter takes effect when GuC is loaded which is >>> controlled through enable_guc parameter. Add this relation info. >> ^^^ >> Extra "." >> >>> in parameter description and documentation. >>> Earlier, this patch was added to sanitize guc_log_level like old >>> GuC parameters enable_guc_loading/submission. With new parameter >>> enable_guc, sanitization of guc_log_level is no more needed. >> >> Hmm, I think we still need to sanitize log_level if it was wrongly >> enabled without enabling GuC first (in intel_uc_sanitize_options). > I think it would not be harmful as all decisions based on it will be > gated by USES_GUC. I was wrong. i915_guc_log_register/unregister were changed by my series to only rely on guc_log_level. I have added HAS_GUC check in those function in v4 patch. Is that option okay or we should sanitize this parameter? >> >>> >>> v2: Added documentation to intel_guc_log.c and param description >>> about GuC loading dependency. (Michal Wajdeczko) >>> >>> v3: Removed sanitization of module parameter guc_log_level. >>> Previous review comments not applicable now. >>> >>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2 >>> --- >>> drivers/gpu/drm/i915/i915_params.c | 3 ++- >>> drivers/gpu/drm/i915/intel_guc_log.c | 1 + >>> 2 files changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_params.c >>> b/drivers/gpu/drm/i915/i915_params.c >>> index b5f3eb4..a93a6ca 100644 >>> --- a/drivers/gpu/drm/i915/i915_params.c >>> +++ b/drivers/gpu/drm/i915/i915_params.c >>> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = { >>> "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)"); >>> i915_param_named(guc_log_level, int, 0400, >>> - "GuC firmware logging level (-1:disabled (default), >>> 0-3:enabled)"); >>> + "GuC firmware logging level. This takes effect only if GuC is >>> to be " >>> + "loaded (depends on enable_guc) (-1:disabled (default), >>> 0-3:enabled)"); >> >> Btw, I was planing to change above values to follow schema used in >> other modparams: >> >> -1: auto (then it can be controlled by USES_GUC and DRM_I915_DEBUG_GUC) >> 0: disabled >> 1: enabled (legacy level 0) >> 2: enabled (legacy level 1) >> 3: enabled (legacy level 2) >> 4: enabled (legacy level 3) >> >> So now I'm not sure that I want your patch ;) >> > Makes sense. Will drop this patch. > > Thanks > Sagar >>> i915_param_named_unsafe(guc_firmware_path, charp, 0400, >>> "GuC firmware path to use instead of the default one"); >>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c >>> b/drivers/gpu/drm/i915/intel_guc_log.c >>> index 59a9021..d0131bc 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_log.c >>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >>> @@ -34,6 +34,7 @@ >>> * DOC: GuC firmware log >>> * >>> * Firmware log is enabled by setting i915.guc_log_level to >>> non-negative level. >>> + * This takes effect only if GuC is to be loaded based on enable_guc. >> >> ... based on i915.enable_guc modparam. >> >>> * Log data is printed out via reading debugfs i915_guc_log_dump. >>> Reading from >>> * i915_guc_load_status will print out firmware loading status and >>> scratch >>> * registers value. >
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b5f3eb4..a93a6ca 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = { "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)"); i915_param_named(guc_log_level, int, 0400, - "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); + "GuC firmware logging level. This takes effect only if GuC is to be " + "loaded (depends on enable_guc) (-1:disabled (default), 0-3:enabled)"); i915_param_named_unsafe(guc_firmware_path, charp, 0400, "GuC firmware path to use instead of the default one"); diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 59a9021..d0131bc 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -34,6 +34,7 @@ * DOC: GuC firmware log * * Firmware log is enabled by setting i915.guc_log_level to non-negative level. + * This takes effect only if GuC is to be loaded based on enable_guc. * Log data is printed out via reading debugfs i915_guc_log_dump. Reading from * i915_guc_load_status will print out firmware loading status and scratch * registers value.