Re: [PATCH 01/11] drm/i915: Decouple GuC log setup from verbosity parameter

Goel, Akash
2016-06-27T16:26:00Z

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