[Intel-gfx] [PATCH] drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Intel-gfx] [PATCH] drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Jani Nikula]

Underscore prefix the index macros, and place
INTEL_HWS_CSB_WRITE_INDEX() as a macro next to them, to declutter
i915_drv.h.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine.h               | 6 ++++--
 drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 +-
 drivers/gpu/drm/i915/gvt/execlist.c                  | 2 +-
 drivers/gpu/drm/i915/i915_drv.h                      | 8 --------
 4 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 0e353d8c2bc8..faf26ed37d01 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -180,8 +180,10 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
 #define I915_GEM_HWS_SCRATCH		0x80
 
 #define I915_HWS_CSB_BUF0_INDEX		0x10
-#define I915_HWS_CSB_WRITE_INDEX	0x1f
-#define ICL_HWS_CSB_WRITE_INDEX		0x2f
+#define _I915_HWS_CSB_WRITE_INDEX	0x1f
+#define _ICL_HWS_CSB_WRITE_INDEX		0x2f
+#define INTEL_HWS_CSB_WRITE_INDEX(__i915) \
+	(GRAPHICS_VER(__i915) >= 11 ? _ICL_HWS_CSB_WRITE_INDEX : _I915_HWS_CSB_WRITE_INDEX)
 
 void intel_engine_stop(struct intel_engine_cs *engine);
 void intel_engine_cleanup(struct intel_engine_cs *engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 9bb7c863172f..961d795220a3 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -3503,7 +3503,7 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
 		(u64 *)&engine->status_page.addr[I915_HWS_CSB_BUF0_INDEX];
 
 	execlists->csb_write =
-		&engine->status_page.addr[intel_hws_csb_write_index(i915)];
+		&engine->status_page.addr[INTEL_HWS_CSB_WRITE_INDEX(i915)];
 
 	if (GRAPHICS_VER(i915) < 11)
 		execlists->csb_size = GEN8_CSB_ENTRIES;
diff --git a/drivers/gpu/drm/i915/gvt/execlist.c b/drivers/gpu/drm/i915/gvt/execlist.c
index c8dcda6d4f0d..66d354c4195b 100644
--- a/drivers/gpu/drm/i915/gvt/execlist.c
+++ b/drivers/gpu/drm/i915/gvt/execlist.c
@@ -163,7 +163,7 @@ static void emulate_csb_update(struct intel_vgpu_execlist *execlist,
 					       hwsp_gpa + I915_HWS_CSB_BUF0_INDEX * 4 + write_pointer * 8,
 					       status, 8);
 		intel_gvt_hypervisor_write_gpa(vgpu,
-					       hwsp_gpa + intel_hws_csb_write_index(execlist->engine->i915) * 4,
+					       hwsp_gpa + INTEL_HWS_CSB_WRITE_INDEX(execlist->engine->i915) * 4,
 					       &write_pointer, 4);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8c1706fd81f9..05656cc738d1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1739,14 +1739,6 @@ mkwrite_device_info(struct drm_i915_private *dev_priv)
 	return (struct intel_device_info *)INTEL_INFO(dev_priv);
 }
 
-static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)
-{
-	if (GRAPHICS_VER(i915) >= 11)
-		return ICL_HWS_CSB_WRITE_INDEX;
-	else
-		return I915_HWS_CSB_WRITE_INDEX;
-}
-
 static inline enum i915_map_type
 i915_coherent_map_type(struct drm_i915_private *i915,
 		       struct drm_i915_gem_object *obj, bool always_coherent)
-- 
2.30.2

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Patchwork]

== Series Details ==

Series: drm/i915: move intel_hws_csb_write_index() out of i915_drv.h
URL   : https://patchwork.freedesktop.org/series/99853/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
8ed7bbe053b5 drm/i915: move intel_hws_csb_write_index() out of i915_drv.h
-:51: WARNING:LONG_LINE: line length of 112 exceeds 100 columns
#51: FILE: drivers/gpu/drm/i915/gvt/execlist.c:166:
+					       hwsp_gpa + INTEL_HWS_CSB_WRITE_INDEX(execlist->engine->i915) * 4,

total: 0 errors, 1 warnings, 0 checks, 42 lines checked

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Patchwork]

== Series Details ==

Series: drm/i915: move intel_hws_csb_write_index() out of i915_drv.h
URL   : https://patchwork.freedesktop.org/series/99853/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4698 bytes --]

== Series Details ==

Series: drm/i915: move intel_hws_csb_write_index() out of i915_drv.h
URL   : https://patchwork.freedesktop.org/series/99853/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11203 -> Patchwork_22211
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_22211 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_22211, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/index.html

Participating hosts (45 -> 43)
------------------------------

  Missing    (2): shard-tglu fi-pnv-d510 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_22211:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - fi-bdw-5557u:       NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/fi-bdw-5557u/igt@i915_selftest@live@hangcheck.html

  
Known issues
------------

  Here are the changes found in Patchwork_22211 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@requests:
    - fi-blb-e6850:       [PASS][2] -> [DMESG-FAIL][3] ([i915#4528] / [i915#5026])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11203/fi-blb-e6850/igt@i915_selftest@live@requests.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/fi-blb-e6850/igt@i915_selftest@live@requests.html

  * igt@kms_chamelium@vga-edid-read:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][4] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/fi-bdw-5557u/igt@kms_chamelium@vga-edid-read.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-b:
    - fi-cfl-8109u:       [PASS][5] -> [DMESG-WARN][6] ([i915#295]) +12 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11203/fi-cfl-8109u/igt@kms_pipe_crc_basic@read-crc-pipe-b.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/fi-cfl-8109u/igt@kms_pipe_crc_basic@read-crc-pipe-b.html

  * igt@kms_psr@cursor_plane_move:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][7] ([fdo#109271]) +13 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/fi-bdw-5557u/igt@kms_psr@cursor_plane_move.html

  * igt@runner@aborted:
    - fi-blb-e6850:       NOTRUN -> [FAIL][8] ([fdo#109271] / [i915#2403] / [i915#2426] / [i915#4312])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/fi-blb-e6850/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-skl-guc:         [DMESG-FAIL][9] ([i915#2291] / [i915#541]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11203/fi-skl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/fi-skl-guc/igt@i915_selftest@live@gt_heartbeat.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#2291]: https://gitlab.freedesktop.org/drm/intel/issues/2291
  [i915#2403]: https://gitlab.freedesktop.org/drm/intel/issues/2403
  [i915#2426]: https://gitlab.freedesktop.org/drm/intel/issues/2426
  [i915#295]: https://gitlab.freedesktop.org/drm/intel/issues/295
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#4898]: https://gitlab.freedesktop.org/drm/intel/issues/4898
  [i915#5026]: https://gitlab.freedesktop.org/drm/intel/issues/5026
  [i915#541]: https://gitlab.freedesktop.org/drm/intel/issues/541


Build changes
-------------

  * Linux: CI_DRM_11203 -> Patchwork_22211

  CI-20190529: 20190529
  CI_DRM_11203: e0f14f95759ad65e896868e1f9efd3247d93d28e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6341: a96674e747ea2f2431bbf8813156adc44ec3162a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_22211: 8ed7bbe053b56b8ca9dc69705b7c8a719f5fd692 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8ed7bbe053b5 drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22211/index.html

[-- Attachment #2: Type: text/html, Size: 5535 bytes --]

Re: [Intel-gfx] [PATCH] drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Tvrtko Ursulin]

On 08/02/2022 17:16, Jani Nikula wrote:
> Underscore prefix the index macros, and place
> INTEL_HWS_CSB_WRITE_INDEX() as a macro next to them, to declutter
> i915_drv.h.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine.h               | 6 ++++--
>   drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 +-
>   drivers/gpu/drm/i915/gvt/execlist.c                  | 2 +-
>   drivers/gpu/drm/i915/i915_drv.h                      | 8 --------
>   4 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index 0e353d8c2bc8..faf26ed37d01 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -180,8 +180,10 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
>   #define I915_GEM_HWS_SCRATCH		0x80
>   
>   #define I915_HWS_CSB_BUF0_INDEX		0x10
> -#define I915_HWS_CSB_WRITE_INDEX	0x1f
> -#define ICL_HWS_CSB_WRITE_INDEX		0x2f
> +#define _I915_HWS_CSB_WRITE_INDEX	0x1f
> +#define _ICL_HWS_CSB_WRITE_INDEX		0x2f

I don't quite get why would these two be the only ones which need 
underscore prefix?

> +#define INTEL_HWS_CSB_WRITE_INDEX(__i915) \
> +	(GRAPHICS_VER(__i915) >= 11 ? _ICL_HWS_CSB_WRITE_INDEX : _I915_HWS_CSB_WRITE_INDEX)

Secondly, on the point of the best new home for it, it is better than 
i915_drv.h that is for sure. But is it the best I am not sure.

CSB in general seems to have identity crisis with some bits being in 
intel_engine.h, but some also in intel_lrc.h. Neither seems completely 
right to me. It should all really be in intel_execlists_submission.h. 
Unless someone would then make an argument the latter is about the i915 
execlists scheduler backend and shouldn't strictly contain execlists 
hardware definitions like the CSB layout. Still, at the moment for me it 
feels like a better place than either intel_engine.h or intel_lrc.h.

And probably all three execlists_ prefix functions from intel_engine.h 
should go to intel_execlists_submission.h as well.

We can leave the ramblings for later and for now just explain why 
underscores please, depending on which r-b or not.

Regards,

Tvrtko

>   
>   void intel_engine_stop(struct intel_engine_cs *engine);
>   void intel_engine_cleanup(struct intel_engine_cs *engine);
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 9bb7c863172f..961d795220a3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -3503,7 +3503,7 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
>   		(u64 *)&engine->status_page.addr[I915_HWS_CSB_BUF0_INDEX];
>   
>   	execlists->csb_write =
> -		&engine->status_page.addr[intel_hws_csb_write_index(i915)];
> +		&engine->status_page.addr[INTEL_HWS_CSB_WRITE_INDEX(i915)];
>   
>   	if (GRAPHICS_VER(i915) < 11)
>   		execlists->csb_size = GEN8_CSB_ENTRIES;
> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c b/drivers/gpu/drm/i915/gvt/execlist.c
> index c8dcda6d4f0d..66d354c4195b 100644
> --- a/drivers/gpu/drm/i915/gvt/execlist.c
> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
> @@ -163,7 +163,7 @@ static void emulate_csb_update(struct intel_vgpu_execlist *execlist,
>   					       hwsp_gpa + I915_HWS_CSB_BUF0_INDEX * 4 + write_pointer * 8,
>   					       status, 8);
>   		intel_gvt_hypervisor_write_gpa(vgpu,
> -					       hwsp_gpa + intel_hws_csb_write_index(execlist->engine->i915) * 4,
> +					       hwsp_gpa + INTEL_HWS_CSB_WRITE_INDEX(execlist->engine->i915) * 4,
>   					       &write_pointer, 4);
>   	}
>   
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8c1706fd81f9..05656cc738d1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1739,14 +1739,6 @@ mkwrite_device_info(struct drm_i915_private *dev_priv)
>   	return (struct intel_device_info *)INTEL_INFO(dev_priv);
>   }
>   
> -static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)
> -{
> -	if (GRAPHICS_VER(i915) >= 11)
> -		return ICL_HWS_CSB_WRITE_INDEX;
> -	else
> -		return I915_HWS_CSB_WRITE_INDEX;
> -}
> -
>   static inline enum i915_map_type
>   i915_coherent_map_type(struct drm_i915_private *i915,
>   		       struct drm_i915_gem_object *obj, bool always_coherent)

Re: [Intel-gfx] [PATCH] drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Jani Nikula]

On Wed, 09 Feb 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 08/02/2022 17:16, Jani Nikula wrote:
>> Underscore prefix the index macros, and place
>> INTEL_HWS_CSB_WRITE_INDEX() as a macro next to them, to declutter
>> i915_drv.h.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine.h               | 6 ++++--
>>   drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 +-
>>   drivers/gpu/drm/i915/gvt/execlist.c                  | 2 +-
>>   drivers/gpu/drm/i915/i915_drv.h                      | 8 --------
>>   4 files changed, 6 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
>> index 0e353d8c2bc8..faf26ed37d01 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
>> @@ -180,8 +180,10 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
>>   #define I915_GEM_HWS_SCRATCH		0x80
>>   
>>   #define I915_HWS_CSB_BUF0_INDEX		0x10
>> -#define I915_HWS_CSB_WRITE_INDEX	0x1f
>> -#define ICL_HWS_CSB_WRITE_INDEX		0x2f
>> +#define _I915_HWS_CSB_WRITE_INDEX	0x1f
>> +#define _ICL_HWS_CSB_WRITE_INDEX		0x2f
>
> I don't quite get why would these two be the only ones which need 
> underscore prefix?

The others are used directly, these two should only be used via
INTEL_HWS_CSB_WRITE_INDEX(), like they are. That's the hint with the
underscores. Matches what's done in i915_reg.h for example for register
instances and choosing the right register instance.

>
>> +#define INTEL_HWS_CSB_WRITE_INDEX(__i915) \
>> +	(GRAPHICS_VER(__i915) >= 11 ? _ICL_HWS_CSB_WRITE_INDEX : _I915_HWS_CSB_WRITE_INDEX)
>
> Secondly, on the point of the best new home for it, it is better than 
> i915_drv.h that is for sure. But is it the best I am not sure.

From my POV this one's pretty clear. The index macros are here, and this
is a wrapper to choose the correct index macro. They should be together.

As to moving all of them somewhere in gt/gem, I think it's for follow-up
(by someone else).


BR,
Jani.

>
> CSB in general seems to have identity crisis with some bits being in 
> intel_engine.h, but some also in intel_lrc.h. Neither seems completely 
> right to me. It should all really be in intel_execlists_submission.h. 
> Unless someone would then make an argument the latter is about the i915 
> execlists scheduler backend and shouldn't strictly contain execlists 
> hardware definitions like the CSB layout. Still, at the moment for me it 
> feels like a better place than either intel_engine.h or intel_lrc.h.
>
> And probably all three execlists_ prefix functions from intel_engine.h 
> should go to intel_execlists_submission.h as well.
>
> We can leave the ramblings for later and for now just explain why 
> underscores please, depending on which r-b or not.
>
> Regards,
>
> Tvrtko
>
>>   
>>   void intel_engine_stop(struct intel_engine_cs *engine);
>>   void intel_engine_cleanup(struct intel_engine_cs *engine);
>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> index 9bb7c863172f..961d795220a3 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> @@ -3503,7 +3503,7 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
>>   		(u64 *)&engine->status_page.addr[I915_HWS_CSB_BUF0_INDEX];
>>   
>>   	execlists->csb_write =
>> -		&engine->status_page.addr[intel_hws_csb_write_index(i915)];
>> +		&engine->status_page.addr[INTEL_HWS_CSB_WRITE_INDEX(i915)];
>>   
>>   	if (GRAPHICS_VER(i915) < 11)
>>   		execlists->csb_size = GEN8_CSB_ENTRIES;
>> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c b/drivers/gpu/drm/i915/gvt/execlist.c
>> index c8dcda6d4f0d..66d354c4195b 100644
>> --- a/drivers/gpu/drm/i915/gvt/execlist.c
>> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
>> @@ -163,7 +163,7 @@ static void emulate_csb_update(struct intel_vgpu_execlist *execlist,
>>   					       hwsp_gpa + I915_HWS_CSB_BUF0_INDEX * 4 + write_pointer * 8,
>>   					       status, 8);
>>   		intel_gvt_hypervisor_write_gpa(vgpu,
>> -					       hwsp_gpa + intel_hws_csb_write_index(execlist->engine->i915) * 4,
>> +					       hwsp_gpa + INTEL_HWS_CSB_WRITE_INDEX(execlist->engine->i915) * 4,
>>   					       &write_pointer, 4);
>>   	}
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 8c1706fd81f9..05656cc738d1 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1739,14 +1739,6 @@ mkwrite_device_info(struct drm_i915_private *dev_priv)
>>   	return (struct intel_device_info *)INTEL_INFO(dev_priv);
>>   }
>>   
>> -static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)
>> -{
>> -	if (GRAPHICS_VER(i915) >= 11)
>> -		return ICL_HWS_CSB_WRITE_INDEX;
>> -	else
>> -		return I915_HWS_CSB_WRITE_INDEX;
>> -}
>> -
>>   static inline enum i915_map_type
>>   i915_coherent_map_type(struct drm_i915_private *i915,
>>   		       struct drm_i915_gem_object *obj, bool always_coherent)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Tvrtko Ursulin]

On 09/02/2022 10:00, Jani Nikula wrote:
> On Wed, 09 Feb 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 08/02/2022 17:16, Jani Nikula wrote:
>>> Underscore prefix the index macros, and place
>>> INTEL_HWS_CSB_WRITE_INDEX() as a macro next to them, to declutter
>>> i915_drv.h.
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_engine.h               | 6 ++++--
>>>    drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 +-
>>>    drivers/gpu/drm/i915/gvt/execlist.c                  | 2 +-
>>>    drivers/gpu/drm/i915/i915_drv.h                      | 8 --------
>>>    4 files changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
>>> index 0e353d8c2bc8..faf26ed37d01 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
>>> @@ -180,8 +180,10 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
>>>    #define I915_GEM_HWS_SCRATCH		0x80
>>>    
>>>    #define I915_HWS_CSB_BUF0_INDEX		0x10
>>> -#define I915_HWS_CSB_WRITE_INDEX	0x1f
>>> -#define ICL_HWS_CSB_WRITE_INDEX		0x2f
>>> +#define _I915_HWS_CSB_WRITE_INDEX	0x1f
>>> +#define _ICL_HWS_CSB_WRITE_INDEX		0x2f
>>
>> I don't quite get why would these two be the only ones which need
>> underscore prefix?
> 
> The others are used directly, these two should only be used via
> INTEL_HWS_CSB_WRITE_INDEX(), like they are. That's the hint with the
> underscores. Matches what's done in i915_reg.h for example for register
> instances and choosing the right register instance.

Oh that logic, okay, I don't think it is "should" and would have left it 
as is (closer example than register groups is I think per gen variance 
of CSB layout in intel_lrc_reg.h, or engine mmio base), but don't mind 
hugely either.
>>
>>> +#define INTEL_HWS_CSB_WRITE_INDEX(__i915) \
>>> +	(GRAPHICS_VER(__i915) >= 11 ? _ICL_HWS_CSB_WRITE_INDEX : _I915_HWS_CSB_WRITE_INDEX)
>>
>> Secondly, on the point of the best new home for it, it is better than
>> i915_drv.h that is for sure. But is it the best I am not sure.
> 
>>From my POV this one's pretty clear. The index macros are here, and this
> is a wrapper to choose the correct index macro. They should be together.
> 
> As to moving all of them somewhere in gt/gem, I think it's for follow-up
> (by someone else).

Yes yes, that's why I called it ramblings. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> 
> 
> BR,
> Jani.
> 
>>
>> CSB in general seems to have identity crisis with some bits being in
>> intel_engine.h, but some also in intel_lrc.h. Neither seems completely
>> right to me. It should all really be in intel_execlists_submission.h.
>> Unless someone would then make an argument the latter is about the i915
>> execlists scheduler backend and shouldn't strictly contain execlists
>> hardware definitions like the CSB layout. Still, at the moment for me it
>> feels like a better place than either intel_engine.h or intel_lrc.h.
>>
>> And probably all three execlists_ prefix functions from intel_engine.h
>> should go to intel_execlists_submission.h as well.
>>
>> We can leave the ramblings for later and for now just explain why
>> underscores please, depending on which r-b or not.
>>
>> Regards,
>>
>> Tvrtko
>>
>>>    
>>>    void intel_engine_stop(struct intel_engine_cs *engine);
>>>    void intel_engine_cleanup(struct intel_engine_cs *engine);
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> index 9bb7c863172f..961d795220a3 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> @@ -3503,7 +3503,7 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
>>>    		(u64 *)&engine->status_page.addr[I915_HWS_CSB_BUF0_INDEX];
>>>    
>>>    	execlists->csb_write =
>>> -		&engine->status_page.addr[intel_hws_csb_write_index(i915)];
>>> +		&engine->status_page.addr[INTEL_HWS_CSB_WRITE_INDEX(i915)];
>>>    
>>>    	if (GRAPHICS_VER(i915) < 11)
>>>    		execlists->csb_size = GEN8_CSB_ENTRIES;
>>> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c b/drivers/gpu/drm/i915/gvt/execlist.c
>>> index c8dcda6d4f0d..66d354c4195b 100644
>>> --- a/drivers/gpu/drm/i915/gvt/execlist.c
>>> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
>>> @@ -163,7 +163,7 @@ static void emulate_csb_update(struct intel_vgpu_execlist *execlist,
>>>    					       hwsp_gpa + I915_HWS_CSB_BUF0_INDEX * 4 + write_pointer * 8,
>>>    					       status, 8);
>>>    		intel_gvt_hypervisor_write_gpa(vgpu,
>>> -					       hwsp_gpa + intel_hws_csb_write_index(execlist->engine->i915) * 4,
>>> +					       hwsp_gpa + INTEL_HWS_CSB_WRITE_INDEX(execlist->engine->i915) * 4,
>>>    					       &write_pointer, 4);
>>>    	}
>>>    
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 8c1706fd81f9..05656cc738d1 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1739,14 +1739,6 @@ mkwrite_device_info(struct drm_i915_private *dev_priv)
>>>    	return (struct intel_device_info *)INTEL_INFO(dev_priv);
>>>    }
>>>    
>>> -static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)
>>> -{
>>> -	if (GRAPHICS_VER(i915) >= 11)
>>> -		return ICL_HWS_CSB_WRITE_INDEX;
>>> -	else
>>> -		return I915_HWS_CSB_WRITE_INDEX;
>>> -}
>>> -
>>>    static inline enum i915_map_type
>>>    i915_coherent_map_type(struct drm_i915_private *i915,
>>>    		       struct drm_i915_gem_object *obj, bool always_coherent)
> 

Re: [Intel-gfx] [PATCH] drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Jani Nikula]

On Wed, 09 Feb 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 09/02/2022 10:00, Jani Nikula wrote:
>> On Wed, 09 Feb 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 08/02/2022 17:16, Jani Nikula wrote:
>>>> Underscore prefix the index macros, and place
>>>> INTEL_HWS_CSB_WRITE_INDEX() as a macro next to them, to declutter
>>>> i915_drv.h.
>>>>
>>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gt/intel_engine.h               | 6 ++++--
>>>>    drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 +-
>>>>    drivers/gpu/drm/i915/gvt/execlist.c                  | 2 +-
>>>>    drivers/gpu/drm/i915/i915_drv.h                      | 8 --------
>>>>    4 files changed, 6 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
>>>> index 0e353d8c2bc8..faf26ed37d01 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
>>>> @@ -180,8 +180,10 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
>>>>    #define I915_GEM_HWS_SCRATCH		0x80
>>>>    
>>>>    #define I915_HWS_CSB_BUF0_INDEX		0x10
>>>> -#define I915_HWS_CSB_WRITE_INDEX	0x1f
>>>> -#define ICL_HWS_CSB_WRITE_INDEX		0x2f
>>>> +#define _I915_HWS_CSB_WRITE_INDEX	0x1f
>>>> +#define _ICL_HWS_CSB_WRITE_INDEX		0x2f
>>>
>>> I don't quite get why would these two be the only ones which need
>>> underscore prefix?
>> 
>> The others are used directly, these two should only be used via
>> INTEL_HWS_CSB_WRITE_INDEX(), like they are. That's the hint with the
>> underscores. Matches what's done in i915_reg.h for example for register
>> instances and choosing the right register instance.
>
> Oh that logic, okay, I don't think it is "should" and would have left it 
> as is (closer example than register groups is I think per gen variance 
> of CSB layout in intel_lrc_reg.h, or engine mmio base), but don't mind 
> hugely either.

