[Bug 21515] VENDOR_WINE vs VENDOR_ATI with xf86-video-ati
wine-bugs at winehq.org
wine-bugs at winehq.org
Fri Feb 12 00:44:48 CST 2010
http://bugs.winehq.org/show_bug.cgi?id=21515
P.Panon <ppanon at shaw.ca> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #26197|0 |1
is obsolete| |
--- Comment #74 from P.Panon <ppanon at 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.
--
Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email
Do not reply to this email, post in Bugzilla using the
above URL to reply.
------- You are receiving this mail because: -------
You are watching all bug changes.
More information about the wine-bugs
mailing list