Re: [PATCH] drm/i915: New vfunc prepare_request

Re: [PATCH] drm/i915: New vfunc prepare_request

[kbuild test robot]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: New vfunc prepare_request

[Oscar Mateo]

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(-)

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).
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: New vfunc prepare_request

[Patchwork]

== Series Details ==

Series: drm/i915: New vfunc prepare_request
URL   : https://patchwork.freedesktop.org/series/23727/
State : success

== Summary ==

Series 23727v1 drm/i915: New vfunc prepare_request
https://patchwork.freedesktop.org/api/1.0/series/23727/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:432s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:425s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:572s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:553s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:485s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:483s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:411s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:401s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:409s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:489s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:469s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:462s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:573s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:447s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:567s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:461s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:493s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:535s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:401s

1d490e4b6d5324cfbf8dc800cf4a99471252802c drm-tip: 2017y-04m-28d-14h-14m-47s UTC integration manifest
338839e drm/i915: New vfunc prepare_request

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4585/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: New vfunc prepare_request

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: New vfunc prepare_request

[Oscar Mateo]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: New vfunc prepare_request

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: New vfunc prepare_request

[Oscar Mateo]

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!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: fix ptr_ret.cocci warnings

[kbuild test robot]

drivers/gpu/drm/i915/intel_lrc.c:632:1-3: WARNING: PTR_ERR_OR_ZERO can be used


 Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR

Generated by: scripts/coccinelle/api/ptr_ret.cocci

CC: Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 intel_lrc.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -629,10 +629,7 @@ static bool insert_request(struct i915_p
 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;
+	return PTR_ERR_OR_ZERO(cs);
 }
 
 static void execlists_submit_request(struct drm_i915_gem_request *request)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx