[PATCH v2] libcamera: camera_sensor: Cache pixel rate

[Paul Elder]

On Tue, Jan 23, 2024 at 04:03:19AM +0200, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Jan 22, 2024 at 09:13:58PM +0900, Paul Elder wrote:
> > Cache the pixel rate at set format time instead of fetching it from the
> > v4l2 subdev every time it's needed.
> 
> Could you please explain in the commit message why this is needed ? Is
> it "just" a small optimization, or does it fix an issue ?

I want it for adding libcamera controls to CameraSensor because it's
needed for both getting and setting the hblank and vblank controls and I
want to avoid subdev_->getControls() every time we do that.

> > It needs to be cached at set format time as opposed to initialization
> > time as some sensor drives (eg. imx708) change the pixel rate depending
> 
> s/drives/drivers/
> 
> > on the mode.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - Cache the pixel rate at set format time instead of at initialization
> >   time
> > ---
> >  include/libcamera/internal/camera_sensor.h | 2 ++
> >  src/libcamera/camera_sensor.cpp            | 8 +++++++-
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 60a8b106d..da3bf12b3 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -105,6 +105,8 @@ private:
> >  	std::string model_;
> >  	std::string id_;
> >  
> > +	uint64_t pixelRate_;
> > +
> >  	V4L2Subdevice::Formats formats_;
> >  	std::vector<unsigned int> mbusCodes_;
> >  	std::vector<Size> sizes_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 0ef78d9c8..127610321 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -515,6 +515,9 @@ int CameraSensor::initProperties()
> >  		properties_.set(properties::draft::ColorFilterArrangement, cfa);
> >  	}
> >  
> > +	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE });
> > +	pixelRate_ = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();
> > +
> >  	return 0;
> >  }
> >  
> > @@ -814,6 +817,9 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format, Transform transform)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE });
> > +	pixelRate_ = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();
> > +
> >  	updateControlInfo();
> >  	return 0;
> >  }
> > @@ -1080,7 +1086,7 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const
> >  		return -EINVAL;
> >  	}
> >  
> > -	info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();
> 
> This avoids fetching the pixel rate from the ControlList, but it's still
> fetched from the driver when populating the control list. I'm thus
> curious to know the reason for this patch.

Right, I should remove fetching the pixel rate.


Thanks,

Paul

> 
> > +	info->pixelRate = pixelRate_;
> >  
> >  	const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);
> >  	info->minLineLength = info->outputSize.width + hblank.min().get<int32_t>();