Re: [PATCH 2/3] drm/i915/perf: Add support for report sizes that are not power of 2

[Ashutosh Dixit <ashutosh.dixit@intel.com>]

On Tue, 17 Sep 2019 10:33:51 -0700, Umesh Nerlige Ramappa wrote:
>
> On Mon, Sep 16, 2019 at 09:11:58PM -0700, Ashutosh Dixit wrote:
> > On Mon, 16 Sep 2019 12:17:54 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >> On Sun, Sep 15, 2019 at 02:24:41PM +0300, Lionel Landwerlin wrote:
> >> > On 14/09/2019 02:06, Umesh Nerlige Ramappa wrote:
> >> >> OA perf unit supports non-power of 2 report sizes. Enable support for
> >> >> these sizes in the driver.
> >> >>
> >> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_perf.c | 59 ++++++++++++--------------------
> >> >>  1 file changed, 21 insertions(+), 38 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> >> >> index 50b6d154fd46..482fca3da7de 100644
> >> >> --- a/drivers/gpu/drm/i915/i915_perf.c
> >> >> +++ b/drivers/gpu/drm/i915/i915_perf.c
> >> >> @@ -450,7 +450,7 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
> >> >>	u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma);
> >> >>	int report_size = stream->oa_buffer.format_size;
> >> >>	unsigned long flags;
> >> >> -	u32 hw_tail;
> >> >> +	u32 hw_tail, aging_tail;
> >> >>	u64 now;
> >> >>	/* We have to consider the (unlikely) possibility that read() errors
> >> >> @@ -459,16 +459,17 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
> >> >>	 */
> >> >>	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
> >> >> -	hw_tail = dev_priv->perf.ops.oa_hw_tail_read(stream);
> >> >> +	hw_tail = dev_priv->perf.ops.oa_hw_tail_read(stream) - gtt_offset;
> >> >> +	aging_tail = stream->oa_buffer.aging_tail - gtt_offset;
> >> >>	/* The tail pointer increases in 64 byte increments,
> >> >>	 * not in report_size steps...
> >> >>	 */
> >> >> -	hw_tail &= ~(report_size - 1);
> >> >> +	hw_tail = OA_TAKEN(hw_tail, (OA_TAKEN(hw_tail, aging_tail) % report_size));
> >> >
> >> >
> >> > I'm struggling to parse this line above and I'm not 100% sure it's correct.
> >> >
> >> > Could add a comment to explain what is going on?
> >>
> >> The aging tail is always pointing to the boundary of a report whereas
> >> the hw_tail is advancing in 64 byte increments.
> >>
> >> The innermost OA_TAKEN is returning the number of bytes between the
> >> hw_tail and the aging_tail. The modulo is getting the size of the
> >> partial report (if any).
> >>
> >> The outermost OA_TAKEN is subtracting the size of partial report from
> >> the hw_tail to get a hw_tail that points to the boundary of the last
> >> full report.
> >>
> >> The value of hw_tail would be the same as from the deleted line of code
> >> above this line.
> >
> > Looks like aging_tail should not be needed (it is complicating the
> > expression and is not there in the original expression). All we need is a
> > "circular modulus". For example would the following work?
>
> original expression assumes all report sizes are powers of 2 and hence does
> not need a reference (like aging_tail) to rounddown the hw_tail.
>
> >
> >    if (hw_tail < report_size)
> >        hw_tail += OA_BUFFER_SIZE;
>
> Assuming that this condition is detecting a report split across the OA
> buffer boundary, the above check will not catch the split in cases where
> there are multiple reports generated before we read the hw_tail.
>
> >    hw_tail = rounddown(hw_tail, report_size);
> >
> > Another way (if we want to avoid the division) would be to maintain a
> > cached copy of hw_tail in SW which is successively (and circularly)
> > incremented by report_size till it catches up with hw_tail read from
> > HW. But probably the first method above is simpler.
>
> aging_tail is a cached copy of the hw_tail that advances only in report
> size increments.

Umesh is right, the previous aging_tail needs to be taken into
account. Basically we need to start from the previous aging_tail and
continue incrementing by report_size till we catch up with the new hw_tail
(at the previous report_size boundary, which gives the value of the new
aging_tail).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx