[libcamera-devel] [PATCH 8/9] libcamera: pipeline: ipu3: Use buffer mapping

[Jacopo Mondi]

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 at jmondi.org>
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190709/23c5c543/attachment.sig>