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

[Eric Anholt]

Markus Wick <markus at selfnet.de> writes:

> 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.

These both sound good to me.  Squashed this in:

diff --git a/glamor/glamor_core.c b/glamor/glamor_core.c
index e4538f8..a6a6039 100644
--- a/glamor/glamor_core.c
+++ b/glamor/glamor_core.c
@@ -118,7 +118,7 @@ glamor_prepare_access(DrawablePtr drawable, glamor_access_t access)
                pixmap_priv->base.mapped_for_write);
         return TRUE;
     }
-    pixmap_priv->base.mapped_for_write = (access == GLAMOR_ACCESS_RW);
+    pixmap_priv->base.map_access = access;
 
     return glamor_download_pixmap_to_cpu(pixmap, access);
 }
@@ -314,11 +314,15 @@ glamor_finish_access(DrawablePtr drawable, glamor_access_t access_mode)
     if (!GLAMOR_PIXMAP_PRIV_HAS_FBO_DOWNLOADED(pixmap_priv))
         return;
 
-    /* If we are doing a series of unmaps from a nested map, we're done. */
+    /* If we are doing a series of unmaps from a nested map, we're
+     * done.  None of the callers do any rendering to maps after
+     * starting an unmap sequence, so we don't need to delay until the
+     * last nested unmap.
+     */
     if (!pixmap->devPrivate.ptr)
         return;
 
-    if (pixmap_priv->base.mapped_for_write) {
+    if (pixmap_priv->base.map_access == GLAMOR_ACCESS_RW) {
         glamor_restore_pixmap_to_texture(pixmap);
     }
 
diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
index f6d1b22..776de06 100644
--- a/glamor/glamor_priv.h
+++ b/glamor/glamor_priv.h
@@ -415,7 +415,7 @@ typedef struct glamor_pixmap_private_base {
      * glamor_prepare_access), determies whether we should re-upload
      * that data on glamor_finish_access().
      */
-    bool mapped_for_write;
+    glamor_access_t map_access;
     unsigned char is_picture:1;
     unsigned char gl_tex:1;
     glamor_pixmap_fbo *fbo;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140313/e055da90/attachment.pgp>