| Message ID | 20190805155133.11335-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Mon, Aug 05, 2019 at 05:51:32PM +0200, Niklas Söderlund wrote: > Most of the video device tests are based on vimc and Linux commit > 85ab1aa1fac17bcd ("media: vimc: deb: fix default sink bayer format") > changes the default media bus format for the debayer subdevices. This > leads to a -EPIPE error when trying to use the raw capture video device > nodes. > > Fix this by propagating the media bus format used on the debayer > subdevcie to the sensor. This allows the same code to function on s/subdevcie/subdevice/ And as for patch 2/4, I would propagate it the other way around, from the sensor to the debayering subdev. > kernels previous to the change and after it. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > .../v4l2_videodevice_test.cpp | 22 +++++++++++++++++++ > test/v4l2_videodevice/v4l2_videodevice_test.h | 7 +++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp > index b26d06ad45197c8f..b1d4eaf6e9e4a4b1 100644 > --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp > @@ -62,6 +62,26 @@ int V4L2VideoDeviceTest::init() > if (ret) > return TestFail; > > + if (driver_ == "vimc") { A bit hack-ish, but for tests I think we can live with this. > + V4L2SubdeviceFormat subformat = {}; > + subformat.size.width = 640; > + subformat.size.height = 480; > + > + sensor_ = new CameraSensor(media_->getEntityByName("Sensor A")); > + if (sensor_->init()) > + return TestFail; > + > + debayer_ = new V4L2Subdevice(media_->getEntityByName("Debayer A")); > + if (debayer_->open()) > + return TestFail; > + > + if (debayer_->setFormat(0, &subformat)) > + return TestFail; > + > + if (sensor_->setFormat(&subformat)) > + return TestFail; > + } > + > if (capture_->open()) > return TestFail; > > @@ -83,5 +103,7 @@ void V4L2VideoDeviceTest::cleanup() > capture_->releaseBuffers(); > capture_->close(); > > + delete debayer_; > + delete sensor_; > delete capture_; > }; > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h > index 3321b5a4f98fdb1d..34dd231c6d9d108d 100644 > --- a/test/v4l2_videodevice/v4l2_videodevice_test.h > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h > @@ -13,8 +13,10 @@ > > #include "test.h" > > +#include "camera_sensor.h" > #include "device_enumerator.h" > #include "media_device.h" > +#include "v4l2_subdevice.h" > #include "v4l2_videodevice.h" > > using namespace libcamera; > @@ -23,7 +25,8 @@ class V4L2VideoDeviceTest : public Test > { > public: > V4L2VideoDeviceTest(const char *driver, const char *entity) > - : driver_(driver), entity_(entity), capture_(nullptr) > + : driver_(driver), entity_(entity), sensor_(nullptr), > + debayer_(nullptr), capture_(nullptr) > { > } > > @@ -35,6 +38,8 @@ protected: > std::string entity_; > std::unique_ptr<DeviceEnumerator> enumerator_; > std::shared_ptr<MediaDevice> media_; > + CameraSensor *sensor_; > + V4L2Subdevice *debayer_; Do you need to store these internally, can't they be local variables in V4L2VideoDeviceTest::init() ? > V4L2VideoDevice *capture_; > BufferPool pool_; > };
Hi Laurent, Thanks for your feedback. On 2019-08-05 20:37:27 +0300, Laurent Pinchart wrote: > > @@ -35,6 +38,8 @@ protected: > > std::string entity_; > > std::unique_ptr<DeviceEnumerator> enumerator_; > > std::shared_ptr<MediaDevice> media_; > > + CameraSensor *sensor_; > > + V4L2Subdevice *debayer_; > > Do you need to store these internally, can't they be local variables in > V4L2VideoDeviceTest::init() ? If I store them locally in V4L2VideoDeviceTest::init() I would need to handle all error cases and delete them if I don't want to leak memory for valgrind, so I went for this option. I also fear they might be needed outside init() soon enough once this problem is sorted out upstream. I have keep this for v2.
On Wed, Aug 07, 2019 at 10:47:12PM +0200, Niklas Söderlund wrote: > Hi Laurent, > > Thanks for your feedback. > > On 2019-08-05 20:37:27 +0300, Laurent Pinchart wrote: > > > @@ -35,6 +38,8 @@ protected: > > > std::string entity_; > > > std::unique_ptr<DeviceEnumerator> enumerator_; > > > std::shared_ptr<MediaDevice> media_; > > > + CameraSensor *sensor_; > > > + V4L2Subdevice *debayer_; > > > > Do you need to store these internally, can't they be local variables in > > V4L2VideoDeviceTest::init() ? > > If I store them locally in V4L2VideoDeviceTest::init() I would need to > handle all error cases and delete them if I don't want to leak memory > for valgrind, so I went for this option. I also fear they might be > needed outside init() soon enough once this problem is sorted out > upstream. I have keep this for v2. OK, please disregard my comment on v2 then.
diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp index b26d06ad45197c8f..b1d4eaf6e9e4a4b1 100644 --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp @@ -62,6 +62,26 @@ int V4L2VideoDeviceTest::init() if (ret) return TestFail; + if (driver_ == "vimc") { + V4L2SubdeviceFormat subformat = {}; + subformat.size.width = 640; + subformat.size.height = 480; + + sensor_ = new CameraSensor(media_->getEntityByName("Sensor A")); + if (sensor_->init()) + return TestFail; + + debayer_ = new V4L2Subdevice(media_->getEntityByName("Debayer A")); + if (debayer_->open()) + return TestFail; + + if (debayer_->setFormat(0, &subformat)) + return TestFail; + + if (sensor_->setFormat(&subformat)) + return TestFail; + } + if (capture_->open()) return TestFail; @@ -83,5 +103,7 @@ void V4L2VideoDeviceTest::cleanup() capture_->releaseBuffers(); capture_->close(); + delete debayer_; + delete sensor_; delete capture_; }; diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h index 3321b5a4f98fdb1d..34dd231c6d9d108d 100644 --- a/test/v4l2_videodevice/v4l2_videodevice_test.h +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h @@ -13,8 +13,10 @@ #include "test.h" +#include "camera_sensor.h" #include "device_enumerator.h" #include "media_device.h" +#include "v4l2_subdevice.h" #include "v4l2_videodevice.h" using namespace libcamera; @@ -23,7 +25,8 @@ class V4L2VideoDeviceTest : public Test { public: V4L2VideoDeviceTest(const char *driver, const char *entity) - : driver_(driver), entity_(entity), capture_(nullptr) + : driver_(driver), entity_(entity), sensor_(nullptr), + debayer_(nullptr), capture_(nullptr) { } @@ -35,6 +38,8 @@ protected: std::string entity_; std::unique_ptr<DeviceEnumerator> enumerator_; std::shared_ptr<MediaDevice> media_; + CameraSensor *sensor_; + V4L2Subdevice *debayer_; V4L2VideoDevice *capture_; BufferPool pool_; };
Most of the video device tests are based on vimc and Linux commit 85ab1aa1fac17bcd ("media: vimc: deb: fix default sink bayer format") changes the default media bus format for the debayer subdevices. This leads to a -EPIPE error when trying to use the raw capture video device nodes. Fix this by propagating the media bus format used on the debayer subdevcie to the sensor. This allows the same code to function on kernels previous to the change and after it. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- .../v4l2_videodevice_test.cpp | 22 +++++++++++++++++++ test/v4l2_videodevice/v4l2_videodevice_test.h | 7 +++++- 2 files changed, 28 insertions(+), 1 deletion(-)