[v1,10/35] pipeline: rkisp1: Add a frameStart function to handle DelayedControls::applyControls

24776 diff mbox series

Move the call to applyControls into an intermediate function for
upcoming modfications.

While at it, properly disconnect the signal on failure.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

2025. 10. 24. 10:50 keltezéssel, Stefan Klug írta:
> Move the call to applyControls into an intermediate function for
> upcoming modfications.
> 
> While at it, properly disconnect the signal on failure.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 17250cbd5ae6..9be9f44db479 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1540,6 +1540,7 @@ const uint32_t kDefaultExtParamsBlocks = 0xfffff;
> 
>   int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   {
> +	utils::ScopeExitActions actions;
>   	int ret;
> 
>   	std::unique_ptr<RkISP1CameraData> data =
> @@ -1570,8 +1571,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   	data->delayedCtrls_ =
>   		std::make_unique<DelayedControls>(data->sensor_->device(),
>   						  params);
> -	isp_->frameStart.connect(data->delayedCtrls_.get(),
> -				 &DelayedControls::applyControls);
> +	isp_->frameStart.connect(this,
> +				 &PipelineHandlerRkISP1::frameStart);

I thought the declaration was missing, but it turns out the function is
declared but not defined. (Not removed in 3809fd77463f8f9f30d8da56fc3b103c683fba72.)

I realize that this is kind of unrelated, but why register the signal
handler in every `createCamera()` invocation? Is it not sufficient
to do that once? Especially now that the handler has been completely
decoupled from `RkISP1CameraData`?


> +
> +	actions += [&]() { isp_->frameStart.disconnect(this); };
> 
>   	uint32_t supportedBlocks = kDefaultExtParamsBlocks;
> 
> @@ -1603,9 +1606,20 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> 
>   	registerCamera(std::move(camera));
> 
> +	actions.release();
> +
>   	return 0;
>   }
> 
> +void PipelineHandlerRkISP1::frameStart(uint32_t sequence)
> +{
> +	if (!activeCamera_)
> +		return;
> +
> +	RkISP1CameraData *data = cameraData(activeCamera_);
> +	data->delayedCtrls_->applyControls(sequence);
> +}
> +
>   bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>   {
>   	DeviceMatch dm("rkisp1");
> --
> 2.48.1
>

Hi Barnabás,

Thank you for the review.

Quoting Barnabás Pőcze (2025-10-24 11:17:15)
> 2025. 10. 24. 10:50 keltezéssel, Stefan Klug írta:
> > Move the call to applyControls into an intermediate function for
> > upcoming modfications.
> > 
> > While at it, properly disconnect the signal on failure.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 17250cbd5ae6..9be9f44db479 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1540,6 +1540,7 @@ const uint32_t kDefaultExtParamsBlocks = 0xfffff;
> > 
> >   int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >   {
> > +     utils::ScopeExitActions actions;
> >       int ret;
> > 
> >       std::unique_ptr<RkISP1CameraData> data =
> > @@ -1570,8 +1571,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >       data->delayedCtrls_ =
> >               std::make_unique<DelayedControls>(data->sensor_->device(),
> >                                                 params);
> > -     isp_->frameStart.connect(data->delayedCtrls_.get(),
> > -                              &DelayedControls::applyControls);
> > +     isp_->frameStart.connect(this,
> > +                              &PipelineHandlerRkISP1::frameStart);
> 
> I thought the declaration was missing, but it turns out the function is
> declared but not defined. (Not removed in 3809fd77463f8f9f30d8da56fc3b103c683fba72.)
> 
> I realize that this is kind of unrelated, but why register the signal
> handler in every `createCamera()` invocation? Is it not sufficient
> to do that once? Especially now that the handler has been completely
> decoupled from `RkISP1CameraData`?
> 

I don't think this is unrelated. I changed it for v2, to connect inside
the match() function, where all the other signals get connected.

Best regards,
Stefan

> 
> > +
> > +     actions += [&]() { isp_->frameStart.disconnect(this); };
> > 
> >       uint32_t supportedBlocks = kDefaultExtParamsBlocks;
> > 
> > @@ -1603,9 +1606,20 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > 
> >       registerCamera(std::move(camera));
> > 
> > +     actions.release();
> > +
> >       return 0;
> >   }
> > 
> > +void PipelineHandlerRkISP1::frameStart(uint32_t sequence)
> > +{
> > +     if (!activeCamera_)
> > +             return;
> > +
> > +     RkISP1CameraData *data = cameraData(activeCamera_);
> > +     data->delayedCtrls_->applyControls(sequence);
> > +}
> > +
> >   bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >   {
> >       DeviceMatch dm("rkisp1");
> > --
> > 2.48.1
> > 
>