| Message ID | 20190805155133.11335-3-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:31PM +0200, Niklas Söderlund wrote: > Linux commit 85ab1aa1fac17bcd ("media: vimc: deb: fix default sink bayer > format") which is part of v5.2 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> > --- > src/libcamera/pipeline/vimc.cpp | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 3d6808222a8a2c5d..ae612b48436d1164 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -25,6 +25,7 @@ > #include "pipeline_handler.h" > #include "utils.h" > #include "v4l2_controls.h" > +#include "v4l2_subdevice.h" > #include "v4l2_videodevice.h" > > namespace libcamera { > @@ -35,13 +36,15 @@ class VimcCameraData : public CameraData > { > public: > VimcCameraData(PipelineHandler *pipe) > - : CameraData(pipe), video_(nullptr), sensor_(nullptr) > + : CameraData(pipe), video_(nullptr), debayer_(nullptr), > + sensor_(nullptr) > { > } > > ~VimcCameraData() > { > delete sensor_; > + delete debayer_; > delete video_; > } > > @@ -49,6 +52,7 @@ public: > void bufferReady(Buffer *buffer); > > V4L2VideoDevice *video_; > + V4L2Subdevice *debayer_; > CameraSensor *sensor_; > Stream stream_; > }; > @@ -188,6 +192,17 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > format.fourcc != cfg.pixelFormat) > return -EINVAL; > > + V4L2SubdeviceFormat subformat = {}; > + subformat.size = cfg.size; > + > + ret = data->debayer_->setFormat(0, &subformat); > + if (ret) > + return ret; > + > + ret = data->sensor_->setFormat(&subformat); > + if (ret) > + return ret; The sensor comes before the debayering subdevice, shouldn't you set the format on the sensor first ? That may require setting the bus format in subformat, but I think you can simply hardcode that. I would also configure this before configuring the video node. > + > cfg.setStream(&data->stream_); > > return 0; > @@ -340,6 +355,10 @@ int VimcCameraData::init(MediaDevice *media) > if (video_->open()) > return -ENODEV; > > + debayer_ = new V4L2Subdevice(media->getEntityByName("Debayer B")); > + if (debayer_->open()) > + return -ENODEV; > + > video_->bufferReady.connect(this, &VimcCameraData::bufferReady); > > sensor_ = new CameraSensor(media->getEntityByName("Sensor B"));
Hi Laurent, Thanks for your feedback. On 2019-08-05 20:34:39 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Mon, Aug 05, 2019 at 05:51:31PM +0200, Niklas Söderlund wrote: > > Linux commit 85ab1aa1fac17bcd ("media: vimc: deb: fix default sink bayer > > format") which is part of v5.2 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> > > --- > > src/libcamera/pipeline/vimc.cpp | 21 ++++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index 3d6808222a8a2c5d..ae612b48436d1164 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -25,6 +25,7 @@ > > #include "pipeline_handler.h" > > #include "utils.h" > > #include "v4l2_controls.h" > > +#include "v4l2_subdevice.h" > > #include "v4l2_videodevice.h" > > > > namespace libcamera { > > @@ -35,13 +36,15 @@ class VimcCameraData : public CameraData > > { > > public: > > VimcCameraData(PipelineHandler *pipe) > > - : CameraData(pipe), video_(nullptr), sensor_(nullptr) > > + : CameraData(pipe), video_(nullptr), debayer_(nullptr), > > + sensor_(nullptr) > > { > > } > > > > ~VimcCameraData() > > { > > delete sensor_; > > + delete debayer_; > > delete video_; > > } > > > > @@ -49,6 +52,7 @@ public: > > void bufferReady(Buffer *buffer); > > > > V4L2VideoDevice *video_; > > + V4L2Subdevice *debayer_; > > CameraSensor *sensor_; > > Stream stream_; > > }; > > @@ -188,6 +192,17 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > > format.fourcc != cfg.pixelFormat) > > return -EINVAL; > > > > + V4L2SubdeviceFormat subformat = {}; > > + subformat.size = cfg.size; > > + > > + ret = data->debayer_->setFormat(0, &subformat); > > + if (ret) > > + return ret; > > + > > + ret = data->sensor_->setFormat(&subformat); > > + if (ret) > > + return ret; > > The sensor comes before the debayering subdevice, shouldn't you set the > format on the sensor first ? That may require setting the bus format in > subformat, but I think you can simply hardcode that. I would also > configure this before configuring the video node. I tried that at first but there seems more work is required on the vimc kernel driver upstream and this was a way to mitigate the current state. I agree it was not the best idea. After closer looking at the problem it seems that the only way to support v5.2 and v5.1 (and earlier) in a nicer way is to turn the raw capture camera into only dealing with bayer formats. This don't seem to be the intent of the Linux driver as it exposes other formats but they all render -EPIPE on either v5.1 or v5.2. I have seen some Revert commits on the linux-media ML but they seems to have died out in activity [1] :-( I'm not sure what is the best way forward here, turning vimc into only delivering bayer formats is not ideal but I fear it's our best option until upstream is fixed. I have patches for this and I will send a v2 of this series doing this, but I'm open to other solutions which will unblock running on v5.2 (and v5.1) as that is my development environment waiting for upstream is not an appetizing option for me. 1. https://lkml.org/lkml/2019/7/9/682 > > > + > > cfg.setStream(&data->stream_); > > > > return 0; > > @@ -340,6 +355,10 @@ int VimcCameraData::init(MediaDevice *media) > > if (video_->open()) > > return -ENODEV; > > > > + debayer_ = new V4L2Subdevice(media->getEntityByName("Debayer B")); > > + if (debayer_->open()) > > + return -ENODEV; > > + > > video_->bufferReady.connect(this, &VimcCameraData::bufferReady); > > > > sensor_ = new CameraSensor(media->getEntityByName("Sensor B")); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 3d6808222a8a2c5d..ae612b48436d1164 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -25,6 +25,7 @@ #include "pipeline_handler.h" #include "utils.h" #include "v4l2_controls.h" +#include "v4l2_subdevice.h" #include "v4l2_videodevice.h" namespace libcamera { @@ -35,13 +36,15 @@ class VimcCameraData : public CameraData { public: VimcCameraData(PipelineHandler *pipe) - : CameraData(pipe), video_(nullptr), sensor_(nullptr) + : CameraData(pipe), video_(nullptr), debayer_(nullptr), + sensor_(nullptr) { } ~VimcCameraData() { delete sensor_; + delete debayer_; delete video_; } @@ -49,6 +52,7 @@ public: void bufferReady(Buffer *buffer); V4L2VideoDevice *video_; + V4L2Subdevice *debayer_; CameraSensor *sensor_; Stream stream_; }; @@ -188,6 +192,17 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) format.fourcc != cfg.pixelFormat) return -EINVAL; + V4L2SubdeviceFormat subformat = {}; + subformat.size = cfg.size; + + ret = data->debayer_->setFormat(0, &subformat); + if (ret) + return ret; + + ret = data->sensor_->setFormat(&subformat); + if (ret) + return ret; + cfg.setStream(&data->stream_); return 0; @@ -340,6 +355,10 @@ int VimcCameraData::init(MediaDevice *media) if (video_->open()) return -ENODEV; + debayer_ = new V4L2Subdevice(media->getEntityByName("Debayer B")); + if (debayer_->open()) + return -ENODEV; + video_->bufferReady.connect(this, &VimcCameraData::bufferReady); sensor_ = new CameraSensor(media->getEntityByName("Sensor B"));
Linux commit 85ab1aa1fac17bcd ("media: vimc: deb: fix default sink bayer format") which is part of v5.2 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> --- src/libcamera/pipeline/vimc.cpp | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)