[v2] libcamera: camera_sensor: Cache pixel rate
diff mbox series

Message ID 20240122121358.3426521-1-paul.elder@ideasonboard.com
State New
Headers show
Series
  • [v2] libcamera: camera_sensor: Cache pixel rate
Related show

Commit Message

Paul Elder Jan. 22, 2024, 12:13 p.m. UTC 
Cache the pixel rate at set format time instead of fetching it from the
v4l2 subdev every time it's needed.

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
on the mode.

Signed-off-by: Paul Elder <paul.elder@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(-)

Comments

Laurent Pinchart Jan. 23, 2024, 2:03 a.m. UTC | #1 
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 ?

> 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@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.

> +	info->pixelRate = pixelRate_;
>  
>  	const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);
>  	info->minLineLength = info->outputSize.width + hblank.min().get<int32_t>();
Umang Jain Jan. 23, 2024, 5:51 a.m. UTC | #2 
Hi Paul

On 1/22/24 5:43 PM, Paul Elder wrote:
> Cache the pixel rate at set format time instead of fetching it from the
> v4l2 subdev every time it's needed.
>
> 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
> on the mode.
>
> Signed-off-by: Paul Elder <paul.elder@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>();
> +

Do we have to cache it at two places? One initProperties() and other in 
setFormat() ?

In the change log, you have used ' instead of '  so I am a bit confused..
>   	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>();
> +	info->pixelRate = pixelRate_;
>   
>   	const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);
>   	info->minLineLength = info->outputSize.width + hblank.min().get<int32_t>();
Paul Elder Jan. 23, 2024, 8:18 a.m. UTC | #3 
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@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>();
Paul Elder Jan. 23, 2024, 8:27 a.m. UTC | #4 
On Tue, Jan 23, 2024 at 11:21:55AM +0530, Umang Jain wrote:
> Hi Paul
> 
> On 1/22/24 5:43 PM, Paul Elder wrote:
> > Cache the pixel rate at set format time instead of fetching it from the
> > v4l2 subdev every time it's needed.
> > 
> > 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
> > on the mode.
> > 
> > Signed-off-by: Paul Elder <paul.elder@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>();
> > +
> 
> Do we have to cache it at two places? One initProperties() and other in
> setFormat() ?
> 
> In the change log, you have used ' instead of '  so I am a bit confused..

Oops. Yeah we need to cache it in both places.


Paul

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

Patch
diff mbox series 

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