[PATCH 18/27] glamor: Allow nested mapping of pixmaps.

[Markus Wick]

Am 2014-03-11 22:30, schrieb Eric Anholt:
> The common pattern is to do nested if statements making calls to
> prepare_access() and then pop those mappings back off in each set of
> braces.  Some cases checked for src == dst to avoid leaking mappings,
> but others didn't.  Others didn't even do the nested mappings, so a
> failure in the outer map would result in trying to umap the inner and
> failing.
> 
> By allowing nested mappings, we can fix both problems by not requiring
> the care from the caller, plus we can allow a simpler nesting of all
> the prepares in one if statement.
> 
> Signed-off-by: Eric Anholt <eric at anholt.net>
> ---
>  glamor/glamor_core.c | 19 ++++++++++++++++++-
>  glamor/glamor_priv.h |  6 ++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/glamor/glamor_core.c b/glamor/glamor_core.c
> index 5883809..7a7ca08 100644
> --- a/glamor/glamor_core.c
> +++ b/glamor/glamor_core.c
> @@ -105,6 +105,19 @@ Bool
>  glamor_prepare_access(DrawablePtr drawable, glamor_access_t access)
>  {
>      PixmapPtr pixmap = glamor_get_drawable_pixmap(drawable);
> +    glamor_pixmap_private *pixmap_priv = 
> glamor_get_pixmap_private(pixmap);
> +
> +    if (pixmap->devPrivate.ptr) {
> +        /* Already mapped, nothing needs to be done.  Note that we
> +         * aren't allowing promotion from RO to RW, because it would
> +         * require re-mapping the PBO.
> +         */
> +        assert(!GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv) ||
> +               access == GLAMOR_ACCESS_RO ||
> +               pixmap_priv->base.mapped_for_write);
> +        return TRUE;
> +    }
> +    pixmap_priv->base.mapped_for_write = (access == GLAMOR_ACCESS_RW);
> 
>      return glamor_download_pixmap_to_cpu(pixmap, access);
>  }
> @@ -300,7 +313,11 @@ glamor_finish_access(DrawablePtr drawable,
> glamor_access_t access_mode)
>      if (!GLAMOR_PIXMAP_PRIV_HAS_FBO_DOWNLOADED(pixmap_priv))
>          return;
> 
> -    if (access_mode != GLAMOR_ACCESS_RO) {
> +    /* If we are doing a series of unmaps from a nested map, we're 
> done. */
> +    if (!pixmap->devPrivate.ptr)
> +        return;

In my opinion, there should be a note that this will unmap on the 
innermost call to finish_access, but the outer functions maybe still 
want to access this pixmap.

> +
> +    if (pixmap_priv->base.mapped_for_write) {
>          glamor_restore_pixmap_to_texture(pixmap);
>      }
> 
> diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
> index 7f41025..24a3575 100644
> --- a/glamor/glamor_priv.h
> +++ b/glamor/glamor_priv.h
> @@ -410,6 +410,12 @@ typedef struct glamor_pixmap_clipped_regions {
>  typedef struct glamor_pixmap_private_base {
>      glamor_pixmap_type_t type;
>      enum glamor_fbo_state gl_fbo;
> +    /**
> +     * If devPrivate.ptr is non-NULL (meaning we're within
> +     * glamor_prepare_access), determies whether we should re-upload
> +     * that data on glamor_finish_access().
> +     */
> +    bool mapped_for_write;

Why haven't you used the enum glamor_access? Memory footprint shouldn't 
matter that much.

>      unsigned char is_picture:1;
>      unsigned char gl_tex:1;
>      glamor_pixmap_fbo *fbo;