[libcamera-devel,v2,03/11] libcamera: media_device: Only read device information in populate()

Message ID 20190508151831.12274-4-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamerea: Add support for exclusive access to cameras between processes
Related show

Commit Message

Niklas Söderlund May 8, 2019, 3:18 p.m. UTC
There is no reason to reread the MEDIA_IOC_DEVICE_INFO information every
time the media device is opened. Move it populate() where it will be
read once together the other information about the media device.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/media_device.cpp | 51 +++++++++++++++-------------------
 1 file changed, 22 insertions(+), 29 deletions(-)

Comments

Laurent Pinchart May 11, 2019, 3:19 a.m. UTC | #1
Hi Niklas,

Thank you for the patch.

On Wed, May 08, 2019 at 05:18:23PM +0200, Niklas Söderlund wrote:
> There is no reason to reread the MEDIA_IOC_DEVICE_INFO information every
> time the media device is opened. Move it populate() where it will be
> read once together the other information about the media device.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/libcamera/media_device.cpp | 51 +++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 4b3b8f1fa3e6aaad..416a0d0c207ea72d 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -126,18 +126,11 @@ bool MediaDevice::acquire()
>   */
>  
>  /**
> - * \brief Open a media device and retrieve device information
> - *
> - * Before populating the media graph or performing any operation that interact
> - * with the device node associated with the media device, the device node must
> - * be opened.
> - *
> - * This function also retrieves media device information from the device node,
> - * which can be queried through driver().
> - *
> - * If the device is already open the function returns -EBUSY.
> + * \brief Open the media device
>   *
>   * \return 0 on success or a negative error code otherwise
> + * \retval -EBUSY Media device already open
> + * \sa close()
>   */
>  int MediaDevice::open()
>  {
> @@ -156,20 +149,6 @@ int MediaDevice::open()
>  	}
>  	fd_ = ret;
>  
> -	struct media_device_info info = { };
> -	ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> -	if (ret) {
> -		ret = -errno;
> -		LOG(MediaDevice, Error)
> -			<< "Failed to get media device info "
> -			<< ": " << strerror(-ret);
> -		return ret;
> -	}
> -
> -	driver_ = info.driver;
> -	model_ = info.model;
> -	version_ = info.media_version;
> -
>  	return 0;
>  }
>  
> @@ -196,12 +175,13 @@ void MediaDevice::close()
>  }
>  
>  /**
> - * \brief Populate the media graph with media objects
> + * \brief Populate the MediaDevice with device information and media objects
>   *
> - * This function enumerates all media objects in the media device graph and
> - * creates their MediaObject representations. All entities, pads and links are
> - * stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
> - * references between objects. Interfaces are not processed.
> + * This function retrieves the media device information and enumerates all
> + * media objects in the media device graph and creates their MediaObject
> + * representations. All entities, pads and links are stored as MediaEntity,
> + * MediaPad and MediaLink respectively, with cross-references between objects.
> + * Interfaces are not processed.
>   *
>   * Entities are stored in a separate list in the MediaDevice to ease lookup,
>   * while pads are accessible from the entity they belong to and links from the
> @@ -225,6 +205,19 @@ int MediaDevice::populate()
>  	if (ret)
>  		return ret;
>  
> +	struct media_device_info info = {};
> +	ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(MediaDevice, Error)
> +			<< "Failed to get media device info " << strerror(-ret);
> +		goto done;
> +	}
> +
> +	driver_ = info.driver;
> +	model_ = info.model;
> +	version_ = info.media_version;
> +
>  	/*
>  	 * Keep calling G_TOPOLOGY until the version number stays stable.
>  	 */
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch

diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
index 4b3b8f1fa3e6aaad..416a0d0c207ea72d 100644
--- a/src/libcamera/media_device.cpp
+++ b/src/libcamera/media_device.cpp
@@ -126,18 +126,11 @@  bool MediaDevice::acquire()
  */
 
 /**
- * \brief Open a media device and retrieve device information
- *
- * Before populating the media graph or performing any operation that interact
- * with the device node associated with the media device, the device node must
- * be opened.
- *
- * This function also retrieves media device information from the device node,
- * which can be queried through driver().
- *
- * If the device is already open the function returns -EBUSY.
+ * \brief Open the media device
  *
  * \return 0 on success or a negative error code otherwise
+ * \retval -EBUSY Media device already open
+ * \sa close()
  */
 int MediaDevice::open()
 {
@@ -156,20 +149,6 @@  int MediaDevice::open()
 	}
 	fd_ = ret;
 
-	struct media_device_info info = { };
-	ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
-	if (ret) {
-		ret = -errno;
-		LOG(MediaDevice, Error)
-			<< "Failed to get media device info "
-			<< ": " << strerror(-ret);
-		return ret;
-	}
-
-	driver_ = info.driver;
-	model_ = info.model;
-	version_ = info.media_version;
-
 	return 0;
 }
 
@@ -196,12 +175,13 @@  void MediaDevice::close()
 }
 
 /**
- * \brief Populate the media graph with media objects
+ * \brief Populate the MediaDevice with device information and media objects
  *
- * This function enumerates all media objects in the media device graph and
- * creates their MediaObject representations. All entities, pads and links are
- * stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
- * references between objects. Interfaces are not processed.
+ * This function retrieves the media device information and enumerates all
+ * media objects in the media device graph and creates their MediaObject
+ * representations. All entities, pads and links are stored as MediaEntity,
+ * MediaPad and MediaLink respectively, with cross-references between objects.
+ * Interfaces are not processed.
  *
  * Entities are stored in a separate list in the MediaDevice to ease lookup,
  * while pads are accessible from the entity they belong to and links from the
@@ -225,6 +205,19 @@  int MediaDevice::populate()
 	if (ret)
 		return ret;
 
+	struct media_device_info info = {};
+	ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
+	if (ret) {
+		ret = -errno;
+		LOG(MediaDevice, Error)
+			<< "Failed to get media device info " << strerror(-ret);
+		goto done;
+	}
+
+	driver_ = info.driver;
+	model_ = info.model;
+	version_ = info.media_version;
+
 	/*
 	 * Keep calling G_TOPOLOGY until the version number stays stable.
 	 */