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

[Ian Romanick]

On 02/10/2016 12:35 PM, Matt Turner wrote:
> 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

Paraphrasing what Matt says below... It hurts the existing situation
because it gives us a false sense of security.

>> 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.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>