[Mesa-dev] [PATCH v2 3/5] i965: perf: read slice/unslice frequencies from OA reports

[Lionel Landwerlin]

On 16/04/18 23:16, Kenneth Graunke wrote:
> On Tuesday, April 3, 2018 7:48:11 AM PDT Lionel Landwerlin wrote:
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> ---
>>   src/mesa/drivers/dri/i965/brw_performance_query.c | 37 +++++++++++++++++++++++
>>   src/mesa/drivers/dri/i965/brw_performance_query.h | 12 ++++++++
>>   2 files changed, 49 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_performance_query.c b/src/mesa/drivers/dri/i965/brw_performance_query.c
>> index 32cf96a333d..d20bc58ee33 100644
>> --- a/src/mesa/drivers/dri/i965/brw_performance_query.c
>> +++ b/src/mesa/drivers/dri/i965/brw_performance_query.c
>> @@ -1341,6 +1341,42 @@ brw_is_perf_query_ready(struct gl_context *ctx,
>>      return false;
>>   }
>>   
>> +static void
>> +gen8_read_report_clock_ratios(const uint32_t *report,
>> +                              uint64_t *slice_freq_hz,
>> +                              uint64_t *unslice_freq_hz)
>> +{
>> +   uint32_t unslice_freq = report[0] & 0x1ff;
>> +   uint32_t slice_freq_low = (report[0] >> 25) & 0x7f;
>> +   uint32_t slice_freq_high = (report[0] >> 9) & 0x3;
>> +   uint32_t slice_freq = slice_freq_low | (slice_freq_high << 7);
>> +
>> +   *slice_freq_hz = slice_freq * 16666000ULL;
>> +   *unslice_freq_hz = unslice_freq * 16666000ULL;
> What is this reading?  Could we add a comment to the function?
> Otherwise, I'm not sure where to even begin to look for this in docs.

Apologies, I've just added a fairly descriptive comment. I should have 
put that in the first version.

>
>> +}
>> +
>> +static void
>> +read_slice_unslice_frequencies(struct brw_context *brw,
>> +                               struct brw_perf_query_object *obj)
>> +{
>> +   const struct gen_device_info *devinfo = &brw->screen->devinfo;
>> +   uint32_t *begin_report, *end_report;
>> +
>> +   /* No data available before gen8. */
>> +   if (devinfo->gen < 8)
>> +      return;
>> +
>> +   begin_report = obj->oa.map;
>> +   end_report = obj->oa.map + MI_RPC_BO_END_OFFSET_BYTES;
> I'm probably missing something obvious here, but...this looks fishy.
>
> What actually writes these values to the OA buffer?  And here, you're
> reading the starting value from DWord 0 of the buffer...is that really
> where those values live?
>
> I guess I would have expected to see a capture_uncore_frequencies()
> and MI_SLICE_FREQ_START_OFFSET_BYTES or something, like patch 2/5 does.

We read these values from the OA reports. It's bits mashed into the 
RPT_ID field.
RPT_ID is the first dword indeed.

>
>> +
>> +   gen8_read_report_clock_ratios(begin_report,
>> +                                 &obj->oa.slice_frequency[0],
>> +                                 &obj->oa.unslice_frequency[0]);
>> +   gen8_read_report_clock_ratios(end_report,
>> +                                 &obj->oa.slice_frequency[1],
>> +                                 &obj->oa.unslice_frequency[1]);
>> +}
>> +
>>   static void
>>   read_gt_frequency(struct brw_context *brw,
>>                     struct brw_perf_query_object *obj)
>> @@ -1382,6 +1418,7 @@ get_oa_counter_data(struct brw_context *brw,
>>   
>>      if (!obj->oa.results_accumulated) {
>>         read_gt_frequency(brw, obj);
>> +      read_slice_unslice_frequencies(brw, obj);
>>         accumulate_oa_reports(brw, obj);
>>         assert(obj->oa.results_accumulated);
>>   
>> diff --git a/src/mesa/drivers/dri/i965/brw_performance_query.h b/src/mesa/drivers/dri/i965/brw_performance_query.h
>> index f8732738b4e..6d3ecf0be3a 100644
>> --- a/src/mesa/drivers/dri/i965/brw_performance_query.h
>> +++ b/src/mesa/drivers/dri/i965/brw_performance_query.h
>> @@ -118,6 +118,18 @@ struct brw_perf_query_object
>>             * Frequency of the GT at begin and end of the query.
>>             */
>>            uint64_t gt_frequency[2];
>> +
>> +         /**
>> +          * Frequency in the the slices of the GT at the begin and end of the
>> +          * query.
>> +          */
>> +         uint64_t slice_frequency[2];
>> +
>> +         /**
>> +          * Frequency in the the unslice of the GT at the begin and end of the
>> +          * query.
>> +          */
>> +         uint64_t unslice_frequency[2];
>>         } oa;
>>   
>>         struct {
>>