| Message ID | 20200924145143.117733-4-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Sep 24, 2020 at 04:51:43PM +0200, Jacopo Mondi wrote: > Be more precise in commenting why the ImgU shall not be configured > if only the RAW stream is requested. > > As an example, if the ImgU gets unecessary configured: > cam -srole=viewfinder -c2 -C -> WORKS > cam -srole=stillraw -c2 -C -> WORKS > cam -srole=viewfinder -c2 -C -> Failed to queue buffer 0: Invalid argument > > Since commit 1bd2e981a209 ("libcamera: ipu3: Fix RAW+YUV capture") > the ImgU configuration procedure also correctly implements the > assumption that at least one of the YUV output is being operated. If > that's not the case, as in the RAW-only capture use case, the code > tries to access a non-existing configuration. One more reason to > exit early if no YUV stream is requested. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 477f85a45755..2f90ad2e2780 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -459,9 +459,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > > /* > - * If the ImgU gets configured with proper IF, BDS and GDC sizes, it > - * is then expected that frames are dequeued from its main output > - * otherwise the system stalls. > + * If the ImgU gets configured it is then expected that buffers are > + * queued to its outputs, otherwise the next capture session > + * that uses the ImgU fails at queueing buffers at its input. "it is then expected" sounds like we expect this, while I believe you mean the driver expects (or seems to expect) this. I'd phrase the comment as * If the ImgU gets configured, its driver seems to expect that * buffers will be queued to its outputs, as otherwise the next * capture session that uses the ImgU fails when queueing * buffers to its input. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > * > * If no ImgU configuration has been computed, it means only a RAW > * stream has been requested: return here to skip the ImgU configuration
Hi Jacopo, Thanks for your work. On 2020-09-24 16:51:43 +0200, Jacopo Mondi wrote: > Be more precise in commenting why the ImgU shall not be configured > if only the RAW stream is requested. > > As an example, if the ImgU gets unecessary configured: > cam -srole=viewfinder -c2 -C -> WORKS > cam -srole=stillraw -c2 -C -> WORKS > cam -srole=viewfinder -c2 -C -> Failed to queue buffer 0: Invalid argument > > Since commit 1bd2e981a209 ("libcamera: ipu3: Fix RAW+YUV capture") Unless you are _very_ lucky the commit hash will not be the same when this patch is merged :-) > the ImgU configuration procedure also correctly implements the > assumption that at least one of the YUV output is being operated. If > that's not the case, as in the RAW-only capture use case, the code > tries to access a non-existing configuration. One more reason to > exit early if no YUV stream is requested. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> With Laurent's comment addressed and the commit hash removed (or proof of guaranteed hash collision ;-P) Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 477f85a45755..2f90ad2e2780 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -459,9 +459,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > > /* > - * If the ImgU gets configured with proper IF, BDS and GDC sizes, it > - * is then expected that frames are dequeued from its main output > - * otherwise the system stalls. > + * If the ImgU gets configured it is then expected that buffers are > + * queued to its outputs, otherwise the next capture session > + * that uses the ImgU fails at queueing buffers at its input. > * > * If no ImgU configuration has been computed, it means only a RAW > * stream has been requested: return here to skip the ImgU configuration > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Laurent, On Tue, Sep 29, 2020 at 07:39:28AM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2020-09-24 16:51:43 +0200, Jacopo Mondi wrote: > > Be more precise in commenting why the ImgU shall not be configured > > if only the RAW stream is requested. > > > > As an example, if the ImgU gets unecessary configured: > > cam -srole=viewfinder -c2 -C -> WORKS > > cam -srole=stillraw -c2 -C -> WORKS > > cam -srole=viewfinder -c2 -C -> Failed to queue buffer 0: Invalid argument > > > > Since commit 1bd2e981a209 ("libcamera: ipu3: Fix RAW+YUV capture") > > Unless you are _very_ lucky the commit hash will not be the same when > this patch is merged :-) > You're very right! > > the ImgU configuration procedure also correctly implements the > > assumption that at least one of the YUV output is being operated. If > > that's not the case, as in the RAW-only capture use case, the code > > tries to access a non-existing configuration. One more reason to > > exit early if no YUV stream is requested. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > With Laurent's comment addressed and the commit hash removed (or proof > of guaranteed hash collision ;-P) > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Thanks, with this fixed I'll now push the series Thanks j > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 477f85a45755..2f90ad2e2780 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -459,9 +459,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > return ret; > > > > /* > > - * If the ImgU gets configured with proper IF, BDS and GDC sizes, it > > - * is then expected that frames are dequeued from its main output > > - * otherwise the system stalls. > > + * If the ImgU gets configured it is then expected that buffers are > > + * queued to its outputs, otherwise the next capture session > > + * that uses the ImgU fails at queueing buffers at its input. > > * > > * If no ImgU configuration has been computed, it means only a RAW > > * stream has been requested: return here to skip the ImgU configuration > > -- > > 2.28.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On Tue, Sep 29, 2020 at 04:56:38PM +0200, Jacopo Mondi wrote: > On Tue, Sep 29, 2020 at 07:39:28AM +0200, Niklas Söderlund wrote: > > On 2020-09-24 16:51:43 +0200, Jacopo Mondi wrote: > > > Be more precise in commenting why the ImgU shall not be configured > > > if only the RAW stream is requested. > > > > > > As an example, if the ImgU gets unecessary configured: > > > cam -srole=viewfinder -c2 -C -> WORKS > > > cam -srole=stillraw -c2 -C -> WORKS > > > cam -srole=viewfinder -c2 -C -> Failed to queue buffer 0: Invalid argument > > > > > > Since commit 1bd2e981a209 ("libcamera: ipu3: Fix RAW+YUV capture") > > > > Unless you are _very_ lucky the commit hash will not be the same when > > this patch is merged :-) > > You're very right! > > > > the ImgU configuration procedure also correctly implements the > > > assumption that at least one of the YUV output is being operated. If > > > that's not the case, as in the RAW-only capture use case, the code > > > tries to access a non-existing configuration. One more reason to > > > exit early if no YUV stream is requested. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > With Laurent's comment addressed and the commit hash removed (or proof > > of guaranteed hash collision ;-P) > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Thanks, with this fixed I'll now push the series If you push the first patch first then you can update this one with the right commit ID before pushing it. > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 477f85a45755..2f90ad2e2780 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -459,9 +459,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > > return ret; > > > > > > /* > > > - * If the ImgU gets configured with proper IF, BDS and GDC sizes, it > > > - * is then expected that frames are dequeued from its main output > > > - * otherwise the system stalls. > > > + * If the ImgU gets configured it is then expected that buffers are > > > + * queued to its outputs, otherwise the next capture session > > > + * that uses the ImgU fails at queueing buffers at its input. > > > * > > > * If no ImgU configuration has been computed, it means only a RAW > > > * stream has been requested: return here to skip the ImgU configuration
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 477f85a45755..2f90ad2e2780 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -459,9 +459,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) return ret; /* - * If the ImgU gets configured with proper IF, BDS and GDC sizes, it - * is then expected that frames are dequeued from its main output - * otherwise the system stalls. + * If the ImgU gets configured it is then expected that buffers are + * queued to its outputs, otherwise the next capture session + * that uses the ImgU fails at queueing buffers at its input. * * If no ImgU configuration has been computed, it means only a RAW * stream has been requested: return here to skip the ImgU configuration
Be more precise in commenting why the ImgU shall not be configured if only the RAW stream is requested. As an example, if the ImgU gets unecessary configured: cam -srole=viewfinder -c2 -C -> WORKS cam -srole=stillraw -c2 -C -> WORKS cam -srole=viewfinder -c2 -C -> Failed to queue buffer 0: Invalid argument Since commit 1bd2e981a209 ("libcamera: ipu3: Fix RAW+YUV capture") the ImgU configuration procedure also correctly implements the assumption that at least one of the YUV output is being operated. If that's not the case, as in the RAW-only capture use case, the code tries to access a non-existing configuration. One more reason to exit early if no YUV stream is requested. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)