| Message ID | 1468158084-22028-2-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/07/16 14:41, akash.goel@intel.com wrote: > From: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > GuC Log buffer allocation was tied up with verbosity level module param > i915.guc_log_level. User would be given a provision to enable firmware > logging at runtime, through a host2guc action, and not necessarily during > Driver load time. But the address of log buffer can be passed only in > init params, at firmware load time, so GuC has to be reset and firmware > needs to be reloaded to pass the log buffer address at runtime. > To avoid reset of GuC & reload of firmware, allocation of log buffer will > be done always but logging would be enabled initially on GuC side based on > the value of module paramter guc_log_level. > > v2: Update commit message to describe the constraint with allocation of > log buffer at runtime. (Tvrtko) > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Signed-off-by: Akash Goel <akash.goel@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 3 --- > drivers/gpu/drm/i915/intel_guc_loader.c | 8 +++++--- > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 2112e02..8a9a0cb 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc) > unsigned long offset; > uint32_t size, flags; > > - if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN) > - return; > - > if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX) > i915.guc_log_level = GUC_LOG_VERBOSITY_MAX; > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 605c696..b211bd0 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -175,11 +175,13 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv) > params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER | > GUC_CTL_VCS2_ENABLED; > > - if (i915.guc_log_level >= 0) { > - params[GUC_CTL_LOG_PARAMS] = guc->log_flags; > + params[GUC_CTL_LOG_PARAMS] = guc->log_flags; guc->log_flags will be zero when logging is not configured because guc is a part of dev_priv. So it looks safe - although I reckon it would be clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else below? > + > + if (i915.guc_log_level >= 0) > params[GUC_CTL_DEBUG] = > i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT; > - } > + else > + params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; I also wonder how come GUC_LOG_DISABLED isn't set today when i915.guc_log_level == -1, given that: #define GUC_LOG_DISABLED (1 << 6) Is that bit set by default somehow if i915 does not program it? > > if (guc->ads_obj) { > u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj) > Regards, Tvrtko
On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote: > > On 10/07/16 14:41, akash.goel@intel.com wrote: >> From: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index 2112e02..8a9a0cb 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc) >> unsigned long offset; >> uint32_t size, flags; >> >> - if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN) >> - return; >> - >> if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX) >> i915.guc_log_level = GUC_LOG_VERBOSITY_MAX; >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> b/drivers/gpu/drm/i915/intel_guc_loader.c >> index 605c696..b211bd0 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -175,11 +175,13 @@ static void set_guc_init_params(struct >> drm_i915_private *dev_priv) >> params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER | >> GUC_CTL_VCS2_ENABLED; >> >> - if (i915.guc_log_level >= 0) { >> - params[GUC_CTL_LOG_PARAMS] = guc->log_flags; >> + params[GUC_CTL_LOG_PARAMS] = guc->log_flags; > > guc->log_flags will be zero when logging is not configured because guc > is a part of dev_priv. So it looks safe - although I reckon it would be > clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else > below? If logging is not enabled at (due to guc_log_level < 0), then also log_flags needs to be setup & passed to GuC firmware. log_flags shall not be zero even when logging is not be enabled (at boot time). Actually log_flags will also contain the address of the log buffer. > >> + >> + if (i915.guc_log_level >= 0) >> params[GUC_CTL_DEBUG] = >> i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT; >> - } >> + else >> + params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; > > I also wonder how come GUC_LOG_DISABLED isn't set today when > i915.guc_log_level == -1, given that: > > #define GUC_LOG_DISABLED (1 << 6) > > Is that bit set by default somehow if i915 does not program it? > Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1. But then log buffer address will go as NULL and GUC_LOG_VALID flag will go as 0, for guc_log_level = -1. So this way logging on GuC side will not get enabled. I hope I understood your concern correctly. Best regards Akash >> >> if (guc->ads_obj) { >> u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj) >> > > Regards, > > Tvrtko >
On 11/07/16 12:41, Goel, Akash wrote: > On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote: >> >> On 10/07/16 14:41, akash.goel@intel.com wrote: >>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> >>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>> index 2112e02..8a9a0cb 100644 >>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>> @@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc) >>> unsigned long offset; >>> uint32_t size, flags; >>> >>> - if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN) >>> - return; >>> - >>> if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX) >>> i915.guc_log_level = GUC_LOG_VERBOSITY_MAX; >>> >>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >>> b/drivers/gpu/drm/i915/intel_guc_loader.c >>> index 605c696..b211bd0 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >>> @@ -175,11 +175,13 @@ static void set_guc_init_params(struct >>> drm_i915_private *dev_priv) >>> params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER | >>> GUC_CTL_VCS2_ENABLED; >>> >>> - if (i915.guc_log_level >= 0) { >>> - params[GUC_CTL_LOG_PARAMS] = guc->log_flags; >>> + params[GUC_CTL_LOG_PARAMS] = guc->log_flags; >> >> guc->log_flags will be zero when logging is not configured because guc >> is a part of dev_priv. So it looks safe - although I reckon it would be >> clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else >> below? > > If logging is not enabled at (due to guc_log_level < 0), then also > log_flags needs to be setup & passed to GuC firmware. > log_flags shall not be zero even when logging is not be enabled (at boot > time). > Actually log_flags will also contain the address of the log buffer. Ah yes, I got confused by jumping between one file with your patch applied and one without it. >>> + >>> + if (i915.guc_log_level >= 0) >>> params[GUC_CTL_DEBUG] = >>> i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT; >>> - } >>> + else >>> + params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; >> >> I also wonder how come GUC_LOG_DISABLED isn't set today when >> i915.guc_log_level == -1, given that: >> >> #define GUC_LOG_DISABLED (1 << 6) >> >> Is that bit set by default somehow if i915 does not program it? >> > > Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1. > But then log buffer address will go as NULL and GUC_LOG_VALID flag will > go as 0, for guc_log_level = -1. So this way logging on GuC side will > not get enabled. > I hope I understood your concern correctly. Yes, this clarifies it. Although I do have one more question then - what happens if at boot i915.guc_log_level == -1 and then with later patches logging gets enabled via debugfs - who and how sets params[GUC_CTL_DEBUG]? Host2GuC overrides this parameter? Regards, Tvrtko
On 7/11/2016 5:20 PM, Tvrtko Ursulin wrote: > > On 11/07/16 12:41, Goel, Akash wrote: >> On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote: >>> >>> On 10/07/16 14:41, akash.goel@intel.com wrote: >>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>>> >>>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>>> index 2112e02..8a9a0cb 100644 >>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>> >>>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >>>> b/drivers/gpu/drm/i915/intel_guc_loader.c >>>> index 605c696..b211bd0 100644 >>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >>>> @@ -175,11 +175,13 @@ static void set_guc_init_params(struct >>>> drm_i915_private *dev_priv) >>>> params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER | >>>> GUC_CTL_VCS2_ENABLED; >>>> >>>> - if (i915.guc_log_level >= 0) { >>>> - params[GUC_CTL_LOG_PARAMS] = guc->log_flags; >>>> + params[GUC_CTL_LOG_PARAMS] = guc->log_flags; >>> >>> guc->log_flags will be zero when logging is not configured because guc >>> is a part of dev_priv. So it looks safe - although I reckon it would be >>> clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else >>> below? >> >> If logging is not enabled at (due to guc_log_level < 0), then also >> log_flags needs to be setup & passed to GuC firmware. >> log_flags shall not be zero even when logging is not be enabled (at boot >> time). >> Actually log_flags will also contain the address of the log buffer. > > Ah yes, I got confused by jumping between one file with your patch > applied and one without it. > >>>> + >>>> + if (i915.guc_log_level >= 0) >>>> params[GUC_CTL_DEBUG] = >>>> i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT; >>>> - } >>>> + else >>>> + params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; >>> >>> I also wonder how come GUC_LOG_DISABLED isn't set today when >>> i915.guc_log_level == -1, given that: >>> >>> #define GUC_LOG_DISABLED (1 << 6) >>> >>> Is that bit set by default somehow if i915 does not program it? >>> >> >> Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1. >> But then log buffer address will go as NULL and GUC_LOG_VALID flag will >> go as 0, for guc_log_level = -1. So this way logging on GuC side will >> not get enabled. >> I hope I understood your concern correctly. > > Yes, this clarifies it. Although I do have one more question then - what > happens if at boot i915.guc_log_level == -1 and then with later patches > logging gets enabled via debugfs - who and how sets > params[GUC_CTL_DEBUG]? Host2GuC overrides this parameter? > Yes through Host2GuC action type, UK_LOG_ENABLE_LOGGING, Host will request GuC firmware to enable/disable logging and alter the verbosity level. The params[GUC_CTL_DEBUG] is just part of the firmware initialization parameters and is not used after that. Best regards Akash > Regards, > > Tvrtko > >
On 11/07/16 13:11, Goel, Akash wrote: > > > On 7/11/2016 5:20 PM, Tvrtko Ursulin wrote: >> >> On 11/07/16 12:41, Goel, Akash wrote: >>> On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote: >>>> >>>> On 10/07/16 14:41, akash.goel@intel.com wrote: >>>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>>>> >>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>>>> index 2112e02..8a9a0cb 100644 >>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >>>>> b/drivers/gpu/drm/i915/intel_guc_loader.c >>>>> index 605c696..b211bd0 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >>>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >>>>> @@ -175,11 +175,13 @@ static void set_guc_init_params(struct >>>>> drm_i915_private *dev_priv) >>>>> params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER | >>>>> GUC_CTL_VCS2_ENABLED; >>>>> >>>>> - if (i915.guc_log_level >= 0) { >>>>> - params[GUC_CTL_LOG_PARAMS] = guc->log_flags; >>>>> + params[GUC_CTL_LOG_PARAMS] = guc->log_flags; >>>> >>>> guc->log_flags will be zero when logging is not configured because guc >>>> is a part of dev_priv. So it looks safe - although I reckon it would be >>>> clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else >>>> below? >>> >>> If logging is not enabled at (due to guc_log_level < 0), then also >>> log_flags needs to be setup & passed to GuC firmware. >>> log_flags shall not be zero even when logging is not be enabled (at boot >>> time). >>> Actually log_flags will also contain the address of the log buffer. >> >> Ah yes, I got confused by jumping between one file with your patch >> applied and one without it. >> >>>>> + >>>>> + if (i915.guc_log_level >= 0) >>>>> params[GUC_CTL_DEBUG] = >>>>> i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT; >>>>> - } >>>>> + else >>>>> + params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; >>>> >>>> I also wonder how come GUC_LOG_DISABLED isn't set today when >>>> i915.guc_log_level == -1, given that: >>>> >>>> #define GUC_LOG_DISABLED (1 << 6) >>>> >>>> Is that bit set by default somehow if i915 does not program it? >>>> >>> >>> Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1. >>> But then log buffer address will go as NULL and GUC_LOG_VALID flag will >>> go as 0, for guc_log_level = -1. So this way logging on GuC side will >>> not get enabled. >>> I hope I understood your concern correctly. >> >> Yes, this clarifies it. Although I do have one more question then - what >> happens if at boot i915.guc_log_level == -1 and then with later patches >> logging gets enabled via debugfs - who and how sets >> params[GUC_CTL_DEBUG]? Host2GuC overrides this parameter? >> > > Yes through Host2GuC action type, UK_LOG_ENABLE_LOGGING, Host will > request GuC firmware to enable/disable logging and alter the verbosity > level. > > The params[GUC_CTL_DEBUG] is just part of the firmware initialization > parameters and is not used after that. Got it now, in that case: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 2112e02..8a9a0cb 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc) unsigned long offset; uint32_t size, flags; - if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN) - return; - if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX) i915.guc_log_level = GUC_LOG_VERBOSITY_MAX; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 605c696..b211bd0 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -175,11 +175,13 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv) params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER | GUC_CTL_VCS2_ENABLED; - if (i915.guc_log_level >= 0) { - params[GUC_CTL_LOG_PARAMS] = guc->log_flags; + params[GUC_CTL_LOG_PARAMS] = guc->log_flags; + + if (i915.guc_log_level >= 0) params[GUC_CTL_DEBUG] = i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT; - } + else + params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; if (guc->ads_obj) { u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)