Re: [PATCH 1/2] drm/radeon: Make sure CS mutex is held across GPU reset.

["Michel Dänzer" <michel@daenzer.net>]

On Fre, 2011-09-16 at 04:31 -0400, Konrad Rzeszutek Wilk wrote: 
> On Thu, Sep 15, 2011 at 08:21:00PM +0200, Michel Dänzer wrote:
> > From: Michel Dänzer <michel.daenzer@amd.com>
> > 
> > This was only the case if the GPU reset was triggered from the CS ioctl,
> > otherwise other processes could happily enter the CS ioctl and wreak havoc
> > during the GPU reset.
> > 
> > This is a little complicated because the GPU reset can be triggered from the
> > CS ioctl, in which case we're already holding the mutex, or from other call
> > paths, in which case we need to lock the mutex. AFAICT the mutex API doesn't
> > allow nested locking or finding out the mutex owner, so we need to handle this
> > with some helper functions.
> > 
> > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> > ---
> >  drivers/gpu/drm/radeon/radeon.h        |   60 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/radeon/radeon_cs.c     |   14 ++++----
> >  drivers/gpu/drm/radeon/radeon_device.c |   16 ++++++++
> >  3 files changed, 83 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> > index ef0e0e0..89304d9 100644
> > --- a/drivers/gpu/drm/radeon/radeon.h
> > +++ b/drivers/gpu/drm/radeon/radeon.h
> > @@ -1203,6 +1203,8 @@ struct radeon_device {
> >  	struct radeon_pm		pm;
> >  	uint32_t			bios_scratch[RADEON_BIOS_NUM_SCRATCH];
> >  	struct mutex			cs_mutex;
> > +	struct task_struct		*cs_mutex_owner;
> > +	struct mutex			cs_mutex_owner_mutex;
> 
> That is a bit of 'mutex.. mutex'. What about just having the same
> name as before?

What do you mean? This adds a second mutex protecting the owner field.

Though now you got me thinking... Maybe the second mutex isn't necessary
at all. Let me try that.


> > +/*
> > + * Check if this process locked the CS mutex already; if it didn't, lock it.
> > + *
> > + * Returns:
> > + * true: This function locked the mutex.
> > + * false: This function didn't lock the mutex (this process already locked it
> > + * before), so the caller probably shouldn't unlock it.
> > + */
> > +static inline int cs_mutex_ensure_locked(struct radeon_device *rdev)
> > +{
> > +	int took_mutex;
> > +	int have_mutex;
> 
> I think you meant 'bool'?
> 
> > +
> > +	mutex_lock(&rdev->cs_mutex_owner_mutex);
> > +	took_mutex = mutex_trylock(&rdev->cs_mutex);
> > +	if (took_mutex) {
> > +		/* The mutex was unlocked before, so it's ours now */
> > +		rdev->cs_mutex_owner = current;
> > +		have_mutex = true;
> 
> consider you set that here..
> > +	} else {
> > +		/* Somebody already locked the mutex. Was it this process? */
> > +		have_mutex = (rdev->cs_mutex_owner == current);
> > +	}
> > +	mutex_unlock(&rdev->cs_mutex_owner_mutex);
> > +
> > +	if (!have_mutex) {
> > +		/* Another process locked the mutex, take it */
> > +		cs_mutex_lock(rdev);
> > +		took_mutex = true;
> > +	}
> > +
> > +	return took_mutex;
> 
> and if it is really going to be bool, you might as well change the
> return signature and make it the function decleration return bool
> instead of int.

Yeah, I can change that. I'll send a v2 patch.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel