| Message ID | 20190508165814.26201-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On 08/05/2019 17:58, Niklas Söderlund wrote: > When initializing the test reset any media links and set a know > resolutions. This is needed to put the device under test into known > state and not have the v4l2 device tests depend on that no one have > touched the device before the test is executed. > > The resolution is picked purely at random and could possibly be moved to > each test case if there is a need for different resolutions for a > specific one. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > test/v4l2_device/v4l2_device_test.cpp | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp > index ee5a8e009bef2a5e..5bd80a6c68d796b6 100644 > --- a/test/v4l2_device/v4l2_device_test.cpp > +++ b/test/v4l2_device/v4l2_device_test.cpp > @@ -54,7 +54,24 @@ int V4L2DeviceTest::init() > if (!capture_) > return TestFail; > > - return capture_->open(); > + media_->acquire(); Should you check the return code of acquire? Does it block using lockf() internally? > + if (media_->disableLinks()) Where do we recreate / re-enable the links? or does it reset to a sane default? > + return TestFail; Is it an issue here that the media_ is not released? I now can't recall if the acquire/release is about locking globally using lockf() or if it's just a state machine state change. If it must be released - perhaps we should do some sort of auto-lock release by using the destructor of a local object, so that it releases when it goes out of scope. > + media_->release(); > + > + if (capture_->open()) > + return TestFail; > + > + V4L2DeviceFormat format = {}; > + if (capture_->getFormat(&format)) > + return TestFail; > + > + format.size.width = 640; > + format.size.height = 480; > + if (capture_->setFormat(&format)) > + return TestFail; > + > + return TestPass; > } > > void V4L2DeviceTest::cleanup() >
Hello, On Thu, May 09, 2019 at 10:30:41AM +0100, Kieran Bingham wrote: > On 08/05/2019 17:58, Niklas Söderlund wrote: > > When initializing the test reset any media links and set a know > > resolutions. This is needed to put the device under test into known > > state and not have the v4l2 device tests depend on that no one have > > touched the device before the test is executed. > > > > The resolution is picked purely at random and could possibly be moved to > > each test case if there is a need for different resolutions for a > > specific one. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > test/v4l2_device/v4l2_device_test.cpp | 19 ++++++++++++++++++- > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp > > index ee5a8e009bef2a5e..5bd80a6c68d796b6 100644 > > --- a/test/v4l2_device/v4l2_device_test.cpp > > +++ b/test/v4l2_device/v4l2_device_test.cpp > > @@ -54,7 +54,24 @@ int V4L2DeviceTest::init() > > if (!capture_) > > return TestFail; > > > > - return capture_->open(); > > + media_->acquire(); > > Should you check the return code of acquire? > > Does it block using lockf() internally? It won't block, it will return an error, so I think it's a good idea to check for that. > > + if (media_->disableLinks()) > > Where do we recreate / re-enable the links? > > or does it reset to a sane default? It won't enable links, so any test depending on this would have to enable links manually. > > + return TestFail; > > Is it an issue here that the media_ is not released? I now can't recall > if the acquire/release is about locking globally using lockf() or if > it's just a state machine state change. > > If it must be released - perhaps we should do some sort of auto-lock > release by using the destructor of a local object, so that it releases > when it goes out of scope. lockf() will not persist once the file descriptor is closed, so it shouldn't be a problem, but proper error handling is still a good idea. With those issues fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + media_->release(); > > + > > + if (capture_->open()) > > + return TestFail; > > + > > + V4L2DeviceFormat format = {}; > > + if (capture_->getFormat(&format)) > > + return TestFail; > > + > > + format.size.width = 640; > > + format.size.height = 480; > > + if (capture_->setFormat(&format)) > > + return TestFail; > > + > > + return TestPass; > > } > > > > void V4L2DeviceTest::cleanup()
Hi Kieran, Laurent, Thanks for your feedback. On 2019-05-11 05:27:31 +0300, Laurent Pinchart wrote: > Hello, > > On Thu, May 09, 2019 at 10:30:41AM +0100, Kieran Bingham wrote: > > On 08/05/2019 17:58, Niklas Söderlund wrote: > > > When initializing the test reset any media links and set a know > > > resolutions. This is needed to put the device under test into known > > > state and not have the v4l2 device tests depend on that no one have > > > touched the device before the test is executed. > > > > > > The resolution is picked purely at random and could possibly be moved to > > > each test case if there is a need for different resolutions for a > > > specific one. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > test/v4l2_device/v4l2_device_test.cpp | 19 ++++++++++++++++++- > > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > > > diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp > > > index ee5a8e009bef2a5e..5bd80a6c68d796b6 100644 > > > --- a/test/v4l2_device/v4l2_device_test.cpp > > > +++ b/test/v4l2_device/v4l2_device_test.cpp > > > @@ -54,7 +54,24 @@ int V4L2DeviceTest::init() > > > if (!capture_) > > > return TestFail; > > > > > > - return capture_->open(); > > > + media_->acquire(); > > > > Should you check the return code of acquire? > > > > Does it block using lockf() internally? > > It won't block, it will return an error, so I think it's a good idea to > check for that. Good catch, I will add a check for the return code. > > > > + if (media_->disableLinks()) > > > > Where do we recreate / re-enable the links? > > > > or does it reset to a sane default? > > It won't enable links, so any test depending on this would have to > enable links manually. Yes, each test who inherits from the base would need to enable links if it depends on them being enabled. > > > > + return TestFail; > > > > Is it an issue here that the media_ is not released? I now can't recall > > if the acquire/release is about locking globally using lockf() or if > > it's just a state machine state change. > > > > If it must be released - perhaps we should do some sort of auto-lock > > release by using the destructor of a local object, so that it releases > > when it goes out of scope. > > lockf() will not persist once the file descriptor is closed, so it > shouldn't be a problem, but proper error handling is still a good idea. I feel that handling the error here and call release() if we can't reset the links is a bit overkill for a test case and reduces the readability of the code for little gain. I do however not feel strongly about this and will do so in the next version. > > With those issues fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > + media_->release(); > > > + > > > + if (capture_->open()) > > > + return TestFail; > > > + > > > + V4L2DeviceFormat format = {}; > > > + if (capture_->getFormat(&format)) > > > + return TestFail; > > > + > > > + format.size.width = 640; > > > + format.size.height = 480; > > > + if (capture_->setFormat(&format)) > > > + return TestFail; > > > + > > > + return TestPass; > > > } > > > > > > void V4L2DeviceTest::cleanup() > > -- > Regards, > > Laurent Pinchart
diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp index ee5a8e009bef2a5e..5bd80a6c68d796b6 100644 --- a/test/v4l2_device/v4l2_device_test.cpp +++ b/test/v4l2_device/v4l2_device_test.cpp @@ -54,7 +54,24 @@ int V4L2DeviceTest::init() if (!capture_) return TestFail; - return capture_->open(); + media_->acquire(); + if (media_->disableLinks()) + return TestFail; + media_->release(); + + if (capture_->open()) + return TestFail; + + V4L2DeviceFormat format = {}; + if (capture_->getFormat(&format)) + return TestFail; + + format.size.width = 640; + format.size.height = 480; + if (capture_->setFormat(&format)) + return TestFail; + + return TestPass; } void V4L2DeviceTest::cleanup()
When initializing the test reset any media links and set a know resolutions. This is needed to put the device under test into known state and not have the v4l2 device tests depend on that no one have touched the device before the test is executed. The resolution is picked purely at random and could possibly be moved to each test case if there is a need for different resolutions for a specific one. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- test/v4l2_device/v4l2_device_test.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)