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

[Konrad Rzeszutek Wilk]

On Fri, Sep 16, 2011 at 10:40:23AM +0200, Michel Dänzer wrote:
> 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 at 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 at 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.

Oh, somehow I thought it was replacing the 'cs_mutex'.. but now that I look
again that is not the case.
> 
> Though now you got me thinking... Maybe the second mutex isn't necessary
> at all. Let me try that.

Ok.
> 
> 
> > > +/*
> > > + * 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.

Excellent.