| Message ID | 20190704225334.26170-9-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-07-05 00:53:33 +0200, Jacopo Mondi wrote: > In order to support the usage of application provided buffer, retrieve > the buffer to use on video devices using the Request in order to allow > the stream to perform buffer mapping, if requested. > > The IPU3 was the only pipeline handler to access the Request map > directly instead of using Request::findBuffer(). I now see why we really wish to hide the buffers() method. I think we need to think hard on how to do that. But that could be done on-top of this work. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 28dcefe3d19f..49aa27ff20d4 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -725,7 +725,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request) > > for (auto it : request->buffers()) { > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > - Buffer *buffer = it.second; > + Buffer *buffer = request->findBuffer(stream); > > int ret = stream->device_->dev->queueBuffer(buffer); > if (ret < 0) > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 06/07/2019 13:12, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2019-07-05 00:53:33 +0200, Jacopo Mondi wrote: >> In order to support the usage of application provided buffer, retrieve >> the buffer to use on video devices using the Request in order to allow >> the stream to perform buffer mapping, if requested. >> >> The IPU3 was the only pipeline handler to access the Request map >> directly instead of using Request::findBuffer(). > > I now see why we really wish to hide the buffers() method. I think we > need to think hard on how to do that. But that could be done on-top of > this work. > >> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 28dcefe3d19f..49aa27ff20d4 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -725,7 +725,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request) >> >> for (auto it : request->buffers()) { >> IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); >> - Buffer *buffer = it.second; >> + Buffer *buffer = request->findBuffer(stream); This looks really weird ... "For each buffer, identify the stream, and then identify the buffer for that stream..." ... isn't that just ... the buffer(iterator) you started with ? I think from my understanding of the series it's because there are now 2 'Buffer*' objects for each buffer queued (one to hold the V4L2 information, and one to hold .. the dmabuf? In my head, all of this information should be in the same Buffer* ... >> >> int ret = stream->device_->dev->queueBuffer(buffer); >> if (ret < 0) >> -- >> 2.21.0 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran, On Mon, Jul 08, 2019 at 10:29:44AM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 06/07/2019 13:12, Niklas Söderlund wrote: > > Hi Jacopo, > > > > Thanks for your work. > > > > On 2019-07-05 00:53:33 +0200, Jacopo Mondi wrote: > >> In order to support the usage of application provided buffer, retrieve > >> the buffer to use on video devices using the Request in order to allow > >> the stream to perform buffer mapping, if requested. > >> > >> The IPU3 was the only pipeline handler to access the Request map > >> directly instead of using Request::findBuffer(). > > > > I now see why we really wish to hide the buffers() method. I think we > > need to think hard on how to do that. But that could be done on-top of > > this work. > > > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 28dcefe3d19f..49aa27ff20d4 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -725,7 +725,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request) > >> > >> for (auto it : request->buffers()) { > >> IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > >> - Buffer *buffer = it.second; > >> + Buffer *buffer = request->findBuffer(stream); > > This looks really weird ... > > "For each buffer, identify the stream, and then identify the buffer for > that stream..." ... isn't that just ... the buffer(iterator) you started > with ? These are not the same Buffers, one is the one which maps onto the V4L2 device one, the other ones comes from outside. As a general reply to your "importBuffers()" question, what you're asking for is in my RFC version, and it has been partially shut down because that would required application to create pools, and that's not a a problem per-se, it is just not needed :) Furthermore, I think we should move to restrict applications to access the Stream's pool completely, or expose it as read-only objects if we really have to. Pools are an additional abstraction that would require more work for application, which really just need buffers. > > > I think from my understanding of the series it's because there are now 2 > 'Buffer*' objects for each buffer queued (one to hold the V4L2 > information, and one to hold .. the dmabuf? > > In my head, all of this information should be in the same Buffer* ... As the V4L2 video device works today this is not possible. Buffers are always dequeued from the internal pool, which is not exposed to application (and its buffers should not be used directly, for the simple reason that they are a finite number and we might need to 'map' a much larger number of different dmabufs file descriptors). Anyway, this is going through heavy rework as we speech, especially the Buffer class and associated objects, so this will be changes a lot. I think the mapping part should stay similar to what's here, so for the moment just please "hold on" :) Thanks j > > > > > > >> > >> int ret = stream->device_->dev->queueBuffer(buffer); > >> if (ret < 0) > >> -- > >> 2.21.0 > >> > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 28dcefe3d19f..49aa27ff20d4 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -725,7 +725,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request) for (auto it : request->buffers()) { IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); - Buffer *buffer = it.second; + Buffer *buffer = request->findBuffer(stream); int ret = stream->device_->dev->queueBuffer(buffer); if (ret < 0)
In order to support the usage of application provided buffer, retrieve the buffer to use on video devices using the Request in order to allow the stream to perform buffer mapping, if requested. The IPU3 was the only pipeline handler to access the Request map directly instead of using Request::findBuffer(). Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)