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.
Excerpts from Ian Romanick's message of Wed Nov 11 20:06:04 +0000 2009:
> Eric's suggestion of doing 3 commits (XML, regeneration, real code) is
> probably even better.
Yes, this makes a lot of sense when trying to review these paths.
> Substantive comments are below.
>
> > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > ---
> > src/mesa/glapi/APPLE_object_purgeable.xml | 35 +
> > src/mesa/glapi/Makefile | 1 +
> > src/mesa/glapi/gl_API.xml | 1 +
> > src/mesa/glapi/glapidispatch.h | 33 +-
> > src/mesa/glapi/glapioffsets.h | 18 +-
> > src/mesa/glapi/glapitable.h | 13 +-
> > src/mesa/glapi/glapitemp.h | 44 +-
> > src/mesa/glapi/glprocs.h | 634 ++--
> > src/mesa/main/api_exec.c | 6 +
> > src/mesa/main/bufferobj.c | 151 +
> > src/mesa/main/bufferobj.h | 11 +
> > src/mesa/main/dd.h | 10 +
> > src/mesa/main/enums.c | 6054
> > +++++++++++++++--------------
> > src/mesa/main/extensions.c | 1 +
> > src/mesa/main/mfeatures.h | 1 +
> > src/mesa/main/mtypes.h | 2 +
> > src/mesa/main/remap_helper.h | 2992 +++++++-------
> > src/mesa/sparc/glapi_sparc.S | 19 +-
> > src/mesa/x86-64/glapi_x86-64.S | 181 +-
> > src/mesa/x86/glapi_x86.S | 23 +-
>
> 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.
> > 20 files changed, 5334 insertions(+), 4896 deletions(-)
> > create mode 100644 src/mesa/glapi/APPLE_object_purgeable.xml
> >
> > diff --git a/src/mesa/glapi/APPLE_object_purgeable.xml
> > b/src/mesa/glapi/APPLE_object_purgeable.xml
> > new file mode 100644
> > index 0000000..ba70e87
> > --- /dev/null
> > +++ b/src/mesa/glapi/APPLE_object_purgeable.xml
> > @@ -0,0 +1,35 @@
> > +<?xml version="1.0"?>
> > +<!DOCTYPE OpenGLAPI SYSTEM "gl_API.dtd">
> > +
> > +<OpenGLAPI>
> > +<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?
> > + 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.
> > + 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.
> > 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?
> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > index 94d29a7..82d6e30 100644
> > --- a/src/mesa/main/mtypes.h
> > +++ b/src/mesa/main/mtypes.h
> > @@ -1410,6 +1410,7 @@ struct gl_buffer_object
> > GLsizeiptr Length; /**< Mapped length */
> > /*...@}*/
> > GLboolean Written; /**< Ever written to? (for debugging) */
> > + GLenum purgeable; /**< Purgeable status of the buffer */
>
> Again, this should be a boolean. The enum is only used by the driver.
Done. And 'Purgeable' to boot.
-ickle
--
Chris Wilson, Intel Open Source Technology Centre
------------------------------------------------------------------------------
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
