drm/i915/cmdparser: whitelist needed predicate registers for Anv

Submitted by Lionel Landwerlin on Jan. 11, 2019, 5:53 p.m.

Details

Message ID 20190111175336.2593-1-lionel.g.landwerlin@intel.com
State New
Headers show
Series "drm/i915/cmdparser: whitelist needed predicate registers for Anv" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Lionel Landwerlin Jan. 11, 2019, 5:53 p.m.
There is no reason not to whitelist those registers. In particular
MI_PREDICATE_RESULT can be loaded outside of MI_PREDICATE through
other registers to predicate other commands.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +++++-
 drivers/gpu/drm/i915/i915_reg.h        | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 95478db9998b..c5bf14c3f540 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -549,6 +549,8 @@  static const struct drm_i915_reg_descriptor gen7_render_regs[] = {
 	REG64_IDX(RING_TIMESTAMP, RENDER_RING_BASE),
 	REG64(MI_PREDICATE_SRC0),
 	REG64(MI_PREDICATE_SRC1),
+	REG64(MI_PREDICATE_DATA),
+	REG32(MI_PREDICATE_RESULT),
 	REG32(GEN7_3DPRIM_END_OFFSET),
 	REG32(GEN7_3DPRIM_START_VERTEX),
 	REG32(GEN7_3DPRIM_VERTEX_COUNT),
@@ -1382,6 +1384,8 @@  int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv)
 	 *    the parser enabled.
 	 * 9. Don't whitelist or handle oacontrol specially, as ownership
 	 *    for oacontrol state is moving to i915-perf.
+	 * 10. Whitelist predicate data/result registers for conditional
+	 *     rendering in Anv.
 	 */
-	return 9;
+	return 10;
 }
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ca8026ec0655..5c05d73eec8f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -461,6 +461,8 @@  static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define MI_PREDICATE_SRC0_UDW	_MMIO(0x2400 + 4)
 #define MI_PREDICATE_SRC1	_MMIO(0x2408)
 #define MI_PREDICATE_SRC1_UDW	_MMIO(0x2408 + 4)
+#define MI_PREDICATE_DATA	_MMIO(0x2410)
+#define MI_PREDICATE_RESULT	_MMIO(0x2418)
 
 #define MI_PREDICATE_RESULT_2	_MMIO(0x2214)
 #define  LOWER_SLICE_ENABLED	(1 << 0)

Comments

