On 02/10/2016 12:35 PM, Matt Turner wrote: > On Tue, Feb 9, 2016 at 10:04 PM, Ben Widawsky > <benjamin.widaw...@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.ki...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev