[Intel-gfx] [PATCH 6/6] drm/i915: Add more OA configs for BDW, CHV, SKL + BXT

[Robert Bragg]

On Wed, Mar 1, 2017 at 1:00 PM, Matthew Auld
<matthew.william.auld at gmail.com> wrote:
> On 02/22, Robert Bragg wrote:
>> These are auto generated from an XML description of metric sets,
>> currently maintained in gputop, ref:
>>
>>  https://github.com/rib/gputop
>>  > gputop-data/oa-*.xml
>>  > scripts/i915-perf-kernelgen.py
>>
>>  $ make -C gputop-data -f Makefile.xml
>>
>> Signed-off-by: Robert Bragg <robert at sixbynine.org>
>> ---
>
> <SNIP>
>
>> +
>> +static const struct i915_oa_reg *
>> +get_compute_extended_mux_config(struct drm_i915_private *dev_priv,
>> +                             int *len)
>> +{
>> +     if (INTEL_INFO(dev_priv)->sseu.subslice_mask & 0x01) {
>> +             *len = ARRAY_SIZE(mux_config_compute_extended_1_0_subslices_0x01);
>> +             return mux_config_compute_extended_1_0_subslices_0x01;
>> +     } else if (INTEL_INFO(dev_priv)->sseu.subslice_mask & 0x08) {
>> +             *len = ARRAY_SIZE(mux_config_compute_extended_1_1_subslices_0x08);
>> +             return mux_config_compute_extended_1_1_subslices_0x08;
>> +     } else if (INTEL_INFO(dev_priv)->sseu.subslice_mask & 0x02) {
>> +             *len = ARRAY_SIZE(mux_config_compute_extended_1_2_subslices_0x02);
>> +             return mux_config_compute_extended_1_2_subslices_0x02;
>> +     } else if (INTEL_INFO(dev_priv)->sseu.subslice_mask & 0x10) {
>> +             *len = ARRAY_SIZE(mux_config_compute_extended_1_3_subslices_0x10);
>> +             return mux_config_compute_extended_1_3_subslices_0x10;
>> +     } else if (INTEL_INFO(dev_priv)->sseu.subslice_mask & 0x04) {
>> +             *len = ARRAY_SIZE(mux_config_compute_extended_1_4_subslices_0x04);
>> +             return mux_config_compute_extended_1_4_subslices_0x04;
>> +     } else if (INTEL_INFO(dev_priv)->sseu.subslice_mask & 0x20) {
>> +             *len = ARRAY_SIZE(mux_config_compute_extended_1_5_subslices_0x20);
>> +             return mux_config_compute_extended_1_5_subslices_0x20;
>> +     *len = ARRAY_SIZE(mux_config_compute_extended);
>> +     return mux_config_compute_extended;
> It looks like your script doesn't properly handle the unconditional mux
> config here.

Very well spotted!

This is a pretty ambiguous case and I'm not entirely sure what the
right thing to do here is and have asked the folks that came up with
the original OA config for their feedback. I *think* in this case it's
supposed to combine both as if the unconditional config were logically
concatenated at the end of all the conditional configs.

Locally I've at least updated the i915-perf-kernelgen.py script to
print a loud warning out for this so I could double check in case
there were other similar cases I'd missed - this BDW config seems to
be the only one with this ambiguity though. This makes me think that I
should update the mdapi-xml-convert.py script to just hide this
special case from our oa-*.xml file by explicitly concatenating with
the conditional configs if that's appropriate - then we don't need to
support multiple NOA configs in the i915 perf implementation. The
scripts can then have strict assertions that this doesn't change in
the future.

>
> <SNIP>
>
>> +
>> +static const struct i915_oa_reg *
>> +get_test_oa_mux_config(struct drm_i915_private *dev_priv,
>> +                    int *len)
>> +{
>> +     *len = ARRAY_SIZE(mux_config_test_oa);
>> +     return mux_config_test_oa;
>> +}
>> +
>> +int i915_oa_select_metric_set_bdw(struct drm_i915_private *dev_priv)
>> +{
>> +     dev_priv->perf.oa.mux_regs = NULL;
>> +     dev_priv->perf.oa.mux_regs_len = 0;
>> +     dev_priv->perf.oa.b_counter_regs = NULL;
>> +     dev_priv->perf.oa.b_counter_regs_len = 0;
>> +     dev_priv->perf.oa.flex_regs = NULL;
>> +     dev_priv->perf.oa.flex_regs_len = 0;
>> +
>> +     switch (dev_priv->perf.oa.metrics_set) {
>> +     case METRIC_SET_ID_RENDER_BASIC:
>> +             dev_priv->perf.oa.mux_regs =
>> +                     get_render_basic_mux_config(dev_priv,
>> +                                                 &dev_priv->perf.oa.mux_regs_len);
>> +             if (!dev_priv->perf.oa.mux_regs) {
>> +                     DRM_DEBUG_DRIVER("No suitable MUX config for \"RENDER_BASIC\" metric set");
> Your script also needs to output a newline for DRM_DEBUG_DRIVER.

Ah yep, fixed locally thanks,

>