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

[Goel, Akash]

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 at intel.com wrote:
>>>> From: Sagar Arun Kamble <sagar.a.kamble at 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 at intel.com>
>>>> Signed-off-by: Akash Goel <akash.goel at 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