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.daen...@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.daen...@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