Ok, sent v2 with the underscores removed.

>>>
>>>> +#define INTEL_HWS_CSB_WRITE_INDEX(__i915) \
>>>> +	(GRAPHICS_VER(__i915) >= 11 ? _ICL_HWS_CSB_WRITE_INDEX : _I915_HWS_CSB_WRITE_INDEX)
>>>
>>> Secondly, on the point of the best new home for it, it is better than
>>> i915_drv.h that is for sure. But is it the best I am not sure.
>> 
>>>From my POV this one's pretty clear. The index macros are here, and this
>> is a wrapper to choose the correct index macro. They should be together.
>> 
>> As to moving all of them somewhere in gt/gem, I think it's for follow-up
>> (by someone else).
>
> Yes yes, that's why I called it ramblings. :)
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks,
Jani.

>
> Regards,
>
> Tvrtko
>
>> 
>> 
>> BR,
>> Jani.
>> 
>>>
>>> CSB in general seems to have identity crisis with some bits being in
>>> intel_engine.h, but some also in intel_lrc.h. Neither seems completely
>>> right to me. It should all really be in intel_execlists_submission.h.
>>> Unless someone would then make an argument the latter is about the i915
>>> execlists scheduler backend and shouldn't strictly contain execlists
>>> hardware definitions like the CSB layout. Still, at the moment for me it
>>> feels like a better place than either intel_engine.h or intel_lrc.h.
>>>
>>> And probably all three execlists_ prefix functions from intel_engine.h
>>> should go to intel_execlists_submission.h as well.
>>>
>>> We can leave the ramblings for later and for now just explain why
>>> underscores please, depending on which r-b or not.
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>    
>>>>    void intel_engine_stop(struct intel_engine_cs *engine);
>>>>    void intel_engine_cleanup(struct intel_engine_cs *engine);
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>>> index 9bb7c863172f..961d795220a3 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>>> @@ -3503,7 +3503,7 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
>>>>    		(u64 *)&engine->status_page.addr[I915_HWS_CSB_BUF0_INDEX];
>>>>    
>>>>    	execlists->csb_write =
>>>> -		&engine->status_page.addr[intel_hws_csb_write_index(i915)];
>>>> +		&engine->status_page.addr[INTEL_HWS_CSB_WRITE_INDEX(i915)];
>>>>    
>>>>    	if (GRAPHICS_VER(i915) < 11)
>>>>    		execlists->csb_size = GEN8_CSB_ENTRIES;
>>>> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c b/drivers/gpu/drm/i915/gvt/execlist.c
>>>> index c8dcda6d4f0d..66d354c4195b 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/execlist.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
>>>> @@ -163,7 +163,7 @@ static void emulate_csb_update(struct intel_vgpu_execlist *execlist,
>>>>    					       hwsp_gpa + I915_HWS_CSB_BUF0_INDEX * 4 + write_pointer * 8,
>>>>    					       status, 8);
>>>>    		intel_gvt_hypervisor_write_gpa(vgpu,
>>>> -					       hwsp_gpa + intel_hws_csb_write_index(execlist->engine->i915) * 4,
>>>> +					       hwsp_gpa + INTEL_HWS_CSB_WRITE_INDEX(execlist->engine->i915) * 4,
>>>>    					       &write_pointer, 4);
>>>>    	}
>>>>    
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 8c1706fd81f9..05656cc738d1 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1739,14 +1739,6 @@ mkwrite_device_info(struct drm_i915_private *dev_priv)
>>>>    	return (struct intel_device_info *)INTEL_INFO(dev_priv);
>>>>    }
>>>>    
>>>> -static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)
>>>> -{
>>>> -	if (GRAPHICS_VER(i915) >= 11)
>>>> -		return ICL_HWS_CSB_WRITE_INDEX;
>>>> -	else
>>>> -		return I915_HWS_CSB_WRITE_INDEX;
>>>> -}
>>>> -
>>>>    static inline enum i915_map_type
>>>>    i915_coherent_map_type(struct drm_i915_private *i915,
>>>>    		       struct drm_i915_gem_object *obj, bool always_coherent)
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center

[Intel-gfx] [PATCH] drm/i915: move intel_hws_csb_write_index() out of i915_drv.h

[Jani Nikula]

Underscore prefix the index macros, and place
INTEL_HWS_CSB_WRITE_INDEX() as a macro next to them, to declutter
i915_drv.h.

v2: Don't underscore the index macros (Tvrtko)

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine.h               | 2 ++
 drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 +-
 drivers/gpu/drm/i915/gvt/execlist.c                  | 2 +-
 drivers/gpu/drm/i915/i915_drv.h                      | 8 --------
 4 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 0e353d8c2bc8..be4b1e65442f 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -182,6 +182,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
 #define I915_HWS_CSB_BUF0_INDEX		0x10
 #define I915_HWS_CSB_WRITE_INDEX	0x1f
 #define ICL_HWS_CSB_WRITE_INDEX		0x2f
+#define INTEL_HWS_CSB_WRITE_INDEX(__i915) \
+	(GRAPHICS_VER(__i915) >= 11 ? ICL_HWS_CSB_WRITE_INDEX : I915_HWS_CSB_WRITE_INDEX)
 
 void intel_engine_stop(struct intel_engine_cs *engine);
 void intel_engine_cleanup(struct intel_engine_cs *engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 9bb7c863172f..961d795220a3 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -3503,7 +3503,7 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
 		(u64 *)&engine->status_page.addr[I915_HWS_CSB_BUF0_INDEX];
 
 	execlists->csb_write =
-		&engine->status_page.addr[intel_hws_csb_write_index(i915)];
+		&engine->status_page.addr[INTEL_HWS_CSB_WRITE_INDEX(i915)];
 
 	if (GRAPHICS_VER(i915) < 11)
 		execlists->csb_size = GEN8_CSB_ENTRIES;
diff --git a/drivers/gpu/drm/i915/gvt/execlist.c b/drivers/gpu/drm/i915/gvt/execlist.c
index c8dcda6d4f0d..66d354c4195b 100644
--- a/drivers/gpu/drm/i915/gvt/execlist.c
+++ b/drivers/gpu/drm/i915/gvt/execlist.c
@@ -163,7 +163,7 @@ static void emulate_csb_update(struct intel_vgpu_execlist *execlist,
 					       hwsp_gpa + I915_HWS_CSB_BUF0_INDEX * 4 + write_pointer * 8,
 					       status, 8);
 		intel_gvt_hypervisor_write_gpa(vgpu,
-					       hwsp_gpa + intel_hws_csb_write_index(execlist->engine->i915) * 4,
+					       hwsp_gpa + INTEL_HWS_CSB_WRITE_INDEX(execlist->engine->i915) * 4,
 					       &write_pointer, 4);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ac0fcb9a4ca..b91d7209396f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1701,14 +1701,6 @@ mkwrite_device_info(struct drm_i915_private *dev_priv)
 	return (struct intel_device_info *)INTEL_INFO(dev_priv);
 }
 
-static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)
-{
-	if (GRAPHICS_VER(i915) >= 11)
-		return ICL_HWS_CSB_WRITE_INDEX;
-	else
-		return I915_HWS_CSB_WRITE_INDEX;
-}
-
 static inline enum i915_map_type
 i915_coherent_map_type(struct drm_i915_private *i915,
 		       struct drm_i915_gem_object *obj, bool always_coherent)
-- 
2.30.2