Re: [Intel-gfx] [PATCH 4/4] drm/i915/perf: Map OA buffer to user space for gen12 performance query

[Chris Wilson <chris.p.wilson@intel.com>]

Quoting Umesh Nerlige Ramappa (2020-07-21 03:17:59)
> Hi Chris,
> 
> I have added your comments, but I have a few questions below:
> 
> Thanks for your help,
> Umesh
> 
> On Mon, Jul 20, 2020 at 07:00:12PM -0700, Umesh Nerlige Ramappa wrote:
> >From: Piotr Maciejewski <piotr.maciejewski@intel.com>
> >+static void vm_open_oa(struct vm_area_struct *vma)
> >+{
> >+      struct i915_perf_stream *stream = vma->vm_private_data;
> >+
> >+      GEM_BUG_ON(!stream);
> >+      kref_get(&stream->refcount);
> 
> When user calls mmap, after this call the count is 2 (since kref_init 
> started off the count with 1).
> 
> >+}
> >+
> >+static void vm_close_oa(struct vm_area_struct *vma)
> >+{
> >+      struct i915_perf_stream *stream = vma->vm_private_data;
> >+
> >+      GEM_BUG_ON(!stream);
> >+      kref_put(&stream->refcount, free_stream);
> 
> When the user closes perf fd or calls unmap, vm_close_oa is called and 
> refcount goes to 1, so not sure how the refcount goes to 0 so that 
> free_stream is called. Should I kref_put when closing perf fd also?

Yes, at a glance, I would say i915_perf_destroy_locked() should replace
the kfree(stream) with a kref_put.

> >+static vm_fault_t vm_fault_oa(struct vm_fault *vmf)
> >+{
> >+      struct vm_area_struct *vma = vmf->vma;
> >+      struct i915_perf_stream *stream = vma->vm_private_data;
> >+      struct drm_i915_gem_object *obj = stream->oa_buffer.vma->obj;
> >+      int err;
> >+
> >+      if (READ_ONCE(stream->closed))
> >+              return VM_FAULT_SIGBUS;
> >+
> >+      err = i915_gem_object_pin_pages(obj);
> >+      if (err)
> >+              goto out;
> >+
> >+      err = remap_io_sg(vma,
> >+                        vma->vm_start, vma->vm_end - vma->vm_start,
> >+                        obj->mm.pages->sgl, -1);
> >+
> >+      i915_gem_object_unpin_pages(obj);
> >+
> >+out:
> >+      return i915_error_to_vmf_fault(err);
> >+}
> >+
> >+static const struct vm_operations_struct vm_ops_oa = {
> >+      .open = vm_open_oa,
> >+      .close = vm_close_oa,
> >+      .fault = vm_fault_oa,
> >+};
> >+
> >+int i915_perf_mmap(struct file *file, struct vm_area_struct *vma)
> >+{
> >+      struct i915_perf_stream *stream = file->private_data;
> >+
> >+      if (i915_perf_stream_paranoid && !perfmon_capable()) {
> >+              DRM_DEBUG("Insufficient privileges to map OA buffer\n");
> >+              return -EACCES;
> >+      }
> >+
> >+      switch (vma->vm_pgoff) {
> >+      case I915_PERF_OA_BUFFER_MMAP_OFFSET:
> >+              if (vma->vm_end - vma->vm_start > OA_BUFFER_SIZE)
> >+                      return -EINVAL;
> >+
> >+              if (vma->vm_flags & VM_WRITE)
> >+                      return -EINVAL;
> >+
> >+              break;
> >+
> >+      default:
> >+              return -EINVAL;
> >+      }
> >+
> >+      vma->vm_flags &= ~(VM_MAYWRITE | VM_MAYEXEC | VM_MAYSHARE);
> >+      vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> >+      vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> >+      vma->vm_private_data = stream;
> >+      vma->vm_ops = &vm_ops_oa;
> >+      vm_open_oa(vma);
> 
> I am calling the open explicitly here to get the reference, otherwise I 
> don't see open being called by anyone/anything.

The initial vm_ops->open is implicit, i.e. the mmap starts opened, and
the vm_ops->open() is only called when the vma is cloned (e.g. on
fork()).

> If user calls mmap multiple times, does he get a unique vma each time?

Yes.

> Do I need to track all of them (and their sizes) and then unmap them 
> when perf_fd is closed?

Yes... But we can zap them all with a single unmap_mapping_range() call,
since they all exist within the perf_fd->f_mapping.

> >diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> >index a36a455ae336..2efbe35c5fa9 100644
> >--- a/drivers/gpu/drm/i915/i915_perf_types.h
> >+++ b/drivers/gpu/drm/i915/i915_perf_types.h
> >@@ -311,6 +311,18 @@ struct i915_perf_stream {
> >        * buffer should be checked for available data.
> >        */
> >       u64 poll_oa_period;
> >+
> >+      /**
> >+       * @closed: Open or closed state of the stream.
> >+       * True if stream is closed.
> >+       */
> >+      bool closed;
> 
> closed is part of the stream itself, so I cannot kfree(stream) until the 
> refcount goes to 0.

Correct. kfree must be part of the kref_put cb.

> >+      /**
> >+       * @refcount: References to the mapped OA buffer managed by this
> >+       * stream.
> >+       */
> >+      struct kref refcount;
> > };
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx