Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 11 Dec 2018 08:39:05 +0800
From:   leo.yan@linaro.org
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Coresight ML <coresight@lists.linaro.org>,
        Mike Leach <mike.leach@linaro.org>,
        Robert Walker <robert.walker@arm.com>
Subject: Re: [PATCH v2 5/6] perf cs-etm: Treat EO_TRACE element as trace
 discontinuity
Message-ID: <20181211003905.GA21018@leoy-ThinkPad-X240s>
References: <1544431981-24144-1-git-send-email-leo.yan@linaro.org>
 <1544431981-24144-6-git-send-email-leo.yan@linaro.org>
 <20181210230445.GD12152@xps15>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181210230445.GD12152@xps15>
User-Agent: Mutt/1.10+31 (9cdd884) (2018-06-19)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mathieu,

On Mon, Dec 10, 2018 at 04:04:45PM -0700, Mathieu Poirier wrote:
> On Mon, Dec 10, 2018 at 04:53:00PM +0800, Leo Yan wrote:
> > If decoder outputs EO_TRACE element, it means the end of the trace
> > buffer; this is a discontinuity and in this case the end of trace data
> > needs to be saved.
> > 
> > This patch generates CS_ETM_DISCONTINUITY packet for EO_TRACE element
> > hereby flushing the end of trace data in cs-etm.c.
> > 
> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Cc: Mike Leach <mike.leach@linaro.org>
> > Cc: Robert Walker <robert.walker@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > index 46b67f1..bcb5c98 100644
> > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > @@ -411,6 +411,7 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> >  	case OCSD_GEN_TRC_ELEM_UNKNOWN:
> >  		break;
> >  	case OCSD_GEN_TRC_ELEM_NO_SYNC:
> > +	case OCSD_GEN_TRC_ELEM_EO_TRACE:
> >  		resp = cs_etm_decoder__buffer_discontinuity(decoder,
> >  							    trace_chan_id);
> 
> If you were to get rid of decoder::trace_on at the beginning of this set you
> could put NO_SYNC, EO_TRACE and TRACE_ON together and call
> cs_etm_decoder__buffer_discontinuity() only once.  I wouldn't mention it if you did
> not have to respin but since you do, might as well just do it.  But that's
> entirely up to you considering, at least in my opinion, that you have addressed
> all of Mike and Rob' comments.

Thanks for suggestion, this makes sense.  Will spin new patch series to
address this and also follow up other suggestions for this series.

Thanks,
Leo Yan

> If you do not want to deal with decoder::trace_on as part of this set:
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
> >  		decoder->trace_on = false;
> > @@ -431,7 +432,6 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
> >  		decoder->packet_buffer[decoder->tail].exc_ret = true;
> >  		break;
> >  	case OCSD_GEN_TRC_ELEM_PE_CONTEXT:
> > -	case OCSD_GEN_TRC_ELEM_EO_TRACE:
> >  	case OCSD_GEN_TRC_ELEM_ADDR_NACC:
> >  	case OCSD_GEN_TRC_ELEM_TIMESTAMP:
> >  	case OCSD_GEN_TRC_ELEM_CYCLE_COUNT:
> > -- 
> > 2.7.4
> >