[01/10] ipa: libipa: Allow creation of empty Histogram
diff mbox series

Message ID 20240322131451.3092931-2-dan.scally@ideasonboard.com
State Superseded
Headers show
Series
  • Centralise Agc into libipa
Related show

Commit Message

Dan Scally March 22, 2024, 1:14 p.m. UTC
For convenience's sake allow the creation of empty Histograms so
they can be embedded within other Classes and filled out with
data at some later point in time.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/ipa/libipa/histogram.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Stefan Klug March 25, 2024, 10:19 a.m. UTC | #1
Hi Dan,

thank you for the patch.

On Fri, Mar 22, 2024 at 01:14:42PM +0000, Daniel Scally wrote:
> For convenience's sake allow the creation of empty Histograms so
> they can be embedded within other Classes and filled out with
> data at some later point in time.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  src/ipa/libipa/histogram.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h
> index 05bb4b80..db13c155 100644
> --- a/src/ipa/libipa/histogram.h
> +++ b/src/ipa/libipa/histogram.h
> @@ -22,6 +22,7 @@ namespace ipa {
>  class Histogram
>  {
>  public:
> +	Histogram() { cumulative_.push_back(0); };

The second semicolon is not necessary (CI will check that).

With that fixed:

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers Stefan

>  	Histogram(Span<const uint32_t> data);
>  	size_t bins() const { return cumulative_.size() - 1; }
>  	uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }
> -- 
> 2.34.1
>
Jacopo Mondi March 25, 2024, 3:45 p.m. UTC | #2
Hi Dan

On Fri, Mar 22, 2024 at 01:14:42PM +0000, Daniel Scally wrote:
> For convenience's sake allow the creation of empty Histograms so
> they can be embedded within other Classes and filled out with
> data at some later point in time.
>

How are you going to add data to the Histogram since they seem to be
expected to be passed in at construction time ?

> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  src/ipa/libipa/histogram.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h
> index 05bb4b80..db13c155 100644
> --- a/src/ipa/libipa/histogram.h
> +++ b/src/ipa/libipa/histogram.h
> @@ -22,6 +22,7 @@ namespace ipa {
>  class Histogram
>  {
>  public:
> +	Histogram() { cumulative_.push_back(0); };
>  	Histogram(Span<const uint32_t> data);
>  	size_t bins() const { return cumulative_.size() - 1; }
>  	uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }
> --
> 2.34.1
>
Dan Scally March 25, 2024, 6:42 p.m. UTC | #3
Hi Jacopo

On 25/03/2024 15:45, Jacopo Mondi wrote:
> Hi Dan
>
> On Fri, Mar 22, 2024 at 01:14:42PM +0000, Daniel Scally wrote:
>> For convenience's sake allow the creation of empty Histograms so
>> they can be embedded within other Classes and filled out with
>> data at some later point in time.
>>
> How are you going to add data to the Histogram since they seem to be
> expected to be passed in at construction time ?


See for example patch #8, with:


+ hist_ = Histogram(Span<const uint32_t>(params->hist.hist_bins, + context.hw->numHistogramBins));

>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>>   src/ipa/libipa/histogram.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h
>> index 05bb4b80..db13c155 100644
>> --- a/src/ipa/libipa/histogram.h
>> +++ b/src/ipa/libipa/histogram.h
>> @@ -22,6 +22,7 @@ namespace ipa {
>>   class Histogram
>>   {
>>   public:
>> +	Histogram() { cumulative_.push_back(0); };
>>   	Histogram(Span<const uint32_t> data);
>>   	size_t bins() const { return cumulative_.size() - 1; }
>>   	uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }
>> --
>> 2.34.1
>>
Laurent Pinchart April 5, 2024, 9:30 p.m. UTC | #4
Hi Dan,

Thank you for the patch.

On Mon, Mar 25, 2024 at 06:42:32PM +0000, Daniel Scally wrote:
> On 25/03/2024 15:45, Jacopo Mondi wrote:
> > On Fri, Mar 22, 2024 at 01:14:42PM +0000, Daniel Scally wrote:
> >> For convenience's sake allow the creation of empty Histograms so
> >> they can be embedded within other Classes and filled out with
> >> data at some later point in time.
> >>
> > How are you going to add data to the Histogram since they seem to be
> > expected to be passed in at construction time ?
> 
> See for example patch #8, with:
> 
> + hist_ = Histogram(Span<const uint32_t>(params->hist.hist_bins, + context.hw->numHistogramBins));
> 
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >>   src/ipa/libipa/histogram.h | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h
> >> index 05bb4b80..db13c155 100644
> >> --- a/src/ipa/libipa/histogram.h
> >> +++ b/src/ipa/libipa/histogram.h
> >> @@ -22,6 +22,7 @@ namespace ipa {
> >>   class Histogram
> >>   {
> >>   public:
> >> +	Histogram() { cumulative_.push_back(0); };

Apart from the missing documentation, this looks OK.

> >>   	Histogram(Span<const uint32_t> data);
> >>   	size_t bins() const { return cumulative_.size() - 1; }
> >>   	uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }
Paul Elder April 8, 2024, 7:04 a.m. UTC | #5
Hi Dan,

On Fri, Mar 22, 2024 at 01:14:42PM +0000, Daniel Scally wrote:
> For convenience's sake allow the creation of empty Histograms so
> they can be embedded within other Classes and filled out with
> data at some later point in time.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/libipa/histogram.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h
> index 05bb4b80..db13c155 100644
> --- a/src/ipa/libipa/histogram.h
> +++ b/src/ipa/libipa/histogram.h
> @@ -22,6 +22,7 @@ namespace ipa {
>  class Histogram
>  {
>  public:
> +	Histogram() { cumulative_.push_back(0); };
>  	Histogram(Span<const uint32_t> data);
>  	size_t bins() const { return cumulative_.size() - 1; }
>  	uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }
> -- 
> 2.34.1
>

Patch
diff mbox series

diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h
index 05bb4b80..db13c155 100644
--- a/src/ipa/libipa/histogram.h
+++ b/src/ipa/libipa/histogram.h
@@ -22,6 +22,7 @@  namespace ipa {
 class Histogram
 {
 public:
+	Histogram() { cumulative_.push_back(0); };
 	Histogram(Span<const uint32_t> data);
 	size_t bins() const { return cumulative_.size() - 1; }
 	uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }