[v3,13/39] libcamera: software_isp: Make output DMA sync contingent
diff mbox series

Message ID 20251015012251.17508-14-bryan.odonoghue@linaro.org
State New
Headers show
Series
  • Add GLES 2.0 GPUISP to libcamera
Related show

Commit Message

Bryan O'Donoghue Oct. 15, 2025, 1:22 a.m. UTC 
The DMA sync output buffer from the GPU need only have its cache
invalidated if the CPU is going to modify the buffer. Right now this is
not required for gpuisp so only act on the output buffer if it is
non-null.

Suggested-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 src/libcamera/software_isp/debayer.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Oct. 15, 2025, 10:12 p.m. UTC | #1 
Quoting Bryan O'Donoghue (2025-10-15 02:22:25)
> The DMA sync output buffer from the GPU need only have its cache
> invalidated if the CPU is going to modify the buffer. Right now this is
> not required for gpuisp so only act on the output buffer if it is
> non-null.
> 
> Suggested-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  src/libcamera/software_isp/debayer.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> index 847067aa..96737c45 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -223,8 +223,10 @@ void Debayer::dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *inpu
>         for (const FrameBuffer::Plane &plane : input->planes())
>                 dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);
>  

Completly nit-picking (and what you have is fine here so don't change
unless /you/ prefer this) but here I might have done:


{
	for (const FrameBuffer::Plane &plane : input->planes())
		dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);

	/* 
	 * Output buffer handling is optional as cache operations are expensive
	 * and unnecessary on GPU ISP where the CPU does not write to
	 * the output buffer.
	 */
	if (!output)
		return

	for (const FrameBuffer::Plane &plane : output->planes())
		dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
}

Either way:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> -       for (const FrameBuffer::Plane &plane : output->planes())
> -               dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
> +       if (output) {
> +               for (const FrameBuffer::Plane &plane : output->planes())
> +                       dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
> +       }
>  }
>  
>  } /* namespace libcamera */
> -- 
> 2.51.0
>
Milan Zamazal Oct. 16, 2025, 8:38 a.m. UTC | #2 
Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Bryan O'Donoghue (2025-10-15 02:22:25)
>> The DMA sync output buffer from the GPU need only have its cache
>> invalidated if the CPU is going to modify the buffer. Right now this is
>
>> not required for gpuisp so only act on the output buffer if it is
>> non-null.
>> 
>> Suggested-by: Robert Mader <robert.mader@collabora.com>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>  src/libcamera/software_isp/debayer.cpp | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>> index 847067aa..96737c45 100644
>> --- a/src/libcamera/software_isp/debayer.cpp
>> +++ b/src/libcamera/software_isp/debayer.cpp
>> @@ -223,8 +223,10 @@ void Debayer::dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *inpu
>>         for (const FrameBuffer::Plane &plane : input->planes())
>>                 dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);
>>  
>
> Completly nit-picking (and what you have is fine here so don't change
> unless /you/ prefer this) but here I might have done:
>
>
> {
> 	for (const FrameBuffer::Plane &plane : input->planes())
> 		dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);
>
> 	/* 
> 	 * Output buffer handling is optional as cache operations are expensive
> 	 * and unnecessary on GPU ISP where the CPU does not write to
> 	 * the output buffer.
> 	 */

Such comments in the code are very useful, at least for non-experts
(like me).  Voting for such a change.  With that:

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

> 	if (!output)
> 		return
>
> 	for (const FrameBuffer::Plane &plane : output->planes())
> 		dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
> }
>
> Either way:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> -       for (const FrameBuffer::Plane &plane : output->planes())
>> -               dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
>> +       if (output) {
>> +               for (const FrameBuffer::Plane &plane : output->planes())
>> +                       dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
>> +       }
>>  }
>>  
>>  } /* namespace libcamera */
>> -- 
>> 2.51.0
>>

Patch
diff mbox series 

diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
index 847067aa..96737c45 100644
--- a/src/libcamera/software_isp/debayer.cpp
+++ b/src/libcamera/software_isp/debayer.cpp
@@ -223,8 +223,10 @@  void Debayer::dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *inpu
 	for (const FrameBuffer::Plane &plane : input->planes())
 		dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);
 
-	for (const FrameBuffer::Plane &plane : output->planes())
-		dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
+	if (output) {
+		for (const FrameBuffer::Plane &plane : output->planes())
+			dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
+	}
 }
 
 } /* namespace libcamera */