[Intel-gfx] [PATCH v5 04/10] drm/i915/perf: implement active wait for noa configurations

[Lionel Landwerlin]

On 27/06/2019 11:27, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-06-27 09:00:39)
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 2f1dc9be3bfb..b2f5ba87921c 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -410,6 +410,8 @@ static int alloc_oa_config_buffer(struct drm_i915_private *i915,
>>          size_t config_length = 0;
>>          u32 *cs;
>>   
>> +       lockdep_assert_held(&i915->drm.struct_mutex);
> What inside *this* function depends on the struct_mutex? The noa_wait
> vma is already pinned and I didn't spot anything else added that might
> be contentious.


Thanks, will drop this.


>
>>          if (oa_config->mux_regs_len > 0) {
>>                  config_length += DIV_ROUND_UP(oa_config->mux_regs_len,
>>                                                MI_LOAD_REGISTER_IMM_MAX_REGS) * 4;
>> @@ -425,7 +427,7 @@ static int alloc_oa_config_buffer(struct drm_i915_private *i915,
>>                                                MI_LOAD_REGISTER_IMM_MAX_REGS) * 4;
>>                  config_length += oa_config->flex_regs_len * 8;
>>          }
>> -       config_length += 4; /* MI_BATCH_BUFFER_END */
>> +       config_length += 12; /* MI_BATCH_BUFFER_START into noa_wait loop */
>>          config_length = ALIGN(config_length, I915_GTT_PAGE_SIZE);
>>   
>>          bo = i915_gem_object_create_shmem(i915, config_length);
>> @@ -442,7 +444,12 @@ static int alloc_oa_config_buffer(struct drm_i915_private *i915,
>>          cs = write_cs_mi_lri(cs, oa_config->b_counter_regs, oa_config->b_counter_regs_len);
>>          cs = write_cs_mi_lri(cs, oa_config->flex_regs, oa_config->flex_regs_len);
>>   
>> -       *cs++ = MI_BATCH_BUFFER_END;
>> +
>> +       /* Jump into the NOA wait busy loop. */
>> +       *cs++ = (INTEL_GEN(i915) < 8 ?
>> +                MI_BATCH_BUFFER_START : MI_BATCH_BUFFER_START_GEN8);
>> +       *cs++ = i915->perf.oa.noa_wait->node.start;
> i915_ggtt_offset(i915->perf.oa.noa_wait);
> double checks that you are in the ggtt and so assuming u32 is valid.


Right, will do.


>
> This is operating inside the user context right? (If I remember
> correctly, you plug into the user dispatch) If so, where do you save and
> restore the CS_GPR overwritten?


Duh! In a previous iteration I was saving this in the scratch BO as a 
given location.

Should I alloc my own buffer?


Thanks a lot,


-Lionel


> -Chris
>