| Message ID | 20201105001546.1690179-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On 05/11/2020 00:15, Niklas Söderlund wrote: > There is no reason to expose and call a separate configureStat() when > the statistics video device can be configured with the exact same > parameters as part of configure(). Move the configuration internally to > the ImgUDevice simplifying the interface, there is no functional change. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 7 +++++++ > src/libcamera/pipeline/ipu3/imgu.h | 7 ------- > src/libcamera/pipeline/ipu3/ipu3.cpp | 11 ----------- > 3 files changed, 7 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index a4d74a62f69a0c97..0a3bf62020fd23fb 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -477,6 +477,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF > > LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString(); > > + StreamConfiguration statCfg = {}; > + statCfg.size = inputFormat->size; > + V4L2DeviceFormat statFormat; > + ret = configureVideoDevice(stat_.get(), PAD_STAT, statCfg, &statFormat); > + if (ret) > + return ret; > + Shouldn't you do something with statCfg or statFormat? Hrm ... maybe this happens later ... > return 0; > } > > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > index c73ac5a5a37cfe0e..37f5ae77c99ff8fe 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.h > +++ b/src/libcamera/pipeline/ipu3/imgu.h > @@ -61,13 +61,6 @@ public: > outputFormat); > } > > - int configureStat(const StreamConfiguration &cfg, > - V4L2DeviceFormat *outputFormat) > - { > - return configureVideoDevice(stat_.get(), PAD_STAT, cfg, > - outputFormat);> - } > - > int allocateBuffers(unsigned int bufferCount); > void freeBuffers(); > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 3f0232bc1eaad048..c559d160084f87e7 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -518,17 +518,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > } > > - /* > - * Apply the largest available format to the stat node. > - * \todo Revise this when we'll actually use the stat node. > - */ > - StreamConfiguration statCfg = {}; > - statCfg.size = cio2Format.size; > - > - ret = imgu->configureStat(statCfg, &outputFormat); was outputFormat ignored/unused before ? perhaps it was... Ok - Given that this is just about the code move, and I think this is unused here, that explains why this commit doesn't 'use' the statCfg or statFormat above - so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > - if (ret) > - return ret; > - > /* Apply the "pipe_mode" control to the ImgU subdevice. */ > ControlList ctrls(imgu->imgu_->controls()); > ctrls.set(V4L2_CID_IPU3_PIPE_MODE, >
Hi Niklas, On Thu, Nov 05, 2020 at 01:15:38AM +0100, Niklas Söderlund wrote: > There is no reason to expose and call a separate configureStat() when > the statistics video device can be configured with the exact same > parameters as part of configure(). Move the configuration internally to > the ImgUDevice simplifying the interface, there is no functional change. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 7 +++++++ > src/libcamera/pipeline/ipu3/imgu.h | 7 ------- > src/libcamera/pipeline/ipu3/ipu3.cpp | 11 ----------- > 3 files changed, 7 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index a4d74a62f69a0c97..0a3bf62020fd23fb 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -477,6 +477,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF > > LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString(); > > + StreamConfiguration statCfg = {}; > + statCfg.size = inputFormat->size; > + V4L2DeviceFormat statFormat; > + ret = configureVideoDevice(stat_.get(), PAD_STAT, statCfg, &statFormat); > + if (ret) > + return ret; > + This was in the same call path before, but just to make sure, I read in ImgUDevice::configureVideoDevice(): /* No need to apply format to the stat node. */ if (dev == stat_.get()) return 0; No size need to be configured on the stat video device, but only on the ImgU stat source pad ? Thanks j > return 0; > } > > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > index c73ac5a5a37cfe0e..37f5ae77c99ff8fe 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.h > +++ b/src/libcamera/pipeline/ipu3/imgu.h > @@ -61,13 +61,6 @@ public: > outputFormat); > } > > - int configureStat(const StreamConfiguration &cfg, > - V4L2DeviceFormat *outputFormat) > - { > - return configureVideoDevice(stat_.get(), PAD_STAT, cfg, > - outputFormat); > - } > - > int allocateBuffers(unsigned int bufferCount); > void freeBuffers(); > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 3f0232bc1eaad048..c559d160084f87e7 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -518,17 +518,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > } > > - /* > - * Apply the largest available format to the stat node. > - * \todo Revise this when we'll actually use the stat node. > - */ > - StreamConfiguration statCfg = {}; > - statCfg.size = cio2Format.size; > - > - ret = imgu->configureStat(statCfg, &outputFormat); > - if (ret) > - return ret; > - > /* Apply the "pipe_mode" control to the ImgU subdevice. */ > ControlList ctrls(imgu->imgu_->controls()); > ctrls.set(V4L2_CID_IPU3_PIPE_MODE, > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Mon, Nov 09, 2020 at 11:18:50AM +0100, Jacopo Mondi wrote: > On Thu, Nov 05, 2020 at 01:15:38AM +0100, Niklas Söderlund wrote: > > There is no reason to expose and call a separate configureStat() when > > the statistics video device can be configured with the exact same > > parameters as part of configure(). Move the configuration internally to > > the ImgUDevice simplifying the interface, there is no functional change. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/ipu3/imgu.cpp | 7 +++++++ > > src/libcamera/pipeline/ipu3/imgu.h | 7 ------- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 11 ----------- > > 3 files changed, 7 insertions(+), 18 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > index a4d74a62f69a0c97..0a3bf62020fd23fb 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > @@ -477,6 +477,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF > > > > LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString(); > > > > + StreamConfiguration statCfg = {}; > > + statCfg.size = inputFormat->size; > > + V4L2DeviceFormat statFormat; > > + ret = configureVideoDevice(stat_.get(), PAD_STAT, statCfg, &statFormat); Maybe we should make the last parameter optional at some point, not sure. > > + if (ret) > > + return ret; > > + > > This was in the same call path before, but just to make sure, I read > in ImgUDevice::configureVideoDevice(): > > /* No need to apply format to the stat node. */ > if (dev == stat_.get()) > return 0; > > No size need to be configured on the stat video device, but only on > the ImgU stat source pad ? The ImgU driver sets .vidioc_s_fmt_meta_cap to imgu_vidioc_g_meta_fmt(), so there's indeed no need to set the format on the video node. We may want to capture this in a comment in ImgUDevice::configureVideoDevice() (strictly speaking out of scope for this patch, but I wouldn't mind adding it here as a separate patch seems a bit overkill - I however know how much Niklas likes to split patches :-)). I've had a quick look at the subdev configuration, and it seems to format on the stats pad is completely ignored, so we could even simplify the code further. This needs to be double-checked though, I'm not very familiar with the driver. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > return 0; > > } > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > > index c73ac5a5a37cfe0e..37f5ae77c99ff8fe 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.h > > +++ b/src/libcamera/pipeline/ipu3/imgu.h > > @@ -61,13 +61,6 @@ public: > > outputFormat); > > } > > > > - int configureStat(const StreamConfiguration &cfg, > > - V4L2DeviceFormat *outputFormat) > > - { > > - return configureVideoDevice(stat_.get(), PAD_STAT, cfg, > > - outputFormat); > > - } > > - > > int allocateBuffers(unsigned int bufferCount); > > void freeBuffers(); > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 3f0232bc1eaad048..c559d160084f87e7 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -518,17 +518,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > return ret; > > } > > > > - /* > > - * Apply the largest available format to the stat node. > > - * \todo Revise this when we'll actually use the stat node. > > - */ > > - StreamConfiguration statCfg = {}; > > - statCfg.size = cio2Format.size; > > - > > - ret = imgu->configureStat(statCfg, &outputFormat); > > - if (ret) > > - return ret; > > - > > /* Apply the "pipe_mode" control to the ImgU subdevice. */ > > ControlList ctrls(imgu->imgu_->controls()); > > ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index a4d74a62f69a0c97..0a3bf62020fd23fb 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -477,6 +477,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString(); + StreamConfiguration statCfg = {}; + statCfg.size = inputFormat->size; + V4L2DeviceFormat statFormat; + ret = configureVideoDevice(stat_.get(), PAD_STAT, statCfg, &statFormat); + if (ret) + return ret; + return 0; } diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h index c73ac5a5a37cfe0e..37f5ae77c99ff8fe 100644 --- a/src/libcamera/pipeline/ipu3/imgu.h +++ b/src/libcamera/pipeline/ipu3/imgu.h @@ -61,13 +61,6 @@ public: outputFormat); } - int configureStat(const StreamConfiguration &cfg, - V4L2DeviceFormat *outputFormat) - { - return configureVideoDevice(stat_.get(), PAD_STAT, cfg, - outputFormat); - } - int allocateBuffers(unsigned int bufferCount); void freeBuffers(); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 3f0232bc1eaad048..c559d160084f87e7 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -518,17 +518,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) return ret; } - /* - * Apply the largest available format to the stat node. - * \todo Revise this when we'll actually use the stat node. - */ - StreamConfiguration statCfg = {}; - statCfg.size = cio2Format.size; - - ret = imgu->configureStat(statCfg, &outputFormat); - if (ret) - return ret; - /* Apply the "pipe_mode" control to the ImgU subdevice. */ ControlList ctrls(imgu->imgu_->controls()); ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
There is no reason to expose and call a separate configureStat() when the statistics video device can be configured with the exact same parameters as part of configure(). Move the configuration internally to the ImgUDevice simplifying the interface, there is no functional change. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/ipu3/imgu.cpp | 7 +++++++ src/libcamera/pipeline/ipu3/imgu.h | 7 ------- src/libcamera/pipeline/ipu3/ipu3.cpp | 11 ----------- 3 files changed, 7 insertions(+), 18 deletions(-)