diff mbox series

i915/pmu: Move execlist stats initialization to execlist specific setup

Message ID 20230912212247.1828681-1-umesh.nerlige.ramappa@intel.com (mailing list archive)
State New, archived
Headers show
Series i915/pmu: Move execlist stats initialization to execlist specific setup | expand

Commit Message

Umesh Nerlige Ramappa Sept. 12, 2023, 9:22 p.m. UTC 
engine->stats is a union of execlist and guc stat objects. When execlist
specific fields are initialized, the initial state of guc stats is
affected. This results in bad busyness values when using GuC mode. Move
the execlist initialization from common code to execlist specific code.

Fixes: 77cdd054dd2c ("drm/i915/pmu: Connect engine busyness stats from GuC to pmu")
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c            | 1 -
 drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Alan Previn Sept. 13, 2023, 8:50 p.m. UTC | #1 
I went up the call stack to ensure the differences between the
old and new location isnt skipping over other functions that may reference
something engine related (that may also end up triggering stats variabls).

Without digging further, i see the old postion here:
i915_driver_probe -> i915_driver_mmio_probe -> intel_gt_init_mmio -> intel_engines_init_mmio -> intel_engine_setup
new postion here:
i915_driver_probe -> i915_gem_init -> (for_each_gt) intel_gt_init -> intel_engines_init -> setup (intel_execlists_submission_setup)

And between i915_driver_mmio_probe and i915_gem_init are only mem/ggtt, display and irq related init functions.
That said, LGTM (although you do need to address the BAT failure before merging):

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

On Tue, 2023-09-12 at 14:22 -0700, Umesh Nerlige Ramappa wrote:
> engine->stats is a union of execlist and guc stat objects. When execlist
> specific fields are initialized, the initial state of guc stats is
> affected. This results in bad busyness values when using GuC mode. Move
> the execlist initialization from common code to execlist specific code.
> 
> Fixes: 77cdd054dd2c ("drm/i915/pmu: Connect engine busyness stats from GuC to pmu")
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c            | 1 -
>  drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 2 ++
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index dfb69fc977a0..84a75c95f3f7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -558,7 +558,6 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id,
>  		DRIVER_CAPS(i915)->has_logical_contexts = true;
>  
>  	ewma__engine_latency_init(&engine->latency);
> -	seqcount_init(&engine->stats.execlists.lock);
>  
>  	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 4d05321dc5b5..e8f42ec6b1b4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -3548,6 +3548,8 @@ int intel_execlists_submission_setup(struct intel_engine_cs *engine)
>  	logical_ring_default_vfuncs(engine);
>  	logical_ring_default_irqs(engine);
>  
> +	seqcount_init(&engine->stats.execlists.lock);
> +
>  	if (engine->flags & I915_ENGINE_HAS_RCS_REG_STATE)
>  		rcs_submission_override(engine);
>
Umesh Nerlige Ramappa Sept. 15, 2023, 4:02 p.m. UTC | #2 
On Thu, Sep 14, 2023 at 04:18:34AM +0000, Patchwork wrote:
>   Patch Details
>
>Series:  i915/pmu: Move execlist stats initialization to execlist specific setup
>         (rev2)
>URL:     [1]https://patchwork.freedesktop.org/series/123616/
>State:   failure
>Details: [2]https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123616v2/index.html
>
>     CI Bug Log - changes from CI_DRM_13627_full -> Patchwork_123616v2_full
>
>Summary
>
>   FAILURE
>
>   Serious unknown changes coming with Patchwork_123616v2_full absolutely
>   need to be
>   verified manually.
>
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_123616v2_full, please notify your bug team
>   (lgci.bug.filing@intel.com) to allow them
>   to document this new failure mode, which will reduce false positives in
>   CI.
>
>Participating hosts (9 -> 10)
>
>   Additional (1): shard-tglu0
>
>Possible new issues
>
>   Here are the unknown changes that may have been introduced in
>   Patchwork_123616v2_full:
>
>  IGT changes
>
>    Possible regressions
>
>     * igt@perf_pmu@rc6-all-gts:
>
>          * shard-mtlp: NOTRUN -> [3]ABORT

This is an existing bug and not related to this patch:

https://patchwork.freedesktop.org/series/123616/#rev2

