Re: [PATCH 11/12] ddraw: cache active DCs

[Jonas Maebe]

Riccardo Bortolato wrote on Wed, 23 Sep 2015:

Disclaimer: I'm everything but an expert on Wine internals, so I'm  
just asking in case I'm not missing something obvious.
> - surface_impl = wined3d_surface_get_parent(wined3d_surface);
> - *Surface = &surface_impl->IDirectDrawSurface7_iface;
> - IDirectDrawSurface7_AddRef(*Surface);
> - TRACE("Returning surface %p.\n", Surface);
> - return DD_OK;
> - wined3d_mutex_lock();
> - LIST_FOR_EACH_ENTRY(surface_impl, &ddraw->active_dcs, struct
>
> ddraw_surface, active_dcs_entry)

Here you get a mutex lock before iterating over the active_dcs.
> -
>    if (surface_dc == hdc)
> -
>    {
> -
>        wined3d_mutex_unlock();
> -
>        *Surface = &surface_impl->IDirectDrawSurface7_iface;
> -
>        IDirectDrawSurface7_AddRef(*Surface);
> -
>        TRACE("Returning surface %p.\n", Surface);
> -
>        return DD_OK;
> -
>    }

Here you free the lock before increasing the reference count. At first  
sight, it would seem that you either don't need the lock while  
iterating, or that you should increase the reference count before  
releasing the lock in order to avoid races (unless the lock is for a  
completely unrelated reason)
> --- a/dlls/ddraw/surface.c
> +++ b/dlls/ddraw/surface.c
> @@ -2176,7 +2176,9 @@ static HRESULT WINAPI  
> ddraw_surface7_GetDC(IDirectDrawSurface7 *iface, HDC *hdc)
>              if(hdc) *hdc = NULL;
>              return DDERR_INVALIDPARAMS;
> -
>    default: return hr;
> -
>    default:
> -
>        list_add_head(&surface->ddraw->active_dcs,  
>
> &surface->active_dcs_entry);

Here you add something to the actives_dcs list without locking, so  
this can race with the iterating (unless this is implicitly  
synchronised in another way I'm not aware of).
> @@ -2246,6 +2248,8 @@ static HRESULT WINAPI  
> ddraw_surface7_ReleaseDC(IDirectDrawSurface7 *iface, HDC h
>          hr = ddraw_surface_update_frontbuffer(surface, NULL, FALSE);
>      wined3d_mutex_unlock();
> - list_remove(&surface->active_dcs_entry);
> - return hr;
>
> }

Similarly, here you remove an item after releasing the lock, so it can  
race with both adding and iterating (same caveat).

Jonas

[PATCH 11/12] ddraw: cache active DCs

[Riccardo Bortolato]

The idea behind this is that only wined3d calls need to lock that mutex.

Once we come back to ddraw, we can release it.

Ciao,
Riccardo

2015-09-23 11:25 GMT+02:00 Jonas Maebe :
> Riccardo Bortolato wrote on Wed, 23 Sep 2015:
>
> Disclaimer: I'm everything but an expert on Wine internals, so I'm just
> asking in case I'm not missing something obvious.
> > - surface_impl = wined3d_surface_get_parent(wined3d_surface);
> > - *Surface = &surface_impl->IDirectDrawSurface7_iface;
> > - IDirectDrawSurface7_AddRef(*Surface);
> > - TRACE("Returning surface %p.\n", Surface);
> > - return DD_OK;
> > - wined3d_mutex_lock();
> > - LIST_FOR_EACH_ENTRY(surface_impl, &ddraw->active_dcs, struct
> >
> > ddraw_surface, active_dcs_entry)
>
> Here you get a mutex lock before iterating over the active_dcs.
> > -
> >    if (surface_dc == hdc)
> > -
> >    {
> > -
> >        wined3d_mutex_unlock();
> > -
> >        *Surface = &surface_impl->IDirectDrawSurface7_iface;
> > -
> >        IDirectDrawSurface7_AddRef(*Surface);
> > -
> >        TRACE("Returning surface %p.\n", Surface);
> > -
> >        return DD_OK;
> > -
> >    }
>
> Here you free the lock before increasing the reference count. At first
> sight, it would seem that you either don't need the lock while iterating, or
> that you should increase the reference count before releasing the lock in
> order to avoid races (unless the lock is for a completely unrelated reason)
> > --- a/dlls/ddraw/surface.c
> > +++ b/dlls/ddraw/surface.c
> > @@ -2176,7 +2176,9 @@ static HRESULT WINAPI
> > ddraw_surface7_GetDC(IDirectDrawSurface7 *iface, HDC *hdc)
> >              if(hdc) *hdc = NULL;
> >              return DDERR_INVALIDPARAMS;
> > -
> >    default: return hr;
> > -
> >    default:
> > -
> >        list_add_head(&surface->ddraw->active_dcs,
> >
> > &surface->active_dcs_entry);
>
> Here you add something to the actives_dcs list without locking, so this can
> race with the iterating (unless this is implicitly synchronised in another
> way I'm not aware of).
> > @@ -2246,6 +2248,8 @@ static HRESULT WINAPI
> > ddraw_surface7_ReleaseDC(IDirectDrawSurface7 *iface, HDC h
> >          hr = ddraw_surface_update_frontbuffer(surface, NULL, FALSE);
> >      wined3d_mutex_unlock();
> > - list_remove(&surface->active_dcs_entry);
> > - return hr;
> >
> > }
>
> Similarly, here you remove an item after releasing the lock, so it can race
> with both adding and iterating (same caveat).
>
> Jonas