Re: [Intel-gfx] [PATCH 4/4] drm/i915/perf: add new open param to configure polling of OA buffer

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

On Thu, 12 Mar 2020 16:05:02 -0700, Umesh Nerlige Ramappa wrote:
>
> From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>
> This new parameter let's the application choose how often the OA
> buffer should be checked on the CPU side for data availability. Longer
> polling period tend to reduce CPU overhead if the application does not
> care about somewhat real time data collection.

Lionely already has comments on this so skipping those.

> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 21a63644846f..ca139ac31b11 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -262,11 +262,11 @@
>   */
>  #define OA_TAIL_MARGIN_NSEC	100000ULL
>
> -/* frequency for checking whether the OA unit has written new reports to the
> - * circular OA buffer...
> +/* The default frequency for checking whether the OA unit has written new
> + * reports to the circular OA buffer...
>   */
> -#define POLL_FREQUENCY 200
> -#define POLL_PERIOD (NSEC_PER_SEC / POLL_FREQUENCY)
> +#define DEFAULT_POLL_FREQUENCY 200
> +#define DEFAULT_POLL_PERIOD (NSEC_PER_SEC / DEFAULT_POLL_FREQUENCY)

nit, but I mostly like to have "units" as part of the name, so I'd recommed
calling this DEFAULT_POLL_PERIOD_NS.

> @@ -349,6 +349,8 @@ static const struct i915_oa_format gen12_oa_formats[I915_OA_FORMAT_MAX] = {
>   * @oa_periodic: Whether to enable periodic OA unit sampling
>   * @oa_period_exponent: The OA unit sampling period is derived from this
>   * @engine: The engine (typically rcs0) being monitored by the OA unit
> + * @poll_oa_period: The period at which the CPU will check for OA data
> + * availability
>   *
>   * As read_properties_unlocked() enumerates and validates the properties given
>   * to open a stream of metrics the configuration is built up in the structure
> @@ -368,6 +370,7 @@ struct perf_open_properties {
>	int oa_period_exponent;
>
>	struct intel_engine_cs *engine;
> +	u64 poll_oa_period;

poll_oa_period_ns?

> @@ -2642,9 +2645,9 @@ static void i915_oa_stream_enable(struct i915_perf_stream *stream)
>
>	stream->perf->ops.oa_enable(stream);
>
> -	if (stream->periodic)
> +	if (stream->periodic && stream->poll_oa_period)

poll_oa_period check is not required, it's always present.

> @@ -3617,6 +3625,14 @@ static int read_properties_unlocked(struct i915_perf *perf,
>		case DRM_I915_PERF_PROP_HOLD_PREEMPTION:
>			props->hold_preemption = !!value;
>			break;
> +		case DRM_I915_PERF_PROP_POLL_OA_DELAY:
> +			if (value < 100000 /* 100us */) {

So no maximum? That's probably ok, reap what you sow. Have we tested 100
us, seems low, anyway.

> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> index 9ee7c58e70d5..01559ead22e2 100644
> --- a/drivers/gpu/drm/i915/i915_perf_types.h
> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> @@ -304,6 +304,12 @@ struct i915_perf_stream {
>	 * reprogrammed.
>	 */
>	struct i915_vma *noa_wait;
> +
> +	/**
> +	 * @poll_oa_period: The period in nanoseconds at which the OA
> +	 * buffer should be checked for available data.
> +	 */
> +	u64 poll_oa_period;

poll_oa_period_ns?

>  };
>
>  /**
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 2813e579b480..dd511e7f795d 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1969,6 +1969,19 @@ enum drm_i915_perf_property_id {
>	 */
>	DRM_I915_PERF_PROP_HOLD_PREEMPTION,
>
> +	/**
> +	 * This optional parameter specifies the timer interval in nanoseconds
> +	 * at which the i915 driver will check the OA buffer for available data.
> +	 * Minimum allowed value is 100 microseconds. A default value is used by
> +	 * the driver if this parameter is not specified. Note that a large
> +	 * value may reduce cpu consumption during OA perf captures, but it
> +	 * would also potentially result in OA buffer overwrite as the captures
> +	 * reach end of the OA buffer.
> +	 *
> +	 * This property is available in perf revision 4.
> +	 */
> +	DRM_I915_PERF_PROP_POLL_OA_DELAY,

Is DRM_I915_PERF_PROP_POLL_OA_PERIOD_NS a better name? At least PERIOD
instead of DELAY.

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