[PATCH v2 1/2] win32u: Don't pass physdev to wine_get_wgl_driver() driver function.

[Paul Gofman]

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?