[PATCH v2 2/6] ddraw: Create a separate texture for drawing with bindable sysmem surfaces.

[Henri Verbeet]

On Fri, 26 Feb 2021 at 00:20, Paul Gofman <pgofman at codeweavers.com> wrote:
>  dlls/ddraw/ddraw_private.h |  55 ++++++++++++
>  dlls/ddraw/device.c        |  18 ++--
>  dlls/ddraw/surface.c       | 167 +++++++++++++++++++++++++++++++------
>  dlls/ddraw/tests/ddraw1.c  |   1 -
>  dlls/ddraw/tests/ddraw2.c  |   1 -
>  dlls/ddraw/tests/ddraw4.c  |   1 -
>  dlls/ddraw/tests/ddraw7.c  |   7 +-
>  7 files changed, 207 insertions(+), 43 deletions(-)
>
Broadly this seems fine. Perhaps this can be split a little though:
    - Allow creation of system memory textures/RTs/DSs by creating the
second texture. At this point you could simply synchronise both
surfaces before and after each draw.
    - Tests
    - CreateDevice() restrictions.
    - SetRenderTarget() restrictions.
    - Better location tracking. And this could possibly be split
itself; e.g. ddraw_surface_get_any_texture() is essentially a further
optimisation.

> @@ -6951,6 +6951,12 @@ static HRESULT d3d_device_init(struct d3d_device *device, struct ddraw *ddraw, c
>      device->hardware_device = IsEqualGUID(&IID_IDirect3DTnLHalDevice, guid)
>              || IsEqualGUID(&IID_IDirect3DHALDevice, guid);
>
> +    if (device->hardware_device && !(target->surface_desc.ddsCaps.dwCaps & DDSCAPS_VIDEOMEMORY))
> +    {
> +        WARN("Surface %p is not in video memory.\n", target);
> +        return D3DERR_SURFACENOTINVIDMEM;
> +    }
> +
>      if (outer_unknown)
>          device->outer_unknown = outer_unknown;
>      else
> @@ -7038,12 +7044,6 @@ HRESULT d3d_device_create(struct ddraw *ddraw, const GUID *guid, struct ddraw_su
>          return DDERR_OUTOFMEMORY;
>      }
>
> -    if (!(target->surface_desc.ddsCaps.dwCaps & DDSCAPS_VIDEOMEMORY))
> -    {
> -        WARN("Surface %p is not in video memory.\n", target);
> -        return D3DERR_SURFACENOTINVIDMEM;
> -    }
> -
Was there any particular reason to move this? It's fine if there's a
reason, but on first sight it seems somewhat arbitrary.

> @@ -121,7 +122,9 @@ HRESULT ddraw_surface_update_frontbuffer(struct ddraw_surface *surface,
>          return hr;
>      }
>
> -    if (FAILED(hr = wined3d_texture_get_dc(surface->wined3d_texture, surface->sub_resource_idx, &surface_dc)))
> +    src_texture = ddraw_surface_get_default_texture(surface, DDRAW_SURFACE_READ);
> +
> +    if (FAILED(hr = wined3d_texture_get_dc(src_texture, surface->sub_resource_idx, &surface_dc)))
>      {
>          ERR("Failed to get surface DC, hr %#x.\n", hr);
>          return hr;
I don't think that's quite right in the "read" case.

> +static void ddraw_surface_create_draw_texture(struct ddraw_surface *surface)
> +{
> +    DDSURFACEDESC2 *desc = &surface->surface_desc;
> +    struct wined3d_resource *draw_texture_resource;
> +    struct wined3d_resource_desc wined3d_desc;
> +    unsigned int i, layer_count, level_count;
> +    struct wined3d_texture *draw_texture;
> +    struct ddraw_surface *parent;
> +    unsigned int bind_flags;
> +    HRESULT hr;
> +
> +    if (!(desc->ddsCaps.dwCaps & DDSCAPS_SYSTEMMEMORY))
> +        return;
> +
Shouldn't we also check whether we have a hardware device or not here?

> @@ -6695,7 +6810,7 @@ struct wined3d_rendertarget_view *ddraw_surface_get_rendertarget_view(struct ddr
>      if (surface->wined3d_rtv)
>          return surface->wined3d_rtv;
>
> -    if (FAILED(hr = wined3d_rendertarget_view_create_from_sub_resource(surface->wined3d_texture,
> +    if (FAILED(hr = wined3d_rendertarget_view_create_from_sub_resource(ddraw_surface_get_draw_texture(surface, 0),
>              surface->sub_resource_idx, surface, &ddraw_view_wined3d_parent_ops, &surface->wined3d_rtv)))
>      {
>          ERR("Failed to create rendertarget view, hr %#x.\n", hr);
The ddraw_surface_get_draw_texture() here seems a little awkward,
since typically that function implies synchronising texture contents,
but we don't care about that here. Not wrong though.