From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 1/2] drm/radeon: Make sure CS mutex is held across GPU reset. Date: Fri, 16 Sep 2011 04:59:10 -0400 Message-ID: <20110916085910.GF884@phenom.oracle.com> References: <1316110861-17145-1-git-send-email-michel@daenzer.net> <20110916083144.GB884@phenom.oracle.com> <1316162423.17417.83.camel@thor.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from rcsinet15.oracle.com (rcsinet15.oracle.com [148.87.113.117]) by gabe.freedesktop.org (Postfix) with ESMTP id 10243A09F3 for ; Fri, 16 Sep 2011 01:59:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1316162423.17417.83.camel@thor.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Michel =?iso-8859-1?Q?D=E4nzer?= Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Fri, Sep 16, 2011 at 10:40:23AM +0200, Michel D=E4nzer 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=E4nzer wrote: > > > From: Michel D=E4nzer > > > = > > > This was only the case if the GPU reset was triggered from the CS ioc= tl, > > > 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 f= rom the > > > CS ioctl, in which case we're already holding the mutex, or from othe= r 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 ha= ndle this > > > with some helper functions. > > > = > > > Signed-off-by: Michel D=E4nzer > > > --- > > > 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 =3D mutex_trylock(&rdev->cs_mutex); > > > + if (took_mutex) { > > > + /* The mutex was unlocked before, so it's ours now */ > > > + rdev->cs_mutex_owner =3D current; > > > + have_mutex =3D true; > > = > > consider you set that here.. > > > + } else { > > > + /* Somebody already locked the mutex. Was it this process? */ > > > + have_mutex =3D (rdev->cs_mutex_owner =3D=3D current); > > > + } > > > + mutex_unlock(&rdev->cs_mutex_owner_mutex); > > > + > > > + if (!have_mutex) { > > > + /* Another process locked the mutex, take it */ > > > + cs_mutex_lock(rdev); > > > + took_mutex =3D 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.