-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Chris Wilson wrote:
> Ian, thanks for your detailed comments! The patch was just guess work
> from looking at similar extensions - I couldn't see a step-by-step guide
> on how to add an extension to Mesa.
That's the tough thing about adding a new extension... there are 47
places where you have to modify code in a fairly mechanical way.
I looked back at the patch, and there's a more significant problem. The
code in _mesa_ObjectPurgeableAPPLE does not correctly handle texture
objects or renderbuffers correctly. These object types are *not*
gl_buffer_object. Calling get_buffer on the name of a texture will
either return NULL or return a buffer object that, by luck, has the same
name.
This means you'll probably need three entries in dd_function_table, and
_mesa_ObjectPurgeableAPPLE will need to look-up each object type separately.
>> Are there any hooks needed in dlist.c?
>
> I didn't think so. The specification didn't mention display lists and it
> made no sense to me as you need to check the status of a purgeable
> texture before use (and potentially replace/reload the texture) so
> storing such textures inside a display list would be incorrect.
Right... but when we're compiling a display list the dispatch table gets
changed. The _mesa_init_save_table code sets function pointers for
functions that are not saved in display lists to the "regular" versions.
Do a simple test: write code that starts compiling a display list,
then calls glObjectPurgeableAPPLE. What happens?
>>> +<category name="GL_APPLE_object_purgeable" number="371">
>>> + <enum name="RELEASED_APPLE" value="0x8A19"/>
>>> + <enum name="VOLATILE_APPLE" value="0x8A1A"/>
>>> + <enum name="RETAINED_APPLE" value="0x8A1B"/>
>>> + <enum name="UNDEFINED_APPLE" value="0x8A1C"/>
>>> + <enum name="PURGEABLE_APPLE" value="0x8A1D"/>
>>> +
>>> + <enum name="BUFFER_OBJECT_APPLE" value="0x85B3"/>
>> Do any of these enums have associated Gets?
>
> So if the enum can be used a pname, the associated function should be
> included in a <size> node?
Right. Search in gl_API.xml for 'size name="Get' for some examples.
Also, if any of these are queryable with glGetIntegerv and friends, they
need to be added to src/mesa/main/get_gen.py. Then run 'python
get_gen.py > get.c'.
>>> + if (bufObj->purgeable) {
>>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>>> + "glObjectPurgeable(name = 0x%x) is already purgeable",
>>> name);
>>> + return bufObj->purgeable;
>>> + }
>>> +
>>> + bufObj->purgeable = option;
>> This is wrong. ObjectPurgableAPPLE always sets the purgeable flag to
>> true.
>
> Yes. I was just using TRUE in the general non-zero sense, so just used the
> hint. However, using a simple boolean avoids the temptation of storing
> the returned status from the driver functions...
>
>> The 'option' parameter gives the GL a hint as to how the
>> application intends to use the object in the future. VOLATILE means the
>> application doesn't know whether it will use the buffer again (i.e.,
>> only release it under memory pressure), and RELEASED means that it knows
>> it's done with it (maybe release it now).
>
>> On the flip side, the option to ObjectUnpurgable is really a hint of
>> what to do when the buffer object has been paged out of VRAM on a
>> discrete graphics card. We don't have to worry about this at all on UMA
>> systems.
>
> Indeed, as you can see from the implementation for Intel, we only use
> the hint to release the texture if it falls out of the aperture. We
> could try a more subtle approach that uses the hint to only discard the
> texture if we would be forced to swap. I'm not sure if the kernel has
> the appropriate level of granularity for that though.
As I suggested in a comment to the other patch, madvise(MADV_DONTNEED)
is almost what we want. However, it lacks the ability to find out later
if the pages were discarded (and zero filled) or not.
>>> + if (ctx->Driver.ObjectPurgeable)
>>> + bufObj->purgeable = ctx->Driver.ObjectPurgeable(ctx, bufObj, option);
>> This is weird. Usually driver functions don't return a value to set in
>> the object's state. The driver function just sets that state.
>
> Done, we only need to report the current status hint as reported by the
> driver to the user.
>
>>> + retval = bufObj->purgeable = 0;
>>> + if (ctx->Driver.ObjectUnpurgeable)
>>> + retval = ctx->Driver.ObjectUnpurgeable(ctx, bufObj, option);
>>> +
>>> + return retval;
>> Similar comment here as in the previous function. The purgeable flag
>> must be false on exit, but this function is supposed to return option.
>
> Hmm, just a quibble as it shouldn't just return 'option' but the status
> as reported by the driver.
I double checked. You are correct.
>>> diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
>>> index 54cf37c..ba233dd 100644
>>> --- a/src/mesa/main/extensions.c
>>> +++ b/src/mesa/main/extensions.c
>>> @@ -147,6 +147,7 @@ static const struct {
>>> { OFF, "GL_APPLE_client_storage", F(APPLE_client_storage) },
>>> { ON, "GL_APPLE_packed_pixels", F(APPLE_packed_pixels) },
>>> { OFF, "GL_APPLE_vertex_array_object",
>>> F(APPLE_vertex_array_object) },
>>> + { OFF, "GL_APPLE_object_purgeable", F(APPLE_object_purgeable) },
>> ^
>> Trivial whitespace error.
>>
>>> { OFF, "GL_ATI_blend_equation_separate",
>>> F(EXT_blend_equation_separate) },
>>> { OFF, "GL_ATI_envmap_bumpmap", F(ATI_envmap_bumpmap) },
>>> { OFF, "GL_ATI_texture_env_combine3",
>>> F(ATI_texture_env_combine3)},
>> This function should be enabled in the software paths. There's a
>> function in this file that does that.
>
> Found it: _mesa_enable_sw_extensions(). Would it be sensible to enable
> APPLE_object_purgeable by default since it is a no-op unless supported
> by the underlying driver and memory-manager?
I don't have a strong opinion. I usually use the following check list
for determining whether to enable an extension by default:
1. Does it have a no-op implementation?
2. Does it have a negative performance impact when used on a hardware
driver with the no-op implementation?
3.a. Do lots of apps want to use it by default?
3.b. Does Mesa want to use it internally?
Perhaps Brian or Eric will have an opinion. The spec also says
"requires OpenGL 1.5", so maybe not.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAkr8YSsACgkQX1gOwKyEAw9AxgCfVVc00vFmST8QFm6lkk7XmKmb
VfsAnjy7SnVpVsVdC9QxC+cimwK6o/3w
=L6I0
-----END PGP SIGNATURE-----
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
