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 24-05-18 om 03:05 schreef Dhinakaran Pandiyan:
> On Tue, 2018-04-10 at 17:28 +0200, Maarten Lankhorst wrote:
>> 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@lin
>>>>>>>>> ux.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_dat
>>>>>> a_count(cr
>>>>>> c),
>>>>>> -						crc->lock);
>>>>>> +	ret = wait_event_interruptible_lock_irq_timeout(crc-
>>>>>>> wq,
>>>>>> +							crtc
>>>>>> _crc_data_
>>>>>> count(crc),
>>>>>> +							crc-
>>>>>>> lock,
>>>>>> +							msec
>>>>>> s_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...
> We still have to fix this to enable PSR.
>
> The kernel has to disable PSR for hardware to generate pipe CRC's but
> doing so changes the purpose of the tests, for e.g.,
> kms_frontbuffer_tracking:basic aims to check CRC's with default
> features.
>
> I am thinking we should return error from the kernel instead of
> blocking when we know that the HW cannot generate CRC. Secondly, we
> should add DDI_CRC_RES_SRD_[A, B, ...] sources for CRC generation and
> make the tests select them if needed.
>
> Let me know what you think.
>
>
>
>
+1, sounds good.

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