From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/6] drm/i915: Allow late allocation of request for i915_add_request() Date: Thu, 12 Jul 2012 19:43:47 +0200 Message-ID: <20120712174347.GL5039@phenom.ffwll.local> References: <1342106019-17806-1-git-send-email-chris@chris-wilson.co.uk> <1342106019-17806-2-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f44.google.com (mail-wg0-f44.google.com [74.125.82.44]) by gabe.freedesktop.org (Postfix) with ESMTP id 4C5419E7B1 for ; Thu, 12 Jul 2012 10:43:47 -0700 (PDT) Received: by wgbdr13 with SMTP id dr13so1676761wgb.13 for ; Thu, 12 Jul 2012 10:43:46 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1342106019-17806-2-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Jul 12, 2012 at 04:13:34PM +0100, Chris Wilson wrote: > Request preallocation was added to i915_add_request() in order to > support the overlay. However, not all uses care and can quite happily > ignore the failure to allocate the request as they will simply repeat > the request in the future. > > By pushing the allocation down into i915_add_request(), we can then > remove some rather ugly error handling in the callers. > > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/i915_drv.h | 6 ++--- > drivers/gpu/drm/i915/i915_gem.c | 39 ++++++++++------------------ > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +---- > 3 files changed, 17 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 627fe35..51e234e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1358,9 +1358,9 @@ void i915_gem_init_ppgtt(struct drm_device *dev); > void i915_gem_cleanup_ringbuffer(struct drm_device *dev); > int __must_check i915_gpu_idle(struct drm_device *dev); > int __must_check i915_gem_idle(struct drm_device *dev); > -int __must_check i915_add_request(struct intel_ring_buffer *ring, > - struct drm_file *file, > - struct drm_i915_gem_request *request); > +int i915_add_request(struct intel_ring_buffer *ring, > + struct drm_file *file, > + struct drm_i915_gem_request *request); > int __must_check i915_wait_seqno(struct intel_ring_buffer *ring, > uint32_t seqno); > int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 53c3946..875b745 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1597,7 +1597,6 @@ i915_add_request(struct intel_ring_buffer *ring, > ring->gpu_caches_dirty = false; > } > > - BUG_ON(request == NULL); > seqno = i915_gem_next_request_seqno(ring); > > /* Record the position of the start of the request so that > @@ -1609,7 +1608,13 @@ i915_add_request(struct intel_ring_buffer *ring, > > ret = ring->add_request(ring, &seqno); > if (ret) > - return ret; > + return ret; > + > + if (request == NULL) { > + request = kmalloc(sizeof(*request), GFP_KERNEL); > + if (request == NULL) > + return -ENOMEM; > + } Hm, shouldn't we try to allocate the request struct before we emit the request? Otherwise looks good. -Daniel > > trace_i915_gem_request_add(ring, seqno); > > @@ -1859,14 +1864,8 @@ i915_gem_retire_work_handler(struct work_struct *work) > */ > idle = true; > for_each_ring(ring, dev_priv, i) { > - if (ring->gpu_caches_dirty) { > - struct drm_i915_gem_request *request; > - > - request = kzalloc(sizeof(*request), GFP_KERNEL); > - if (request == NULL || > - i915_add_request(ring, NULL, request)) > - kfree(request); > - } > + if (ring->gpu_caches_dirty) > + i915_add_request(ring, NULL, NULL); > > idle &= list_empty(&ring->request_list); > } > @@ -1913,25 +1912,13 @@ i915_gem_check_wedge(struct drm_i915_private *dev_priv, > static int > i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno) > { > - int ret = 0; > + int ret; > > BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex)); > > - if (seqno == ring->outstanding_lazy_request) { > - struct drm_i915_gem_request *request; > - > - request = kzalloc(sizeof(*request), GFP_KERNEL); > - if (request == NULL) > - return -ENOMEM; > - > - ret = i915_add_request(ring, NULL, request); > - if (ret) { > - kfree(request); > - return ret; > - } > - > - BUG_ON(seqno != request->seqno); > - } > + ret = 0; > + if (seqno == ring->outstanding_lazy_request) > + ret = i915_add_request(ring, NULL, NULL); > > return ret; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 88e2e11..0c26692 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -967,16 +967,11 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev, > struct drm_file *file, > struct intel_ring_buffer *ring) > { > - struct drm_i915_gem_request *request; > - > /* Unconditionally force add_request to emit a full flush. */ > ring->gpu_caches_dirty = true; > > /* Add a breadcrumb for the completion of the batch buffer */ > - request = kzalloc(sizeof(*request), GFP_KERNEL); > - if (request == NULL || i915_add_request(ring, file, request)) { > - kfree(request); > - } > + (void)i915_add_request(ring, file, NULL); > } > > static int > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48