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