On Thu, Mar 05, 2015 at 03:01:21PM +0000, Tomas Elf wrote:
> On 19/02/2015 17:17, John.C.Harrison@xxxxxxxxx wrote:
> >From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> >In order to explicitly manage requests from creation to submission, it is
> >necessary to be able to explicitly create them in the first place. This patch
> >adds an indirection wrapper to the request creation function so that it can be
> >called from generic code without having to worry about execlist vs legacy mode.
> >
> >For: VIZ-5115
> >Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> >---
> > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> > drivers/gpu/drm/i915/i915_gem.c | 2 ++
> > drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
> > drivers/gpu/drm/i915/intel_lrc.h | 2 ++
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 6 +++---
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
> > 6 files changed, 14 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index b350910..87a4a2e 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -1908,6 +1908,8 @@ struct drm_i915_private {
> >
> > /* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
> > struct {
> >+ int (*alloc_request)(struct intel_engine_cs *ring,
> >+ struct intel_context *ctx);
> > int (*do_execbuf)(struct i915_execbuffer_params *params,
> > struct drm_i915_gem_execbuffer2 *args,
> > struct list_head *vmas);
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 7a0dc7c..cf959e3 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -4860,11 +4860,13 @@ int i915_gem_init(struct drm_device *dev)
> > }
> >
> > if (!i915.enable_execlists) {
> >+ dev_priv->gt.alloc_request = intel_ring_alloc_request;
> > 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;
> > } else {
> >+ dev_priv->gt.alloc_request = intel_logical_ring_alloc_request;
> > dev_priv->gt.do_execbuf = intel_execlists_submission;
> > dev_priv->gt.init_rings = intel_logical_rings_init;
> > dev_priv->gt.cleanup_ring = intel_logical_ring_cleanup;
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index dc474b4..8628abf 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -856,8 +856,8 @@ void intel_lr_context_unpin(struct intel_engine_cs *ring,
> > }
> > }
> >
> >-static int logical_ring_alloc_request(struct intel_engine_cs *ring,
> >- struct intel_context *ctx)
> >+int intel_logical_ring_alloc_request(struct intel_engine_cs *ring,
> >+ struct intel_context *ctx)
> > {
> > struct drm_i915_gem_request *request;
> > struct drm_i915_private *dev_private = ring->dev->dev_private;
> >@@ -1066,7 +1066,7 @@ int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf,
> > return ret;
> >
> > /* Preallocate the olr before touching the ring */
> >- ret = logical_ring_alloc_request(ring, ctx);
> >+ ret = intel_logical_ring_alloc_request(ring, ctx);
> > if (ret)
> > return ret;
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> >index 3a6abce..3cc38bd 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.h
> >+++ b/drivers/gpu/drm/i915/intel_lrc.h
> >@@ -36,6 +36,8 @@
> > #define RING_CONTEXT_STATUS_PTR(ring) ((ring)->mmio_base+0x3a0)
> >
> > /* Logical Rings */
> >+int __must_check intel_logical_ring_alloc_request(struct intel_engine_cs *ring,
> >+ struct intel_context *ctx);
> > void intel_logical_ring_stop(struct intel_engine_cs *ring);
> > void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
> > int intel_logical_rings_init(struct drm_device *dev);
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >index 7fd89e5..635707a 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >@@ -2163,8 +2163,8 @@ int intel_ring_idle(struct intel_engine_cs *ring)
> > return i915_wait_request(req);
> > }
> >
> >-static int
> >-intel_ring_alloc_request(struct intel_engine_cs *ring)
> >+int
> >+intel_ring_alloc_request(struct intel_engine_cs *ring, struct intel_context *ctx)
> > {
> > int ret;
> > struct drm_i915_gem_request *request;
> >@@ -2229,7 +2229,7 @@ int intel_ring_begin(struct intel_engine_cs *ring,
> > return ret;
> >
> > /* Preallocate the olr before touching the ring */
> >- ret = intel_ring_alloc_request(ring);
> >+ ret = intel_ring_alloc_request(ring, NULL);
> > if (ret)
> > return ret;
> >
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >index ffa3724..2fd960a 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >@@ -392,6 +392,8 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring);
> >
> > int __must_check intel_ring_begin(struct intel_engine_cs *ring, int n);
> > int __must_check intel_ring_cacheline_align(struct intel_engine_cs *ring);
> >+int __must_check intel_ring_alloc_request(struct intel_engine_cs *ring,
> >+ struct intel_context *ctx);
> > static inline void intel_ring_emit(struct intel_engine_cs *ring,
> > u32 data)
> > {
> >
>
> I find the whole idea of having virtual functions pointing to public
> functions kind of strange since it allows for two ways of accessing the
> function. If you look at the way we set up the virtual ring buffer functions
> those virtual functions are static and the set up is done in
> intel_ringbuffer.c without exposing the functions to the outside world.
> Having the set up take place in i915_gem.c forces the virtual functions to
> be public, which is not very nice. It would've been a better idea to pass
> the virtual function struct for initialization inside intel_lrc.c and
> intel_ringbuffer.c, which would have avoided making those functions public.
>
> I'd like to hear some input from someone else on this on this, like e.g.
> Daniel Vetter (since he was a strong proponent of the legacy/execlist split,
> which was the underlying reason for this construct in the first place - not
> saying that it couldn't have been done in some other way, though).
Yeah generally the pattern is to have one public _init function per file
which sets up the vfunc tables as needed. Follow-up patches would be
really nice though indeed.
-Daniel
>
> Obviously, this is not the fault of the patch at hand and you're just
> following the already established pattern of setting up these virtual
> functions so I can't fault you for that.
>
> Therefore I'm ok with this change (but not the construct as a whole):
>
> Reviewed-by: Tomas Elf <tomas.elf@xxxxxxxxx>
>
> Thanks,
> Tomas
>
> _______________________________________________
> 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
