Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Mirek]

Hi, i just installed latest version of wine and now 3DMark 2003 and 
Flatout 2 is not working anymore, i look at the patches commited 
yesterday and found this patch which is causing this regression.

Mirek

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Stefan Dösinger]

Am Samstag 28 Oktober 2006 10:35 schrieb Mirek:
> Hi, i just installed latest version of wine and now 3DMark 2003 and
> Flatout 2 is not working anymore, i look at the patches commited
> yesterday and found this patch which is causing this regression.

In which way do they not work any more? Is there any debug output, or can you 
provide screenshots?

Can you test ARB and glsl shaders?

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Mirek]

Hi, screen is just black in those two apps, other apps (3DMark 2001, 
2005, 2006, NFS MW, Oblivion, TRL, W3, WoW, Prey, Starcraft, Diablo 2, 
GTA SA, HL 2 EO, HL, NFS 3, NFS 4, Turok, Ignition, Carmageddon 2, 
Carmageddon 3, Pandemonium 2) works without any problem.

Here is screenshot from 3DMark 2003 with GLSL enabled and disabled:
http://www.czela.net/pub/Czela_Debian_2.5/mirek/wine/regression/3DMark2003_A...
http://www.czela.net/pub/Czela_Debian_2.5/mirek/wine/regression/3DMark2003_G...

My software: Debian Sid, kernel 2.6.18.1 SMP, nVidia 9625 drivers.
My hardware: Core 2 Extreme, GF 6800 GS, 1GB RAM

Do you need anything else?

Mirek

Stefan Dösinger napsal(a):
> Am Samstag 28 Oktober 2006 10:35 schrieb Mirek:
> > Hi, i just installed latest version of wine and now 3DMark 2003 and
> > Flatout 2 is not working anymore, i look at the patches commited
> > yesterday and found this patch which is causing this regression.
>
> In which way do they not work any more? Is there any debug output, or can you 
> provide screenshots?
>
> Can you test ARB and glsl shaders?

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Ivan Gyurdiev]

The MOVA instruction is a 2.0 instruction, which cannot be fully 
implemented in ARB.
However, the patch shouldn't have caused a regression - if you're seeing 
a black screen with GLSL also, that means another patch is responsible 
for this.

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Mirek]

I tried it three times with same result. Can someone else test if 3DMark 
2003 is working with latest wine?

Mirek

Ivan Gyurdiev napsal(a):
> The MOVA instruction is a 2.0 instruction, which cannot be fully 
> implemented in ARB.
> However, the patch shouldn't have caused a regression - if you're seeing 
> a black screen with GLSL also, that means another patch is responsible 
> for this.

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Ivan Gyurdiev]

Mirek wrote:
> I tried it three times with same result. Can someone else test if 
> 3DMark 2003 is working with latest wine?

You shouldn't test if it's working now - test if it's working before the 
MOVA patch, and if it's not working after the MOVA patch [ no other 
patches in between ].

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Mirek]

Yes I did that this way. I tried it three times as it is now in CVS 
(3DMark 2003 and Flatout 2 not working) and three times without this 
patch (only) (3DMark 2003 and Flatout 2 works just perfect).

Mirek

Ivan Gyurdiev napsal(a):
> Mirek wrote:
> > I tried it three times with same result. Can someone else test if 
> > 3DMark 2003 is working with latest wine?
>
> You shouldn't test if it's working now - test if it's working before the 
> MOVA patch, and if it's not working after the MOVA patch [ no other 
> patches in between ].

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Tom Wickline]

On 10/28/06, Mirek  wrote:
> Yes I did that this way. I tried it three times as it is now in CVS
> (3DMark 2003 and Flatout 2 not working) and three times without this
> patch (only) (3DMark 2003 and Flatout 2 works just perfect).
>
> Mirek

I see the same thing as Mirek,  3dmark03 is totally broken now.

Tom

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Tom Wickline]

> I see the same thing as Mirek,  3dmark03 is totally broken now.

And yes I tested GLSL and ARB and before and after with the same results as his.
So yes, this is a major regression :D
> Tom

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Ivan Gyurdiev]

Tom Wickline wrote:
> > I see the same thing as Mirek,  3dmark03 is totally broken now.
>
> And yes I tested GLSL and ARB and before and after with the same 
> results as his.
> So yes, this is a major regression :D

When testing with the GLSL backend, can you please do a +d3d_shader 
trace, find the MOVA instruction, and send me the entire shader that 
it's part of (most importantly the shader version). The only way I see 
that this patch could break the GLSL backend is if the instruction 
version check is wrong. MSDN claims this instruction is available in 
shaders 2.0 and higher, but it wouldn't be the first time MSDN is dead 
wrong.

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Mirek]

Here is log with 3DMark running for about 3 seconds with patch and 
without, trace+d3d.

http://www.czela.net/pub/Czela_Debian_2.5/mirek/wine/regression/3DMark03.tar...

Mirek

