| Message ID | 20210622023654.969162-4-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro On Tue, Jun 22, 2021 at 11:36:53AM +0900, Hirokazu Honda wrote: > Apply a camera sensor a requested test pattern mode. The test Apply to the ... > pattern mode can be specified per frame. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/pipeline/ipu3/cio2.cpp | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index 8548f749..9d1ff07d 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -12,6 +12,7 @@ > #include <libcamera/control_ids.h> > #include <libcamera/formats.h> > #include <libcamera/geometry.h> > +#include <libcamera/request.h> > #include <libcamera/stream.h> > > #include "libcamera/internal/camera_sensor.h" > @@ -289,6 +290,20 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer) > buffer->setRequest(request); > } > > + if (request->controls().contains(controls::draft::TestPatternMode)) { > + const uint8_t testPatternMode = > + static_cast<uint8_t>(request->controls().get( > + controls::draft::TestPatternMode)); > + int ret = sensor_->setTestPatternMode(testPatternMode); > + if (ret) { > + LOG(IPU3, Error) > + << "Failed to set test pattern mode: " << ret; > + } else { > + request->metadata().set(controls::draft::TestPatternMode, > + testPatternMode); > + } > + } > + Correct me if I'm wrong, but if we have 3 buffers queued to the CIO2, we queue a 4th one with test pattern enabled and we enable it on the sensor here, won't the 3 frames that complete before this one contain the test pattern ? Cc-ed Laurent to rope him in for this question. Also, can we enable/disable test patter while streaming ? Have you tested this ? > int ret = output_->queueBuffer(buffer); > if (ret) > return nullptr; > -- > 2.32.0.288.g62a8d224e6-goog >
Hi Jacopo, On Tue, Jun 22, 2021 at 7:42 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro > > On Tue, Jun 22, 2021 at 11:36:53AM +0900, Hirokazu Honda wrote: > > Apply a camera sensor a requested test pattern mode. The test > > Apply to the ... > > > pattern mode can be specified per frame. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/libcamera/pipeline/ipu3/cio2.cpp | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > index 8548f749..9d1ff07d 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > @@ -12,6 +12,7 @@ > > #include <libcamera/control_ids.h> > > #include <libcamera/formats.h> > > #include <libcamera/geometry.h> > > +#include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > #include "libcamera/internal/camera_sensor.h" > > @@ -289,6 +290,20 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer) > > buffer->setRequest(request); > > } > > > > + if (request->controls().contains(controls::draft::TestPatternMode)) { > > + const uint8_t testPatternMode = > > + static_cast<uint8_t>(request->controls().get( > > + controls::draft::TestPatternMode)); > > + int ret = sensor_->setTestPatternMode(testPatternMode); > > + if (ret) { > > + LOG(IPU3, Error) > > + << "Failed to set test pattern mode: " << ret; > > + } else { > > + request->metadata().set(controls::draft::TestPatternMode, > > + testPatternMode); > > + } > > + } > > + > > Correct me if I'm wrong, but if we have 3 buffers queued to the CIO2, > we queue a 4th one with test pattern enabled and we enable it on the > sensor here, won't the 3 frames that complete before this one contain > the test pattern ? Cc-ed Laurent to rope him in for this question. > > Also, can we enable/disable test patter while streaming ? Have you > tested this ? I haven't tested that. We don't have such a test. I have the same question. My guess is applying the test pattern mode on fly affects the preceding queued buffers. In intel HAL implementation, it waits until the preceding queued buffers have been dequeued before applying the test pattern mode. We have to do that in libcamera too. -Hiro > > > int ret = output_->queueBuffer(buffer); > > if (ret) > > return nullptr; > > -- > > 2.32.0.288.g62a8d224e6-goog > >
+Laurent Pinchart On Wed, Jun 23, 2021 at 4:36 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > Hi Jacopo, > > On Tue, Jun 22, 2021 at 7:42 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Hiro > > > > On Tue, Jun 22, 2021 at 11:36:53AM +0900, Hirokazu Honda wrote: > > > Apply a camera sensor a requested test pattern mode. The test > > > > Apply to the ... > > > > > pattern mode can be specified per frame. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > src/libcamera/pipeline/ipu3/cio2.cpp | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > index 8548f749..9d1ff07d 100644 > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > @@ -12,6 +12,7 @@ > > > #include <libcamera/control_ids.h> > > > #include <libcamera/formats.h> > > > #include <libcamera/geometry.h> > > > +#include <libcamera/request.h> > > > #include <libcamera/stream.h> > > > > > > #include "libcamera/internal/camera_sensor.h" > > > @@ -289,6 +290,20 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer) > > > buffer->setRequest(request); > > > } > > > > > > + if (request->controls().contains(controls::draft::TestPatternMode)) { > > > + const uint8_t testPatternMode = > > > + static_cast<uint8_t>(request->controls().get( > > > + controls::draft::TestPatternMode)); > > > + int ret = sensor_->setTestPatternMode(testPatternMode); > > > + if (ret) { > > > + LOG(IPU3, Error) > > > + << "Failed to set test pattern mode: " << ret; > > > + } else { > > > + request->metadata().set(controls::draft::TestPatternMode, > > > + testPatternMode); > > > + } > > > + } > > > + > > > > Correct me if I'm wrong, but if we have 3 buffers queued to the CIO2, > > we queue a 4th one with test pattern enabled and we enable it on the > > sensor here, won't the 3 frames that complete before this one contain > > the test pattern ? Cc-ed Laurent to rope him in for this question. > > > > Also, can we enable/disable test patter while streaming ? Have you > > tested this ? > > I haven't tested that. We don't have such a test. > I have the same question. My guess is applying the test pattern mode > on fly affects the preceding queued buffers. > In intel HAL implementation, it waits until the preceding queued > buffers have been dequeued before applying the test pattern mode. > We have to do that in libcamera too. > Laurent, what do you think the best to do this? I am thinking to wait in each pipeline handler by queueing pending capture requests until that time. > -Hiro > > > > > > > int ret = output_->queueBuffer(buffer); > > > if (ret) > > > return nullptr; > > > -- > > > 2.32.0.288.g62a8d224e6-goog > > >
On 22/06/2021 11:43, Jacopo Mondi wrote: > On Tue, Jun 22, 2021 at 11:36:53AM +0900, Hirokazu Honda wrote: >> Apply a camera sensor a requested test pattern mode. The test > > Apply to the ... "Apply the requested test pattern mode to the camera sensor."
Hi Hiro, On Wed, Jun 23, 2021 at 05:08:38PM +0900, Hirokazu Honda wrote: > On Wed, Jun 23, 2021 at 4:36 PM Hirokazu Honda wrote: > > On Tue, Jun 22, 2021 at 7:42 PM Jacopo Mondi wrote: > > > On Tue, Jun 22, 2021 at 11:36:53AM +0900, Hirokazu Honda wrote: > > > > Apply a camera sensor a requested test pattern mode. The test > > > > > > Apply to the ... > > > > > > > pattern mode can be specified per frame. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 15 +++++++++++++++ > > > > 1 file changed, 15 insertions(+) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > index 8548f749..9d1ff07d 100644 > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > @@ -12,6 +12,7 @@ > > > > #include <libcamera/control_ids.h> > > > > #include <libcamera/formats.h> > > > > #include <libcamera/geometry.h> > > > > +#include <libcamera/request.h> > > > > #include <libcamera/stream.h> > > > > > > > > #include "libcamera/internal/camera_sensor.h" > > > > @@ -289,6 +290,20 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer) > > > > buffer->setRequest(request); > > > > } > > > > > > > > + if (request->controls().contains(controls::draft::TestPatternMode)) { > > > > + const uint8_t testPatternMode = > > > > + static_cast<uint8_t>(request->controls().get( > > > > + controls::draft::TestPatternMode)); > > > > + int ret = sensor_->setTestPatternMode(testPatternMode); > > > > + if (ret) { > > > > + LOG(IPU3, Error) > > > > + << "Failed to set test pattern mode: " << ret; > > > > + } else { > > > > + request->metadata().set(controls::draft::TestPatternMode, > > > > + testPatternMode); > > > > + } > > > > + } > > > > + > > > > > > Correct me if I'm wrong, but if we have 3 buffers queued to the CIO2, > > > we queue a 4th one with test pattern enabled and we enable it on the > > > sensor here, won't the 3 frames that complete before this one contain > > > the test pattern ? Cc-ed Laurent to rope him in for this question. > > > > > > Also, can we enable/disable test patter while streaming ? Have you > > > tested this ? > > > > I haven't tested that. We don't have such a test. > > I have the same question. My guess is applying the test pattern mode > > on fly affects the preceding queued buffers. > > In intel HAL implementation, it waits until the preceding queued > > buffers have been dequeued before applying the test pattern mode. > > We have to do that in libcamera too. Yes, this needs to be handled correctly to work in per-frame-control mode. > Laurent, what do you think the best to do this? > I am thinking to wait in each pipeline handler by queueing pending > capture requests until that time. Implementing this in the pipeline handler is the way to go. The IPA also needs to implement such a mechanism for most controls. We could handle the test pattern mode the same way by passing it to the IPA that would pass it back at the right time, but this isn't implemented in the open-source IPU3 IPA yet, and it would likely complicate support of the closed-source IPA. I'm thus thinking it may be better to handle the test pattern mode in the pipeline handler. The implementation should keep requests in a queue, and apply the correct control value in the handler of the CIO2 frameStart signal. I'd do this in ipu3.cpp, not cio2.cpp. The frameStart signal is currently connected to the DelayedControls::applyControls() function, you will need to connect it to a function of IPU3CameraData, call DelayedControls::applyControls() there, and apply the test pattern control too. > > > > int ret = output_->queueBuffer(buffer); > > > > if (ret) > > > > return nullptr;
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index 8548f749..9d1ff07d 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -12,6 +12,7 @@ #include <libcamera/control_ids.h> #include <libcamera/formats.h> #include <libcamera/geometry.h> +#include <libcamera/request.h> #include <libcamera/stream.h> #include "libcamera/internal/camera_sensor.h" @@ -289,6 +290,20 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer) buffer->setRequest(request); } + if (request->controls().contains(controls::draft::TestPatternMode)) { + const uint8_t testPatternMode = + static_cast<uint8_t>(request->controls().get( + controls::draft::TestPatternMode)); + int ret = sensor_->setTestPatternMode(testPatternMode); + if (ret) { + LOG(IPU3, Error) + << "Failed to set test pattern mode: " << ret; + } else { + request->metadata().set(controls::draft::TestPatternMode, + testPatternMode); + } + } + int ret = output_->queueBuffer(buffer); if (ret) return nullptr;
Apply a camera sensor a requested test pattern mode. The test pattern mode can be specified per frame. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/libcamera/pipeline/ipu3/cio2.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+)