diff mbox series

[4/5] drm/i915/guc: Capture list clean up - 3

Message ID 20230406222617.790484-5-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series Improvements to GuC error capture list processing | expand

Commit Message

John Harrison April 6, 2023, 10:26 p.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

Fix Xe_LP name.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 .../gpu/drm/i915/gt/uc/intel_guc_capture.c    | 44 +++++++++----------
 1 file changed, 21 insertions(+), 23 deletions(-)

Comments

Alan Previn April 25, 2023, 7:05 p.m. UTC | #1 
On Thu, 2023-04-06 at 15:26 -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Fix Xe_LP name.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
alan:snip


> -/* GEN9/XE_LPD - Render / Compute Per-Engine-Instance */
> +/* GEN8+ Render / Compute Per-Engine-Instance */
alan: two comments on this:
1. shouldnt this go with the earlier patch?
2. i agree with renaming the names of the register to reflect the when
   all of those registers got first introduced... however, considering
   we only support GuC on Gen12 and beyond (we do have select CI-tests
   that enable GuC on Gen9 but not on Gen8 and before), should we also
   change the comments? I think the comment should reflect the usage
   not just follow the same name of the registe #define - else why even
   add the comments. (please apply this same comment for gen8_vd_inst_regs,
   gen8_vec_inst_regs and gen8_blt_inst_regs).
alternatively, we could keep those GEN8+ comments above the list but maybe
add just one comment in the default list - see below.

alan: snip

> @@ -366,7 +364,7 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
>  	const struct __guc_mmio_reg_descr_group *lists;
>  
>  	if (GRAPHICS_VER(i915) >= 12)
> -		lists = xe_lpd_lists;
> +		lists = xe_lp_lists;
>  	else
>  		lists = default_lists;
alan: perhaps add a comment that we really don't support any of this
on anything below Gen9?

>
John Harrison April 26, 2023, 5:29 p.m. UTC | #2 
On 4/25/2023 12:05, Teres Alexis, Alan Previn wrote:
> On Thu, 2023-04-06 at 15:26 -0700,John.C.Harrison@Intel.com  wrote:
>> From: John Harrison<John.C.Harrison@Intel.com>
>>
>> Fix Xe_LP name.
>>
>> Signed-off-by: John Harrison<John.C.Harrison@Intel.com>
> alan:snip
>
>
>> -/* GEN9/XE_LPD - Render / Compute Per-Engine-Instance */
>> +/* GEN8+ Render / Compute Per-Engine-Instance */
> alan: two comments on this:
> 1. shouldnt this go with the earlier patch?
See comment in cover letter:

    NB: The changes are being sent as multiple patches to make code review
    simpler. However, before merging it may be better to squash into a
    single patch, especially if it going to be sent with a 'fixes' tag.



> 2. i agree with renaming the names of the register to reflect the when
>     all of those registers got first introduced... however, considering
>     we only support GuC on Gen12 and beyond (we do have select CI-tests
>     that enable GuC on Gen9 but not on Gen8 and before), should we also
>     change the comments? I think the comment should reflect the usage
>     not just follow the same name of the registe #define - else why even
>     add the comments. (please apply this same comment for gen8_vd_inst_regs,
>     gen8_vec_inst_regs and gen8_blt_inst_regs).
> alternatively, we could keep those GEN8+ comments above the list but maybe
> add just one comment in the default list - see below.
Because at some point we might want to start supporting other platforms. 
My view is that the comment should be accurate. These registers exist on 
Gen8+. So if you are building a register list for a Gen8 or later 
device, they can/should be included.

> alan: snip
>
>> @@ -366,7 +364,7 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
>>   	const struct __guc_mmio_reg_descr_group *lists;
>>   
>>   	if (GRAPHICS_VER(i915) >= 12)
>> -		lists = xe_lpd_lists;
>> +		lists = xe_lp_lists;
>>   	else
>>   		lists = default_lists;
> alan: perhaps add a comment that we really don't support any of this
> on anything below Gen9?
It wasn't me that named it 'default_' rather than gen9_. I could add yet 
another rename of s/default_/gen9_/g...

John.

>
>>
John Harrison April 26, 2023, 5:37 p.m. UTC | #3 
On 4/26/2023 10:29, John Harrison wrote:
> On 4/25/2023 12:05, Teres Alexis, Alan Previn wrote:
>> On Thu, 2023-04-06 at 15:26 -0700,John.C.Harrison@Intel.com  wrote:
>>> From: John Harrison<John.C.Harrison@Intel.com>
>>>
>>> Fix Xe_LP name.
>>>
>>> Signed-off-by: John Harrison<John.C.Harrison@Intel.com>
>> alan:snip
>>
>>
>>> -/* GEN9/XE_LPD - Render / Compute Per-Engine-Instance */
>>> +/* GEN8+ Render / Compute Per-Engine-Instance */
>> alan: two comments on this:
>> 1. shouldnt this go with the earlier patch?
> See comment in cover letter:
>
>     NB: The changes are being sent as multiple patches to make code review
>     simpler. However, before merging it may be better to squash into a
>     single patch, especially if it going to be sent with a 'fixes' tag.
>
>
>
>> 2. i agree with renaming the names of the register to reflect the when
>>     all of those registers got first introduced... however, considering
>>     we only support GuC on Gen12 and beyond (we do have select CI-tests
>>     that enable GuC on Gen9 but not on Gen8 and before), should we also
>>     change the comments? I think the comment should reflect the usage
>>     not just follow the same name of the registe #define - else why even
>>     add the comments. (please apply this same comment for gen8_vd_inst_regs,
>>     gen8_vec_inst_regs and gen8_blt_inst_regs).
>> alternatively, we could keep those GEN8+ comments above the list but maybe
>> add just one comment in the default list - see below.
> Because at some point we might want to start supporting other 
> platforms. My view is that the comment should be accurate. These 
> registers exist on Gen8+. So if you are building a register list for a 
> Gen8 or later device, they can/should be included.
>
>> alan: snip
>>
>>> @@ -366,7 +364,7 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
>>>   	const struct __guc_mmio_reg_descr_group *lists;
>>>   
>>>   	if (GRAPHICS_VER(i915) >= 12)
>>> -		lists = xe_lpd_lists;
>>> +		lists = xe_lp_lists;
>>>   	else
>>>   		lists = default_lists;
>> alan: perhaps add a comment that we really don't support any of this
>> on anything below Gen9?
> It wasn't me that named it 'default_' rather than gen9_. I could add 
> yet another rename of s/default_/gen9_/g...
>
> John.
Although looking at the lists, there is nothing gen9 specific anywhere. 
So gen8_ would be the more accurate name.

John.

>
>>>   
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index 7968a495fcfa8..fbd0be4afc6d5 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -96,47 +96,47 @@ 
 	{ GEN12_SFC_DONE(2),        0,      0, "SFC_DONE[2]" }, \
 	{ GEN12_SFC_DONE(3),        0,      0, "SFC_DONE[3]" }
 
-/* XE_LPD - Global */
-static const struct __guc_mmio_reg_descr xe_lpd_global_regs[] = {
+/* XE_LP Global */
+static const struct __guc_mmio_reg_descr xe_lp_global_regs[] = {
 	COMMON_BASE_GLOBAL,
 	COMMON_GEN9BASE_GLOBAL,
 	COMMON_GEN12BASE_GLOBAL,
 };
 
-/* XE_LPD - Render / Compute Per-Class */
-static const struct __guc_mmio_reg_descr xe_lpd_rc_class_regs[] = {
+/* XE_LP Render / Compute Per-Class */
+static const struct __guc_mmio_reg_descr xe_lp_rc_class_regs[] = {
 	COMMON_BASE_HAS_EU,
 	COMMON_BASE_RENDER,
 	COMMON_GEN12BASE_RENDER,
 };
 
-/* GEN9/XE_LPD - Render / Compute Per-Engine-Instance */
+/* GEN8+ Render / Compute Per-Engine-Instance */
 static const struct __guc_mmio_reg_descr gen8_rc_inst_regs[] = {
 	COMMON_BASE_ENGINE_INSTANCE,
 };
 
-/* GEN9/XE_LPD - Media Decode/Encode Per-Engine-Instance */
+/* GEN8+ Media Decode/Encode Per-Engine-Instance */
 static const struct __guc_mmio_reg_descr gen8_vd_inst_regs[] = {
 	COMMON_BASE_ENGINE_INSTANCE,
 };
 
-/* XE_LPD - Video Enhancement Per-Class */
-static const struct __guc_mmio_reg_descr xe_lpd_vec_class_regs[] = {
+/* XE_LP Video Enhancement Per-Class */
+static const struct __guc_mmio_reg_descr xe_lp_vec_class_regs[] = {
 	COMMON_GEN12BASE_VEC,
 };
 
-/* GEN9/XE_LPD - Video Enhancement Per-Engine-Instance */
+/* GEN8+ Video Enhancement Per-Engine-Instance */
 static const struct __guc_mmio_reg_descr gen8_vec_inst_regs[] = {
 	COMMON_BASE_ENGINE_INSTANCE,
 };
 
-/* GEN9/XE_LPD - Blitter Per-Engine-Instance */
+/* GEN8+ Blitter Per-Engine-Instance */
 static const struct __guc_mmio_reg_descr gen8_blt_inst_regs[] = {
 	COMMON_BASE_ENGINE_INSTANCE,
 };
 
-/* XE_LPD - GSC Per-Engine-Instance */
-static const struct __guc_mmio_reg_descr xe_lpd_gsc_inst_regs[] = {
+/* XE_LP - GSC Per-Engine-Instance */
+static const struct __guc_mmio_reg_descr xe_lp_gsc_inst_regs[] = {
 	COMMON_BASE_ENGINE_INSTANCE,
 };
 
@@ -153,10 +153,8 @@  static const struct __guc_mmio_reg_descr default_rc_class_regs[] = {
 };
 
 /*
- * Empty lists:
- * GEN9/XE_LPD - Blitter Per-Class
- * GEN9/XE_LPD - Media Decode/Encode Per-Class
- * GEN9 - VEC Class
+ * Empty list to prevent warnings about unknown class/instance types
+ * as not all class/instanace types have entries on all platforms.
  */
 static const struct __guc_mmio_reg_descr empty_regs_list[] = {
 };
@@ -191,20 +189,20 @@  static const struct __guc_mmio_reg_descr_group default_lists[] = {
 	{}
 };
 
-static const struct __guc_mmio_reg_descr_group xe_lpd_lists[] = {
-	MAKE_REGLIST(xe_lpd_global_regs, PF, GLOBAL, 0),
-	MAKE_REGLIST(xe_lpd_rc_class_regs, PF, ENGINE_CLASS, GUC_RENDER_CLASS),
+static const struct __guc_mmio_reg_descr_group xe_lp_lists[] = {
+	MAKE_REGLIST(xe_lp_global_regs, PF, GLOBAL, 0),
+	MAKE_REGLIST(xe_lp_rc_class_regs, PF, ENGINE_CLASS, GUC_RENDER_CLASS),
 	MAKE_REGLIST(gen8_rc_inst_regs, PF, ENGINE_INSTANCE, GUC_RENDER_CLASS),
-	MAKE_REGLIST(xe_lpd_rc_class_regs, PF, ENGINE_CLASS, GUC_COMPUTE_CLASS),
+	MAKE_REGLIST(xe_lp_rc_class_regs, PF, ENGINE_CLASS, GUC_COMPUTE_CLASS),
 	MAKE_REGLIST(gen8_rc_inst_regs, PF, ENGINE_INSTANCE, GUC_COMPUTE_CLASS),
 	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_VIDEO_CLASS),
 	MAKE_REGLIST(gen8_vd_inst_regs, PF, ENGINE_INSTANCE, GUC_VIDEO_CLASS),
-	MAKE_REGLIST(xe_lpd_vec_class_regs, PF, ENGINE_CLASS, GUC_VIDEOENHANCE_CLASS),
+	MAKE_REGLIST(xe_lp_vec_class_regs, PF, ENGINE_CLASS, GUC_VIDEOENHANCE_CLASS),
 	MAKE_REGLIST(gen8_vec_inst_regs, PF, ENGINE_INSTANCE, GUC_VIDEOENHANCE_CLASS),
 	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_BLITTER_CLASS),
 	MAKE_REGLIST(gen8_blt_inst_regs, PF, ENGINE_INSTANCE, GUC_BLITTER_CLASS),
 	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_GSC_OTHER_CLASS),
-	MAKE_REGLIST(xe_lpd_gsc_inst_regs, PF, ENGINE_INSTANCE, GUC_GSC_OTHER_CLASS),
+	MAKE_REGLIST(xe_lp_gsc_inst_regs, PF, ENGINE_INSTANCE, GUC_GSC_OTHER_CLASS),
 	{}
 };
 
@@ -366,7 +364,7 @@  guc_capture_get_device_reglist(struct intel_guc *guc)
 	const struct __guc_mmio_reg_descr_group *lists;
 
 	if (GRAPHICS_VER(i915) >= 12)
-		lists = xe_lpd_lists;
+		lists = xe_lp_lists;
 	else
 		lists = default_lists;