Re: [PATCH 15/17] drm/xe/oa/uapi: OA buffer mmap

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 08 Jan 2024 11:50:40 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Thomas/Umesh,

> On Tue, Jan 02, 2024 at 12:16:12PM +0100, Thomas Hellström wrote:
> > On Thu, 2023-12-07 at 22:43 -0800, Ashutosh Dixit wrote:
> >> Allow the OA buffer to be mmap'd to userspace. This is needed for the
> >> MMIO
> >> trigger use case. Even otherwise, with whitelisted OA head/tail ptr
> >> registers, userspace can receive/interpret OA data from the mmap'd
> >> buffer
> >> without issuing read()'s on the OA stream fd.
> >>
> >> Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_oa.c | 53
> >> ++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 53 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> >> index 42f32d4359f2c..97779cbb83ee8 100644
> >> --- a/drivers/gpu/drm/xe/xe_oa.c
> >> +++ b/drivers/gpu/drm/xe/xe_oa.c
> >> @@ -898,6 +898,8 @@ static int xe_oa_alloc_oa_buffer(struct
> >> xe_oa_stream *stream)
> >>  		return PTR_ERR(bo);
> >>
> >>  	stream->oa_buffer.bo = bo;
> >> +	/* mmap implementation requires OA buffer to be in system
> >> memory */
> >> +	xe_assert(stream->oa->xe, bo->vmap.is_iomem == 0);
> >>  	stream->oa_buffer.vaddr = bo->vmap.vaddr;
> >>  	return 0;
> >>  }
> >> @@ -1174,6 +1176,9 @@ static int xe_oa_release(struct inode *inode,
> >> struct file *file)
> >>  	struct xe_oa_stream *stream = file->private_data;
> >>  	struct xe_gt *gt = stream->gt;
> >>
> >> +	/* Zap mmap's */
> >> +	unmap_mapping_range(file->f_mapping, 0, -1, 1);
> >> +
> >
> > Can release() get called at all if there is a live mapping()? Meaning
> > the unmap_mapping_range() shouldn't be needed?
>
> If user closes the fd, but has not called unmap, then release will not get
> called. If unmap_mapping_range() doesn't do anything extra compared to
> unmap(), then I agree that we coud drop this.

I am removing unmap_mapping_range. I checked and found that:

* If munmap is not called and fd is closed, release is not called
* However, release is called (even in the above case) when the process
  exits. But at process exit, I think we can safely assume that resources
  allocated for the process will be automatically freed. So there seems to
  be no reason to retain unmap_mapping_range.
* Also, in general, I am not seeing unmap_mapping_range being called in
  other places in the kernel which use remap_pfn_range.

So removing unmap_mapping_range because of the above reasons.

Thanks.
--
Ashutosh