[libcamera-devel,1/2] libcamera: pipeline: RKISP1 remove rockchip-sy-mipi-dphy

1597 diff mbox series

Remove subdevice rockchip-sy-mipi-dphy from the pipeline.
Sensors are connected direcly to rkisp1-isp-subdev.

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---
Hello,

This depends on the v7[1] of driver being accepted upstream. But I'm
submitting anyway for reference.

[1] https://patchwork.kernel.org/project/linux-media/list/?series=141793

Thanks
Helen
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 34 +++++-------------------
 utils/rkisp1/rkisp1-capture.sh           |  4 +--
 2 files changed, 8 insertions(+), 30 deletions(-)

Hi Helen,

Thank you for the patch.

On Wed, Jul 03, 2019 at 05:21:53PM -0300, Helen Koike wrote:
> Remove subdevice rockchip-sy-mipi-dphy from the pipeline.
> Sensors are connected direcly to rkisp1-isp-subdev.
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> Hello,
> 
> This depends on the v7[1] of driver being accepted upstream. But I'm
> submitting anyway for reference.
> 
> [1] https://patchwork.kernel.org/project/linux-media/list/?series=141793
> 
> Thanks
> Helen
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 34 +++++-------------------
>  utils/rkisp1/rkisp1-capture.sh           |  4 +--
>  2 files changed, 8 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4a5898d..358e2c8 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -104,7 +104,6 @@ private:
>  	void bufferReady(Buffer *buffer);
>  
>  	MediaDevice *media_;
> -	V4L2Subdevice *dphy_;
>  	V4L2Subdevice *isp_;
>  	V4L2VideoDevice *video_;
>  
> @@ -201,8 +200,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  }
>  
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> -	: PipelineHandler(manager), dphy_(nullptr), isp_(nullptr),
> -	  video_(nullptr)
> +	: PipelineHandler(manager), isp_(nullptr), video_(nullptr)
>  {
>  }
>  
> @@ -210,7 +208,6 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
>  {
>  	delete video_;
>  	delete isp_;
> -	delete dphy_;
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -250,7 +247,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();
> @@ -282,18 +279,14 @@ 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;
> -
> -	ret = dphy_->getFormat(1, &format);
> -	if (ret < 0)
> -		return ret;
> +	LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString();

I think you can skip this, as the previous debugging message prints the
exact same format.

>  
>  	ret = isp_->setFormat(0, &format);
>  	if (ret < 0)
>  		return ret;
>  
> +	LOG(RkISP1, Debug) << "ISP configured with " << format.toString();
> +
>  	V4L2DeviceFormat outputFormat = {};
>  	outputFormat.fourcc = cfg.pixelFormat;
>  	outputFormat.size = cfg.size;
> @@ -393,14 +386,6 @@ int PipelineHandlerRkISP1::initLinks()
>  	if (ret < 0)
>  		return ret;
>  
> -	link = media_->link("rockchip-sy-mipi-dphy", 1, "rkisp1-isp-subdev", 0);
> -	if (!link)
> -		return -ENODEV;
> -
> -	ret = link->setEnabled(true);
> -	if (ret < 0)
> -		return ret;
> -
>  	link = media_->link("rkisp1-isp-subdev", 2, "rkisp1_mainpath", 0);
>  	if (!link)
>  		return -ENODEV;
> @@ -442,17 +427,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	dm.add("rkisp1_mainpath");
>  	dm.add("rkisp1-statistics");
>  	dm.add("rkisp1-input-params");
> -	dm.add("rockchip-sy-mipi-dphy");
>  
>  	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)
> -		return false;
> -
>  	isp_ = V4L2Subdevice::fromEntityName(media_, "rkisp1-isp-subdev");
>  	if (isp_->open() < 0)
>  		return false;
> @@ -471,10 +451,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

s/ISP/ISP CSI-2/

Apart from these small issues this looks good to me, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I will however wait until the rkisp1 patch series goes through review
before applying this patch.

>  	 * 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..8a6f6eb 100755
> --- a/utils/rkisp1/rkisp1-capture.sh
> +++ b/utils/rkisp1/rkisp1-capture.sh
> @@ -68,12 +68,10 @@ 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 "'$sensor':0 -> 'rkisp1-isp-subdev':0 [1]"
>  	$mediactl -l "'rkisp1-isp-subdev':2 -> 'rkisp1_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]"
>  }