Re: [PATCH v6 06/11] drm/i915: Enable i915 perf stream for Haswell OA unit

On Thu, Oct 20, 2016 at 10:19:05PM +0100, Robert Bragg wrote:

+int i915_gem_context_pin_legacy_rcs_state(struct drm_i915_private *dev_priv,

• 			  struct i915_gem_context *ctx,
  

• 			  u64 flags)
  

This is still no.

+static int alloc_oa_buffer(struct drm_i915_private *dev_priv) +{

• struct drm_i915_gem_object *bo;
• enum i915_map_type map;
• struct i915_vma *vma;
• int ret;
• BUG_ON(dev_priv->perf.oa.oa_buffer.obj);
• ret = i915_mutex_lock_interruptible(&dev_priv->drm);
• if (ret)

• return ret;
  

• BUILD_BUG_ON_NOT_POWER_OF_2(OA_BUFFER_SIZE);
• BUILD_BUG_ON(OA_BUFFER_SIZE < SZ_128K || OA_BUFFER_SIZE > SZ_16M);
• bo = i915_gem_object_create(&dev_priv->drm, OA_BUFFER_SIZE);
• if (IS_ERR(bo)) {

• DRM_ERROR("Failed to allocate OA buffer\n");
  

• ret = PTR_ERR(bo);
  

• goto unlock;
  

• }
• dev_priv->perf.oa.oa_buffer.obj = bo;
• ret = i915_gem_object_set_cache_level(bo, I915_CACHE_LLC);
• if (ret)

• goto err_unref;
  

• /* PreHSW required 512K alignment, HSW requires 16M */
• vma = i915_gem_object_ggtt_pin(bo, NULL, 0, SZ_16M, PIN_MAPPABLE);
• if (IS_ERR(vma)) {

• ret = PTR_ERR(vma);
  

• goto err_unref;
  

• }
• dev_priv->perf.oa.oa_buffer.vma = vma;
• map = HAS_LLC(dev_priv) ? I915_MAP_WB : I915_MAP_WC;

You set the hw up to do coherent writes into the CPU cache, and then you request WC access to the pages? With set_cache_level(LLC) you can use MAP_WB on both llc and snoop based architectures. Fortunately this is only HSW!

• dev_priv->perf.oa.oa_buffer.gtt_offset = i915_ggtt_offset(vma);

I haven't spotted the advantage of storing both the ggtt_offset in addition to the vma (or the bo as well as the vma).

• dev_priv->perf.oa.oa_buffer.addr = i915_gem_object_pin_map(bo, map);
• if (IS_ERR(dev_priv->perf.oa.oa_buffer.addr)) {

• ret = PTR_ERR(dev_priv->perf.oa.oa_buffer.addr);
  

• goto err_unpin;
  

• }