Re: [PATCH 01/14] media: qcom: camss: Introduce CPU latency QoS during streaming

[Loic Poulain]

Hi Bryan,

On Thu, Oct 29, 2026 at 10:55 AM <bryan.odonoghue@xxxxxxxxxx> wrote:
>
> From: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>
> On a quiescent system - for example the X1 CRD booted to a simple ramdisk
> rootfs during development of V4L2_EVENT_FRAME_SYNC it was observed that
> interrupt for buffer done could be delayed until after the SOF for the next
> frame.
>
> Fiddling about with the interrupt bits, clearing them and changing the
> logic of V4L2_EVENT_FRAME_SYNC to buffer completion didn't work and besides
> "felt wrong".
>
> Tracing it looks like this:
> <idle>-0       [000] d.h1.    36.374380: csid_isr: rdi0 sof=1 done=0
> <idle>-0       [000] d.h1.    36.406601: csid_isr: rdi0 sof=1 done=1
> <idle>-0       [000] d.h1.    36.439910: csid_isr: rdi0 sof=1 done=1
> <idle>-0       [000] d.h1.    36.473245: csid_isr: rdi0 sof=1 done=1
> <idle>-0       [000] d.h1.    36.504786: csid_isr: rdi0 sof=0 done=1
> <idle>-0       [000] d.h1.    36.506350: csid_isr: rdi0 sof=1 done=0
>
> Tracing with /dev/cpu_dma_latency = 0 looks like this:
> <idle>-0       [000] d.h1.   556.149115: csid_isr: rdi0 sof=1 done=0
> <idle>-0       [000] d.h1.   556.181338: csid_isr: rdi0 sof=0 done=1
> <idle>-0       [000] d.h1.   556.182880: csid_isr: rdi0 sof=1 done=0
> <idle>-0       [000] d.h1.   556.214643: csid_isr: rdi0 sof=0 done=1
> <idle>-0       [000] d.h1.   556.216185: csid_isr: rdi0 sof=1 done=0
>
> The second is the pattern you expect CSI SOF followed by data.
>
> We will require SOF to track and understand which data-frame is correlated
> to which camera stastics and parameters frame. On an ordinary system with
> several things pinging off at once, WiFi, display, keyboard you will be
> unlikely to hit deep idle and hence unlikely to see the long exit. However
> we should design for the case where the system is otherwise quiescent with
> only the camera streaming.
>
> Choosing a request of 300us - 1000us seemed to work fine but cutting the
> margin finer, similar to the value on IPU6.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
>  drivers/media/platform/qcom/camss/camss-video.c |  4 ++++
>  drivers/media/platform/qcom/camss/camss.c       | 27 +++++++++++++++++++++++++
>  drivers/media/platform/qcom/camss/camss.h       |  7 +++++++
>  3 files changed, 38 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 0852eb6f13154..191cb71d02e19 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -286,6 +286,8 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>                         goto error;
>         }
>
> +       camss_cpu_latency_get(video->camss);
> +
>         return 0;
>
>  error:
> @@ -306,6 +308,8 @@ static void video_stop_streaming(struct vb2_queue *q)
>         struct v4l2_subdev *subdev;
>         int ret;
>
> +       camss_cpu_latency_put(video->camss);
> +
>         entity = &vdev->entity;
>         while (1) {
>                 pad = &entity->pads[0];
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c54185c93a8a9..06e1bf82f4836 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -5064,6 +5064,29 @@ void camss_buf_done(struct camss *camss, int hw_id, int port_id)
>         }
>  }
>
> +#define CAMSS_CPU_LATENCY_US   300
> +
> +void camss_cpu_latency_get(struct camss *camss)
> +{
> +       mutex_lock(&camss->cpu_latency_lock);
> +
> +       if (camss->cpu_latency_users++ == 0)
> +               cpu_latency_qos_add_request(&camss->cpu_latency,
> +                                           CAMSS_CPU_LATENCY_US);
> +
> +       mutex_unlock(&camss->cpu_latency_lock);

I agree that we need such a latency request mechanism, but the current
approach requires additional logic and locking.

What about one of the following alternatives instead?
- Move the latency request handling to the suspend/resume paths. The
request lifecycle would then be naturally serialized and balanced, and
in practice there are very few cases where the device is resumed but
not actively streaming.
- Alternatively, add a dedicated latency request instance to each VFE
and let each VFE manage its own request during start/stop. That would
keep the ownership local to the VFE and avoid the need for extra
locking altogether.

Regards,
Loic

> +}
> +
> +void camss_cpu_latency_put(struct camss *camss)
> +{
> +       mutex_lock(&camss->cpu_latency_lock);
> +
> +       if (--camss->cpu_latency_users == 0)
> +               cpu_latency_qos_remove_request(&camss->cpu_latency);
> +
> +       mutex_unlock(&camss->cpu_latency_lock);
> +}
> +
>  /*
>   * camss_register_entities - Register subdev nodes and create links
>   * @camss: CAMSS device
> @@ -5429,6 +5452,10 @@ static int camss_probe(struct platform_device *pdev)
>         camss->dev = dev;
>         platform_set_drvdata(pdev, camss);
>
> +       ret = devm_mutex_init(dev, &camss->cpu_latency_lock);
> +       if (ret)
> +               return ret;
> +
>         camss->csiphy = devm_kcalloc(dev, camss->res->csiphy_num,
>                                      sizeof(*camss->csiphy), GFP_KERNEL);
>         if (!camss->csiphy)
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 7dc2fd9f03e0d..9341fdc9bca0e 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -11,6 +11,8 @@
>  #define QC_MSM_CAMSS_H
>
>  #include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_qos.h>
>  #include <linux/types.h>
>  #include <media/v4l2-async.h>
>  #include <media/v4l2-device.h>
> @@ -141,6 +143,9 @@ struct camss {
>         struct icc_path *icc_path[ICC_SM8250_COUNT];
>         const struct camss_resources *res;
>         bool legacy_phy;
> +       struct mutex cpu_latency_lock;  /* Guards cpu_latency_users */
> +       unsigned int cpu_latency_users;
> +       struct pm_qos_request cpu_latency;
>  };
>
>  struct camss_camera_interface {
> @@ -182,5 +187,7 @@ void camss_delete(struct camss *camss);
>  void camss_buf_done(struct camss *camss, int hw_id, int port_id);
>  void camss_reg_update(struct camss *camss, int hw_id,
>                       int port_id, bool is_clear);
> +void camss_cpu_latency_get(struct camss *camss);
> +void camss_cpu_latency_put(struct camss *camss);
>
>  #endif /* QC_MSM_CAMSS_H */
>
> --
> 2.56.0
>