April 2017 - Wine-Devel - list.winehq.org

Re: [v2 6/6] d3dx9: Support parameters sharing in effect.

by Paul Gofman

On 04/25/2017 07:34 PM, Matteo Bruni wrote: > 2017-04-25 18:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >> On 04/25/2017 07:09 PM, Matteo Bruni wrote: >>> >>>>>> static void free_effect_pool(struct d3dx_effect_pool *pool) >>>>>> { >>>>>> + unsigned int i; >>>>>> + >>>>>> + for (i = 0; i < pool->table_size; ++i) >>>>>> + { >>>>>> + if (pool->shared_data[i].refcount) >>>>>> + ERR("Releasing pool with referenced parameters, shared >>>>>> data >>>>>> is not freed.\n"); >>>>>> + } >>>>> This can be triggered by an application so it should be a WARN(). >>>>> BTW, this probably needs a test. On pool destruction, the effects >>>>> still using it might be "unshared" i.e. data from the pool might be >>>>> moved back to the effects. Or maybe the effects still using the pool >>>>> are simply broken and accessing parameter data after the pool is >>>>> destroyed crashes. >>>>> >>>>> >>>> Yes, sure, there is one of the marginal cases to be tested more (probably >>>> along with more precise understanding on how shared shader parameters >>>> setting works when sharing non-NULL and NULL shader). Until then maybe we >>>> could leave something more severe than WARN here, as if we reach this >>>> point >>>> it most likely mean that something is wrong and it would be nice to have >>>> an >>>> indication right away. >>> I think we want to know what happens before we go ahead with the code >>> above. >>> >> I just tested, manually releasing pool before effect just crash when I use >> native d3dx dll. Please see attached patch. I have to do release twice as it >> is referenced from 2 effects. If I release pool just once, it crashes in >> freeing effect. > Excellent. Adding the test in an if (0) {} + a comment would probably > be nice for documentation purposes. For the implementation I'd go with > WARN + releasing shared data. > Could you please advise how to best add it: just as this patch in if(0) {}, or a separate test?

April 25, 2017

Re: [v2 6/6] d3dx9: Support parameters sharing in effect.

by Matteo Bruni

2017-04-25 18:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 04/25/2017 07:09 PM, Matteo Bruni wrote: >> >> >>>>> static void free_effect_pool(struct d3dx_effect_pool *pool) >>>>> { >>>>> + unsigned int i; >>>>> + >>>>> + for (i = 0; i < pool->table_size; ++i) >>>>> + { >>>>> + if (pool->shared_data[i].refcount) >>>>> + ERR("Releasing pool with referenced parameters, shared >>>>> data >>>>> is not freed.\n"); >>>>> + } >>>> >>>> This can be triggered by an application so it should be a WARN(). >>>> BTW, this probably needs a test. On pool destruction, the effects >>>> still using it might be "unshared" i.e. data from the pool might be >>>> moved back to the effects. Or maybe the effects still using the pool >>>> are simply broken and accessing parameter data after the pool is >>>> destroyed crashes. >>>> >>>> >>> Yes, sure, there is one of the marginal cases to be tested more (probably >>> along with more precise understanding on how shared shader parameters >>> setting works when sharing non-NULL and NULL shader). Until then maybe we >>> could leave something more severe than WARN here, as if we reach this >>> point >>> it most likely mean that something is wrong and it would be nice to have >>> an >>> indication right away. >> >> I think we want to know what happens before we go ahead with the code >> above. >> > I just tested, manually releasing pool before effect just crash when I use > native d3dx dll. Please see attached patch. I have to do release twice as it > is referenced from 2 effects. If I release pool just once, it crashes in > freeing effect. Excellent. Adding the test in an if (0) {} + a comment would probably be nice for documentation purposes. For the implementation I'd go with WARN + releasing shared data.

April 25, 2017

Re: [v2 6/6] d3dx9: Support parameters sharing in effect.

by Paul Gofman

On 04/25/2017 07:09 PM, Matteo Bruni wrote: > >>>> static void free_effect_pool(struct d3dx_effect_pool *pool) >>>> { >>>> + unsigned int i; >>>> + >>>> + for (i = 0; i < pool->table_size; ++i) >>>> + { >>>> + if (pool->shared_data[i].refcount) >>>> + ERR("Releasing pool with referenced parameters, shared data >>>> is not freed.\n"); >>>> + } >>> This can be triggered by an application so it should be a WARN(). >>> BTW, this probably needs a test. On pool destruction, the effects >>> still using it might be "unshared" i.e. data from the pool might be >>> moved back to the effects. Or maybe the effects still using the pool >>> are simply broken and accessing parameter data after the pool is >>> destroyed crashes. >>> >>> >> Yes, sure, there is one of the marginal cases to be tested more (probably >> along with more precise understanding on how shared shader parameters >> setting works when sharing non-NULL and NULL shader). Until then maybe we >> could leave something more severe than WARN here, as if we reach this point >> it most likely mean that something is wrong and it would be nice to have an >> indication right away. > I think we want to know what happens before we go ahead with the code above. > I just tested, manually releasing pool before effect just crash when I use native d3dx dll. Please see attached patch. I have to do release twice as it is referenced from 2 effects. If I release pool just once, it crashes in freeing effect.

April 25, 2017

Re: [v2 6/6] d3dx9: Support parameters sharing in effect.

by Matteo Bruni

2017-04-25 18:03 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 04/25/2017 06:35 PM, Matteo Bruni wrote: >> >> >>> +static BOOL param_zero_data_func(void *dummy, struct >>> d3dx_parameter *param) >>> +{ >>> + param->data = NULL; >>> + return FALSE; >>> +} >>> + >>> +static void d3dx_pool_release_shared_parameter(struct d3dx_parameter >>> *param) >>> +{ >>> + LONG new_refcount; >>> + >>> + if (!(param->flags & PARAMETER_FLAG_SHARED) || !param->shared_data) >>> + return; >> >> No need to check for the PARAMETER_FLAG_SHARED flag. Maybe it makes >> sense to assert that the flag is there if param->shared_data is not >> NULL. > > I need to check this flag because shared_data pointer is "unioned" with > reference_param, and removing the check should break freeing of non-shared > parameters with non-zero referenced_param. Checking just a flag won't do > either, as actually the parameter marked as shared could have nothing to be > shared with. Good point. >>> >>> static void free_effect_pool(struct d3dx_effect_pool *pool) >>> { >>> + unsigned int i; >>> + >>> + for (i = 0; i < pool->table_size; ++i) >>> + { >>> + if (pool->shared_data[i].refcount) >>> + ERR("Releasing pool with referenced parameters, shared data >>> is not freed.\n"); >>> + } >> >> This can be triggered by an application so it should be a WARN(). >> BTW, this probably needs a test. On pool destruction, the effects >> still using it might be "unshared" i.e. data from the pool might be >> moved back to the effects. Or maybe the effects still using the pool >> are simply broken and accessing parameter data after the pool is >> destroyed crashes. >> >> > Yes, sure, there is one of the marginal cases to be tested more (probably > along with more precise understanding on how shared shader parameters > setting works when sharing non-NULL and NULL shader). Until then maybe we > could leave something more severe than WARN here, as if we reach this point > it most likely mean that something is wrong and it would be nice to have an > indication right away. I think we want to know what happens before we go ahead with the code above.

April 25, 2017

Re: [v2 6/6] d3dx9: Support parameters sharing in effect.

by Paul Gofman

On 04/25/2017 06:35 PM, Matteo Bruni wrote: > >> +static BOOL param_zero_data_func(void *dummy, struct d3dx_parameter *param) >> +{ >> + param->data = NULL; >> + return FALSE; >> +} >> + >> +static void d3dx_pool_release_shared_parameter(struct d3dx_parameter *param) >> +{ >> + LONG new_refcount; >> + >> + if (!(param->flags & PARAMETER_FLAG_SHARED) || !param->shared_data) >> + return; > No need to check for the PARAMETER_FLAG_SHARED flag. Maybe it makes > sense to assert that the flag is there if param->shared_data is not > NULL. I need to check this flag because shared_data pointer is "unioned" with reference_param, and removing the check should break freeing of non-shared parameters with non-zero referenced_param. Checking just a flag won't do either, as actually the parameter marked as shared could have nothing to be shared with. >> static void free_effect_pool(struct d3dx_effect_pool *pool) >> { >> + unsigned int i; >> + >> + for (i = 0; i < pool->table_size; ++i) >> + { >> + if (pool->shared_data[i].refcount) >> + ERR("Releasing pool with referenced parameters, shared data is not freed.\n"); >> + } > This can be triggered by an application so it should be a WARN(). > BTW, this probably needs a test. On pool destruction, the effects > still using it might be "unshared" i.e. data from the pool might be > moved back to the effects. Or maybe the effects still using the pool > are simply broken and accessing parameter data after the pool is > destroyed crashes. > > Yes, sure, there is one of the marginal cases to be tested more (probably along with more precise understanding on how shared shader parameters setting works when sharing non-NULL and NULL shader). Until then maybe we could leave something more severe than WARN here, as if we reach this point it most likely mean that something is wrong and it would be nice to have an indication right away.

April 25, 2017

Re: [PATCH 2/5] ddraw: Ignore unsupported blit flags.

by Henri Verbeet

On 25 April 2017 at 17:37, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >> @@ -1577,9 +1578,26 @@ static HRESULT WINAPI DECLSPEC_HOTPATCH ddraw_surface7_Blt(IDirectDrawSurface7 * >> TRACE("iface %p, dst_rect %s, src_surface %p, src_rect %s, flags %#x, fx %p.\n", >> iface, wine_dbgstr_rect(dst_rect), src_surface, wine_dbgstr_rect(src_rect), flags, fx); >> >> - /* Check for validity of the flags here. WineD3D Has the software-opengl selection path and would have >> - * to check at 2 places, and sometimes do double checks. This also saves the call to wined3d :-) >> - */ >> + unsupported_flags = DDBLT_ALPHADEST >> + | DDBLT_ALPHADESTCONSTOVERRIDE >> + | DDBLT_ALPHADESTNEG >> + | DDBLT_ALPHADESTSURFACEOVERRIDE >> + | DDBLT_ALPHAEDGEBLEND >> + | DDBLT_ALPHASRC >> + | DDBLT_ALPHASRCCONSTOVERRIDE >> + | DDBLT_ALPHASRCNEG >> + | DDBLT_ALPHASRCSURFACEOVERRIDE >> + | DDBLT_ZBUFFER >> + | DDBLT_ZBUFFERDESTCONSTOVERRIDE >> + | DDBLT_ZBUFFERDESTOVERRIDE >> + | DDBLT_ZBUFFERSRCCONSTOVERRIDE >> + | DDBLT_ZBUFFERSRCOVERRIDE; > > Hardly going to matter but wouldn't it be better as a static const? > Probably not, it's an integer constant.

April 25, 2017

Re: [v4 PATCH 2/2] wined3d: Update only dirty regions in wined3d_device_update_texture().

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=30446 Your paranoid android. === w1064 (64 bit visual) === visual.c:7941: Test failed: Input test: Quad 3(2crd-wrongidx) returned color 0x00ff00ff, expected 0x00ff0080 visual.c:7989: Test failed: Got unexpected color 0x00007580 for quad 2 (different colors).

April 25, 2017

Re: [PATCH 2/5] ddraw: Ignore unsupported blit flags.

by Matteo Bruni

2017-04-24 23:31 GMT+02:00 Henri Verbeet <hverbeet(a)codeweavers.com>: > Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com> > --- > dlls/ddraw/surface.c | 24 +++++++++++++++++++++--- > dlls/ddraw/tests/ddraw1.c | 4 ++-- > dlls/ddraw/tests/ddraw2.c | 4 ++-- > dlls/ddraw/tests/ddraw4.c | 4 ++-- > dlls/ddraw/tests/ddraw7.c | 4 ++-- > 5 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/dlls/ddraw/surface.c b/dlls/ddraw/surface.c > index 6c4ed11..90731f0 100644 > --- a/dlls/ddraw/surface.c > +++ b/dlls/ddraw/surface.c > @@ -1570,6 +1570,7 @@ static HRESULT WINAPI DECLSPEC_HOTPATCH ddraw_surface7_Blt(IDirectDrawSurface7 * > struct ddraw_surface *dst_impl = impl_from_IDirectDrawSurface7(iface); > struct ddraw_surface *src_impl = unsafe_impl_from_IDirectDrawSurface7(src_surface); > struct wined3d_blt_fx wined3d_fx; > + DWORD unsupported_flags; > DWORD fill_colour = 0; > HRESULT hr = DD_OK; > DDBLTFX rop_fx; > @@ -1577,9 +1578,26 @@ static HRESULT WINAPI DECLSPEC_HOTPATCH ddraw_surface7_Blt(IDirectDrawSurface7 * > TRACE("iface %p, dst_rect %s, src_surface %p, src_rect %s, flags %#x, fx %p.\n", > iface, wine_dbgstr_rect(dst_rect), src_surface, wine_dbgstr_rect(src_rect), flags, fx); > > - /* Check for validity of the flags here. WineD3D Has the software-opengl selection path and would have > - * to check at 2 places, and sometimes do double checks. This also saves the call to wined3d :-) > - */ > + unsupported_flags = DDBLT_ALPHADEST > + | DDBLT_ALPHADESTCONSTOVERRIDE > + | DDBLT_ALPHADESTNEG > + | DDBLT_ALPHADESTSURFACEOVERRIDE > + | DDBLT_ALPHAEDGEBLEND > + | DDBLT_ALPHASRC > + | DDBLT_ALPHASRCCONSTOVERRIDE > + | DDBLT_ALPHASRCNEG > + | DDBLT_ALPHASRCSURFACEOVERRIDE > + | DDBLT_ZBUFFER > + | DDBLT_ZBUFFERDESTCONSTOVERRIDE > + | DDBLT_ZBUFFERDESTOVERRIDE > + | DDBLT_ZBUFFERSRCCONSTOVERRIDE > + | DDBLT_ZBUFFERSRCOVERRIDE; Hardly going to matter but wouldn't it be better as a static const?

April 25, 2017

Re: [v2 6/6] d3dx9: Support parameters sharing in effect.

by Matteo Bruni

2017-04-24 13:36 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > +struct d3dx_shared_data > +{ > + void *data; > + LONG refcount; > + struct d3dx_parameter **parameters; > + unsigned int table_size; > +}; A bit nitpicky but generally for those dynamic arrays we call "refcount" "count" and "table_size" "size". > + new_refcount = InterlockedIncrement(&pool->shared_data[i].refcount); I think you don't need the Interlocked functions for this count given that adding / removing multiple effects to a pool is expected to be racy. > + if (new_refcount >= pool->shared_data[i].table_size) > + { > + if (!pool->shared_data[i].table_size) > + { > + new_size = INITIAL_SHARED_DATA_TABLE_SIZE; > + pool->shared_data[i].parameters = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, > + sizeof(*pool->shared_data[i].parameters) * INITIAL_SHARED_DATA_TABLE_SIZE); > + } > + else > + { > + new_size = pool->shared_data[i].table_size * 2; > + pool->shared_data[i].parameters = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, > + pool->shared_data[i].parameters, > + sizeof(*pool->shared_data[i].parameters) * new_size); > + } > + pool->shared_data[i].table_size = new_size; > + } > + > + param->shared_data = &pool->shared_data[i]; > + pool->shared_data[i].parameters[new_refcount - 1] = param; > + > + TRACE("name %s, parameter idx %u, new refcount %u.\n", param->name, i, > + new_refcount); > + return; > +} Unnecessary return. > +static BOOL param_zero_data_func(void *dummy, struct d3dx_parameter *param) > +{ > + param->data = NULL; > + return FALSE; > +} > + > +static void d3dx_pool_release_shared_parameter(struct d3dx_parameter *param) > +{ > + LONG new_refcount; > + > + if (!(param->flags & PARAMETER_FLAG_SHARED) || !param->shared_data) > + return; No need to check for the PARAMETER_FLAG_SHARED flag. Maybe it makes sense to assert that the flag is there if param->shared_data is not NULL. > static void free_effect_pool(struct d3dx_effect_pool *pool) > { > + unsigned int i; > + > + for (i = 0; i < pool->table_size; ++i) > + { > + if (pool->shared_data[i].refcount) > + ERR("Releasing pool with referenced parameters, shared data is not freed.\n"); > + } This can be triggered by an application so it should be a WARN(). BTW, this probably needs a test. On pool destruction, the effects still using it might be "unshared" i.e. data from the pool might be moved back to the effects. Or maybe the effects still using the pool are simply broken and accessing parameter data after the pool is destroyed crashes.

April 25, 2017

Re: [v2 4/6] d3dx9: Reference effect pool implementation in d3dx9_base_effect structure.

by Matteo Bruni

2017-04-24 13:36 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > /*** IUnknown methods ***/ > -static HRESULT WINAPI ID3DXEffectPoolImpl_QueryInterface(ID3DXEffectPool *iface, REFIID riid, void **object) > +static HRESULT WINAPI d3dx_effect_pool_QueryInterface(ID3DXEffectPool *iface, REFIID riid, void **object) > { > TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), object); Please update / cleanup this TRACE too. > -static ULONG WINAPI ID3DXEffectPoolImpl_AddRef(ID3DXEffectPool *iface) > +static ULONG WINAPI d3dx_effect_pool_AddRef(ID3DXEffectPool *iface) > { > - struct ID3DXEffectPoolImpl *This = impl_from_ID3DXEffectPool(iface); > + struct d3dx_effect_pool *pool = impl_from_ID3DXEffectPool(iface); > > - TRACE("(%p)->(): AddRef from %u\n", This, This->ref); > + TRACE("iface %p, pool %p, ref %u.\n", iface, pool, pool->ref); Tracing both iface and pool doesn't seem useful and I don't think I've ever seen it anywhere else. Also for AddRef() we usually go with a TRACE in the form: TRACE("%p increasing refcount to %u.\n", iface, refcount); Similarly for Release(). Have a look at recent implementations of the IUnknown functions like e.g. d3d11_hull_shader_* in dlls/d3d11/shader.c.

April 25, 2017