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

[Robert Bragg <robert@xxxxxxxxxxxxx>]

On Thu, Oct 20, 2016 at 11:10 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:

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!

hmm, yeah it looks like I unwittingly added this recently as part of a rebase, I think from lazily copying some similar code from intel_ringbuffer.c when I hit a conflict, without thinking more carefully, sorry.

 

> +     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).

right, it looks like this can be cleaned up.

 

> +     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;
> +     }

--
Chris Wilson, Intel Open Source Technology Centre

Thanks,

- Robert

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx