Re: [v5 PATCH 1/3] wined3d: Send texture updates through the command stream.

[Stefan Dösinger]

Am 2017-05-24 um 15:49 schrieb Masanori Kakura:
> +        for (j = 0; j < layer_count; ++j)
> +        {
> +            wined3d_cs_emit_blt_sub_resource(cs,
> +                    &dst_texture->resource, j * dst_level_count + i, &box,
> +                    &src_texture->resource, j * src_level_count + i + src_skip_levels, &box,
> +                    0, NULL, WINED3D_TEXF_POINT);
> +        }
> I don't like emitting new operations in the CS handler instead of doing
> the actual operation. Maybe this can be avoided with some refactoring of
> wined3d_cs_exec_blt_sub_resource?

[v5 PATCH 1/3] wined3d: Send texture updates through the command stream.

[Henri Verbeet]

On 24 May 2017 at 16:08, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
> Am 2017-05-24 um 15:49 schrieb Masanori Kakura:
> > +        for (j = 0; j < layer_count; ++j)
> > +        {
> > +            wined3d_cs_emit_blt_sub_resource(cs,
> > +                    &dst_texture->resource, j * dst_level_count + i, &box,
> > +                    &src_texture->resource, j * src_level_count + i + src_skip_levels, &box,
> > +                    0, NULL, WINED3D_TEXF_POINT);
> > +        }
> > I don't like emitting new operations in the CS handler instead of doing
> > the actual operation. Maybe this can be avoided with some refactoring of
> > wined3d_cs_exec_blt_sub_resource?

Yeah, that's a consideration, although it probably shouldn't be the
focus right now.

[v5 PATCH 1/3] wined3d: Send texture updates through the command stream.

[Masanori Kakura]

On Wed, 24 May 2017 16:08:30 +0200
Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
> Am 2017-05-24 um 15:49 schrieb Masanori Kakura:
> > +        for (j = 0; j < layer_count; ++j)
> > +        {
> > +            wined3d_cs_emit_blt_sub_resource(cs,
> > +                    &dst_texture->resource, j * dst_level_count + i, &box,
> > +                    &src_texture->resource, j * src_level_count + i + src_skip_levels, &box,
> > +                    0, NULL, WINED3D_TEXF_POINT);
> > +        }
> > I don't like emitting new operations in the CS handler instead of doing
> > the actual operation. Maybe this can be avoided with some refactoring of
> > wined3d_cs_exec_blt_sub_resource?

I wrote 2 new patches but I'm not sure which is better.

----- Patch 1 (modify wined3d_cs_exec_blt_sub_resource) -----
static void wined3d_cs_exec_blt_sub_resource(struct wined3d_cs *cs, const void *data)
{
    /* ... */

error:
    if (op->opcode == WINED3D_CS_OP_BLT_SUB_RESOURCE)
    {
        if (op->src_resource)
            wined3d_resource_release(op->src_resource);
        wined3d_resource_release(op->dst_resource);
    }
}

static void wined3d_cs_exec_update_texture(struct wined3d_cs *cs, const void *data)
{
    /* ... */

    for (i = 0; i < level_count; ++i)
    {
        width = wined3d_texture_get_level_width(dst_texture, i);
        height = wined3d_texture_get_level_height(dst_texture, i);
        depth = wined3d_texture_get_level_depth(dst_texture, i);
        wined3d_box_set(&box, 0, 0, width, height, 0, depth);

        for (j = 0; j < layer_count; ++j)
        {
            const struct wined3d_cs_blt_sub_resource op_blt = {WINED3D_CS_OP_UPDATE_TEXTURE,
                    &dst_texture->resource, j * dst_level_count + i, box,
                    &src_texture->resource, j * src_level_count + i + src_skip_levels, box,
                    0, NULL, WINED3D_TEXF_POINT};

            wined3d_cs_exec_blt_sub_resource(cs, &op_blt);
        }
    }

    wined3d_resource_release(&src_texture->resource);
    wined3d_resource_release(&dst_texture->resource);
}

