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

[Robert Bragg]

On Thu, Oct 20, 2016 at 11:10 PM, Chris Wilson <chris at chris-wilson.co.uk>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161021/f35d133f/attachment.html>