From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Prevent recursion by retiring requests when the ring is full Date: Tue, 28 Jan 2014 13:15:35 +0200 Message-ID: <20140128111535.GV9454@intel.com> References: <1390862587-26669-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id D9AF8FC136 for ; Tue, 28 Jan 2014 03:15:39 -0800 (PST) Content-Disposition: inline In-Reply-To: <1390862587-26669-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Jan 27, 2014 at 10:43:07PM +0000, Chris Wilson wrote: > As the VM do not track activity of objects and instead use a large > hammer to forcibly idle and evict all of their associated objects when > one is released, it is possible for that to cause a recursion when we > need to wait for free space on a ring and call retire requests. > (intel_ring_begin -> intel_ring_wait_request -> > i915_gem_retire_requests_ring -> i915_gem_context_free -> > i915_gem_evict_vm -> i915_gpu_idle -> intel_ring_begin etc) > = > In order to remove the requirement for calling retire-requests from > intel_ring_wait_request, we have to inline a couple of steps from > retiring requests, notably we have to record the position of the request > we wait for and use that to update the available ring space. > = > Signed-off-by: Chris Wilson Looks good to me. Reviewed-by: Ville Syrj=E4l=E4 I do have a couple of questions about request->tail though. We set it to -1 in intel_ring_wait_request(). Isn't that going to cause problems for i915_request_guilty()? When not -1, request->tail points to just before the commands that .add_request() adds to the ring. So that means intel_ring_wait_request() might have to wait for one extra request, and I guess more importantly if the GPU hangs inside the .add_request() commands, we won't attribute the hang to the request in question. Was it designe to be that way, or is there a bug here? > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 25 +++++-------------------- > 1 file changed, 5 insertions(+), 20 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i9= 15/intel_ringbuffer.c > index 10ff32d09c14..0da7c257159a 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1431,28 +1431,16 @@ void intel_cleanup_ring_buffer(struct intel_ring_= buffer *ring) > cleanup_status_page(ring); > } > = > -static int intel_ring_wait_seqno(struct intel_ring_buffer *ring, u32 seq= no) > -{ > - int ret; > - > - ret =3D i915_wait_seqno(ring, seqno); > - if (!ret) > - i915_gem_retire_requests_ring(ring); > - > - return ret; > -} > - > static int intel_ring_wait_request(struct intel_ring_buffer *ring, int n) > { > struct drm_i915_gem_request *request; > - u32 seqno =3D 0; > + u32 seqno =3D 0, tail; > int ret; > = > - i915_gem_retire_requests_ring(ring); > - > if (ring->last_retired_head !=3D -1) { > ring->head =3D ring->last_retired_head; > ring->last_retired_head =3D -1; > + > ring->space =3D ring_space(ring); > if (ring->space >=3D n) > return 0; > @@ -1469,6 +1457,7 @@ static int intel_ring_wait_request(struct intel_rin= g_buffer *ring, int n) > space +=3D ring->size; > if (space >=3D n) { > seqno =3D request->seqno; > + tail =3D request->tail; > break; > } > = > @@ -1483,15 +1472,11 @@ static int intel_ring_wait_request(struct intel_r= ing_buffer *ring, int n) > if (seqno =3D=3D 0) > return -ENOSPC; > = > - ret =3D intel_ring_wait_seqno(ring, seqno); > + ret =3D i915_wait_seqno(ring, seqno); > if (ret) > return ret; > = > - if (WARN_ON(ring->last_retired_head =3D=3D -1)) > - return -ENOSPC; > - > - ring->head =3D ring->last_retired_head; > - ring->last_retired_head =3D -1; > + ring->head =3D tail; > ring->space =3D ring_space(ring); > if (WARN_ON(ring->space < n)) > return -ENOSPC; > -- = > 1.8.5.3 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC