Re: [igt-dev] [PATCH i-g-t] lib/igt_debugfs: Add timeouts to opening pipe CRC fd.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 06-04-18 om 00:39 schreef Dhinakaran Pandiyan:
> On Thu, 2018-04-05 at 22:10 +0000, Souza, Jose wrote:
>> On Thu, 2018-04-05 at 22:15 +0100, Chris Wilson wrote:
>>> Quoting Souza, Jose (2018-04-05 22:03:46)
>>>> On Thu, 2018-04-05 at 12:49 +0200, Maarten Lankhorst wrote:
>>>>> This will fix the PSR tests to fail slightly faster, since they
>>>>> wait
>>>>> indefinitely for a CRC that never comes during open.
> Because the pipe is not active and hence doesn't generate crc's any
> more? The timeouts are needed but any idea how we should deal with the
> lack of CRCs? 
>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
>>>>> om>
>>>>> ---
>>>>>  lib/igt_debugfs.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
>>>>> index 8adc02e9cc47..094df564b6f4 100644
>>>>> --- a/lib/igt_debugfs.c
>>>>> +++ b/lib/igt_debugfs.c
>>>>> @@ -757,7 +757,10 @@ void igt_pipe_crc_start(igt_pipe_crc_t
>>>>> *pipe_crc)
>>>>>  
>>>>>       sprintf(buf, "crtc-%d/crc/data", pipe_crc->pipe);
>>>>>  
>>>>> +     igt_set_timeout(5, "Opening crc fd, which waits for first
>>>>> CRC.");
>>>>>       pipe_crc->crc_fd = openat(pipe_crc->dir, buf, pipe_crc-
>>>>>> flags);
>>>>> +     igt_reset_timeout();
>>>> Hum I was able to reproduce this one too in a KBL, I was thinking
>>>> in
>>>> add a timeout in the kernel side, if it don't get the CRC buffer
>>>> filled
>>>> it would return a error.
>>> That would be unexpected behaviour for a read() interface. If you
>>> want a
>>> timeout, implement poll().
>> Not change the read() but return a error in open() when timeout
>> happens:
>>
>> "
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
>> b/drivers/gpu/drm/drm_debugfs_crc.c
>> index 9f8312137cad..223cd45b3aff 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -206,9 +206,10 @@ static int crtc_crc_open(struct inode *inode,
>> struct file *filep)
>>  	 * guess when this particular piece of HW will be ready to
>> start
>>  	 * generating CRCs.
>>  	 */
>> -	ret = wait_event_interruptible_lock_irq(crc->wq,
>> -						crtc_crc_data_count(cr
>> c),
>> -						crc->lock);
>> +	ret = wait_event_interruptible_lock_irq_timeout(crc->wq,
>> +							crtc_crc_data_
>> count(crc),
>> +							crc->lock,
>> +							msecs_to_jiffi
>> es(2500));
>>  	spin_unlock_irq(&crc->lock);
>>  
>>  	if (ret)

I think this should be fixed in userspace, and the fix of doing a
timeout in igt_pipe_crc_start should work. The kernel side is correct
to block forever, and this helped us caught bugs when igt/debugfs_test
was reading /sys/kernel/debug/dri/0/crtc-0/crc/data and being blocked
until the test hit its internal timeout (10s), we would lose that
check if kernel times out sooner.

On your patch itself, I think the _timeout function returns a long with
the remainder of the timeout, and 0 for timed out, the if (ret) below
wouldn't work any more in that case.


~Maarten
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev