http://bugs.winehq.org/show_bug.cgi?id=21515
--- Comment #41 from P.Panon ppanon@shaw.ca 2010-02-07 21:00:13 --- (In reply to comment #40)
One more addendum regarding my last patch, I also wasn't quite sure what to do with some of the match functions like match_geforce5(). There I changed the vendor comparison to a comparison of the gl_vendor, however I'm not sure if that's correct
It depends on where the function is used. The geforce5 one is used mainly for the non power of two quirk, which is a hardware limit, so it would apply regardless of the GL driver vendor. Other functions like match_ati_r300_500_apple are used to work around driver specific issues, so you want to match driver + hw_vendor + card_id
I recommend to introduce the separation of HW and driver vendor in small patches without first adding any new features like detection of new mesa cards.
Well, that's all very nice but detection of new mesa cards is this bug report is all about after all, as I'm sure cruiseoveride will be happy to remind us :-) So while I'm sure you would like all this code cleaned up, we would like the detection of the new mesa cards. And as I pointed out, my configuration doesn't report GLX info properly so I have a serious problem with testing my results.
Once the GL driver vendor is in place, and the HW vendor is untouched you can discuss and move over the matching functions one by one. For each of those we'll have to look at what the quirk does to decide what to do.
A possible solution is to split up the single matching function into 4, e.g. match_gl_vendor, match_hw_vendor, match_card, match_extra and only apply the quirk if all 4 return TRUE. This would avoid complex single-quirk match functions. If one field doesn't matter to a specific quirk(e.g. the NP2 GF5 quirk) the quirk could use a function that always returns TRUE, or we add special handling for a NULL function.
I guess that depends on how many driver quirks you foresee having to handle. If you have a bunch of card-based quirks (or caps-based tests like the dx10 or spec alpha tests) that are going across multiple GL_VENDOR values, that's going to really blow up your table size, It might be a bit more scalable in the long run, but it doesn't seem to be a huge issue with the few tests you have now so that refactoring shouldn't hold up this bug. Right now I would rather just keep the existing setup with clarification on a few tests of whether they apply to gl_vendor or card_vendor. Besides the match_geforce5(), I think the only other one that needs to be clarified is match_ati_r300_to_500(): should it also match for GL_VENDOR_ATI and GL_VENDOR_APPLE.
I really don't think it should be holding up a solution to this bug. On the other hand, I do appreciate the advantages of the suggestion in comment #39 on code readabilty since ...guess_card() is pretty hairy right now, so I'll see if I can do something about that.