Re: [PATCH 10/15] drm/i915: Enable GuC firmware log

Tvrtko Ursulin
2015-06-16T12:29:25Z

On 06/16/2015 12:40 PM, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 10:26:40AM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/15/2015 07:36 PM, Dave Gordon wrote:
>>> From: Alex Dai <yu.dai@intel.com>
>>>
>>> Allocate a GEM object to hold GuC log data. A debugfs interface
>>> (i915_guc_log_dump) is provided to print out the log content.
>>>
>>> Issue: VIZ-4884
>>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++++++++++
>>> drivers/gpu/drm/i915/i915_guc_submission.c | 43 ++++++++++++++++++++++++++++
>>> 2 files changed, 72 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index c52a745..b0aa4af 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2388,6 +2388,34 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>>> return 0;
>>> }
>>>
>>> +static int i915_guc_log_dump(struct seq_file *m, void *data)
>>> +{
>>> + struct drm_info_node *node = m->private;
>>> + struct drm_device *dev = node->minor->dev;
>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>> + struct drm_i915_gem_object *log_obj = dev_priv->guc.log_obj;
>>> + u32 *log;
>>> + int i = 0, pg;
>>> +
>>> + if (!log_obj)
>>> + return 0;
>>> +
>>> + for (pg = 0; pg < log_obj->base.size / PAGE_SIZE; pg++) {
>>> + log = kmap_atomic(i915_gem_object_get_page(log_obj, pg));
>>> +
>>> + for (i = 0; i < PAGE_SIZE / sizeof(u32); i += 4)
>>> + seq_printf(m, "0x%08x 0x%08x 0x%08x 0x%08x\n",
>>> + *(log + i), *(log + i + 1),
>>> + *(log + i + 2), *(log + i + 3));
>>> +
>>> + kunmap_atomic(log);
>>> + }
>>
>> This doesn't look performance critical, but you could also use
>> sg_miter_ family of functions/macros to iterate and kmap sg list
>> pages. I did not bother figuring out what kind of smarts
>> i915_gem_object_get_page does, but it is not likely it can beat
>> sg_miter_ for efficiency.
>
> It does. I have patches to replace more uses of sg_page_iter because it
> is the slow point in many functions.

Oh wow, so a "3rd party" random access iterator, used in sequential mode
over a naturally sequential data structure, beats the native sequential
iterator for performance? Amazing. :)

Regards,

Tvrtko