| Message ID | 20230522201749.4094742-1-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/perf: Clear out entire reports after reading if not power of 2 size | expand |
On Mon, May 22, 2023 at 01:17:49PM -0700, Ashutosh Dixit wrote: >Clearing out report id and timestamp as means to detect unlanded reports >only works if report size is power of 2. That is, only when report size is >a sub-multiple of the OA buffer size can we be certain that reports will >land at the same place each time in the OA buffer (after rewind). If report >size is not a power of 2, we need to zero out the entire report to be able >to detect unlanded reports reliably. > >Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >--- > drivers/gpu/drm/i915/i915_perf.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >index 19d5652300eeb..58284156428dc 100644 >--- a/drivers/gpu/drm/i915/i915_perf.c >+++ b/drivers/gpu/drm/i915/i915_perf.c >@@ -877,12 +877,17 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, > stream->oa_buffer.last_ctx_id = ctx_id; > } > >- /* >- * Clear out the report id and timestamp as a means to detect unlanded >- * reports. >- */ >- oa_report_id_clear(stream, report32); >- oa_timestamp_clear(stream, report32); >+ if (is_power_of_2(report_size)) { >+ /* >+ * Clear out the report id and timestamp as a means >+ * to detect unlanded reports. >+ */ >+ oa_report_id_clear(stream, report32); >+ oa_timestamp_clear(stream, report32); >+ } else { >+ /* Zero out the entire report */ >+ memset(report32, 0, report_size); Indeed, this was a bug. For a minute, I started wondering if this is the issue I am running into with the other patch posted for DG2, but then I see the issue within the first fill of the OA buffer where chunks of the reports are zeroed out, so this is a new issue. lgtm, Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Thanks, Umesh >+ } > } > > if (start_offset != *offset) { >-- >2.38.0 >
On Mon, 22 May 2023 14:34:18 -0700, Umesh Nerlige Ramappa wrote: > > On Mon, May 22, 2023 at 01:17:49PM -0700, Ashutosh Dixit wrote: > > Clearing out report id and timestamp as means to detect unlanded reports > > only works if report size is power of 2. That is, only when report size is > > a sub-multiple of the OA buffer size can we be certain that reports will > > land at the same place each time in the OA buffer (after rewind). If report > > size is not a power of 2, we need to zero out the entire report to be able > > to detect unlanded reports reliably. > > > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/i915_perf.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > > index 19d5652300eeb..58284156428dc 100644 > > --- a/drivers/gpu/drm/i915/i915_perf.c > > +++ b/drivers/gpu/drm/i915/i915_perf.c > > @@ -877,12 +877,17 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, > > stream->oa_buffer.last_ctx_id = ctx_id; > > } > > > > - /* > > - * Clear out the report id and timestamp as a means to detect unlanded > > - * reports. > > - */ > > - oa_report_id_clear(stream, report32); > > - oa_timestamp_clear(stream, report32); > > + if (is_power_of_2(report_size)) { > > + /* > > + * Clear out the report id and timestamp as a means > > + * to detect unlanded reports. > > + */ > > + oa_report_id_clear(stream, report32); > > + oa_timestamp_clear(stream, report32); > > + } else { > > + /* Zero out the entire report */ > > + memset(report32, 0, report_size); > > Indeed, this was a bug. For a minute, I started wondering if this is the > issue I am running into with the other patch posted for DG2, but then I see > the issue within the first fill of the OA buffer where chunks of the > reports are zeroed out, so this is a new issue. Yes I saw this while reviewing your patch. And also I thought your issue was happening on DG2 with power of 2 report size, only on MTL OAM we introduce non power of 2 report size. > lgtm, > > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Thanks. -- Ashutosh > > > + } > > } > > > > if (start_offset != *offset) { > > -- > > 2.38.0 > >
On 22/05/2023 23:17, Ashutosh Dixit wrote: > Clearing out report id and timestamp as means to detect unlanded reports > only works if report size is power of 2. That is, only when report size is > a sub-multiple of the OA buffer size can we be certain that reports will > land at the same place each time in the OA buffer (after rewind). If report > size is not a power of 2, we need to zero out the entire report to be able > to detect unlanded reports reliably. > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Sad but necessary unfortunately.... Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_perf.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index 19d5652300eeb..58284156428dc 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -877,12 +877,17 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, > stream->oa_buffer.last_ctx_id = ctx_id; > } > > - /* > - * Clear out the report id and timestamp as a means to detect unlanded > - * reports. > - */ > - oa_report_id_clear(stream, report32); > - oa_timestamp_clear(stream, report32); > + if (is_power_of_2(report_size)) { > + /* > + * Clear out the report id and timestamp as a means > + * to detect unlanded reports. > + */ > + oa_report_id_clear(stream, report32); > + oa_timestamp_clear(stream, report32); > + } else { > + /* Zero out the entire report */ > + memset(report32, 0, report_size); > + } > } > > if (start_offset != *offset) {
On Mon, May 22, 2023 at 02:50:51PM -0700, Dixit, Ashutosh wrote: >On Mon, 22 May 2023 14:34:18 -0700, Umesh Nerlige Ramappa wrote: >> >> On Mon, May 22, 2023 at 01:17:49PM -0700, Ashutosh Dixit wrote: >> > Clearing out report id and timestamp as means to detect unlanded reports >> > only works if report size is power of 2. That is, only when report size is >> > a sub-multiple of the OA buffer size can we be certain that reports will >> > land at the same place each time in the OA buffer (after rewind). If report >> > size is not a power of 2, we need to zero out the entire report to be able >> > to detect unlanded reports reliably. >> > >> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_perf.c | 17 +++++++++++------ >> > 1 file changed, 11 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >> > index 19d5652300eeb..58284156428dc 100644 >> > --- a/drivers/gpu/drm/i915/i915_perf.c >> > +++ b/drivers/gpu/drm/i915/i915_perf.c >> > @@ -877,12 +877,17 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, >> > stream->oa_buffer.last_ctx_id = ctx_id; >> > } >> > >> > - /* >> > - * Clear out the report id and timestamp as a means to detect unlanded >> > - * reports. >> > - */ >> > - oa_report_id_clear(stream, report32); >> > - oa_timestamp_clear(stream, report32); >> > + if (is_power_of_2(report_size)) { >> > + /* >> > + * Clear out the report id and timestamp as a means >> > + * to detect unlanded reports. >> > + */ >> > + oa_report_id_clear(stream, report32); >> > + oa_timestamp_clear(stream, report32); >> > + } else { >> > + /* Zero out the entire report */ >> > + memset(report32, 0, report_size); >> >> Indeed, this was a bug. For a minute, I started wondering if this is the >> issue I am running into with the other patch posted for DG2, but then I see >> the issue within the first fill of the OA buffer where chunks of the >> reports are zeroed out, so this is a new issue. > >Yes I saw this while reviewing your patch. And also I thought your issue >was happening on DG2 with power of 2 report size, only on MTL OAM we >introduce non power of 2 report size. > >> lgtm, >> >> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Maybe this should include Fixes: tag pointing to the patch that introduced the OAM non-power-of-2 format. Umesh > >Thanks. >-- >Ashutosh > >> >> > + } >> > } >> > >> > if (start_offset != *offset) { >> > -- >> > 2.38.0 >> >
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 19d5652300eeb..58284156428dc 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -877,12 +877,17 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream, stream->oa_buffer.last_ctx_id = ctx_id; } - /* - * Clear out the report id and timestamp as a means to detect unlanded - * reports. - */ - oa_report_id_clear(stream, report32); - oa_timestamp_clear(stream, report32); + if (is_power_of_2(report_size)) { + /* + * Clear out the report id and timestamp as a means + * to detect unlanded reports. + */ + oa_report_id_clear(stream, report32); + oa_timestamp_clear(stream, report32); + } else { + /* Zero out the entire report */ + memset(report32, 0, report_size); + } } if (start_offset != *offset) {
Clearing out report id and timestamp as means to detect unlanded reports only works if report size is power of 2. That is, only when report size is a sub-multiple of the OA buffer size can we be certain that reports will land at the same place each time in the OA buffer (after rewind). If report size is not a power of 2, we need to zero out the entire report to be able to detect unlanded reports reliably. Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)