| Message ID | 20220623144410.20725-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Naushir, Thanks for the series ! On 23/06/2022 16:44, Naushir Patuck via libcamera-devel wrote: > Hi, > > This series adds a sensor temperature metadata control to libcamera. This > control is returned out through Request metadata when available. There is an > open PR [1] to enable the temperature sensor on the imx477 on the Raspberry Pi > platforms. > > At present, there is no mechanism in V4L2 to return the temperature from the > sensor device driver, so we rely on the embedded data to fetch the values every > frame. I think this PR along with a V4L2 control introduction (V4L2_CID_TEMPERATURE :-) ?) could be interesting to be sent to the linux-media ML at the same time ? JM > > Thanks, > Naush > > [1] https://github.com/raspberrypi/linux/pull/5067 > > Naushir Patuck (3): > libcamera: controls: Add SensorTemperature control > ipa: raspberrypi: Add sensor temperature to DeviceStatus > ipa: raspberrypi: imx477: Get sensor temperature from embedded data > > src/ipa/raspberrypi/cam_helper.cpp | 7 ++++--- > src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++- > src/ipa/raspberrypi/controller/device_status.cpp | 3 +++ > src/ipa/raspberrypi/controller/device_status.h | 4 +++- > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > src/libcamera/control_ids.yaml | 10 ++++++++++ > 6 files changed, 26 insertions(+), 5 deletions(-) >
On 23/06/2022 16:50, Jean-Michel Hautbois via libcamera-devel wrote: > Hi Naushir, > > Thanks for the series ! > > On 23/06/2022 16:44, Naushir Patuck via libcamera-devel wrote: >> Hi, >> >> This series adds a sensor temperature metadata control to libcamera. >> This >> control is returned out through Request metadata when available. >> There is an >> open PR [1] to enable the temperature sensor on the imx477 on the >> Raspberry Pi >> platforms. >> >> At present, there is no mechanism in V4L2 to return the temperature >> from the >> sensor device driver, so we rely on the embedded data to fetch the >> values every >> frame. > > I think this PR along with a V4L2 control introduction > (V4L2_CID_TEMPERATURE :-) ?) could be interesting to be sent to the > linux-media ML at the same time ? I forgot to ask the use case though ? :-) > > JM > >> >> Thanks, >> Naush >> >> [1] https://github.com/raspberrypi/linux/pull/5067 >> >> Naushir Patuck (3): >> libcamera: controls: Add SensorTemperature control >> ipa: raspberrypi: Add sensor temperature to DeviceStatus >> ipa: raspberrypi: imx477: Get sensor temperature from embedded data >> >> src/ipa/raspberrypi/cam_helper.cpp | 7 ++++--- >> src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++- >> src/ipa/raspberrypi/controller/device_status.cpp | 3 +++ >> src/ipa/raspberrypi/controller/device_status.h | 4 +++- >> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ >> src/libcamera/control_ids.yaml | 10 ++++++++++ >> 6 files changed, 26 insertions(+), 5 deletions(-) >>
Hi Jean-Michel, On Thu, 23 Jun 2022 at 15:51, Jean-Michel Hautbois < jeanmichel.hautbois@yoseli.org> wrote: > > > On 23/06/2022 16:50, Jean-Michel Hautbois via libcamera-devel wrote: > > Hi Naushir, > > > > Thanks for the series ! > > > > On 23/06/2022 16:44, Naushir Patuck via libcamera-devel wrote: > >> Hi, > >> > >> This series adds a sensor temperature metadata control to libcamera. > >> This > >> control is returned out through Request metadata when available. > >> There is an > >> open PR [1] to enable the temperature sensor on the imx477 on the > >> Raspberry Pi > >> platforms. > >> > >> At present, there is no mechanism in V4L2 to return the temperature > >> from the > >> sensor device driver, so we rely on the embedded data to fetch the > >> values every > >> frame. > > > > I think this PR along with a V4L2 control introduction > > (V4L2_CID_TEMPERATURE :-) ?) could be interesting to be sent to the > > linux-media ML at the same time ? > I was hoping to decouple this with V4L2 changes for now. From what I can tell, the topic of V4L2_CID_TEMPERATURE is a long ongoing discussion, and my change here does not rely on it. So our users can get this feature earlier! > > I forgot to ask the use case though ? :-) > We have had a few requests for this feature, see https://github.com/raspberrypi/libcamera-apps/issues/306 for the most recent one where the temperature value is useful for long exposures to help with dark frame subtraction in post processing for astrophotography. Regards, Naush > > > > > JM > > > >> > >> Thanks, > >> Naush > >> > >> [1] https://github.com/raspberrypi/linux/pull/5067 > >> > >> Naushir Patuck (3): > >> libcamera: controls: Add SensorTemperature control > >> ipa: raspberrypi: Add sensor temperature to DeviceStatus > >> ipa: raspberrypi: imx477: Get sensor temperature from embedded data > >> > >> src/ipa/raspberrypi/cam_helper.cpp | 7 ++++--- > >> src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++- > >> src/ipa/raspberrypi/controller/device_status.cpp | 3 +++ > >> src/ipa/raspberrypi/controller/device_status.h | 4 +++- > >> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > >> src/libcamera/control_ids.yaml | 10 ++++++++++ > >> 6 files changed, 26 insertions(+), 5 deletions(-) > >> >
Hi Naush and Jean-Michel On Thu, 23 Jun 2022 at 15:56, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Hi Jean-Michel, > > On Thu, 23 Jun 2022 at 15:51, Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org> wrote: >> >> >> >> On 23/06/2022 16:50, Jean-Michel Hautbois via libcamera-devel wrote: >> > Hi Naushir, >> > >> > Thanks for the series ! >> > >> > On 23/06/2022 16:44, Naushir Patuck via libcamera-devel wrote: >> >> Hi, >> >> >> >> This series adds a sensor temperature metadata control to libcamera. >> >> This >> >> control is returned out through Request metadata when available. >> >> There is an >> >> open PR [1] to enable the temperature sensor on the imx477 on the >> >> Raspberry Pi >> >> platforms. >> >> >> >> At present, there is no mechanism in V4L2 to return the temperature >> >> from the >> >> sensor device driver, so we rely on the embedded data to fetch the >> >> values every >> >> frame. >> > >> > I think this PR along with a V4L2 control introduction >> > (V4L2_CID_TEMPERATURE :-) ?) could be interesting to be sent to the >> > linux-media ML at the same time ? > > > I was hoping to decouple this with V4L2 changes for now. From what I can tell, > the topic of V4L2_CID_TEMPERATURE is a long ongoing discussion, and my > change here does not rely on it. So our users can get this feature earlier! I've flagged it for discussion at the proposed Media Summit alongside ELCE in September - https://www.spinics.net/lists/linux-media/msg212772.html Laurent has responded in support of V4L2_CID_TEMPERATURE in preference to hwmon API, but that still doesn't make it a done deal. When Benjamin brought it up in https://lore.kernel.org/linux-media/20220415111845.27130-3-benjamin.mugnier@foss.st.com/, it became quite a long debate with no resolution, leading to the patch just being dropped. As Naush says, with imx477 (and others) carrying this value in embedded metadata, we may as well extract it from there and avoid the complication. It'll actually be a more up to date value than reading it on demand over I2C. Dave >> >> >> I forgot to ask the use case though ? :-) > > > We have had a few requests for this feature, see https://github.com/raspberrypi/libcamera-apps/issues/306 > for the most recent one where the temperature value is useful for long exposures to > help with dark frame subtraction in post processing for astrophotography. > > Regards, > Naush > >> >> >> > >> > JM >> > >> >> >> >> Thanks, >> >> Naush >> >> >> >> [1] https://github.com/raspberrypi/linux/pull/5067 >> >> >> >> Naushir Patuck (3): >> >> libcamera: controls: Add SensorTemperature control >> >> ipa: raspberrypi: Add sensor temperature to DeviceStatus >> >> ipa: raspberrypi: imx477: Get sensor temperature from embedded data >> >> >> >> src/ipa/raspberrypi/cam_helper.cpp | 7 ++++--- >> >> src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++- >> >> src/ipa/raspberrypi/controller/device_status.cpp | 3 +++ >> >> src/ipa/raspberrypi/controller/device_status.h | 4 +++- >> >> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ >> >> src/libcamera/control_ids.yaml | 10 ++++++++++ >> >> 6 files changed, 26 insertions(+), 5 deletions(-) >> >>
On Thu, 23 Jun 2022 at 16:12, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > Hi Naush and Jean-Michel > > On Thu, 23 Jun 2022 at 15:56, Naushir Patuck via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: > > > > Hi Jean-Michel, > > > > On Thu, 23 Jun 2022 at 15:51, Jean-Michel Hautbois < > jeanmichel.hautbois@yoseli.org> wrote: > >> > >> > >> > >> On 23/06/2022 16:50, Jean-Michel Hautbois via libcamera-devel wrote: > >> > Hi Naushir, > >> > > >> > Thanks for the series ! > >> > > >> > On 23/06/2022 16:44, Naushir Patuck via libcamera-devel wrote: > >> >> Hi, > >> >> > >> >> This series adds a sensor temperature metadata control to libcamera. > >> >> This > >> >> control is returned out through Request metadata when available. > >> >> There is an > >> >> open PR [1] to enable the temperature sensor on the imx477 on the > >> >> Raspberry Pi > >> >> platforms. > >> >> > >> >> At present, there is no mechanism in V4L2 to return the temperature > >> >> from the > >> >> sensor device driver, so we rely on the embedded data to fetch the > >> >> values every > >> >> frame. > >> > > >> > I think this PR along with a V4L2 control introduction > >> > (V4L2_CID_TEMPERATURE :-) ?) could be interesting to be sent to the > >> > linux-media ML at the same time ? > > > > > > I was hoping to decouple this with V4L2 changes for now. From what I > can tell, > > the topic of V4L2_CID_TEMPERATURE is a long ongoing discussion, and my > > change here does not rely on it. So our users can get this feature > earlier! > > I've flagged it for discussion at the proposed Media Summit alongside > ELCE in September - > https://www.spinics.net/lists/linux-media/msg212772.html > > Laurent has responded in support of V4L2_CID_TEMPERATURE in preference > to hwmon API, but that still doesn't make it a done deal. > When Benjamin brought it up in > > https://lore.kernel.org/linux-media/20220415111845.27130-3-benjamin.mugnier@foss.st.com/ > , > it became quite a long debate with no resolution, leading to the patch > just being dropped. > > As Naush says, with imx477 (and others) carrying this value in > embedded metadata, we may as well extract it from there and avoid the > complication. It'll actually be a more up to date value than reading > it on demand over I2C. > That's a good point - embedded data values will always be guaranteed to be synced with the frame in question. But sync granularity might not be such a big deal for a temperature measurement... Naush > > Dave > > >> > >> > >> I forgot to ask the use case though ? :-) > > > > > > We have had a few requests for this feature, see > https://github.com/raspberrypi/libcamera-apps/issues/306 > > for the most recent one where the temperature value is useful for long > exposures to > > help with dark frame subtraction in post processing for astrophotography. > > > > Regards, > > Naush > > > >> > >> > >> > > >> > JM > >> > > >> >> > >> >> Thanks, > >> >> Naush > >> >> > >> >> [1] https://github.com/raspberrypi/linux/pull/5067 > >> >> > >> >> Naushir Patuck (3): > >> >> libcamera: controls: Add SensorTemperature control > >> >> ipa: raspberrypi: Add sensor temperature to DeviceStatus > >> >> ipa: raspberrypi: imx477: Get sensor temperature from embedded > data > >> >> > >> >> src/ipa/raspberrypi/cam_helper.cpp | 7 ++++--- > >> >> src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++- > >> >> src/ipa/raspberrypi/controller/device_status.cpp | 3 +++ > >> >> src/ipa/raspberrypi/controller/device_status.h | 4 +++- > >> >> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > >> >> src/libcamera/control_ids.yaml | 10 ++++++++++ > >> >> 6 files changed, 26 insertions(+), 5 deletions(-) > >> >> >
Quoting Naushir Patuck via libcamera-devel (2022-06-23 16:16:12) > On Thu, 23 Jun 2022 at 16:12, Dave Stevenson <dave.stevenson@raspberrypi.com> > wrote: > > > Hi Naush and Jean-Michel > > > > On Thu, 23 Jun 2022 at 15:56, Naushir Patuck via libcamera-devel > > <libcamera-devel@lists.libcamera.org> wrote: > > > > > > Hi Jean-Michel, > > > > > > On Thu, 23 Jun 2022 at 15:51, Jean-Michel Hautbois < > > jeanmichel.hautbois@yoseli.org> wrote: > > >> > > >> > > >> > > >> On 23/06/2022 16:50, Jean-Michel Hautbois via libcamera-devel wrote: > > >> > Hi Naushir, > > >> > > > >> > Thanks for the series ! > > >> > > > >> > On 23/06/2022 16:44, Naushir Patuck via libcamera-devel wrote: > > >> >> Hi, > > >> >> > > >> >> This series adds a sensor temperature metadata control to libcamera. > > >> >> This > > >> >> control is returned out through Request metadata when available. > > >> >> There is an > > >> >> open PR [1] to enable the temperature sensor on the imx477 on the > > >> >> Raspberry Pi > > >> >> platforms. > > >> >> > > >> >> At present, there is no mechanism in V4L2 to return the temperature > > >> >> from the > > >> >> sensor device driver, so we rely on the embedded data to fetch the > > >> >> values every > > >> >> frame. > > >> > > > >> > I think this PR along with a V4L2 control introduction > > >> > (V4L2_CID_TEMPERATURE :-) ?) could be interesting to be sent to the > > >> > linux-media ML at the same time ? > > > > > > > > > I was hoping to decouple this with V4L2 changes for now. From what I > > can tell, > > > the topic of V4L2_CID_TEMPERATURE is a long ongoing discussion, and my > > > change here does not rely on it. So our users can get this feature > > earlier! > > > > I've flagged it for discussion at the proposed Media Summit alongside > > ELCE in September - > > https://www.spinics.net/lists/linux-media/msg212772.html > > > > Laurent has responded in support of V4L2_CID_TEMPERATURE in preference > > to hwmon API, but that still doesn't make it a done deal. > > When Benjamin brought it up in > > > > https://lore.kernel.org/linux-media/20220415111845.27130-3-benjamin.mugnier@foss.st.com/ > > , > > it became quite a long debate with no resolution, leading to the patch > > just being dropped. > > > > As Naush says, with imx477 (and others) carrying this value in > > embedded metadata, we may as well extract it from there and avoid the > > complication. It'll actually be a more up to date value than reading > > it on demand over I2C. > > > > That's a good point - embedded data values will always be guaranteed to be > synced with the > frame in question. But sync granularity might not be such a big deal for a > temperature measurement... Not critical no, - but it helps to know that it's at the right frame. To me - reading this from the embedded metadata is 'more' correct anyway. (At least from our libcamera perspective where we can access that data). If we need a V4L2 control to be able to provide arbitrary reads of the value - perhaps there's merit in still exposing it as a control - but to me - having this available in the metadata is entirely suitable for use. And we don't have to wait for any kernel interface updates! Perhaps there might be sensors that don't send the temperature value in the embedded metadata, which might need the v4l2 control interface - but that's not the case here so to me this seems like the right approach overall. Now to the details in the patches ... -- Kieran > Naush > > > > > > > Dave > > > > >> > > >> > > >> I forgot to ask the use case though ? :-) > > > > > > > > > We have had a few requests for this feature, see > > https://github.com/raspberrypi/libcamera-apps/issues/306 > > > for the most recent one where the temperature value is useful for long > > exposures to > > > help with dark frame subtraction in post processing for astrophotography. > > > > > > Regards, > > > Naush > > > > > >> > > >> > > >> > > > >> > JM > > >> > > > >> >> > > >> >> Thanks, > > >> >> Naush > > >> >> > > >> >> [1] https://github.com/raspberrypi/linux/pull/5067 > > >> >> > > >> >> Naushir Patuck (3): > > >> >> libcamera: controls: Add SensorTemperature control > > >> >> ipa: raspberrypi: Add sensor temperature to DeviceStatus > > >> >> ipa: raspberrypi: imx477: Get sensor temperature from embedded > > data > > >> >> > > >> >> src/ipa/raspberrypi/cam_helper.cpp | 7 ++++--- > > >> >> src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++- > > >> >> src/ipa/raspberrypi/controller/device_status.cpp | 3 +++ > > >> >> src/ipa/raspberrypi/controller/device_status.h | 4 +++- > > >> >> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > > >> >> src/libcamera/control_ids.yaml | 10 ++++++++++ > > >> >> 6 files changed, 26 insertions(+), 5 deletions(-) > > >> >> > >
Hi Dave ! On 23/06/2022 17:12, Dave Stevenson wrote: > Hi Naush and Jean-Michel > > On Thu, 23 Jun 2022 at 15:56, Naushir Patuck via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: >> >> Hi Jean-Michel, >> >> On Thu, 23 Jun 2022 at 15:51, Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org> wrote: >>> >>> >>> >>> On 23/06/2022 16:50, Jean-Michel Hautbois via libcamera-devel wrote: >>>> Hi Naushir, >>>> >>>> Thanks for the series ! >>>> >>>> On 23/06/2022 16:44, Naushir Patuck via libcamera-devel wrote: >>>>> Hi, >>>>> >>>>> This series adds a sensor temperature metadata control to libcamera. >>>>> This >>>>> control is returned out through Request metadata when available. >>>>> There is an >>>>> open PR [1] to enable the temperature sensor on the imx477 on the >>>>> Raspberry Pi >>>>> platforms. >>>>> >>>>> At present, there is no mechanism in V4L2 to return the temperature >>>>> from the >>>>> sensor device driver, so we rely on the embedded data to fetch the >>>>> values every >>>>> frame. >>>> >>>> I think this PR along with a V4L2 control introduction >>>> (V4L2_CID_TEMPERATURE :-) ?) could be interesting to be sent to the >>>> linux-media ML at the same time ? >> >> >> I was hoping to decouple this with V4L2 changes for now. From what I can tell, >> the topic of V4L2_CID_TEMPERATURE is a long ongoing discussion, and my >> change here does not rely on it. So our users can get this feature earlier! > > I've flagged it for discussion at the proposed Media Summit alongside > ELCE in September - > https://www.spinics.net/lists/linux-media/msg212772.html > > Laurent has responded in support of V4L2_CID_TEMPERATURE in preference > to hwmon API, but that still doesn't make it a done deal. > When Benjamin brought it up in > https://lore.kernel.org/linux-media/20220415111845.27130-3-benjamin.mugnier@foss.st.com/, > it became quite a long debate with no resolution, leading to the patch > just being dropped. Thanks for the discussion :-) ! > > As Naush says, with imx477 (and others) carrying this value in > embedded metadata, we may as well extract it from there and avoid the > complication. It'll actually be a more up to date value than reading > it on demand over I2C. Yep, I get the point ;-). > > Dave > >>> >>> >>> I forgot to ask the use case though ? :-) >> >> >> We have had a few requests for this feature, see https://github.com/raspberrypi/libcamera-apps/issues/306 >> for the most recent one where the temperature value is useful for long exposures to >> help with dark frame subtraction in post processing for astrophotography. Thanks for the reference :-). JM >> >> Regards, >> Naush >> >>> >>> >>>> >>>> JM >>>> >>>>> >>>>> Thanks, >>>>> Naush >>>>> >>>>> [1] https://github.com/raspberrypi/linux/pull/5067 >>>>> >>>>> Naushir Patuck (3): >>>>> libcamera: controls: Add SensorTemperature control >>>>> ipa: raspberrypi: Add sensor temperature to DeviceStatus >>>>> ipa: raspberrypi: imx477: Get sensor temperature from embedded data >>>>> >>>>> src/ipa/raspberrypi/cam_helper.cpp | 7 ++++--- >>>>> src/ipa/raspberrypi/cam_helper_imx477.cpp | 5 ++++- >>>>> src/ipa/raspberrypi/controller/device_status.cpp | 3 +++ >>>>> src/ipa/raspberrypi/controller/device_status.h | 4 +++- >>>>> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ >>>>> src/libcamera/control_ids.yaml | 10 ++++++++++ >>>>> 6 files changed, 26 insertions(+), 5 deletions(-) >>>>>