Markus Wick <[email protected]> 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 <[email protected]> >> --- >> 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;
pgp24dWx06STG.pgp
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
