| Message ID | 8ad5a0874203dacfd488fe6827e3feab14dc959c.1564513804.git.helen.koike@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Helen, Thank you for the patch, and sorry for the late reply. On Tue, Jul 30, 2019 at 04:10:07PM -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 s/need/needs/ > libcamera. > > Signed-off-by: Helen Koike <helen.koike@collabora.com> > > --- > > Hi, > > I'm re-sending this patch standalone as Laurent suggested. > > Thanks > Helen > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index efa9604..bc7cb3f 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -286,6 +286,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (ret < 0) > return ret; > > + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); > + > ret = dphy_->getFormat(1, &format); > if (ret < 0) > return ret; > @@ -294,6 +296,18 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (ret < 0) > return ret; > > + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); > + > + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ s/output/output./ According to the code below, isn't this pad 2 ? I would write "YUYV8_2X8 is required on the ISP source path pad for YUV output.". Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Please let me know if you are fine with those small changes, I will then add them when applying the patch. Niklas, I know you have a running environment for the Scarlet tablet, would you be able to test this patch ? > + 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; > outputFormat.size = cfg.size;
Hi Laurent, On 8/8/19 7:01 PM, Laurent Pinchart wrote: > Hi Helen, > > Thank you for the patch, and sorry for the late reply. no problem, thanks for reviewing. > > On Tue, Jul 30, 2019 at 04:10:07PM -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 > > s/need/needs/ > >> libcamera. >> >> Signed-off-by: Helen Koike <helen.koike@collabora.com> >> >> --- >> >> Hi, >> >> I'm re-sending this patch standalone as Laurent suggested. >> >> Thanks >> Helen >> --- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index efa9604..bc7cb3f 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> @@ -286,6 +286,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >> if (ret < 0) >> return ret; >> >> + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); >> + >> ret = dphy_->getFormat(1, &format); >> if (ret < 0) >> return ret; >> @@ -294,6 +296,18 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >> if (ret < 0) >> return ret; >> >> + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); >> + >> + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ > > s/output/output./ > > According to the code below, isn't this pad 2 ? I would write "YUYV8_2X8 > is required on the ISP source path pad for YUV output.". Yes, it is pad 1, sorry about that > > Apart from that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Please let me know if you are fine with those small changes, I will then > add them when applying the patch. Yes, I'm fine with those changes, thanks! Regards, Helen > > Niklas, I know you have a running environment for the Scarlet tablet, > would you be able to test this patch ? > >> + 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; >> outputFormat.size = cfg.size; >
Hi Helen, On Thu, Aug 08, 2019 at 10:31:58PM -0300, Helen Koike wrote: > On 8/8/19 7:01 PM, Laurent Pinchart wrote: > > Hi Helen, > > > > Thank you for the patch, and sorry for the late reply. > > no problem, thanks for reviewing. > > > On Tue, Jul 30, 2019 at 04:10:07PM -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 > > > > s/need/needs/ > > > >> libcamera. > >> > >> Signed-off-by: Helen Koike <helen.koike@collabora.com> > >> > >> --- > >> > >> Hi, > >> > >> I'm re-sending this patch standalone as Laurent suggested. > >> > >> Thanks > >> Helen > >> --- > >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> index efa9604..bc7cb3f 100644 > >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> @@ -286,6 +286,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > >> if (ret < 0) > >> return ret; > >> > >> + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); > >> + > >> ret = dphy_->getFormat(1, &format); > >> if (ret < 0) > >> return ret; > >> @@ -294,6 +296,18 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > >> if (ret < 0) > >> return ret; > >> > >> + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); > >> + > >> + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ > > > > s/output/output./ > > > > According to the code below, isn't this pad 2 ? I would write "YUYV8_2X8 > > is required on the ISP source path pad for YUV output.". > > Yes, it is pad 1, sorry about that Do you mean the comment is right and the code should be changed ? > > Apart from that, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Please let me know if you are fine with those small changes, I will then > > add them when applying the patch. > > Yes, I'm fine with those changes, thanks! > > > Niklas, I know you have a running environment for the Scarlet tablet, > > would you be able to test this patch ? > > > >> + 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; > >> outputFormat.size = cfg.size;
On 8/9/19 4:26 PM, Laurent Pinchart wrote: > Hi Helen, > > On Thu, Aug 08, 2019 at 10:31:58PM -0300, Helen Koike wrote: >> On 8/8/19 7:01 PM, Laurent Pinchart wrote: >>> Hi Helen, >>> >>> Thank you for the patch, and sorry for the late reply. >> >> no problem, thanks for reviewing. >> >>> On Tue, Jul 30, 2019 at 04:10:07PM -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 >>> >>> s/need/needs/ >>> >>>> libcamera. >>>> >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com> >>>> >>>> --- >>>> >>>> Hi, >>>> >>>> I'm re-sending this patch standalone as Laurent suggested. >>>> >>>> Thanks >>>> Helen >>>> --- >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>>> index efa9604..bc7cb3f 100644 >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>>> @@ -286,6 +286,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>>> if (ret < 0) >>>> return ret; >>>> >>>> + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); >>>> + >>>> ret = dphy_->getFormat(1, &format); >>>> if (ret < 0) >>>> return ret; >>>> @@ -294,6 +296,18 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>>> if (ret < 0) >>>> return ret; >>>> >>>> + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); >>>> + >>>> + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ >>> >>> s/output/output./ >>> >>> According to the code below, isn't this pad 2 ? I would write "YUYV8_2X8 >>> is required on the ISP source path pad for YUV output.". >> >> Yes, it is pad 1, sorry about that > > Do you mean the comment is right and the code should be changed ? Ops, sorry again for this confusion, it is pad 2 (I don't know why I wrote pad 1), the code is right, the comment should be changed. Thanks Helen > >>> Apart from that, >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>> Please let me know if you are fine with those small changes, I will then >>> add them when applying the patch. >> >> Yes, I'm fine with those changes, thanks! >> >>> Niklas, I know you have a running environment for the Scarlet tablet, >>> would you be able to test this patch ? >>> >>>> + 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; >>>> outputFormat.size = cfg.size; >
Hi Helen, On Fri, Aug 09, 2019 at 07:16:23PM -0300, Helen Koike wrote: > On 8/9/19 4:26 PM, Laurent Pinchart wrote: > > On Thu, Aug 08, 2019 at 10:31:58PM -0300, Helen Koike wrote: > >> On 8/8/19 7:01 PM, Laurent Pinchart wrote: > >>> Hi Helen, > >>> > >>> Thank you for the patch, and sorry for the late reply. > >> > >> no problem, thanks for reviewing. > >> > >>> On Tue, Jul 30, 2019 at 04:10:07PM -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 > >>> > >>> s/need/needs/ > >>> > >>>> libcamera. > >>>> > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com> > >>>> > >>>> --- > >>>> > >>>> Hi, > >>>> > >>>> I'm re-sending this patch standalone as Laurent suggested. > >>>> > >>>> Thanks > >>>> Helen > >>>> --- > >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++++ > >>>> 1 file changed, 14 insertions(+) > >>>> > >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>>> index efa9604..bc7cb3f 100644 > >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>>> @@ -286,6 +286,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > >>>> if (ret < 0) > >>>> return ret; > >>>> > >>>> + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); > >>>> + > >>>> ret = dphy_->getFormat(1, &format); > >>>> if (ret < 0) > >>>> return ret; > >>>> @@ -294,6 +296,18 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > >>>> if (ret < 0) > >>>> return ret; > >>>> > >>>> + LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); > >>>> + > >>>> + /* YUYV8_2X8 is required in ISP pad 1 for non-bayer output */ > >>> > >>> s/output/output./ > >>> > >>> According to the code below, isn't this pad 2 ? I would write "YUYV8_2X8 > >>> is required on the ISP source path pad for YUV output.". > >> > >> Yes, it is pad 1, sorry about that > > > > Do you mean the comment is right and the code should be changed ? > > Ops, sorry again for this confusion, it is pad 2 (I don't know why I wrote pad 1), > the code is right, the comment should be changed. Comment fixed and patch pushed. Thank you. > >>> Apart from that, > >>> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> > >>> Please let me know if you are fine with those small changes, I will then > >>> add them when applying the patch. > >> > >> Yes, I'm fine with those changes, thanks! > >> > >>> Niklas, I know you have a running environment for the Scarlet tablet, > >>> would you be able to test this patch ? > >>> > >>>> + 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; > >>>> outputFormat.size = cfg.size;
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index efa9604..bc7cb3f 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -286,6 +286,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) if (ret < 0) return ret; + LOG(RkISP1, Debug) << "Configuring ISP input pad with " << format.toString(); + ret = dphy_->getFormat(1, &format); if (ret < 0) return ret; @@ -294,6 +296,18 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) if (ret < 0) return ret; + 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; outputFormat.size = cfg.size;
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> --- Hi, I'm re-sending this patch standalone as Laurent suggested. Thanks Helen --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+)