[Mesa-dev,RFC] mesa: add support for multiple buffer mappings

Submitted by Marek Olšák on Feb. 5, 2014, 10:06 p.m.

Details

Message ID 1391637998-5293-1-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák Feb. 5, 2014, 10:06 p.m.
From: Marek Olšák <marek.olsak@amd.com>

OpenGL allows a buffer to be mapped only once, but we also map buffers
internally, e.g. in the software primitive restart fallback, for PBOs,
vbo_get_minmax_index, etc. This has always been a problem, but it will
be a bigger problem with persistent buffer mappings, which will prevent
all Mesa functions from mapping buffers for internal purposes.

This adds a driver inteface to core Mesa which supports multiple buffer
mappings and allows 2 mappings: one for the GL user and one for Mesa.

Note that Gallium supports an unlimited number of buffer and texture
mappings, so it's not really an issue for Gallium.

This is just the interface change. Please review. If you don't like it,
feel free to propose how you would do it. Thank you.

---
 src/mesa/main/dd.h     |  9 ++++++---
 src/mesa/main/mtypes.h | 25 ++++++++++++++++++-------
 2 files changed, 24 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
index ab135f4..fdd0d94 100644
--- a/src/mesa/main/dd.h
+++ b/src/mesa/main/dd.h
@@ -598,14 +598,17 @@  struct dd_function_table {
     */
    void * (*MapBufferRange)( struct gl_context *ctx, GLintptr offset,
                              GLsizeiptr length, GLbitfield access,
-                             struct gl_buffer_object *obj);
+                             struct gl_buffer_object *obj,
+                             enum gl_map_buffer_index index);
 
    void (*FlushMappedBufferRange)(struct gl_context *ctx,
                                   GLintptr offset, GLsizeiptr length,
-                                  struct gl_buffer_object *obj);
+                                  struct gl_buffer_object *obj,
+                                  enum gl_map_buffer_index index);
 
    GLboolean (*UnmapBuffer)( struct gl_context *ctx,
-			     struct gl_buffer_object *obj );
+			     struct gl_buffer_object *obj,
+                             enum gl_map_buffer_index index);
    /*@}*/
 
    /**
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 4aaad16..5200377 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -1437,6 +1437,22 @@  struct gl_viewport_attrib
    GLmatrix _WindowMap;		/**< Mapping transformation as a matrix. */
 };
 
+enum gl_map_buffer_index {
+   MAP_CTX_USER,
+   MAP_CTX_INTERNAL,
+
+   NUM_MAP_BUFFER_CONTEXTS
+};
+
+/**
+ * Fields describing a mapped buffer range.
+ */
+struct gl_map_buffer_context {
+   GLbitfield AccessFlags; /**< Mask of GL_MAP_x_BIT flags */
+   GLvoid *Pointer;     /**< User-space address of mapping */
+   GLintptr Offset;     /**< Mapped offset */
+   GLsizeiptr Length;   /**< Mapped length */
+};
 
 /**
  * GL_ARB_vertex/pixel_buffer_object buffer object
@@ -1451,17 +1467,12 @@  struct gl_buffer_object
    GLbitfield StorageFlags; /**< GL_MAP_PERSISTENT_BIT, etc. */
    GLsizeiptrARB Size;  /**< Size of buffer storage in bytes */
    GLubyte *Data;       /**< Location of storage either in RAM or VRAM. */
-   /** Fields describing a mapped buffer */
-   /*@{*/
-   GLbitfield AccessFlags; /**< Mask of GL_MAP_x_BIT flags */
-   GLvoid *Pointer;     /**< User-space address of mapping */
-   GLintptr Offset;     /**< Mapped offset */
-   GLsizeiptr Length;   /**< Mapped length */
-   /*@}*/
    GLboolean DeletePending;   /**< true if buffer object is removed from the hash */
    GLboolean Written;   /**< Ever written to? (for debugging) */
    GLboolean Purgeable; /**< Is the buffer purgeable under memory pressure? */
    GLboolean Immutable; /**< GL_ARB_buffer_storage */
+
+   struct gl_map_buffer_context Mappings[NUM_MAP_BUFFER_CONTEXTS];
 };
 
 

Comments

Marek Olšák <maraeo@gmail.com> writes:

> From: Marek Olšák <marek.olsak@amd.com>
>
> OpenGL allows a buffer to be mapped only once, but we also map buffers
> internally, e.g. in the software primitive restart fallback, for PBOs,
> vbo_get_minmax_index, etc. This has always been a problem, but it will
> be a bigger problem with persistent buffer mappings, which will prevent
> all Mesa functions from mapping buffers for internal purposes.
>
> This adds a driver inteface to core Mesa which supports multiple buffer
> mappings and allows 2 mappings: one for the GL user and one for Mesa.
>
> Note that Gallium supports an unlimited number of buffer and texture
> mappings, so it's not really an issue for Gallium.
>
> This is just the interface change. Please review. If you don't like it,
> feel free to propose how you would do it. Thank you.

Having an index makes sense to me -- you have to either have that in the
caller, or have the MBR hook return a pointer to private storage that
gets passed in to Unmap, which sucks more.

My only question is whether we want to make Offset/Length/Pointer be
GL-only properties that are managed by glMapBuffer() and
glMapBufferRange() for us, then have drivers like intel store whatever
the need in their own private area.

Only thing I'd definitely do here is
s/gl_map_buffer_context/gl_buffer_mapping/ or something -- context
sounds weird to describe that set of state.
On 02/05/2014 03:06 PM, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> OpenGL allows a buffer to be mapped only once, but we also map buffers
> internally, e.g. in the software primitive restart fallback, for PBOs,
> vbo_get_minmax_index, etc. This has always been a problem, but it will
> be a bigger problem with persistent buffer mappings, which will prevent
> all Mesa functions from mapping buffers for internal purposes.
>
> This adds a driver inteface to core Mesa which supports multiple buffer
> mappings and allows 2 mappings: one for the GL user and one for Mesa.
>
> Note that Gallium supports an unlimited number of buffer and texture
> mappings, so it's not really an issue for Gallium.
>
> This is just the interface change. Please review. If you don't like it,
> feel free to propose how you would do it. Thank you.
>
> ---
>   src/mesa/main/dd.h     |  9 ++++++---
>   src/mesa/main/mtypes.h | 25 ++++++++++++++++++-------
>   2 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
> index ab135f4..fdd0d94 100644
> --- a/src/mesa/main/dd.h
> +++ b/src/mesa/main/dd.h
> @@ -598,14 +598,17 @@ struct dd_function_table {
>       */
>      void * (*MapBufferRange)( struct gl_context *ctx, GLintptr offset,
>                                GLsizeiptr length, GLbitfield access,
> -                             struct gl_buffer_object *obj);
> +                             struct gl_buffer_object *obj,
> +                             enum gl_map_buffer_index index);
>
>      void (*FlushMappedBufferRange)(struct gl_context *ctx,
>                                     GLintptr offset, GLsizeiptr length,
> -                                  struct gl_buffer_object *obj);
> +                                  struct gl_buffer_object *obj,
> +                                  enum gl_map_buffer_index index);
>
>      GLboolean (*UnmapBuffer)( struct gl_context *ctx,
> -			     struct gl_buffer_object *obj );
> +			     struct gl_buffer_object *obj,
> +                             enum gl_map_buffer_index index);
>      /*@}*/
>
>      /**
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 4aaad16..5200377 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1437,6 +1437,22 @@ struct gl_viewport_attrib
>      GLmatrix _WindowMap;		/**< Mapping transformation as a matrix. */
>   };
>
> +enum gl_map_buffer_index {
> +   MAP_CTX_USER,
> +   MAP_CTX_INTERNAL,
> +
> +   NUM_MAP_BUFFER_CONTEXTS
> +};