Ivan Gyurdiev napsal(a):
> Tom Wickline wrote:
> > > I see the same thing as Mirek,  3dmark03 is totally broken now.
> >
> > And yes I tested GLSL and ARB and before and after with the same 
> > results as his.
> > So yes, this is a major regression :D
>
> When testing with the GLSL backend, can you please do a +d3d_shader 
> trace, find the MOVA instruction, and send me the entire shader that 
> it's part of (most importantly the shader version). The only way I see 
> that this patch could break the GLSL backend is if the instruction 
> version check is wrong. MSDN claims this instruction is available in 
> shaders 2.0 and higher, but it wouldn't be the first time MSDN is dead 
> wrong.

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Ivan Gyurdiev]

> Here is log with 3DMark running for about 3 seconds with patch and 
> without, trace+d3d.
>
> http://www.czela.net/pub/Czela_Debian_2.5/mirek/wine/regression/3DMark03.tar...

There's no mova instruction in that log.

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Ivan Gyurdiev]

Ivan Gyurdiev wrote:
> > Here is log with 3DMark running for about 3 seconds with patch and 
> > without, trace+d3d.
> >
> > http://www.czela.net/pub/Czela_Debian_2.5/mirek/wine/regression/3DMark03.tar...
>
> There's no mova instruction in that log.

...grep for mova, or for "unrecognized" or "Unrecognized" or "unhandled" 
or "Unhandled".
I see the app uses a number of vertex shaders 1.1, so maybe mova is a 
valid 1.1 instruction.

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Mirek]

Ok, i just found why is it not working with this patch, and why is it
working without this patch. It is based on position of definition MOVA
in vertexshader.c, if MOVA is on "after patch" position it is not
working, but if i move line "
{WINED3DSIO_MOVA, "mova",  NULL, 1, 2, vshader_mova, vshader_hw_map2gl,
shader_glsl_mov, WINED3DVS_VERSION(2,0), -1}," to position of definition
before patch, evrything is working.

Mirek

Ivan Gyurdiev napsal(a):
> > Here is log with 3DMark running for about 3 seconds with patch and 
> > without, trace+d3d.
> >
> > http://www.czela.net/pub/Czela_Debian_2.5/mirek/wine/regression/3DMark03.tar...
>
> There's no mova instruction in that log.

Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.

[Ivan Gyurdiev]

Mirek wrote:
> Ok, i just found why is it not working with this patch, and why is it
> working without this patch. It is based on position of definition MOVA
> in vertexshader.c, if MOVA is on "after patch" position it is not
> working, but if i move line "
> {WINED3DSIO_MOVA, "mova",  NULL, 1, 2, vshader_mova, vshader_hw_map2gl,
> shader_glsl_mov, WINED3DVS_VERSION(2,0), -1}," to position of definition
> before patch, evrything is working.

...
that's because Jason seems to have introduced order requirements on the 
table that weren't previously there. The mnxn implementation will 
attempt to access the table as an array, which will break once MOVA is 
moved before DP3:

glsl_shader.c:            tmpArg.opcode = 
&IWineD3DVertexShaderImpl_shader_ins[WINED3DSIO_DP4];
glsl_shader.c:            tmpArg.opcode = 
&IWineD3DVertexShaderImpl_shader_ins[WINED3DSIO_DP3];

The instruction table currently does not support this, and it only 
happens to work for the first few instructions [ which the MOVA change 
breaks ].

Regression in patch wined3d: Implement D3DSIO_MOVAin ARB backend.

[Aric Cyr]

Ivan Gyurdiev <ivg231 <at> gmail.com> writes:
> that's because Jason seems to have introduced order requirements on the 
> table that weren't previously there. The mnxn implementation will 
> attempt to access the table as an array, which will break once MOVA is 
> moved before DP3:
>
> glsl_shader.c:            tmpArg.opcode = 
> &IWineD3DVertexShaderImpl_shader_ins[WINED3DSIO_DP4];
> glsl_shader.c:            tmpArg.opcode = 
> &IWineD3DVertexShaderImpl_shader_ins[WINED3DSIO_DP3];

Yup, seems like the temptation to dereference the array directly was too great.

These should be replaced to use baseshader.c:get_shader_opcode(...), I think,
instead of the array access.  Exact same thing also in arb_program_shader.c too.

To prevent such things in the future, is there any way to remove the enum
WINED3DSHADER_INSTRUCTION_OPCODE_TYPE and place it somewhere less visible that
the included-everywhere wined3d_private_types.h?  A quick glance at the code
seems to suggest it is not feasible right now, but maybe someone has an idea?

Or better yet, since device.c is the only legitimate user of
IWineD3DVertexShaderImpl_shader_ins[], don't extern that array in
wined3d_private.h, but only in device.c (i.e. move the extern declaration from
wined3d_private.h to device.c).  Then glsl_shader.c and arb_program_shader.c and
future source would properly fail to compile on this type of mistake.

In the worst case a comment to the effect of
  /* don't dereference this array directly */ 
certainly seems in order...

- Aric