[PATCH v4 0/5] Update CoreSight infrastructure to select a default sink.

[PATCH v4 0/5] Update CoreSight infrastructure to select a default sink.

[Mike Leach]

This patchset provides a proposed infrastructure to allow for the automatic
selection of a sink during CoreSight tracing operations.

Currently starting tracing using perf requires a sink selection on the
command line:-

sudo ./perf record -e cs_etm/@tmc_etr0/ --per-thread uname -a

After this set (and the follow-up perf change set) the infrastructure will
be able to select a default sink:-

sudo ./perf record -e cs_etm// --per-thread uname -a

This matches with the default operation provided with perf and intelpt.

Where no sink is specified at the start of a trace session, the CoreSight
system will walk the connection graph from the source ETM, to find a
suitable sink using the first encountered highest priority device.

The CoreSight infrastructure is updated to define sink sub_types to
differentiate between sinks with built in buffers (ETB / ETF) - BUFFER
type, and those that use system memory (ETR) - SYSMEM - types.

SYSMEM types are considered higher priority.

When two sinks are found of equal priority, then the closest sink to the
source in terms of connection nodes is chosen.

The automatic sink selection will also operate if an ETM is enabled using
sysfs commands, and no sink is currently enabled.

Applies to Linux coresight/next branch

Changes since v3:
1) Removed RFC designation and distributed to wider audience.
2) Split set into CoreSight driver code (this set), and perf user runtime set.
3) Minor cosmetic changes.

Mike Leach (5):
  coresight: Fix comment in main header file.
  coresight: Add default sink selection to CoreSight base
  coresight: tmc: Update sink types for default selection.
  coresight: etm: perf: Add default sink selection to etm perf
  coresight: sysfs: Allow select default sink on source enable.

 .../hwtracing/coresight/coresight-etm-perf.c  |  17 +-
 drivers/hwtracing/coresight/coresight-priv.h  |   2 +
 drivers/hwtracing/coresight/coresight-tmc.c   |   3 +-
 drivers/hwtracing/coresight/coresight.c       | 147 +++++++++++++++++-
 include/linux/coresight.h                     |   6 +-
 5 files changed, 168 insertions(+), 7 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/5] coresight: Fix comment in main header file.

[Mike Leach]

Comment for an elemnt in the coresight_device structure appears to have
been corrupted & makes no sense. Fix this before making further changes.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 include/linux/coresight.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index e3e9f0e3a878..84dc695e87d4 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -179,7 +179,8 @@ struct coresight_sysfs_link {
  * @enable:	'true' if component is currently part of an active path.
  * @activated:	'true' only if a _sink_ has been activated.  A sink can be
  *		activated but not yet enabled.  Enabling for a _sink_
- *		appens when a source has been selected for that it.
+ *		happens when a source has been selected and a path is enabled
+ *		from source to that sink.
  * @ea:		Device attribute for sink representation under PMU directory.
  * @ect_dev:	Associated cross trigger device. Not part of the trace data
  *		path or connections.
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/5] coresight: Fix comment in main header file.

[Suzuki K Poulose]

On 05/26/2020 11:46 AM, Mike Leach wrote:
> Comment for an elemnt in the coresight_device structure appears to have
> been corrupted & makes no sense. Fix this before making further changes.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/5] coresight: Fix comment in main header file.

[Mathieu Poirier]

On Tue, May 26, 2020 at 11:46:38AM +0100, Mike Leach wrote:
> Comment for an elemnt in the coresight_device structure appears to have
> been corrupted & makes no sense. Fix this before making further changes.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> ---
>  include/linux/coresight.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index e3e9f0e3a878..84dc695e87d4 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -179,7 +179,8 @@ struct coresight_sysfs_link {
>   * @enable:	'true' if component is currently part of an active path.
>   * @activated:	'true' only if a _sink_ has been activated.  A sink can be
>   *		activated but not yet enabled.  Enabling for a _sink_
> - *		appens when a source has been selected for that it.
> + *		happens when a source has been selected and a path is enabled
> + *		from source to that sink.
>   * @ea:		Device attribute for sink representation under PMU directory.
>   * @ect_dev:	Associated cross trigger device. Not part of the trace data
>   *		path or connections.
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/5] coresight: Add default sink selection to CoreSight base

[Mike Leach]

Adds a method to select a suitable sink connected to a given source.

In cases where no sink is defined, the coresight_find_default_sink
routine can search from a given source, through the child connections
until a suitable sink is found.

The suitability is defined in by the sink coresight_dev_subtype on the
CoreSight device, and the distance from the source by counting
connections.

Higher value subtype is preferred - where these are equal, shorter
distance from source is used as a tie-break.

This allows for default sink to be discovered were none is specified
(e.g. perf command line)

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/hwtracing/coresight/coresight-priv.h |   2 +
 drivers/hwtracing/coresight/coresight.c      | 136 +++++++++++++++++++
 include/linux/coresight.h                    |   3 +
 3 files changed, 141 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 36c943ae94d5..f2dc625ea585 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -150,6 +150,8 @@ int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
 struct coresight_device *coresight_get_sink(struct list_head *path);
 struct coresight_device *coresight_get_enabled_sink(bool reset);
 struct coresight_device *coresight_get_sink_by_id(u32 id);
+struct coresight_device *
+coresight_find_default_sink(struct coresight_device *csdev);
 struct list_head *coresight_build_path(struct coresight_device *csdev,
 				       struct coresight_device *sink);
 void coresight_release_path(struct list_head *path);
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index f3efbb3b2b4d..7632d060e25d 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -769,6 +769,142 @@ void coresight_release_path(struct list_head *path)
 	path = NULL;
 }
 
+/* return true if the device is a suitable type for a default sink */
+static inline bool coresight_is_def_sink_type(struct coresight_device *csdev)
+{
+	/* sink & correct subtype */
+	if (((csdev->type == CORESIGHT_DEV_TYPE_SINK) ||
+	     (csdev->type == CORESIGHT_DEV_TYPE_LINKSINK)) &&
+	    (csdev->subtype.sink_subtype >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER))
+		return true;
+	return false;
+}
+
+/**
+ * coresight_select_best_sink - return the best sink for use as default from
+ * the two provided.
+ *
+ * @sink:	current best sink.
+ * @depth:      search depth where current sink was found.
+ * @new_sink:	new sink for comparison with current sink.
+ * @new_depth:  search depth where new sink was found.
+ *
+ * Sinks prioritised according to coresight_dev_subtype_sink, with only
+ * subtypes CORESIGHT_DEV_SUBTYPE_SINK_BUFFER or higher being used.
+ *
+ * Where two sinks of equal priority are found, the sink closest to the
+ * source is used (smallest search depth).
+ *
+ * return @new_sink & update @depth if better than @sink, else return @sink.
+ */
+static struct coresight_device *
+coresight_select_best_sink(struct coresight_device *sink, int *depth,
+			   struct coresight_device *new_sink, int new_depth)
+{
+	bool update = false;
+
+	if (!sink) {
+		/* first found at this level */
+		update = true;
+	} else if (new_sink->subtype.sink_subtype >
+		   sink->subtype.sink_subtype) {
+		/* found better sink */
+		update = true;
+	} else if ((new_sink->subtype.sink_subtype ==
+		    sink->subtype.sink_subtype) &&
+		   (*depth > new_depth)) {
+		/* found same but closer sink */
+		update = true;
+	}
+
+	if (update)
+		*depth = new_depth;
+	return update ? new_sink : sink;
+}
+
+/**
+ * coresight_find_sink - recursive function to walk trace connections from
+ * source to find a suitable default sink.
+ *
+ * @csdev: source / current device to check.
+ * @depth: [in] search depth of calling dev, [out] depth of found sink.
+ *
+ * This will walk the connection path from a source (ETM) till a suitable
+ * sink is encountered and return that sink to the original caller.
+ *
+ * If current device is a plain sink return that & depth, otherwise recursively
+ * call child connections looking for a sink. Select best possible using
+ * coresight_select_best_sink.
+ *
+ * return best sink found, or NULL if not found at this node or child nodes.
+ */
+static struct coresight_device *
+coresight_find_sink(struct coresight_device *csdev, int *depth)
+{
+	int i, curr_depth = *depth + 1, found_depth = 0;
+	struct coresight_device *found_sink = NULL;
+
+	if (coresight_is_def_sink_type(csdev)) {
+		found_depth = curr_depth;
+		found_sink = csdev;
+		if (csdev->type == CORESIGHT_DEV_TYPE_SINK)
+			goto return_def_sink;
+		/* look past LINKSINK for something better */
+	}
+
+	/*
+	 * Not a sink we want - or possible child sink may be better.
+	 * recursively explore each port found on this element.
+	 */
+	for (i = 0; i < csdev->pdata->nr_outport; i++) {
+		struct coresight_device *child_dev, *sink = NULL;
+		int child_depth = curr_depth;
+
+		child_dev = csdev->pdata->conns[i].child_dev;
+		if (child_dev)
+			sink = coresight_find_sink(child_dev, &child_depth);
+
+		if (sink)
+			found_sink = coresight_select_best_sink(found_sink,
+								&found_depth,
+								sink,
+								child_depth);
+	}
+
+return_def_sink:
+	/* return found sink and depth */
+	if (found_sink)
+		*depth = found_depth;
+	return found_sink;
+}
+
+/**
+ * coresight_find_default_sink: Find a sink suitable for use as a
+ * default sink.
+ *
+ * @csdev: starting source to find a connected sink.
+ *
+ * Walks connections graph looking for a suitable sink to enable for the
+ * supplied source. Uses CoreSight device subtypes and distance from source
+ * to select the best sink.
+ *
+ * If a sink is found, then the default sink for this device is set and
+ * will be automatically used in future.
+ *
+ * Used in cases where the CoreSight user (perf / sysfs) has not selected a
+ * sink.
+ */
+struct coresight_device *
+coresight_find_default_sink(struct coresight_device *csdev)
+{
+	int depth = 0;
+
+	/* look for a default sink if we have not found for this device */
+	if (!csdev->def_sink)
+		csdev->def_sink = coresight_find_sink(csdev, &depth);
+	return csdev->def_sink;
+}
+
 /** coresight_validate_source - make sure a source has the right credentials
  *  @csdev:	the device structure for a source.
  *  @function:	the function this was called from.
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 84dc695e87d4..58fffdecdbfd 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -48,6 +48,7 @@ enum coresight_dev_subtype_sink {
 	CORESIGHT_DEV_SUBTYPE_SINK_NONE,
 	CORESIGHT_DEV_SUBTYPE_SINK_PORT,
 	CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
+	CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
 };
 
 enum coresight_dev_subtype_link {
@@ -182,6 +183,7 @@ struct coresight_sysfs_link {
  *		happens when a source has been selected and a path is enabled
  *		from source to that sink.
  * @ea:		Device attribute for sink representation under PMU directory.
+ * @def_sink:	cached reference to default sink found for this device.
  * @ect_dev:	Associated cross trigger device. Not part of the trace data
  *		path or connections.
  * @nr_links:   number of sysfs links created to other components from this
@@ -200,6 +202,7 @@ struct coresight_device {
 	/* sink specific fields */
 	bool activated;	/* true only if a sink is part of a path */
 	struct dev_ext_attribute *ea;
+	struct coresight_device *def_sink;
 	/* cross trigger handling */
 	struct coresight_device *ect_dev;
 	/* sysfs links between components */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/5] coresight: Add default sink selection to CoreSight base

[Suzuki K Poulose]

On 05/26/2020 11:46 AM, Mike Leach wrote:
> Adds a method to select a suitable sink connected to a given source.
> 
> In cases where no sink is defined, the coresight_find_default_sink
> routine can search from a given source, through the child connections
> until a suitable sink is found.
> 
> The suitability is defined in by the sink coresight_dev_subtype on the
> CoreSight device, and the distance from the source by counting
> connections.
> 
> Higher value subtype is preferred - where these are equal, shorter
> distance from source is used as a tie-break.
> 
> This allows for default sink to be discovered were none is specified
> (e.g. perf command line)
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>

Feel free to add:
Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>

> +/**
> + * coresight_find_default_sink: Find a sink suitable for use as a
> + * default sink.
> + *
> + * @csdev: starting source to find a connected sink.
> + *
> + * Walks connections graph looking for a suitable sink to enable for the
> + * supplied source. Uses CoreSight device subtypes and distance from source
> + * to select the best sink.
> + *
> + * If a sink is found, then the default sink for this device is set and
> + * will be automatically used in future.
> + *
> + * Used in cases where the CoreSight user (perf / sysfs) has not selected a
> + * sink.
> + */
> +struct coresight_device *
> +coresight_find_default_sink(struct coresight_device *csdev)
> +{
> +	int depth = 0;
> +
> +	/* look for a default sink if we have not found for this device */
> +	if (!csdev->def_sink)
> +		csdev->def_sink = coresight_find_sink(csdev, &depth);

Could we add a helper to clear the cached "def-sink" from all the
"sources", when the sink is going away ? You may be able to do this via
coresight_remove_match()

Rest looks good to me.
Suzuki



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/5] coresight: Add default sink selection to CoreSight base

[Mike Leach]

Hi Suzuki,

On Tue, 2 Jun 2020 at 11:20, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 05/26/2020 11:46 AM, Mike Leach wrote:
> > Adds a method to select a suitable sink connected to a given source.
> >
> > In cases where no sink is defined, the coresight_find_default_sink
> > routine can search from a given source, through the child connections
> > until a suitable sink is found.
> >
> > The suitability is defined in by the sink coresight_dev_subtype on the
> > CoreSight device, and the distance from the source by counting
> > connections.
> >
> > Higher value subtype is preferred - where these are equal, shorter
> > distance from source is used as a tie-break.
> >
> > This allows for default sink to be discovered were none is specified
> > (e.g. perf command line)
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
>
> Feel free to add:
> Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>

Will do.

> > +/**
> > + * coresight_find_default_sink: Find a sink suitable for use as a
> > + * default sink.Yes - the newer topologies will need some changes - beyond what we are handling here.
However - especially for 1:1 -  the best way may be to always use the
default sink - as specifying multiple sinks on the perf command line
may be problematical.


> > + *
> > + * @csdev: starting source to find a connected sink.
> > + *
> > + * Walks connections graph looking for a suitable sink to enable for the
> > + * supplied source. Uses CoreSight device subtypes and distance from source
> > + * to select the best sink.
> > + *
> > + * If a sink is found, then the default sink for this device is set and
> > + * will be automatically used in future.
> > + *
> > + * Used in cases where the CoreSight user (perf / sysfs) has not selected a
> > + * sink.
> > + */
> > +struct coresight_device *
> > +coresight_find_default_sink(struct coresight_device *csdev)
> > +{
> > +     int depth = 0;
> > +
> > +     /* look for a default sink if we have not found for this device */
> > +     if (!csdev->def_sink)
> > +             csdev->def_sink = coresight_find_sink(csdev, &depth);
>
> Could we add a helper to clear the cached "def-sink" from all the
> "sources", when the sink is going away ? You may be able to do this via
> coresight_remove_match()
>

Is this needed in the current state of the CoreSight driver
infrastructure? The topology is fixed for the lifetime of the device
so a sink cannot disappear without the rest of the CoreSight going
away - i.e. reboot / shutdown.
If there is concern about a race condition then something similar to
coresight_remove_match can be developed, but the actual function only
works on immediate connections / output ports.

Thanks

Mike


> Rest looks good to me.
> Suzuki
>
>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/5] coresight: Add default sink selection to CoreSight base

[Mathieu Poirier]

On Tue, May 26, 2020 at 11:46:39AM +0100, Mike Leach wrote:
> Adds a method to select a suitable sink connected to a given source.
> 
> In cases where no sink is defined, the coresight_find_default_sink
> routine can search from a given source, through the child connections
> until a suitable sink is found.
> 
> The suitability is defined in by the sink coresight_dev_subtype on the
> CoreSight device, and the distance from the source by counting
> connections.
> 
> Higher value subtype is preferred - where these are equal, shorter
> distance from source is used as a tie-break.
> 
> This allows for default sink to be discovered were none is specified
> (e.g. perf command line)
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Leo has also added a RB for this patch, please add it on when you rebase on
v5.8-rc1.

