From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id C9A036E21A for ; Fri, 15 Oct 2021 03:42:33 +0000 (UTC) Date: Thu, 14 Oct 2021 20:41:39 -0700 Message-ID: <87lf2vc5u4.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20211015033103.GB3607@zkempczy-mobl2> References: <20211014081917.15977-1-zbigniew.kempczynski@intel.com> <20211014081917.15977-8-zbigniew.kempczynski@intel.com> <87r1cncquu.wl-ashutosh.dixit@intel.com> <20211015024905.GA3607@zkempczy-mobl2> <87mtnbc6ww.wl-ashutosh.dixit@intel.com> <20211015033103.GB3607@zkempczy-mobl2> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-2 Content-Transfer-Encoding: quoted-printable Subject: Re: [igt-dev] [PATCH i-g-t 7/7] benchmarks/gem_exec_fault: Add softpin mode to support gens with ppgtt List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Zbigniew =?ISO-8859-2?Q?Kempczy=F1ski?= Cc: igt-dev@lists.freedesktop.org, Petri Latvala List-ID: On Thu, 14 Oct 2021 20:31:03 -0700, Zbigniew Kempczy=F1ski wrote: > > On Thu, Oct 14, 2021 at 08:18:23PM -0700, Dixit, Ashutosh wrote: > > On Thu, 14 Oct 2021 19:49:05 -0700, Zbigniew Kempczy=F1ski wrote: > > > > > > On Thu, Oct 14, 2021 at 01:07:37PM -0700, Dixit, Ashutosh wrote: > > > > On Thu, 14 Oct 2021 01:19:17 -0700, Zbigniew Kempczy=F1ski wrote: > > > > > > > > > > @@ -127,9 +151,14 @@ static int loop(uint64_t size, unsigned ring= , int reps, int ncpus, > > > > > obj.alignment =3D 0; > > > > > gem_execbuf(fd, &execbuf); > > > > > > > > > > - /* fault out */ > > > > > - obj.alignment =3D 1ull << 63; > > > > > - __gem_execbuf(fd, &execbuf); > > > > > + if (ahnd) { > > > > > + obj.offset =3D get_offset(ahnd, obj.handle, size, 0); > > > > > + obj.flags |=3D EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_4= 8B_ADDRESS; > > > > > + } else { > > > > > + /* fault out */ > > > > > + obj.alignment =3D 1ull << 63; > > > > > + __gem_execbuf(fd, &execbuf); > > > > > + } > > > > > > > > Bug above, __gem_execbuf should be moved out of the else {}. > > > > > > No, it shouldn't. Normal execbuf will lead to unbind/bind with new of= fset > > > and no 'alignment' fault-out execbuf is necessary. > > > > Ah, you are right. Though in that case I think, if the loop has N > > iterations, the number of binds is N and the number of unbinds will be = (N - > > 1). Is it worth fixing that? Basically I think we might need to add a b= ind > > outside the first iteration of the loop so that we have an unbind in the > > first iteration itself. Then we will have N binds and N unbinds I think. > > You're right, for softpin case we got N-1. But I don't think we want to > compare results between alignment / softpin paths but for dedicated chang= es > in the kernel. So then that missing unbind doesn't matter. I thought it is benchmarking the time taken for N binds and N unbinds so not too sure how much difference N binds and N-1 unbinds make. Anyway please go ahead and merge if you think it's fine. Reviewed-by: Ashutosh Dixit