http://bugs.winehq.org/show_bug.cgi?id=21515
P.Panon ppanon@shaw.ca changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #26197|0 |1 is obsolete| |
--- Comment #74 from P.Panon ppanon@shaw.ca 2010-02-12 00:44:47 --- Created an attachment (id=26219) --> (http://bugs.winehq.org/attachment.cgi?id=26219) tarball of 5 sequential patches to 1.1.38
(In reply to comment #72)
The patches look pretty good, just a few comments:
Patch 2: HW_VENDOR_MESA isn't needed any longer, just use HW_VENDOR_WINE if the real HW vendor isn't properly detected.
Patch 3:
- /* if (match_apple(gl_info, gl_renderer, gl_vendor, card_vendor, device)) return FALSE;
- if (strstr(gl_renderer, "DRI")) return FALSE; Filter out Mesa DRI drivers. */
return TRUE;Yes, shouldn't be necessary, just remove both lines. I'd also invert the gl_vendor if check if(GL_VENDOR == GL_VENDOR_ATI) return TRUE; else return FALSE;
Patch 4:
- for (i = 0; i < (sizeof(vendor_card_select_table) / sizeof(*vendor_card_select_table)); ++i)
- {
if ((vendor_card_select_table[i].gl_vendor != *gl_vendor)|| (vendor_card_select_table[i].card_vendor != *card_vendor))continue;TRACE_(d3d_caps)("Applying card_selector \"%s\".\n", vendor_card_select_table[i].description);return vendor_card_select_table[i].select_card(gl_info, gl_renderer, vidmem);- /* Default to generic Nvidia hardware based on the supported OpenGL extensions. The choice
I recommend to check the select_card result, to allow the card detection code to return an unknown card(e.g. 0x0000) if the detection fails for some reason. Then the loop can abort and use the generic nvidia card selection code below.
The original card detection routine had a default Lowest Common Denominator card choice for each hardware vendor. I don't see why that needs to change since it seems to me that practice actually make more sense than falling though to a default of an unrelated vendor's product. The change you're asking for is also completely orthogonal to this bug and is not a necessary refactoring. Changing it is more likely to break existing behaviour and cause a regression which would hold up this time-sensitive patch.
/* Geforce6/7 lowend */
/* If it's GL_VENDOR_APPLE, then it could also be an ATI card, so allow it to fall through */ *vidmem = 64; /* */ return CARD_NVIDIA_GEFORCE_6200; /* Geforce 6100/6150/6200/7300/7400/7500 */Why would we end up in select_card_nvidia_binary on OSX with an ATI card?
That was left over from one of my earlier attempts at dealing with card detection before refactoring that function per your recommendation.
Patch 5:
+enum wined3d_pci_device (select_card_intel_mesa)(const struct wined3d_gl_info *gl_info, const char *gl_renderer,
unsigned int *vidmem )+{
FIXME_(d3d_caps)("Card selection not handled for Mesa Intel driver\n");if (WINE_D3D9_CAPABLE(gl_info)) return CARD_NVIDIA_GEFORCEFX_5600;This will lead to a PCI vendor 0x8086, pci device 0x0312 which doesn't exist. Some other nvidia card if interpreted as an Intel device might be a SATA Raid controller *gg*
Point taken. I've set it to default to the LCD for the existing Intel binary detection. They appear to officially all be DX9 capable in Windows, even if the Linux blobs, mesa drivers, or Apple GL drivers don't support all the necessary capabilities for DX9 emulation, so there's not much difference in the expectations of a windows app.
That taps into the suggestion concerning patch 4 - allow the detection functions to fail, and add a FIXME to the generic nvidia guessing code, something like FIXME("Implement a card ID detection function for GL vendor %s, HW vendor %s\n", debug_gl_vendor(gl_vendor), debug_hw_vendor(hw_vendor));
General: I don't know how you generated the patches, but the patch format(filename, lack of author info etc) is unknown to me.
It's called diff -u
I recommend to use git-format-patch, it allows you to add a description of what the patch does into the patch, and adds your name and Email address. There are some higher level tools like stacked git that can do this as well.
Thanks for the recommendation, perhaps at some point in time I'll find it useful, but I'm just not interested in learning your source control tool chain at this time. I just don't have time to set it up and learn it right now. If you're basically satisfied with the technical approach to the patch then to get this patch in I would suggest you or Luca try something like:
for i in `seq 1 5`; do patch -p0 < directx.c.p$i; [whatever check-in command you use]; done
I also no longer have time to play another round or more of "Now please fix this too". This updated patch archive addresses the remaining serious issues you've raised so there doesn't seem to be any major reason to hold it up. If you want to make some additional minor changes like you card_detect FIXME suggestion above (which is redundant since it already happens to a certain extent at vendor detection), you're welcome to unroll the above loop and make edits between the patch command and the check-in.
I wanted this to get into Ubuntu by a certain date, and with Curan's announcement in comment #73 that it's going into Debian unstable, there's a good chance it will be picked up. Banging my head here trying to satisfy you isn't going to change those chances significantly. My itch is scratched. Thanks for your past help. Peace out.