[libcamera-devel,2/2] ipa: vimc: Synchronise parameter buffer ops naming
diff mbox series

Message ID 20220408105439.144182-3-umang.jain@ideasonboard.com
State Accepted
Headers show
Series
  • VIMC IPA nitpicks
Related show

Commit Message

Umang Jain April 8, 2022, 10:54 a.m. UTC 
Synchronise the names of the operations with respect to parameters
buffer with the names used in other IPA interfaces.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 include/libcamera/ipa/vimc.mojom     | 4 ++--
 src/ipa/vimc/vimc.cpp                | 6 +++---
 src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)

Comments

Kieran Bingham April 8, 2022, 11:43 a.m. UTC | #1 
Quoting Umang Jain via libcamera-devel (2022-04-08 11:54:39)
> Synchronise the names of the operations with respect to parameters
> buffer with the names used in other IPA interfaces.
> 

The VIMC pipeline handler doesn't yet use an ISP, so I was curious at
what buffers this would represent - but actually I think it's fine to
keep it aligned at least.

I expect we could also use the VIMC pipeline handler with any GPU based
ISP we might develop so I expect it will get more alignment with that
development too.

So for this - I think it's fine.


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

> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  include/libcamera/ipa/vimc.mojom     | 4 ++--
>  src/ipa/vimc/vimc.cpp                | 6 +++---
>  src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++----
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> index cdc03ffb..718b9674 100644
> --- a/include/libcamera/ipa/vimc.mojom
> +++ b/include/libcamera/ipa/vimc.mojom
> @@ -37,9 +37,9 @@ interface IPAVimcInterface {
>          * interface functions that mimick how other pipeline handlers typically
>          * handle parameters at runtime.
>          */
> -       [async] fillParams(uint32 frame, uint32 bufferId);
> +       [async] fillParamsBuffer(uint32 frame, uint32 bufferId);
>  };
>  
>  interface IPAVimcEventInterface {
> -       paramsFilled(uint32 bufferId);
> +       paramsBufferReady(uint32 bufferId);
>  };
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index a62e72b0..85afb279 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -44,7 +44,7 @@ public:
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  
>         void queueRequest(uint32_t frame, const ControlList &controls) override;
> -       void fillParams(uint32_t frame, uint32_t bufferId) override;
> +       void fillParamsBuffer(uint32_t frame, uint32_t bufferId) override;
>  
>  private:
>         void initTrace();
> @@ -134,7 +134,7 @@ void IPAVimc::queueRequest([[maybe_unused]] uint32_t frame,
>  {
>  }
>  
> -void IPAVimc::fillParams([[maybe_unused]] uint32_t frame, uint32_t bufferId)
> +void IPAVimc::fillParamsBuffer([[maybe_unused]] uint32_t frame, uint32_t bufferId)
>  {
>         auto it = buffers_.find(bufferId);
>         if (it == buffers_.end()) {
> @@ -142,7 +142,7 @@ void IPAVimc::fillParams([[maybe_unused]] uint32_t frame, uint32_t bufferId)
>                 return;
>         }
>  
> -       paramsFilled.emit(bufferId);
> +       paramsBufferReady.emit(bufferId);
>  }
>  
>  void IPAVimc::initTrace()
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 69b83d07..fff95a34 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -53,7 +53,7 @@ public:
>         int init();
>         int allocateMockIPABuffers();
>         void bufferReady(FrameBuffer *buffer);
> -       void paramsFilled(unsigned int id);
> +       void paramsBufferReady(unsigned int id);
>  
>         MediaDevice *media_;
>         std::unique_ptr<CameraSensor> sensor_;
> @@ -467,7 +467,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>                 return false;
>         }
>  
> -       data->ipa_->paramsFilled.connect(data.get(), &VimcCameraData::paramsFilled);
> +       data->ipa_->paramsBufferReady.connect(data.get(), &VimcCameraData::paramsBufferReady);
>  
>         std::string conf = data->ipa_->configurationFile("vimc.conf");
>         data->ipa_->init(IPASettings{ conf, data->sensor_->model() });
> @@ -589,7 +589,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
>         pipe->completeBuffer(request, buffer);
>         pipe->completeRequest(request);
>  
> -       ipa_->fillParams(request->sequence(), mockIPABufs_[0]->cookie());
> +       ipa_->fillParamsBuffer(request->sequence(), mockIPABufs_[0]->cookie());
>  }
>  
>  int VimcCameraData::allocateMockIPABuffers()
> @@ -607,7 +607,7 @@ int VimcCameraData::allocateMockIPABuffers()
>         return video_->exportBuffers(kBufCount, &mockIPABufs_);
>  }
>  
> -void VimcCameraData::paramsFilled([[maybe_unused]] unsigned int id)
> +void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id)
>  {
>  }
>  
> -- 
> 2.31.0
>
Umang Jain April 8, 2022, 12:20 p.m. UTC | #2 
Hi Kieran,

On 4/8/22 17:13, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2022-04-08 11:54:39)
>> Synchronise the names of the operations with respect to parameters
>> buffer with the names used in other IPA interfaces.
>>
> The VIMC pipeline handler doesn't yet use an ISP, so I was curious at
> what buffers this would represent - but actually I think it's fine to
> keep it aligned at least.


These are mock buffers. They enable IPA IPC tests we have in tests/.

Also look at,

https://git.linuxtv.org/libcamera.git/commit/?id=3c5732d04a879d71d928e200d399c8d61b0da27a

https://git.linuxtv.org/libcamera.git/commit/?id=c2437e8cdefc7ae7efbfbd8662d822a02133084d

>
> I expect we could also use the VIMC pipeline handler with any GPU based
> ISP we might develop so I expect it will get more alignment with that
> development too.


Yes.

>
> So for this - I think it's fine.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


Thanks!

>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   include/libcamera/ipa/vimc.mojom     | 4 ++--
>>   src/ipa/vimc/vimc.cpp                | 6 +++---
>>   src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++----
>>   3 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
>> index cdc03ffb..718b9674 100644
>> --- a/include/libcamera/ipa/vimc.mojom
>> +++ b/include/libcamera/ipa/vimc.mojom
>> @@ -37,9 +37,9 @@ interface IPAVimcInterface {
>>           * interface functions that mimick how other pipeline handlers typically
>>           * handle parameters at runtime.
>>           */
>> -       [async] fillParams(uint32 frame, uint32 bufferId);
>> +       [async] fillParamsBuffer(uint32 frame, uint32 bufferId);
>>   };
>>   
>>   interface IPAVimcEventInterface {
>> -       paramsFilled(uint32 bufferId);
>> +       paramsBufferReady(uint32 bufferId);
>>   };
>> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
>> index a62e72b0..85afb279 100644
>> --- a/src/ipa/vimc/vimc.cpp
>> +++ b/src/ipa/vimc/vimc.cpp
>> @@ -44,7 +44,7 @@ public:
>>          void unmapBuffers(const std::vector<unsigned int> &ids) override;
>>   
>>          void queueRequest(uint32_t frame, const ControlList &controls) override;
>> -       void fillParams(uint32_t frame, uint32_t bufferId) override;
>> +       void fillParamsBuffer(uint32_t frame, uint32_t bufferId) override;
>>   
>>   private:
>>          void initTrace();
>> @@ -134,7 +134,7 @@ void IPAVimc::queueRequest([[maybe_unused]] uint32_t frame,
>>   {
>>   }
>>   
>> -void IPAVimc::fillParams([[maybe_unused]] uint32_t frame, uint32_t bufferId)
>> +void IPAVimc::fillParamsBuffer([[maybe_unused]] uint32_t frame, uint32_t bufferId)
>>   {
>>          auto it = buffers_.find(bufferId);
>>          if (it == buffers_.end()) {
>> @@ -142,7 +142,7 @@ void IPAVimc::fillParams([[maybe_unused]] uint32_t frame, uint32_t bufferId)
>>                  return;
>>          }
>>   
>> -       paramsFilled.emit(bufferId);
>> +       paramsBufferReady.emit(bufferId);
>>   }
>>   
>>   void IPAVimc::initTrace()
>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>> index 69b83d07..fff95a34 100644
>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>> @@ -53,7 +53,7 @@ public:
>>          int init();
>>          int allocateMockIPABuffers();
>>          void bufferReady(FrameBuffer *buffer);
>> -       void paramsFilled(unsigned int id);
>> +       void paramsBufferReady(unsigned int id);
>>   
>>          MediaDevice *media_;
>>          std::unique_ptr<CameraSensor> sensor_;
>> @@ -467,7 +467,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>>                  return false;
>>          }
>>   
>> -       data->ipa_->paramsFilled.connect(data.get(), &VimcCameraData::paramsFilled);
>> +       data->ipa_->paramsBufferReady.connect(data.get(), &VimcCameraData::paramsBufferReady);
>>   
>>          std::string conf = data->ipa_->configurationFile("vimc.conf");
>>          data->ipa_->init(IPASettings{ conf, data->sensor_->model() });
>> @@ -589,7 +589,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
>>          pipe->completeBuffer(request, buffer);
>>          pipe->completeRequest(request);
>>   
>> -       ipa_->fillParams(request->sequence(), mockIPABufs_[0]->cookie());
>> +       ipa_->fillParamsBuffer(request->sequence(), mockIPABufs_[0]->cookie());
>>   }
>>   
>>   int VimcCameraData::allocateMockIPABuffers()
>> @@ -607,7 +607,7 @@ int VimcCameraData::allocateMockIPABuffers()
>>          return video_->exportBuffers(kBufCount, &mockIPABufs_);
>>   }
>>   
>> -void VimcCameraData::paramsFilled([[maybe_unused]] unsigned int id)
>> +void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id)
>>   {
>>   }
>>   
>> -- 
>> 2.31.0
>>

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
index cdc03ffb..718b9674 100644
--- a/include/libcamera/ipa/vimc.mojom
+++ b/include/libcamera/ipa/vimc.mojom
@@ -37,9 +37,9 @@  interface IPAVimcInterface {
 	 * interface functions that mimick how other pipeline handlers typically
 	 * handle parameters at runtime.
 	 */
-	[async] fillParams(uint32 frame, uint32 bufferId);
+	[async] fillParamsBuffer(uint32 frame, uint32 bufferId);
 };
 
 interface IPAVimcEventInterface {
-	paramsFilled(uint32 bufferId);
+	paramsBufferReady(uint32 bufferId);
 };
diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
index a62e72b0..85afb279 100644
--- a/src/ipa/vimc/vimc.cpp
+++ b/src/ipa/vimc/vimc.cpp
@@ -44,7 +44,7 @@  public:
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 
 	void queueRequest(uint32_t frame, const ControlList &controls) override;
-	void fillParams(uint32_t frame, uint32_t bufferId) override;
+	void fillParamsBuffer(uint32_t frame, uint32_t bufferId) override;
 
 private:
 	void initTrace();
@@ -134,7 +134,7 @@  void IPAVimc::queueRequest([[maybe_unused]] uint32_t frame,
 {
 }
 
-void IPAVimc::fillParams([[maybe_unused]] uint32_t frame, uint32_t bufferId)
+void IPAVimc::fillParamsBuffer([[maybe_unused]] uint32_t frame, uint32_t bufferId)
 {
 	auto it = buffers_.find(bufferId);
 	if (it == buffers_.end()) {
@@ -142,7 +142,7 @@  void IPAVimc::fillParams([[maybe_unused]] uint32_t frame, uint32_t bufferId)
 		return;
 	}
 
-	paramsFilled.emit(bufferId);
+	paramsBufferReady.emit(bufferId);
 }
 
 void IPAVimc::initTrace()
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 69b83d07..fff95a34 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -53,7 +53,7 @@  public:
 	int init();
 	int allocateMockIPABuffers();
 	void bufferReady(FrameBuffer *buffer);
-	void paramsFilled(unsigned int id);
+	void paramsBufferReady(unsigned int id);
 
 	MediaDevice *media_;
 	std::unique_ptr<CameraSensor> sensor_;
@@ -467,7 +467,7 @@  bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
 		return false;
 	}
 
-	data->ipa_->paramsFilled.connect(data.get(), &VimcCameraData::paramsFilled);
+	data->ipa_->paramsBufferReady.connect(data.get(), &VimcCameraData::paramsBufferReady);
 
 	std::string conf = data->ipa_->configurationFile("vimc.conf");
 	data->ipa_->init(IPASettings{ conf, data->sensor_->model() });
@@ -589,7 +589,7 @@  void VimcCameraData::bufferReady(FrameBuffer *buffer)
 	pipe->completeBuffer(request, buffer);
 	pipe->completeRequest(request);
 
-	ipa_->fillParams(request->sequence(), mockIPABufs_[0]->cookie());
+	ipa_->fillParamsBuffer(request->sequence(), mockIPABufs_[0]->cookie());
 }
 
 int VimcCameraData::allocateMockIPABuffers()
@@ -607,7 +607,7 @@  int VimcCameraData::allocateMockIPABuffers()
 	return video_->exportBuffers(kBufCount, &mockIPABufs_);
 }
 
-void VimcCameraData::paramsFilled([[maybe_unused]] unsigned int id)
+void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id)
 {
 }