> ---
>  drivers/hwtracing/coresight/coresight-priv.h |   2 +
>  drivers/hwtracing/coresight/coresight.c      | 136 +++++++++++++++++++
>  include/linux/coresight.h                    |   3 +
>  3 files changed, 141 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 36c943ae94d5..f2dc625ea585 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -150,6 +150,8 @@ int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
>  struct coresight_device *coresight_get_sink(struct list_head *path);
>  struct coresight_device *coresight_get_enabled_sink(bool reset);
>  struct coresight_device *coresight_get_sink_by_id(u32 id);
> +struct coresight_device *
> +coresight_find_default_sink(struct coresight_device *csdev);
>  struct list_head *coresight_build_path(struct coresight_device *csdev,
>  				       struct coresight_device *sink);
>  void coresight_release_path(struct list_head *path);
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index f3efbb3b2b4d..7632d060e25d 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -769,6 +769,142 @@ void coresight_release_path(struct list_head *path)
>  	path = NULL;
>  }
>  
> +/* return true if the device is a suitable type for a default sink */
> +static inline bool coresight_is_def_sink_type(struct coresight_device *csdev)
> +{
> +	/* sink & correct subtype */
> +	if (((csdev->type == CORESIGHT_DEV_TYPE_SINK) ||
> +	     (csdev->type == CORESIGHT_DEV_TYPE_LINKSINK)) &&
> +	    (csdev->subtype.sink_subtype >= CORESIGHT_DEV_SUBTYPE_SINK_BUFFER))
> +		return true;
> +	return false;
> +}
> +
> +/**
> + * coresight_select_best_sink - return the best sink for use as default from
> + * the two provided.
> + *
> + * @sink:	current best sink.
> + * @depth:      search depth where current sink was found.
> + * @new_sink:	new sink for comparison with current sink.
> + * @new_depth:  search depth where new sink was found.
> + *
> + * Sinks prioritised according to coresight_dev_subtype_sink, with only
> + * subtypes CORESIGHT_DEV_SUBTYPE_SINK_BUFFER or higher being used.
> + *
> + * Where two sinks of equal priority are found, the sink closest to the
> + * source is used (smallest search depth).
> + *
> + * return @new_sink & update @depth if better than @sink, else return @sink.
> + */
> +static struct coresight_device *
> +coresight_select_best_sink(struct coresight_device *sink, int *depth,
> +			   struct coresight_device *new_sink, int new_depth)
> +{
> +	bool update = false;
> +
> +	if (!sink) {
> +		/* first found at this level */
> +		update = true;
> +	} else if (new_sink->subtype.sink_subtype >
> +		   sink->subtype.sink_subtype) {
> +		/* found better sink */
> +		update = true;
> +	} else if ((new_sink->subtype.sink_subtype ==
> +		    sink->subtype.sink_subtype) &&
> +		   (*depth > new_depth)) {
> +		/* found same but closer sink */
> +		update = true;
> +	}
> +
> +	if (update)
> +		*depth = new_depth;
> +	return update ? new_sink : sink;
> +}
> +
> +/**
> + * coresight_find_sink - recursive function to walk trace connections from
> + * source to find a suitable default sink.
> + *
> + * @csdev: source / current device to check.
> + * @depth: [in] search depth of calling dev, [out] depth of found sink.
> + *
> + * This will walk the connection path from a source (ETM) till a suitable
> + * sink is encountered and return that sink to the original caller.
> + *
> + * If current device is a plain sink return that & depth, otherwise recursively
> + * call child connections looking for a sink. Select best possible using
> + * coresight_select_best_sink.
> + *
> + * return best sink found, or NULL if not found at this node or child nodes.
> + */
> +static struct coresight_device *
> +coresight_find_sink(struct coresight_device *csdev, int *depth)
> +{
> +	int i, curr_depth = *depth + 1, found_depth = 0;
> +	struct coresight_device *found_sink = NULL;
> +
> +	if (coresight_is_def_sink_type(csdev)) {
> +		found_depth = curr_depth;
> +		found_sink = csdev;
> +		if (csdev->type == CORESIGHT_DEV_TYPE_SINK)
> +			goto return_def_sink;
> +		/* look past LINKSINK for something better */
> +	}
> +
> +	/*
> +	 * Not a sink we want - or possible child sink may be better.
> +	 * recursively explore each port found on this element.
> +	 */
> +	for (i = 0; i < csdev->pdata->nr_outport; i++) {
> +		struct coresight_device *child_dev, *sink = NULL;
> +		int child_depth = curr_depth;
> +
> +		child_dev = csdev->pdata->conns[i].child_dev;
> +		if (child_dev)
> +			sink = coresight_find_sink(child_dev, &child_depth);
> +
> +		if (sink)
> +			found_sink = coresight_select_best_sink(found_sink,
> +								&found_depth,
> +								sink,
> +								child_depth);
> +	}
> +
> +return_def_sink:
> +	/* return found sink and depth */
> +	if (found_sink)
> +		*depth = found_depth;
> +	return found_sink;
> +}
> +
> +/**
> + * coresight_find_default_sink: Find a sink suitable for use as a
> + * default sink.
> + *
> + * @csdev: starting source to find a connected sink.
> + *
> + * Walks connections graph looking for a suitable sink to enable for the
> + * supplied source. Uses CoreSight device subtypes and distance from source
> + * to select the best sink.
> + *
> + * If a sink is found, then the default sink for this device is set and
> + * will be automatically used in future.
> + *
> + * Used in cases where the CoreSight user (perf / sysfs) has not selected a
> + * sink.
> + */
> +struct coresight_device *
> +coresight_find_default_sink(struct coresight_device *csdev)
> +{
> +	int depth = 0;
> +
> +	/* look for a default sink if we have not found for this device */
> +	if (!csdev->def_sink)
> +		csdev->def_sink = coresight_find_sink(csdev, &depth);
> +	return csdev->def_sink;
> +}
> +
>  /** coresight_validate_source - make sure a source has the right credentials
>   *  @csdev:	the device structure for a source.
>   *  @function:	the function this was called from.
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 84dc695e87d4..58fffdecdbfd 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -48,6 +48,7 @@ enum coresight_dev_subtype_sink {
>  	CORESIGHT_DEV_SUBTYPE_SINK_NONE,
>  	CORESIGHT_DEV_SUBTYPE_SINK_PORT,
>  	CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
> +	CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,
>  };
>  
>  enum coresight_dev_subtype_link {
> @@ -182,6 +183,7 @@ struct coresight_sysfs_link {
>   *		happens when a source has been selected and a path is enabled
>   *		from source to that sink.
>   * @ea:		Device attribute for sink representation under PMU directory.
> + * @def_sink:	cached reference to default sink found for this device.
>   * @ect_dev:	Associated cross trigger device. Not part of the trace data
>   *		path or connections.
>   * @nr_links:   number of sysfs links created to other components from this
> @@ -200,6 +202,7 @@ struct coresight_device {
>  	/* sink specific fields */
>  	bool activated;	/* true only if a sink is part of a path */
>  	struct dev_ext_attribute *ea;
> +	struct coresight_device *def_sink;
>  	/* cross trigger handling */
>  	struct coresight_device *ect_dev;
>  	/* sysfs links between components */
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/5] coresight: tmc: Update sink types for default selection.

[Mike Leach]

An additional sink subtype is added to differentiate ETB/ETF buffer
sinks and ETR type system memory sinks.

This allows the prioritised selection of default sinks.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 39fba1d16e6e..0d2eb7e0e1bb 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -484,7 +484,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
 		break;
 	case TMC_CONFIG_TYPE_ETR:
 		desc.type = CORESIGHT_DEV_TYPE_SINK;
-		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
+		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
 		desc.ops = &tmc_etr_cs_ops;
 		ret = tmc_etr_setup_caps(dev, devid,
 					 coresight_get_uci_data(id));
@@ -496,6 +496,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
 		break;
 	case TMC_CONFIG_TYPE_ETF:
 		desc.type = CORESIGHT_DEV_TYPE_LINKSINK;
+		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
 		desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
 		desc.ops = &tmc_etf_cs_ops;
 		dev_list = &etf_devs;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/5] coresight: tmc: Update sink types for default selection.

[Suzuki K Poulose]

On 05/26/2020 11:46 AM, Mike Leach wrote:
> An additional sink subtype is added to differentiate ETB/ETF buffer
> sinks and ETR type system memory sinks.
> 
> This allows the prioritised selection of default sinks.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

> ---
>   drivers/hwtracing/coresight/coresight-tmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index 39fba1d16e6e..0d2eb7e0e1bb 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -484,7 +484,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>   		break;
>   	case TMC_CONFIG_TYPE_ETR:
>   		desc.type = CORESIGHT_DEV_TYPE_SINK;
> -		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> +		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
>   		desc.ops = &tmc_etr_cs_ops;
>   		ret = tmc_etr_setup_caps(dev, devid,
>   					 coresight_get_uci_data(id));
> @@ -496,6 +496,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>   		break;
>   	case TMC_CONFIG_TYPE_ETF:
>   		desc.type = CORESIGHT_DEV_TYPE_LINKSINK;
> +		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>   		desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
>   		desc.ops = &tmc_etf_cs_ops;
>   		dev_list = &etf_devs;
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/5] coresight: tmc: Update sink types for default selection.

