diff mbox series

[i-g-t,1/3] intel_gpu_top: Do not repeat header lines in non-interactive output

Message ID 20230203111636.4138202-2-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series More intel_gpu_top improvements | expand

Commit Message

Tvrtko Ursulin Feb. 3, 2023, 11:16 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

If output is redirected to a file, or a pipe, lets not repeat the headers
because that can usually mean user is trying to parse the data later and
so repeated headers are a hindrance.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Caleb Callaway <caleb.callaway@intel.com>
---
 tools/intel_gpu_top.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Kamil Konieczny Feb. 7, 2023, 1:25 p.m. UTC | #1 
On 2023-02-03 at 11:16:34 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> If output is redirected to a file, or a pipe, lets not repeat the headers
> because that can usually mean user is trying to parse the data later and
> so repeated headers are a hindrance.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Caleb Callaway <caleb.callaway@intel.com>

Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  tools/intel_gpu_top.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 0a1de41b3374..e2a7f4753099 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -1391,6 +1391,7 @@ static unsigned int stdout_level;
>  
>  #define STDOUT_HEADER_REPEAT 20
>  static unsigned int stdout_lines = STDOUT_HEADER_REPEAT;
> +static bool stdout_header_repeat;
>  
>  static void
>  stdout_open_struct(const char *name)
> @@ -1580,16 +1581,22 @@ static const struct print_operations term_pops = {
>  
>  static bool print_groups(struct cnt_group **groups)
>  {
> -	unsigned int headers = stdout_lines % STDOUT_HEADER_REPEAT + 1;
> +	static bool headers_printed = false;
>  	bool print_data = true;
>  
> -	if (output_mode == STDOUT && (headers == 1 || headers == 2)) {
> -		for (struct cnt_group **grp = groups; *grp; grp++)
> -			print_data = pops->print_group(*grp, headers);
> +	if (output_mode == STDOUT &&
> +	    (stdout_header_repeat || !headers_printed)) {
> +		unsigned int headers = stdout_lines % STDOUT_HEADER_REPEAT + 1;
> +
> +		if (headers == 1 || headers == 2)
> +			for (struct cnt_group **grp = groups; *grp; grp++)
> +				print_data = pops->print_group(*grp, headers);
> +
> +		headers_printed = print_data;
>  	}
>  
>  	for (struct cnt_group **grp = groups; print_data && *grp; grp++)
> -		pops->print_group(*grp, false);
> +		pops->print_group(*grp, 0);
>  
>  	return print_data;
>  }
> @@ -2512,6 +2519,8 @@ int main(int argc, char **argv)
>  		out = stdout;
>  	}
>  
> +	stdout_header_repeat = output_mode == STDOUT && isatty(fileno(out));
> +
>  	if (signal(SIGINT, sigint_handler) == SIG_ERR)
>  		fprintf(stderr, "Failed to install signal handler!\n");
>  
> -- 
> 2.34.1
>
Kamil Konieczny Feb. 8, 2023, 12:31 p.m. UTC | #2 
Hi Tvrtko,

one small nit, see below.

On 2023-02-03 at 11:16:34 +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> If output is redirected to a file, or a pipe, lets not repeat the headers
> because that can usually mean user is trying to parse the data later and
> so repeated headers are a hindrance.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Caleb Callaway <caleb.callaway@intel.com>
> ---
>  tools/intel_gpu_top.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 0a1de41b3374..e2a7f4753099 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -1391,6 +1391,7 @@ static unsigned int stdout_level;
>  
>  #define STDOUT_HEADER_REPEAT 20
>  static unsigned int stdout_lines = STDOUT_HEADER_REPEAT;
> +static bool stdout_header_repeat;
>  
>  static void
>  stdout_open_struct(const char *name)
> @@ -1580,16 +1581,22 @@ static const struct print_operations term_pops = {
>  
>  static bool print_groups(struct cnt_group **groups)
>  {
> -	unsigned int headers = stdout_lines % STDOUT_HEADER_REPEAT + 1;
> +	static bool headers_printed = false;
----------------------------------- ^
Remove this initialization (use checkpatch from Linux kernel).

Please correct and resend (you can keep my r-b).

Regards,
Kamil

>  	bool print_data = true;
>  
> -	if (output_mode == STDOUT && (headers == 1 || headers == 2)) {
> -		for (struct cnt_group **grp = groups; *grp; grp++)
> -			print_data = pops->print_group(*grp, headers);
> +	if (output_mode == STDOUT &&
> +	    (stdout_header_repeat || !headers_printed)) {
> +		unsigned int headers = stdout_lines % STDOUT_HEADER_REPEAT + 1;
> +
> +		if (headers == 1 || headers == 2)
> +			for (struct cnt_group **grp = groups; *grp; grp++)
> +				print_data = pops->print_group(*grp, headers);
> +
> +		headers_printed = print_data;
>  	}
>  
>  	for (struct cnt_group **grp = groups; print_data && *grp; grp++)
> -		pops->print_group(*grp, false);
> +		pops->print_group(*grp, 0);
>  
>  	return print_data;
>  }
> @@ -2512,6 +2519,8 @@ int main(int argc, char **argv)
>  		out = stdout;
>  	}
>  
> +	stdout_header_repeat = output_mode == STDOUT && isatty(fileno(out));
> +
>  	if (signal(SIGINT, sigint_handler) == SIG_ERR)
>  		fprintf(stderr, "Failed to install signal handler!\n");
>  
> -- 
> 2.34.1
>
Tvrtko Ursulin Feb. 9, 2023, 8:24 a.m. UTC | #3 
On 08/02/2023 12:31, Kamil Konieczny wrote:
> Hi Tvrtko,
> 
> one small nit, see below.
> 
> On 2023-02-03 at 11:16:34 +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> If output is redirected to a file, or a pipe, lets not repeat the headers
>> because that can usually mean user is trying to parse the data later and
>> so repeated headers are a hindrance.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Caleb Callaway <caleb.callaway@intel.com>
>> ---
>>   tools/intel_gpu_top.c | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>> index 0a1de41b3374..e2a7f4753099 100644
>> --- a/tools/intel_gpu_top.c
>> +++ b/tools/intel_gpu_top.c
>> @@ -1391,6 +1391,7 @@ static unsigned int stdout_level;
>>   
>>   #define STDOUT_HEADER_REPEAT 20
>>   static unsigned int stdout_lines = STDOUT_HEADER_REPEAT;
>> +static bool stdout_header_repeat;
>>   
>>   static void
>>   stdout_open_struct(const char *name)
>> @@ -1580,16 +1581,22 @@ static const struct print_operations term_pops = {
>>   
>>   static bool print_groups(struct cnt_group **groups)
>>   {
>> -	unsigned int headers = stdout_lines % STDOUT_HEADER_REPEAT + 1;
>> +	static bool headers_printed = false;
> ----------------------------------- ^
> Remove this initialization (use checkpatch from Linux kernel).
> 
> Please correct and resend (you can keep my r-b).

Fixed and pushed, thanks for the review!

Regards,

Tvrtko
diff mbox series

Patch

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 0a1de41b3374..e2a7f4753099 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -1391,6 +1391,7 @@  static unsigned int stdout_level;
 
 #define STDOUT_HEADER_REPEAT 20
 static unsigned int stdout_lines = STDOUT_HEADER_REPEAT;
+static bool stdout_header_repeat;
 
 static void
 stdout_open_struct(const char *name)
@@ -1580,16 +1581,22 @@  static const struct print_operations term_pops = {
 
 static bool print_groups(struct cnt_group **groups)
 {
-	unsigned int headers = stdout_lines % STDOUT_HEADER_REPEAT + 1;
+	static bool headers_printed = false;
 	bool print_data = true;
 
-	if (output_mode == STDOUT && (headers == 1 || headers == 2)) {
-		for (struct cnt_group **grp = groups; *grp; grp++)
-			print_data = pops->print_group(*grp, headers);
+	if (output_mode == STDOUT &&
+	    (stdout_header_repeat || !headers_printed)) {
+		unsigned int headers = stdout_lines % STDOUT_HEADER_REPEAT + 1;
+
+		if (headers == 1 || headers == 2)
+			for (struct cnt_group **grp = groups; *grp; grp++)
+				print_data = pops->print_group(*grp, headers);
+
+		headers_printed = print_data;
 	}
 
 	for (struct cnt_group **grp = groups; print_data && *grp; grp++)
-		pops->print_group(*grp, false);
+		pops->print_group(*grp, 0);
 
 	return print_data;
 }
@@ -2512,6 +2519,8 @@  int main(int argc, char **argv)
 		out = stdout;
 	}
 
+	stdout_header_repeat = output_mode == STDOUT && isatty(fileno(out));
+
 	if (signal(SIGINT, sigint_handler) == SIG_ERR)
 		fprintf(stderr, "Failed to install signal handler!\n");