Re: [PATCH i-g-t v4 01/13] lib/sw_sync: Add helper functions for managing synchronization primitives

[Gustavo Padovan <gustavo.padovan@collabora.com>]

2016-09-15 Robert Foss <robert.foss@collabora.com>:

> 
> 
> On 2016-09-15 02:35 PM, Robert Foss wrote:
> > 
> > 
> > On 2016-09-15 02:46 AM, Chris Wilson wrote:
> > > On Wed, Sep 14, 2016 at 11:04:30AM -0400, robert.foss@collabora.com
> > > wrote:
> > > > +void sw_sync_timeline_inc(int fd, uint32_t count)
> > > > +{
> > > > +    uint32_t arg = count;
> > > > +
> > > > +    if (fd == 0)
> > > > +        return;
> > > 
> > > But fd = 0 is a valid fd, and might be a timeline somewhere.
> > > 
> > > Did you mean count == 0 ?
> > > 
> > > And even then (unless it is defined as an error condition in the kernel
> > > ABI, and it should not be...) we should pass it through to the kernel.
> > 
> > You're right, I'll change it in v5.
> > 
> > > 
> > > > +    do_ioctl(fd, SW_SYNC_IOC_INC, &arg);
> > > > +}
> > > > +
> > > 
> > > > +int sw_sync_wait(int fence, int timeout)
> > > > +{
> > > > +    struct pollfd fds;
> > > > +    int ret;
> > > > +
> > > > +    fds.fd = fence;
> > > > +    fds.events = POLLIN | POLLERR;
> > > 
> > > POLLERR is always implied and doesn't need to be specified (it is
> > > meaningless in .events).
> > > 
> > > int sw_sync_wait(int fence, int timeout)
> > > {
> > > #if BEING_FANCY
> > >     return poll(&(struct pollfd){fd, POLLIN}, 1, timeout);
> > > #else
> > >     struct pollfd pfd = { fd, POLLIN };
> > >     return poll(&pfd, 1, timeout);
> > > #endif
> > > }
> > > 
> > > Indentation has gone wrong, double check the whitespace.
> > 
> > That is definitely nicer looking. I'll drop it in for v5.
> > 
> > > 
> > > 
> > > How do fences operate after their timeline is closed? (Are they
> > > automatically signaled, or do they persist and are signaled normally?) Is
> > > there a test for using fences from a closed timeline (I was looking but
> > > didn't notice one).
> > 
> > I did some quick tests just to confirm, closing the timeline signals all
> > of its fences.
> 
> Actually, my quick test was wrong. A fence is _not_ signaled on when its
> timeline has been closed.
> 
> So you would like to see a test that confirms that a fence on closed
> timeline is not signaled?

Yes, please. Add this test and maybe another the signals the fence
before closing the timeline and the check if the fence is indeed
signaled.

Gustavo

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