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

[Laurent Pinchart]

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 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 ?
> 
> 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 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