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

[Tomas Elf <tomas.elf@intel.com>]

On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> 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
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   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)

You didn't change the parameter name in the prototype declaration in 
i915_drv.h .

Thanks,
Tomas

>   {
>   	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 aafcef3..f30a8e3 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -620,7 +620,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.
> @@ -633,7 +633,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;
> @@ -706,7 +706,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;
>
> @@ -1163,9 +1163,9 @@ static int gen9_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 f635735..b66fc4d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -82,7 +82,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 d17e76d..ca7de3d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1690,7 +1690,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;
>
> @@ -1701,7 +1701,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);
>
> @@ -1714,8 +1715,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;
> @@ -1733,7 +1734,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;
>
> @@ -1765,7 +1766,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);
> @@ -1776,7 +1778,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;
>
> @@ -1785,7 +1787,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;
> @@ -2352,9 +2355,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);
> @@ -2373,8 +2377,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;
>
> @@ -2384,7 +2388,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);
> @@ -2396,7 +2400,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;
>
> @@ -2406,7 +2410,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 b6c484f..ffa3724 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
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx