[Mesa-dev] [PATCH v3] workarounds: Update workaround names and platforms

[Matt Turner]

On Tue, Feb 9, 2016 at 10:04 PM, Ben Widawsky
<benjamin.widawsky at intel.com> wrote:
> On Tue, Feb 09, 2016 at 10:35:45AM -0800, Matt Turner wrote:
>> On Tue, Feb 9, 2016 at 9:44 AM, Sameer Kibey <sameer.kibey at intel.com> wrote:
>> > Update the format in which workarounds are documented
>> > in the source code. This allows mesa to be parsed
>> > by the list-workarounds utility in intel-gpu-tools.
>>
>> I don't know that I find this valuable.
>>
>> Ben touched on one concern -- keeping it updated. But I have another,
>> and that's whether the information is accurate, or useful at all.
>>
>>
>
> I think the bottom line is it really doesn't hurt the existing situation. The
> primary benefit is it gives us internally a way to easily compare the
> workarounds for different drivers.

That's part of the concern. I don't have any idea how this information
is going to be used. If I've understood correctly, it's going to be
fed into the workarounds database so that we can compare what we
implement vs the Windows driver.

That sounds nice, but we use that database (perhaps inappropriately)
to learn about hardware bugs. What happens when we start feeding bad
information into that database? (See this patch)

I get that this is just futzing with comments. But can't we do
something better that actually captures what the code is doing? I'm
thinking a brw_wa bitfield struct, created with the screen and
initialized with an expression that corresponds to the data we want to
encode. For example, for WaCMPInstFlagDepClearedEarly:ivb,hsw,vlv we'd
have

   WaCMPInstFlagDepClearedEarly = brw->gen == 7;

and then the code that implements the workaround would simply be
wrapped in "if (Wa->WaCMPInstFlagDepClearedEarly)"

I've heard people say that "this is what the kernel does" and that
they already have scripts, but that doesn't address the problem of the
comments not matching the code. I grepped for "Wa" in
drivers/gpu/drm/i915 and literally the first workaround I looked at
(WaProgramMiArbOnOffAroundMiSetContext in i915_gem_context.c) seems to
suffer from the problem I'm talking about -- it reads:

        /* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
        if (INTEL_INFO(ring->dev)->gen >= 7) {

which first makes me wonder if it's actually only Gen7 and 8 that the
workaround applies to (which the comment would lead you to believe).
So I checked the workarounds database, and it actually says it's just
HSW, VLV, and IVB, but then it lists source files and it appears in
files beyond Gen8. So in four pieces of data, we have three
conflicting answers.

Can a kernel developer weigh in? Someone please convince me that all
of this isn't just cargo-culted.

It's certainly valuable to be able to determine the status of
workarounds in the driver. I just don't see how this can accomplish
that goal.