| Message ID | 20200316024146.2474424-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Mon, Mar 16, 2020 at 03:41:44AM +0100, Niklas Söderlund wrote: > This helper may be used to memory copy a while FrameBuffer content to s/while/whole/ ? > another buffer. The operation is not fast and should not be used without > grate care by pipelines. Unless you really meant to talk about BBQs, s/grate/great/ > > The intended use-case is to have an option to copy out RAW buffers from > the middle of an pipeline. s/an/a/ > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/buffer.h | 1 + > src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 8e5ec699e3925eee..669d2591a90e5f37 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -60,6 +60,7 @@ public: > private: > friend class Request; /* Needed to update request_. */ > friend class V4L2VideoDevice; /* Needed to update metadata_. */ > + friend int FrameBufferMemCopy(FrameBuffer *destination, const FrameBuffer *source); > > std::vector<Plane> planes_; > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > index 673a63d3d1658190..f680d1879b57a68b 100644 > --- a/src/libcamera/buffer.cpp > +++ b/src/libcamera/buffer.cpp > @@ -211,4 +211,47 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > * core never modifies the buffer cookie. > */ > > +int FrameBufferMemCopy(FrameBuffer *dst, const FrameBuffer *src) This is a function, it should start with a lower-case f, and should have a proper declaration in buffer.h. This being said, I think it would be best to move it to a member function. How about int FrameBuffer::copyTo(FrameBuffer *dst) ? Or maybe FrameBuffer::copyFrom(FrameBuffer *src) ? Documentation is also needed. > +{ > + if (dst->planes_.size() != src->planes_.size()) { > + LOG(Buffer, Error) << "Different number of planes"; > + return -EINVAL; > + } > + > + for (unsigned int i = 0; i < dst->planes_.size(); i++) { > + if (dst->planes_[i].length < src->planes_[i].length) { > + LOG(Buffer, Error) << "Plane " << i << " is too small"; > + return -EINVAL; > + } > + } We don't support that yet, so it's not really much of a concern, but will we have to handle the case where the stride differs ? And how about data offsets (when we'll have them too) ? Will we store that information in FrameBuffer::Plane or FrameMetadata::Plane ? I suspect the latter, so we'll have to ensure that metadata is valid, is it guaranteed ? > + > + for (unsigned int i = 0; i < dst->planes_.size(); i++) { > + void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED, > + dst->planes_[i].fd.fd(), 0); void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED, dst->planes_[i].fd.fd(), 0); And I think I'd name the variable dstmem or something similar. > + > + if (out == MAP_FAILED) { > + LOG(Buffer, Error) << "Failed to map output plane " << i; s/output/destination/ > + return -EINVAL; > + } > + > + void *in = mmap(NULL, src->planes_[i].length, PROT_READ, MAP_SHARED, > + src->planes_[i].fd.fd(), 0); Same here, with srcmem. > + > + if (in == MAP_FAILED) { > + munmap(out, dst->planes_[i].length); > + LOG(Buffer, Error) << "Failed to map input plane " << i; s/input/source/ > + return -EINVAL; > + } > + > + memcpy(out, in, src->planes_[i].length); length, or bytesused from the metadata ? > + > + munmap(in, src->planes_[i].length); > + munmap(out, dst->planes_[i].length); I really don't like how we create and tear down mappings every time :-( The alternative is to cache the mappings in the FrameBuffer class, but that's a slippery slope. We may not need to fix this now, but I think we need to consider our options. > + } > + > + dst->metadata_ = src->metadata_; > + > + return 0; > +} > + > } /* namespace libcamera */
Hi Laurent, Thanks for your feedback. On 2020-03-23 13:12:31 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Mon, Mar 16, 2020 at 03:41:44AM +0100, Niklas Söderlund wrote: > > This helper may be used to memory copy a while FrameBuffer content to > > s/while/whole/ ? > > > another buffer. The operation is not fast and should not be used without > > grate care by pipelines. > > Unless you really meant to talk about BBQs, s/grate/great/ > > > > > The intended use-case is to have an option to copy out RAW buffers from > > the middle of an pipeline. > > s/an/a/ > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/buffer.h | 1 + > > src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > > index 8e5ec699e3925eee..669d2591a90e5f37 100644 > > --- a/include/libcamera/buffer.h > > +++ b/include/libcamera/buffer.h > > @@ -60,6 +60,7 @@ public: > > private: > > friend class Request; /* Needed to update request_. */ > > friend class V4L2VideoDevice; /* Needed to update metadata_. */ > > + friend int FrameBufferMemCopy(FrameBuffer *destination, const FrameBuffer *source); > > > > std::vector<Plane> planes_; > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > > index 673a63d3d1658190..f680d1879b57a68b 100644 > > --- a/src/libcamera/buffer.cpp > > +++ b/src/libcamera/buffer.cpp > > @@ -211,4 +211,47 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > * core never modifies the buffer cookie. > > */ > > > > +int FrameBufferMemCopy(FrameBuffer *dst, const FrameBuffer *src) > > This is a function, it should start with a lower-case f, and should have > a proper declaration in buffer.h. This being said, I think it would be > best to move it to a member function. How about > > int FrameBuffer::copyTo(FrameBuffer *dst) > > ? Or maybe FrameBuffer::copyFrom(FrameBuffer *src) ? I like copyFrom(). > > Documentation is also needed. Yes. > > > +{ > > + if (dst->planes_.size() != src->planes_.size()) { > > + LOG(Buffer, Error) << "Different number of planes"; > > + return -EINVAL; > > + } > > + > > + for (unsigned int i = 0; i < dst->planes_.size(); i++) { > > + if (dst->planes_[i].length < src->planes_[i].length) { > > + LOG(Buffer, Error) << "Plane " << i << " is too small"; > > + return -EINVAL; > > + } > > + } > > We don't support that yet, so it's not really much of a concern, but > will we have to handle the case where the stride differs ? And how about > data offsets (when we'll have them too) ? Will we store that information > in FrameBuffer::Plane or FrameMetadata::Plane ? I suspect the latter, so > we'll have to ensure that metadata is valid, is it guaranteed ? I'm not sure how we will deal with strides and offsets when we add support for it. When I write this code my intention was to allow copy it one-to-one. I thought about verifying the buffer status before allowing it to be copied but decided against it. Do you think such a check is the right thing? I don't feel strongly about it. > > > + > > + for (unsigned int i = 0; i < dst->planes_.size(); i++) { > > + void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED, > > + dst->planes_[i].fd.fd(), 0); > > void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, > MAP_SHARED, dst->planes_[i].fd.fd(), 0); > > And I think I'd name the variable dstmem or something similar. > > > + > > + if (out == MAP_FAILED) { > > + LOG(Buffer, Error) << "Failed to map output plane " << i; > > s/output/destination/ > > > + return -EINVAL; > > + } > > + > > + void *in = mmap(NULL, src->planes_[i].length, PROT_READ, MAP_SHARED, > > + src->planes_[i].fd.fd(), 0); > > Same here, with srcmem. > > > + > > + if (in == MAP_FAILED) { > > + munmap(out, dst->planes_[i].length); > > + LOG(Buffer, Error) << "Failed to map input plane " << i; > > s/input/source/ > > > + return -EINVAL; > > + } > > + > > + memcpy(out, in, src->planes_[i].length); > > length, or bytesused from the metadata ? I picked length as it describes the full buffer length. But it might make sens to use bytesused as a small optimization. I think it goes hand in hand with your question above, if the buffer status shall be validated or not. > > > + > > + munmap(in, src->planes_[i].length); > > + munmap(out, dst->planes_[i].length); > > I really don't like how we create and tear down mappings every time :-( > The alternative is to cache the mappings in the FrameBuffer class, but > that's a slippery slope. We may not need to fix this now, but I think we > need to consider our options. I don't like it either, but adding a cache of it to FrameBuffer even less so. But maybe we will be forced to do so at some point. > > > + } > > + > > + dst->metadata_ = src->metadata_; > > + > > + return 0; > > +} > > + > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Mon, Mar 23, 2020 at 08:07:27PM +0100, Niklas Söderlund wrote: > On 2020-03-23 13:12:31 +0200, Laurent Pinchart wrote: > > On Mon, Mar 16, 2020 at 03:41:44AM +0100, Niklas Söderlund wrote: > >> This helper may be used to memory copy a while FrameBuffer content to > > > > s/while/whole/ ? > > > >> another buffer. The operation is not fast and should not be used without > >> grate care by pipelines. > > > > Unless you really meant to talk about BBQs, s/grate/great/ > > > >> > >> The intended use-case is to have an option to copy out RAW buffers from > >> the middle of an pipeline. > > > > s/an/a/ > > > >> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- > >> include/libcamera/buffer.h | 1 + > >> src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 44 insertions(+) > >> > >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > >> index 8e5ec699e3925eee..669d2591a90e5f37 100644 > >> --- a/include/libcamera/buffer.h > >> +++ b/include/libcamera/buffer.h > >> @@ -60,6 +60,7 @@ public: > >> private: > >> friend class Request; /* Needed to update request_. */ > >> friend class V4L2VideoDevice; /* Needed to update metadata_. */ > >> + friend int FrameBufferMemCopy(FrameBuffer *destination, const FrameBuffer *source); > >> > >> std::vector<Plane> planes_; > >> > >> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > >> index 673a63d3d1658190..f680d1879b57a68b 100644 > >> --- a/src/libcamera/buffer.cpp > >> +++ b/src/libcamera/buffer.cpp > >> @@ -211,4 +211,47 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > >> * core never modifies the buffer cookie. > >> */ > >> > >> +int FrameBufferMemCopy(FrameBuffer *dst, const FrameBuffer *src) > > > > This is a function, it should start with a lower-case f, and should have > > a proper declaration in buffer.h. This being said, I think it would be > > best to move it to a member function. How about > > > > int FrameBuffer::copyTo(FrameBuffer *dst) > > > > ? Or maybe FrameBuffer::copyFrom(FrameBuffer *src) ? > > I like copyFrom(). > > > > > Documentation is also needed. > > Yes. > > >> +{ > >> + if (dst->planes_.size() != src->planes_.size()) { > >> + LOG(Buffer, Error) << "Different number of planes"; > >> + return -EINVAL; > >> + } > >> + > >> + for (unsigned int i = 0; i < dst->planes_.size(); i++) { > >> + if (dst->planes_[i].length < src->planes_[i].length) { > >> + LOG(Buffer, Error) << "Plane " << i << " is too small"; > >> + return -EINVAL; > >> + } > >> + } > > > > We don't support that yet, so it's not really much of a concern, but > > will we have to handle the case where the stride differs ? And how about > > data offsets (when we'll have them too) ? Will we store that information > > in FrameBuffer::Plane or FrameMetadata::Plane ? I suspect the latter, so > > we'll have to ensure that metadata is valid, is it guaranteed ? > > I'm not sure how we will deal with strides and offsets when we add > support for it. When I write this code my intention was to allow copy it > one-to-one. I thought about verifying the buffer status before allowing > it to be copied but decided against it. > > Do you think such a check is the right thing? I don't feel strongly > about it. If the source and destination have different strides then we should do a stride-aware copy. Imagine a source with a padding to 128 bytes on every line, and a destination that wouldn't have that constraint. The destination size would actually be smaller than the source. We'd have to copy the data, not the padding. I wouldn't necessarily validate the state (the caller should know better), but I think the stride will need to be taken into account as it defines the data layout. > >> + > >> + for (unsigned int i = 0; i < dst->planes_.size(); i++) { > >> + void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED, > >> + dst->planes_[i].fd.fd(), 0); > > > > void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, > > MAP_SHARED, dst->planes_[i].fd.fd(), 0); > > > > And I think I'd name the variable dstmem or something similar. > > > >> + > >> + if (out == MAP_FAILED) { > >> + LOG(Buffer, Error) << "Failed to map output plane " << i; > > > > s/output/destination/ > > > >> + return -EINVAL; > >> + } > >> + > >> + void *in = mmap(NULL, src->planes_[i].length, PROT_READ, MAP_SHARED, > >> + src->planes_[i].fd.fd(), 0); > > > > Same here, with srcmem. > > > >> + > >> + if (in == MAP_FAILED) { > >> + munmap(out, dst->planes_[i].length); > >> + LOG(Buffer, Error) << "Failed to map input plane " << i; > > > > s/input/source/ > > > >> + return -EINVAL; > >> + } > >> + > >> + memcpy(out, in, src->planes_[i].length); > > > > length, or bytesused from the metadata ? > > I picked length as it describes the full buffer length. But it might > make sens to use bytesused as a small optimization. I think it goes hand > in hand with your question above, if the buffer status shall be > validated or not. > > >> + > >> + munmap(in, src->planes_[i].length); > >> + munmap(out, dst->planes_[i].length); > > > > I really don't like how we create and tear down mappings every time :-( > > The alternative is to cache the mappings in the FrameBuffer class, but > > that's a slippery slope. We may not need to fix this now, but I think we > > need to consider our options. > > I don't like it either, but adding a cache of it to FrameBuffer even > less so. But maybe we will be forced to do so at some point. > > >> + } > >> + > >> + dst->metadata_ = src->metadata_; > >> + > >> + return 0; > >> +} > >> + > >> } /* namespace libcamera */
diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h index 8e5ec699e3925eee..669d2591a90e5f37 100644 --- a/include/libcamera/buffer.h +++ b/include/libcamera/buffer.h @@ -60,6 +60,7 @@ public: private: friend class Request; /* Needed to update request_. */ friend class V4L2VideoDevice; /* Needed to update metadata_. */ + friend int FrameBufferMemCopy(FrameBuffer *destination, const FrameBuffer *source); std::vector<Plane> planes_; diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp index 673a63d3d1658190..f680d1879b57a68b 100644 --- a/src/libcamera/buffer.cpp +++ b/src/libcamera/buffer.cpp @@ -211,4 +211,47 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) * core never modifies the buffer cookie. */ +int FrameBufferMemCopy(FrameBuffer *dst, const FrameBuffer *src) +{ + if (dst->planes_.size() != src->planes_.size()) { + LOG(Buffer, Error) << "Different number of planes"; + return -EINVAL; + } + + for (unsigned int i = 0; i < dst->planes_.size(); i++) { + if (dst->planes_[i].length < src->planes_[i].length) { + LOG(Buffer, Error) << "Plane " << i << " is too small"; + return -EINVAL; + } + } + + for (unsigned int i = 0; i < dst->planes_.size(); i++) { + void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED, + dst->planes_[i].fd.fd(), 0); + + if (out == MAP_FAILED) { + LOG(Buffer, Error) << "Failed to map output plane " << i; + return -EINVAL; + } + + void *in = mmap(NULL, src->planes_[i].length, PROT_READ, MAP_SHARED, + src->planes_[i].fd.fd(), 0); + + if (in == MAP_FAILED) { + munmap(out, dst->planes_[i].length); + LOG(Buffer, Error) << "Failed to map input plane " << i; + return -EINVAL; + } + + memcpy(out, in, src->planes_[i].length); + + munmap(in, src->planes_[i].length); + munmap(out, dst->planes_[i].length); + } + + dst->metadata_ = src->metadata_; + + return 0; +} + } /* namespace libcamera */
This helper may be used to memory copy a while FrameBuffer content to another buffer. The operation is not fast and should not be used without grate care by pipelines. The intended use-case is to have an option to copy out RAW buffers from the middle of an pipeline. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/buffer.h | 1 + src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)