Support for D3DQUERY_TIMESTAMP* queries...

[Adam Martinson]

Feedback please.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Am 03.05.2011 um 16:51 schrieb Adam Martinson:
> +#ifndef WINE_NO_TRACE_MSGS
> +        for (; i < query->n_ids; ++i)
> +            TRACE("Allocated timestamp query %u in context %p.\n", query->ids[i], context);
> +#endif
> use if(TRACE_ON(d3d)) instead. It'll be statically FALSE if there are no trace messages, and you also avoid the loop at runtime when traces aren't on.
> +struct _wined3d_timestamp_query
> +{
> +    struct list             entry;
> +    struct wined3d_context *context;
> +    int                     n_ids;
> +    GLuint                  ids[1];
> +};
> I'm not sure if this is necessary, I think we can just give struct wined3d_timestamp_query a ids[3] field.
> +typedef struct D3D10_QUERY_DATA_TIMESTAMP_DISJOINT {
> +    UINT64 Frequency;
> +    BOOL Disjoint;
> +} D3D10_QUERY_DATA_TIMESTAMP_DISJOINT;
> Wrong code style, it looks like you copypasted .idl code into a .c file. Please define this in wined3d.idl using WINED3D_QUERY_... names. The wined3d.idl file will soon be gone and replaced with a plain .h header.

If you don't need those types for implementing the d3d9 queries ignore the d3d10 stuff for now.
> +                static BOOL warned = FALSE;
> +                if (!warned)
> +                {
> +                    WARN("GL_ARB_timer_query not supported, returning WINED3DERR_NOTAVAILABLE\n");
> +                    warned = TRUE;
> +                }
> I don't think we have to limit WARNs to one output only. WARNs aren't shown by default. This kind of limiting is used for FIXMEs only to make sure repeated FIXMEs don't spam the terminal and hide other output.

I have to go now, I'll be back later. If I have time and am not tired I'll read this a bit more and check the tests. But probably Henri has sent some comments by then too.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)

iQIcBAEBAgAGBQJNwDRoAAoJEN0/YqbEcdMwC+oQAJGAxuDNh2bGbBNGuFs5uxXo
Jkf7+Kgb4aXuhYReGzCX5l1nYK82UBMGWC4j53tKWnziw4VueHuyuKwpmHO+EpRk
cjgz5pi4WBT3EGTuvCbnQ9ayOKRI76OKa45mD0uFCMvsM/cHKh0QcUJD4SpkmHLM
97sZuNpBuWy6vqNTfSgTirpQCI/gBUlQZVHKBesVhsJJ7yVZG5TgZjARKasYzoVg
Q6hHKK6qWby2NnawYpKQp51ie0OV3W65JZPHQ3fn+3ogrCcsL27u2SQE8MqHhKt4
PYxzGRfl+LCjdIIJ7UekXKawFlh6DqCF+j+9KEonJwSVzduj96Y+j0La5QML9eFW
Kjk53LLgzrb8kQcSmMS6YLNot158am4twu9uE8Lf6KjO5ADXIdRqdISFCipqecu/
cJc0IyYEaKgcrFaRcCRuPtZreXaK8QjH0n7KzvEW9I+74UN0cegTIN37yIeAwev+
oPYTwGr6fUHtOUQ/8t/jYMvgm0Up8kLnK9OU9LWJPcTOVh+BtyeKI2vmg/QPvCCk
lbPLuZD59JHdxyBDrKqYqyxIRcX6aLGctnRH5t+QVh5RHLmr8rcbZDm3JIEsDyHN
dUE41KiDa/oad76pSW5OL2NU+QHLbVgnJ6IQjGQSdH7gF4zkLHX8VYTaWdFqYRMy
U2KFQKkMYUCWBaGxaS4B
=jvdN
-----END PGP SIGNATURE-----

[Henri Verbeet]

On 3 May 2011 16:51, Adam Martinson <amartinson(a)codeweavers.com> wrote:
> Feedback please.

This is in addition to what Stefan already said:
> +/* GL_ARB_timer_query */
> +#ifndef GL_ARB_timer_query
> +#define GL_ARB_timer_query 1
> +#define GL_TIME_ELAPSED_ARB                                 0x88BF
> +#define GL_TIMESTAMP_ARB                                    0x8E28
> +#endif
> +
> +typedef void (WINE_GLAPI *PFNGLQUERYCOUNTERPROC) (GLuint id, GLenum target);
> +typedef void (WINE_GLAPI *PFNGLGETQUERYOBJECTI64VPROC) (GLuint id, GLenum pname, GLint64 *params);
> +typedef void (WINE_GLAPI *PFNGLGETQUERYOBJECTUI64VPROC) (GLuint id, GLenum pname, GLuint64 *params);

Where did you get these? The style reminds me of glext.h.
> @@ -95,26 +95,31 @@ static HRESULT WINAPI IDirect3DQuery9Impl_GetDevice(IDirect3DQuery9 *iface,
>  static D3DQUERYTYPE WINAPI IDirect3DQuery9Impl_GetType(IDirect3DQuery9 *iface)
>  {
>      IDirect3DQuery9Impl *This = impl_from_IDirect3DQuery9(iface);
> -    HRESULT hr;
> +    D3DQUERYTYPE ret;
>
> TRACE("iface %p.\n", iface);
>
> wined3d_mutex_lock();
> -    hr = wined3d_query_get_type(This->wineD3DQuery);
> +    ret = wined3d_query_get_type(This->wineD3DQuery);
>      wined3d_mutex_unlock();
>
> -    return hr;
> +    return ret;
>  }
> This makes sense, but should clearly be a separate patch. If you're
> changing the name anyway, "type" would probably be a more reasonable
> name than "ret".
> +struct _wined3d_timestamp_query
> +{
> +    struct list             entry;
> +    struct wined3d_context *context;
> +    int                     n_ids;
> +    GLuint                  ids[1];
> +};
> ...
> +            query->extendedData = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct _wined3d_timestamp_query));
> ...
> +struct wined3d_timestamp_query
> +{
> +    struct list             entry;
> +    struct wined3d_context *context;
> +    int                     n_ids;
> +    GLuint                  ids[];
> +};
> That's ugly and fragile.

Please try to write coherent patches. Patch 4/7 in particular looks
like it was just copy-pasted together. Patch 5/7 could have easily
been at least 4 separate patches.

[Adam Martinson]

OK, I think I addressed those issues, let me know how these look.

[Henri Verbeet]

On 4 May 2011 20:38, Adam Martinson <amartinson(a)codeweavers.com> wrote:
> OK, I think I addressed those issues, let me know how these look.

> +#define GL_TIME_ELAPSED_ARB                                 0x88BF
> +#define GL_TIMESTAMP_ARB                                    0x8E28
> Most of wined3d uses lowercase hex literals.
> +    type = wined3d_query_get_type(This->wineD3DQuery);
> +    if (type == D3DQUERYTYPE_TIMESTAMPDISJOINT)
> That's not a wined3d type. Also, this patch doesn't make sense on its own.
> +static void test_timestamp_queries(IDirect3D9 *pD3d, HWND hwnd)
> Please don't use names like pD3d, pDevice, etc. There's also still a
> fair amount of inconsistencies in this patch.

Patch 7/12 just adds dead code. It also seems to me the way of
splitting the d3d9 and d3d10 disjoint queries is more complicated than
needed. The difference can be handled completely in d3d10, since
"ARB_TIMER_QUERY_FREQ" is just a constant that could easily just be
part of the wined3d interface.

[Adam Martinson]

On 05/09/2011 10:03 AM, Henri Verbeet wrote:
> > +static void test_timestamp_queries(IDirect3D9 *pD3d, HWND hwnd)
> > Please don't use names like pD3d, pDevice, etc. There's also still a
> > fair amount of inconsistencies in this patch.
> > Most of the places I did that I did so to match the naming conventions 
> > in the rest of the file, I think I've got that cleaned up now.  As for 
> > inconsistencies, I need something more specific...
> Patch 7/12 just adds dead code. It also seems to me the way of
> splitting the d3d9 and d3d10 disjoint queries is more complicated than
> needed. The difference can be handled completely in d3d10, since
> "ARB_TIMER_QUERY_FREQ" is just a constant that could easily just be
> part of the wined3d interface.
> Ok, I've reworked things a bit.  I removed the stuff for handling the 
> d3d10 stuff, but I think this way it should be easy to handle inside 
> d3d10 when we get there.  Let me know what you think.