We've been doing typedefs for the enums in mtypes.h and I'd probably use 
shorter names.  Maybe this:

typedef enum
{
    MAP_USER,
    MAP_INTERNAL,
    MAP_BUFFER_COUNT;
} gl_map_buffer_index;



> +
> +/**
> + * Fields describing a mapped buffer range.
> + */
> +struct gl_map_buffer_context {
> +   GLbitfield AccessFlags; /**< Mask of GL_MAP_x_BIT flags */
> +   GLvoid *Pointer;     /**< User-space address of mapping */
> +   GLintptr Offset;     /**< Mapped offset */
> +   GLsizeiptr Length;   /**< Mapped length */
> +};

I agree with Eric's naming suggestion here.

Otherwise, this looks OK to me, though I haven't really thought it 
through very much.

-Brian
On 02/05/2014 02:06 PM, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> OpenGL allows a buffer to be mapped only once, but we also map buffers
> internally, e.g. in the software primitive restart fallback, for PBOs,
> vbo_get_minmax_index, etc. This has always been a problem, but it will
> be a bigger problem with persistent buffer mappings, which will prevent
> all Mesa functions from mapping buffers for internal purposes.
> 
> This adds a driver inteface to core Mesa which supports multiple buffer
> mappings and allows 2 mappings: one for the GL user and one for Mesa.
> 
> Note that Gallium supports an unlimited number of buffer and texture
> mappings, so it's not really an issue for Gallium.
> 
> This is just the interface change. Please review. If you don't like it,
> feel free to propose how you would do it. Thank you.

This seems like a reasonable enough interface to me.  If we decide that
it's insufficient or ugly later, it should be easy enough to change.  I
also like Eric's and Brian's suggestions.

