[libcamera-devel] [PATCH v2] libcamera: pipeline: RKISP1 configure isp output pad

[Laurent Pinchart]

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 at 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 at 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;

-- 
Regards,

Laurent Pinchart