Thanks,
Umesh
>
>    Suppressed
>
>   The following results come from untrusted machines, tests, or statuses.
>   They do not affect the overall result.
>
>     * {igt@kms_feature_discovery@display-4x}:
>
>          * shard-mtlp: NOTRUN -> [4]SKIP
>
>New tests
>
>   New tests have been introduced between CI_DRM_13627_full and
>   Patchwork_123616v2_full:
>
>  New IGT tests (4)
>
>     * igt@kms_atomic_transition@plane-all-transition-nonblocking@pipe-a-hdmi-a-4:
>
>          * Statuses : 1 pass(s)
>          * Exec time: [0.0] s
>
>     * igt@kms_atomic_transition@plane-all-transition-nonblocking@pipe-b-hdmi-a-4:
>
>          * Statuses : 1 pass(s)
>          * Exec time: [0.0] s
>
>     * igt@kms_cursor_crc@cursor-onscreen-128x128@pipe-a-hdmi-a-4:
>
>          * Statuses : 1 pass(s)
>          * Exec time: [0.0] s
>
>     * igt@kms_cursor_crc@cursor-onscreen-128x128@pipe-d-hdmi-a-4:
>
>          * Statuses : 1 pass(s)
>          * Exec time: [0.0] s
>
>Known issues
>
...
Umesh Nerlige Ramappa Sept. 15, 2023, 4:39 p.m. UTC | #3 
On Fri, Sep 15, 2023 at 09:02:05AM -0700, Umesh Nerlige Ramappa wrote:
>On Thu, Sep 14, 2023 at 04:18:34AM +0000, Patchwork wrote:
>>  Patch Details
>>
>>Series:  i915/pmu: Move execlist stats initialization to execlist specific setup
>>        (rev2)
>>URL:     [1]https://patchwork.freedesktop.org/series/123616/
>>State:   failure
>>Details: [2]https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123616v2/index.html
>>
>>    CI Bug Log - changes from CI_DRM_13627_full -> Patchwork_123616v2_full
>>
>>Summary
>>
>>  FAILURE
>>
>>  Serious unknown changes coming with Patchwork_123616v2_full absolutely
>>  need to be
>>  verified manually.
>>
>>  If you think the reported changes have nothing to do with the changes
>>  introduced in Patchwork_123616v2_full, please notify your bug team
>>  (lgci.bug.filing@intel.com) to allow them
>>  to document this new failure mode, which will reduce false positives in
>>  CI.
>>
>>Participating hosts (9 -> 10)
>>
>>  Additional (1): shard-tglu0
>>
>>Possible new issues
>>
>>  Here are the unknown changes that may have been introduced in
>>  Patchwork_123616v2_full:
>>
>> IGT changes
>>
>>   Possible regressions
>>
>>    * igt@perf_pmu@rc6-all-gts:
>>
>>         * shard-mtlp: NOTRUN -> [3]ABORT
>
>This is an existing bug and not related to this patch:
>
>https://patchwork.freedesktop.org/series/123616/#rev2

Bug link: https://gitlab.freedesktop.org/drm/intel/issues/9335
>
>Thanks,
>Umesh
>>
>>   Suppressed
>>
>>  The following results come from untrusted machines, tests, or statuses.
>>  They do not affect the overall result.
>>
>>    * {igt@kms_feature_discovery@display-4x}:
>>
>>         * shard-mtlp: NOTRUN -> [4]SKIP
>>
>>New tests
>>
>>  New tests have been introduced between CI_DRM_13627_full and
>>  Patchwork_123616v2_full:
>>
>> New IGT tests (4)
>>
>>    * igt@kms_atomic_transition@plane-all-transition-nonblocking@pipe-a-hdmi-a-4:
>>
>>         * Statuses : 1 pass(s)
>>         * Exec time: [0.0] s
>>
>>    * igt@kms_atomic_transition@plane-all-transition-nonblocking@pipe-b-hdmi-a-4:
>>
>>         * Statuses : 1 pass(s)
>>         * Exec time: [0.0] s
>>
>>    * igt@kms_cursor_crc@cursor-onscreen-128x128@pipe-a-hdmi-a-4:
>>
>>         * Statuses : 1 pass(s)
>>         * Exec time: [0.0] s
>>
>>    * igt@kms_cursor_crc@cursor-onscreen-128x128@pipe-d-hdmi-a-4:
>>
>>         * Statuses : 1 pass(s)
>>         * Exec time: [0.0] s
>>
>>Known issues
>>
>...
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index dfb69fc977a0..84a75c95f3f7 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -558,7 +558,6 @@  static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id,
 		DRIVER_CAPS(i915)->has_logical_contexts = true;
 
 	ewma__engine_latency_init(&engine->latency);
-	seqcount_init(&engine->stats.execlists.lock);
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 4d05321dc5b5..e8f42ec6b1b4 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -3548,6 +3548,8 @@  int intel_execlists_submission_setup(struct intel_engine_cs *engine)
 	logical_ring_default_vfuncs(engine);
 	logical_ring_default_irqs(engine);
 
+	seqcount_init(&engine->stats.execlists.lock);
+
 	if (engine->flags & I915_ENGINE_HAS_RCS_REG_STATE)
 		rcs_submission_override(engine);