[libcamera-devel,v1,6/9] ipa: raspberrypi: Pass lens shading table through configure() function
diff mbox series

Message ID 20200628231934.29025-7-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Support passing custom data to IPA configure()
Related show

Commit Message

Laurent Pinchart June 28, 2020, 11:19 p.m. UTC 
The IPAInterface now accepts custom configuration data. Use it to pass
the lens shading table instead of using a custom IPA event. This will
allow starting the IPA when starting the camera, instead of pre-starting
it early in order to process the lens shading table allocation event.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/ipa/raspberrypi.h                |  5 ++++-
 src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
 3 files changed, 14 insertions(+), 13 deletions(-)

Comments

Niklas Söderlund June 29, 2020, 2:51 p.m. UTC | #1 
Hi Laurent,

Thanks for your work.

On 2020-06-29 02:19:31 +0300, Laurent Pinchart wrote:
> The IPAInterface now accepts custom configuration data. Use it to pass
> the lens shading table instead of using a custom IPA event. This will
> allow starting the IPA when starting the camera, instead of pre-starting
> it early in order to process the lens shading table allocation event.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
>  3 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a18ce9a884b6..c335d0259549 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -10,6 +10,10 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  
> +enum RPiConfigParameters {
> +	RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> +};
> +
>  enum RPiOperations {
>  	RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
>  	RPI_IPA_ACTION_V4L2_SET_ISP,
> @@ -21,7 +25,6 @@ enum RPiOperations {
>  	RPI_IPA_EVENT_SIGNAL_STAT_READY,
>  	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
>  	RPI_IPA_EVENT_QUEUE_REQUEST,
> -	RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
>  };
>  
>  enum RPiIpaMask {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 860be22ddb5d..c9f7dea375a5 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		applyAGC(&agcStatus);
>  
>  	lastMode_ = mode_;
> +
> +	/* Store the lens shading table pointer and handle if available. */
> +	if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {
> +		lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> +		lsTableHandle_ = ipaConfig.data[1];
> +	}
>  }
>  
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  		break;
>  	}
>  
> -	case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> -		lsTable_ = reinterpret_cast<void *>(event.data[0]);
> -		lsTableHandle_ = event.data[1];
> -		break;
> -	}
> -
>  	default:
>  		LOG(IPARPI, Error) << "Unknown event " << event.operation;
>  		break;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0f9237a7f346..903796790f44 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
>  {
>  	std::map<unsigned int, IPAStream> streamConfig;
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> +	IPAOperationData ipaConfig = {};
>  
>  	/* Get the device format to pass to the IPA. */
>  	V4L2DeviceFormat sensorFormat;
> @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
>  		 * The vcsm allocation will always be in the memory region
>  		 * < 32-bits to allow Videocore to access the memory.
>  		 */
> -		IPAOperationData op;
> -		op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> -		op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> -			    vcsm_.getVCHandle(lsTable_) };
> -		ipa_->processEvent(op);
> +		ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> +		ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> +				   vcsm_.getVCHandle(lsTable_) };

Sending a pointer to the IPA caught my eye as potentially dangerous. If 
I understand the situation correctly this is a temporary workaround 
while vc_sm_cma is reworked to support dmabuf? Do we know the status of 
that work? In the mean time should we add a warning/todo here so we know 
whats going on if we ever troubleshoot an issue to this location?

This have however nothing to do with this patch,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

>  	}
>  
>  	CameraSensorInfo sensorInfo = {};
> @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
>  	}
>  
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	IPAOperationData ipaConfig;
>  	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
>  			nullptr);
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart June 29, 2020, 3:03 p.m. UTC | #2 
Hi Niklas,