> ---
>  src/mesa/main/dd.h     |  9 ++++++---
>  src/mesa/main/mtypes.h | 25 ++++++++++++++++++-------
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
> index ab135f4..fdd0d94 100644
> --- a/src/mesa/main/dd.h
> +++ b/src/mesa/main/dd.h
> @@ -598,14 +598,17 @@ struct dd_function_table {
>      */
>     void * (*MapBufferRange)( struct gl_context *ctx, GLintptr offset,
>                               GLsizeiptr length, GLbitfield access,
> -                             struct gl_buffer_object *obj);
> +                             struct gl_buffer_object *obj,
> +                             enum gl_map_buffer_index index);
>  
>     void (*FlushMappedBufferRange)(struct gl_context *ctx,
>                                    GLintptr offset, GLsizeiptr length,
> -                                  struct gl_buffer_object *obj);
> +                                  struct gl_buffer_object *obj,
> +                                  enum gl_map_buffer_index index);
>  
>     GLboolean (*UnmapBuffer)( struct gl_context *ctx,
> -			     struct gl_buffer_object *obj );
> +			     struct gl_buffer_object *obj,
> +                             enum gl_map_buffer_index index);
>     /*@}*/
>  
>     /**
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 4aaad16..5200377 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1437,6 +1437,22 @@ struct gl_viewport_attrib
>     GLmatrix _WindowMap;		/**< Mapping transformation as a matrix. */
>  };
>  
> +enum gl_map_buffer_index {
> +   MAP_CTX_USER,
> +   MAP_CTX_INTERNAL,
> +
> +   NUM_MAP_BUFFER_CONTEXTS
> +};
> +
> +/**
> + * Fields describing a mapped buffer range.
> + */
> +struct gl_map_buffer_context {
> +   GLbitfield AccessFlags; /**< Mask of GL_MAP_x_BIT flags */
> +   GLvoid *Pointer;     /**< User-space address of mapping */
> +   GLintptr Offset;     /**< Mapped offset */
> +   GLsizeiptr Length;   /**< Mapped length */
> +};
>  
>  /**
>   * GL_ARB_vertex/pixel_buffer_object buffer object
> @@ -1451,17 +1467,12 @@ struct gl_buffer_object
>     GLbitfield StorageFlags; /**< GL_MAP_PERSISTENT_BIT, etc. */
>     GLsizeiptrARB Size;  /**< Size of buffer storage in bytes */
>     GLubyte *Data;       /**< Location of storage either in RAM or VRAM. */
> -   /** Fields describing a mapped buffer */
> -   /*@{*/
> -   GLbitfield AccessFlags; /**< Mask of GL_MAP_x_BIT flags */
> -   GLvoid *Pointer;     /**< User-space address of mapping */
> -   GLintptr Offset;     /**< Mapped offset */
> -   GLsizeiptr Length;   /**< Mapped length */
> -   /*@}*/
>     GLboolean DeletePending;   /**< true if buffer object is removed from the hash */
>     GLboolean Written;   /**< Ever written to? (for debugging) */
>     GLboolean Purgeable; /**< Is the buffer purgeable under memory pressure? */
>     GLboolean Immutable; /**< GL_ARB_buffer_storage */
> +
> +   struct gl_map_buffer_context Mappings[NUM_MAP_BUFFER_CONTEXTS];
>  };
>  
>
On Thursday 06 February 2014, Eric Anholt wrote:
> Marek Olšák <maraeo@gmail.com> writes:
> 
> > From: Marek Olšák <marek.olsak@amd.com>
> >
> > OpenGL allows a buffer to be mapped only once, but we also map buffers
> > internally, e.g. in the software primitive restart fallback, for PBOs,
> > vbo_get_minmax_index, etc. This has always been a problem, but it will
> > be a bigger problem with persistent buffer mappings, which will prevent
> > all Mesa functions from mapping buffers for internal purposes.
> >
> > This adds a driver inteface to core Mesa which supports multiple buffer
> > mappings and allows 2 mappings: one for the GL user and one for Mesa.
> >
> > Note that Gallium supports an unlimited number of buffer and texture
> > mappings, so it's not really an issue for Gallium.
> >
> > This is just the interface change. Please review. If you don't like it,
> > feel free to propose how you would do it. Thank you.
> 
> Having an index makes sense to me -- you have to either have that in the
> caller, or have the MBR hook return a pointer to private storage that
> gets passed in to Unmap, which sucks more.
> 
> My only question is whether we want to make Offset/Length/Pointer be
> GL-only properties that are managed by glMapBuffer() and
> glMapBufferRange() for us, then have drivers like intel store whatever
> the need in their own private area.

The code in api_arrayelt.c uses the Pointer field, so it will be easier
to adapt it to the new interface if the driver hook continues to set it.

Fredrik
On Thu, Feb 6, 2014 at 11:08 PM, Eric Anholt <eric@anholt.net> wrote:
> Marek Olšák <maraeo@gmail.com> writes:
>
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> OpenGL allows a buffer to be mapped only once, but we also map buffers
>> internally, e.g. in the software primitive restart fallback, for PBOs,
>> vbo_get_minmax_index, etc. This has always been a problem, but it will
>> be a bigger problem with persistent buffer mappings, which will prevent
>> all Mesa functions from mapping buffers for internal purposes.
>>
>> This adds a driver inteface to core Mesa which supports multiple buffer
>> mappings and allows 2 mappings: one for the GL user and one for Mesa.
>>
>> Note that Gallium supports an unlimited number of buffer and texture
>> mappings, so it's not really an issue for Gallium.
>>
>> This is just the interface change. Please review. If you don't like it,
>> feel free to propose how you would do it. Thank you.
>
> Having an index makes sense to me -- you have to either have that in the
> caller, or have the MBR hook return a pointer to private storage that
> gets passed in to Unmap, which sucks more.
>
> My only question is whether we want to make Offset/Length/Pointer be
> GL-only properties that are managed by glMapBuffer() and
> glMapBufferRange() for us, then have drivers like intel store whatever
> the need in their own private area.

Offset/Length/Pointer are used by the vbo module and other code. It's
not easy to get rid of them.

Marek