| Message ID | 20190220131757.14004-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your patch. On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > Return default configuration for the output stream produced by the imgu > 'output' video node. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > 1 file changed, 3 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 9694d0ce51ab..9065073913a2 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera, > { > IPU3CameraData *data = cameraData(camera); > std::map<Stream *, StreamConfiguration> configs; > - V4L2SubdeviceFormat format = {}; > - > - /* > - * FIXME: As of now, return the image format reported by the sensor. > - * In future good defaults should be provided for each stream. > - */ > - if (data->sensor_->getFormat(0, &format)) { > - LOG(IPU3, Error) << "Failed to create stream configurations"; > - return configs; > - } > > StreamConfiguration config = {}; > - config.width = format.width; > - config.height = format.height; > - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > + config.width = 2560; > + config.height = 1920; > + config.pixelFormat = V4L2_PIX_FMT_NV12; This is a good change and trust that the format you selected are the correct one so I have not verified it by reading or testing on the IPU3. Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> I'm just curious why the pixelformat changes, did something change in the upstream driver or is this in preparation of some later change? > config.bufferCount = 4; > > configs[&data->stream_] = config; > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your patch. > > On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > > Return default configuration for the output stream produced by the imgu > > 'output' video node. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > > 1 file changed, 3 insertions(+), 13 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 9694d0ce51ab..9065073913a2 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera, > > { > > IPU3CameraData *data = cameraData(camera); > > std::map<Stream *, StreamConfiguration> configs; > > - V4L2SubdeviceFormat format = {}; > > - > > - /* > > - * FIXME: As of now, return the image format reported by the sensor. > > - * In future good defaults should be provided for each stream. > > - */ > > - if (data->sensor_->getFormat(0, &format)) { > > - LOG(IPU3, Error) << "Failed to create stream configurations"; > > - return configs; > > - } > > > > StreamConfiguration config = {}; > > - config.width = format.width; > > - config.height = format.height; > > - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > > + config.width = 2560; > > + config.height = 1920; > > + config.pixelFormat = V4L2_PIX_FMT_NV12; > > This is a good change and trust that the format you selected are the > correct one so I have not verified it by reading or testing on the IPU3. > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > I'm just curious why the pixelformat changes, did something change in > the upstream driver or is this in preparation of some later change? > We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different. Thanks j > > config.bufferCount = 4; > > > > configs[&data->stream_] = config; > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On Thu, Feb 21, 2019 at 05:21:16PM +0100, Jacopo Mondi wrote: > On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > > On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > >> Return default configuration for the output stream produced by the imgu > >> 'output' video node. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > >> 1 file changed, 3 insertions(+), 13 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 9694d0ce51ab..9065073913a2 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera, > >> { > >> IPU3CameraData *data = cameraData(camera); > >> std::map<Stream *, StreamConfiguration> configs; > >> - V4L2SubdeviceFormat format = {}; > >> - > >> - /* > >> - * FIXME: As of now, return the image format reported by the sensor. > >> - * In future good defaults should be provided for each stream. > >> - */ > >> - if (data->sensor_->getFormat(0, &format)) { > >> - LOG(IPU3, Error) << "Failed to create stream configurations"; > >> - return configs; > >> - } > >> > >> StreamConfiguration config = {}; > >> - config.width = format.width; > >> - config.height = format.height; > >> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > >> + config.width = 2560; > >> + config.height = 1920; > >> + config.pixelFormat = V4L2_PIX_FMT_NV12; > > > > This is a good change and trust that the format you selected are the > > correct one so I have not verified it by reading or testing on the IPU3. > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > I'm just curious why the pixelformat changes, did something change in > > the upstream driver or is this in preparation of some later change? > > We're preparing to provide frames to the application from the IMGU not > from the CIO2 unit. The output format of the two is different. But until we get that, this patch breaks capture with the IPU3, doesn't it ? I think it should be squashed with the patch that will add ImgU support, not get merged separately. > >> config.bufferCount = 4; > >> > >> configs[&data->stream_] = config;
Hi, May I know why hard coded the width and height? > > + config.width = 2560; > > + config.height = 1920; > > + config.pixelFormat = V4L2_PIX_FMT_NV12; Thanks, -Jerry -----Original Message----- From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi Sent: Friday, February 22, 2019 12:21 AM To: Niklas Söderlund <niklas.soderlund@ragnatech.se> Cc: libcamera-devel@lists.libcamera.org Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration Hi Niklas, On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your patch. > > On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > > Return default configuration for the output stream produced by the > > imgu 'output' video node. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > > 1 file changed, 3 insertions(+), 13 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > > b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 9694d0ce51ab..9065073913a2 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera > > *camera, { > > IPU3CameraData *data = cameraData(camera); > > std::map<Stream *, StreamConfiguration> configs; > > - V4L2SubdeviceFormat format = {}; > > - > > - /* > > - * FIXME: As of now, return the image format reported by the sensor. > > - * In future good defaults should be provided for each stream. > > - */ > > - if (data->sensor_->getFormat(0, &format)) { > > - LOG(IPU3, Error) << "Failed to create stream configurations"; > > - return configs; > > - } > > > > StreamConfiguration config = {}; > > - config.width = format.width; > > - config.height = format.height; > > - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > > + config.width = 2560; > > + config.height = 1920; > > + config.pixelFormat = V4L2_PIX_FMT_NV12; > > This is a good change and trust that the format you selected are the > correct one so I have not verified it by reading or testing on the IPU3. > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > I'm just curious why the pixelformat changes, did something change in > the upstream driver or is this in preparation of some later change? > We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different. Thanks j > > config.bufferCount = 4; > > > > configs[&data->stream_] = config; > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jerry,
thanks for your comment
On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote:
> Hi,
>
> May I know why hard coded the width and height?
> > > + config.width = 2560;
> > > + config.height = 1920;
> > > + config.pixelFormat = V4L2_PIX_FMT_NV12;
>
This happens in the 'streamConfiguration()' methods, which
applications use to obtain a known good default resolution and image
format the camera can provide. In this case I've selected 2560x1920
NV12 format as "default" for the single stream the IPU3 camera
provides at the moment.
Once application receives a known default format it is then free to
change any parameter of that, and provide it to libcamera using the
Camera::configureStreams() methods. The Camera instance will pass the
configurations to the pipeline handlers, that wil try to apply it to
the hardware.
To sum up: we need to provide to applications a default configuration
suitable for the hardware, and that's why we hardcode those values
here. Starting from that default configuration, applications can
configure streams with the desired width, height and pixel format and
ask libcamera to apply it to the hardware.
Does this clarify your concerns?
Thanks
j
> Thanks,
> -Jerry
>
> -----Original Message-----
> From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi
> Sent: Friday, February 22, 2019 12:21 AM
> To: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Cc: libcamera-devel@lists.libcamera.org
> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration
>
> Hi Niklas,
>
> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your patch.
> >
> > On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote:
> > > Return default configuration for the output stream produced by the
> > > imgu 'output' video node.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++-------------
> > > 1 file changed, 3 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 9694d0ce51ab..9065073913a2 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera
> > > *camera, {
> > > IPU3CameraData *data = cameraData(camera);
> > > std::map<Stream *, StreamConfiguration> configs;
> > > - V4L2SubdeviceFormat format = {};
> > > -
> > > - /*
> > > - * FIXME: As of now, return the image format reported by the sensor.
> > > - * In future good defaults should be provided for each stream.
> > > - */
> > > - if (data->sensor_->getFormat(0, &format)) {
> > > - LOG(IPU3, Error) << "Failed to create stream configurations";
> > > - return configs;
> > > - }
> > >
> > > StreamConfiguration config = {};
> > > - config.width = format.width;
> > > - config.height = format.height;
> > > - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> > > + config.width = 2560;
> > > + config.height = 1920;
> > > + config.pixelFormat = V4L2_PIX_FMT_NV12;
> >
> > This is a good change and trust that the format you selected are the
> > correct one so I have not verified it by reading or testing on the IPU3.
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >
> > I'm just curious why the pixelformat changes, did something change in
> > the upstream driver or is this in preparation of some later change?
> >
>
> We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different.
>
> Thanks
> j
>
> > > config.bufferCount = 4;
> > >
> > > configs[&data->stream_] = config;
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
Hi Jacopo, On Fri, Feb 22, 2019 at 11:33:05AM +0100, Jacopo Mondi wrote: > Hi Jerry, > thanks for your comment > > On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote: > > Hi, > > > > May I know why hard coded the width and height? > >>> + config.width = 2560; > >>> + config.height = 1920; > >>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > > > > > This happens in the 'streamConfiguration()' methods, which > applications use to obtain a known good default resolution and image > format the camera can provide. In this case I've selected 2560x1920 > NV12 format as "default" for the single stream the IPU3 camera > provides at the moment. > > Once application receives a known default format it is then free to > change any parameter of that, and provide it to libcamera using the > Camera::configureStreams() methods. The Camera instance will pass the > configurations to the pipeline handlers, that wil try to apply it to > the hardware. > > To sum up: we need to provide to applications a default configuration > suitable for the hardware, and that's why we hardcode those values > here. Starting from that default configuration, applications can > configure streams with the desired width, height and pixel format and > ask libcamera to apply it to the hardware. As mentioned in a previous e-mail I think you will have to make this dynamic. The above resolution may be fine on the hardware we use for testing the IPU3, but it needs to be sensor-dependent. > Does this clarify your concerns? > > > -----Original Message----- > > From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi > > Sent: Friday, February 22, 2019 12:21 AM > > To: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Cc: libcamera-devel@lists.libcamera.org > > Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration > > > > Hi Niklas, > > > > On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > >> Hi Jacopo, > >> > >> Thanks for your patch. > >> > >> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > >>> Return default configuration for the output stream produced by the > >>> imgu 'output' video node. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > >>> 1 file changed, 3 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> index 9694d0ce51ab..9065073913a2 100644 > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera > >>> *camera, { > >>> IPU3CameraData *data = cameraData(camera); > >>> std::map<Stream *, StreamConfiguration> configs; > >>> - V4L2SubdeviceFormat format = {}; > >>> - > >>> - /* > >>> - * FIXME: As of now, return the image format reported by the sensor. > >>> - * In future good defaults should be provided for each stream. > >>> - */ > >>> - if (data->sensor_->getFormat(0, &format)) { > >>> - LOG(IPU3, Error) << "Failed to create stream configurations"; > >>> - return configs; > >>> - } > >>> > >>> StreamConfiguration config = {}; > >>> - config.width = format.width; > >>> - config.height = format.height; > >>> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > >>> + config.width = 2560; > >>> + config.height = 1920; > >>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > >> > >> This is a good change and trust that the format you selected are the > >> correct one so I have not verified it by reading or testing on the IPU3. > >> > >> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> > >> I'm just curious why the pixelformat changes, did something change in > >> the upstream driver or is this in preparation of some later change? > >> > > > > We're preparing to provide frames to the application from the IMGU > > not from the CIO2 unit. The output format of the two is different. > > > >>> config.bufferCount = 4; > >>> > >>> configs[&data->stream_] = config;
Hi Jacopo, On 22/02/2019 10:33, Jacopo Mondi wrote: > Hi Jerry, > thanks for your comment > > On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote: >> Hi, >> >> May I know why hard coded the width and height? >>>> + config.width = 2560; >>>> + config.height = 1920; >>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; >> > > > This happens in the 'streamConfiguration()' methods, which > applications use to obtain a known good default resolution and image > format the camera can provide. In this case I've selected 2560x1920 > NV12 format as "default" for the single stream the IPU3 camera > provides at the moment. > > Once application receives a known default format it is then free to > change any parameter of that, and provide it to libcamera using the > Camera::configureStreams() methods. The Camera instance will pass the > configurations to the pipeline handlers, that wil try to apply it to > the hardware. > > To sum up: we need to provide to applications a default configuration > suitable for the hardware, and that's why we hardcode those values > here. Starting from that default configuration, applications can > configure streams with the desired width, height and pixel format and > ask libcamera to apply it to the hardware. > > Does this clarify your concerns? I think it was discussed somewhere but I'm not sure where, but what is the reason why we can't ask the hardware for it's "current" format to set the 'default' ? I feel like anything we define statically in the pipeline is just as wrong as any initial default provided by the hardware .. -- Kieran > Thanks > j > >> Thanks, >> -Jerry >> >> -----Original Message----- >> From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi >> Sent: Friday, February 22, 2019 12:21 AM >> To: Niklas Söderlund <niklas.soderlund@ragnatech.se> >> Cc: libcamera-devel@lists.libcamera.org >> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration >> >> Hi Niklas, >> >> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: >>> Hi Jacopo, >>> >>> Thanks for your patch. >>> >>> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: >>>> Return default configuration for the output stream produced by the >>>> imgu 'output' video node. >>>> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>> --- >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- >>>> 1 file changed, 3 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> index 9694d0ce51ab..9065073913a2 100644 >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera >>>> *camera, { >>>> IPU3CameraData *data = cameraData(camera); >>>> std::map<Stream *, StreamConfiguration> configs; >>>> - V4L2SubdeviceFormat format = {}; >>>> - >>>> - /* >>>> - * FIXME: As of now, return the image format reported by the sensor. >>>> - * In future good defaults should be provided for each stream. >>>> - */ >>>> - if (data->sensor_->getFormat(0, &format)) { >>>> - LOG(IPU3, Error) << "Failed to create stream configurations"; >>>> - return configs; >>>> - } >>>> >>>> StreamConfiguration config = {}; >>>> - config.width = format.width; >>>> - config.height = format.height; >>>> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; >>>> + config.width = 2560; >>>> + config.height = 1920; >>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; >>> >>> This is a good change and trust that the format you selected are the >>> correct one so I have not verified it by reading or testing on the IPU3. >>> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>> >>> I'm just curious why the pixelformat changes, did something change in >>> the upstream driver or is this in preparation of some later change? >>> >> >> We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different. >> >> Thanks >> j >> >>>> config.bufferCount = 4; >>>> >>>> configs[&data->stream_] = config; >>>> -- >>>> 2.20.1 >>>> >>>> _______________________________________________ >>>> libcamera-devel mailing list >>>> libcamera-devel@lists.libcamera.org >>>> https://lists.libcamera.org/listinfo/libcamera-devel >>> >>> -- >>> Regards, >>> Niklas Söderlund >>> >>> _______________________________________________ >>> libcamera-devel mailing list >>> libcamera-devel@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Fri, Feb 22, 2019 at 10:57:35AM +0000, Kieran Bingham wrote: > On 22/02/2019 10:33, Jacopo Mondi wrote: > > On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote: > >> Hi, > >> > >> May I know why hard coded the width and height? > >>>> + config.width = 2560; > >>>> + config.height = 1920; > >>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > > > > This happens in the 'streamConfiguration()' methods, which > > applications use to obtain a known good default resolution and image > > format the camera can provide. In this case I've selected 2560x1920 > > NV12 format as "default" for the single stream the IPU3 camera > > provides at the moment. > > > > Once application receives a known default format it is then free to > > change any parameter of that, and provide it to libcamera using the > > Camera::configureStreams() methods. The Camera instance will pass the > > configurations to the pipeline handlers, that wil try to apply it to > > the hardware. > > > > To sum up: we need to provide to applications a default configuration > > suitable for the hardware, and that's why we hardcode those values > > here. Starting from that default configuration, applications can > > configure streams with the desired width, height and pixel format and > > ask libcamera to apply it to the hardware. > > > > Does this clarify your concerns? > > I think it was discussed somewhere but I'm not sure where, but what is > the reason why we can't ask the hardware for it's "current" format to > set the 'default' ? The current format isn't a good idea, as it can vary based on the history of device usage, and is thus not suitable as a default. I however agree that we should compute a reasonable default based on the hardware capabilities. > I feel like anything we define statically in the pipeline is just as > wrong as any initial default provided by the hardware .. > > >> -----Original Message----- > >> From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi > >> Sent: Friday, February 22, 2019 12:21 AM > >> To: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> Cc: libcamera-devel@lists.libcamera.org > >> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration > >> > >> Hi Niklas, > >> > >> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > >>> Hi Jacopo, > >>> > >>> Thanks for your patch. > >>> > >>> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > >>>> Return default configuration for the output stream produced by the > >>>> imgu 'output' video node. > >>>> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>> --- > >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > >>>> 1 file changed, 3 insertions(+), 13 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> index 9694d0ce51ab..9065073913a2 100644 > >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera > >>>> *camera, { > >>>> IPU3CameraData *data = cameraData(camera); > >>>> std::map<Stream *, StreamConfiguration> configs; > >>>> - V4L2SubdeviceFormat format = {}; > >>>> - > >>>> - /* > >>>> - * FIXME: As of now, return the image format reported by the sensor. > >>>> - * In future good defaults should be provided for each stream. > >>>> - */ > >>>> - if (data->sensor_->getFormat(0, &format)) { > >>>> - LOG(IPU3, Error) << "Failed to create stream configurations"; > >>>> - return configs; > >>>> - } > >>>> > >>>> StreamConfiguration config = {}; > >>>> - config.width = format.width; > >>>> - config.height = format.height; > >>>> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > >>>> + config.width = 2560; > >>>> + config.height = 1920; > >>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > >>> > >>> This is a good change and trust that the format you selected are the > >>> correct one so I have not verified it by reading or testing on the IPU3. > >>> > >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>> > >>> I'm just curious why the pixelformat changes, did something change in > >>> the upstream driver or is this in preparation of some later change? > >>> > >> > >> We're preparing to provide frames to the application from the IMGU > >> not from the CIO2 unit. The output format of the two is different. > >> > >>>> config.bufferCount = 4; > >>>> > >>>> configs[&data->stream_] = config;
Hi Kieran, Laurent, On Fri, Feb 22, 2019 at 10:57:35AM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 22/02/2019 10:33, Jacopo Mondi wrote: > > Hi Jerry, > > thanks for your comment > > > > On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote: > >> Hi, > >> > >> May I know why hard coded the width and height? > >>>> + config.width = 2560; > >>>> + config.height = 1920; > >>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > >> > > > > > > This happens in the 'streamConfiguration()' methods, which > > applications use to obtain a known good default resolution and image > > format the camera can provide. In this case I've selected 2560x1920 > > NV12 format as "default" for the single stream the IPU3 camera > > provides at the moment. > > > > Once application receives a known default format it is then free to > > change any parameter of that, and provide it to libcamera using the > > Camera::configureStreams() methods. The Camera instance will pass the > > configurations to the pipeline handlers, that wil try to apply it to > > the hardware. > > > > To sum up: we need to provide to applications a default configuration > > suitable for the hardware, and that's why we hardcode those values > > here. Starting from that default configuration, applications can > > configure streams with the desired width, height and pixel format and > > ask libcamera to apply it to the hardware. > > > > Does this clarify your concerns? > > I think it was discussed somewhere but I'm not sure where, but what is > the reason why we can't ask the hardware for it's "current" format to > set the 'default' ? > Because at the current stage the 'hardware' is both the sensor provided resolution and the output resolution provided by the ImgU, which do not match, if we use their defaults. And as images will be provided from ImgU not the CIO2 as before, I cannot just claim the format applied on the sensor and report that. That resolution is known to work for the current use case, and I put it there not to break capture. Later on, with full ImgU support should westart from the sensor resolution and adjust it to one the ImgU can provide, or start from the default one applied to the ImgU unit, and make sure the sensor can provide something big enough to fit into that? Also, I know realize this was probably what Jerry was asking in is email, not an explanation of the stream format negotiation procedure... > I feel like anything we define statically in the pipeline is just as > wrong as any initial default provided by the hardware .. > Why do you consider initial defaults provided by the hardware wrong? I mean, the issue here to me is "which default", but somewhere it has to be defined, right? Thanks j > -- > Kieran > > > > > > Thanks > > j > > > >> Thanks, > >> -Jerry > >> > >> -----Original Message----- > >> From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi > >> Sent: Friday, February 22, 2019 12:21 AM > >> To: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> Cc: libcamera-devel@lists.libcamera.org > >> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration > >> > >> Hi Niklas, > >> > >> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > >>> Hi Jacopo, > >>> > >>> Thanks for your patch. > >>> > >>> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > >>>> Return default configuration for the output stream produced by the > >>>> imgu 'output' video node. > >>>> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>> --- > >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > >>>> 1 file changed, 3 insertions(+), 13 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> index 9694d0ce51ab..9065073913a2 100644 > >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera > >>>> *camera, { > >>>> IPU3CameraData *data = cameraData(camera); > >>>> std::map<Stream *, StreamConfiguration> configs; > >>>> - V4L2SubdeviceFormat format = {}; > >>>> - > >>>> - /* > >>>> - * FIXME: As of now, return the image format reported by the sensor. > >>>> - * In future good defaults should be provided for each stream. > >>>> - */ > >>>> - if (data->sensor_->getFormat(0, &format)) { > >>>> - LOG(IPU3, Error) << "Failed to create stream configurations"; > >>>> - return configs; > >>>> - } > >>>> > >>>> StreamConfiguration config = {}; > >>>> - config.width = format.width; > >>>> - config.height = format.height; > >>>> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > >>>> + config.width = 2560; > >>>> + config.height = 1920; > >>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > >>> > >>> This is a good change and trust that the format you selected are the > >>> correct one so I have not verified it by reading or testing on the IPU3. > >>> > >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>> > >>> I'm just curious why the pixelformat changes, did something change in > >>> the upstream driver or is this in preparation of some later change? > >>> > >> > >> We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different. > >> > >> Thanks > >> j > >> > >>>> config.bufferCount = 4; > >>>> > >>>> configs[&data->stream_] = config; > >>>> -- > >>>> 2.20.1 > >>>> > >>>> _______________________________________________ > >>>> libcamera-devel mailing list > >>>> libcamera-devel@lists.libcamera.org > >>>> https://lists.libcamera.org/listinfo/libcamera-devel > >>> > >>> -- > >>> Regards, > >>> Niklas Söderlund > >>> > >>> _______________________________________________ > >>> libcamera-devel mailing list > >>> libcamera-devel@lists.libcamera.org > >>> https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran
Hi Jacopo, On Fri, Feb 22, 2019 at 12:08:42PM +0100, Jacopo Mondi wrote: > On Fri, Feb 22, 2019 at 10:57:35AM +0000, Kieran Bingham wrote: > > On 22/02/2019 10:33, Jacopo Mondi wrote: > >> On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote: > >>> Hi, > >>> > >>> May I know why hard coded the width and height? > >>>>> + config.width = 2560; > >>>>> + config.height = 1920; > >>>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > >> > >> This happens in the 'streamConfiguration()' methods, which > >> applications use to obtain a known good default resolution and image > >> format the camera can provide. In this case I've selected 2560x1920 > >> NV12 format as "default" for the single stream the IPU3 camera > >> provides at the moment. > >> > >> Once application receives a known default format it is then free to > >> change any parameter of that, and provide it to libcamera using the > >> Camera::configureStreams() methods. The Camera instance will pass the > >> configurations to the pipeline handlers, that wil try to apply it to > >> the hardware. > >> > >> To sum up: we need to provide to applications a default configuration > >> suitable for the hardware, and that's why we hardcode those values > >> here. Starting from that default configuration, applications can > >> configure streams with the desired width, height and pixel format and > >> ask libcamera to apply it to the hardware. > >> > >> Does this clarify your concerns? > > > > I think it was discussed somewhere but I'm not sure where, but what is > > the reason why we can't ask the hardware for it's "current" format to > > set the 'default' ? > > Because at the current stage the 'hardware' is both the sensor > provided resolution and the output resolution provided by the ImgU, > which do not match, if we use their defaults. And as images will be > provided from ImgU not the CIO2 as before, I cannot just claim the > format applied on the sensor and report that. The ImgU is a memory to memory device, it can operate on pretty much any input resolution, includes a scaler, and can crop. The default resolution should come from the sensor, not the ImgU. > That resolution is known to work for the current use case, and I put > it there not to break capture. Later on, with full ImgU support should > westart from the sensor resolution and adjust it to one the ImgU can > provide, or start from the default one applied to the ImgU unit, and > make sure the sensor can provide something big enough to fit into > that? > > Also, I know realize this was probably what Jerry was asking in is > email, not an explanation of the stream format negotiation > procedure... > > > I feel like anything we define statically in the pipeline is just as > > wrong as any initial default provided by the hardware .. > > Why do you consider initial defaults provided by the hardware wrong? I > mean, the issue here to me is "which default", but somewhere it has to > be defined, right? > > >>> -----Original Message----- > >>> From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi > >>> Sent: Friday, February 22, 2019 12:21 AM > >>> To: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>> Cc: libcamera-devel@lists.libcamera.org > >>> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration > >>> > >>> Hi Niklas, > >>> > >>> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > >>>> Hi Jacopo, > >>>> > >>>> Thanks for your patch. > >>>> > >>>> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > >>>>> Return default configuration for the output stream produced by the > >>>>> imgu 'output' video node. > >>>>> > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>> --- > >>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > >>>>> 1 file changed, 3 insertions(+), 13 deletions(-) > >>>>> > >>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>>> index 9694d0ce51ab..9065073913a2 100644 > >>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera > >>>>> *camera, { > >>>>> IPU3CameraData *data = cameraData(camera); > >>>>> std::map<Stream *, StreamConfiguration> configs; > >>>>> - V4L2SubdeviceFormat format = {}; > >>>>> - > >>>>> - /* > >>>>> - * FIXME: As of now, return the image format reported by the sensor. > >>>>> - * In future good defaults should be provided for each stream. > >>>>> - */ > >>>>> - if (data->sensor_->getFormat(0, &format)) { > >>>>> - LOG(IPU3, Error) << "Failed to create stream configurations"; > >>>>> - return configs; > >>>>> - } > >>>>> > >>>>> StreamConfiguration config = {}; > >>>>> - config.width = format.width; > >>>>> - config.height = format.height; > >>>>> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > >>>>> + config.width = 2560; > >>>>> + config.height = 1920; > >>>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > >>>> > >>>> This is a good change and trust that the format you selected are the > >>>> correct one so I have not verified it by reading or testing on the IPU3. > >>>> > >>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>>> > >>>> I'm just curious why the pixelformat changes, did something change in > >>>> the upstream driver or is this in preparation of some later change? > >>>> > >>> > >>> We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different. > >>> > >>>>> config.bufferCount = 4; > >>>>> > >>>>> configs[&data->stream_] = config;
Hi Laurent, On Fri, Feb 22, 2019 at 01:44:14PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Feb 22, 2019 at 12:08:42PM +0100, Jacopo Mondi wrote: > > On Fri, Feb 22, 2019 at 10:57:35AM +0000, Kieran Bingham wrote: > > > On 22/02/2019 10:33, Jacopo Mondi wrote: > > >> On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote: > > >>> Hi, > > >>> > > >>> May I know why hard coded the width and height? > > >>>>> + config.width = 2560; > > >>>>> + config.height = 1920; > > >>>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > > >> > > >> This happens in the 'streamConfiguration()' methods, which > > >> applications use to obtain a known good default resolution and image > > >> format the camera can provide. In this case I've selected 2560x1920 > > >> NV12 format as "default" for the single stream the IPU3 camera > > >> provides at the moment. > > >> > > >> Once application receives a known default format it is then free to > > >> change any parameter of that, and provide it to libcamera using the > > >> Camera::configureStreams() methods. The Camera instance will pass the > > >> configurations to the pipeline handlers, that wil try to apply it to > > >> the hardware. > > >> > > >> To sum up: we need to provide to applications a default configuration > > >> suitable for the hardware, and that's why we hardcode those values > > >> here. Starting from that default configuration, applications can > > >> configure streams with the desired width, height and pixel format and > > >> ask libcamera to apply it to the hardware. > > >> > > >> Does this clarify your concerns? > > > > > > I think it was discussed somewhere but I'm not sure where, but what is > > > the reason why we can't ask the hardware for it's "current" format to > > > set the 'default' ? > > > > Because at the current stage the 'hardware' is both the sensor > > provided resolution and the output resolution provided by the ImgU, > > which do not match, if we use their defaults. And as images will be > > provided from ImgU not the CIO2 as before, I cannot just claim the > > format applied on the sensor and report that. > > The ImgU is a memory to memory device, it can operate on pretty much any > input resolution, includes a scaler, and can crop. The default > resolution should come from the sensor, not the ImgU. > True indeed, but if I'm not mistaken, the ImgU has a default programmed to be 1920x1080, not all sensors can provide that resolution (yes, I understand, it's a corner case, most modern sensors can, but I feel this is worth checking, isn't it?) Thanks j > > That resolution is known to work for the current use case, and I put > > it there not to break capture. Later on, with full ImgU support should > > westart from the sensor resolution and adjust it to one the ImgU can > > provide, or start from the default one applied to the ImgU unit, and > > make sure the sensor can provide something big enough to fit into > > that? > > > > Also, I know realize this was probably what Jerry was asking in is > > email, not an explanation of the stream format negotiation > > procedure... > > > > > I feel like anything we define statically in the pipeline is just as > > > wrong as any initial default provided by the hardware .. > > > > Why do you consider initial defaults provided by the hardware wrong? I > > mean, the issue here to me is "which default", but somewhere it has to > > be defined, right? > > > > >>> -----Original Message----- > > >>> From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi > > >>> Sent: Friday, February 22, 2019 12:21 AM > > >>> To: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > >>> Cc: libcamera-devel@lists.libcamera.org > > >>> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration > > >>> > > >>> Hi Niklas, > > >>> > > >>> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > > >>>> Hi Jacopo, > > >>>> > > >>>> Thanks for your patch. > > >>>> > > >>>> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > > >>>>> Return default configuration for the output stream produced by the > > >>>>> imgu 'output' video node. > > >>>>> > > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >>>>> --- > > >>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > > >>>>> 1 file changed, 3 insertions(+), 13 deletions(-) > > >>>>> > > >>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > > >>>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >>>>> index 9694d0ce51ab..9065073913a2 100644 > > >>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > >>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >>>>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera > > >>>>> *camera, { > > >>>>> IPU3CameraData *data = cameraData(camera); > > >>>>> std::map<Stream *, StreamConfiguration> configs; > > >>>>> - V4L2SubdeviceFormat format = {}; > > >>>>> - > > >>>>> - /* > > >>>>> - * FIXME: As of now, return the image format reported by the sensor. > > >>>>> - * In future good defaults should be provided for each stream. > > >>>>> - */ > > >>>>> - if (data->sensor_->getFormat(0, &format)) { > > >>>>> - LOG(IPU3, Error) << "Failed to create stream configurations"; > > >>>>> - return configs; > > >>>>> - } > > >>>>> > > >>>>> StreamConfiguration config = {}; > > >>>>> - config.width = format.width; > > >>>>> - config.height = format.height; > > >>>>> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > > >>>>> + config.width = 2560; > > >>>>> + config.height = 1920; > > >>>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > > >>>> > > >>>> This is a good change and trust that the format you selected are the > > >>>> correct one so I have not verified it by reading or testing on the IPU3. > > >>>> > > >>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > >>>> > > >>>> I'm just curious why the pixelformat changes, did something change in > > >>>> the upstream driver or is this in preparation of some later change? > > >>>> > > >>> > > >>> We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different. > > >>> > > >>>>> config.bufferCount = 4; > > >>>>> > > >>>>> configs[&data->stream_] = config; > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Jacopo, On 22/02/2019 11:44, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Feb 22, 2019 at 12:08:42PM +0100, Jacopo Mondi wrote: >> On Fri, Feb 22, 2019 at 10:57:35AM +0000, Kieran Bingham wrote: >>> On 22/02/2019 10:33, Jacopo Mondi wrote: >>>> On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote: >>>>> Hi, >>>>> >>>>> May I know why hard coded the width and height? >>>>>>> + config.width = 2560; >>>>>>> + config.height = 1920; >>>>>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; >>>> >>>> This happens in the 'streamConfiguration()' methods, which >>>> applications use to obtain a known good default resolution and image >>>> format the camera can provide. In this case I've selected 2560x1920 >>>> NV12 format as "default" for the single stream the IPU3 camera >>>> provides at the moment. >>>> >>>> Once application receives a known default format it is then free to >>>> change any parameter of that, and provide it to libcamera using the >>>> Camera::configureStreams() methods. The Camera instance will pass the >>>> configurations to the pipeline handlers, that wil try to apply it to >>>> the hardware. >>>> >>>> To sum up: we need to provide to applications a default configuration >>>> suitable for the hardware, and that's why we hardcode those values >>>> here. Starting from that default configuration, applications can >>>> configure streams with the desired width, height and pixel format and >>>> ask libcamera to apply it to the hardware. >>>> >>>> Does this clarify your concerns? >>> >>> I think it was discussed somewhere but I'm not sure where, but what is >>> the reason why we can't ask the hardware for it's "current" format to >>> set the 'default' ? >> >> Because at the current stage the 'hardware' is both the sensor >> provided resolution and the output resolution provided by the ImgU, >> which do not match, if we use their defaults. And as images will be >> provided from ImgU not the CIO2 as before, I cannot just claim the >> format applied on the sensor and report that. > > The ImgU is a memory to memory device, it can operate on pretty much any > input resolution, includes a scaler, and can crop. The default > resolution should come from the sensor, not the ImgU. How about to provide a default, we obtain the resolution from the Sensor, (I assume this is constant-ish?) and the rest from the ImgU ? So we would return a default of a non-scaled capture but with a pixel format supported by the ImgU output? >> That resolution is known to work for the current use case, and I put >> it there not to break capture. Later on, with full ImgU support should >> westart from the sensor resolution and adjust it to one the ImgU can >> provide, or start from the default one applied to the ImgU unit, and >> make sure the sensor can provide something big enough to fit into >> that? >> >> Also, I know realize this was probably what Jerry was asking in is >> email, not an explanation of the stream format negotiation >> procedure... >> >>> I feel like anything we define statically in the pipeline is just as >>> wrong as any initial default provided by the hardware .. >> >> Why do you consider initial defaults provided by the hardware wrong? I >> mean, the issue here to me is "which default", but somewhere it has to >> be defined, right? Yes, I think I was essentially saying the same thing as Laurent in another e-mail ... ( > The current format isn't a good idea, as it can vary based on the > history of device usage, and is thus not suitable as a default. I > however agree that we should compute a reasonable default based on the > hardware capabilities. ) >> >>>>> -----Original Message----- >>>>> From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi >>>>> Sent: Friday, February 22, 2019 12:21 AM >>>>> To: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>>>> Cc: libcamera-devel@lists.libcamera.org >>>>> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration >>>>> >>>>> Hi Niklas, >>>>> >>>>> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: >>>>>> Hi Jacopo, >>>>>> >>>>>> Thanks for your patch. >>>>>> >>>>>> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: >>>>>>> Return default configuration for the output stream produced by the >>>>>>> imgu 'output' video node. >>>>>>> >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>>>>> --- >>>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- >>>>>>> 1 file changed, 3 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>>>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>>>>> index 9694d0ce51ab..9065073913a2 100644 >>>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>>>>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera >>>>>>> *camera, { >>>>>>> IPU3CameraData *data = cameraData(camera); >>>>>>> std::map<Stream *, StreamConfiguration> configs; >>>>>>> - V4L2SubdeviceFormat format = {}; >>>>>>> - >>>>>>> - /* >>>>>>> - * FIXME: As of now, return the image format reported by the sensor. >>>>>>> - * In future good defaults should be provided for each stream. >>>>>>> - */ >>>>>>> - if (data->sensor_->getFormat(0, &format)) { >>>>>>> - LOG(IPU3, Error) << "Failed to create stream configurations"; >>>>>>> - return configs; >>>>>>> - } >>>>>>> >>>>>>> StreamConfiguration config = {}; >>>>>>> - config.width = format.width; >>>>>>> - config.height = format.height; >>>>>>> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; >>>>>>> + config.width = 2560; >>>>>>> + config.height = 1920; >>>>>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; >>>>>> >>>>>> This is a good change and trust that the format you selected are the >>>>>> correct one so I have not verified it by reading or testing on the IPU3. >>>>>> >>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>>>>> >>>>>> I'm just curious why the pixelformat changes, did something change in >>>>>> the upstream driver or is this in preparation of some later change? >>>>>> >>>>> >>>>> We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different. >>>>> >>>>>>> config.bufferCount = 4; >>>>>>> >>>>>>> configs[&data->stream_] = config; >
Hi Jacopo, On Fri, Feb 22, 2019 at 01:05:15PM +0100, Jacopo Mondi wrote: > On Fri, Feb 22, 2019 at 01:44:14PM +0200, Laurent Pinchart wrote: > > On Fri, Feb 22, 2019 at 12:08:42PM +0100, Jacopo Mondi wrote: > >> On Fri, Feb 22, 2019 at 10:57:35AM +0000, Kieran Bingham wrote: > >>> On 22/02/2019 10:33, Jacopo Mondi wrote: > >>>> On Fri, Feb 22, 2019 at 02:30:09AM +0000, Hu, Jerry W wrote: > >>>>> Hi, > >>>>> > >>>>> May I know why hard coded the width and height? > >>>>>>> + config.width = 2560; > >>>>>>> + config.height = 1920; > >>>>>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > >>>> > >>>> This happens in the 'streamConfiguration()' methods, which > >>>> applications use to obtain a known good default resolution and image > >>>> format the camera can provide. In this case I've selected 2560x1920 > >>>> NV12 format as "default" for the single stream the IPU3 camera > >>>> provides at the moment. > >>>> > >>>> Once application receives a known default format it is then free to > >>>> change any parameter of that, and provide it to libcamera using the > >>>> Camera::configureStreams() methods. The Camera instance will pass the > >>>> configurations to the pipeline handlers, that wil try to apply it to > >>>> the hardware. > >>>> > >>>> To sum up: we need to provide to applications a default configuration > >>>> suitable for the hardware, and that's why we hardcode those values > >>>> here. Starting from that default configuration, applications can > >>>> configure streams with the desired width, height and pixel format and > >>>> ask libcamera to apply it to the hardware. > >>>> > >>>> Does this clarify your concerns? > >>> > >>> I think it was discussed somewhere but I'm not sure where, but what is > >>> the reason why we can't ask the hardware for it's "current" format to > >>> set the 'default' ? > >> > >> Because at the current stage the 'hardware' is both the sensor > >> provided resolution and the output resolution provided by the ImgU, > >> which do not match, if we use their defaults. And as images will be > >> provided from ImgU not the CIO2 as before, I cannot just claim the > >> format applied on the sensor and report that. > > > > The ImgU is a memory to memory device, it can operate on pretty much any > > input resolution, includes a scaler, and can crop. The default > > resolution should come from the sensor, not the ImgU. > > True indeed, but if I'm not mistaken, the ImgU has a default > programmed to be 1920x1080, not all sensors can provide that > resolution (yes, I understand, it's a corner > case, most modern sensors can, but I feel this is worth checking, > isn't it?) But the default ImgU settings don't matter at all :-) From an application point of view the ImgU isn't visible, we have one or more cameras, with one or more streams each, and we need to report a default configuration for those streams. That's only dependent on the sensor, not the ImgU (or, to be exact, the only dependencies on the ImgU are min/max resolution and min/max scaling factor). > >> That resolution is known to work for the current use case, and I put > >> it there not to break capture. Later on, with full ImgU support should > >> westart from the sensor resolution and adjust it to one the ImgU can > >> provide, or start from the default one applied to the ImgU unit, and > >> make sure the sensor can provide something big enough to fit into > >> that? > >> > >> Also, I know realize this was probably what Jerry was asking in is > >> email, not an explanation of the stream format negotiation > >> procedure... > >> > >>> I feel like anything we define statically in the pipeline is just as > >>> wrong as any initial default provided by the hardware .. > >> > >> Why do you consider initial defaults provided by the hardware wrong? I > >> mean, the issue here to me is "which default", but somewhere it has to > >> be defined, right? > >> > >>>>> -----Original Message----- > >>>>> From: libcamera-devel [mailto:libcamera-devel-bounces@lists.libcamera.org] On Behalf Of Jacopo Mondi > >>>>> Sent: Friday, February 22, 2019 12:21 AM > >>>>> To: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>>>> Cc: libcamera-devel@lists.libcamera.org > >>>>> Subject: Re: [libcamera-devel] [PATCH 2/5] libcamera: ipu3: Return video output default configuration > >>>>> > >>>>> Hi Niklas, > >>>>> > >>>>> On Thu, Feb 21, 2019 at 04:58:35PM +0100, Niklas Söderlund wrote: > >>>>>> Hi Jacopo, > >>>>>> > >>>>>> Thanks for your patch. > >>>>>> > >>>>>> On 2019-02-20 14:17:54 +0100, Jacopo Mondi wrote: > >>>>>>> Return default configuration for the output stream produced by the > >>>>>>> imgu 'output' video node. > >>>>>>> > >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>>>> --- > >>>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- > >>>>>>> 1 file changed, 3 insertions(+), 13 deletions(-) > >>>>>>> > >>>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>>>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>>>>> index 9694d0ce51ab..9065073913a2 100644 > >>>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>>>>> @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera > >>>>>>> *camera, { > >>>>>>> IPU3CameraData *data = cameraData(camera); > >>>>>>> std::map<Stream *, StreamConfiguration> configs; > >>>>>>> - V4L2SubdeviceFormat format = {}; > >>>>>>> - > >>>>>>> - /* > >>>>>>> - * FIXME: As of now, return the image format reported by the sensor. > >>>>>>> - * In future good defaults should be provided for each stream. > >>>>>>> - */ > >>>>>>> - if (data->sensor_->getFormat(0, &format)) { > >>>>>>> - LOG(IPU3, Error) << "Failed to create stream configurations"; > >>>>>>> - return configs; > >>>>>>> - } > >>>>>>> > >>>>>>> StreamConfiguration config = {}; > >>>>>>> - config.width = format.width; > >>>>>>> - config.height = format.height; > >>>>>>> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > >>>>>>> + config.width = 2560; > >>>>>>> + config.height = 1920; > >>>>>>> + config.pixelFormat = V4L2_PIX_FMT_NV12; > >>>>>> > >>>>>> This is a good change and trust that the format you selected are the > >>>>>> correct one so I have not verified it by reading or testing on the IPU3. > >>>>>> > >>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>>>>> > >>>>>> I'm just curious why the pixelformat changes, did something change in > >>>>>> the upstream driver or is this in preparation of some later change? > >>>>>> > >>>>> > >>>>> We're preparing to provide frames to the application from the IMGU not from the CIO2 unit. The output format of the two is different. > >>>>> > >>>>>>> config.bufferCount = 4; > >>>>>>> > >>>>>>> configs[&data->stream_] = config;
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 9694d0ce51ab..9065073913a2 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -99,21 +99,11 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera, { IPU3CameraData *data = cameraData(camera); std::map<Stream *, StreamConfiguration> configs; - V4L2SubdeviceFormat format = {}; - - /* - * FIXME: As of now, return the image format reported by the sensor. - * In future good defaults should be provided for each stream. - */ - if (data->sensor_->getFormat(0, &format)) { - LOG(IPU3, Error) << "Failed to create stream configurations"; - return configs; - } StreamConfiguration config = {}; - config.width = format.width; - config.height = format.height; - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; + config.width = 2560; + config.height = 1920; + config.pixelFormat = V4L2_PIX_FMT_NV12; config.bufferCount = 4; configs[&data->stream_] = config;
Return default configuration for the output stream produced by the imgu 'output' video node. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)