| Message ID | 1508309222-26406-5-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/10/2017 07:46, Sagar Arun Kamble wrote: > Parameter guc_log_level needs to be sanitized based on GuC support and > enable_guc_loading parameter since it depends on them like > enable_guc_submission. This will make GuC logging paths independent of > enable_guc_submission parameter in further patches. > > v2: Added documentation to intel_guc_log.c and param description about > GuC loading dependency. (Michal Wajdeczko) > > 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> > --- > drivers/gpu/drm/i915/i915_params.c | 4 +++- > drivers/gpu/drm/i915/intel_guc_log.c | 1 + > drivers/gpu/drm/i915/intel_uc.c | 10 +++++++--- > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index b4faeb6..774c56e 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = { > "(-1=auto, 0=never [default], 1=if available, 2=required)"); > > 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_loading) (-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 f53c663..200f0a1 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_loading. > * 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/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 62738ad..8feefcd 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) > { > if (!HAS_GUC(dev_priv)) { > if (i915_modparams.enable_guc_loading > 0 || > - i915_modparams.enable_guc_submission > 0) > + i915_modparams.enable_guc_submission > 0 || > + i915_modparams.guc_log_level > 0) > DRM_INFO("Ignoring GuC options, no hardware\n"); Hm, this won't fire on <=gen8 once enable_guc_submission starts to default to one? Or we can worry about that when we get there... > > i915_modparams.enable_guc_loading = 0; > i915_modparams.enable_guc_submission = 0; > + i915_modparams.guc_log_level = -1; > return; > } > > @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) > i915_modparams.enable_guc_loading = 0; > } > > - /* Can't enable guc submission without guc loaded */ > - if (!i915_modparams.enable_guc_loading) > + /* Can't enable guc submission and logging without guc loaded */ > + if (!i915_modparams.enable_guc_loading) { > i915_modparams.enable_guc_submission = 0; > + i915_modparams.guc_log_level = -1; Hm2, how does this interact with the changes which are removing enable_guc_loading? Or it is a race? I was just thinking that we should probably let the user know if they asked for logging, but it cannot happen due guc loading turned off, to have analogy with the same when GuC is not available in the first place. Does that make sense? > + } > > /* A negative value means "use platform default" */ > if (i915_modparams.enable_guc_submission < 0) > But since it looks like a cleanup even without the above: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote: > > On 18/10/2017 07:46, Sagar Arun Kamble wrote: >> Parameter guc_log_level needs to be sanitized based on GuC support and >> enable_guc_loading parameter since it depends on them like >> enable_guc_submission. This will make GuC logging paths independent of >> enable_guc_submission parameter in further patches. >> >> v2: Added documentation to intel_guc_log.c and param description about >> GuC loading dependency. (Michal Wajdeczko) >> >> 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> >> --- >> drivers/gpu/drm/i915/i915_params.c | 4 +++- >> drivers/gpu/drm/i915/intel_guc_log.c | 1 + >> drivers/gpu/drm/i915/intel_uc.c | 10 +++++++--- >> 3 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_params.c >> b/drivers/gpu/drm/i915/i915_params.c >> index b4faeb6..774c56e 100644 >> --- a/drivers/gpu/drm/i915/i915_params.c >> +++ b/drivers/gpu/drm/i915/i915_params.c >> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = { >> "(-1=auto, 0=never [default], 1=if available, 2=required)"); >> 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_loading) (-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 f53c663..200f0a1 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_loading. >> * 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/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 62738ad..8feefcd 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct >> drm_i915_private *dev_priv) >> { >> if (!HAS_GUC(dev_priv)) { >> if (i915_modparams.enable_guc_loading > 0 || >> - i915_modparams.enable_guc_submission > 0) >> + i915_modparams.enable_guc_submission > 0 || >> + i915_modparams.guc_log_level > 0) >> DRM_INFO("Ignoring GuC options, no hardware\n"); > > Hm, this won't fire on <=gen8 once enable_guc_submission starts to > default to one? Or we can worry about that when we get there... This will fire for <=gen8 for +ve values which is expected. > >> i915_modparams.enable_guc_loading = 0; >> i915_modparams.enable_guc_submission = 0; >> + i915_modparams.guc_log_level = -1; >> return; >> } >> @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct >> drm_i915_private *dev_priv) >> i915_modparams.enable_guc_loading = 0; >> } >> - /* Can't enable guc submission without guc loaded */ >> - if (!i915_modparams.enable_guc_loading) >> + /* Can't enable guc submission and logging without guc loaded */ >> + if (!i915_modparams.enable_guc_loading) { >> i915_modparams.enable_guc_submission = 0; >> + i915_modparams.guc_log_level = -1; > > Hm2, how does this interact with the changes which are removing > enable_guc_loading? Or it is a race? It interacts. Will race. I think I should pause this series till the other one gets merged. > > I was just thinking that we should probably let the user know if they > asked for logging, but it cannot happen due guc loading turned off, to > have analogy with the same when GuC is not available in the first place. will change with enable_guc_loading change. > > Does that make sense? Yes :) > >> + } >> /* A negative value means "use platform default" */ >> if (i915_modparams.enable_guc_submission < 0) >> > > But since it looks like a cleanup even without the above: > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Regards, > > Tvrtko
On 18/10/2017 16:50, Sagar Arun Kamble wrote: > On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote: >> >> On 18/10/2017 07:46, Sagar Arun Kamble wrote: >>> Parameter guc_log_level needs to be sanitized based on GuC support and >>> enable_guc_loading parameter since it depends on them like >>> enable_guc_submission. This will make GuC logging paths independent of >>> enable_guc_submission parameter in further patches. >>> >>> v2: Added documentation to intel_guc_log.c and param description about >>> GuC loading dependency. (Michal Wajdeczko) >>> >>> 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> >>> --- >>> drivers/gpu/drm/i915/i915_params.c | 4 +++- >>> drivers/gpu/drm/i915/intel_guc_log.c | 1 + >>> drivers/gpu/drm/i915/intel_uc.c | 10 +++++++--- >>> 3 files changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_params.c >>> b/drivers/gpu/drm/i915/i915_params.c >>> index b4faeb6..774c56e 100644 >>> --- a/drivers/gpu/drm/i915/i915_params.c >>> +++ b/drivers/gpu/drm/i915/i915_params.c >>> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = { >>> "(-1=auto, 0=never [default], 1=if available, 2=required)"); >>> 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_loading) (-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 f53c663..200f0a1 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_loading. >>> * 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/intel_uc.c >>> b/drivers/gpu/drm/i915/intel_uc.c >>> index 62738ad..8feefcd 100644 >>> --- a/drivers/gpu/drm/i915/intel_uc.c >>> +++ b/drivers/gpu/drm/i915/intel_uc.c >>> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct >>> drm_i915_private *dev_priv) >>> { >>> if (!HAS_GUC(dev_priv)) { >>> if (i915_modparams.enable_guc_loading > 0 || >>> - i915_modparams.enable_guc_submission > 0) >>> + i915_modparams.enable_guc_submission > 0 || >>> + i915_modparams.guc_log_level > 0) >>> DRM_INFO("Ignoring GuC options, no hardware\n"); >> >> Hm, this won't fire on <=gen8 once enable_guc_submission starts to >> default to one? Or we can worry about that when we get there... > This will fire for <=gen8 for +ve values which is expected. >> >>> i915_modparams.enable_guc_loading = 0; >>> i915_modparams.enable_guc_submission = 0; >>> + i915_modparams.guc_log_level = -1; >>> return; >>> } >>> @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct >>> drm_i915_private *dev_priv) >>> i915_modparams.enable_guc_loading = 0; >>> } >>> - /* Can't enable guc submission without guc loaded */ >>> - if (!i915_modparams.enable_guc_loading) >>> + /* Can't enable guc submission and logging without guc loaded */ >>> + if (!i915_modparams.enable_guc_loading) { >>> i915_modparams.enable_guc_submission = 0; >>> + i915_modparams.guc_log_level = -1; >> >> Hm2, how does this interact with the changes which are removing >> enable_guc_loading? Or it is a race? > It interacts. Will race. I think I should pause this series till the > other one gets merged. Okay, it's your call (as in you guys who are refactoring GuC heavily at the moment). Should I continue to the end of this one then? It is not that big so I just as well can. Regards, Tvrtko
On 10/19/2017 12:52 PM, Tvrtko Ursulin wrote: > > On 18/10/2017 16:50, Sagar Arun Kamble wrote: >> On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote: >>> >>> On 18/10/2017 07:46, Sagar Arun Kamble wrote: >>>> Parameter guc_log_level needs to be sanitized based on GuC support and >>>> enable_guc_loading parameter since it depends on them like >>>> enable_guc_submission. This will make GuC logging paths independent of >>>> enable_guc_submission parameter in further patches. >>>> >>>> v2: Added documentation to intel_guc_log.c and param description about >>>> GuC loading dependency. (Michal Wajdeczko) >>>> >>>> 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> >>>> --- >>>> drivers/gpu/drm/i915/i915_params.c | 4 +++- >>>> drivers/gpu/drm/i915/intel_guc_log.c | 1 + >>>> drivers/gpu/drm/i915/intel_uc.c | 10 +++++++--- >>>> 3 files changed, 11 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_params.c >>>> b/drivers/gpu/drm/i915/i915_params.c >>>> index b4faeb6..774c56e 100644 >>>> --- a/drivers/gpu/drm/i915/i915_params.c >>>> +++ b/drivers/gpu/drm/i915/i915_params.c >>>> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly >>>> = { >>>> "(-1=auto, 0=never [default], 1=if available, 2=required)"); >>>> 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_loading) (-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 f53c663..200f0a1 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_loading. >>>> * 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/intel_uc.c >>>> b/drivers/gpu/drm/i915/intel_uc.c >>>> index 62738ad..8feefcd 100644 >>>> --- a/drivers/gpu/drm/i915/intel_uc.c >>>> +++ b/drivers/gpu/drm/i915/intel_uc.c >>>> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct >>>> drm_i915_private *dev_priv) >>>> { >>>> if (!HAS_GUC(dev_priv)) { >>>> if (i915_modparams.enable_guc_loading > 0 || >>>> - i915_modparams.enable_guc_submission > 0) >>>> + i915_modparams.enable_guc_submission > 0 || >>>> + i915_modparams.guc_log_level > 0) >>>> DRM_INFO("Ignoring GuC options, no hardware\n"); >>> >>> Hm, this won't fire on <=gen8 once enable_guc_submission starts to >>> default to one? Or we can worry about that when we get there... >> This will fire for <=gen8 for +ve values which is expected. >>> >>>> i915_modparams.enable_guc_loading = 0; >>>> i915_modparams.enable_guc_submission = 0; >>>> + i915_modparams.guc_log_level = -1; >>>> return; >>>> } >>>> @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct >>>> drm_i915_private *dev_priv) >>>> i915_modparams.enable_guc_loading = 0; >>>> } >>>> - /* Can't enable guc submission without guc loaded */ >>>> - if (!i915_modparams.enable_guc_loading) >>>> + /* Can't enable guc submission and logging without guc loaded */ >>>> + if (!i915_modparams.enable_guc_loading) { >>>> i915_modparams.enable_guc_submission = 0; >>>> + i915_modparams.guc_log_level = -1; >>> >>> Hm2, how does this interact with the changes which are removing >>> enable_guc_loading? Or it is a race? >> It interacts. Will race. I think I should pause this series till the >> other one gets merged. > > Okay, it's your call (as in you guys who are refactoring GuC heavily > at the moment). > > Should I continue to the end of this one then? It is not that big so I > just as well can. Thanks for the review. Will address all the inputs and post after Sujaritha's series as there will be some conflict. I had thought of having this review in parallel as patches are largely independent but still there is some conflict so I will defer next rev. for some more time. > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b4faeb6..774c56e 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = { "(-1=auto, 0=never [default], 1=if available, 2=required)"); 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_loading) (-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 f53c663..200f0a1 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_loading. * 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/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 62738ad..8feefcd 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) { if (!HAS_GUC(dev_priv)) { if (i915_modparams.enable_guc_loading > 0 || - i915_modparams.enable_guc_submission > 0) + i915_modparams.enable_guc_submission > 0 || + i915_modparams.guc_log_level > 0) DRM_INFO("Ignoring GuC options, no hardware\n"); i915_modparams.enable_guc_loading = 0; i915_modparams.enable_guc_submission = 0; + i915_modparams.guc_log_level = -1; return; } @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) i915_modparams.enable_guc_loading = 0; } - /* Can't enable guc submission without guc loaded */ - if (!i915_modparams.enable_guc_loading) + /* Can't enable guc submission and logging without guc loaded */ + if (!i915_modparams.enable_guc_loading) { i915_modparams.enable_guc_submission = 0; + i915_modparams.guc_log_level = -1; + } /* A negative value means "use platform default" */ if (i915_modparams.enable_guc_submission < 0)
Parameter guc_log_level needs to be sanitized based on GuC support and enable_guc_loading parameter since it depends on them like enable_guc_submission. This will make GuC logging paths independent of enable_guc_submission parameter in further patches. v2: Added documentation to intel_guc_log.c and param description about GuC loading dependency. (Michal Wajdeczko) 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> --- drivers/gpu/drm/i915/i915_params.c | 4 +++- drivers/gpu/drm/i915/intel_guc_log.c | 1 + drivers/gpu/drm/i915/intel_uc.c | 10 +++++++--- 3 files changed, 11 insertions(+), 4 deletions(-)