[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: sync topology with upstream driver
Helen Koike
helen.koike at collabora.com
Fri Jan 17 15:19:18 CET 2020
Hi,
On 1/16/20 10:27 PM, Niklas Söderlund wrote:
> Hi Helen,
>
> Thanks for your work.
>
> On 2020-01-15 21:45:53 -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 libramera to work with upstream driver.
>>
>> * 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>
>
> I tested this change with the driver merged in the media-tree and it
> works as expected, great work! I have some small nits bellow but other
> then that I think it's ready to go.
>
> When testing this I was unable to locate any up-to-date patches updating
> devicetree files to the state of the driver in staging and had to hack
> something of my own to get it working. Is there patches somewhere? Is
> their a plan to upstream the DT changes for Scarlet ?
I have a draft version, which is available in my tree:
https://gitlab.collabora.com/koike/linux/tree/rockchip/isp/v13
I plan to upstream them, but the bindings are still in staging, and
I want to take a closer look to this patchset before we move the
bindings out of staging: https://patchwork.kernel.org/patch/10988217/
Looks like they conflict, or describe the same thing (I plan to take
a look and reply to the patch).
>
>> ---
>>
>> Hi,
>>
>> I'm not sure if it is better to wait the driver to get out of staging,
>> but in any case I'm sending it here in case others want to use it.
>>
>> It is also available at:
>> https://gitlab.collabora.com/koike/libcamera
>>
>> Thanks
>> Helen
>>
>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 +++++++++++++-----------
>> utils/rkisp1/rkisp1-capture.sh | 16 +++---
>> 2 files changed, 43 insertions(+), 36 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 389a99c..279bbb6 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -207,8 +207,8 @@ private:
>> int freeBuffers(Camera *camera);
>>
>> MediaDevice *media_;
>> - V4L2Subdevice *dphy_;
>> V4L2Subdevice *isp_;
>> + V4L2Subdevice *resizer_;
>> V4L2VideoDevice *video_;
>> V4L2VideoDevice *param_;
>> V4L2VideoDevice *stat_;
>> @@ -513,7 +513,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>> }
>>
>> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>> - : PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),
>> + : PipelineHandler(manager), isp_(nullptr), resizer_(nullptr),
>> video_(nullptr), param_(nullptr), stat_(nullptr)
>> {
>> }
>> @@ -523,8 +523,8 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
>> delete param_;
>> delete stat_;
>> delete video_;
>> + delete resizer_;
>> delete isp_;
>> - delete dphy_;
>> }
>>
>> /* -----------------------------------------------------------------------------
>> @@ -564,7 +564,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>> * Configure the sensor links: enable the link corresponding to this
>> * camera and disable all the other sensor links.
>> */
>> - const MediaPad *pad = dphy_->entity()->getPadByIndex(0);
>> + const MediaPad *pad = isp_->entity()->getPadByIndex(0);
>>
>> for (MediaLink *link : pad->links()) {
>> bool enable = link->source()->entity() == sensor->entity();
>> @@ -596,16 +596,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>>
>> LOG(RkISP1, Debug) << "Sensor configured with " << format.toString();
>>
>> - ret = dphy_->setFormat(0, &format);
>> - 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;
>> -
>> ret = isp_->setFormat(0, &format);
>> if (ret < 0)
>> return ret;
>> @@ -622,6 +612,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>>
>> LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString();
>>
>> + ret = resizer_->setFormat(0, &format);
>> + if (ret < 0)
>> + return ret;
>> +
>> + LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString();
>> +
>> + format.size = cfg.size;
>> +
>> + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString();
>> +
>> + ret = resizer_->setFormat(1, &format);
>> + if (ret < 0)
>> + return ret;
>> +
>> + LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString();
>> +
>> V4L2DeviceFormat outputFormat = {};
>> outputFormat.fourcc = video_->toV4L2Fourcc(cfg.pixelFormat);
>> outputFormat.size = cfg.size;
>> @@ -868,7 +874,7 @@ int PipelineHandlerRkISP1::initLinks()
>> if (ret < 0)
>> return ret;
>>
>> - link = media_->link("rockchip-sy-mipi-dphy", 1, "rkisp1-isp-subdev", 0);
>> + link = media_->link("rkisp1_isp", 2, "rkisp1_resizer_mainpath", 0);
>> if (!link)
>> return -ENODEV;
>>
>> @@ -876,7 +882,7 @@ int PipelineHandlerRkISP1::initLinks()
>> if (ret < 0)
>> return ret;
>>
>> - link = media_->link("rkisp1-isp-subdev", 2, "rkisp1_mainpath", 0);
>> + link = media_->link("rkisp1_resizer_mainpath", 1, "rkisp1_mainpath", 0);
>> if (!link)
>> return -ENODEV;
>>
>> @@ -923,24 +929,25 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>> const MediaPad *pad;
>>
>> DeviceMatch dm("rkisp1");
>> - dm.add("rkisp1-isp-subdev");
>> + dm.add("rkisp1_isp");
>> + dm.add("rkisp1_resizer_selfpath");
>> + dm.add("rkisp1_resizer_mainpath");
>> dm.add("rkisp1_selfpath");
>> dm.add("rkisp1_mainpath");
>> - dm.add("rkisp1-statistics");
>> - dm.add("rkisp1-input-params");
>> - dm.add("rockchip-sy-mipi-dphy");
>> + dm.add("rkisp1_stats");
>> + dm.add("rkisp1_params");
>>
>> media_ = acquireMediaDevice(enumerator, dm);
>> if (!media_)
>> return false;
>>
>> /* Create the V4L2 subdevices we will need. */
>> - dphy_ = V4L2Subdevice::fromEntityName(media_, "rockchip-sy-mipi-dphy");
>> - if (dphy_->open() < 0)
>> + isp_ = V4L2Subdevice::fromEntityName(media_, "rkisp1_isp");
>> + if (isp_->open() < 0)
>> return false;
>>
>> - isp_ = V4L2Subdevice::fromEntityName(media_, "rkisp1-isp-subdev");
>> - if (isp_->open() < 0)
>> + resizer_ = V4L2Subdevice::fromEntityName(media_, "rkisp1_resizer_mainpath");
>> + if (resizer_->open() < 0)
>> return false;
>>
>> /* Locate and open the capture video node. */
>> @@ -948,11 +955,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>> if (video_->open() < 0)
>> return false;
>>
>> - stat_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1-statistics");
>> + stat_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_stats");
>> if (stat_->open() < 0)
>> return false;
>>
>> - param_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1-input-params");
>> + param_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_params");
>> if (param_->open() < 0)
>> return false;
>>
>> @@ -967,10 +974,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>> }
>>
>> /*
>> - * Enumerate all sensors connected to the CSI-2 receiver and create one
>> + * Enumerate all sensors connected to the ISP receiver and create one
>
> I would s/receiver//
ack
>
>> * camera instance for each of them.
>> */
>> - pad = dphy_->entity()->getPadByIndex(0);
>> + pad = isp_->entity()->getPadByIndex(0);
>> if (!pad)
>> return false;
>>
>> 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() {
>
> I would split the changes to the capture script in it's own patch.
ok
Thanks
Helen
>
>>
>> $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
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>
More information about the libcamera-devel
mailing list