[Intel-gfx] [PATCH 15/17] drm/i915: Increase GuC log buffer size to reduce flush interrupts

[Tvrtko Ursulin]

On 18/07/16 13:35, Goel, Akash wrote:
>
>
> On 7/18/2016 3:24 PM, Tvrtko Ursulin wrote:
>>
>> On 15/07/16 17:20, Goel, Akash wrote:
>>> On 7/15/2016 8:37 PM, Tvrtko Ursulin wrote:
>>>> On 15/07/16 15:42, Goel, Akash wrote:
>>>>> On 7/15/2016 5:27 PM, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 10/07/16 14:41, akash.goel at intel.com wrote:
>>>>>>> From: Akash Goel <akash.goel at intel.com>
>>>>>>>
>>>>>>> In cases where GuC generate logs at a very high rate,
>>>>>>> correspondingly
>>>>>>> the rate of flush interrupts is also very high.
>>>>>>> So far total 8 pages were allocated for storing both ISR & DPC logs.
>>>>>>> As per the half-full draining protocol followed by GuC, by doubling
>>>>>>> the number of pages, the frequency of flush interrupts can be cut
>>>>>>> down
>>>>>>> to almost half, which then helps in reducing the logging overhead.
>>>>>>> So now allocating 8 pages apiece for ISR & DPC logs.
>>>>>>>
>>>>>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>>> Signed-off-by: Akash Goel <akash.goel at intel.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/i915/intel_guc_fwif.h | 8 ++++----
>>>>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h
>>>>>>> b/drivers/gpu/drm/i915/intel_guc_fwif.h
>>>>>>> index 1de6928..7521ed5 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
>>>>>>> @@ -104,9 +104,9 @@
>>>>>>>   #define   GUC_LOG_ALLOC_IN_MEGABYTE    (1 << 3)
>>>>>>>   #define   GUC_LOG_CRASH_PAGES        1
>>>>>>>   #define   GUC_LOG_CRASH_SHIFT        4
>>>>>>> -#define   GUC_LOG_DPC_PAGES        3
>>>>>>> +#define   GUC_LOG_DPC_PAGES        7
>>>>>>>   #define   GUC_LOG_DPC_SHIFT        6
>>>>>>> -#define   GUC_LOG_ISR_PAGES        3
>>>>>>> +#define   GUC_LOG_ISR_PAGES        7
>>>>>>>   #define   GUC_LOG_ISR_SHIFT        9
>>>>>>>   #define   GUC_LOG_BUF_ADDR_SHIFT    12
>>>>>>>
>>>>>>> @@ -436,9 +436,9 @@ enum guc_log_buffer_type {
>>>>>>>    *        |   Crash dump state header     |
>>>>>>>    * Page1  +-------------------------------+
>>>>>>>    *        |           ISR logs            |
>>>>>>> - * Page5  +-------------------------------+
>>>>>>> - *        |           DPC logs            |
>>>>>>>    * Page9  +-------------------------------+
>>>>>>> + *        |           DPC logs            |
>>>>>>> + * Page17 +-------------------------------+
>>>>>>>    *        |         Crash Dump logs       |
>>>>>>>    *        +-------------------------------+
>>>>>>>    *
>>>>>>>
>>>>>>
>>>>>> I don't mind - but does it help? And how much and for what? Haven't
>>>>>> you
>>>>>> later found that the uncached reads were the main issue?
>>>>> This change along with kthread patch, helped reduce the overflow
>>>>> counts
>>>>> and even eliminate them for some benchmarks.
>>>>>
>>>>> Though with the impending optimization for Uncached reads there should
>>>>> be further improvements but in my view, notwithstanding the
>>>>> improvement
>>>>> w.r.t overflow count, its still a better configuration to work with as
>>>>> flush interrupt frequency is cut down to half and not able to see any
>>>>> apparent downsides to it.
>>>>
>>>> I was primarily thinking to go with a minimal and simplest set of
>>>> patches to implement the feature.
>>>>
>>> I second that and working with the same intent.
>>>
>>>> Logic was that apparently none of the smart and complex optimisations
>>>> managed to solve the dropped interrupt issue, until the slowness of the
>>>> uncached read was discovered to be the real/main issue.
>>>>
>>>> So it seems that is something that definitely needs to be implemented.
>>>> (Whether or not it will be possible to use SSE instructions to do the
>>>> read I don't know.)
>>>>
>>>
>>> log buffer resizing and rt priority kthread changes have definitely
>>> helped significantly.
>>>
>>> Only of late we realized that there is a potential way to speed up
>>> Uncached reads also. Moreover I am yet to test that on kernel side.
>>> So until that is tested & proves to be enough, we have to rely on the
>>> other optimizations & can't dismiss them
>>
>> Maybe, depends if, what I thought was the case, none of the other
>> optimizations actually enabled a drop-free logging in all interesting
>> scenarios.
>>
>> If we conclude that simply improving the copy speed removes the need for
>> any other optimisations and complications, we can talk about whether
>> every individual one of those still makes sense.
>>
> In my opinion we should keep this change, regardless of the copying
> speed up. Moreover this is a straight forward change.
>
> Actually this also helps in reducing the output log file size, apart
> from reducing the flush interrupt count.
> With the original settings, 44 KB was needed for one snapshot.
> With the modified settings, 76 KB is needed for one snapshot but it
> will be equivalent to 2 snapshots of the original setting.
> So 12KB saving, every 88 KB, over the original setting.

That is indeed a good benefit. Did not realize there is this space 
wastage problem.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko