Re: [PATCH 01/51] drm/i915: Rename 'flags' to 'dispatch_flags' for better code reading

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 27, 2015 at 12:14:06PM +0000, John Harrison wrote:
> On 25/02/2015 21:34, Daniel Vetter wrote:
> >On Fri, Feb 13, 2015 at 11:48:10AM +0000, John.C.Harrison@xxxxxxxxx wrote:
> >>From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >>
> >>There is a flags word that is passed through the execbuffer code path all the
> >>way from initial decoding of the user parameters down to the very final dispatch
> >>buffer call. It is simply called 'flags'. Unfortuantely, there are many other
> >>flags words floating around in the same blocks of code. Even more once the GPU
> >>scheduler arrives.
> >>
> >>This patch makes it more obvious exactly which flags word is which by renaming
> >>'flags' to 'dispatch_flags'. Note that the bit definitions for this flags word
> >>already have an 'I915_DISPATCH_' prefix on them and so are not quite so
> >>ambiguous.
> >>
> >>For: VIZ-1587
> >I've thought we've decided that the tag is OTC-Jira or similar. For:
> >certainly looks a bit too generic, and a prefix helps in namespacing if
> >you want to scan with automated tools for this.
> 
> The wiki page about GMIN tagging (which comes from the OTC) very clearly
> says to use 'For:' in the case of multi-patch series or 'Issue:' for single
> patches and talks about automated tools using these tags to update and/or
> close bugs. It also says the tags should be used for linux kernel
> upstreaming as well.

Well I guess that wasn't a very nice decision to make without the
namespacing ... Whom do I need to send a mail to to make a fuzz about
this? I really don't think For: and Issue: are really great ideas for tags
to internal tracking when we're supposed to add them to the public git
repo.

Maybe just add them all to cc on this thread.
-Daniel

> 
> 
> >
> >>Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> >Anyway looks like a good idea, queued for -next, thanks for the patch.
> >-Daniel
> >>---
> >>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   25 ++++++++++----------
> >>  drivers/gpu/drm/i915/intel_lrc.c           |   10 ++++----
> >>  drivers/gpu/drm/i915/intel_lrc.h           |    2 +-
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c    |   35 ++++++++++++++++------------
> >>  drivers/gpu/drm/i915/intel_ringbuffer.h    |    4 ++--
> >>  5 files changed, 41 insertions(+), 35 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>index b773368..ec9ea45 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>@@ -1138,7 +1138,7 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
> >>  			       struct drm_i915_gem_execbuffer2 *args,
> >>  			       struct list_head *vmas,
> >>  			       struct drm_i915_gem_object *batch_obj,
> >>-			       u64 exec_start, u32 flags)
> >>+			       u64 exec_start, u32 dispatch_flags)
> >>  {
> >>  	struct drm_clip_rect *cliprects = NULL;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>@@ -1266,19 +1266,19 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
> >>  			ret = ring->dispatch_execbuffer(ring,
> >>  							exec_start, exec_len,
> >>-							flags);
> >>+							dispatch_flags);
> >>  			if (ret)
> >>  				goto error;
> >>  		}
> >>  	} else {
> >>  		ret = ring->dispatch_execbuffer(ring,
> >>  						exec_start, exec_len,
> >>-						flags);
> >>+						dispatch_flags);
> >>  		if (ret)
> >>  			return ret;
> >>  	}
> >>-	trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), flags);
> >>+	trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), dispatch_flags);
> >>  	i915_gem_execbuffer_move_to_active(vmas, ring);
> >>  	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
> >>@@ -1353,7 +1353,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >>  	struct i915_address_space *vm;
> >>  	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
> >>  	u64 exec_start = args->batch_start_offset;
> >>-	u32 flags;
> >>+	u32 dispatch_flags;
> >>  	int ret;
> >>  	bool need_relocs;
> >>@@ -1364,15 +1364,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >>  	if (ret)
> >>  		return ret;
> >>-	flags = 0;
> >>+	dispatch_flags = 0;
> >>  	if (args->flags & I915_EXEC_SECURE) {
> >>  		if (!file->is_master || !capable(CAP_SYS_ADMIN))
> >>  		    return -EPERM;
> >>-		flags |= I915_DISPATCH_SECURE;
> >>+		dispatch_flags |= I915_DISPATCH_SECURE;
> >>  	}
> >>  	if (args->flags & I915_EXEC_IS_PINNED)
> >>-		flags |= I915_DISPATCH_PINNED;
> >>+		dispatch_flags |= I915_DISPATCH_PINNED;
> >>  	if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) {
> >>  		DRM_DEBUG("execbuf with unknown ring: %d\n",
> >>@@ -1495,7 +1495,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >>  						      args->batch_start_offset,
> >>  						      args->batch_len,
> >>  						      file->is_master,
> >>-						      &flags);
> >>+						      &dispatch_flags);
> >>  		if (IS_ERR(batch_obj)) {
> >>  			ret = PTR_ERR(batch_obj);
> >>  			goto err;
> >>@@ -1507,7 +1507,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >>  	/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
> >>  	 * batch" bit. Hence we need to pin secure batches into the global gtt.
> >>  	 * hsw should have this fixed, but bdw mucks it up again. */
> >>-	if (flags & I915_DISPATCH_SECURE) {
> >>+	if (dispatch_flags & I915_DISPATCH_SECURE) {
> >>  		/*
> >>  		 * So on first glance it looks freaky that we pin the batch here
> >>  		 * outside of the reservation loop. But:
> >>@@ -1527,7 +1527,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >>  		exec_start += i915_gem_obj_offset(batch_obj, vm);
> >>  	ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
> >>-				      &eb->vmas, batch_obj, exec_start, flags);
> >>+				      &eb->vmas, batch_obj, exec_start,
> >>+				      dispatch_flags);
> >>  	/*
> >>  	 * FIXME: We crucially rely upon the active tracking for the (ppgtt)
> >>@@ -1535,7 +1536,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >>  	 * needs to be adjusted to also track the ggtt batch vma properly as
> >>  	 * active.
> >>  	 */
> >>-	if (flags & I915_DISPATCH_SECURE)
> >>+	if (dispatch_flags & I915_DISPATCH_SECURE)
> >>  		i915_gem_object_ggtt_unpin(batch_obj);
> >>  err:
> >>  	/* the request owns the ref now */
> >>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>index a94346f..0376285 100644
> >>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>@@ -611,7 +611,7 @@ static int execlists_move_to_gpu(struct intel_ringbuffer *ringbuf,
> >>   * @vmas: list of vmas.
> >>   * @batch_obj: the batchbuffer to submit.
> >>   * @exec_start: batchbuffer start virtual address pointer.
> >>- * @flags: translated execbuffer call flags.
> >>+ * @dispatch_flags: translated execbuffer call flags.
> >>   *
> >>   * This is the evil twin version of i915_gem_ringbuffer_submission. It abstracts
> >>   * away the submission details of the execbuffer ioctl call.
> >>@@ -624,7 +624,7 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
> >>  			       struct drm_i915_gem_execbuffer2 *args,
> >>  			       struct list_head *vmas,
> >>  			       struct drm_i915_gem_object *batch_obj,
> >>-			       u64 exec_start, u32 flags)
> >>+			       u64 exec_start, u32 dispatch_flags)
> >>  {
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
> >>@@ -697,7 +697,7 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
> >>  		dev_priv->relative_constants_mode = instp_mode;
> >>  	}
> >>-	ret = ring->emit_bb_start(ringbuf, ctx, exec_start, flags);
> >>+	ret = ring->emit_bb_start(ringbuf, ctx, exec_start, dispatch_flags);
> >>  	if (ret)
> >>  		return ret;
> >>@@ -1142,9 +1142,9 @@ static int gen8_init_render_ring(struct intel_engine_cs *ring)
> >>  static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
> >>  			      struct intel_context *ctx,
> >>-			      u64 offset, unsigned flags)
> >>+			      u64 offset, unsigned dispatch_flags)
> >>  {
> >>-	bool ppgtt = !(flags & I915_DISPATCH_SECURE);
> >>+	bool ppgtt = !(dispatch_flags & I915_DISPATCH_SECURE);
> >>  	int ret;
> >>  	ret = intel_logical_ring_begin(ringbuf, ctx, 4);
> >>diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> >>index 6f2d7da..3093836 100644
> >>--- a/drivers/gpu/drm/i915/intel_lrc.h
> >>+++ b/drivers/gpu/drm/i915/intel_lrc.h
> >>@@ -86,7 +86,7 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
> >>  			       struct drm_i915_gem_execbuffer2 *args,
> >>  			       struct list_head *vmas,
> >>  			       struct drm_i915_gem_object *batch_obj,
> >>-			       u64 exec_start, u32 flags);
> >>+			       u64 exec_start, u32 dispatch_flags);
> >>  u32 intel_execlists_ctx_id(struct drm_i915_gem_object *ctx_obj);
> >>  void intel_lrc_irq_handler(struct intel_engine_cs *ring);
> >>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>index 0bd3976..d611608 100644
> >>--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>@@ -1611,7 +1611,7 @@ gen8_ring_put_irq(struct intel_engine_cs *ring)
> >>  static int
> >>  i965_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  			 u64 offset, u32 length,
> >>-			 unsigned flags)
> >>+			 unsigned dispatch_flags)
> >>  {
> >>  	int ret;
> >>@@ -1622,7 +1622,8 @@ i965_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  	intel_ring_emit(ring,
> >>  			MI_BATCH_BUFFER_START |
> >>  			MI_BATCH_GTT |
> >>-			(flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_I965));
> >>+			(dispatch_flags & I915_DISPATCH_SECURE ?
> >>+			 0 : MI_BATCH_NON_SECURE_I965));
> >>  	intel_ring_emit(ring, offset);
> >>  	intel_ring_advance(ring);
> >>@@ -1635,8 +1636,8 @@ i965_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  #define I830_WA_SIZE max(I830_TLB_ENTRIES*4096, I830_BATCH_LIMIT)
> >>  static int
> >>  i830_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>-				u64 offset, u32 len,
> >>-				unsigned flags)
> >>+			 u64 offset, u32 len,
> >>+			 unsigned dispatch_flags)
> >>  {
> >>  	u32 cs_offset = ring->scratch.gtt_offset;
> >>  	int ret;
> >>@@ -1654,7 +1655,7 @@ i830_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  	intel_ring_emit(ring, MI_NOOP);
> >>  	intel_ring_advance(ring);
> >>-	if ((flags & I915_DISPATCH_PINNED) == 0) {
> >>+	if ((dispatch_flags & I915_DISPATCH_PINNED) == 0) {
> >>  		if (len > I830_BATCH_LIMIT)
> >>  			return -ENOSPC;
> >>@@ -1686,7 +1687,8 @@ i830_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  		return ret;
> >>  	intel_ring_emit(ring, MI_BATCH_BUFFER);
> >>-	intel_ring_emit(ring, offset | (flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE));
> >>+	intel_ring_emit(ring, offset | (dispatch_flags & I915_DISPATCH_SECURE ?
> >>+					0 : MI_BATCH_NON_SECURE));
> >>  	intel_ring_emit(ring, offset + len - 8);
> >>  	intel_ring_emit(ring, MI_NOOP);
> >>  	intel_ring_advance(ring);
> >>@@ -1697,7 +1699,7 @@ i830_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  static int
> >>  i915_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  			 u64 offset, u32 len,
> >>-			 unsigned flags)
> >>+			 unsigned dispatch_flags)
> >>  {
> >>  	int ret;
> >>@@ -1706,7 +1708,8 @@ i915_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  		return ret;
> >>  	intel_ring_emit(ring, MI_BATCH_BUFFER_START | MI_BATCH_GTT);
> >>-	intel_ring_emit(ring, offset | (flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE));
> >>+	intel_ring_emit(ring, offset | (dispatch_flags & I915_DISPATCH_SECURE ?
> >>+					0 : MI_BATCH_NON_SECURE));
> >>  	intel_ring_advance(ring);
> >>  	return 0;
> >>@@ -2265,9 +2268,10 @@ static int gen6_bsd_ring_flush(struct intel_engine_cs *ring,
> >>  static int
> >>  gen8_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  			      u64 offset, u32 len,
> >>-			      unsigned flags)
> >>+			      unsigned dispatch_flags)
> >>  {
> >>-	bool ppgtt = USES_PPGTT(ring->dev) && !(flags & I915_DISPATCH_SECURE);
> >>+	bool ppgtt = USES_PPGTT(ring->dev) &&
> >>+			!(dispatch_flags & I915_DISPATCH_SECURE);
> >>  	int ret;
> >>  	ret = intel_ring_begin(ring, 4);
> >>@@ -2286,8 +2290,8 @@ gen8_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  static int
> >>  hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>-			      u64 offset, u32 len,
> >>-			      unsigned flags)
> >>+			     u64 offset, u32 len,
> >>+			     unsigned dispatch_flags)
> >>  {
> >>  	int ret;
> >>@@ -2297,7 +2301,7 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  	intel_ring_emit(ring,
> >>  			MI_BATCH_BUFFER_START |
> >>-			(flags & I915_DISPATCH_SECURE ?
> >>+			(dispatch_flags & I915_DISPATCH_SECURE ?
> >>  			 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW));
> >>  	/* bit0-7 is the length on GEN6+ */
> >>  	intel_ring_emit(ring, offset);
> >>@@ -2309,7 +2313,7 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  static int
> >>  gen6_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  			      u64 offset, u32 len,
> >>-			      unsigned flags)
> >>+			      unsigned dispatch_flags)
> >>  {
> >>  	int ret;
> >>@@ -2319,7 +2323,8 @@ gen6_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
> >>  	intel_ring_emit(ring,
> >>  			MI_BATCH_BUFFER_START |
> >>-			(flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_I965));
> >>+			(dispatch_flags & I915_DISPATCH_SECURE ?
> >>+			 0 : MI_BATCH_NON_SECURE_I965));
> >>  	/* bit0-7 is the length on GEN6+ */
> >>  	intel_ring_emit(ring, offset);
> >>  	intel_ring_advance(ring);
> >>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>index 714f3fd..26e5774 100644
> >>--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>@@ -164,7 +164,7 @@ struct  intel_engine_cs {
> >>  				     u32 seqno);
> >>  	int		(*dispatch_execbuffer)(struct intel_engine_cs *ring,
> >>  					       u64 offset, u32 length,
> >>-					       unsigned flags);
> >>+					       unsigned dispatch_flags);
> >>  #define I915_DISPATCH_SECURE 0x1
> >>  #define I915_DISPATCH_PINNED 0x2
> >>  	void		(*cleanup)(struct intel_engine_cs *ring);
> >>@@ -242,7 +242,7 @@ struct  intel_engine_cs {
> >>  				      u32 flush_domains);
> >>  	int		(*emit_bb_start)(struct intel_ringbuffer *ringbuf,
> >>  					 struct intel_context *ctx,
> >>-					 u64 offset, unsigned flags);
> >>+					 u64 offset, unsigned dispatch_flags);
> >>  	/**
> >>  	 * List of objects currently involved in rendering from the
> >>-- 
> >>1.7.9.5
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux