| Message ID | 1424366285-29232-7-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > 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@Intel.com> > --- > 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). 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@intel.com> Thanks, Tomas
On Thu, Mar 05, 2015 at 03:01:21PM +0000, Tomas Elf wrote: > On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote: > >From: John Harrison <John.C.Harrison@Intel.com> > > > >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@Intel.com> > >--- > > 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@intel.com> > > Thanks, > Tomas > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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) {