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@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > > ---
> > > 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
