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 > 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