From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Volkin, Bradley D" Subject: Re: [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine Date: Mon, 19 May 2014 09:40:59 -0700 Message-ID: <20140519164059.GA21010@bdvolkin-ubuntu-desktop> References: <20140519122037.GK8790@phenom.ffwll.local> <92648605EABDA246B775AAB04C95A7A3012EF898@IRSMSX103.ger.corp.intel.com> <92648605EABDA246B775AAB04C95A7A3012EF990@IRSMSX103.ger.corp.intel.com> <20140519151155.GQ8790@phenom.ffwll.local> <92648605EABDA246B775AAB04C95A7A3012EFA46@IRSMSX103.ger.corp.intel.com> <20140519154938.GT8790@phenom.ffwll.local> <92648605EABDA246B775AAB04C95A7A3012EFB6A@IRSMSX103.ger.corp.intel.com> <20140519162418.GA20948@bdvolkin-ubuntu-desktop> <92648605EABDA246B775AAB04C95A7A3012EFC01@IRSMSX103.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id A26916E7E1 for ; Mon, 19 May 2014 09:40:30 -0700 (PDT) Content-Disposition: inline In-Reply-To: <92648605EABDA246B775AAB04C95A7A3012EFC01@IRSMSX103.ger.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Mateo Lozano, Oscar" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Mon, May 19, 2014 at 09:33:37AM -0700, Mateo Lozano, Oscar wrote: > > -----Original Message----- > > From: Volkin, Bradley D > > Sent: Monday, May 19, 2014 5:24 PM > > To: Mateo Lozano, Oscar > > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 06/50] drm/i915: > > s/intel_ring_buffer/intel_engine > > = > > On Mon, May 19, 2014 at 09:12:26AM -0700, Mateo Lozano, Oscar wrote: > > > BTW: do you want me to kill private_default_ctx as well? It doesn=B4t= look very > > useful... > > = > > Isn't private_default_ctx the one that's actually used when userspace s= pecifies > > DEFAULT_CONTEXT_ID? > = > What I see is a normal idr_find: Right, but i915_gem_context_open() does: idr_init(&file_priv->context_idr); file_priv->private_default_ctx =3D i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev)); And i915_gem_create_context() calls __create_hw_context(), which does: if (file_priv !=3D NULL) { ret =3D idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID, 0, GFP_KERNEL); if (ret < 0) goto err_out; } else ret =3D DEFAULT_CONTEXT_ID; So I think the idr_find() should indirectly give us private_default_ctx. Brad > = > struct i915_hw_context * > i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id) > { > struct i915_hw_context *ctx; > = > ctx =3D (struct i915_hw_context *)idr_find(&file_priv->context_idr, id); > if (!ctx) > return ERR_PTR(-ENOENT); > = > return ctx; > } > = > I think Chris has almost killed it off completely: > = > commit 691e6415c891b8b2b082a120b896b443531c4d45 > Author: Chris Wilson > Date: Wed Apr 9 09:07:36 2014 +0100 > = > drm/i915: Always use kref tracking for all contexts. > = > If we always initialize kref for the context, even if we are using fa= ke > contexts for hangstats when there is no hw support, we can forgo the > dance to dereference the ctx->obj and inspect whether we are permitted > to use kref inside i915_gem_context_reference() and _unreference(). > = > My ulterior motive here is to improve the debugging of a use-after-fr= ee > of ctx->obj. This patch avoids the dereference here and instead forces > the assertion checks associated with kref. > = > v2: Refactor the fake contexts to being even more like the real > contexts, so that there is much less duplicated and special case code. > = > v3: Tweaks. > v4: Tweaks, minor.