diff mbox series

[3/3] drm/i915/perf: Initialize gen12 OA buffer unconditionally

Message ID 20230909011626.1643734-4-ashutosh.dixit@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/perf: A few fixes and enhancements | expand

Commit Message

Dixit, Ashutosh Sept. 9, 2023, 1:16 a.m. UTC 
From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Correct values for OAR counters are still dependent on enabling the
GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
bit means OAG unit will write reports to the OAG buffer, so
initialize the OAG buffer unconditionally for all use cases.

BSpec: 46822

Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Dixit, Ashutosh Sept. 9, 2023, 1:24 a.m. UTC | #1 
On Fri, 08 Sep 2023 18:16:26 -0700, Ashutosh Dixit wrote:
>

Hi Umesh,

> From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>
> Correct values for OAR counters are still dependent on enabling the
> GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
> bit means OAG unit will write reports to the OAG buffer, so
> initialize the OAG buffer unconditionally for all use cases.
>
> BSpec: 46822
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_perf.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 1347e4ec9dd5a..30cf37d6e79be 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -3032,12 +3032,12 @@ static void gen12_oa_enable(struct i915_perf_stream *stream)
>	u32 val;
>
>	/*
> -	 * If we don't want OA reports from the OA buffer, then we don't even
> -	 * need to program the OAG unit.
> +	 * BSpec: 46822
> +	 * Correct values for OAR counters are still dependent on enabling the
> +	 * GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
> +	 * bit means OAG unit will write reports to the OAG buffer, so
> +	 * initialize the OAG buffer correctly.
>	 */
> -	if (!(stream->sample_flags & SAMPLE_OA_REPORT))
> -		return;
> -
>	gen12_init_oa_buffer(stream);
>
>	regs = __oa_regs(stream);

Looks like this should be needed, I can R-b it.

However, gen12_test_mi_rpc IGT says:

	/* OA unit configuration:
	 * DRM_I915_PERF_PROP_SAMPLE_OA is no longer required for Gen12
	 * because the OAR unit increments counters only for the
	 * relevant context. No other parameters are needed since we do
	 * not rely on the OA buffer anymore to normalize the counter
	 * values.
	 */

So gen12_test_mi_rpc doesn't set DRM_I915_PERF_PROP_SAMPLE_OA and also
seems to be passing in CI (don't see it but there seem to be no open
bugs). Thoughts?

Thanks.
--
Ashutosh
Umesh Nerlige Ramappa Sept. 13, 2023, 1:46 a.m. UTC | #2 
On Fri, Sep 08, 2023 at 06:24:16PM -0700, Dixit, Ashutosh wrote:
>On Fri, 08 Sep 2023 18:16:26 -0700, Ashutosh Dixit wrote:
>>
>
>Hi Umesh,
>
>> From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>
>> Correct values for OAR counters are still dependent on enabling the
>> GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
>> bit means OAG unit will write reports to the OAG buffer, so
>> initialize the OAG buffer unconditionally for all use cases.
>>
>> BSpec: 46822
>>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_perf.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 1347e4ec9dd5a..30cf37d6e79be 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -3032,12 +3032,12 @@ static void gen12_oa_enable(struct i915_perf_stream *stream)
>>	u32 val;
>>
>>	/*
>> -	 * If we don't want OA reports from the OA buffer, then we don't even
>> -	 * need to program the OAG unit.
>> +	 * BSpec: 46822
>> +	 * Correct values for OAR counters are still dependent on enabling the
>> +	 * GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
>> +	 * bit means OAG unit will write reports to the OAG buffer, so
>> +	 * initialize the OAG buffer correctly.
>>	 */
>> -	if (!(stream->sample_flags & SAMPLE_OA_REPORT))
>> -		return;
>> -
>>	gen12_init_oa_buffer(stream);
>>
>>	regs = __oa_regs(stream);
>
>Looks like this should be needed, I can R-b it.
>
>However, gen12_test_mi_rpc IGT says:
>
>	/* OA unit configuration:
>	 * DRM_I915_PERF_PROP_SAMPLE_OA is no longer required for Gen12
>	 * because the OAR unit increments counters only for the
>	 * relevant context. No other parameters are needed since we do
>	 * not rely on the OA buffer anymore to normalize the counter
>	 * values.
>	 */

That's wrong. When TGL support was added, this was misunderstood and I 
removed the OAR-OAG dependency. Ideally we should enforce user to pass 
SAMPLE_OA always, but now that will break uabi.

For for the OAR case, let's just enable OAG unconditionally so that the 
OAR counters tick correctly. While we do that, we should disable all 
events that trigger a report into the OA buffer. In addition, I would 
also allocate the smallest OA buffer size for this case, so that memory 
impact is low.

Needs a Fixes tag with the commit that enabled OA for TGL.

Regards,
Umesh

>
>So gen12_test_mi_rpc doesn't set DRM_I915_PERF_PROP_SAMPLE_OA and also
>seems to be passing in CI (don't see it but there seem to be no open
>bugs). Thoughts?
>
>Thanks.
>--
>Ashutosh
Dixit, Ashutosh Sept. 13, 2023, 4:59 p.m. UTC | #3 
On Tue, 12 Sep 2023 18:46:12 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Fri, Sep 08, 2023 at 06:24:16PM -0700, Dixit, Ashutosh wrote:
> > On Fri, 08 Sep 2023 18:16:26 -0700, Ashutosh Dixit wrote:
> >>
> >
> >> From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >>
> >> Correct values for OAR counters are still dependent on enabling the
> >> GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
> >> bit means OAG unit will write reports to the OAG buffer, so
> >> initialize the OAG buffer unconditionally for all use cases.
> >>
> >> BSpec: 46822
> >>
> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_perf.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> >> index 1347e4ec9dd5a..30cf37d6e79be 100644
> >> --- a/drivers/gpu/drm/i915/i915_perf.c
> >> +++ b/drivers/gpu/drm/i915/i915_perf.c
> >> @@ -3032,12 +3032,12 @@ static void gen12_oa_enable(struct i915_perf_stream *stream)
> >>	u32 val;
> >>
> >>	/*
> >> -	 * If we don't want OA reports from the OA buffer, then we don't even
> >> -	 * need to program the OAG unit.
> >> +	 * BSpec: 46822
> >> +	 * Correct values for OAR counters are still dependent on enabling the
> >> +	 * GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
> >> +	 * bit means OAG unit will write reports to the OAG buffer, so
> >> +	 * initialize the OAG buffer correctly.
> >>	 */
> >> -	if (!(stream->sample_flags & SAMPLE_OA_REPORT))
> >> -		return;
> >> -
> >>	gen12_init_oa_buffer(stream);
> >>
> >>	regs = __oa_regs(stream);
> >
> > Looks like this should be needed, I can R-b it.
> >
> > However, gen12_test_mi_rpc IGT says:
> >
> >	/* OA unit configuration:
> >	 * DRM_I915_PERF_PROP_SAMPLE_OA is no longer required for Gen12
> >	 * because the OAR unit increments counters only for the
> >	 * relevant context. No other parameters are needed since we do
> >	 * not rely on the OA buffer anymore to normalize the counter
> >	 * values.
> >	 */
>
> That's wrong. When TGL support was added, this was misunderstood and I
> removed the OAR-OAG dependency. Ideally we should enforce user to pass
> SAMPLE_OA always, but now that will break uabi.

SAMPLE_OA has other effects like enabling the timer etc. Also, if we
enforce user to always pass it, we can do it in the kernel itself and
ignore the uapi parameter.

>
> For for the OAR case, let's just enable OAG unconditionally so that the OAR
> counters tick correctly. While we do that, we should disable all events
> that trigger a report into the OA buffer. In addition, I would also
> allocate the smallest OA buffer size for this case, so that memory impact
> is low.
>
> Needs a Fixes tag with the commit that enabled OA for TGL.

If you think this patch is needed, I think it's better you do this so that
you can stay the patch author and I can be the reviewer, otherwise we'll
need to go hunt for another reviewer.

Thanks.
--
Ashutosh


> >
> > So gen12_test_mi_rpc doesn't set DRM_I915_PERF_PROP_SAMPLE_OA and also
> > seems to be passing in CI (don't see it but there seem to be no open
> > bugs). Thoughts?
> >
> > Thanks.
> > --
> > Ashutosh
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 1347e4ec9dd5a..30cf37d6e79be 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -3032,12 +3032,12 @@  static void gen12_oa_enable(struct i915_perf_stream *stream)
 	u32 val;
 
 	/*
-	 * If we don't want OA reports from the OA buffer, then we don't even
-	 * need to program the OAG unit.
+	 * BSpec: 46822
+	 * Correct values for OAR counters are still dependent on enabling the
+	 * GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
+	 * bit means OAG unit will write reports to the OAG buffer, so
+	 * initialize the OAG buffer correctly.
 	 */
-	if (!(stream->sample_flags & SAMPLE_OA_REPORT))
-		return;
-
 	gen12_init_oa_buffer(stream);
 
 	regs = __oa_regs(stream);