| Message ID | 20220527144447.94891-19-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, Thank you for the patch. On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote: > The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to s/current/currently/ > see that cm.get_ready_requests() returns something. This is not always > the case, as there may be two eventfd events queued, and the first call > to cm.get_ready_requests() returns all the requests, and thus the second > call returns none. > > Remove the self.assertTrue(len(ready_reqs) > 0) assert. This convinces me even more that there's room for improvement :-) It seems a bit confusing for applications. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > test/py/unittests.py | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/test/py/unittests.py b/test/py/unittests.py > index 45b35223..33b35a0a 100755 > --- a/test/py/unittests.py > +++ b/test/py/unittests.py > @@ -287,8 +287,6 @@ class SimpleCaptureMethods(CameraTesterBase): > > ready_reqs = cm.get_ready_requests() > > - self.assertTrue(len(ready_reqs) > 0) > - > reqs += ready_reqs > > if len(reqs) == num_bufs:
On 27/05/2022 22:09, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote: >> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to > > s/current/currently/ > >> see that cm.get_ready_requests() returns something. This is not always >> the case, as there may be two eventfd events queued, and the first call >> to cm.get_ready_requests() returns all the requests, and thus the second >> call returns none. The above is actually not quite true. There are no "eventfd events", just a value, and if that value is > 0, the eventfd is readable. So what happens here is that the user calls read_event(), which clear that value. Then, before the user calls get_ready_requests(), there's a request-complete event, and the bindings increase the value. And now, when the user calls get_ready_requests(), it clears all the requests, but the eventfd value is 1 and thus the select on the eventfd will trigger immediately again, but there are no requests in the list. >> Remove the self.assertTrue(len(ready_reqs) > 0) assert. > > This convinces me even more that there's room for improvement :-) It > seems a bit confusing for applications. I'm open to suggestions =). I like the current method as it's very straightforward, just one mutex which is only kept when adding a Request to the gReqList, or swap()'ing the gReqList to a local var. And the read_event() and get_ready_requests() should allow the caller to handle the event loop as he wishes, both blocking and non-blocking. Now, we do always call read_event() and get_ready_requests() together in our .py scripts. So perhaps we should merge them, and add new separate functions if someone ever needs them. However, it won't change the problem solved in this patch in any way. We could also change the merged function so that the mutex is kept while read() is called and the gReqList is swap()'d. This would solve the problem fixed in this patch. However, it would mean that we'd possibly be blocked in read() while keeping the mutex and that would cause a deadlock. Anyway, my opinion is that it's not an issue if there's an event from libcamera but get_ready_requests() doesn't return anything. Tomi
Hi Tomi, On Mon, May 30, 2022 at 10:01:11AM +0300, Tomi Valkeinen wrote: > On 27/05/2022 22:09, Laurent Pinchart wrote: > > On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote: > >> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to > > > > s/current/currently/ > > > >> see that cm.get_ready_requests() returns something. This is not always > >> the case, as there may be two eventfd events queued, and the first call > >> to cm.get_ready_requests() returns all the requests, and thus the second > >> call returns none. > > The above is actually not quite true. There are no "eventfd events", > just a value, and if that value is > 0, the eventfd is readable. > > So what happens here is that the user calls read_event(), which clear > that value. Then, before the user calls get_ready_requests(), there's a > request-complete event, and the bindings increase the value. And now, > when the user calls get_ready_requests(), it clears all the requests, > but the eventfd value is 1 and thus the select on the eventfd will > trigger immediately again, but there are no requests in the list. > > >> Remove the self.assertTrue(len(ready_reqs) > 0) assert. > > > > This convinces me even more that there's room for improvement :-) It > > seems a bit confusing for applications. > > I'm open to suggestions =). > > I like the current method as it's very straightforward, just one mutex > which is only kept when adding a Request to the gReqList, or swap()'ing > the gReqList to a local var. And the read_event() and > get_ready_requests() should allow the caller to handle the event loop as > he wishes, both blocking and non-blocking. > > Now, we do always call read_event() and get_ready_requests() together in > our .py scripts. So perhaps we should merge them, and add new separate > functions if someone ever needs them. However, it won't change the > problem solved in this patch in any way. > > We could also change the merged function so that the mutex is kept while > read() is called and the gReqList is swap()'d. This would solve the > problem fixed in this patch. However, it would mean that we'd possibly > be blocked in read() while keeping the mutex and that would cause a > deadlock. > > Anyway, my opinion is that it's not an issue if there's an event from > libcamera but get_ready_requests() doesn't return anything. I've expressed myself incorrectly. This is totally fine, what I'd like to see removed is the read_event() function. What happens if you call read_event) in get_ready_requests() ?
On 30/05/2022 11:25, Laurent Pinchart wrote: > Hi Tomi, > > On Mon, May 30, 2022 at 10:01:11AM +0300, Tomi Valkeinen wrote: >> On 27/05/2022 22:09, Laurent Pinchart wrote: >>> On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote: >>>> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to >>> >>> s/current/currently/ >>> >>>> see that cm.get_ready_requests() returns something. This is not always >>>> the case, as there may be two eventfd events queued, and the first call >>>> to cm.get_ready_requests() returns all the requests, and thus the second >>>> call returns none. >> >> The above is actually not quite true. There are no "eventfd events", >> just a value, and if that value is > 0, the eventfd is readable. >> >> So what happens here is that the user calls read_event(), which clear >> that value. Then, before the user calls get_ready_requests(), there's a >> request-complete event, and the bindings increase the value. And now, >> when the user calls get_ready_requests(), it clears all the requests, >> but the eventfd value is 1 and thus the select on the eventfd will >> trigger immediately again, but there are no requests in the list. >> >>>> Remove the self.assertTrue(len(ready_reqs) > 0) assert. >>> >>> This convinces me even more that there's room for improvement :-) It >>> seems a bit confusing for applications. >> >> I'm open to suggestions =). >> >> I like the current method as it's very straightforward, just one mutex >> which is only kept when adding a Request to the gReqList, or swap()'ing >> the gReqList to a local var. And the read_event() and >> get_ready_requests() should allow the caller to handle the event loop as >> he wishes, both blocking and non-blocking. >> >> Now, we do always call read_event() and get_ready_requests() together in >> our .py scripts. So perhaps we should merge them, and add new separate >> functions if someone ever needs them. However, it won't change the >> problem solved in this patch in any way. >> >> We could also change the merged function so that the mutex is kept while >> read() is called and the gReqList is swap()'d. This would solve the >> problem fixed in this patch. However, it would mean that we'd possibly >> be blocked in read() while keeping the mutex and that would cause a >> deadlock. >> >> Anyway, my opinion is that it's not an issue if there's an event from >> libcamera but get_ready_requests() doesn't return anything. > > I've expressed myself incorrectly. This is totally fine, what I'd like > to see removed is the read_event() function. What happens if you call > read_event) in get_ready_requests() ? read_event() blocks until there is an "event". In our current .py files this is fine, because in all the cases we either 1) call read_event() & get_ready_requests() because select indicates that there is an event, or 2) we want the read_event() to block if there's no event. I think the only case where merging read_event() and get_ready_requests() would cause a problem is if you want to do polling. At the moment you can just call get_ready_requests() and it never blocks and returns any requests that have been completed. Then again, even with a merged function the above could be accomplished if the user changes the eventfd to non-blocking mode in the application code. With that change, read_event() never blocks, although at the moment it throws an exception if read() fails (because at the moment read() should never fail). We should probably then change it to return a ret val instead. But I don't really know if polling is a use case we're interested in supporting. Even blocking read feels a bit pointless. I think in all but the most simple examples you want to properly wait on the fd, instead of polling or blocking. Especially as it's trivial to do with Python. Tomi
Hi Tomi, On Mon, May 30, 2022 at 11:36:52AM +0300, Tomi Valkeinen wrote: > On 30/05/2022 11:25, Laurent Pinchart wrote: > > On Mon, May 30, 2022 at 10:01:11AM +0300, Tomi Valkeinen wrote: > >> On 27/05/2022 22:09, Laurent Pinchart wrote: > >>> On Fri, May 27, 2022 at 05:44:35PM +0300, Tomi Valkeinen wrote: > >>>> The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to > >>> > >>> s/current/currently/ > >>> > >>>> see that cm.get_ready_requests() returns something. This is not always > >>>> the case, as there may be two eventfd events queued, and the first call > >>>> to cm.get_ready_requests() returns all the requests, and thus the second > >>>> call returns none. > >> > >> The above is actually not quite true. There are no "eventfd events", > >> just a value, and if that value is > 0, the eventfd is readable. > >> > >> So what happens here is that the user calls read_event(), which clear > >> that value. Then, before the user calls get_ready_requests(), there's a > >> request-complete event, and the bindings increase the value. And now, > >> when the user calls get_ready_requests(), it clears all the requests, > >> but the eventfd value is 1 and thus the select on the eventfd will > >> trigger immediately again, but there are no requests in the list. > >> > >>>> Remove the self.assertTrue(len(ready_reqs) > 0) assert. > >>> > >>> This convinces me even more that there's room for improvement :-) It > >>> seems a bit confusing for applications. > >> > >> I'm open to suggestions =). > >> > >> I like the current method as it's very straightforward, just one mutex > >> which is only kept when adding a Request to the gReqList, or swap()'ing > >> the gReqList to a local var. And the read_event() and > >> get_ready_requests() should allow the caller to handle the event loop as > >> he wishes, both blocking and non-blocking. > >> > >> Now, we do always call read_event() and get_ready_requests() together in > >> our .py scripts. So perhaps we should merge them, and add new separate > >> functions if someone ever needs them. However, it won't change the > >> problem solved in this patch in any way. > >> > >> We could also change the merged function so that the mutex is kept while > >> read() is called and the gReqList is swap()'d. This would solve the > >> problem fixed in this patch. However, it would mean that we'd possibly > >> be blocked in read() while keeping the mutex and that would cause a > >> deadlock. > >> > >> Anyway, my opinion is that it's not an issue if there's an event from > >> libcamera but get_ready_requests() doesn't return anything. > > > > I've expressed myself incorrectly. This is totally fine, what I'd like > > to see removed is the read_event() function. What happens if you call > > read_event) in get_ready_requests() ? > > read_event() blocks until there is an "event". In our current .py files > this is fine, because in all the cases we either 1) call read_event() & > get_ready_requests() because select indicates that there is an event, or > 2) we want the read_event() to block if there's no event. > > I think the only case where merging read_event() and > get_ready_requests() would cause a problem is if you want to do polling. > At the moment you can just call get_ready_requests() and it never blocks > and returns any requests that have been completed. > > Then again, even with a merged function the above could be accomplished > if the user changes the eventfd to non-blocking mode in the application > code. With that change, read_event() never blocks, although at the > moment it throws an exception if read() fails (because at the moment > read() should never fail). We should probably then change it to return a > ret val instead. > > But I don't really know if polling is a use case we're interested in > supporting. Even blocking read feels a bit pointless. I think in all but > the most simple examples you want to properly wait on the fd, instead of > polling or blocking. Especially as it's trivial to do with Python. Agreed :-)
diff --git a/test/py/unittests.py b/test/py/unittests.py index 45b35223..33b35a0a 100755 --- a/test/py/unittests.py +++ b/test/py/unittests.py @@ -287,8 +287,6 @@ class SimpleCaptureMethods(CameraTesterBase): ready_reqs = cm.get_ready_requests() - self.assertTrue(len(ready_reqs) > 0) - reqs += ready_reqs if len(reqs) == num_bufs:
The test_select() current uses self.assertTrue(len(ready_reqs) > 0) to see that cm.get_ready_requests() returns something. This is not always the case, as there may be two eventfd events queued, and the first call to cm.get_ready_requests() returns all the requests, and thus the second call returns none. Remove the self.assertTrue(len(ready_reqs) > 0) assert. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- test/py/unittests.py | 2 -- 1 file changed, 2 deletions(-)