Re: [PATCH 1/3] drm/xe/oa: Use xe_map layer

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

On Mon, 13 Apr 2026 11:05:09 -0700, Umesh Nerlige Ramappa wrote:
>
> On Fri, Apr 10, 2026 at 05:49:03PM -0700, Dixit, Ashutosh wrote:
> > On Thu, 09 Apr 2026 16:17:52 -0700, Ashutosh Dixit wrote:
> >>
> >
> > Hi Umesh,
> >
> >> +/* Because there is no iosys_map_copy_to_user() or copy_to_user_from_iomem() */
> >> +static int xe_oa_copy_to_user(struct xe_oa_stream *stream, void __user *dst, u32 head, u32 len)
> >> +{
> >> +	u32 __user *dptr = dst;
> >> +
> >> +	xe_gt_assert(stream->gt, len % 4 == 0);
> >> +
> >> +	for (int i = 0; i < len / 4; i++) {
> >> +		struct iosys_map *map = &stream->oa_buffer.bo->vmap;
> >> +		u32 val = xe_map_rd(stream->oa->xe, map, head, u32);
> >> +		int ret = put_user(val, dptr++);
> >> +
> >> +		if (ret)
> >> +			return ret;
> >> +		head += 4;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >
> > Unfortunately, with this copy_to_user() implementation we are seeing buffer
> > overflow, implying there is significant performance impact, even when OA
> > buffer is in system memory:
> >
> >  Starting subtest: non-zero-reason
> >  Starting dynamic subtest: oag-0
> >  (xe_oa:6798) CRITICAL: Test assertion failure function test_non_zero_reason, file ../tests/intel/xe_oa.c:2693:
> >  (xe_oa:6798) CRITICAL: Failed assertion: !(oa_status & DRM_XE_OASTATUS_BUFFER_OVERFLOW)
> >
> > That is, we are unable to read data at the rate at which HW is writing
> > data, causing buffer overrun.
> >
> > Therefore in v3 I have basically reverted to the copy_to_user()
> > implementation of v1, except that I have taken the copy_to_user() out of
> > xe_oa.c and moved it to xe_map.h, to make that implementation "official"
> > (and OA code to strictly use the xe_map layer). This seems to be the best
> > approach to me, it avoids unnecessary copies and works correctly on all of
> > our systems of interest.
> >
> > Also, the previous code, without this patch, is of course doing this exact
> > same thing, except that the use of the xe_map layer make these assumptions
> > explicit.
> >
> > If you still don't think we should be doing this, I can try to get a second
> > opinion about this patch. Thanks.
>
> Yes, please include someone who can give us an Ack or some inputs on this
> approach because we are hitting some issues.

I have asked. What I posted in v3 is my preferred option since it avoids the
extra copy. But if that doesn't go through, the function below has the extra
copy, but seems to have the same performace as v3, so that is also an option:

static int xe_oa_copy_to_user(struct xe_oa_stream *stream, void __user *dst, u32 head, u32 len)
{
       struct iosys_map *map = &stream->oa_buffer.bo->vmap;
       u8 buf[1024]; // Allocate as part of stream

       xe_map_memcpy_from(xe, buf, map, head, len);
       return copy_to_user(dst, buf, len);
}

So I am planning to try this next, if v3 doesn't go through. Thanks.