[PATCH v2 1/2] win32u: Don't pass physdev to wine_get_wgl_driver() driver function.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- Supersedes 223960. v2: - added patch. Specific GL driver initialization does not depend on anything in the device or context besides making a decision to call or skip the next driver. Copying the driver constant function pointers out allows to call driver's _wine_get_wgl_driver outside of any DC lock in the next patch. dlls/win32u/dibdrv/dc.c | 11 ++++++----- dlls/win32u/driver.c | 24 +++++++++++++++++++++--- dlls/wineandroid.drv/init.c | 11 ++--------- dlls/winemac.drv/opengl.c | 4 +++- dlls/winex11.drv/init.c | 11 ++--------- include/wine/gdi_driver.h | 2 +- 6 files changed, 35 insertions(+), 28 deletions(-) diff --git a/dlls/win32u/dibdrv/dc.c b/dlls/win32u/dibdrv/dc.c index a96b613d0a3..4cc95e6b843 100644 --- a/dlls/win32u/dibdrv/dc.c +++ b/dlls/win32u/dibdrv/dc.c @@ -605,8 +605,10 @@ static struct opengl_funcs opengl_funcs = /********************************************************************** * dibdrv_wine_get_wgl_driver */ -static struct opengl_funcs * CDECL dibdrv_wine_get_wgl_driver( PHYSDEV dev, UINT version ) +static struct opengl_funcs * CDECL dibdrv_wine_get_wgl_driver( UINT version, const struct gdi_dc_funcs ***next_funcs ) { + *next_funcs = NULL; + if (version != WINE_WGL_DRIVER_VERSION) { ERR( "version mismatch, opengl32 wants %u but dibdrv has %u\n", version, WINE_WGL_DRIVER_VERSION ); @@ -1177,11 +1179,10 @@ static INT CDECL windrv_StretchDIBits( PHYSDEV dev, INT x_dst, INT y_dst, INT wi return ret; } -static struct opengl_funcs * CDECL windrv_wine_get_wgl_driver( PHYSDEV dev, UINT version ) +static struct opengl_funcs * CDECL windrv_wine_get_wgl_driver( UINT version, const struct gdi_dc_funcs ***next_funcs ) { - dev = GET_NEXT_PHYSDEV( dev, wine_get_wgl_driver ); - if (dev->funcs == &dib_driver) dev = GET_NEXT_PHYSDEV( dev, wine_get_wgl_driver ); - return dev->funcs->wine_get_wgl_driver( dev, version ); + if (*(*next_funcs + 1) == &dib_driver) ++*next_funcs; + return NULL; } static const struct gdi_dc_funcs window_driver = diff --git a/dlls/win32u/driver.c b/dlls/win32u/driver.c index 39996086c6d..f527c6c51cf 100644 --- a/dlls/win32u/driver.c +++ b/dlls/win32u/driver.c @@ -579,8 +579,10 @@ static NTSTATUS CDECL nulldrv_D3DKMTSetVidPnSourceOwner( const D3DKMT_SETVIDPNSO return STATUS_PROCEDURE_NOT_FOUND; } -static struct opengl_funcs * CDECL nulldrv_wine_get_wgl_driver( PHYSDEV dev, UINT version ) +static struct opengl_funcs * CDECL nulldrv_wine_get_wgl_driver( UINT version, const struct gdi_dc_funcs ***next_funcs ) { + *next_funcs = NULL; + return (void *)-1; } @@ -1347,13 +1349,29 @@ NTSTATUS WINAPI NtGdiDdDDICheckVidPnExclusiveOwnership( const D3DKMT_CHECKVIDPNE */ struct opengl_funcs * CDECL __wine_get_wgl_driver( HDC hdc, UINT version ) { + const struct gdi_dc_funcs *driver_funcs[11], **next_funcs; struct opengl_funcs *ret = NULL; DC * dc = get_dc_ptr( hdc ); + PHYSDEV physdev; if (dc) { - PHYSDEV physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); - ret = physdev->funcs->wine_get_wgl_driver( physdev, version ); + next_funcs = driver_funcs; + physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); + while (physdev && next_funcs - driver_funcs < ARRAY_SIZE(driver_funcs) - 1) + { + *next_funcs++ = physdev->funcs; + if (physdev->funcs == &null_driver) break; + physdev = GET_NEXT_PHYSDEV( physdev, wine_get_wgl_driver ); + } + *next_funcs = NULL; + next_funcs = driver_funcs; + while (*next_funcs) + { + ret = (*next_funcs)->wine_get_wgl_driver( version, &next_funcs ); + if (ret || !next_funcs) break; + ++next_funcs; + } release_dc_ptr( dc ); } return ret; diff --git a/dlls/wineandroid.drv/init.c b/dlls/wineandroid.drv/init.c index ed9116eb47e..3c7da3f780f 100644 --- a/dlls/wineandroid.drv/init.c +++ b/dlls/wineandroid.drv/init.c @@ -261,16 +261,9 @@ BOOL CDECL ANDROID_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devm /********************************************************************** * ANDROID_wine_get_wgl_driver */ -static struct opengl_funcs * CDECL ANDROID_wine_get_wgl_driver( PHYSDEV dev, UINT version ) +static struct opengl_funcs * CDECL ANDROID_wine_get_wgl_driver( UINT version, const struct gdi_dc_funcs ***next_funcs ) { - struct opengl_funcs *ret; - - if (!(ret = get_wgl_driver( version ))) - { - dev = GET_NEXT_PHYSDEV( dev, wine_get_wgl_driver ); - ret = dev->funcs->wine_get_wgl_driver( dev, version ); - } - return ret; + return get_wgl_driver( version ); } diff --git a/dlls/winemac.drv/opengl.c b/dlls/winemac.drv/opengl.c index daf194c2aec..b5670906e93 100644 --- a/dlls/winemac.drv/opengl.c +++ b/dlls/winemac.drv/opengl.c @@ -4632,10 +4632,12 @@ static struct opengl_funcs opengl_funcs = /********************************************************************** * macdrv_wine_get_wgl_driver */ -struct opengl_funcs * CDECL macdrv_wine_get_wgl_driver(PHYSDEV dev, UINT version) +struct opengl_funcs * CDECL macdrv_wine_get_wgl_driver(UINT version, const struct gdi_dc_funcs ***next_funcs) { static INIT_ONCE opengl_init = INIT_ONCE_STATIC_INIT; + *next_funcs = NULL; + if (version != WINE_WGL_DRIVER_VERSION) { ERR("version mismatch, opengl32 wants %u but macdrv has %u\n", version, WINE_WGL_DRIVER_VERSION); diff --git a/dlls/winex11.drv/init.c b/dlls/winex11.drv/init.c index 5b31c352a23..ebd221c2d38 100644 --- a/dlls/winex11.drv/init.c +++ b/dlls/winex11.drv/init.c @@ -312,16 +312,9 @@ static INT CDECL X11DRV_ExtEscape( PHYSDEV dev, INT escape, INT in_count, LPCVOI /********************************************************************** * X11DRV_wine_get_wgl_driver */ -static struct opengl_funcs * CDECL X11DRV_wine_get_wgl_driver( PHYSDEV dev, UINT version ) +static struct opengl_funcs * CDECL X11DRV_wine_get_wgl_driver( UINT version, const struct gdi_dc_funcs ***next_funcs ) { - struct opengl_funcs *ret; - - if (!(ret = get_glx_driver( version ))) - { - dev = GET_NEXT_PHYSDEV( dev, wine_get_wgl_driver ); - ret = dev->funcs->wine_get_wgl_driver( dev, version ); - } - return ret; + return get_glx_driver( version ); } /********************************************************************** diff --git a/include/wine/gdi_driver.h b/include/wine/gdi_driver.h index 567a6c21608..0e3bbade6c4 100644 --- a/include/wine/gdi_driver.h +++ b/include/wine/gdi_driver.h @@ -159,7 +159,7 @@ struct gdi_dc_funcs BOOL (CDECL *pUnrealizePalette)(HPALETTE); NTSTATUS (CDECL *pD3DKMTCheckVidPnExclusiveOwnership)(const D3DKMT_CHECKVIDPNEXCLUSIVEOWNERSHIP *); NTSTATUS (CDECL *pD3DKMTSetVidPnSourceOwner)(const D3DKMT_SETVIDPNSOURCEOWNER *); - struct opengl_funcs * (CDECL *wine_get_wgl_driver)(PHYSDEV,UINT); + struct opengl_funcs * (CDECL *wine_get_wgl_driver)(UINT,const struct gdi_dc_funcs ***); /* priority order for the driver on the stack */ UINT priority; -- 2.34.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - call actual driver init function outside of any locks. dlls/win32u/dc.c | 19 ------------- dlls/win32u/driver.c | 55 +++++++++++++++++++++++-------------- dlls/win32u/ntgdi_private.h | 21 ++++++++++++++ 3 files changed, 56 insertions(+), 39 deletions(-) diff --git a/dlls/win32u/dc.c b/dlls/win32u/dc.c index aed2d3684a7..cb38c4161ee 100644 --- a/dlls/win32u/dc.c +++ b/dlls/win32u/dc.c @@ -63,25 +63,6 @@ static const struct gdi_obj_funcs dc_funcs = }; -static inline DC *get_dc_obj( HDC hdc ) -{ - DWORD type; - DC *dc = get_any_obj_ptr( hdc, &type ); - if (!dc) return NULL; - - switch (type) - { - case NTGDI_OBJ_DC: - case NTGDI_OBJ_MEMDC: - case NTGDI_OBJ_ENHMETADC: - return dc; - default: - GDI_ReleaseObj( hdc ); - SetLastError( ERROR_INVALID_HANDLE ); - return NULL; - } -} - /* alloc DC_ATTR from a pool of memory accessible from client */ static DC_ATTR *alloc_dc_attr(void) { diff --git a/dlls/win32u/driver.c b/dlls/win32u/driver.c index f527c6c51cf..eda49ab2700 100644 --- a/dlls/win32u/driver.c +++ b/dlls/win32u/driver.c @@ -1350,29 +1350,44 @@ NTSTATUS WINAPI NtGdiDdDDICheckVidPnExclusiveOwnership( const D3DKMT_CHECKVIDPNE struct opengl_funcs * CDECL __wine_get_wgl_driver( HDC hdc, UINT version ) { const struct gdi_dc_funcs *driver_funcs[11], **next_funcs; - struct opengl_funcs *ret = NULL; - DC * dc = get_dc_ptr( hdc ); + struct opengl_funcs *ret; PHYSDEV physdev; + DC * dc; - if (dc) + if (!(dc = get_dc_obj( hdc ))) return NULL; + if (dc->attr->disabled) { - next_funcs = driver_funcs; - physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); - while (physdev && next_funcs - driver_funcs < ARRAY_SIZE(driver_funcs) - 1) - { - *next_funcs++ = physdev->funcs; - if (physdev->funcs == &null_driver) break; - physdev = GET_NEXT_PHYSDEV( physdev, wine_get_wgl_driver ); - } - *next_funcs = NULL; - next_funcs = driver_funcs; - while (*next_funcs) - { - ret = (*next_funcs)->wine_get_wgl_driver( version, &next_funcs ); - if (ret || !next_funcs) break; - ++next_funcs; - } - release_dc_ptr( dc ); + GDI_ReleaseObj( hdc ); + return NULL; + } + if ((ret = dc->gl_funcs)) + { + GDI_ReleaseObj( hdc ); + return ret; } + + next_funcs = driver_funcs; + physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); + while (physdev && next_funcs - driver_funcs < ARRAY_SIZE(driver_funcs) - 1) + { + *next_funcs++ = physdev->funcs; + if (physdev->funcs == &null_driver) break; + physdev = GET_NEXT_PHYSDEV( physdev, wine_get_wgl_driver ); + } + *next_funcs = NULL; + GDI_ReleaseObj( hdc ); + + next_funcs = driver_funcs; + while (*next_funcs) + { + ret = (*next_funcs)->wine_get_wgl_driver( version, &next_funcs ); + if (ret || !next_funcs) break; + ++next_funcs; + } + + if (!(dc = get_dc_obj( hdc ))) return NULL; + if (dc->gl_funcs) ret = dc->gl_funcs; + else dc->gl_funcs = ret; + GDI_ReleaseObj( hdc ); return ret; } diff --git a/dlls/win32u/ntgdi_private.h b/dlls/win32u/ntgdi_private.h index bfeb4557da7..54a4dc17eaf 100644 --- a/dlls/win32u/ntgdi_private.h +++ b/dlls/win32u/ntgdi_private.h @@ -87,6 +87,8 @@ typedef struct tagDC XFORM xformVport2World; /* Inverse of the above transformation */ BOOL vport2WorldValid; /* Is xformVport2World valid? */ RECT bounds; /* Current bounding rect */ + + struct opengl_funcs *gl_funcs; /* Opengl driver functions */ } DC; /* Certain functions will do no further processing if the driver returns this. @@ -660,6 +662,25 @@ static inline void copy_bitmapinfo( BITMAPINFO *dst, const BITMAPINFO *src ) memcpy( dst, src, get_dib_info_size( src, DIB_RGB_COLORS )); } +static inline DC *get_dc_obj( HDC hdc ) +{ + DWORD type; + DC *dc = get_any_obj_ptr( hdc, &type ); + if (!dc) return NULL; + + switch (type) + { + case NTGDI_OBJ_DC: + case NTGDI_OBJ_MEMDC: + case NTGDI_OBJ_ENHMETADC: + return dc; + default: + GDI_ReleaseObj( hdc ); + SetLastError( ERROR_INVALID_HANDLE ); + return NULL; + } +} + extern void CDECL free_heap_bits( struct gdi_image_bits *bits ) DECLSPEC_HIDDEN; void set_gdi_client_ptr( HGDIOBJ handle, void *ptr ) DECLSPEC_HIDDEN; -- 2.34.1
Hi Paul, On 2/1/22 18:32, Paul Gofman wrote:
@@ -1347,13 +1349,29 @@ NTSTATUS WINAPI NtGdiDdDDICheckVidPnExclusiveOwnership( const D3DKMT_CHECKVIDPNE */ struct opengl_funcs * CDECL __wine_get_wgl_driver( HDC hdc, UINT version ) { + const struct gdi_dc_funcs *driver_funcs[11], **next_funcs; struct opengl_funcs *ret = NULL; DC * dc = get_dc_ptr( hdc ); + PHYSDEV physdev;
if (dc) { - PHYSDEV physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); - ret = physdev->funcs->wine_get_wgl_driver( physdev, version ); + next_funcs = driver_funcs; + physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); + while (physdev && next_funcs - driver_funcs < ARRAY_SIZE(driver_funcs) - 1) + { + *next_funcs++ = physdev->funcs; + if (physdev->funcs == &null_driver) break; + physdev = GET_NEXT_PHYSDEV( physdev, wine_get_wgl_driver ); + } + *next_funcs = NULL; + next_funcs = driver_funcs; + while (*next_funcs) + { + ret = (*next_funcs)->wine_get_wgl_driver( version, &next_funcs ); + if (ret || !next_funcs) break; + ++next_funcs; + }
If the current PHYSDEV locking does not fit what we need here, it may be indeed a good idea to use something different here. It seems unique in this aspect anyway. However, it doesn't seem like hacking around PHYSDEV pointers like this is really what we need here. You could, for example, move wine_get_wgl from gdi_dc_funcs to user_driver_funcs (similar to what 9596e7f2a4e972c35efdca38b01d068bfc055d36 did for Vulkan) and call it with no PHYSDEV involved. __wine_get_wgl_driver could look roughly like this: if ((hwnd = NtUserWindowFromDC( hdc )) && (ret = user_driver->wine_get_wgl_driver( hwnd, version )) return ret; if (selected correct bitmap on hdc) return get_osmesa_driver( version ); return (void *)-1; How does it look to you? Thanks, Jacek
On 2/3/22 17:25, Jacek Caban wrote:
Hi Paul,
On 2/1/22 18:32, Paul Gofman wrote:
@@ -1347,13 +1349,29 @@ NTSTATUS WINAPI NtGdiDdDDICheckVidPnExclusiveOwnership( const D3DKMT_CHECKVIDPNE */ struct opengl_funcs * CDECL __wine_get_wgl_driver( HDC hdc, UINT version ) { + const struct gdi_dc_funcs *driver_funcs[11], **next_funcs; struct opengl_funcs *ret = NULL; DC * dc = get_dc_ptr( hdc ); + PHYSDEV physdev; if (dc) { - PHYSDEV physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); - ret = physdev->funcs->wine_get_wgl_driver( physdev, version ); + next_funcs = driver_funcs; + physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); + while (physdev && next_funcs - driver_funcs < ARRAY_SIZE(driver_funcs) - 1) + { + *next_funcs++ = physdev->funcs; + if (physdev->funcs == &null_driver) break; + physdev = GET_NEXT_PHYSDEV( physdev, wine_get_wgl_driver ); + } + *next_funcs = NULL; + next_funcs = driver_funcs; + while (*next_funcs) + { + ret = (*next_funcs)->wine_get_wgl_driver( version, &next_funcs ); + if (ret || !next_funcs) break; + ++next_funcs; + }
If the current PHYSDEV locking does not fit what we need here, it may be indeed a good idea to use something different here. It seems unique in this aspect anyway. However, it doesn't seem like hacking around PHYSDEV pointers like this is really what we need here. You could, for example, move wine_get_wgl from gdi_dc_funcs to user_driver_funcs (similar to what 9596e7f2a4e972c35efdca38b01d068bfc055d36 did for Vulkan) and call it with no PHYSDEV involved. __wine_get_wgl_driver could look roughly like this:
if ((hwnd = NtUserWindowFromDC( hdc )) && (ret = user_driver->wine_get_wgl_driver( hwnd, version )) return ret;
if (selected correct bitmap on hdc) return get_osmesa_driver( version );
return (void *)-1;
How does it look to you?
I am not quite sure, but this looks a bit convoluted to me. First in terms of getting user hwnd from dc (which can work as I understand but brings some dependency on the not directly related locking and logic). Then heuristics in __wine_get_wgl_driver() for guessing if osmesa should be used while currently it is more straightforward as all that knowledge is already incorporated into the driver chain, there is the logic spread elsewhere which controls that. I realize that hacking out driver function pointers indeed does not look nice. Maybe if getting hwnd is really not an issue (I assume user_callbacks->pWindowFromDC() can be used for that?) we can indicate in the DC structure if osmesa should be used (to be set when the dibdrv is added to device chain not through windrv)? This way we won't be duplicating that logic.
On 2/3/22 18:41, Paul Gofman wrote:
On 2/3/22 17:25, Jacek Caban wrote:
Hi Paul,
On 2/1/22 18:32, Paul Gofman wrote:
@@ -1347,13 +1349,29 @@ NTSTATUS WINAPI NtGdiDdDDICheckVidPnExclusiveOwnership( const D3DKMT_CHECKVIDPNE */ struct opengl_funcs * CDECL __wine_get_wgl_driver( HDC hdc, UINT version ) { + const struct gdi_dc_funcs *driver_funcs[11], **next_funcs; struct opengl_funcs *ret = NULL; DC * dc = get_dc_ptr( hdc ); + PHYSDEV physdev; if (dc) { - PHYSDEV physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); - ret = physdev->funcs->wine_get_wgl_driver( physdev, version ); + next_funcs = driver_funcs; + physdev = GET_DC_PHYSDEV( dc, wine_get_wgl_driver ); + while (physdev && next_funcs - driver_funcs < ARRAY_SIZE(driver_funcs) - 1) + { + *next_funcs++ = physdev->funcs; + if (physdev->funcs == &null_driver) break; + physdev = GET_NEXT_PHYSDEV( physdev, wine_get_wgl_driver ); + } + *next_funcs = NULL; + next_funcs = driver_funcs; + while (*next_funcs) + { + ret = (*next_funcs)->wine_get_wgl_driver( version, &next_funcs ); + if (ret || !next_funcs) break; + ++next_funcs; + }
If the current PHYSDEV locking does not fit what we need here, it may be indeed a good idea to use something different here. It seems unique in this aspect anyway. However, it doesn't seem like hacking around PHYSDEV pointers like this is really what we need here. You could, for example, move wine_get_wgl from gdi_dc_funcs to user_driver_funcs (similar to what 9596e7f2a4e972c35efdca38b01d068bfc055d36 did for Vulkan) and call it with no PHYSDEV involved. __wine_get_wgl_driver could look roughly like this:
if ((hwnd = NtUserWindowFromDC( hdc )) && (ret = user_driver->wine_get_wgl_driver( hwnd, version )) return ret;
if (selected correct bitmap on hdc) return get_osmesa_driver( version );
return (void *)-1;
How does it look to you?
I am not quite sure, but this looks a bit convoluted to me. First in terms of getting user hwnd from dc (which can work as I understand but brings some dependency on the not directly related locking and logic). Then heuristics in __wine_get_wgl_driver() for guessing if osmesa should be used while currently it is more straightforward as all that knowledge is already incorporated into the driver chain, there is the logic spread elsewhere which controls that.
I realize that hacking out driver function pointers indeed does not look nice. Maybe if getting hwnd is really not an issue (I assume user_callbacks->pWindowFromDC() can be used for that?) we can indicate in the DC structure if osmesa should be used (to be set when the dibdrv is added to device chain not through windrv)? This way we won't be duplicating that logic.
I've got another idea. Given we don't actually need anything from the DC when getting GL functions from the drivers besides the DC ptr don't get freed, can't we just GDI_inc_ref_count() on the DC handle and proceed to physdev->funcs->wine_get_wgl_driver() without holding any locks and without changing anything in the current physdev drivers?
participants (2)
-
Jacek Caban -
Paul Gofman