[v8,8/8] libcamera: software_isp: Refactor SoftwareIsp to use DmaBufAllocator::exportBuffers
diff mbox series

Message ID 20240805135104.139932-9-chenghaoyang@google.com
State Superseded
Headers show
Series
  • Add VirtualPipelineHandler
Related show

Commit Message

Harvey Yang Aug. 5, 2024, 1:48 p.m. UTC 
As the helper function DmaBufAllocator::exportBuffers is added, we can
avoid some code duplication in SoftwareIsp as well.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/libcamera/software_isp/software_isp.cpp | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

Comments

Milan Zamazal Aug. 6, 2024, 10:29 a.m. UTC | #1 
Hi,

thank you for the patch.

Harvey Yang <chenghaoyang@chromium.org> writes:

> As the helper function DmaBufAllocator::exportBuffers is added, we can
> avoid some code duplication in SoftwareIsp as well.
>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  src/libcamera/software_isp/software_isp.cpp | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index c8748d88..5240eb3f 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -256,23 +256,7 @@ int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count,
>  	if (stream == nullptr)
>  		return -EINVAL;
>  
> -	for (unsigned int i = 0; i < count; i++) {
> -		const std::string name = "frame-" + std::to_string(i);
> -		const size_t frameSize = debayer_->frameSize();
> -
> -		FrameBuffer::Plane outPlane;
> -		outPlane.fd = SharedFD(dmaHeap_.alloc(name.c_str(), frameSize));
> -		if (!outPlane.fd.isValid()) {
> -			LOG(SoftwareIsp, Error)
> -				<< "failed to allocate a dma_buf";
> -			return -ENOMEM;
> -		}
> -		outPlane.offset = 0;
> -		outPlane.length = frameSize;
> -
> -		std::vector<FrameBuffer::Plane> planes{ outPlane };
> -		buffers->emplace_back(std::make_unique<FrameBuffer>(std::move(planes)));
> -	}
> +	dmaHeap_.exportBuffers(count, { debayer_->frameSize() }, buffers);
>  
>  	return count;

I think it should be

  return dmaHeap_.exportBuffers(count, { debayer_->frameSize() }, buffers);

so that the right value is returned on errors.

With that change:

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

>  }
Cheng-Hao Yang Aug. 6, 2024, 11:43 a.m. UTC | #2 
Thanks for the review Milan!

Uploaded to Gitlab:
https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/7e25bf9aaaa62decb441fdc1627653e4933a43e0

To avoid the spam, I'll upload another series of patches with other updates.

BR,
Harvey

On Tue, Aug 6, 2024 at 12:29 PM Milan Zamazal <mzamazal@redhat.com> wrote:

> Hi,
>
> thank you for the patch.
>
> Harvey Yang <chenghaoyang@chromium.org> writes:
>
> > As the helper function DmaBufAllocator::exportBuffers is added, we can
> > avoid some code duplication in SoftwareIsp as well.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  src/libcamera/software_isp/software_isp.cpp | 18 +-----------------
> >  1 file changed, 1 insertion(+), 17 deletions(-)
> >
> > diff --git a/src/libcamera/software_isp/software_isp.cpp
> b/src/libcamera/software_isp/software_isp.cpp
> > index c8748d88..5240eb3f 100644
> > --- a/src/libcamera/software_isp/software_isp.cpp
> > +++ b/src/libcamera/software_isp/software_isp.cpp
> > @@ -256,23 +256,7 @@ int SoftwareIsp::exportBuffers(const Stream
> *stream, unsigned int count,
> >       if (stream == nullptr)
> >               return -EINVAL;
> >
> > -     for (unsigned int i = 0; i < count; i++) {
> > -             const std::string name = "frame-" + std::to_string(i);
> > -             const size_t frameSize = debayer_->frameSize();
> > -
> > -             FrameBuffer::Plane outPlane;
> > -             outPlane.fd = SharedFD(dmaHeap_.alloc(name.c_str(),
> frameSize));
> > -             if (!outPlane.fd.isValid()) {
> > -                     LOG(SoftwareIsp, Error)
> > -                             << "failed to allocate a dma_buf";
> > -                     return -ENOMEM;
> > -             }
> > -             outPlane.offset = 0;
> > -             outPlane.length = frameSize;
> > -
> > -             std::vector<FrameBuffer::Plane> planes{ outPlane };
> > -
>  buffers->emplace_back(std::make_unique<FrameBuffer>(std::move(planes)));
> > -     }
> > +     dmaHeap_.exportBuffers(count, { debayer_->frameSize() }, buffers);
> >
> >       return count;
>
> I think it should be
>
>   return dmaHeap_.exportBuffers(count, { debayer_->frameSize() }, buffers);
>
> so that the right value is returned on errors.
>
> With that change:
>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>
> >  }
>
>

Patch
diff mbox series 

diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index c8748d88..5240eb3f 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -256,23 +256,7 @@  int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count,
 	if (stream == nullptr)
 		return -EINVAL;
 
-	for (unsigned int i = 0; i < count; i++) {
-		const std::string name = "frame-" + std::to_string(i);
-		const size_t frameSize = debayer_->frameSize();
-
-		FrameBuffer::Plane outPlane;
-		outPlane.fd = SharedFD(dmaHeap_.alloc(name.c_str(), frameSize));
-		if (!outPlane.fd.isValid()) {
-			LOG(SoftwareIsp, Error)
-				<< "failed to allocate a dma_buf";
-			return -ENOMEM;
-		}
-		outPlane.offset = 0;
-		outPlane.length = frameSize;
-
-		std::vector<FrameBuffer::Plane> planes{ outPlane };
-		buffers->emplace_back(std::make_unique<FrameBuffer>(std::move(planes)));
-	}
+	dmaHeap_.exportBuffers(count, { debayer_->frameSize() }, buffers);
 
 	return count;
 }