Re: [PATCH 3/3] drm/xe/oa: Destroy dangling oa streams when xe_file is removed

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

On Thu, 15 Aug 2024 11:17:05 -0700, Lucas De Marchi wrote:
>

Hi Jose,

> On Thu, Aug 15, 2024 at 09:27:58AM GMT, Jose Souza wrote:
> > If an application opens an oa stream and for whatever reason it don't
> > close the stream the oa unit associeted with it will never be release,
> > so no other application can use it.
> > The only workaround was to unload the driver or reboot.

The premise of this patch and the need for it seems misplaced. How are you
reproducing this?

OA stream open returns a fd. Whether or not the process closes the stream,
fd will get closed on process exit, which triggers the the release method
(xe_oa_release(), specified in xe_oa_fops), which does the cleanup and
makes the OA unit available again. I just verified this works without
closing the OA stream or reloading the module.

> > So here adding a list of oa stream and and when a xe_file is closed
> > it will also destroy all oa stream with the same xe_file.

We will also need to explain why something like this was never needed in
i915, since this mechanism is the same between i915 and xe.

> >
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_device.c   |  2 ++
> > drivers/gpu/drm/xe/xe_oa.c       | 28 +++++++++++++++++++++++++++-
> > drivers/gpu/drm/xe/xe_oa.h       |  1 +
> > drivers/gpu/drm/xe/xe_oa_types.h |  8 ++++++++
> > 4 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 2063283871503..b098685ed3636 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -158,6 +158,8 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
> >
> >	xe_pm_runtime_get(xe);
> >
> > +	xe_oa_file_destroy_stream(xef);
>
> I'm not sure about this impl giving xef for oa to delete from a list,
> will leave the to Ashutosh. Comment below about a mistake though
>
> > static int xe_oa_alloc_oa_buffer(struct xe_oa_stream *stream)
> > @@ -1480,11 +1484,15 @@ static int xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
> >		goto exit;
> >	}
> >
> > +	stream->xef = param->xef;
>
> if you keep a pointer to xef, you need to get a ref. Use
> xe_file_get()/xe_file_put().

Done here:
https://patchwork.freedesktop.org/patch/607603/?series=137058&rev=1

Thanks.
--
Ashutosh