diff mbox series

[v8,6/9] drm/i915/gt: Refactor intel_emit_pipe_control_cs() in a single function

Message ID 20230721161514.818895-7-andi.shyti@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Update AUX invalidation sequence | expand

Commit Message

Andi Shyti July 21, 2023, 4:15 p.m. UTC 
Just a trivial refactoring for reducing the number of code
duplicate. This will come at handy in the next commits.

Meantime, propagate the error to the above layers if we fail to
emit the pipe control.

Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
Cc: <stable@vger.kernel.org> # v5.8+
---
 drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 47 +++++++++++++-----------
 1 file changed, 26 insertions(+), 21 deletions(-)

Comments

Andrzej Hajda July 24, 2023, 7:54 a.m. UTC | #1 
On 21.07.2023 18:15, Andi Shyti wrote:
> Just a trivial refactoring for reducing the number of code
> duplicate. This will come at handy in the next commits.
>
> Meantime, propagate the error to the above layers if we fail to
> emit the pipe control.
>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v5.8+

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 47 +++++++++++++-----------
>   1 file changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> index 139a7e69f5c4d..5e19b45a5cabe 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -7,6 +7,7 @@
>   #include "i915_drv.h"
>   #include "intel_engine_regs.h"
>   #include "intel_gpu_commands.h"
> +#include "intel_gt_print.h"
>   #include "intel_lrc.h"
>   #include "intel_ring.h"
>   
> @@ -189,23 +190,30 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv
>   	return cs;
>   }
>   
> +static int gen12_emit_pipe_control_cs(struct i915_request *rq, u32 bit_group_0,
> +				      u32 bit_group_1, u32 offset)
> +{
> +	u32 *cs;
> +
> +	cs = intel_ring_begin(rq, 6);
> +	if (IS_ERR(cs))
> +		return PTR_ERR(cs);
> +
> +	cs = gen12_emit_pipe_control(cs, bit_group_0, bit_group_1,
> +				     LRC_PPHWSP_SCRATCH_ADDR);
> +	intel_ring_advance(rq, cs);
> +
> +	return 0;
> +}
> +
>   static int mtl_dummy_pipe_control(struct i915_request *rq)
>   {
>   	/* Wa_14016712196 */
>   	if (IS_MTL_GRAPHICS_STEP(rq->engine->i915, M, STEP_A0, STEP_B0) ||
> -	    IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0)) {
> -		u32 *cs;
> -
> -		/* dummy PIPE_CONTROL + depth flush */
> -		cs = intel_ring_begin(rq, 6);
> -		if (IS_ERR(cs))
> -			return PTR_ERR(cs);
> -		cs = gen12_emit_pipe_control(cs,
> -					     0,
> -					     PIPE_CONTROL_DEPTH_CACHE_FLUSH,
> -					     LRC_PPHWSP_SCRATCH_ADDR);
> -		intel_ring_advance(rq, cs);
> -	}
> +	    IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0))
> +		return gen12_emit_pipe_control_cs(rq, 0,
> +					PIPE_CONTROL_DEPTH_CACHE_FLUSH,
> +					LRC_PPHWSP_SCRATCH_ADDR);
>   
>   	return 0;
>   }
> @@ -222,7 +230,6 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
>   		u32 bit_group_0 = 0;
>   		u32 bit_group_1 = 0;
>   		int err;
> -		u32 *cs;
>   
>   		err = mtl_dummy_pipe_control(rq);
>   		if (err)
> @@ -256,13 +263,11 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
>   		else if (engine->class == COMPUTE_CLASS)
>   			bit_group_1 &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
>   
> -		cs = intel_ring_begin(rq, 6);
> -		if (IS_ERR(cs))
> -			return PTR_ERR(cs);
> -
> -		cs = gen12_emit_pipe_control(cs, bit_group_0, bit_group_1,
> -					     LRC_PPHWSP_SCRATCH_ADDR);
> -		intel_ring_advance(rq, cs);
> +		err = gen12_emit_pipe_control_cs(rq, bit_group_0, bit_group_1,
> +						 LRC_PPHWSP_SCRATCH_ADDR);
> +		if (err)
> +			gt_warn(engine->gt,
> +				"Failed to emit flush pipe control\n");
>   	}
>   
>   	if (mode & EMIT_INVALIDATE) {
Nirmoy Das July 24, 2023, 9:07 a.m. UTC | #2 
Hi  Andi,

On 7/21/2023 6:15 PM, Andi Shyti wrote:
> Just a trivial refactoring for reducing the number of code
> duplicate. This will come at handy in the next commits.
>
> Meantime, propagate the error to the above layers if we fail to
> emit the pipe control.
>
> Signed-off-by: Andi Shyti<andi.shyti@linux.intel.com>
> Cc:<stable@vger.kernel.org>  # v5.8+
> ---
>   drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 47 +++++++++++++-----------
>   1 file changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> index 139a7e69f5c4d..5e19b45a5cabe 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -7,6 +7,7 @@
>   #include "i915_drv.h"
>   #include "intel_engine_regs.h"
>   #include "intel_gpu_commands.h"
> +#include "intel_gt_print.h"
>   #include "intel_lrc.h"
>   #include "intel_ring.h"
>   
> @@ -189,23 +190,30 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv
>   	return cs;
>   }
>   
> +static int gen12_emit_pipe_control_cs(struct i915_request *rq, u32 bit_group_0,
> +				      u32 bit_group_1, u32 offset)
> +{
> +	u32 *cs;
> +
> +	cs = intel_ring_begin(rq, 6);
> +	if (IS_ERR(cs))
> +		return PTR_ERR(cs);
> +
> +	cs = gen12_emit_pipe_control(cs, bit_group_0, bit_group_1,
> +				     LRC_PPHWSP_SCRATCH_ADDR);
> +	intel_ring_advance(rq, cs);
> +
> +	return 0;
> +}
> +
>   static int mtl_dummy_pipe_control(struct i915_request *rq)
>   {
>   	/* Wa_14016712196 */
>   	if (IS_MTL_GRAPHICS_STEP(rq->engine->i915, M, STEP_A0, STEP_B0) ||
> -	    IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0)) {
> -		u32 *cs;
> -
> -		/* dummy PIPE_CONTROL + depth flush */
> -		cs = intel_ring_begin(rq, 6);
> -		if (IS_ERR(cs))
> -			return PTR_ERR(cs);
> -		cs = gen12_emit_pipe_control(cs,
> -					     0,
> -					     PIPE_CONTROL_DEPTH_CACHE_FLUSH,
> -					     LRC_PPHWSP_SCRATCH_ADDR);
> -		intel_ring_advance(rq, cs);
> -	}
> +	    IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0))
> +		return gen12_emit_pipe_control_cs(rq, 0,
> +					PIPE_CONTROL_DEPTH_CACHE_FLUSH,
> +					LRC_PPHWSP_SCRATCH_ADDR);

Don't think this will get backported to 5.8+. I think mtl introduced 
after that.

If that is not an issue for rest of the series and then

|Reviewed-by: Nirmoy Das <nirmoy.das@intel.com> Regards, Nirmoy |


>   
>   	return 0;
>   }
> @@ -222,7 +230,6 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
>   		u32 bit_group_0 = 0;
>   		u32 bit_group_1 = 0;
>   		int err;
> -		u32 *cs;
>   
>   		err = mtl_dummy_pipe_control(rq);
>   		if (err)
> @@ -256,13 +263,11 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
>   		else if (engine->class == COMPUTE_CLASS)
>   			bit_group_1 &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
>   
> -		cs = intel_ring_begin(rq, 6);
> -		if (IS_ERR(cs))
> -			return PTR_ERR(cs);
> -
> -		cs = gen12_emit_pipe_control(cs, bit_group_0, bit_group_1,
> -					     LRC_PPHWSP_SCRATCH_ADDR);
> -		intel_ring_advance(rq, cs);
> +		err = gen12_emit_pipe_control_cs(rq, bit_group_0, bit_group_1,
> +						 LRC_PPHWSP_SCRATCH_ADDR);
> +		if (err)
> +			gt_warn(engine->gt,
> +				"Failed to emit flush pipe control\n");
>   	}
>   
>   	if (mode & EMIT_INVALIDATE) {
Andi Shyti July 24, 2023, 9:16 a.m. UTC | #3 
Hi Nirmoy,

>      static int mtl_dummy_pipe_control(struct i915_request *rq)
>      {
>             /* Wa_14016712196 */
>             if (IS_MTL_GRAPHICS_STEP(rq->engine->i915, M, STEP_A0, STEP_B0) ||
>     -           IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0)) {
>     -               u32 *cs;
>     -
>     -               /* dummy PIPE_CONTROL + depth flush */
>     -               cs = intel_ring_begin(rq, 6);
>     -               if (IS_ERR(cs))
>     -                       return PTR_ERR(cs);
>     -               cs = gen12_emit_pipe_control(cs,
>     -                                            0,
>     -                                            PIPE_CONTROL_DEPTH_CACHE_FLUSH,
>     -                                            LRC_PPHWSP_SCRATCH_ADDR);
>     -               intel_ring_advance(rq, cs);
>     -       }
>     +           IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0))
>     +               return gen12_emit_pipe_control_cs(rq, 0,
>     +                                       PIPE_CONTROL_DEPTH_CACHE_FLUSH,
>     +                                       LRC_PPHWSP_SCRATCH_ADDR);
> 
> Don't think this will get backported to 5.8+. I think mtl introduced after
> that.
> 
> If that is not an issue for rest of the series and then

to be honest I don't think I fully understood the stable
policies, as in this series only two are the patches that are
really fixing things and the rest are only support.

In this case I think this will produce a conflict that will be
eventually fixed (... I guess!).

> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

Thanks,
Andi
Nirmoy Das July 24, 2023, 9:37 a.m. UTC | #4 
Hi Andi

On 7/24/2023 11:16 AM, Andi Shyti wrote:
> Hi Nirmoy,
>
>>       static int mtl_dummy_pipe_control(struct i915_request *rq)
>>       {
>>              /* Wa_14016712196 */
>>              if (IS_MTL_GRAPHICS_STEP(rq->engine->i915, M, STEP_A0, STEP_B0) ||
>>      -           IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0)) {
>>      -               u32 *cs;
>>      -
>>      -               /* dummy PIPE_CONTROL + depth flush */
>>      -               cs = intel_ring_begin(rq, 6);
>>      -               if (IS_ERR(cs))
>>      -                       return PTR_ERR(cs);
>>      -               cs = gen12_emit_pipe_control(cs,
>>      -                                            0,
>>      -                                            PIPE_CONTROL_DEPTH_CACHE_FLUSH,
>>      -                                            LRC_PPHWSP_SCRATCH_ADDR);
>>      -               intel_ring_advance(rq, cs);
>>      -       }
>>      +           IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0))
>>      +               return gen12_emit_pipe_control_cs(rq, 0,
>>      +                                       PIPE_CONTROL_DEPTH_CACHE_FLUSH,
>>      +                                       LRC_PPHWSP_SCRATCH_ADDR);
>>
>> Don't think this will get backported to 5.8+. I think mtl introduced after
>> that.
>>
>> If that is not an issue for rest of the series and then
> to be honest I don't think I fully understood the stable
> policies, as in this series only two are the patches that are
> really fixing things and the rest are only support.
>
> In this case I think this will produce a conflict that will be
> eventually fixed (... I guess!).


As far as I know, it is developer responsibility to port the patch to 
stable version if there is conflict.

Regards,

Nirmoy

>
>> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
> Thanks,
> Andi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
index 139a7e69f5c4d..5e19b45a5cabe 100644
--- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
@@ -7,6 +7,7 @@ 
 #include "i915_drv.h"
 #include "intel_engine_regs.h"
 #include "intel_gpu_commands.h"
+#include "intel_gt_print.h"
 #include "intel_lrc.h"
 #include "intel_ring.h"
 
@@ -189,23 +190,30 @@  u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv
 	return cs;
 }
 
+static int gen12_emit_pipe_control_cs(struct i915_request *rq, u32 bit_group_0,
+				      u32 bit_group_1, u32 offset)
+{
+	u32 *cs;
+
+	cs = intel_ring_begin(rq, 6);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	cs = gen12_emit_pipe_control(cs, bit_group_0, bit_group_1,
+				     LRC_PPHWSP_SCRATCH_ADDR);
+	intel_ring_advance(rq, cs);
+
+	return 0;
+}
+
 static int mtl_dummy_pipe_control(struct i915_request *rq)
 {
 	/* Wa_14016712196 */
 	if (IS_MTL_GRAPHICS_STEP(rq->engine->i915, M, STEP_A0, STEP_B0) ||
-	    IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0)) {
-		u32 *cs;
-
-		/* dummy PIPE_CONTROL + depth flush */
-		cs = intel_ring_begin(rq, 6);
-		if (IS_ERR(cs))
-			return PTR_ERR(cs);
-		cs = gen12_emit_pipe_control(cs,
-					     0,
-					     PIPE_CONTROL_DEPTH_CACHE_FLUSH,
-					     LRC_PPHWSP_SCRATCH_ADDR);
-		intel_ring_advance(rq, cs);
-	}
+	    IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0))
+		return gen12_emit_pipe_control_cs(rq, 0,
+					PIPE_CONTROL_DEPTH_CACHE_FLUSH,
+					LRC_PPHWSP_SCRATCH_ADDR);
 
 	return 0;
 }
@@ -222,7 +230,6 @@  int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
 		u32 bit_group_0 = 0;
 		u32 bit_group_1 = 0;
 		int err;
-		u32 *cs;
 
 		err = mtl_dummy_pipe_control(rq);
 		if (err)
@@ -256,13 +263,11 @@  int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
 		else if (engine->class == COMPUTE_CLASS)
 			bit_group_1 &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
 
-		cs = intel_ring_begin(rq, 6);
-		if (IS_ERR(cs))
-			return PTR_ERR(cs);
-
-		cs = gen12_emit_pipe_control(cs, bit_group_0, bit_group_1,
-					     LRC_PPHWSP_SCRATCH_ADDR);
-		intel_ring_advance(rq, cs);
+		err = gen12_emit_pipe_control_cs(rq, bit_group_0, bit_group_1,
+						 LRC_PPHWSP_SCRATCH_ADDR);
+		if (err)
+			gt_warn(engine->gt,
+				"Failed to emit flush pipe control\n");
 	}
 
 	if (mode & EMIT_INVALIDATE) {