Re: [PATCH 4/4] wined3d: add BLIT_OP_COMPLEX_UPLOAD

[Henri Verbeet]

On 17 May 2010 13:11, Roderick Colenbrander  wrote:
> This patch introduces a new blit operation to allow complex uploads from LoadLocation. It allows hw accelerated
> p8 uploads to function again (I broke them by accident). Without this blit_operation there is no way to detect
> this single case at the moment in which we allow the P8 shader (remember both the source and destination have
> complex fixups set, so this doesn't pass).

I'm not so convinced this needs a separate blit operation, it should
always be possible to do blits between surfaces with the same formats
as long as no stretching is involved.

[PATCH 4/4] wined3d: add BLIT_OP_COMPLEX_UPLOAD

[Roderick Colenbrander]

On Mon, May 17, 2010 at 1:29 PM, Henri Verbeet  wrote:
> On 17 May 2010 13:11, Roderick Colenbrander  wrote:
> > This patch introduces a new blit operation to allow complex uploads from LoadLocation. It allows hw accelerated
> > p8 uploads to function again (I broke them by accident). Without this blit_operation there is no way to detect
> > this single case at the moment in which we allow the P8 shader (remember both the source and destination have
> > complex fixups set, so this doesn't pass).
>
> I'm not so convinced this needs a separate blit operation, it should
> always be possible to do blits between surfaces with the same formats
> as long as no stretching is involved.

I now allow this in arbfp blit_supported using this check:
    if (src_fixup == dst_fixup &&
is_complex_fixup(dst_format_desc->color_fixup) && src_rect && dst_rect
        && (src_rect->right - src_rect->left == dst_rect->right -
dst_rect->left)
        && (abs(src_rect->bottom - src_rect->top) ==
abs(dst_rect->bottom - dst_rect->top))
        && (dst_usage & WINED3DUSAGE_RENDERTARGET))

A check like that works for two cases:
1) checks if 8-bit -> 8-bit blit is supported between two different surfaces
2) checks if hw accelerated 8-bit upload is around (d3dfmt_get_conv
checks and it is what BLIT_OP_COMPLEX_UPLOAD was about)

My main worry is d3dfmt_get_conv in case of the ffp backend.
Originally there were GL extension checks there to perform format
fixups and at this point blit_supported is used there for this
purpose. If I would add the above check it would either disallow (1)
in case I would add a EXT_PALETTED_TEXTURE or if I won't include this
extension check it would break the format fixup part of
d3dfmt_get_conv. BLIT_OP_COMPLEX_FIXUP would prevent such issues. (I
could just add the EXT_PALETTED_CHECK if it is fine if we don't
support converted_p8 -> converted_p8 blits between two different
surfaces using GL; I think depending on surface location flags a
cpu_blit might make more sense as GL won't gain you much since you
have to do two surface conversions)

So the actual issue is d3dfmt_get_conv. For a part the p8 fixup is in
the formats table and for a part here. I could add a generic p8 format
to the utils table without GL checks it would fix a part of the
mentioned problem (the fixup code in d3dfmt_get_conv would then be
called but effectively it would do nothing since the internal format
and types are the same).

Overall I think that somehow the blit_shader should worry about the
formats during 'blit_surface' and decide whether to use hw conversion
or software conversion (the ffp backend should likely support both).
This could work for the geneal Blt/BltFast case but won't for
UnlockRect in the current design where it uses
LoadLocation(IN_TEXTURE) + LoadLocation(IN_DRAWABLE) but it would have
to use the blit_shader there.

Roderick