On Mon, Aug 11, 2014 at 04:36:53PM +0200, Daniel Vetter wrote:
> On Thu, Jul 24, 2014 at 05:04:21PM +0100, Thomas Daniel wrote:
> > From: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> >
> > As suggested by Daniel Vetter. The idea, in subsequent patches, is to
> > provide an alternative to these vfuncs for the Execlists submission
> > mechanism.
> >
> > v2: Splitted into two and reordered to illustrate our intentions, instead
> > of showing it off. Also, remove the add_request vfunc and added the
> > stop_ring one.
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 24 ++++++++++++++++++++++++
> > drivers/gpu/drm/i915/i915_gem.c | 15 +++++++++++----
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++++++++++----------
> > 3 files changed, 45 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ff2c373..1caed52 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1617,6 +1617,21 @@ struct drm_i915_private {
> > /* Old ums support infrastructure, same warning applies. */
> > struct i915_ums_state ums;
> >
> > + /* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
> > + struct {
> > + int (*do_execbuf) (struct drm_device *dev, struct drm_file *file,
> > + struct intel_engine_cs *ring,
> > + struct intel_context *ctx,
> > + struct drm_i915_gem_execbuffer2 *args,
> > + struct list_head *vmas,
> > + struct drm_i915_gem_object *batch_obj,
> > + u64 exec_start, u32 flags);
> > + int (*init_rings) (struct drm_device *dev);
> > + void (*cleanup_ring) (struct intel_engine_cs *ring);
> > + void (*stop_ring) (struct intel_engine_cs *ring);
>
> My rule of thumb is that with just one caller it's probably better to not
> have a vtable - just makes it harder to follow the code flow.
>
> Usually (with non-borked code design at least) init/teardown functions
> have only one caller, so there's a good chance I'll ask you to remove them
> again.
Also checkpatch (and my eyes!) where unhappy about the space you've put
between ) and (. Fixed that too.
-Daniel
>
> > + bool (*is_ring_initialized) (struct intel_engine_cs *ring);
>
> This one is unused and I didn't really like the taste of it. So I killed
> it.
> -Daniel
>
> > + } gt;
> > +
> > /*
> > * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
> > * will be rejected. Instead look for a better place.
> > @@ -2224,6 +2239,14 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> > struct drm_file *file_priv);
> > int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
> > struct drm_file *file_priv);
> > +int i915_gem_ringbuffer_submission(struct drm_device *dev,
> > + struct drm_file *file,
> > + struct intel_engine_cs *ring,
> > + struct intel_context *ctx,
> > + struct drm_i915_gem_execbuffer2 *args,
> > + struct list_head *vmas,
> > + struct drm_i915_gem_object *batch_obj,
> > + u64 exec_start, u32 flags);
> > int i915_gem_execbuffer(struct drm_device *dev, void *data,
> > struct drm_file *file_priv);
> > int i915_gem_execbuffer2(struct drm_device *dev, void *data,
> > @@ -2376,6 +2399,7 @@ void i915_gem_reset(struct drm_device *dev);
> > bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
> > int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
> > int __must_check i915_gem_init(struct drm_device *dev);
> > +int i915_gem_init_rings(struct drm_device *dev);
> > int __must_check i915_gem_init_hw(struct drm_device *dev);
> > int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
> > void i915_gem_init_swizzling(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d8bf4fa..6544286 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4518,7 +4518,7 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
> > int i;
> >
> > for_each_ring(ring, dev_priv, i)
> > - intel_stop_ring_buffer(ring);
> > + dev_priv->gt.stop_ring(ring);
> > }
> >
> > int
> > @@ -4635,7 +4635,7 @@ intel_enable_blt(struct drm_device *dev)
> > return true;
> > }
> >
> > -static int i915_gem_init_rings(struct drm_device *dev)
> > +int i915_gem_init_rings(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > int ret;
> > @@ -4718,7 +4718,7 @@ i915_gem_init_hw(struct drm_device *dev)
> >
> > i915_gem_init_swizzling(dev);
> >
> > - ret = i915_gem_init_rings(dev);
> > + ret = dev_priv->gt.init_rings(dev);
> > if (ret)
> > return ret;
> >
> > @@ -4759,6 +4759,13 @@ int i915_gem_init(struct drm_device *dev)
> > DRM_DEBUG_DRIVER("allow wake ack timed out\n");
> > }
> >
> > + if (!i915.enable_execlists) {
> > + dev_priv->gt.do_execbuf = i915_gem_ringbuffer_submission;
> > + dev_priv->gt.init_rings = i915_gem_init_rings;
> > + dev_priv->gt.cleanup_ring = intel_cleanup_ring_buffer;
> > + dev_priv->gt.stop_ring = intel_stop_ring_buffer;
> > + }
> > +
> > i915_gem_init_userptr(dev);
> > i915_gem_init_global_gtt(dev);
> >
> > @@ -4794,7 +4801,7 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev)
> > int i;
> >
> > for_each_ring(ring, dev_priv, i)
> > - intel_cleanup_ring_buffer(ring);
> > + dev_priv->gt.cleanup_ring(ring);
> > }
> >
> > int
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 4e9b387..8c63d79 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1034,14 +1034,14 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
> > return 0;
> > }
> >
> > -static int
> > -legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
> > - struct intel_engine_cs *ring,
> > - struct intel_context *ctx,
> > - struct drm_i915_gem_execbuffer2 *args,
> > - struct list_head *vmas,
> > - struct drm_i915_gem_object *batch_obj,
> > - u64 exec_start, u32 flags)
> > +int
> > +i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
> > + struct intel_engine_cs *ring,
> > + struct intel_context *ctx,
> > + struct drm_i915_gem_execbuffer2 *args,
> > + struct list_head *vmas,
> > + struct drm_i915_gem_object *batch_obj,
> > + u64 exec_start, u32 flags)
> > {
> > struct drm_clip_rect *cliprects = NULL;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -1408,8 +1408,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > else
> > exec_start += i915_gem_obj_offset(batch_obj, vm);
> >
> > - ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
> > - args, &eb->vmas, batch_obj, exec_start, flags);
> > + ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args,
> > + &eb->vmas, batch_obj, exec_start, flags);
> > if (ret)
> > goto err;
> >
> > --
> > 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
--
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
