diff mbox series

media: qcom: camss: Power pipeline only when streaming

Message ID 20250526232837.686822-3-mailingradian@gmail.com
State New
Headers show
Series media: qcom: camss: Power pipeline only when streaming | expand

Commit Message

Richard Acayan May 26, 2025, 11:28 p.m. UTC 
The libcamera plugin for Pipewire may keep an open file descriptor to
the video device, even while streaming. This simplifies its operation,
as it only needs to keep track of a number instead of a file path. When
the video device is open but not streaming, the pipeline can be powered
off. Move the pipeline power management to the prepare_streaming and
unprepare_streaming functions.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 .../media/platform/qcom/camss/camss-video.c   | 39 ++++++++++++-------
 1 file changed, 25 insertions(+), 14 deletions(-)

Comments

Bryan O'Donoghue May 27, 2025, 2:37 p.m. UTC | #1 
On 27/05/2025 09:03, Loic Poulain wrote:
> On Tue, May 27, 2025 at 1:28 AM Richard Acayan<mailingradian@gmail.com> wrote:
>> The libcamera plugin for Pipewire may keep an open file descriptor to
>> the video device, even while streaming. This simplifies its operation,
>> as it only needs to keep track of a number instead of a file path. When
>> the video device is open but not streaming, the pipeline can be powered
>> off. Move the pipeline power management to the prepare_streaming and
>> unprepare_streaming functions.
> It seems to affect more than just this specific driver then? According
> to the documentation in v4l2-mc.h, v4l2_pipeline_pm_get() is intended
> to be called during video node open. If we're changing that behavior,
> we should also update the function's documentation accordingly so the
> change can be properly discussed and understood by a broader audience.

True but then the patch that added that comment also contains

@@ -999,7 +996,7 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, 
unsigned int count)
                 dev_err(cap->rkisp1->dev, "power up failed %d\n", ret);
                 goto err_destroy_dummy;
         }
-       ret = v4l2_pipeline_pm_use(entity, 1);
+       ret = v4l2_pipeline_pm_get(entity);

Seems to me like a perfectly valid use-case to have a persistent handle 
but only want to power-up the pipeline when you have work to-do.

---
bod
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index aa021fd5e123..8d05802d1735 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -225,6 +225,21 @@  static int video_check_format(struct camss_video *video)
 	return 0;
 }
 
+static int video_prepare_streaming(struct vb2_queue *q)
+{
+	struct camss_video *video = vb2_get_drv_priv(q);
+	struct video_device *vdev = &video->vdev;
+	int ret;
+
+	ret = v4l2_pipeline_pm_get(&vdev->entity);
+	if (ret < 0) {
+		dev_err(video->camss->dev, "Failed to power up pipeline: %d\n",
+			ret);
+	}
+
+	return ret;
+}
+
 static int video_start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct camss_video *video = vb2_get_drv_priv(q);
@@ -308,13 +323,23 @@  static void video_stop_streaming(struct vb2_queue *q)
 	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
 }
 
+static void video_unprepare_streaming(struct vb2_queue *q)
+{
+	struct camss_video *video = vb2_get_drv_priv(q);
+	struct video_device *vdev = &video->vdev;
+
+	v4l2_pipeline_pm_put(&vdev->entity);
+}
+
 static const struct vb2_ops msm_video_vb2_q_ops = {
 	.queue_setup     = video_queue_setup,
 	.buf_init        = video_buf_init,
 	.buf_prepare     = video_buf_prepare,
 	.buf_queue       = video_buf_queue,
+	.prepare_streaming = video_prepare_streaming,
 	.start_streaming = video_start_streaming,
 	.stop_streaming  = video_stop_streaming,
+	.unprepare_streaming = video_unprepare_streaming,
 };
 
 /* -----------------------------------------------------------------------------
@@ -599,20 +624,10 @@  static int video_open(struct file *file)
 
 	file->private_data = vfh;
 
-	ret = v4l2_pipeline_pm_get(&vdev->entity);
-	if (ret < 0) {
-		dev_err(video->camss->dev, "Failed to power up pipeline: %d\n",
-			ret);
-		goto error_pm_use;
-	}
-
 	mutex_unlock(&video->lock);
 
 	return 0;
 
-error_pm_use:
-	v4l2_fh_release(file);
-
 error_alloc:
 	mutex_unlock(&video->lock);
 
@@ -621,12 +636,8 @@  static int video_open(struct file *file)
 
 static int video_release(struct file *file)
 {
-	struct video_device *vdev = video_devdata(file);
-
 	vb2_fop_release(file);
 
-	v4l2_pipeline_pm_put(&vdev->entity);
-
 	file->private_data = NULL;
 
 	return 0;