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

[Helen Koike]

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

Thanks
Helen

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