Hi Lionel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v5.0-rc1 next-20190111]
[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/Lionel-Landwerlin/drm-i915-cmdparser-whitelist-needed-predicate-registers-for-Anv/20190113-231821
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x072-201902 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/i915_cmd_parser.c:552:8: error: 'MI_PREDICATE_DATA_UDW' undeclared here (not in a function); did you mean 'MI_PREDICATE_SRC1_UDW'?
     REG64(MI_PREDICATE_DATA),
           ^
   drivers/gpu/drm/i915/i915_cmd_parser.c:530:12: note: in definition of macro 'REG64'
     { .addr = _reg ## _UDW }
               ^~~~

vim +552 drivers/gpu/drm/i915/i915_cmd_parser.c

   516	
   517	/* Convenience macro for adding 32-bit registers. */
   518	#define REG32(_reg, ...) \
   519		{ .addr = (_reg), __VA_ARGS__ }
   520	
   521	/*
   522	 * Convenience macro for adding 64-bit registers.
   523	 *
   524	 * Some registers that userspace accesses are 64 bits. The register
   525	 * access commands only allow 32-bit accesses. Hence, we have to include
   526	 * entries for both halves of the 64-bit registers.
   527	 */
   528	#define REG64(_reg) \
   529		{ .addr = _reg }, \
   530		{ .addr = _reg ## _UDW }
   531	
   532	#define REG64_IDX(_reg, idx) \
   533		{ .addr = _reg(idx) }, \
   534		{ .addr = _reg ## _UDW(idx) }
   535	
   536	static const struct drm_i915_reg_descriptor gen7_render_regs[] = {
   537		REG64(GPGPU_THREADS_DISPATCHED),
   538		REG64(HS_INVOCATION_COUNT),
   539		REG64(DS_INVOCATION_COUNT),
   540		REG64(IA_VERTICES_COUNT),
   541		REG64(IA_PRIMITIVES_COUNT),
   542		REG64(VS_INVOCATION_COUNT),
   543		REG64(GS_INVOCATION_COUNT),
   544		REG64(GS_PRIMITIVES_COUNT),
   545		REG64(CL_INVOCATION_COUNT),
   546		REG64(CL_PRIMITIVES_COUNT),
   547		REG64(PS_INVOCATION_COUNT),
   548		REG64(PS_DEPTH_COUNT),
   549		REG64_IDX(RING_TIMESTAMP, RENDER_RING_BASE),
   550		REG64(MI_PREDICATE_SRC0),
   551		REG64(MI_PREDICATE_SRC1),
 > 552		REG64(MI_PREDICATE_DATA),
   553		REG32(MI_PREDICATE_RESULT),
   554		REG32(GEN7_3DPRIM_END_OFFSET),
   555		REG32(GEN7_3DPRIM_START_VERTEX),
   556		REG32(GEN7_3DPRIM_VERTEX_COUNT),
   557		REG32(GEN7_3DPRIM_INSTANCE_COUNT),
   558		REG32(GEN7_3DPRIM_START_INSTANCE),
   559		REG32(GEN7_3DPRIM_BASE_VERTEX),
   560		REG32(GEN7_GPGPU_DISPATCHDIMX),
   561		REG32(GEN7_GPGPU_DISPATCHDIMY),
   562		REG32(GEN7_GPGPU_DISPATCHDIMZ),
   563		REG64_IDX(RING_TIMESTAMP, BSD_RING_BASE),
   564		REG64_IDX(GEN7_SO_NUM_PRIMS_WRITTEN, 0),
   565		REG64_IDX(GEN7_SO_NUM_PRIMS_WRITTEN, 1),
   566		REG64_IDX(GEN7_SO_NUM_PRIMS_WRITTEN, 2),
   567		REG64_IDX(GEN7_SO_NUM_PRIMS_WRITTEN, 3),
   568		REG64_IDX(GEN7_SO_PRIM_STORAGE_NEEDED, 0),
   569		REG64_IDX(GEN7_SO_PRIM_STORAGE_NEEDED, 1),
   570		REG64_IDX(GEN7_SO_PRIM_STORAGE_NEEDED, 2),
   571		REG64_IDX(GEN7_SO_PRIM_STORAGE_NEEDED, 3),
   572		REG32(GEN7_SO_WRITE_OFFSET(0)),
   573		REG32(GEN7_SO_WRITE_OFFSET(1)),
   574		REG32(GEN7_SO_WRITE_OFFSET(2)),
   575		REG32(GEN7_SO_WRITE_OFFSET(3)),
   576		REG32(GEN7_L3SQCREG1),
   577		REG32(GEN7_L3CNTLREG2),
   578		REG32(GEN7_L3CNTLREG3),
   579		REG64_IDX(RING_TIMESTAMP, BLT_RING_BASE),
   580	};
   581	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Hi Lionel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v5.0-rc1 next-20190111]
[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/Lionel-Landwerlin/drm-i915-cmdparser-whitelist-needed-predicate-registers-for-Anv/20190113-231821
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-s1-201902 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> drivers/gpu//drm/i915/i915_cmd_parser.c:552:8: error: 'MI_PREDICATE_DATA_UDW' undeclared here (not in a function)
     REG64(MI_PREDICATE_DATA),
           ^
   drivers/gpu//drm/i915/i915_cmd_parser.c:530:12: note: in definition of macro 'REG64'
     { .addr = _reg ## _UDW }
               ^~~~

vim +/MI_PREDICATE_DATA_UDW +552 drivers/gpu//drm/i915/i915_cmd_parser.c

   516	
   517	/* Convenience macro for adding 32-bit registers. */
   518	#define REG32(_reg, ...) \
   519		{ .addr = (_reg), __VA_ARGS__ }
   520	
   521	/*
   522	 * Convenience macro for adding 64-bit registers.
   523	 *
   524	 * Some registers that userspace accesses are 64 bits. The register
   525	 * access commands only allow 32-bit accesses. Hence, we have to include
   526	 * entries for both halves of the 64-bit registers.
   527	 */
   528	#define REG64(_reg) \
   529		{ .addr = _reg }, \
   530		{ .addr = _reg ## _UDW }
   531	
   532	#define REG64_IDX(_reg, idx) \
   533		{ .addr = _reg(idx) }, \
   534		{ .addr = _reg ## _UDW(idx) }
   535	
   536	static const struct drm_i915_reg_descriptor gen7_render_regs[] = {
   537		REG64(GPGPU_THREADS_DISPATCHED),
   538		REG64(HS_INVOCATION_COUNT),
   539		REG64(DS_INVOCATION_COUNT),
   540		REG64(IA_VERTICES_COUNT),
   541		REG64(IA_PRIMITIVES_COUNT),
   542		REG64(VS_INVOCATION_COUNT),
   543		REG64(GS_INVOCATION_COUNT),
   544		REG64(GS_PRIMITIVES_COUNT),
   545		REG64(CL_INVOCATION_COUNT),
   546		REG64(CL_PRIMITIVES_COUNT),
   547		REG64(PS_INVOCATION_COUNT),
   548		REG64(PS_DEPTH_COUNT),
   549		REG64_IDX(RING_TIMESTAMP, RENDER_RING_BASE),
   550		REG64(MI_PREDICATE_SRC0),
   551		REG64(MI_PREDICATE_SRC1),
 > 552		REG64(MI_PREDICATE_DATA),
   553		REG32(MI_PREDICATE_RESULT),
   554		REG32(GEN7_3DPRIM_END_OFFSET),
   555		REG32(GEN7_3DPRIM_START_VERTEX),
   556		REG32(GEN7_3DPRIM_VERTEX_COUNT),
   557		REG32(GEN7_3DPRIM_INSTANCE_COUNT),
   558		REG32(GEN7_3DPRIM_START_INSTANCE),
   559		REG32(GEN7_3DPRIM_BASE_VERTEX),
   560		REG32(GEN7_GPGPU_DISPATCHDIMX),
   561		REG32(GEN7_GPGPU_DISPATCHDIMY),
   562		REG32(GEN7_GPGPU_DISPATCHDIMZ),
   563		REG64_IDX(RING_TIMESTAMP, BSD_RING_BASE),
   564		REG64_IDX(GEN7_SO_NUM_PRIMS_WRITTEN, 0),
   565		REG64_IDX(GEN7_SO_NUM_PRIMS_WRITTEN, 1),
   566		REG64_IDX(GEN7_SO_NUM_PRIMS_WRITTEN, 2),
   567		REG64_IDX(GEN7_SO_NUM_PRIMS_WRITTEN, 3),
   568		REG64_IDX(GEN7_SO_PRIM_STORAGE_NEEDED, 0),
   569		REG64_IDX(GEN7_SO_PRIM_STORAGE_NEEDED, 1),
   570		REG64_IDX(GEN7_SO_PRIM_STORAGE_NEEDED, 2),
   571		REG64_IDX(GEN7_SO_PRIM_STORAGE_NEEDED, 3),
   572		REG32(GEN7_SO_WRITE_OFFSET(0)),
   573		REG32(GEN7_SO_WRITE_OFFSET(1)),
   574		REG32(GEN7_SO_WRITE_OFFSET(2)),
   575		REG32(GEN7_SO_WRITE_OFFSET(3)),
   576		REG32(GEN7_L3SQCREG1),
   577		REG32(GEN7_L3CNTLREG2),
   578		REG32(GEN7_L3CNTLREG3),
   579		REG64_IDX(RING_TIMESTAMP, BLT_RING_BASE),
   580	};
   581	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Quoting Lionel Landwerlin (2019-01-11 19:53:36)
> There is no reason not to whitelist those registers. In particular
> MI_PREDICATE_RESULT can be loaded outside of MI_PREDICATE through
> other registers to predicate other commands.

Link to userspace changes?

Regards, Joonas

> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +++++-
>  drivers/gpu/drm/i915/i915_reg.h        | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 95478db9998b..c5bf14c3f540 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -549,6 +549,8 @@ static const struct drm_i915_reg_descriptor gen7_render_regs[] = {
>         REG64_IDX(RING_TIMESTAMP, RENDER_RING_BASE),
>         REG64(MI_PREDICATE_SRC0),
>         REG64(MI_PREDICATE_SRC1),
> +       REG64(MI_PREDICATE_DATA),
> +       REG32(MI_PREDICATE_RESULT),
>         REG32(GEN7_3DPRIM_END_OFFSET),
>         REG32(GEN7_3DPRIM_START_VERTEX),
>         REG32(GEN7_3DPRIM_VERTEX_COUNT),
> @@ -1382,6 +1384,8 @@ int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv)
>          *    the parser enabled.
>          * 9. Don't whitelist or handle oacontrol specially, as ownership
>          *    for oacontrol state is moving to i915-perf.
> +        * 10. Whitelist predicate data/result registers for conditional
> +        *     rendering in Anv.
>          */
> -       return 9;
> +       return 10;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ca8026ec0655..5c05d73eec8f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -461,6 +461,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define MI_PREDICATE_SRC0_UDW  _MMIO(0x2400 + 4)
>  #define MI_PREDICATE_SRC1      _MMIO(0x2408)
>  #define MI_PREDICATE_SRC1_UDW  _MMIO(0x2408 + 4)
> +#define MI_PREDICATE_DATA      _MMIO(0x2410)
> +#define MI_PREDICATE_RESULT    _MMIO(0x2418)
>  
>  #define MI_PREDICATE_RESULT_2  _MMIO(0x2214)
>  #define  LOWER_SLICE_ENABLED   (1 << 0)
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 01/02/2019 12:30, Joonas Lahtinen wrote:
> Quoting Lionel Landwerlin (2019-01-11 19:53:36)
>> There is no reason not to whitelist those registers. In particular
>> MI_PREDICATE_RESULT can be loaded outside of MI_PREDICATE through
>> other registers to predicate other commands.
> Link to userspace changes?
>
> Regards, Joonas


So we ended up using MI_PREDICATE instead of loading the predicate 
registers using MI_LRI/LRM.

Although a bit sad, it's just easier to have one way to deal with those 
registers rather than add more code to check the command parser version.


-Lionel


>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +++++-
>>   drivers/gpu/drm/i915/i915_reg.h        | 2 ++
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> index 95478db9998b..c5bf14c3f540 100644
>> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> @@ -549,6 +549,8 @@ static const struct drm_i915_reg_descriptor gen7_render_regs[] = {
>>          REG64_IDX(RING_TIMESTAMP, RENDER_RING_BASE),
>>          REG64(MI_PREDICATE_SRC0),
>>          REG64(MI_PREDICATE_SRC1),
>> +       REG64(MI_PREDICATE_DATA),
>> +       REG32(MI_PREDICATE_RESULT),
>>          REG32(GEN7_3DPRIM_END_OFFSET),
>>          REG32(GEN7_3DPRIM_START_VERTEX),
>>          REG32(GEN7_3DPRIM_VERTEX_COUNT),
>> @@ -1382,6 +1384,8 @@ int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv)
>>           *    the parser enabled.
>>           * 9. Don't whitelist or handle oacontrol specially, as ownership
>>           *    for oacontrol state is moving to i915-perf.
>> +        * 10. Whitelist predicate data/result registers for conditional
>> +        *     rendering in Anv.
>>           */
>> -       return 9;
>> +       return 10;
>>   }
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index ca8026ec0655..5c05d73eec8f 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -461,6 +461,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>>   #define MI_PREDICATE_SRC0_UDW  _MMIO(0x2400 + 4)
>>   #define MI_PREDICATE_SRC1      _MMIO(0x2408)
>>   #define MI_PREDICATE_SRC1_UDW  _MMIO(0x2408 + 4)
>> +#define MI_PREDICATE_DATA      _MMIO(0x2410)
>> +#define MI_PREDICATE_RESULT    _MMIO(0x2418)
>>   
>>   #define MI_PREDICATE_RESULT_2  _MMIO(0x2214)
>>   #define  LOWER_SLICE_ENABLED   (1 << 0)
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx