RE: ddraw/tests: Skip cubemap tests when cubemap isn't supported.
skip("No cubemap support\n"); if you return right after that you wouldn't have to change the indention on all the rest of the code
Also I think in the case of the surface test, there should be an easier way to tell if cubemaps are supported, without starting a d3d device. In the worst case just abort on a CreateSurface error
-----Original Message----- From: wine-patches-bounces(a)winehq.org [mailto:wine-patches- bounces(a)winehq.org] On Behalf Of Alexander Dorofeyev Sent: Tuesday, July 29, 2008 5:25 PM To: wine-patches(a)winehq.org Subject: ddraw/tests: Skip cubemap tests when cubemap isn't supported.
Fixes some test failures on systems without cubemap support. --- dlls/ddraw/tests/d3d.c | 428 ++++++++++++++++++++++------------ --------- dlls/ddraw/tests/dsurface.c | 57 ++++++- 2 files changed, 275 insertions(+), 210 deletions(-)
ok finally learned how to do git... here is the git patch... I thought of moving the if statement which is at the bottom of the function to the begining as I am not sure why you would want to reassign the same value then check to see if it was the same.. Why are you doing it that way? As for the test case I am working on it ... I think I figured out the test suite and it might already have what I need to test in the code. Chris
From 2ef9f513d45f8c5a4d66998c46dd77c558ad3024 Mon Sep 17 00:00:00 2001 From: Chris Ahrendt <celticht32(a)aol.com> Date: Tue, 29 Jul 2008 23:53:17 -0400 Subject: [PATCH] Completed Set RenderState in device.c
--- dlls/wined3d/device.c | 207 +++++++++++++++++++++++++++++++++++++++++- include/wine/wined3d_types.h | 7 ++ 2 files changed, 210 insertions(+), 4 deletions(-) diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c index 0a6bd2b..7031a2a 100644 --- a/dlls/wined3d/device.c +++ b/dlls/wined3d/device.c @@ -3248,15 +3248,214 @@ static HRESULT WINAPI IWineD3DDeviceImpl_GetViewport(IWineD3DDevice *iface, WINE /***** * Get / Set Render States - * TODO: Verify against dx9 definitions - *****/ + Parms + State This param will be any member of the WINED3DRENDERSTATETYPE type. + + Value This is dependent on the State value + + ****/ static HRESULT WINAPI IWineD3DDeviceImpl_SetRenderState(IWineD3DDevice *iface, WINED3DRENDERSTATETYPE State, DWORD Value) { IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *)iface; DWORD oldValue = This->stateBlock->renderState[State]; TRACE("(%p)->state = %s(%d), value = %d\n", This, debug_d3drenderstate(State), State, Value); - + + switch(State) + { + /* These States do not need Value checking + They either contain a valid DWORD value or a TRUE == 0 + or FALSE > 0. So do not need the type checking of the + other calls */ + case WINED3DRS_BLENDOPALPHA: + case WINED3DRS_FOGCOLOR: + case WINED3DRS_SEPARATEALPHABLENDENABLE: + case WINED3DRS_TWOSIDEDSTENCILMODE: + case WINED3DRS_ENABLEADAPTIVETESSELLATION: + case WINED3DRS_ANTIALIASEDLINEENABLE: + case WINED3DRS_SCISSORTESTENABLE: + case WINED3DRS_INDEXEDVERTEXBLENDENABLE: + case WINED3DRS_MULTISAMPLEANTIALIAS: + case WINED3DRS_POINTSPRITEENABLE: + case WINED3DRS_POINTSCALEENABLE: + case WINED3DRS_NORMALIZENORMALS: + case WINED3DRS_LOCALVIEWER: + case WINED3DRS_COLORVERTEX: + case WINED3DRS_CLIPPING: + case WINED3DRS_ZENABLE: + case WINED3DRS_STENCILENABLE: + case WINED3DRS_RANGEFOGENABLE: + case WINED3DRS_ALPHABLENDENABLE: + case WINED3DRS_DITHERENABLE: + case WINED3DRS_ZWRITEENABLE: + case WINED3DRS_ALPHATESTENABLE: + case WINED3DRS_FOGENABLE: + case WINED3DRS_SPECULARENABLE: + case WINED3DRS_LIGHTING: + case WINED3DRS_LASTPIXEL: + case WINED3DRS_DEPTHBIAS: + case WINED3DRS_SRGBWRITEENABLE: + case WINED3DRS_BLENDFACTOR: + case WINED3DRS_COLORWRITEENABLE1: + case WINED3DRS_MULTISAMPLEMASK: + case WINED3DRS_AMBIENT: + case WINED3DRS_TEXTUREFACTOR: + case WINED3DRS_STENCILWRITEMASK: + case WINED3DRS_STENCILMASK: + case WINED3DRS_COLORWRITEENABLE2: + case WINED3DRS_COLORWRITEENABLE3: + case WINED3DRS_ADAPTIVETESS_X: + case WINED3DRS_ADAPTIVETESS_Y: + case WINED3DRS_ADAPTIVETESS_Z: + case WINED3DRS_ADAPTIVETESS_W: + case WINED3DRS_SLOPESCALEDEPTHBIAS: + case WINED3DRS_TWEENFACTOR: + case WINED3DRS_COLORWRITEENABLE: + case WINED3DRS_POINTSIZE_MAX: + case WINED3DRS_WRAP0: + case WINED3DRS_WRAP1: + case WINED3DRS_WRAP2: + case WINED3DRS_WRAP3: + case WINED3DRS_WRAP4: + case WINED3DRS_WRAP5: + case WINED3DRS_WRAP6: + case WINED3DRS_WRAP7: + case WINED3DRS_WRAP8: + case WINED3DRS_WRAP9: + case WINED3DRS_WRAP10: + case WINED3DRS_WRAP11: + case WINED3DRS_WRAP12: + case WINED3DRS_WRAP13: + case WINED3DRS_WRAP14: + case WINED3DRS_WRAP15: + case WINED3DRS_STENCILREF: + case WINED3DRS_FOGSTART: + case WINED3DRS_FOGEND: + case WINED3DRS_FOGDENSITY: + case WINED3DRS_CLIPPLANEENABLE: + case WINED3DRS_POINTSIZE: + case WINED3DRS_POINTSIZE_MIN: + case WINED3DRS_POINTSCALE_A: + case WINED3DRS_POINTSCALE_B: + case WINED3DRS_POINTSCALE_C: + break; + + case WINED3DRS_FILLMODE: + if ((Value >= WINED3DFILL_POINT) && (Value <= WINED3DFILL_SOLID)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_SHADEMODE: + if ((Value >= WINED3DSHADE_FLAT) && (Value <= WINED3DSHADE_PHONG)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_SRCBLENDALPHA: + case WINED3DRS_DESTBLENDALPHA: + case WINED3DRS_DESTBLEND: + case WINED3DRS_SRCBLEND: + if ((Value >= WINED3DBLEND_ZERO) && (Value <= WINED3DBLEND_INVSRCCOLOR2)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_CULLMODE: + if ((Value >= WINED3DCULL_NONE) && (Value <= WINED3DCULL_CCW)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_CCW_STENCILFUNC: + case WINED3DRS_STENCILFUNC: + case WINED3DRS_ALPHAFUNC: + case WINED3DRS_ZFUNC: + if ((Value >= WINED3DCMP_NEVER) && (Value <= WINED3DCMP_ALWAYS)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_ALPHAREF: + /* Valid values are between 0 and FF */ + if ((Value >= 0) || + (Value <= 0xFF)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_FOGVERTEXMODE: + case WINED3DRS_FOGTABLEMODE: + if ((Value >= WINED3DFOG_NONE) && (Value <= WINED3DFOG_LINEAR)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_CCW_STENCILFAIL: + case WINED3DRS_CCW_STENCILZFAIL: + case WINED3DRS_CCW_STENCILPASS: + case WINED3DRS_STENCILPASS: + case WINED3DRS_STENCILZFAIL: + case WINED3DRS_STENCILFAIL: + if ((Value >= WINED3DSTENCILOP_KEEP) && (Value <= WINED3DSTENCILOP_DECR)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_SPECULARMATERIALSOURCE: + case WINED3DRS_AMBIENTMATERIALSOURCE: + case WINED3DRS_EMISSIVEMATERIALSOURCE: + case WINED3DRS_DIFFUSEMATERIALSOURCE: + if ((Value >= WINED3DMCS_MATERIAL) && (Value <= WINED3DMCS_COLOR2)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_VERTEXBLEND: + if ((Value >= WINED3DVBF_DISABLE) && (Value <= WINED3DVBF_0WEIGHTS)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_PATCHEDGESTYLE: + if ((Value == WINED3DPATCHEDGE_DISCRETE) || + (Value == WINED3DPATCHEDGE_CONTINUOUS)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_DEBUGMONITORTOKEN: + if ((Value == WINED3DDMT_ENABLE) || + (Value == WINED3DDMT_DISABLE)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_BLENDOP: + if ((Value >= WINED3DBLENDOP_ADD) && (Value <= WINED3DBLENDOP_MAX)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_NORMALDEGREE: + case WINED3DRS_POSITIONDEGREE: + if ((Value >= WINED3DDEGREE_LINEAR)&&(Value <= WINED3DDEGREE_QUINTIC)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + case WINED3DRS_MINTESSELLATIONLEVEL: + case WINED3DRS_MAXTESSELLATIONLEVEL: + if ((Value >= WINED3DDEGREE_LINEAR) && (Value <= WINED3DDEGREE_QUINTIC)) break; + TRACE("Invalid Value\n"); + return WINED3DERR_INVALIDCALL; + break; + + default: + TRACE("Unkown State type = %d\n",State); + return WINED3DERR_INVALIDCALL; + } + This->updateStateBlock->changed.renderState[State] = TRUE; This->updateStateBlock->renderState[State] = Value; @@ -3278,7 +3477,7 @@ static HRESULT WINAPI IWineD3DDeviceImpl_SetRenderState(IWineD3DDevice *iface, W static HRESULT WINAPI IWineD3DDeviceImpl_GetRenderState(IWineD3DDevice *iface, WINED3DRENDERSTATETYPE State, DWORD *pValue) { IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *)iface; - TRACE("(%p) for State %d = %d\n", This, State, This->stateBlock->renderState[State]); + TRACE("(%p) for State %d = %d\n", This, State, This->stateBlock->renderState[State]); *pValue = This->stateBlock->renderState[State]; return WINED3D_OK; } diff --git a/include/wine/wined3d_types.h b/include/wine/wined3d_types.h index c67c311..7cfdeb8 100644 --- a/include/wine/wined3d_types.h +++ b/include/wine/wined3d_types.h @@ -1512,6 +1512,12 @@ typedef enum _WINED3DSURFTYPE { #define WINED3DCLIPPLANE4 (1 << 4) #define WINED3DCLIPPLANE5 (1 << 5) +#define WINED3DDMT_ENABLE 0 +#define WINED3DDMT_DISABLE 1 +#define WINED3DBLEND_INVSRCCOLOR2 17 +#define WINED3DBLEND_SRCCOLOR2 16 + + /* FVF (Flexible Vertex Format) codes */ #define WINED3DFVF_RESERVED0 0x0001 #define WINED3DFVF_POSITION_MASK 0x000E @@ -1782,3 +1788,4 @@ typedef struct _WINEDDOVERLAYFX #define WINEDDFLIP_INTERVAL4 0x04000000 #endif + -- 1.5.5.1
2008/7/30 Chris Ahrendt <celticht32(a)aol.com>:
I thought of moving the if statement which is at the bottom of the function to the begining as I am not sure why you would want to reassign the same value then check to see if it was the same..
Why are you doing it that way?
Please be more careful when reading source code or patch feedback.
H. Verbeet wrote:
2008/7/30 Chris Ahrendt <celticht32(a)aol.com>:
I thought of moving the if statement which is at the bottom of the function to the begining as I am not sure why you would want to reassign the same value then check to see if it was the same..
Why are you doing it that way?
Please be more careful when reading source code or patch feedback. What do you mean ?
Chris
H. Verbeet wrote:
2008/7/30 Chris Ahrendt <celticht32(a)aol.com>:
What do you mean ?
The code doesn't actually do what you say it does. its a patch to finish out IWineD3DDeviceImpl_SetRenderState and verifies against the direct x 9 definitions...
Chris
Stefan Dösinger wrote:
skip("No cubemap support\n"); if you return right after that you wouldn't have to change the indention on all the rest of the code
There are some tests after that block not related to cubemap which I don't want to skip. Other options are moving cubemap block of code to the end (so that I can return then) or skipping the block by goto, but none of this are really better IMO.
Also I think in the case of the surface test, there should be an easier way to tell if cubemaps are supported, without starting a d3d device. In the worst case just abort on a CreateSurface error
I spent some time looking for a better working option a while ago, but couldn't find anything working on Wine, Win98 (qemu) and XP. Will just check CreateSurface if that's ok.
There are some tests after that block not related to cubemap which I don't want to skip. Other options are moving cubemap block of code to the end (so that I can return then) or skipping the block by goto, but none of this are really better IMO. DeviceLoadTest is quite big, does splitting up make sense? Otherwise, what you say is fair enough - I do not care too much about code beauty in the tests as long as the tests work(since one hardly ever reads the test or does lots of work on it, and the Windows apps we run are often crappy as well)
Stefan Dösinger wrote:
DeviceLoadTest is quite big, does splitting up make sense? Otherwise, what you say is fair enough - I do not care too much about code beauty in the tests as long as the tests work(since one hardly ever reads the test or does lots of work on it, and the Windows apps we run are often crappy as well)
I don't care too much either way, but I don't see use in splitting because cubemap part uses same variables, works in a similar way to other non-cubemap parts and generally is part of the whole.
While looking at bug 13335 ( http://bugs.winehq.org/show_bug.cgi?id=13335) which is An ATI and wine bug. I noticed something in one of the updates : http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=462715 Now this set me to searching as I really didn't like the idea of wrapping the libGL calls with 'winmemory' allocations (To many areas of corruption possibilities and just isn't an ellegant solution). The Debian post above points out something very interesting : " After more investigation, I was able to trace this to a bug in my OpenGL initialization logic. The following (psuedocode) sequence, forces fglrx to revert to indirect rendering: dpy = XOpenDisplay() visual = glXChooseVisual(dpy) XCloseDisplay(dpy) .. dpy2 = XOpenDisplay() ctx = glXCreateContext(dpy2, visual) <-- fglrx reverts to indirect rendering .. Changing the code to use the same display connection for glXChooseVisual and glXCreateContext solves this problem." So looking at that I began to Grep the latest git tree to find out where XOpenDisplay glxChooseVisual glxCreateContext XCloseDisplay are used. There are only 2 places (and yes I know they are critical places) where they are used. opengl.c and x11drv_attach.c both in the winex11.drv directory. So I went through and started looking at how they were called and discovered that the display variable (dpy and dpy2 in the above pseudocode) is not reused at all. It is allocated off the local stack of the subroutine it is in. later on gdi_display which is the global variable gets assigned display which calls the XOpenDisplay. This gdi_display is used as the global handle if I am reading the code correctly. Now this is where my pointer knowledge goes south and I think this is right... and this is where the problem might be: in the process_attach routine someone says gdi_display = display. Display is a local pointer to the routine and will go away when the function returns. Here is my theory : with the ATI card being a pig for memory and Wine allocating alot of memory itself the pointer to the driver gets invalidated somehow corrupted etc. and when it is used later X thinks its another display and everything happily crashes. Mind you this is a theory right now but is this possible? I think an easy fix to this would be to do the following instead: gdi_display = XopenDisplay()'; This allocates the display structure off the global heap not the local heap display = gdi_display; and then leave the rest of the code alone. Am I going down the right track with this? If so it would be alot simpler fix that some of the ones out there being forced around. And it should not effect any of the other supported cards. Chris
celticht32(a)aol.com wrote:
While looking at bug 13335 ( http://bugs.winehq.org/show_bug.cgi?id=13335) which is An ATI and wine bug. I noticed something in one of the updates :
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=462715
Now this set me to searching as I really didn't like the idea of wrapping the libGL calls with 'winmemory' allocations
(To many areas of corruption possibilities and just isn't an ellegant solution). The Debian post above points out something
very interesting :
" After more investigation, I was able to trace this to a bug in my OpenGL
initialization logic.
The following (psuedocode) sequence, forces fglrx to revert to indirect
rendering:
dpy = XOpenDisplay()
visual = glXChooseVisual(dpy)
XCloseDisplay(dpy)
..
dpy2 = XOpenDisplay()
ctx = glXCreateContext(dpy2, visual) <-- fglrx reverts to indirect
rendering
..
Changing the code to use the same display connection for glXChooseVisual
and glXCreateContext solves this problem."
So looking at that I began to Grep the latest git tree to find out where
XOpenDisplay
glxChooseVisual
glxCreateContext
XCloseDisplay
are used. There are only 2 places (and yes I know they are critical places) where they are used.
opengl.c and x11drv_attach.c both in the winex11.drv directory. So I went through and started looking
at how they were called and discovered that the display variable (dpy and dpy2 in the above pseudocode) is not
reused at all. It is allocated off the local stack of the subroutine it is in. later on gdi_display which is the global
variable gets assigned display which calls the XOpenDisplay. This gdi_display is used as the global handle
if I am reading the code correctly.
Now this is where my pointer knowledge goes south and I think this is right... and this is where the problem might be:
in the process_attach routine someone says gdi_display = display. Display is a local pointer to the routine and will
go away when the function returns.
Here is my theory :
with the ATI card being a pig for memory and Wine allocating alot of memory itself the pointer to the driver gets invalidated
somehow corrupted etc. and when it is used later X thinks its another display and everything happily crashes.
Mind you this is a theory right now but is this possible? I think an easy fix to this would be to do the following instead:
gdi_display = XopenDisplay()'; This allocates the display structure off the global heap not the local heap
display = gdi_display;
and then leave the rest of the code alone.
Am I going down the right track with this? If so it would be alot simpler fix that some of the ones out there being forced around. And it should not
effect any of the other supported cards.
Chris
------------------------------------------------------------------------ The Famous, the Infamous, the Lame - in your browser. Get the TMZ Toolbar Now <http://toolbar.aol.com/tmz/download.html?NCID=aolcmp00050000000014>!
------------------------------------------------------------------------
That FIXED IT!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! the open gl error is now GONE! I am not using ulimit... I am not using the comment patch in directx. Its a simple Change!!!! in x11drv_main.c change to this and I no longer get the opengl error at all: /* Open display */ if (!(gdi_display = XOpenDisplay( NULL ))) return FALSE; display = gdi_display;
2008/7/30 Stefan Dösinger <stefan(a)codeweavers.com>:
DeviceLoadTest is quite big, does splitting up make sense? Otherwise, what you say is fair enough - I do not care too much about code beauty in the tests as long as the tests work(since one hardly ever reads the test or does lots of work on it, and the Windows apps we run are often crappy as well)
Please think of the poor people who are trying to fix driver bugs :-)
participants (5)
-
Alexander Dorofeyev -
celticht32＠aol.com -
Chris Ahrendt
-
H. Verbeet -
Stefan Dösinger