| Message ID | 1493400369-33879-1-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 28, 2017 at 05:26:09PM +0000, Oscar Mateo wrote: > This will be more useful later to support platforms that need to emit > HW commands at the beginning of every request (more general than emitting > things at the beginning of every batchbuffer, which is already covered by > emit_bb_start). We already have one... You are presenting this without a good reason and failing to transform similar code, which indicates to me that this vfunc isn't that general. > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++++++----- > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 9488578..a5c055a 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -687,6 +687,15 @@ static bool insert_request(struct i915_priotree *pt, struct rb_root *root) > return first; > } > > +static int execlists_prepare_request(struct drm_i915_gem_request *request) > +{ > + u32 *cs = intel_ring_begin(request, 0); > + if (IS_ERR(cs)) > + return PTR_ERR(cs); > + > + return 0; > +} > + > static void execlists_submit_request(struct drm_i915_gem_request *request) > { > struct intel_engine_cs *engine = request->engine; > @@ -879,7 +888,6 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request) > { > struct intel_engine_cs *engine = request->engine; > struct intel_context *ce = &request->ctx->engine[engine->id]; > - u32 *cs; > int ret; > > GEM_BUG_ON(!ce->pin_count); > @@ -904,11 +912,9 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request) > goto err; > } > > - cs = intel_ring_begin(request, 0); > - if (IS_ERR(cs)) { > - ret = PTR_ERR(cs); > + ret = engine->prepare_request(request); > + if (ret) > goto err_unreserve; > - } > > if (!ce->initialised) { > ret = engine->init_context(request); > @@ -1650,6 +1656,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) > > static void execlists_set_default_submission(struct intel_engine_cs *engine) > { > + engine->prepare_request = execlists_prepare_request; > engine->submit_request = execlists_submit_request; > engine->schedule = execlists_schedule; > engine->irq_tasklet.func = intel_lrc_irq_handler; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index d901831..67de978 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -303,6 +303,7 @@ struct intel_engine_cs { > void (*emit_breadcrumb)(struct drm_i915_gem_request *req, > u32 *cs); > int emit_breadcrumb_sz; > + int (*prepare_request)(struct drm_i915_gem_request *req); Why in the emit group? -Chris
Hi Oscar,
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20170428]
[cannot apply to v4.11-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oscar-Mateo/drm-i915-New-vfunc-prepare_request/20170430-041411
base: git://anongit.freedesktop.org/drm-intel for-linux-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/i915/intel_lrc.c:632:1-3: WARNING: PTR_ERR_OR_ZERO can be used
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 04/29/2017 08:31 AM, Chris Wilson wrote: > On Fri, Apr 28, 2017 at 05:26:09PM +0000, Oscar Mateo wrote: >> This will be more useful later to support platforms that need to emit >> HW commands at the beginning of every request (more general than emitting >> things at the beginning of every batchbuffer, which is already covered by >> emit_bb_start). > We already have one... You are presenting this without a good reason and > failing to transform similar code, which indicates to me that this vfunc > isn't that general. It looks like I've missed that. What function are you talking about? >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++++++----- >> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + >> 2 files changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index 9488578..a5c055a 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -687,6 +687,15 @@ static bool insert_request(struct i915_priotree *pt, struct rb_root *root) >> return first; >> } >> >> +static int execlists_prepare_request(struct drm_i915_gem_request *request) >> +{ >> + u32 *cs = intel_ring_begin(request, 0); >> + if (IS_ERR(cs)) >> + return PTR_ERR(cs); >> + >> + return 0; >> +} >> + >> static void execlists_submit_request(struct drm_i915_gem_request *request) >> { >> struct intel_engine_cs *engine = request->engine; >> @@ -879,7 +888,6 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request) >> { >> struct intel_engine_cs *engine = request->engine; >> struct intel_context *ce = &request->ctx->engine[engine->id]; >> - u32 *cs; >> int ret; >> >> GEM_BUG_ON(!ce->pin_count); >> @@ -904,11 +912,9 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request) >> goto err; >> } >> >> - cs = intel_ring_begin(request, 0); >> - if (IS_ERR(cs)) { >> - ret = PTR_ERR(cs); >> + ret = engine->prepare_request(request); >> + if (ret) >> goto err_unreserve; >> - } >> >> if (!ce->initialised) { >> ret = engine->init_context(request); >> @@ -1650,6 +1656,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) >> >> static void execlists_set_default_submission(struct intel_engine_cs *engine) >> { >> + engine->prepare_request = execlists_prepare_request; >> engine->submit_request = execlists_submit_request; >> engine->schedule = execlists_schedule; >> engine->irq_tasklet.func = intel_lrc_irq_handler; >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h >> index d901831..67de978 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >> @@ -303,6 +303,7 @@ struct intel_engine_cs { >> void (*emit_breadcrumb)(struct drm_i915_gem_request *req, >> u32 *cs); >> int emit_breadcrumb_sz; >> + int (*prepare_request)(struct drm_i915_gem_request *req); > Why in the emit group? > -Chris > Missing newline. -- Oscar
On Mon, May 01, 2017 at 07:28:12AM +0000, Oscar Mateo wrote: > > > On 04/29/2017 08:31 AM, Chris Wilson wrote: > >On Fri, Apr 28, 2017 at 05:26:09PM +0000, Oscar Mateo wrote: > >>This will be more useful later to support platforms that need to emit > >>HW commands at the beginning of every request (more general than emitting > >>things at the beginning of every batchbuffer, which is already covered by > >>emit_bb_start). > >We already have one... You are presenting this without a good reason and > >failing to transform similar code, which indicates to me that this vfunc > >isn't that general. > > It looks like I've missed that. What function are you talking about? For example, you can argue that both legacy ring submission and execlists share the same sequence of request_alloc: request->ring = ce->ring (this is now common so can be moved out, thanks to unifying context pin) [guc_wq_reserve we have ideas how to eliminate] some rudimentary reserving of ring space context initialisation <- time to unify the legacy code The choice for adding a new callback here, is we either take more common code out of the request_alloc callback such that we reduce it to you new vfunc -- and so we may end up with 2 common vfuncs called by the core. Or you refactor the current code so that you can specialise request alloc and use some execlist helpers for the common portion. -Chris
On 05/02/2017 08:59 AM, Chris Wilson wrote: > On Mon, May 01, 2017 at 07:28:12AM +0000, Oscar Mateo wrote: >> >> On 04/29/2017 08:31 AM, Chris Wilson wrote: >>> On Fri, Apr 28, 2017 at 05:26:09PM +0000, Oscar Mateo wrote: >>>> This will be more useful later to support platforms that need to emit >>>> HW commands at the beginning of every request (more general than emitting >>>> things at the beginning of every batchbuffer, which is already covered by >>>> emit_bb_start). >>> We already have one... You are presenting this without a good reason and >>> failing to transform similar code, which indicates to me that this vfunc >>> isn't that general. >> It looks like I've missed that. What function are you talking about? > For example, you can argue that both legacy ring submission and > execlists share the same sequence of request_alloc: > > request->ring = ce->ring (this is now common so can be moved > out, thanks to unifying context pin) OK > [guc_wq_reserve we have ideas how to eliminate] Can I simply create a guc_request_alloc for now? This will actually work out nicely for me, because I only need to emit extra stuff in the GuC case anyway. > some rudimentary reserving of ring space > > context initialisation <- time to unify the legacy code OK (or, at least, I'll try) > The choice for adding a new callback here, is we either take more common > code out of the request_alloc callback such that we reduce it to you new > vfunc -- and so we may end up with 2 common vfuncs called by the core. > Or you refactor the current code so that you can specialise request > alloc and use some execlist helpers for the common portion. > -Chris > Sounds reasonable, thanks!
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 9488578..a5c055a 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -687,6 +687,15 @@ static bool insert_request(struct i915_priotree *pt, struct rb_root *root) return first; } +static int execlists_prepare_request(struct drm_i915_gem_request *request) +{ + u32 *cs = intel_ring_begin(request, 0); + if (IS_ERR(cs)) + return PTR_ERR(cs); + + return 0; +} + static void execlists_submit_request(struct drm_i915_gem_request *request) { struct intel_engine_cs *engine = request->engine; @@ -879,7 +888,6 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request) { struct intel_engine_cs *engine = request->engine; struct intel_context *ce = &request->ctx->engine[engine->id]; - u32 *cs; int ret; GEM_BUG_ON(!ce->pin_count); @@ -904,11 +912,9 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request) goto err; } - cs = intel_ring_begin(request, 0); - if (IS_ERR(cs)) { - ret = PTR_ERR(cs); + ret = engine->prepare_request(request); + if (ret) goto err_unreserve; - } if (!ce->initialised) { ret = engine->init_context(request); @@ -1650,6 +1656,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) static void execlists_set_default_submission(struct intel_engine_cs *engine) { + engine->prepare_request = execlists_prepare_request; engine->submit_request = execlists_submit_request; engine->schedule = execlists_schedule; engine->irq_tasklet.func = intel_lrc_irq_handler; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index d901831..67de978 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -303,6 +303,7 @@ struct intel_engine_cs { void (*emit_breadcrumb)(struct drm_i915_gem_request *req, u32 *cs); int emit_breadcrumb_sz; + int (*prepare_request)(struct drm_i915_gem_request *req); /* Pass the request to the hardware queue (e.g. directly into * the legacy ringbuffer or to the end of an execlist).
This will be more useful later to support platforms that need to emit HW commands at the beginning of every request (more general than emitting things at the beginning of every batchbuffer, which is already covered by emit_bb_start). Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++++++----- drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + 2 files changed, 13 insertions(+), 5 deletions(-)