From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH] drm/i915: Consolidate binding parameters into flags Date: Tue, 28 Jan 2014 15:09:42 +0200 Message-ID: <87lhy045ih.fsf@intel.com> References: <1390846259-14915-2-git-send-email-daniel.vetter@ffwll.ch> <1390862093-12195-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id D30BAFBF25 for ; Tue, 28 Jan 2014 05:06:13 -0800 (PST) In-Reply-To: <1390862093-12195-1-git-send-email-daniel.vetter@ffwll.ch> 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: Intel Graphics Development Cc: Daniel Vetter , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Tue, 28 Jan 2014, Daniel Vetter wrote: > Anything more than just one bool parameter is just a pain to read, > symbolic constants are much better. > > Split out from Chris' vma-binding rework patch. > > v2: Undo the behaviour change in object_pin that Chris spotted. > > Cc: Chris Wilson > Cc: Ben Widawsky > Signed-off-by: Daniel Vetter [snip] > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 032def901f98..9399a6fa4c2f 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -544,19 +544,23 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, > struct drm_i915_gem_object *obj = vma->obj; > struct drm_i915_gem_exec_object2 *entry = vma->exec_entry; > bool has_fenced_gpu_access = INTEL_INFO(ring->dev)->gen < 4; > - bool need_fence, need_mappable; > - u32 flags = (entry->flags & EXEC_OBJECT_NEEDS_GTT) && > - !vma->obj->has_global_gtt_mapping ? GLOBAL_BIND : 0; > + bool need_fence; > + unsigned flags; > int ret; > > + flags = 0; > + > need_fence = > has_fenced_gpu_access && > entry->flags & EXEC_OBJECT_NEEDS_FENCE && > obj->tiling_mode != I915_TILING_NONE; > - need_mappable = need_fence || need_reloc_mappable(vma); > + if (need_fence || need_reloc_mappable(vma)) > + flags |= PIN_MAPPABLE; > + > + if (entry->flags & EXEC_OBJECT_NEEDS_GTT) > + flags |= PIN_GLOBAL; It is not obvious to me that this together with the PIN_GLOBAL handling in i915_gem_object_pin() do not introduce a functional change. (Stress on obvious to _me_; it may be obvious to you.) I would have thought it better to first change the two bool parameters to two flags, and then add the new flag in a separate patch to not confuse poor reviewers like myself. [snip] > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index d897a19f887f..a0793c929b95 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -531,9 +531,11 @@ init_pipe_control(struct intel_ring_buffer *ring) > goto err; > } > > - i915_gem_object_set_cache_level(ring->scratch.obj, I915_CACHE_LLC); > + ret = i915_gem_object_set_cache_level(ring->scratch.obj, I915_CACHE_LLC); > + if (ret) > + goto err_unref; This should be split out from the patch just like patch 4/9. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center