Re: [PATCH] tools/intel_guc_logger: Utility for capturing GuC firmware logs in a file

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 07/09/16 09:44, Chris Wilson wrote:
> On Wed, Sep 07, 2016 at 01:40:27PM +0530, Goel, Akash wrote:
>> On 9/6/2016 9:22 PM, Tvrtko Ursulin wrote:

[snip]

>>>>>> +    while (!stop_logging)
>>>>>> +    {
>>>>>> +        if (test_duration && (igt_seconds_elapsed(&start) >
>>>>>> test_duration)) {
>>>>>
>>>>> If you agree to allow no poll period the this would not work right? In
>>>>> that case you would need to use alarm(2) or something.
>>>>>
>>>>
>>>> Can calculate the timeout value for poll call as,
>>>>      if (poll_timeout < 0) {
>>>>          timeout = test_duration - igt_seconds_elapsed(&start))
>>>>      }
>>>
>>> My point was that with indefinite poll loop will not run if there is not
>>> log data so timeout will not work implemented like this.
>>>
>> I understood your concern in first place but probably didn't put
>> forth my point clearly.
>>
>> For more clarity, this is how think it can be addressed.
>>
>> --- a/tools/intel_guc_logger.c
>> +++ b/tools/intel_guc_logger.c
>> @@ -370,6 +370,8 @@ int main(int argc, char **argv)
>>   {
>>   	struct pollfd relay_poll_fd;
>>   	struct timespec start={};
>> +	uint32_t time_elapsed;
>> +	int timeout;
>>   	int nfds;
>>   	int ret;
>>
>> @@ -395,10 +397,17 @@ int main(int argc, char **argv)
>>
>>   	while (!stop_logging)
>>   	{
>> -		if (test_duration && (igt_seconds_elapsed(&start) > test_duration)) {
>> -			igt_debug("Ran for stipulated %d seconds, exit now\n", test_duration);
>> -			stop_logging = true;
>> -			break;
>> +		timeout = poll_timeout;
>> +		if (test_duration) {
>> +			time_elapsed = igt_seconds_elapsed(&start);
>> +			if (time_elapsed >= test_duration) {
>> +				igt_debug("Ran for stipulated %d seconds, exit now\n", test_duration);
>> +				stop_logging = true;
>> +				break;
>> +			}
>> +			if (poll_timeout < 0)
>> +				timeout = (test_duration - time_elapsed) * 1000;
>> 		}
>>
>>   		/* Wait/poll for the new data to be available, relay doesn't
>> @@ -412,7 +421,7 @@ int main(int argc, char **argv)
>>   		 * than a jiffy gap between 2 flush interrupts) and relay runs
>>   		 * out of sub buffers to store the new logs.
>>   		 */
>> -		ret = poll(&relay_poll_fd, nfds, poll_timeout);
>> +		ret = poll(&relay_poll_fd, nfds, timeout);
>>   		if (ret < 0) {
>>   			if (errno == EINTR)
>>   				break;
>>
>> So will not do polling with indefinite timeout and adjust the
>> timeout value as per test's duration.
>> Does it look ok ?
>
> Since the comment still refers to a kernel bug that you've fixed, it can
> just go. The timeout calculation is indeed more simply expressed as
> alarm(timeout).

Yes I wrote privately that's especially true since there is already a 
handler for SIGINT which would do the right thing for SIGALRM as well. I 
don't feel so strongly about this but now that we both think the same 
maybe go for the simpler implementation if you don't mind Akash?

> And fixing the blocking read() is about 10 lines in the kernel...

Haven't checked but if that is the case, since we are already fixing 
relayfs issues, it would be good to do that one as well since it would 
simplify the logger. Because if we do it straight away then we know 
logger can use it, and if we leave it for later then it gets uglier for 
the logger.

But if we cannot make the fix go in the same kernel version (or earlier) 
than the GuC logging then I think we don't need to block on that.

Regards,

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