[Mathieu Poirier]

On Tue, May 26, 2020 at 11:46:40AM +0100, Mike Leach wrote:
> An additional sink subtype is added to differentiate ETB/ETF buffer
> sinks and ETR type system memory sinks.
> 
> This allows the prioritised selection of default sinks.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-tmc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index 39fba1d16e6e..0d2eb7e0e1bb 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -484,7 +484,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>  		break;
>  	case TMC_CONFIG_TYPE_ETR:
>  		desc.type = CORESIGHT_DEV_TYPE_SINK;
> -		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> +		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
>  		desc.ops = &tmc_etr_cs_ops;
>  		ret = tmc_etr_setup_caps(dev, devid,
>  					 coresight_get_uci_data(id));
> @@ -496,6 +496,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>  		break;
>  	case TMC_CONFIG_TYPE_ETF:
>  		desc.type = CORESIGHT_DEV_TYPE_LINKSINK;
> +		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>  		desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
>  		desc.ops = &tmc_etf_cs_ops;
>  		dev_list = &etf_devs;

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf

[Mike Leach]

Add default sink selection to the perf trace handling in the etm driver.
Uses the select default sink infrastructure to select a sink for the perf
session, if no other sink is specified.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 84f1dcb69827..1a3169e69bb1 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		sink = coresight_get_enabled_sink(true);
 	}
 
-	if (!sink)
-		goto err;
-
 	mask = &event_data->mask;
 
 	/*
@@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 			continue;
 		}
 
+		/*
+		 * No sink provided - look for a default sink for one of the
+		 * devices. At present we only support topology where all CPUs
+		 * use the same sink [N:1], so only need to find one sink. The
+		 * coresight_build_path later will remove any CPU that does not
+		 * attach to the sink, or if we have not found a sink.
+		 */
+		if (!sink)
+			sink = coresight_find_default_sink(csdev);
+
 		/*
 		 * Building a path doesn't enable it, it simply builds a
 		 * list of devices from source to sink that can be
@@ -267,6 +274,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 		*etm_event_cpu_path_ptr(event_data, cpu) = path;
 	}
 
+	/* no sink found for any CPU - cannot trace */
+	if (!sink)
+		goto err;
+
 	/* If we don't have any CPUs ready for tracing, abort */
 	cpu = cpumask_first(mask);
 	if (cpu >= nr_cpu_ids)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf

[Suzuki K Poulose]

On 05/26/2020 11:46 AM, Mike Leach wrote:
> Add default sink selection to the perf trace handling in the etm driver.
> Uses the select default sink infrastructure to select a sink for the perf
> session, if no other sink is specified.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>

This patch looks fine to me as such. But please see below for some
discussion on the future support for other configurations.


> ---
>   .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 84f1dcb69827..1a3169e69bb1 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>   		sink = coresight_get_enabled_sink(true);
>   	}
>   
> -	if (!sink)
> -		goto err;
> -
>   	mask = &event_data->mask;
>   
>   	/*
> @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>   			continue;
>   		}
>   
> +		/*
> +		 * No sink provided - look for a default sink for one of the
> +		 * devices. At present we only support topology where all CPUs
> +		 * use the same sink [N:1], so only need to find one sink. The
> +		 * coresight_build_path later will remove any CPU that does not
> +		 * attach to the sink, or if we have not found a sink.
> +		 */
> +		if (!sink)
> +			sink = coresight_find_default_sink(csdev);
> +

While we are here, should we remove the "find enabled sink" if the csink
is not specified via config. ? That step is problematic, as the user may 
not remember which sinks were enabled. Also, we can't hit that with
perf tool as it prevents any invocation without sink (until this change).

So may be this is a good time to get rid of that ?

Also, we may need to do special handling for cases where there multiple
sinks (ETRS) and the cpus in the event mask has different preferred
sink. We can defer it for now as we don't claim to support such
configurations yet.
When we do, we could either :

1) Make sure the event is bound to a single CPU, in which case
the sink remains the same for the event.

OR

2) All the different "preferred" sinks (ETRs selected by the ETM) have
the same capabilitiy. i.e, we can move around the "sink" specific
buffers and use them where we end up using.

We can defer all of this, until we get platforms which ned the support.

Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf

[Mike Leach]

Hi Suzuki,

On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> On 05/26/2020 11:46 AM, Mike Leach wrote:
> > Add default sink selection to the perf trace handling in the etm driver.
> > Uses the select default sink infrastructure to select a sink for the perf
> > session, if no other sink is specified.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
>
> This patch looks fine to me as such. But please see below for some
> discussion on the future support for other configurations.
>
>
> > ---
> >   .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
> >   1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index 84f1dcb69827..1a3169e69bb1 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> >               sink = coresight_get_enabled_sink(true);
> >       }
> >
> > -     if (!sink)
> > -             goto err;
> > -
> >       mask = &event_data->mask;
> >
> >       /*
> > @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> >                       continue;
> >               }
> >
> > +             /*
> > +              * No sink provided - look for a default sink for one of the
> > +              * devices. At present we only support topology where all CPUs
> > +              * use the same sink [N:1], so only need to find one sink. The
> > +              * coresight_build_path later will remove any CPU that does not
> > +              * attach to the sink, or if we have not found a sink.
> > +              */
> > +             if (!sink)
> > +                     sink = coresight_find_default_sink(csdev);
> > +
>
> While we are here, should we remove the "find enabled sink" if the csink
> is not specified via config. ? That step is problematic, as the user may
> not remember which sinks were enabled. Also, we can't hit that with
> perf tool as it prevents any invocation without sink (until this change).
>
> So may be this is a good time to get rid of that ?
>

You are correct - the  'sink = coresight_get_enabled_sink(true);' was
dead code until this patch.
However - if someone has set up their system using sysfs to enable
sinks, then should we not respect that rather than assume they made a
mistake?

Thinking about N:M topologies mentioned below - one method of handling
this is to enable relevant sinks and then let perf trace on any cores
that might use them.

> Also, we may need to do special handling for cases where there multiple
> sinks (ETRS) and the cpus in the event mask has different preferred
> sink. We can defer it for now as we don't claim to support such
> configurations yet.

Yes - the newer topologies will need some changes - beyond what we are
handling here.
However - especially for 1:1 -  the best way may be to always use the
default sink - as specifying multiple sinks on the perf command line
may be problematical.

> When we do, we could either :
>
> 1) Make sure the event is bound to a single CPU, in which case
> the sink remains the same for the event.
>
> OR
>
> 2) All the different "preferred" sinks (ETRs selected by the ETM) have
> the same capabilitiy. i.e, we can move around the "sink" specific
> buffers and use them where we end up using.

If here by "capabilities" we are talking about buffer vs system memory
type sinks then I agree. We may need in future to limit the search
algorithm to ensure that only the best system memory type sinks are
found and eliminate cores attached to only buffer type sinks.

>
> We can defer all of this, until we get platforms which ned the support.
>
> Suzuki

Thanks for the review.

Mike


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf

[Suzuki K Poulose]

On 06/02/2020 02:12 PM, Mike Leach wrote:
> Hi Suzuki,
> 
> On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> On 05/26/2020 11:46 AM, Mike Leach wrote:
>>> Add default sink selection to the perf trace handling in the etm driver.
>>> Uses the select default sink infrastructure to select a sink for the perf
>>> session, if no other sink is specified.
>>>
>>> Signed-off-by: Mike Leach <mike.leach@linaro.org>
>>
>> This patch looks fine to me as such. But please see below for some
>> discussion on the future support for other configurations.
>>
>>
>>> ---
>>>    .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
>>>    1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> index 84f1dcb69827..1a3169e69bb1 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>>>                sink = coresight_get_enabled_sink(true);
>>>        }
>>>
>>> -     if (!sink)
>>> -             goto err;
>>> -
>>>        mask = &event_data->mask;
>>>
>>>        /*
>>> @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>>>                        continue;
>>>                }
>>>
>>> +             /*
>>> +              * No sink provided - look for a default sink for one of the
>>> +              * devices. At present we only support topology where all CPUs
>>> +              * use the same sink [N:1], so only need to find one sink. The
>>> +              * coresight_build_path later will remove any CPU that does not
>>> +              * attach to the sink, or if we have not found a sink.
>>> +              */
>>> +             if (!sink)
>>> +                     sink = coresight_find_default_sink(csdev);
>>> +
>>
>> While we are here, should we remove the "find enabled sink" if the csink
>> is not specified via config. ? That step is problematic, as the user may
>> not remember which sinks were enabled. Also, we can't hit that with
>> perf tool as it prevents any invocation without sink (until this change).
>>
>> So may be this is a good time to get rid of that ?
>>
> 
> You are correct - the  'sink = coresight_get_enabled_sink(true);' was
> dead code until this patch.
> However - if someone has set up their system using sysfs to enable
> sinks, then should we not respect that rather than assume they made a
> mistake?

If someone really wants to use a specific sink, then they could always
specify it via the config attribute and it will be honoured. We need not
carry along this non-intuitive hinting.

> 
> Thinking about N:M topologies mentioned below - one method of handling
> this is to enable relevant sinks and then let perf trace on any cores
> that might use them.
> 
>> Also, we may need to do special handling for cases where there multiple
>> sinks (ETRS) and the cpus in the event mask has different preferred
>> sink. We can defer it for now as we don't claim to support such
>> configurations yet.
> 
> Yes - the newer topologies will need some changes - beyond what we are
> handling here.
> However - especially for 1:1 -  the best way may be to always use the
> default sink - as specifying multiple sinks on the perf command line
> may be problematical.
> 
>> When we do, we could either :
>>
>> 1) Make sure the event is bound to a single CPU, in which case
>> the sink remains the same for the event.
>>
>> OR
>>
>> 2) All the different "preferred" sinks (ETRs selected by the ETM) have
>> the same capabilitiy. i.e, we can move around the "sink" specific
>> buffers and use them where we end up using.
> 
> If here by "capabilities" we are talking about buffer vs system memory
> type sinks then I agree. We may need in future to limit the search

Not necessarily. e.g, if we ever get two different types of system
memory sinks, (e.g, a global ETR and a dedicate "new" sink for a
cluster), we can't keep switching between the two sinks depending on how
they use the buffers. (i.e, direct buffer vs double copy)

Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf

[Mathieu Poirier]

On Tue, Jun 02, 2020 at 02:29:30PM +0100, Suzuki K Poulose wrote:
> On 06/02/2020 02:12 PM, Mike Leach wrote:
> > Hi Suzuki,
> > 
> > On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> > > 
> > > On 05/26/2020 11:46 AM, Mike Leach wrote:
> > > > Add default sink selection to the perf trace handling in the etm driver.
> > > > Uses the select default sink infrastructure to select a sink for the perf
> > > > session, if no other sink is specified.
> > > > 
> > > > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > > 
> > > This patch looks fine to me as such. But please see below for some
> > > discussion on the future support for other configurations.
> > > 
> > > 
> > > > ---
> > > >    .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
> > > >    1 file changed, 14 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > index 84f1dcb69827..1a3169e69bb1 100644
> > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> > > >                sink = coresight_get_enabled_sink(true);
> > > >        }
> > > > 
> > > > -     if (!sink)
> > > > -             goto err;
> > > > -
> > > >        mask = &event_data->mask;
> > > > 
> > > >        /*
> > > > @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> > > >                        continue;
> > > >                }
> > > > 
> > > > +             /*
> > > > +              * No sink provided - look for a default sink for one of the
> > > > +              * devices. At present we only support topology where all CPUs
> > > > +              * use the same sink [N:1], so only need to find one sink. The
> > > > +              * coresight_build_path later will remove any CPU that does not
> > > > +              * attach to the sink, or if we have not found a sink.
> > > > +              */
> > > > +             if (!sink)
> > > > +                     sink = coresight_find_default_sink(csdev);
> > > > +
> > > 
> > > While we are here, should we remove the "find enabled sink" if the csink
> > > is not specified via config. ? That step is problematic, as the user may
> > > not remember which sinks were enabled. Also, we can't hit that with
> > > perf tool as it prevents any invocation without sink (until this change).

Old version of perf tools will take sinks selected on the perf command line and
use the sysfs to communicate that to the kernel.  Granted there may not be that
many (if any), removing coresight_get_enabled_sink() will break those
implementation. 

The real question is if keeping the functionatlity around so troublesome that it
overweighs the drawbacks of removing it. 

> > > 
> > > So may be this is a good time to get rid of that ?
> > > 
> > 
> > You are correct - the  'sink = coresight_get_enabled_sink(true);' was
> > dead code until this patch.
> > However - if someone has set up their system using sysfs to enable
> > sinks, then should we not respect that rather than assume they made a
> > mistake?
> 
> If someone really wants to use a specific sink, then they could always
> specify it via the config attribute and it will be honoured. We need not
> carry along this non-intuitive hinting.
> 
> > 
> > Thinking about N:M topologies mentioned below - one method of handling
> > this is to enable relevant sinks and then let perf trace on any cores
> > that might use them.
> > 
> > > Also, we may need to do special handling for cases where there multiple
> > > sinks (ETRS) and the cpus in the event mask has different preferred
> > > sink. We can defer it for now as we don't claim to support such
> > > configurations yet.
> > 
> > Yes - the newer topologies will need some changes - beyond what we are
> > handling here.
> > However - especially for 1:1 -  the best way may be to always use the
> > default sink - as specifying multiple sinks on the perf command line
> > may be problematical.
> > 
> > > When we do, we could either :
> > > 
> > > 1) Make sure the event is bound to a single CPU, in which case
> > > the sink remains the same for the event.
> > > 
> > > OR
> > > 
> > > 2) All the different "preferred" sinks (ETRs selected by the ETM) have
> > > the same capabilitiy. i.e, we can move around the "sink" specific
> > > buffers and use them where we end up using.
> > 
> > If here by "capabilities" we are talking about buffer vs system memory
> > type sinks then I agree. We may need in future to limit the search
> 
> Not necessarily. e.g, if we ever get two different types of system
> memory sinks, (e.g, a global ETR and a dedicate "new" sink for a
> cluster), we can't keep switching between the two sinks depending on how
> they use the buffers. (i.e, direct buffer vs double copy)
> 
> Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf

[Mike Leach]

Hi,

On Tue, 2 Jun 2020 at 17:59, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>
> On Tue, Jun 02, 2020 at 02:29:30PM +0100, Suzuki K Poulose wrote:
> > On 06/02/2020 02:12 PM, Mike Leach wrote:
> > > Hi Suzuki,
> > >
> > > On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> > > >
> > > > On 05/26/2020 11:46 AM, Mike Leach wrote:
> > > > > Add default sink selection to the perf trace handling in the etm driver.
> > > > > Uses the select default sink infrastructure to select a sink for the perf
> > > > > session, if no other sink is specified.
> > > > >
> > > > > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > > >
> > > > This patch looks fine to me as such. But please see below for some
> > > > discussion on the future support for other configurations.
> > > >
> > > >
> > > > > ---
> > > > >    .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
> > > > >    1 file changed, 14 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > index 84f1dcb69827..1a3169e69bb1 100644
> > > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > > > @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> > > > >                sink = coresight_get_enabled_sink(true);
> > > > >        }
> > > > >
> > > > > -     if (!sink)
> > > > > -             goto err;
> > > > > -
> > > > >        mask = &event_data->mask;
> > > > >
> > > > >        /*
> > > > > @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> > > > >                        continue;
> > > > >                }
> > > > >
> > > > > +             /*
> > > > > +              * No sink provided - look for a default sink for one of the
> > > > > +              * devices. At present we only support topology where all CPUs
> > > > > +              * use the same sink [N:1], so only need to find one sink. The
> > > > > +              * coresight_build_path later will remove any CPU that does not
> > > > > +              * attach to the sink, or if we have not found a sink.
> > > > > +              */
> > > > > +             if (!sink)
> > > > > +                     sink = coresight_find_default_sink(csdev);
> > > > > +
> > > >
> > > > While we are here, should we remove the "find enabled sink" if the csink
> > > > is not specified via config. ? That step is problematic, as the user may
> > > > not remember which sinks were enabled. Also, we can't hit that with
> > > > perf tool as it prevents any invocation without sink (until this change).
>
> Old version of perf tools will take sinks selected on the perf command line and
> use the sysfs to communicate that to the kernel.  Granted there may not be that
> many (if any), removing coresight_get_enabled_sink() will break those
> implementation.
>
> The real question is if keeping the functionatlity around so troublesome that it
> overweighs the drawbacks of removing it.
>
> > > >
> > > > So may be this is a good time to get rid of that ?
> > > >
> > >
> > > You are correct - the  'sink = coresight_get_enabled_sink(true);' was
> > > dead code until this patch.
> > > However - if someone has set up their system using sysfs to enable
> > > sinks, then should we not respect that rather than assume they made a
> > > mistake?
> >
> > If someone really wants to use a specific sink, then they could always
> > specify it via the config attribute and it will be honoured. We need not
> > carry along this non-intuitive hinting.
> >

Problem is - as mentioned below - config can only specify one sink, so
when we support 1:1 / N:M topology we need a way of specifying
multiple sinks. This is one viable option - especially where we are
using entire system configuration settings.

As Mathieu points out - there is little harm in leaving this in - if
we take it out now, we will probably have to replace it with something
similar anyway.

> > >
> > > Thinking about N:M topologies mentioned below - one method of handling
> > > this is to enable relevant sinks and then let perf trace on any cores
> > > that might use them.
> > >
> > > > Also, we may need to do special handling for cases where there multiple
> > > > sinks (ETRS) and the cpus in the event mask has different preferred
> > > > sink. We can defer it for now as we don't claim to support such
> > > > configurations yet.
> > >
> > > Yes - the newer topologies will need some changes - beyond what we are
> > > handling here.
> > > However - especially for 1:1 -  the best way may be to always use the
> > > default sink - as specifying multiple sinks on the perf command line
> > > may be problematical.
> > >
> > > > When we do, we could either :
> > > >
> > > > 1) Make sure the event is bound to a single CPU, in which case
> > > > the sink remains the same for the event.
> > > >
> > > > OR
> > > >
> > > > 2) All the different "preferred" sinks (ETRs selected by the ETM) have
> > > > the same capabilitiy. i.e, we can move around the "sink" specific
> > > > buffers and use them where we end up using.
> > >
> > > If here by "capabilities" we are talking about buffer vs system memory
> > > type sinks then I agree. We may need in future to limit the search
> >
> > Not necessarily. e.g, if we ever get two different types of system
> > memory sinks, (e.g, a global ETR and a dedicate "new" sink for a
> > cluster), we can't keep switching between the two sinks depending on how
> > they use the buffers. (i.e, direct buffer vs double copy)
> >

I would have thought it is an issue for the sink driver to sort this
out on an individual basis. Multiple sinks I would think implies
multiple perf buffers per intelPT, so each sink should have its own
buffer, and hence deal with it in a sink specific way?
Anyhow - as you say - something that can be deferred till we add the
multi-sink support.

Cheers

Mike

> > Suzuki




--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf

[Mathieu Poirier]

On Tue, May 26, 2020 at 11:46:41AM +0100, Mike Leach wrote:
> Add default sink selection to the perf trace handling in the etm driver.
> Uses the select default sink infrastructure to select a sink for the perf
> session, if no other sink is specified.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 84f1dcb69827..1a3169e69bb1 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>  		sink = coresight_get_enabled_sink(true);
>  	}
>  
> -	if (!sink)
> -		goto err;
> -
>  	mask = &event_data->mask;
>  
>  	/*
> @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>  			continue;
>  		}
>  
> +		/*
> +		 * No sink provided - look for a default sink for one of the
> +		 * devices. At present we only support topology where all CPUs
> +		 * use the same sink [N:1], so only need to find one sink. The
> +		 * coresight_build_path later will remove any CPU that does not
> +		 * attach to the sink, or if we have not found a sink.
> +		 */
> +		if (!sink)
> +			sink = coresight_find_default_sink(csdev);
> +
>  		/*
>  		 * Building a path doesn't enable it, it simply builds a
>  		 * list of devices from source to sink that can be
> @@ -267,6 +274,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>  		*etm_event_cpu_path_ptr(event_data, cpu) = path;
>  	}
>  
> +	/* no sink found for any CPU - cannot trace */
> +	if (!sink)
> +		goto err;
> +

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>  	/* If we don't have any CPUs ready for tracing, abort */
>  	cpu = cpumask_first(mask);
>  	if (cpu >= nr_cpu_ids)
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 5/5] coresight: sysfs: Allow select default sink on source enable.

[Mike Leach]

When enabling a trace source using sysfs, allow the CoreSight system to
auto-select a default sink if none has been enabled by the user.

Uses the sink select algorithm that uses the default select priorities
set when sinks are registered with the system. At present this will
prefer ETR over ETB / ETF.

Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 drivers/hwtracing/coresight/coresight.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 7632d060e25d..bd1a52a65d00 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -965,8 +965,15 @@ int coresight_enable(struct coresight_device *csdev)
 	 */
 	sink = coresight_get_enabled_sink(false);
 	if (!sink) {
-		ret = -EINVAL;
-		goto out;
+		/* look for a default sink if nothing enabled */
+		sink = coresight_find_default_sink(csdev);
+		if (!sink) {
+			ret = -EINVAL;
+			goto out;
+		}
+		/* mark the default as enabled */
+		sink->activated = true;
+		dev_info(&sink->dev, "Enabled default sink.");
 	}
 
 	path = coresight_build_path(csdev, sink);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/5] coresight: sysfs: Allow select default sink on source enable.

[Suzuki K Poulose]

Hi Mike,

On 05/26/2020 11:46 AM, Mike Leach wrote:
> When enabling a trace source using sysfs, allow the CoreSight system to
> auto-select a default sink if none has been enabled by the user.
> 
> Uses the sink select algorithm that uses the default select priorities
> set when sinks are registered with the system. At present this will
> prefer ETR over ETB / ETF.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>   drivers/hwtracing/coresight/coresight.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 7632d060e25d..bd1a52a65d00 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -965,8 +965,15 @@ int coresight_enable(struct coresight_device *csdev)
>   	 */
>   	sink = coresight_get_enabled_sink(false);
>   	if (!sink) {
> -		ret = -EINVAL;
> -		goto out;
> +		/* look for a default sink if nothing enabled */
> +		sink = coresight_find_default_sink(csdev);
> +		if (!sink) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		/* mark the default as enabled */
> +		sink->activated = true;
> +		dev_info(&sink->dev, "Enabled default sink.");
>   	}

To be honest, I would drop this change and mandate that the
user enables the sink(s). There is no way to reliably match
which ETM is tracing to the sink above in case multiple ETMs
are being enabled, even with the above message. It is important
for sysfs mode, as the user must collect the trace data manually,
unlike the perf mode where the race data is collected and presented to
the user via memory buffers.

Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/5] coresight: sysfs: Allow select default sink on source enable.

[Mike Leach]

HI Suzuki,

On Tue, 2 Jun 2020 at 12:46, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Hi Mike,
>
> On 05/26/2020 11:46 AM, Mike Leach wrote:
> > When enabling a trace source using sysfs, allow the CoreSight system to
> > auto-select a default sink if none has been enabled by the user.
> >
> > Uses the sink select algorithm that uses the default select priorities
> > set when sinks are registered with the system. At present this will
> > prefer ETR over ETB / ETF.
> >
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> >   drivers/hwtracing/coresight/coresight.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > index 7632d060e25d..bd1a52a65d00 100644
> > --- a/drivers/hwtracing/coresight/coresight.c
> > +++ b/drivers/hwtracing/coresight/coresight.c
> > @@ -965,8 +965,15 @@ int coresight_enable(struct coresight_device *csdev)
> >        */
> >       sink = coresight_get_enabled_sink(false);
> >       if (!sink) {
> > -             ret = -EINVAL;
> > -             goto out;
> > +             /* look for a default sink if nothing enabled */
> > +             sink = coresight_find_default_sink(csdev);
> > +             if (!sink) {
> > +                     ret = -EINVAL;
> > +                     goto out;
> > +             }
> > +             /* mark the default as enabled */
> > +             sink->activated = true;
> > +             dev_info(&sink->dev, "Enabled default sink.");
> >       }
>
> To be honest, I would drop this change and mandate that the
> user enables the sink(s).

This is here to make it easy for users to explore CoreSight using
sysfs - which is what tends to happen when they first start using it.
Also useful for generic test scripts if no sink is needed to check if
the ETM is working.


> There is no way to reliably match
> which ETM is tracing to the sink above in case multiple ETMs
> are being enabled, even with the above message.

I can't imagine users setting multiple sinks, either through the
default route or the explicit set route. Either way the problem is the
same - which sink does the etm hit?
I think this means that we need to improve this rather than drop it.
The default sink could easily be read by from the ETM via sysfs. as
could an addition to provide a last used sink for sysfs.
This would be really useful in developing test scripts that exercised
ETMs on a system without having to figure out the sink used. Such
scripts would then be more portable.

Regards

Mike


>It is important
> for sysfs mode, as the user must collect the trace data manually,
> unlike the perf mode where the race data is collected and presented to
> the user via memory buffers.
>
> Suzuki



--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel