| Message ID | c13f13bb8e0c2fcd35ccac5d78d489add30c54c3.1562185292.git.helen.koike@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Helen, Thank you for the patch. On Wed, Jul 03, 2019 at 05:21:54PM -0300, Helen Koike wrote: > ISP output pad should be set to YUYV8_2X8 for non-bayer output format. > Bayer formats are not listed in RkISP1CameraConfiguration::validate(), > only non-bayer are listed, so we can set YUYV8_2X8 directly. > This need to be changed if we add support for bayer output with > libcamera. > > Signed-off-by: Helen Koike <helen.koike@collabora.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 358e2c8..fc04cf8 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -279,13 +279,23 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); > > - LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString(); > + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); > > ret = isp_->setFormat(0, &format); > if (ret < 0) > return ret; > > - LOG(RkISP1, Debug) << "ISP configured with " << format.toString(); > + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); > + > + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ > + format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8; > + LOG(RkISP1, Debug) << "Configuring ISP output pad with " << format.toString(); > + > + ret = isp_->setFormat(2, &format); > + if (ret < 0) > + return ret; > + > + LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); This looks reasonable to me, but I wonder why the current code works :-) What happens on your test system without this patch ? Or is this needed due to a change in your rkisp1 patch series ? If so, could you briefly explain what that change is ? > > V4L2DeviceFormat outputFormat = {}; > outputFormat.fourcc = cfg.pixelFormat;
On 7/3/19 7:42 PM, Laurent Pinchart wrote: > Hi Helen, > > Thank you for the patch. > > On Wed, Jul 03, 2019 at 05:21:54PM -0300, Helen Koike wrote: >> ISP output pad should be set to YUYV8_2X8 for non-bayer output format. >> Bayer formats are not listed in RkISP1CameraConfiguration::validate(), >> only non-bayer are listed, so we can set YUYV8_2X8 directly. >> This need to be changed if we add support for bayer output with >> libcamera. >> >> Signed-off-by: Helen Koike <helen.koike@collabora.com> >> --- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index 358e2c8..fc04cf8 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> @@ -279,13 +279,23 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >> >> LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); >> >> - LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString(); >> + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); >> >> ret = isp_->setFormat(0, &format); >> if (ret < 0) >> return ret; >> >> - LOG(RkISP1, Debug) << "ISP configured with " << format.toString(); >> + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); >> + >> + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ >> + format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8; >> + LOG(RkISP1, Debug) << "Configuring ISP output pad with " << format.toString(); >> + >> + ret = isp_->setFormat(2, &format); >> + if (ret < 0) >> + return ret; >> + >> + LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); > > This looks reasonable to me, but I wonder why the current code works :-) > What happens on your test system without this patch ? Or is this needed > due to a change in your rkisp1 patch series ? If so, could you briefly > explain what that change is ? It works without this patch if you don't touch the default configuration in the topology, but if you set the output pad to be bayer with media-ctl for instance, then cam tool won't work anymore. Thanks Helen > >> >> V4L2DeviceFormat outputFormat = {}; >> outputFormat.fourcc = cfg.pixelFormat; >
Hi Helen, On Thu, Jul 04, 2019 at 04:44:24PM -0300, Helen Koike wrote: > On 7/3/19 7:42 PM, Laurent Pinchart wrote: > > On Wed, Jul 03, 2019 at 05:21:54PM -0300, Helen Koike wrote: > >> ISP output pad should be set to YUYV8_2X8 for non-bayer output format. > >> Bayer formats are not listed in RkISP1CameraConfiguration::validate(), > >> only non-bayer are listed, so we can set YUYV8_2X8 directly. > >> This need to be changed if we add support for bayer output with > >> libcamera. > >> > >> Signed-off-by: Helen Koike <helen.koike@collabora.com> > >> --- > >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++-- > >> 1 file changed, 12 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> index 358e2c8..fc04cf8 100644 > >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> @@ -279,13 +279,23 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > >> > >> LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); > >> > >> - LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString(); > >> + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); > >> > >> ret = isp_->setFormat(0, &format); > >> if (ret < 0) > >> return ret; > >> > >> - LOG(RkISP1, Debug) << "ISP configured with " << format.toString(); > >> + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); > >> + > >> + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ > >> + format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8; > >> + LOG(RkISP1, Debug) << "Configuring ISP output pad with " << format.toString(); > >> + > >> + ret = isp_->setFormat(2, &format); > >> + if (ret < 0) > >> + return ret; > >> + > >> + LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); > > > > This looks reasonable to me, but I wonder why the current code works :-) > > What happens on your test system without this patch ? Or is this needed > > due to a change in your rkisp1 patch series ? If so, could you briefly > > explain what that change is ? > > It works without this patch if you don't touch the default configuration > in the topology, but if you set the output pad to be bayer with > media-ctl for instance, then cam tool won't work anymore. Of course. This makes complete sense. I would like to delay application 1/2 until the corresponding kernel patch series gets reviewed, but I think 2/2 could be merged already. It however depends on 1/2. Would you mind rebasing it as a standalone patch ? > >> > >> V4L2DeviceFormat outputFormat = {}; > >> outputFormat.fourcc = cfg.pixelFormat;
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 358e2c8..fc04cf8 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -279,13 +279,23 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); - LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString(); + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); ret = isp_->setFormat(0, &format); if (ret < 0) return ret; - LOG(RkISP1, Debug) << "ISP configured with " << format.toString(); + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); + + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ + format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8; + LOG(RkISP1, Debug) << "Configuring ISP output pad with " << format.toString(); + + ret = isp_->setFormat(2, &format); + if (ret < 0) + return ret; + + LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); V4L2DeviceFormat outputFormat = {}; outputFormat.fourcc = cfg.pixelFormat;
ISP output pad should be set to YUYV8_2X8 for non-bayer output format. Bayer formats are not listed in RkISP1CameraConfiguration::validate(), only non-bayer are listed, so we can set YUYV8_2X8 directly. This need to be changed if we add support for bayer output with libcamera. Signed-off-by: Helen Koike <helen.koike@collabora.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)