[libcamera-devel] [PATCH v2 2/2] utils: rkisp1: sync topology with upstream driver in capture script

[Helen Koike]

On 1/17/20 8:42 PM, Niklas Söderlund wrote:
> Hi Helen,
> 
> On 2020-01-17 17:01:48 -0300, Helen Koike wrote:
>>
>>
>> On 1/17/20 2:00 PM, Niklas Söderlund wrote:
>>> Hi Helen,
>>>
>>> Thanks for your patch.
>>>
>>> On 2020-01-17 11:58:02 -0300, Helen Koike wrote:
>>>> rkisp1 kernel driver was merged upstream with minor changes in the
>>>> topology from the original driver libcamera based it's first support to
>>>> rkisp1.
>>>>
>>>> Adapt libcamera capture script to work with upstream driver.
>>>
>>> How did you test this patch? I tried running the script as such:
>>>
>>> $ ./rkisp1-capture.sh --no-save -s 1296x972 "ov5695 7-0036"
>>> Capturing 1296x972 from sensor ov5695 7-0036 in SBGGR10_1X10
>>> Configuring pipeline for ov5695 7-0036 in fmt:SBGGR10_1X10/1296x972
>>> Device /dev/video0 opened.
>>> Device `rkisp1' on `platform:rkisp1' (driver 'rkisp1') supports video, 
>>> capture, with mplanes.
>>> Video format set: YUYV (56595559) 1296x972 field none, 1 planes: 
>>>  * Stride 2592, buffer size 2519424
>>>  Video format: YUYV (56595559) 1296x972 field none, 1 planes: 
>>>   * Stride 2592, buffer size 2519424
>>>   5 buffers requested.
>>>   length: 1 offset: 4215579600 timestamp type/source: mono/EoF
>>>   Buffer 0/0 mapped at address 0x7f8bdb5000.
>>>   length: 1 offset: 4215579600 timestamp type/source: mono/EoF
>>>   Buffer 1/0 mapped at address 0x7f8bb4d000.
>>>   length: 1 offset: 4215579600 timestamp type/source: mono/EoF
>>>   Buffer 2/0 mapped at address 0x7f8b8e5000.
>>>   length: 1 offset: 4215579600 timestamp type/source: mono/EoF
>>>   Buffer 3/0 mapped at address 0x7f8b67d000.
>>>   length: 1 offset: 4215579600 timestamp type/source: mono/EoF
>>>   Buffer 4/0 mapped at address 0x7f8b415000.
>>>
>>> But I get no buffers back and yavta stops here and have to be killed.
>>
>> This is because I was testing other things and forgot this like that
>> enable a link (sorry about that):
>>
>> $mediactl -l "'rkisp1_resizer_mainpath':1 -> 'rkisp1_mainpath':0 [1]"
> 
> No worries.
> 
>>
>> In any case there is a bug in the driver, it shouldn't stall, it should fail
>> in link validation.
>>
>> Also, the driver allows too many links to be disabled (which doesn't make
>> sense). Hold this patchset a bit, let me send a patch making almost all
>> links immutable, so the line above won't be required.
> 
> This also effects the pipeline handler patch, do you want to hold that 
> patch too? Or would you prefer we try and merge both patches in this 
> series (with the fix for the script) now and then you can send a follow 
> up series when the immutable link fixes are merged in the media-tree?

The immutable patch is really small, I was hoping it wouldn't take long to be
accepted, so I think we can wait a few days.

I was also thinking if we could make the function link->setEnabled(true) not
fail if the link is immutable, so the pipeline patch would work with
or without the immutable fix in the driver. But I see that setupLink() just
make the MEDIA_IOC_SETUP_LINK ioctl and it would complicate the function a bit.

Helen

> 
>>
>> Helen
>>
>>>
>>>>
>>>> * Remove subdevice dphy from the pipeline.
>>>> * Add resizer in the pipeline.
>>>> * Fix links.
>>>> * Update entity names.
>>>>
>>>> Signed-off-by: Helen Koike <helen.koike at collabora.com>
>>>>
>>>> ---
>>>>
>>>> changes in v2:
>>>> * New commit (splitted from previous one)
>>>> ---
>>>>  utils/rkisp1/rkisp1-capture.sh | 16 ++++++++--------
>>>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/utils/rkisp1/rkisp1-capture.sh b/utils/rkisp1/rkisp1-capture.sh
>>>> index cffe9fe..4d09f5d 100755
>>>> --- a/utils/rkisp1/rkisp1-capture.sh
>>>> +++ b/utils/rkisp1/rkisp1-capture.sh
>>>> @@ -68,14 +68,14 @@ configure_pipeline() {
>>>>  
>>>>  	$mediactl -r
>>>>  
>>>> -	$mediactl -l "'$sensor':0 -> 'rockchip-sy-mipi-dphy':0 [1]"
>>>> -	$mediactl -l "'rockchip-sy-mipi-dphy':1 -> 'rkisp1-isp-subdev':0 [1]"
>>>> -	$mediactl -l "'rkisp1-isp-subdev':2 -> 'rkisp1_mainpath':0 [1]"
>>>> +	$mediactl -l "'$sensor':0 -> 'rkisp1_isp':0 [1]"
>>>> +	$mediactl -l "'rkisp1_isp':2 -> 'rkisp1_resizer_mainpath':0 [1]"
>>>>  
>>>>  	$mediactl -V "\"$sensor\":0 [$format]"
>>>> -	$mediactl -V "'rockchip-sy-mipi-dphy':1 [$format]"
>>>> -	$mediactl -V "'rkisp1-isp-subdev':0 [$format crop:(0,0)/$sensor_size]"
>>>> -	$mediactl -V "'rkisp1-isp-subdev':2 [fmt:$capture_mbus_code/$capture_size crop:(0,0)/$capture_size]"
>>>> +	$mediactl -V "'rkisp1_isp':0 [$format crop:(0,0)/$sensor_size]"
>>>> +	$mediactl -V "'rkisp1_isp':2 [fmt:$capture_mbus_code/$sensor_size crop:(0,0)/$sensor_size]"
>>>> +	$mediactl -V "'rkisp1_resizer_mainpath':0 [fmt:$capture_mbus_code/$sensor_size crop:(0,0)/$sensor_size]"
>>>> +	$mediactl -V "'rkisp1_resizer_mainpath':1 [fmt:$capture_mbus_code/$capture_size]"
>>>>  }
>>>>  
>>>>  # Capture frames
>>>> @@ -161,8 +161,8 @@ fi
>>>>  
>>>>  sensor_name=$1
>>>>  
>>>> -modprobe mipi_dphy_sy
>>>> -modprobe video_rkisp1
>>>> +modprobe phy_rockchip_dphy_rx0
>>>> +modprobe rockchip_isp1
>>>>  
>>>>  sensor=$(find_sensor $sensor_name) || exit
>>>>  mdev=$(find_media_device rkisp1) || exit
>>>> -- 
>>>> 2.24.0
>>>>
>>>
>