[PATCH 01/10] wined3d: Introduce struct wined3d_dirty_regions.

[Stefan Dösinger]

Hi Masanori,

Thank you for your patches! I have a few comments:

Regarding the big picture I think it would be great if we had a scheme
that also worked for partially mapping resources. It's also necessary to
make the D3DLOCK_NO_DIRTY_UPDATE test on managed textures work. However,
doing this is a lot more work than your patchset and complicates
location management a lot.

If Henri tells you something different follow his advice. He is the
authority for decisions in d3d :-) .

Patch 1:
Am 2017-04-17 um 15:47 schrieb Masanori Kakura:
> +struct wined3d_dirty_regions
> +{
> +    UINT left;
> +    UINT top;
> +    UINT right;
> +    UINT bottom;
> +    UINT front;
> +    UINT back;
> +    struct wined3d_dirty_regions *next;
> +};
A standard wine list would be better IMHO.

Patch 2:
> diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h
> index 785105d7b9..711f98a0a6 100644
> --- a/dlls/wined3d/wined3d_private.h
> +++ b/dlls/wined3d/wined3d_private.h
> @@ -2765,6 +2765,27 @@ struct wined3d_dirty_regions
>      struct wined3d_dirty_regions *next;
>  };
>  
> +static inline BOOL wined3d_dirty_regions_add(struct wined3d_dirty_regions **regions,
> +        UINT left, UINT top, UINT right, UINT bottom, UINT front, UINT back)
I don't think we want this in the header file, I think it'd be better
placed in resource.c. Also note that buffers already have a related
mechanism (buffer->modified_areas).

You can predeclare the functions in the header if you need to call them
from multiple files.

> +{
> +    struct wined3d_dirty_regions *new_region;
> +
> +    if (!(new_region = HeapAlloc(GetProcessHeap(), 0, sizeof(struct wined3d_dirty_regions))))
> +        return FALSE;
HeapAlloc is expensive. I think it is OK for the start, but in the long
run a way to keep some old dirty region elements around and reuse them
instead of always calling Alloc / Free.

Patch 5:
> +    if (!wined3d_dirty_regions_add(&resource->dirty_regions, 0, 0, width, height, 0, depth))
Why not use wined3d_resource_maximize_dirty_region here?

>      wined3d_resource_acquire(resource);
> +    wined3d_dirty_regions_clear(&resource->dirty_regions);
>      wined3d_cs_destroy_object(resource->device->cs, wined3d_resource_destroy_object, resource);
Regions need to be cleared from inside the cs thread in case there are
pending update_texture commands.

Similarly patch 6 needs to put the region modifying into
wined3d_cs_exec_add_dirty_texture_region.

Patch 7:
> +                if (box->left < box->right && box->top < box->bottom && box->front < box->back)
> +                {
You don't need to check the box, texture_resource_sub_resource_map
already does it before it reaches this place of the code. See
wined3d_texture_check_box_dimensions.

Patch 8: I am not sure if this is necessary at all. In d3d9 it doesn't
matter because a UpdateSurface destination can never be an UpdateTexture
source.

Patch 9/10: These should be merged, and I don't think patch 10 looks
right. I think dirty regions should be honored on sublevels as well,
just adjusted (i.e., divide by 2 for every level).

> +    if (src_texture->resource.width <= dst_texture->resource.width
> +            && src_texture->resource.height <= dst_texture->resource.height
> +            && src_texture->resource.depth <= dst_texture->resource.depth)
Why are you adding this check? Is it because the src_skip_levels broke
the if (i == 0) root level detection? If you apply the dirty region to
all levels this problem should go away.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20170417/d005b62e/attachment.sig>