Re: [PATCH 5/6] wined3d: Use PBOs for dynamic volumes

[Henri Verbeet]

On 22 August 2013 23:22, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
> @@ -145,6 +158,9 @@ static DWORD volume_access_from_location(DWORD location)
>          case WINED3D_LOCATION_TEXTURE_RGB:
>              return WINED3D_RESOURCE_ACCESS_GPU;
>
> +        case WINED3D_LOCATION_BUFFER:
> +            return WINED3D_RESOURCE_ACCESS_CPU | WINED3D_RESOURCE_ACCESS_GPU;
> +
> Why does loading into a PBO require CPU access?
> +        case WINED3D_LOCATION_BUFFER:
> +            if (!volume->pbo || !(volume->flags & WINED3D_VFLAG_PBO))
> +            {
> +                ERR("Trying to load WINED3D_LOCATION_BUFFER without setting it up first.\n");
> +                return;
> +            }
> I think there should never be a PBO without WINED3D_VFLAG_PBO being
> set. There's also something to be said for leaving out the return,
> since that would at least in theory allow the compiler to drop the
> extra checks when compiled without debug messages. That probably
> applies to ERRs in general, since they're either never supposed to
> happen, or not supposed to be an ERR.
> +/* Context activation is done by the caller. */
> +static void wined3d_volume_prepare_pbo(struct wined3d_volume *volume, struct wined3d_context *context)
> +{
> +    const struct wined3d_gl_info *gl_info = context->gl_info;
> +
> +    if (volume->pbo)
> +        return;
> +
> +    GL_EXTCALL(glGenBuffersARB(1, &volume->pbo));
> +    checkGLcall("glGenBuffersARB(1, &volume->pbo)");
> +    GL_EXTCALL(glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->pbo));
> +    checkGLcall("glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->pbo)");
> +    GL_EXTCALL(glBufferDataARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->resource.size, NULL, GL_STREAM_DRAW_ARB));
> +    checkGLcall("glBufferDataARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->resource.size, NULL, GL_STREAM_DRAW_ARB");
> +    GL_EXTCALL(glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, 0));
> +    checkGLcall("glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, 0)");
> +
> +    TRACE("Created PBO %u for volume %p.\n", volume->pbo, volume);
> +}
> I think one checkGLcall() per function is enough in general. That's
> even more true these days with ARB_debug_output that will usually give
> more specific error messages than checkGLcall() anyway. Applies to a
> couple of other places as well.

[PATCH 5/6] wined3d: Use PBOs for dynamic volumes

[Stefan Dösinger]

Am 23.08.2013 um 11:34 schrieb Henri Verbeet <hverbeet(a)gmail.com>:
> Why does loading into a PBO require CPU access?
> The only reason why we want to store a volume in a PBO is to map it, which requires CPU access. If the volume doesn't allow CPU access I don't see a reason to load it into a PBO.

Well, there's RGB<->sRGB loading in case GL_EXT_texture_sRGB_decode isn't supported. I am handing that with a temporary heap allocation and don't go through the tracked locations to keep the matter simple. The correct fix for this problem, if it happens, is to implement sRGB_decode in the driver.
> > +        case WINED3D_LOCATION_BUFFER:
> > +            if (!volume->pbo || !(volume->flags & WINED3D_VFLAG_PBO))
> > +            {
> > +                ERR("Trying to load WINED3D_LOCATION_BUFFER without setting it up first.\n");
> > +                return;
> > +            }
> > I think there should never be a PBO without WINED3D_VFLAG_PBO being
> > set.
> > Yup, but it's an ERR path, so neither condition is supposed to ever be true. The thinking behind checking both is as a safeguard for future code changes. I'm checking resource.allocatedMemory and resource.heap_memory in the sysmem case for the same reason.
> There's also something to be said for leaving out the return,
> since that would at least in theory allow the compiler to drop the
> extra checks when compiled without debug messages.
> Good point.

[PATCH 5/6] wined3d: Use PBOs for dynamic volumes

[Henri Verbeet]

On 23 August 2013 12:04, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
> Am 23.08.2013 um 11:34 schrieb Henri Verbeet <hverbeet(a)gmail.com>:
> > Why does loading into a PBO require CPU access?
> > The only reason why we want to store a volume in a PBO is to map it, which requires CPU access. If the volume doesn't allow CPU access I don't see a reason to load it into a PBO.

Yes, but that's more of a higher level policy and wouldn't hold for
e.g. VBOs. You handle that by just not setting WINED3D_VFLAG_PBO for
those volumes.

[PATCH 5/6] wined3d: Use PBOs for dynamic volumes

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2013-08-23 12:29, schrieb Henri Verbeet:
> On 23 August 2013 12:04, Stefan Dösinger <stefan(a)codeweavers.com>
> wrote:
> > Am 23.08.2013 um 11:34 schrieb Henri Verbeet
> > <hverbeet(a)gmail.com>:
> > > Why does loading into a PBO require CPU access?
> > > The only reason why we want to store a volume in a PBO is to map
> > > it, which requires CPU access. If the volume doesn't allow CPU
> > > access I don't see a reason to load it into a PBO.
>
> Yes, but that's more of a higher level policy and wouldn't hold
> for e.g. VBOs. You handle that by just not setting
> WINED3D_VFLAG_PBO for those volumes.
> The rules for surfaces and volumes aren't applicable to buffers
> anyway. A default pool buffer without D3DUSAGE_DYNAMIC is mappable for
> example.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSF01qAAoJEN0/YqbEcdMwUFMP/2WONMZuleeqh3cdEPr+px9m
BdyQ3vsXJxmFPGRMx2+hU1jrISLKfy7sgQ6Q7/884eZPFTiIVc6icD60ySK6iUVt
lKU+M+ymX88icMyVaN8AUBqP1oVTlXFVHqMpYQAfm8O4I5OTliAaiYp50O/AtAvl
I/5P85eUUUxhVXFFfv7+XkErMJeFlZ5plWWHZXXZnG4rJ+4neAR4CPVnwJ7h92Gk
40U5GHmIAw0NwZBKVjj+HuSAUVxKV/ssx1zpXtDC0zaaOjPP7Zpt+LxjIqWLeiCi
WA+FN4zbg+njU2tpUBemFwIPFPBQLu9vZWSxgyhP/Kxa6SI19dFTMTKRl3SyjJo1
HSfAy+qwyUmIJ/y93PgT6MSj5siJfF2gDUpZhrSf1FKkCa2l7FrTKphOVSatVyPp
A6s76N2FofRG93+x10Wrd5Xsf1w5lFBRjXiGdyY1cCFzfC8z/y7xn51HLPc59c33
kBCZsmy+lYz1fOzhLRh1FTt/q8H+DpAucTxjKQL1jPTFRLzUKW4P6CiNgK3C2yqY
SJ2BIEaUufq35lD3xO6Aj5HUvCE7e2tNWvQtgBHi//Daz9DbxcXqR1DnkO9gbEAb
7AT/2taH643O+vR9EDOdeXBGFQDhFrX9ueF+8J9jL6BCEENTw78eAe5Fc1yblyNg
XuBJ2Za2sIR0p2iPF1Gy
=DCmb
-----END PGP SIGNATURE-----

[PATCH 5/6] wined3d: Use PBOs for dynamic volumes

[Henri Verbeet]

On 23 August 2013 13:54, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
> The rules for surfaces and volumes aren't applicable to buffers
> anyway. A default pool buffer without D3DUSAGE_DYNAMIC is mappable for
> example.

I don't think the differences are insurmountable, but the main point
is still that I think this is the wrong level to enforce that kind of
policy.

[PATCH 5/6] wined3d: Use PBOs for dynamic volumes

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2013-08-23 14:01, schrieb Henri Verbeet:
> On 23 August 2013 13:54, Stefan Dösinger <stefan(a)codeweavers.com>
> wrote:
> > The rules for surfaces and volumes aren't applicable to buffers 
> > anyway. A default pool buffer without D3DUSAGE_DYNAMIC is
> > mappable for example.
>
> I don't think the differences are insurmountable, but the main
> point is still that I think this is the wrong level to enforce that
> kind of policy.
> Ok.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSF0/9AAoJEN0/YqbEcdMwmCgP/RmR6JR90aHjc4AzKD6lS1MG
Pmz0PFzsgxvtoNmX+jHRPhwu4QBCXXzwYUaBPhilSZIBUwnjFOBVdIFhMzG3k3hc
xJFNQfbc4wiEagNjZp5i6o3RZLrq9gfoUaAvySMekW11MkJarWTE6getgZJrh50z
9qEB2nxxYS83EVC8iP3WTsmJ0BI9X3DA7a4EU3dnMQqlHpAUA/mMYddy3LbBC0h5
mvYXVGLJEAMBQU4sOcx2rl/MxqIQqQcbqUT04+1U16uVJItB2QgSTAsVKAbmw6N/
uDImq6ISS+6NYpSEyrXPGfMUKJ84GBzErqLMIK5UquvOXARu0pkgWytICwpbRgfx
wlMnSMq7rhxNmugb8BDvbd9GnwVXmio/uA6xix0Iop8xoLpun5xJMMeyXPn9HCFH
S2Ph4tuMpY6ubUehziN1l9HiUAjy3CEghFESv/kD83GwogEQ13NJlITNowfUcbps
XKZOCSuZfUe9PkSWAYJw2wbwFTbjASy9tW7M03fj0jHlKJoQUhYZL6HF0IHrmDpJ
KK7LyqJaQh4AT3DtVX9YVVPubqKWojDm1iwuuAMv6BF9PNY9aaX98TYRaY9OzJ3g
N0qy9hQXvDXI8IxeuMrzEmmVRMz34s7cl9fegOCBuErbfsDasZP5r75TpFIkhc/V
0hUE7kOMV8vKwCHdS02I
=SzlX
-----END PGP SIGNATURE-----