----- Patch 2 (write codes directly) -----
static void wined3d_cs_exec_update_texture(struct wined3d_cs *cs, const void *data)
{
    /* ... */

    /* Update every surface level of the texture. */
    for (i = 0; i < level_count; ++i)
    {
        width = wined3d_texture_get_level_width(dst_texture, i);
        height = wined3d_texture_get_level_height(dst_texture, i);
        depth = wined3d_texture_get_level_depth(dst_texture, i);
        wined3d_box_set(&box, 0, 0, width, height, 0, depth);

        for (j = 0; j < layer_count; ++j)
        {
            struct wined3d_resource *dst_resource = &dst_texture->resource;
            unsigned int dst_sub_resource_idx = j * dst_level_count + i;
            unsigned int src_sub_resource_idx = j * src_level_count + i + src_skip_levels;

            if (dst_resource->type == WINED3D_RTYPE_TEXTURE_2D)
            {
                struct wined3d_surface *dst_surface, *src_surface;

                dst_surface = dst_texture->sub_resources[dst_sub_resource_idx].u.surface;
                src_surface = src_texture->sub_resources[src_sub_resource_idx].u.surface;

                if (FAILED(wined3d_surface_blt(dst_surface, (const RECT *)&box, src_surface,
                        (const RECT *)&box, 0, NULL, WINED3D_TEXF_POINT)))
                    FIXME("Blit failed.\n");
            }
            else if (dst_resource->type == WINED3D_RTYPE_TEXTURE_3D)
            {
                unsigned int update_w, update_h, update_d;
                unsigned int row_pitch, slice_pitch;
                struct wined3d_context *context;
                struct wined3d_bo_address addr;

                update_w = box.right - box.left;
                update_h = box.bottom - box.top;
                update_d = box.back - box.front;

                if (box.left || box.top || box.front)
                {
                    FIXME("Source box %s not supported for %s resources.\n",
                            debug_box(&box), debug_d3dresourcetype(dst_resource->type));
                    continue;
                }

                context = context_acquire(cs->device, NULL, 0);

                if (!wined3d_texture_load_location(src_texture, src_sub_resource_idx,
                        context, src_texture->resource.map_binding))
                {
                    ERR("Failed to load source sub-resource into %s.\n",
                            wined3d_debug_location(src_texture->resource.map_binding));
                    context_release(context);
                    continue;
                }

                if (update_w == width && update_h == height && update_d == depth)
                {
                    wined3d_texture_prepare_texture(dst_texture, context, FALSE);
                }
                else if (!wined3d_texture_load_location(dst_texture, dst_sub_resource_idx,
                        context, WINED3D_LOCATION_TEXTURE_RGB))
                {
                    ERR("Failed to load destination sub-resource.\n");
                    context_release(context);
                    continue;
                }

                wined3d_texture_get_memory(src_texture, src_sub_resource_idx, &addr, src_texture->resource.map_binding);
                wined3d_texture_get_pitch(src_texture, src_sub_resource_idx % src_texture->level_count,
                        &row_pitch, &slice_pitch);

                wined3d_texture_bind_and_dirtify(dst_texture, context, FALSE);
                wined3d_texture_upload_data(dst_texture, dst_sub_resource_idx, context, &box,
                        wined3d_const_bo_address(&addr), row_pitch, slice_pitch);
                wined3d_texture_validate_location(dst_texture, dst_sub_resource_idx, WINED3D_LOCATION_TEXTURE_RGB);
                wined3d_texture_invalidate_location(dst_texture, dst_sub_resource_idx, ~WINED3D_LOCATION_TEXTURE_RGB);

                context_release(context);
            }
            else
            {
                FIXME("Not implemented for %s resources.\n", debug_d3dresourcetype(dst_resource->type));
            }
        }
    }

    wined3d_resource_release(&src_texture->resource);
    wined3d_resource_release(&dst_texture->resource);
}

[v5 PATCH 1/3] wined3d: Send texture updates through the command stream.

[Stefan Dösinger]

Hi Masanori,

I am sorry, I forgot to reply until I saw your patch resend this morning.

Henri's reply
(https://www.winehq.org/pipermail/wine-patches/2017-May/162123.html)
suggests that he doesn't see the command emitting as the pressing issue
in this patch series. So your original approach is fine for now and we
can adjust this specific aspect later.

Since he didn't sign off on the patches I assume there is something else
he doesn't like. I guess it is still the lack of integration with
location tracking
(https://www.winehq.org/pipermail/wine-devel/2017-April/117453.html)

Regarding the options with blit emitting (and again, it's not important
right now), I was thinking about something like this:

/* FIXME: Better name */
static void wined3d_cs_blt_helper(dst_resource, src_resource, ...)
{
    /* Most code from wined3d_cs_exec_blt_sub_resource goes here */
    /* but not wined3d_resource_release() calls */
}

static void wined3d_cs_exec_blt_sub_resource(struct wined3d_cs *cs,
const void *data)
{
    const struct wined3d_cs_blt_sub_resource *op = data;

    wined3d_cs_blt_helper(op->src, op->dst, ...);

    if (op->src_resource)
        wined3d_resource_release(op->src_resource);
    wined3d_resource_release(op->dst_resource);
}

static void wined3d_cs_exec_update_texture(struct wined3d_cs *cs, const
void *data)
{
    /* This looks very similar to your code from may 25, except that it
calls wined3d_cs_blt_helper instead of emit_blt_sub_resource. */
    wined3d_resource_release(src);
    wined3d_resource_release(dst);
}

/* Of course you still need the individual wined3d_cs_emit() calls */

Am 2017-05-25 um 16:19 schrieb Masanori Kakura:
> On Wed, 24 May 2017 16:08:30 +0200
> Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
> > Am 2017-05-24 um 15:49 schrieb Masanori Kakura:
> > > +        for (j = 0; j < layer_count; ++j)
> > > +        {
> > > +            wined3d_cs_emit_blt_sub_resource(cs,
> > > +                    &dst_texture->resource, j * dst_level_count + i, &box,
> > > +                    &src_texture->resource, j * src_level_count + i + src_skip_levels, &box,
> > > +                    0, NULL, WINED3D_TEXF_POINT);
> > > +        }
> > > I don't like emitting new operations in the CS handler instead of doing
> > > the actual operation. Maybe this can be avoided with some refactoring of
> > > wined3d_cs_exec_blt_sub_resource?
>
> I wrote 2 new patches but I'm not sure which is better.
>
> ----- Patch 1 (modify wined3d_cs_exec_blt_sub_resource) -----
> static void wined3d_cs_exec_blt_sub_resource(struct wined3d_cs *cs, const void *data)
> {
>     /* ... */
>
> error:
>     if (op->opcode == WINED3D_CS_OP_BLT_SUB_RESOURCE)
>     {
>         if (op->src_resource)
>             wined3d_resource_release(op->src_resource);
>         wined3d_resource_release(op->dst_resource);
>     }
> }
>
> static void wined3d_cs_exec_update_texture(struct wined3d_cs *cs, const void *data)
> {
>     /* ... */
>
> for (i = 0; i < level_count; ++i)
>     {
>         width = wined3d_texture_get_level_width(dst_texture, i);
>         height = wined3d_texture_get_level_height(dst_texture, i);
>         depth = wined3d_texture_get_level_depth(dst_texture, i);
>         wined3d_box_set(&box, 0, 0, width, height, 0, depth);
>
> for (j = 0; j < layer_count; ++j)
>         {
>             const struct wined3d_cs_blt_sub_resource op_blt = {WINED3D_CS_OP_UPDATE_TEXTURE,
>                     &dst_texture->resource, j * dst_level_count + i, box,
>                     &src_texture->resource, j * src_level_count + i + src_skip_levels, box,
>                     0, NULL, WINED3D_TEXF_POINT};
>
> wined3d_cs_exec_blt_sub_resource(cs, &op_blt);
>         }
>     }
>
> wined3d_resource_release(&src_texture->resource);
>     wined3d_resource_release(&dst_texture->resource);
> }
>
> ----- Patch 2 (write codes directly) -----
> static void wined3d_cs_exec_update_texture(struct wined3d_cs *cs, const void *data)
> {
>     /* ... */
>
> /* Update every surface level of the texture. */
>     for (i = 0; i < level_count; ++i)
>     {
>         width = wined3d_texture_get_level_width(dst_texture, i);
>         height = wined3d_texture_get_level_height(dst_texture, i);
>         depth = wined3d_texture_get_level_depth(dst_texture, i);
>         wined3d_box_set(&box, 0, 0, width, height, 0, depth);
>
> for (j = 0; j < layer_count; ++j)
>         {
>             struct wined3d_resource *dst_resource = &dst_texture->resource;
>             unsigned int dst_sub_resource_idx = j * dst_level_count + i;
>             unsigned int src_sub_resource_idx = j * src_level_count + i + src_skip_levels;
>
> if (dst_resource->type == WINED3D_RTYPE_TEXTURE_2D)
>             {
>                 struct wined3d_surface *dst_surface, *src_surface;
>
> dst_surface = dst_texture->sub_resources[dst_sub_resource_idx].u.surface;
>                 src_surface = src_texture->sub_resources[src_sub_resource_idx].u.surface;
>
> if (FAILED(wined3d_surface_blt(dst_surface, (const RECT *)&box, src_surface,
>                         (const RECT *)&box, 0, NULL, WINED3D_TEXF_POINT)))
>                     FIXME("Blit failed.\n");
>             }
>             else if (dst_resource->type == WINED3D_RTYPE_TEXTURE_3D)
>             {
>                 unsigned int update_w, update_h, update_d;
>                 unsigned int row_pitch, slice_pitch;
>                 struct wined3d_context *context;
>                 struct wined3d_bo_address addr;
>
> update_w = box.right - box.left;
>                 update_h = box.bottom - box.top;
>                 update_d = box.back - box.front;
>
> if (box.left || box.top || box.front)
>                 {
>                     FIXME("Source box %s not supported for %s resources.\n",
>                             debug_box(&box), debug_d3dresourcetype(dst_resource->type));
>                     continue;
>                 }
>
> context = context_acquire(cs->device, NULL, 0);
>
> if (!wined3d_texture_load_location(src_texture, src_sub_resource_idx,
>                         context, src_texture->resource.map_binding))
>                 {
>                     ERR("Failed to load source sub-resource into %s.\n",
>                             wined3d_debug_location(src_texture->resource.map_binding));
>                     context_release(context);
>                     continue;
>                 }
>
> if (update_w == width && update_h == height && update_d == depth)
>                 {
>                     wined3d_texture_prepare_texture(dst_texture, context, FALSE);
>                 }
>                 else if (!wined3d_texture_load_location(dst_texture, dst_sub_resource_idx,
>                         context, WINED3D_LOCATION_TEXTURE_RGB))
>                 {
>                     ERR("Failed to load destination sub-resource.\n");
>                     context_release(context);
>                     continue;
>                 }
>
> wined3d_texture_get_memory(src_texture, src_sub_resource_idx, &addr, src_texture->resource.map_binding);
>                 wined3d_texture_get_pitch(src_texture, src_sub_resource_idx % src_texture->level_count,
>                         &row_pitch, &slice_pitch);
>
> wined3d_texture_bind_and_dirtify(dst_texture, context, FALSE);
>                 wined3d_texture_upload_data(dst_texture, dst_sub_resource_idx, context, &box,
>                         wined3d_const_bo_address(&addr), row_pitch, slice_pitch);
>                 wined3d_texture_validate_location(dst_texture, dst_sub_resource_idx, WINED3D_LOCATION_TEXTURE_RGB);
>                 wined3d_texture_invalidate_location(dst_texture, dst_sub_resource_idx, ~WINED3D_LOCATION_TEXTURE_RGB);
>
> context_release(context);
>             }
>             else
>             {
>                 FIXME("Not implemented for %s resources.\n", debug_d3dresourcetype(dst_resource->type));
>             }
>         }
>     }
>
> wined3d_resource_release(&src_texture->resource);
>     wined3d_resource_release(&dst_texture->resource);
> }