Re: [PATCH] on top of daniel

From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] on top of daniel
Date: Thu, 22 Sep 2011 09:39:21 +0200	[thread overview]
Message-ID: <20110922073921.GD2924@phenom.ffwll.local> (raw)
In-Reply-To: <1316656031-11239-3-git-send-email-ben@bwidawsk.net>

On Wed, Sep 21, 2011 at 06:47:11PM -0700, Ben Widawsky wrote:
> Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  include/drm/i915_drm.h   |    7 +++++++
>  intel/intel_bufmgr_gem.c |   36 ++++++++++++++++++++++++++++++------
>  2 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index adc2392..4b62222 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -189,6 +189,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_OVERLAY_PUT_IMAGE	0x27
>  #define DRM_I915_OVERLAY_ATTRS	0x28
>  #define DRM_I915_GEM_EXECBUFFER2	0x29
> +#define DRM_I915_GEM_GET_CACHE_TYPE	0x2a
>  
>  #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
>  #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -230,6 +231,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_MADVISE	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
>  #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE	DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
>  #define DRM_IOCTL_I915_OVERLAY_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
> +#define DRM_IOCTL_I915_GEM_GET_CACHE_TYPE	DRM_IOR  (DRM_COMMAND_BASE + DRM_I915_GEM_GET_CACHE_TYPE, struct drm_i915_gem_get_cache_type)
>  
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -835,4 +837,9 @@ struct drm_intel_overlay_attrs {
>  	__u32 gamma5;
>  };
>  
> +struct drm_i915_gem_get_cache_type {
> +	__u32 handle;
> +	__u32 cache_level;
> +};
> +
>  #endif				/* _I915_DRM_H_ */
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index ff4b663..65a77d6 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -1032,6 +1032,30 @@ static int do_mmap_cpu(drm_intel_bufmgr_gem *bufmgr_gem,
>  	return 0;
>  }
>  
> +enum i915_cache_level {
> +	I915_CACHE_NONE,
> +	I915_CACHE_LLC,
> +	I915_CACHE_LLC_MLC, /* gen6+ */
> +};
> +
> +static int get_cache_type(drm_intel_bo *bo)
> +{
> +	struct drm_i915_gem_get_cache_type cache = {0, 0};
> +	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
> +	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
> +	int ret = 0;
> +
> +	cache.handle = bo_gem->gem_handle;
> +	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_GET_CACHE_TYPE,
> +		       &cache);
> +
> +	/* This should maintain old behavior */
> +	if (ret)
> +		return 0;

Can you return I915_CACHE_NONE instead of this magic 0?

> +	return cache.cache_level;

Can we do

#define CACHE_LEVEL 0xff
	return cache.cache_leve & CACHE_LEVEL

instead so that we retain a few bits for flags (e.g. gfdt or whatever
other crazy stuff future hw might bring)? Alternatively, just add 64 bits
of pad to the ioctl struct.

> +}
> +
>  static int drm_intel_gem_bo_map(drm_intel_bo *bo, int write_enable)
>  {
>  	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
> @@ -1065,8 +1089,8 @@ static int drm_intel_gem_bo_map(drm_intel_bo *bo, int write_enable)
>  		    strerror(errno));
>  	}
>  
> -	/* TODO ask kernel if llc cached, in that case don't clear this */
> -	bo_gem->in_gtt_domain = 0;
> +	if (get_cache_type(bo) != I915_CACHE_LLC)
> +		bo_gem->in_gtt_domain = 0;
>  
>  	pthread_mutex_unlock(&bufmgr_gem->lock);
>  
> @@ -1335,14 +1359,14 @@ int drm_intel_gem_bo_map_nonblocking(drm_intel_bo *bo)
>  	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
>  	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
>  	struct drm_i915_gem_set_domain set_domain;
> -	int gpu_coherent_cpu_map;
> +	int gpu_coherent_cpu_map = 0;
>  	int ret;
>  
>  	pthread_mutex_lock(&bufmgr_gem->lock);
> -	assert(bo_gem->tiling_mode != I915_TILING_NONE);
> +	assert(bo_gem->tiling_mode == I915_TILING_NONE);

Oops, good catch ;-)

> -	/* TODO ask the kernel about this */
> -	gpu_coherent_cpu_map = 1;
> +	if (bufmgr_gem->gen >= 6)
> +		gpu_coherent_cpu_map = 1;
>  
>  	if (gpu_coherent_cpu_map) {
>  		ret = do_mmap_cpu(bufmgr_gem, bo_gem);
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48