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

[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?

>  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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev