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

[Maarten Lankhorst]

Op 06-04-18 om 20:27 schreef Dhinakaran Pandiyan:
>
>
> On Fri, 2018-04-06 at 09:33 +0200, Maarten Lankhorst wrote:
>> 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 at 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,
> Even if the kernel knows the hardware will not generate CRCs? That seems
> to be the case here, isn't it?
Hm, if kernel knew the hardware doesn't generate CRC's, then this should be fixed in the kernel, currently we have no way to skip tests based on that.
But presumably if we ask for CRC's, we should definitely try to generate CRC's...

Is it ok if I increase the timeout to 10s? It seems 5s is not enough on a haswell laptop, which has to enable the panel fitter as a workaround for CRC generation..

~Maarten