Re: [PATCH] drm/xe/oa: Destroy the stream_lock mutex

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

On Fri, 28 Jun 2024 09:58:57 -0700, Umesh Nerlige Ramappa wrote:
>
> On Thu, Jun 27, 2024 at 10:21:25PM -0700, Ashutosh Dixit wrote:
> > The mutex allocated in xe_oa_stream_init() was never previously
> > destroyed. Do so now.
> >
> > Fixes: e936f885f1e9 ("drm/xe/oa/uapi: Expose OA stream fd")
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>
> Since we are holding a drm reference, what is the logic behind using
> drmm_mutex_init() for some mutexes and just a mutex_init for this one?

I was sort of expecting this question :)

So drmm_mutex_init() is being used for mutexes which are allocated in
device probe, so they will be automatically freed in device unbind/remove.

The mutex here is not a per device mutex, it's a per stream mutex. So
better IMO to free it when the stream closes, so that's why an explicit
mutex_destroy() during xe_oa_stream_destroy() here (since multiple OA
streams can be opened/closed between probe and remove).

(Though, considering that mutex_destroy() is a nop unless
CONFIG_DEBUG_MUTEXES is set (see include/linux/mutex.h), drmm_mutex_init()
will not be the end of the world either). But I thought just take the safe
way out.

Thanks.
--
Ashutosh

> > ---
> > drivers/gpu/drm/xe/xe_oa.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 6cc3f0217341..4188516a7816 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -824,6 +824,8 @@ static void xe_oa_stream_destroy(struct xe_oa_stream *stream)
> >
> >	WRITE_ONCE(u->exclusive_stream, NULL);
> >
> > +	mutex_destroy(&stream->stream_lock);
> > +
> >	xe_oa_disable_metric_set(stream);
> >	xe_exec_queue_put(stream->k_exec_q);
> >
> > --
> > 2.41.0
> >