From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH] drm/radeon: add an exclusive lock for GPU reset Date: Mon, 02 Jul 2012 19:05:51 +0200 Message-ID: <4FF1D4EF.3040806@vodafone.de> References: <1341243563-2559-1-git-send-email-j.glisse@gmail.com> <4FF1CBB5.2060208@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 37BD59E78F for ; Mon, 2 Jul 2012 10:05:53 -0700 (PDT) In-Reply-To: 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: Jerome Glisse Cc: Jerome Glisse , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 02.07.2012 18:41, Jerome Glisse wrote: > On Mon, Jul 2, 2012 at 12:26 PM, Christian K=F6nig > wrote: >> On 02.07.2012 17:39, j.glisse@gmail.com wrote: >>> From: Jerome Glisse >>> >>> GPU reset need to be exclusive, one happening at a time. For this >>> add a rw semaphore so that any path that trigger GPU activities >>> have to take the semaphore as a reader thus allowing concurency. >>> >>> The GPU reset path take the semaphore as a writer ensuring that >>> no concurrent reset take place. >>> >>> Signed-off-by: Jerome Glisse >> NAK, that isn't as bad as the cs mutex was but still to complicated. It's >> just too far up in the call stack, e.g. it tries to catch ioctl operatio= ns, >> instead of catching the underlying hardware operation which is caused by= the >> ioctl/ttm/etc... >> >> Why not just take the ring look as I suggested? >> >> > No we can't use ring lock, we need to protect suspend/resume path and > we need an exclusive lock for that so we need a reset mutex at the > very least. But instead of having a reset mutex i prefer using a rw > lock so that we can stop ioctl until a reset goes through an let > pending ioctl take proper action. Think about multiple context trying > to reset GPU ... > > Really this is the best option, the rw locking wont induce any lock > contention execept in gpu reset case which is anyway bad news. Why? That makes no sense to me. Well I don't want to prevent lock = contention, but understand why we should add locking at the ioctl level. = That violates locking rule number one "lock data instead of code" (or in = our case "lock hardware access instead of code path") and it really is = the reason why we ended up with the cs_mutex protecting practically = everything. Multiple context trying to reset the GPU should be pretty fine, current = it would just reset the GPU twice, but in the future asic_reset should = be much more fine grained and only reset the parts of the GPU which = really needs an reset. Cheers, Christian.