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

[Henri Verbeet]

On Tue, 2 Mar 2021 at 18:03, Paul Gofman  wrote:
> On 3/2/21 19:16, Henri Verbeet wrote:
> > > @@ -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.
>
> Checking the guids seemed a bit more reasonable to me in
> d3d_device_init() which fills all the fields of the struct d3d_device,
> including hardware_device, so I moved the check after the estimation of
> that field. Otherwise, I could check guids in d3d_device_create() and
> pass hardware_device BOOL to d3d_device_init. Should I change that?

Oh, I see, we don't have device->hardware_device yet in
d3d_device_create(). That's fine, no need to change it.
> > > +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?
>
> Do you mean checking if we have a hardware renderer at all and skip the
> attempt to create draw_texture if we don't? But wouldn't it be easier to
> just let it fail without adding an extra check?

No, the other way around, there's no need to create the extra texture
if we do have a hardware device. Granted, that only makes a difference
if applications create system memory surfaces with the relevant caps
on a hardware device, but well, ddraw applications.
> > > @@ -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.
>
> But there are flags which are 0. Do you think it is cleaner to just
> select the texture with '?' in this case?

Perhaps, I haven't quite made up my mind yet. My first instinct would
be to write

if (surface->draw_texture)
        wined3d_texture = surface->draw_texture;
    else
        wined3d_texture = surface->wined3d_texture;

but there's something to be said for always going through
ddraw_surface_get_draw_texture() too.