Re: [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