| Message ID | 20210218124824.1825418-5-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | b147de25537652f8dabf40d5165350dad083a28a |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On Thu, Feb 18, 2021 at 12:48:24PM +0000, Naushir Patuck wrote: > Only update the lens shading control if it is present in the > ControlList. > > Add the dmabuf file descriptor to the lens shading control in-place > rather than taking a copy. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 21 +++++++++---------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index acf2d56cddb2..a60415d93705 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId) > > void RPiCameraData::setIspControls(const ControlList &controls) > { > - ControlList ctrls = controls; > - > - Span<const uint8_t> s = > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > - bcm2835_isp_lens_shading ls = > - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); > - ls.dmabuf = lsTable_.fd(); > - > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), > - sizeof(ls) }); > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + ControlList ctrls = std::move(controls); > + > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > + ControlValue &value = > + const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)); > + Span<uint8_t> s = value.data(); > + bcm2835_isp_lens_shading *ls = > + reinterpret_cast<bcm2835_isp_lens_shading *>(s.data()); > + ls->dmabuf = lsTable_.fd(); > + } > > isp_[Isp::Input].dev()->setControls(&ctrls); > handleState(); > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On 18/02/2021 12:48, Naushir Patuck wrote: > Only update the lens shading control if it is present in the > ControlList. > > Add the dmabuf file descriptor to the lens shading control in-place > rather than taking a copy. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 21 +++++++++---------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index acf2d56cddb2..a60415d93705 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId) > > void RPiCameraData::setIspControls(const ControlList &controls) > { > - ControlList ctrls = controls; > - > - Span<const uint8_t> s = > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > - bcm2835_isp_lens_shading ls = > - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); > - ls.dmabuf = lsTable_.fd(); > - > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), > - sizeof(ls) }); > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + ControlList ctrls = std::move(controls); > + > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > + ControlValue &value = > + const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)); > + Span<uint8_t> s = value.data(); > + bcm2835_isp_lens_shading *ls = > + reinterpret_cast<bcm2835_isp_lens_shading *>(s.data()); > + ls->dmabuf = lsTable_.fd(); > + } > > isp_[Isp::Input].dev()->setControls(&ctrls); > handleState(); >
Hi Naush, Thank you for the patch. On Thu, Feb 18, 2021 at 12:48:24PM +0000, Naushir Patuck wrote: > Only update the lens shading control if it is present in the > ControlList. > > Add the dmabuf file descriptor to the lens shading control in-place > rather than taking a copy. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 21 +++++++++---------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index acf2d56cddb2..a60415d93705 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId) > > void RPiCameraData::setIspControls(const ControlList &controls) > { > - ControlList ctrls = controls; > - > - Span<const uint8_t> s = > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > - bcm2835_isp_lens_shading ls = > - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); > - ls.dmabuf = lsTable_.fd(); > - > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), > - sizeof(ls) }); > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + ControlList ctrls = std::move(controls); std::move() won't have the expected effect here. controls is a const reference, so ctrls will be a copy. It's a bit annoying that the compiler doesn't warn us :-/ The code will run fine, but I think avoiding std::move() would be more explicit as it's easy to overlook the fact that a copy is created otherwise. No need to change this now, but if you get the chance at some point, it would be nice. > + > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > + ControlValue &value = > + const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)); The const_cast is not nice. If we want to support this kind of usage, we should extend the ControlList API to return a non-const ControlValue. > + Span<uint8_t> s = value.data(); > + bcm2835_isp_lens_shading *ls = > + reinterpret_cast<bcm2835_isp_lens_shading *>(s.data()); > + ls->dmabuf = lsTable_.fd(); > + } > > isp_[Isp::Input].dev()->setControls(&ctrls); > handleState();
Hi Laurent, On Sun, 21 Feb 2021 at 13:07, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Thu, Feb 18, 2021 at 12:48:24PM +0000, Naushir Patuck wrote: > > Only update the lens shading control if it is present in the > > ControlList. > > > > Add the dmabuf file descriptor to the lens shading control in-place > > rather than taking a copy. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 +++++++++---------- > > 1 file changed, 10 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index acf2d56cddb2..a60415d93705 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t > bufferId) > > > > void RPiCameraData::setIspControls(const ControlList &controls) > > { > > - ControlList ctrls = controls; > > - > > - Span<const uint8_t> s = > > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > > - bcm2835_isp_lens_shading ls = > > - *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data()); > > - ls.dmabuf = lsTable_.fd(); > > - > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t > *>(&ls), > > - sizeof(ls) }); > > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > > + ControlList ctrls = std::move(controls); > > std::move() won't have the expected effect here. controls is a const > reference, so ctrls will be a copy. It's a bit annoying that the > compiler doesn't warn us :-/ > > The code will run fine, but I think avoiding std::move() would be more > explicit as it's easy to overlook the fact that a copy is created > otherwise. No need to change this now, but if you get the chance at some > point, it would be nice. > Understood, will put it on my todo list. Just curious though, what is the issue with the old method of getting the IPA to fill in the file descriptor in the struct? This would avoid the need of modifying the control list like this in the pipeline handler. Is it to do with testing process isolation between the pipeline handler and IPA? Thanks, Naush > > > + > > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > > + ControlValue &value = > > + const_cast<ControlValue > &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)); > > The const_cast is not nice. If we want to support this kind of usage, we > should extend the ControlList API to return a non-const ControlValue. > > > + Span<uint8_t> s = value.data(); > > + bcm2835_isp_lens_shading *ls = > > + reinterpret_cast<bcm2835_isp_lens_shading > *>(s.data()); > > + ls->dmabuf = lsTable_.fd(); > > + } > > > > isp_[Isp::Input].dev()->setControls(&ctrls); > > handleState(); > > -- > Regards, > > Laurent Pinchart >
On Sun, Feb 21, 2021 at 07:39:38PM +0000, Naushir Patuck wrote: > On Sun, 21 Feb 2021 at 13:07, Laurent Pinchart wrote: > > On Thu, Feb 18, 2021 at 12:48:24PM +0000, Naushir Patuck wrote: > > > Only update the lens shading control if it is present in the > > > ControlList. > > > > > > Add the dmabuf file descriptor to the lens shading control in-place > > > rather than taking a copy. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 +++++++++---------- > > > 1 file changed, 10 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index acf2d56cddb2..a60415d93705 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId) > > > > > > void RPiCameraData::setIspControls(const ControlList &controls) > > > { > > > - ControlList ctrls = controls; > > > - > > > - Span<const uint8_t> s = > > > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > > > - bcm2835_isp_lens_shading ls = > > > - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); > > > - ls.dmabuf = lsTable_.fd(); > > > - > > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), > > > - sizeof(ls) }); > > > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > > > + ControlList ctrls = std::move(controls); > > > > std::move() won't have the expected effect here. controls is a const > > reference, so ctrls will be a copy. It's a bit annoying that the > > compiler doesn't warn us :-/ > > > > The code will run fine, but I think avoiding std::move() would be more > > explicit as it's easy to overlook the fact that a copy is created > > otherwise. No need to change this now, but if you get the chance at some > > point, it would be nice. > > Understood, will put it on my todo list. > > Just curious though, what is the issue with the old method of getting the > IPA to fill in the file descriptor in the struct? > This would avoid the need of modifying the control list like this in the > pipeline handler. Is it to do with testing process > isolation between the pipeline handler and IPA? Yes, it's related to process isolation. When the IPA is run within the libcamera process, file descriptors are just integers, and they can be passed back and forth between the pipeline handler and IPA without any issue. When isolating an IPA in a process, the file descriptors have to be passed through the IPC pipe (the IPA IPC mechanism handles this transparently), and the receiving side will see the file descriptor as an integer local to the process, whose numerical value will usually be different than the one in the sending process. The lens shading table file descriptor can be used in the IPA to map the buffer, but if its numerical value is set in the V4L2_CID_USER_BCM2835_ISP_LENS_SHADING control, there will be no translation on the way back to the pipeline handler as the IPA IPC layer isn't aware that the V4L2 control contains a file descriptor. The control will be set by the libcamera process, using a file descriptor value that corresponds to the IPA process. One way to handle this would be to pass both the file descriptor and its value as an integer to the IPA, but it's a bit of a hack, the IPA shouldn't have to care about this. That's why it's patched in the pipeline handler. > > > + > > > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > > > + ControlValue &value = > > > + const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)); > > > > The const_cast is not nice. If we want to support this kind of usage, we > > should extend the ControlList API to return a non-const ControlValue. Do you think we should have such an extension ? > > > + Span<uint8_t> s = value.data(); > > > + bcm2835_isp_lens_shading *ls = > > > + reinterpret_cast<bcm2835_isp_lens_shading *>(s.data()); > > > + ls->dmabuf = lsTable_.fd(); > > > + } > > > > > > isp_[Isp::Input].dev()->setControls(&ctrls); > > > handleState();
Hi Laurent, On Sun, 21 Feb 2021, 7:52 pm Laurent Pinchart, < laurent.pinchart@ideasonboard.com> wrote: > On Sun, Feb 21, 2021 at 07:39:38PM +0000, Naushir Patuck wrote: > > On Sun, 21 Feb 2021 at 13:07, Laurent Pinchart wrote: > > > On Thu, Feb 18, 2021 at 12:48:24PM +0000, Naushir Patuck wrote: > > > > Only update the lens shading control if it is present in the > > > > ControlList. > > > > > > > > Add the dmabuf file descriptor to the lens shading control in-place > > > > rather than taking a copy. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 > +++++++++---------- > > > > 1 file changed, 10 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index acf2d56cddb2..a60415d93705 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -1338,17 +1338,16 @@ void > RPiCameraData::embeddedComplete(uint32_t bufferId) > > > > > > > > void RPiCameraData::setIspControls(const ControlList &controls) > > > > { > > > > - ControlList ctrls = controls; > > > > - > > > > - Span<const uint8_t> s = > > > > - > ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > > > > - bcm2835_isp_lens_shading ls = > > > > - *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data()); > > > > - ls.dmabuf = lsTable_.fd(); > > > > - > > > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t > *>(&ls), > > > > - sizeof(ls) }); > > > > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > > > > + ControlList ctrls = std::move(controls); > > > > > > std::move() won't have the expected effect here. controls is a const > > > reference, so ctrls will be a copy. It's a bit annoying that the > > > compiler doesn't warn us :-/ > > > > > > The code will run fine, but I think avoiding std::move() would be more > > > explicit as it's easy to overlook the fact that a copy is created > > > otherwise. No need to change this now, but if you get the chance at > some > > > point, it would be nice. > > > > Understood, will put it on my todo list. > > > > Just curious though, what is the issue with the old method of getting the > > IPA to fill in the file descriptor in the struct? > > This would avoid the need of modifying the control list like this in the > > pipeline handler. Is it to do with testing process > > isolation between the pipeline handler and IPA? > > Yes, it's related to process isolation. When the IPA is run within the > libcamera process, file descriptors are just integers, and they can be > passed back and forth between the pipeline handler and IPA without any > issue. > > When isolating an IPA in a process, the file descriptors have to be > passed through the IPC pipe (the IPA IPC mechanism handles this > transparently), and the receiving side will see the file descriptor as > an integer local to the process, whose numerical value will usually be > different than the one in the sending process. The lens shading table > file descriptor can be used in the IPA to map the buffer, but if its > numerical value is set in the V4L2_CID_USER_BCM2835_ISP_LENS_SHADING > control, there will be no translation on the way back to the pipeline > handler as the IPA IPC layer isn't aware that the V4L2 control contains > a file descriptor. The control will be set by the libcamera process, > using a file descriptor value that corresponds to the IPA process. > > One way to handle this would be to pass both the file descriptor and its > value as an integer to the IPA, but it's a bit of a hack, the IPA > shouldn't have to care about this. That's why it's patched in the > pipeline handler. > > > > > + > > > > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > > > > + ControlValue &value = > > > > + const_cast<ControlValue > &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)); > > > > > > The const_cast is not nice. If we want to support this kind of usage, > we > > > should extend the ControlList API to return a non-const ControlValue. > > Do you think we should have such an extension ? > Yes please :-) This would certainly simplify code like the above! > > > > + Span<uint8_t> s = value.data(); > > > > + bcm2835_isp_lens_shading *ls = > > > > + reinterpret_cast<bcm2835_isp_lens_shading > *>(s.data()); > > > > + ls->dmabuf = lsTable_.fd(); > > > > + } > > > > > > > > isp_[Isp::Input].dev()->setControls(&ctrls); > > > > handleState(); > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index acf2d56cddb2..a60415d93705 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId) void RPiCameraData::setIspControls(const ControlList &controls) { - ControlList ctrls = controls; - - Span<const uint8_t> s = - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); - bcm2835_isp_lens_shading ls = - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); - ls.dmabuf = lsTable_.fd(); - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), - sizeof(ls) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); + ControlList ctrls = std::move(controls); + + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { + ControlValue &value = + const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)); + Span<uint8_t> s = value.data(); + bcm2835_isp_lens_shading *ls = + reinterpret_cast<bcm2835_isp_lens_shading *>(s.data()); + ls->dmabuf = lsTable_.fd(); + } isp_[Isp::Input].dev()->setControls(&ctrls); handleState();
Only update the lens shading control if it is present in the ControlList. Add the dmabuf file descriptor to the lens shading control in-place rather than taking a copy. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-)