Re: [PATCH 42/72] drm: generalized bo unmap

On Tue, Dec 29, 2015 at 10:17:20AM +0100, Enrico Weigelt, metux IT consult wrote:
> We have separate implementations of unmapping the bo's from
> process address space, which are essentially doing the same.
> 
> Signed-off-by: Enrico Weigelt, metux IT consult <[email protected]>
> ---
>  src/drm/cairo-drm-bo.c             | 16 ++++++++++++++++
>  src/drm/cairo-drm-i915-surface.c   |  6 ++----
>  src/drm/cairo-drm-i965-surface.c   |  5 ++---
>  src/drm/cairo-drm-intel-private.h  |  4 ----
>  src/drm/cairo-drm-intel.c          | 30 +++++++++++-------------------
>  src/drm/cairo-drm-private.h        |  6 ++++++
>  src/drm/cairo-drm-radeon-private.h |  5 -----
>  src/drm/cairo-drm-radeon-surface.c |  4 ++--
>  src/drm/cairo-drm-radeon.c         | 23 +++++++----------------
>  9 files changed, 46 insertions(+), 53 deletions(-)
> 
> diff --git a/src/drm/cairo-drm-bo.c b/src/drm/cairo-drm-bo.c
> index c82f933..6b946ac 100644
> --- a/src/drm/cairo-drm-bo.c
> +++ b/src/drm/cairo-drm-bo.c
> @@ -34,6 +34,7 @@
>  #include <sys/ioctl.h>
>  #include <errno.h>
>  #include <libdrm/drm.h>
> +#include <sys/mman.h>  /* munmap() */
>  
>  #define ERR_DEBUG(x) x
>  
> @@ -92,8 +93,23 @@ _cairo_drm_bo_close (const cairo_drm_device_t *dev,
>      struct drm_gem_close close;
>      int ret;
>  
> +    _cairo_drm_bo_unmap (bo);
> +
>      close.handle = bo->handle;
>      do {
>  	ret = ioctl (dev->fd, DRM_IOCTL_GEM_CLOSE, &close);
>      } while (ret == -1 && errno == EINTR);
>  }
> +
> +void
> +_cairo_drm_bo_unmap (cairo_drm_bo_t *bo)
> +{
> +    if (unlikely(bo == NULL))
> +	return;
> +
> +    if (unlikely(bo->mapped == NULL))
> +	return;
> +
> +    munmap (bo->mapped, bo->size);
> +    bo->mapped = NULL;
> +}

What is the lifetime expected to be?

It is very important to keep mmapings around for as long as possible,
even to reuse mmappings across different generations of the bo (if the
name remains) i.e. we should only unmap in close. (I haven't checked to
see what remains of the bo-cache.)

> @@ -200,6 +204,8 @@ static cairo_always_inline void
>  cairo_drm_bo_destroy (cairo_device_t *abstract_device,
>  		      cairo_drm_bo_t *bo)
>  {
> +    _cairo_drm_bo_unmap (bo);
> +
>      if (_cairo_reference_count_dec_and_test (&bo->ref_count)) {

For instance, this looks very odd.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo