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