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

[John Harrison <John.C.Harrison@xxxxxxxxx>]

On 05/03/2015 13:21, Tomas Elf wrote:
> On 19/02/2015 17:17, 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
> > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > ---
> >   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 .
Bit late now, this patch has already been merged. Also, it isn't really 
worth following up with an extra fix patch as those parameters get 
removed in a later patch in this series anyway (albeit one that is being 
discussed as contentious).

> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx