| Message ID | 1467029818-3417-2-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/06/16 13:16, akash.goel@intel.com wrote: > From: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > GuC Log buffer allocation was tied up with verbosity level kernel parameter > i915.guc_log_level. User could be given a provision to enable logging at > runtime and not necessarily during load time only. This patch will perform > allocation of shared log buffer always but will initially enable logging on > GuC side through init params based on i915.guc_log_level. > > 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 355b647..28a810f 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -826,9 +826,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 8fe96a2..db3c897 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -173,11 +173,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) > I did not manage to understand what is the benefit of always allocating the log buffer? If the user never enables logging it just wasted 11 pages of memory, correct? Looking at the later patches in the series, could you instead create the log buffer when logging is enabled via debugfs or implicitly via the relayfs access? Or is the problem then that you would then have to reset the GuC to activate it? Regards, Tvrtko
On 6/27/2016 8:30 PM, Tvrtko Ursulin wrote: > > On 27/06/16 13:16, akash.goel@intel.com wrote: >> From: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> >> GuC Log buffer allocation was tied up with verbosity level kernel >> parameter >> i915.guc_log_level. User could be given a provision to enable logging at >> runtime and not necessarily during load time only. This patch will >> perform >> allocation of shared log buffer always but will initially enable >> logging on >> GuC side through init params based on i915.guc_log_level. >> >> 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 355b647..28a810f 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -826,9 +826,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 8fe96a2..db3c897 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -173,11 +173,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) >> > > I did not manage to understand what is the benefit of always allocating > the log buffer? If the user never enables logging it just wasted 11 > pages of memory, correct? > Yes if User never enables the logging at runtime, 11 RAM pages will be wasted. Currently the pages are permanently pinned in GGTT also. The GGTT address of log buffer is passed in the GuC firmware init params, at firmware loading time. Probably this can be circumvented, if pages can be pinned right before enabling logging (but using the same GGTT address). > Looking at the later patches in the series, could you instead create the > log buffer when logging is enabled via debugfs or implicitly via the > relayfs access? > > Or is the problem then that you would then have to reset the GuC to > activate it? Yes GuC would have to be reset & firmware needs to be reloaded to pass the log buffer address. Best regards Akash > > Regards, > > Tvrtko
On 27/06/16 16:32, Goel, Akash wrote: > > > On 6/27/2016 8:30 PM, Tvrtko Ursulin wrote: >> >> On 27/06/16 13:16, akash.goel@intel.com wrote: >>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> >>> GuC Log buffer allocation was tied up with verbosity level kernel >>> parameter >>> i915.guc_log_level. User could be given a provision to enable logging at >>> runtime and not necessarily during load time only. This patch will >>> perform >>> allocation of shared log buffer always but will initially enable >>> logging on >>> GuC side through init params based on i915.guc_log_level. >>> >>> 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 355b647..28a810f 100644 >>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>> @@ -826,9 +826,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 8fe96a2..db3c897 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >>> @@ -173,11 +173,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) >>> >> >> I did not manage to understand what is the benefit of always allocating >> the log buffer? If the user never enables logging it just wasted 11 >> pages of memory, correct? >> > Yes if User never enables the logging at runtime, 11 RAM pages will be > wasted. > > Currently the pages are permanently pinned in GGTT also. > The GGTT address of log buffer is passed in the GuC firmware init > params, at firmware loading time. > > Probably this can be circumvented, if pages can be pinned right before > enabling logging (but using the same GGTT address). > >> Looking at the later patches in the series, could you instead create the >> log buffer when logging is enabled via debugfs or implicitly via the >> relayfs access? >> >> Or is the problem then that you would then have to reset the GuC to >> activate it? > > Yes GuC would have to be reset & firmware needs to be reloaded to pass > the log buffer address. Right, as minimum I think commit message needs to explain that. The current explanation does not hold anyway since it is not possible to enable it via modifying the module parameter. Btw have you considered keeping the module param as a global GuC logging enable and adding new code on top? So keep the current code to only allocate the buffer when module param is set, and then if it isn't fail the later userspace triggered attempts to start the logging (in debugfs or relayfs)? Regards, Tvrtko
On 6/27/2016 9:26 PM, Tvrtko Ursulin wrote: > > On 27/06/16 16:32, Goel, Akash wrote: >> >> >> On 6/27/2016 8:30 PM, Tvrtko Ursulin wrote: >>> >>> On 27/06/16 13:16, akash.goel@intel.com wrote: >>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>>> >>>> GuC Log buffer allocation was tied up with verbosity level kernel >>>> parameter >>>> i915.guc_log_level. User could be given a provision to enable >>>> logging at >>>> runtime and not necessarily during load time only. This patch will >>>> perform >>>> allocation of shared log buffer always but will initially enable >>>> logging on >>>> GuC side through init params based on i915.guc_log_level. >>>> >>>> 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 355b647..28a810f 100644 >>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>>> @@ -826,9 +826,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 8fe96a2..db3c897 100644 >>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >>>> @@ -173,11 +173,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) >>>> >>> >>> I did not manage to understand what is the benefit of always allocating >>> the log buffer? If the user never enables logging it just wasted 11 >>> pages of memory, correct? >>> >> Yes if User never enables the logging at runtime, 11 RAM pages will be >> wasted. >> >> Currently the pages are permanently pinned in GGTT also. >> The GGTT address of log buffer is passed in the GuC firmware init >> params, at firmware loading time. >> >> Probably this can be circumvented, if pages can be pinned right before >> enabling logging (but using the same GGTT address). >> >>> Looking at the later patches in the series, could you instead create the >>> log buffer when logging is enabled via debugfs or implicitly via the >>> relayfs access? >>> >>> Or is the problem then that you would then have to reset the GuC to >>> activate it? >> >> Yes GuC would have to be reset & firmware needs to be reloaded to pass >> the log buffer address. > > Right, as minimum I think commit message needs to explain that. The > current explanation does not hold anyway since it is not possible to > enable it via modifying the module parameter. Right, there should have been an explanation citing the constraint in late allocation of log buffer when logging is enabled. Sorry for missing. > > Btw have you considered keeping the module param as a global GuC logging > enable and adding new code on top? So keep the current code to only > allocate the buffer when module param is set, and then if it isn't fail > the later userspace triggered attempts to start the logging (in debugfs > or relayfs)? Yes that was considered, keeping module param as the master control and allowing disable/enable of logging at runtime (through debugfs) only when module param is set at boot time. IIRC there was a request from Validation to keep logging control independent of boot time value of module param. So even if system booted with guc_log_level as -1, still allow the logging to be enabled at runtime later, through a debugfs interface 'i915_guc_log_control'. Best regards Akash > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 355b647..28a810f 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -826,9 +826,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 8fe96a2..db3c897 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -173,11 +173,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)