(CC'ing Dave)

On Mon, Jun 29, 2020 at 04:51:09PM +0200, Niklas Söderlund wrote:
> On 2020-06-29 02:19:31 +0300, Laurent Pinchart wrote:
> > The IPAInterface now accepts custom configuration data. Use it to pass
> > the lens shading table instead of using a custom IPA event. This will
> > allow starting the IPA when starting the camera, instead of pre-starting
> > it early in order to process the lens shading table allocation event.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
> >  3 files changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index a18ce9a884b6..c335d0259549 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -10,6 +10,10 @@
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >  
> > +enum RPiConfigParameters {
> > +	RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> > +};
> > +
> >  enum RPiOperations {
> >  	RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
> >  	RPI_IPA_ACTION_V4L2_SET_ISP,
> > @@ -21,7 +25,6 @@ enum RPiOperations {
> >  	RPI_IPA_EVENT_SIGNAL_STAT_READY,
> >  	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> >  	RPI_IPA_EVENT_QUEUE_REQUEST,
> > -	RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
> >  };
> >  
> >  enum RPiIpaMask {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 860be22ddb5d..c9f7dea375a5 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  		applyAGC(&agcStatus);
> >  
> >  	lastMode_ = mode_;
> > +
> > +	/* Store the lens shading table pointer and handle if available. */
> > +	if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {
> > +		lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> > +		lsTableHandle_ = ipaConfig.data[1];
> > +	}
> >  }
> >  
> >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >  		break;
> >  	}
> >  
> > -	case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> > -		lsTable_ = reinterpret_cast<void *>(event.data[0]);
> > -		lsTableHandle_ = event.data[1];
> > -		break;
> > -	}
> > -
> >  	default:
> >  		LOG(IPARPI, Error) << "Unknown event " << event.operation;
> >  		break;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0f9237a7f346..903796790f44 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
> >  {
> >  	std::map<unsigned int, IPAStream> streamConfig;
> >  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> > +	IPAOperationData ipaConfig = {};
> >  
> >  	/* Get the device format to pass to the IPA. */
> >  	V4L2DeviceFormat sensorFormat;
> > @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
> >  		 * The vcsm allocation will always be in the memory region
> >  		 * < 32-bits to allow Videocore to access the memory.
> >  		 */
> > -		IPAOperationData op;
> > -		op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> > -		op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > -			    vcsm_.getVCHandle(lsTable_) };
> > -		ipa_->processEvent(op);
> > +		ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> > +		ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > +				   vcsm_.getVCHandle(lsTable_) };
> 
> Sending a pointer to the IPA caught my eye as potentially dangerous. If 
> I understand the situation correctly this is a temporary workaround 
> while vc_sm_cma is reworked to support dmabuf? Do we know the status of 
> that work?

Dave, do you have any update on this ?

> In the mean time should we add a warning/todo here so we know 
> whats going on if we ever troubleshoot an issue to this location?

A warning would be a bit too verbose I think. We can add a \todo
comment. Would you like to submit a patch ? You can use master as a
base, and I'll handle the rebase.

> This have however nothing to do with this patch,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> >  	}
> >  
> >  	CameraSensorInfo sensorInfo = {};
> > @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
> >  	}
> >  
> >  	/* Ready the IPA - it must know about the sensor resolution. */
> > -	IPAOperationData ipaConfig;
> >  	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> >  			nullptr);
> >
Naushir Patuck June 30, 2020, 10:38 a.m. UTC | #3 
Hi Laurent,

Thank you for your patch.

On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The IPAInterface now accepts custom configuration data. Use it to pass
> the lens shading table instead of using a custom IPA event. This will
> allow starting the IPA when starting the camera, instead of pre-starting
> it early in order to process the lens shading table allocation event.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
>  3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a18ce9a884b6..c335d0259549 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -10,6 +10,10 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>
> +enum RPiConfigParameters {
> +       RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> +};

Minor thing, but could you rename this to RPI_IPA_CONFIG_LS_TABLE to
be consistent with other labels?

Otherwise all looks good.

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>



> +
>  enum RPiOperations {
>         RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
>         RPI_IPA_ACTION_V4L2_SET_ISP,
> @@ -21,7 +25,6 @@ enum RPiOperations {
>         RPI_IPA_EVENT_SIGNAL_STAT_READY,
>         RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
>         RPI_IPA_EVENT_QUEUE_REQUEST,
> -       RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
>  };
>
>  enum RPiIpaMask {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 860be22ddb5d..c9f7dea375a5 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>                 applyAGC(&agcStatus);
>
>         lastMode_ = mode_;
> +
> +       /* Store the lens shading table pointer and handle if available. */
> +       if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {
> +               lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> +               lsTableHandle_ = ipaConfig.data[1];
> +       }
>  }
>
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
>                 break;
>         }
>
> -       case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> -               lsTable_ = reinterpret_cast<void *>(event.data[0]);
> -               lsTableHandle_ = event.data[1];
> -               break;
> -       }
> -
>         default:
>                 LOG(IPARPI, Error) << "Unknown event " << event.operation;
>                 break;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0f9237a7f346..903796790f44 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
>  {
>         std::map<unsigned int, IPAStream> streamConfig;
>         std::map<unsigned int, const ControlInfoMap &> entityControls;
> +       IPAOperationData ipaConfig = {};
>
>         /* Get the device format to pass to the IPA. */
>         V4L2DeviceFormat sensorFormat;
> @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
>                  * The vcsm allocation will always be in the memory region
>                  * < 32-bits to allow Videocore to access the memory.
>                  */
> -               IPAOperationData op;
> -               op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> -               op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> -                           vcsm_.getVCHandle(lsTable_) };
> -               ipa_->processEvent(op);
> +               ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> +               ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> +                                  vcsm_.getVCHandle(lsTable_) };
>         }
>
>         CameraSensorInfo sensorInfo = {};
> @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
>         }
>
>         /* Ready the IPA - it must know about the sensor resolution. */
> -       IPAOperationData ipaConfig;
>         ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
>                         nullptr);
>
> --
> Regards,
>
> Laurent Pinchart
>
Jacopo Mondi June 30, 2020, 10:45 a.m. UTC | #4 
Hi Laurent,

On Mon, Jun 29, 2020 at 02:19:31AM +0300, Laurent Pinchart wrote:
> The IPAInterface now accepts custom configuration data. Use it to pass

The IPAInterface::configure()

> the lens shading table instead of using a custom IPA event. This will
> allow starting the IPA when starting the camera, instead of pre-starting
> it early in order to process the lens shading table allocation event.

If the IPA is meant to be started at Camera::start() time, and we pass
the lens shading table at ipa_->configure() time which happens before,
what is different ?

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
>  3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a18ce9a884b6..c335d0259549 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -10,6 +10,10 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>
> +enum RPiConfigParameters {
> +	RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> +};
> +

Is there any value in keeping this enum separated if not creating a
distinction between operations run through processEvent() and
operations run at configure() time ? Can't the list of operation be
unique (which would also avoid calshing, if the same operations has
to be handled both at processEvent() and configure() time.

Also, why (1 << 0) in an enum ? Can't we start from 0 (or 1 as
RPiOperations does) and increase ?

>  enum RPiOperations {
>  	RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
>  	RPI_IPA_ACTION_V4L2_SET_ISP,
> @@ -21,7 +25,6 @@ enum RPiOperations {
>  	RPI_IPA_EVENT_SIGNAL_STAT_READY,
>  	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
>  	RPI_IPA_EVENT_QUEUE_REQUEST,
> -	RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
>  };
>
>  enum RPiIpaMask {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 860be22ddb5d..c9f7dea375a5 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		applyAGC(&agcStatus);
>
>  	lastMode_ = mode_;
> +
> +	/* Store the lens shading table pointer and handle if available. */
> +	if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {

That's why (1 << 0). What is the advantage of using flags, instead of
switching on the operation identifiers as done below in
processEvent() ?

> +		lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> +		lsTableHandle_ = ipaConfig.data[1];
> +	}
>  }
>
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  		break;
>  	}
>
> -	case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> -		lsTable_ = reinterpret_cast<void *>(event.data[0]);
> -		lsTableHandle_ = event.data[1];
> -		break;
> -	}
> -
>  	default:
>  		LOG(IPARPI, Error) << "Unknown event " << event.operation;
>  		break;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0f9237a7f346..903796790f44 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
>  {
>  	std::map<unsigned int, IPAStream> streamConfig;
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> +	IPAOperationData ipaConfig = {};
>
>  	/* Get the device format to pass to the IPA. */
>  	V4L2DeviceFormat sensorFormat;
> @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
>  		 * The vcsm allocation will always be in the memory region
>  		 * < 32-bits to allow Videocore to access the memory.
>  		 */
> -		IPAOperationData op;
> -		op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> -		op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> -			    vcsm_.getVCHandle(lsTable_) };
> -		ipa_->processEvent(op);
> +		ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> +		ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> +				   vcsm_.getVCHandle(lsTable_) };
>  	}
>
>  	CameraSensorInfo sensorInfo = {};
> @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
>  	}
>
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	IPAOperationData ipaConfig;
>  	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
>  			nullptr);
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 2, 2020, 9:31 p.m. UTC | #5 
Hi Jacopo,

On Tue, Jun 30, 2020 at 12:45:08PM +0200, Jacopo Mondi wrote:
> On Mon, Jun 29, 2020 at 02:19:31AM +0300, Laurent Pinchart wrote:
> > The IPAInterface now accepts custom configuration data. Use it to pass
> 
> The IPAInterface::configure()
> 
> > the lens shading table instead of using a custom IPA event. This will
> > allow starting the IPA when starting the camera, instead of pre-starting
> > it early in order to process the lens shading table allocation event.
> 
> If the IPA is meant to be started at Camera::start() time, and we pass
> the lens shading table at ipa_->configure() time which happens before,
> what is different ?

Today the RPi IPA is started much earlier, to work around the lack of
API to pass custom configuration data. This change allows starting the
IPA at the right time.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
> >  3 files changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index a18ce9a884b6..c335d0259549 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -10,6 +10,10 @@
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >
> > +enum RPiConfigParameters {
> > +	RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> > +};
> > +
> 
> Is there any value in keeping this enum separated if not creating a
> distinction between operations run through processEvent() and
> operations run at configure() time ? Can't the list of operation be
> unique (which would also avoid calshing, if the same operations has
> to be handled both at processEvent() and configure() time.

They're different things though, so I think separate enums are best.
Ideally the IPAOperationData operation field should be an enum field of
the appropriate type. See the comment about IPC below.

> Also, why (1 << 0) in an enum ? Can't we start from 0 (or 1 as
> RPiOperations does) and increase ?

See below :-)

> >  enum RPiOperations {
> >  	RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
> >  	RPI_IPA_ACTION_V4L2_SET_ISP,
> > @@ -21,7 +25,6 @@ enum RPiOperations {
> >  	RPI_IPA_EVENT_SIGNAL_STAT_READY,
> >  	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> >  	RPI_IPA_EVENT_QUEUE_REQUEST,
> > -	RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
> >  };
> >
> >  enum RPiIpaMask {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 860be22ddb5d..c9f7dea375a5 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  		applyAGC(&agcStatus);
> >
> >  	lastMode_ = mode_;
> > +
> > +	/* Store the lens shading table pointer and handle if available. */
> > +	if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {
> 
> That's why (1 << 0). What is the advantage of using flags, instead of
> switching on the operation identifiers as done below in
> processEvent() ?

See the rest of the series. This is used to pass multiple configuration
parameters, while processEvents() and queueFrameAction only handle a
single operation. I'd like to make the two more similar though, but I
believe it will involve a rework of how we handle IPA-specific
parameters, including for IPC serialization, and possibly involve some
type of IDL. I'd thus rather not invest too much time now in something
that will very likely change.

> > +		lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> > +		lsTableHandle_ = ipaConfig.data[1];
> > +	}
> >  }
> >
> >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >  		break;
> >  	}
> >
> > -	case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> > -		lsTable_ = reinterpret_cast<void *>(event.data[0]);
> > -		lsTableHandle_ = event.data[1];
> > -		break;
> > -	}
> > -
> >  	default:
> >  		LOG(IPARPI, Error) << "Unknown event " << event.operation;
> >  		break;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0f9237a7f346..903796790f44 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
> >  {
> >  	std::map<unsigned int, IPAStream> streamConfig;
> >  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> > +	IPAOperationData ipaConfig = {};
> >
> >  	/* Get the device format to pass to the IPA. */
> >  	V4L2DeviceFormat sensorFormat;
> > @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
> >  		 * The vcsm allocation will always be in the memory region
> >  		 * < 32-bits to allow Videocore to access the memory.
> >  		 */
> > -		IPAOperationData op;
> > -		op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> > -		op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > -			    vcsm_.getVCHandle(lsTable_) };
> > -		ipa_->processEvent(op);
> > +		ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> > +		ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > +				   vcsm_.getVCHandle(lsTable_) };
> >  	}
> >
> >  	CameraSensorInfo sensorInfo = {};
> > @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
> >  	}
> >
> >  	/* Ready the IPA - it must know about the sensor resolution. */
> > -	IPAOperationData ipaConfig;
> >  	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> >  			nullptr);
> >
Laurent Pinchart July 3, 2020, 7:23 p.m. UTC | #6 
Hi Naush,

On Tue, Jun 30, 2020 at 11:38:21AM +0100, Naushir Patuck wrote:
> On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart wrote:
> >
> > The IPAInterface now accepts custom configuration data. Use it to pass
> > the lens shading table instead of using a custom IPA event. This will
> > allow starting the IPA when starting the camera, instead of pre-starting
> > it early in order to process the lens shading table allocation event.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
> >  3 files changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index a18ce9a884b6..c335d0259549 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -10,6 +10,10 @@
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >
> > +enum RPiConfigParameters {
> > +       RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> > +};
> 
> Minor thing, but could you rename this to RPI_IPA_CONFIG_LS_TABLE to
> be consistent with other labels?

Sure, will do.

> Otherwise all looks good.
> 
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> 
> > +
> >  enum RPiOperations {
> >         RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
> >         RPI_IPA_ACTION_V4L2_SET_ISP,
> > @@ -21,7 +25,6 @@ enum RPiOperations {
> >         RPI_IPA_EVENT_SIGNAL_STAT_READY,
> >         RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> >         RPI_IPA_EVENT_QUEUE_REQUEST,
> > -       RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
> >  };
> >
> >  enum RPiIpaMask {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 860be22ddb5d..c9f7dea375a5 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >                 applyAGC(&agcStatus);
> >
> >         lastMode_ = mode_;
> > +
> > +       /* Store the lens shading table pointer and handle if available. */
> > +       if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {
> > +               lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> > +               lsTableHandle_ = ipaConfig.data[1];
> > +       }
> >  }
> >
> >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >                 break;
> >         }
> >
> > -       case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> > -               lsTable_ = reinterpret_cast<void *>(event.data[0]);
> > -               lsTableHandle_ = event.data[1];
> > -               break;
> > -       }
> > -
> >         default:
> >                 LOG(IPARPI, Error) << "Unknown event " << event.operation;
> >                 break;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0f9237a7f346..903796790f44 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
> >  {
> >         std::map<unsigned int, IPAStream> streamConfig;
> >         std::map<unsigned int, const ControlInfoMap &> entityControls;
> > +       IPAOperationData ipaConfig = {};
> >
> >         /* Get the device format to pass to the IPA. */
> >         V4L2DeviceFormat sensorFormat;
> > @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
> >                  * The vcsm allocation will always be in the memory region
> >                  * < 32-bits to allow Videocore to access the memory.
> >                  */
> > -               IPAOperationData op;
> > -               op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> > -               op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > -                           vcsm_.getVCHandle(lsTable_) };
> > -               ipa_->processEvent(op);
> > +               ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> > +               ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > +                                  vcsm_.getVCHandle(lsTable_) };
> >         }
> >
> >         CameraSensorInfo sensorInfo = {};
> > @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
> >         }
> >
> >         /* Ready the IPA - it must know about the sensor resolution. */
> > -       IPAOperationData ipaConfig;
> >         ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> >                         nullptr);
> >

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index a18ce9a884b6..c335d0259549 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -10,6 +10,10 @@ 
 #include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
 
+enum RPiConfigParameters {
+	RPI_IPA_CONFIG_LSTABLE = (1 << 0),
+};
+
 enum RPiOperations {
 	RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
 	RPI_IPA_ACTION_V4L2_SET_ISP,
@@ -21,7 +25,6 @@  enum RPiOperations {
 	RPI_IPA_EVENT_SIGNAL_STAT_READY,
 	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
 	RPI_IPA_EVENT_QUEUE_REQUEST,
-	RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
 };
 
 enum RPiIpaMask {
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 860be22ddb5d..c9f7dea375a5 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -277,6 +277,12 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		applyAGC(&agcStatus);
 
 	lastMode_ = mode_;
+
+	/* Store the lens shading table pointer and handle if available. */
+	if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {
+		lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
+		lsTableHandle_ = ipaConfig.data[1];
+	}
 }
 
 void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
@@ -358,12 +364,6 @@  void IPARPi::processEvent(const IPAOperationData &event)
 		break;
 	}
 
-	case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
-		lsTable_ = reinterpret_cast<void *>(event.data[0]);
-		lsTableHandle_ = event.data[1];
-		break;
-	}
-
 	default:
 		LOG(IPARPI, Error) << "Unknown event " << event.operation;
 		break;
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 0f9237a7f346..903796790f44 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1109,6 +1109,7 @@  int RPiCameraData::configureIPA()
 {
 	std::map<unsigned int, IPAStream> streamConfig;
 	std::map<unsigned int, const ControlInfoMap &> entityControls;
+	IPAOperationData ipaConfig = {};
 
 	/* Get the device format to pass to the IPA. */
 	V4L2DeviceFormat sensorFormat;
@@ -1138,11 +1139,9 @@  int RPiCameraData::configureIPA()
 		 * The vcsm allocation will always be in the memory region
 		 * < 32-bits to allow Videocore to access the memory.
 		 */
-		IPAOperationData op;
-		op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
-		op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
-			    vcsm_.getVCHandle(lsTable_) };
-		ipa_->processEvent(op);
+		ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
+		ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
+				   vcsm_.getVCHandle(lsTable_) };
 	}
 
 	CameraSensorInfo sensorInfo = {};
@@ -1153,7 +1152,6 @@  int RPiCameraData::configureIPA()
 	}
 
 	/* Ready the IPA - it must know about the sensor resolution. */
-	IPAOperationData ipaConfig;
 	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
 			nullptr);