Re: [Intel-gfx] [PATCH 6/7] drm/i915/perf: add interrupt enabling parameter

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 10/03/2020 22:08, Umesh Nerlige Ramappa wrote:
> On Tue, Mar 03, 2020 at 02:19:04PM -0800, Umesh Nerlige Ramappa wrote:
>> From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>
>> This let's the application choose to be driven by the interrupt
>> mechanism of the HW. In conjuction with long periods for checks for
>> the availability of data on the CPU, this can reduce the CPU load when
>> doing capture of OA data.
>>
>> v2: Version the new parameter (Joonas)
>> v3: Rebase (Umesh)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_perf.c | 58 +++++++++++++++++++++++---------
>> include/uapi/drm/i915_drm.h      | 10 ++++++
>> 2 files changed, 53 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c 
>> b/drivers/gpu/drm/i915/i915_perf.c
>> index 502961da840d..ab41cba85b40 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -252,7 +252,7 @@
>>  * oa_buffer_check().
>>  *
>>  * Most of the implementation details for this workaround are in
>> - * oa_buffer_check_unlocked() and _append_oa_reports()
>> + * oa_buffer_check() and _append_oa_reports()
>>  *
>>  * Note for posterity: previously the driver used to define an 
>> effective tail
>>  * pointer that lagged the real pointer by a 'tail margin' measured 
>> in bytes
>> @@ -447,8 +447,9 @@ static u32 gen7_oa_hw_tail_read(struct 
>> i915_perf_stream *stream)
>> }
>>
>> /**
>> - * oa_buffer_check_unlocked - check for data and update tail ptr state
>> + * oa_buffer_check - check for data and update tail ptr state
>>  * @stream: i915 stream instance
>> + * @lock: whether to take the oa_buffer spin lock
>>  *
>>  * This is either called via fops (for blocking reads in user ctx) or 
>> the poll
>>  * check hrtimer (atomic ctx) to check the OA buffer tail pointer and 
>> check
>> @@ -470,8 +471,9 @@ static u32 gen7_oa_hw_tail_read(struct 
>> i915_perf_stream *stream)
>>  *
>>  * Returns: %true if the OA buffer contains data, else %false
>>  */
>> -static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
>> +static bool oa_buffer_check(struct i915_perf_stream *stream, bool lock)
>
> Hi Lionel,
>
> All callers seem to set the lock to true when calling 
> oa_buffer_check().  Do you recall why the parameter was introduced?  
> If not, we probably want to remove this change.
>
> Thanks,
> Umesh


Err... Sorry, I don't remember.

It's probably a leftover the initial iteration where I was trying to get 
the OA head/tail register from the interrupt.

I guess you can drop that param and leave the function with the 
_unlocked prefix.


Thanks,


-Lionel


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx