media: mali-c55: Fix frame sequence numbers on dual-pipe hardware
Subsystems (3)
PATCH 1/1 media: mali-c55: Fix frame sequence numbers on dual-pipe hardware
The single frame_sequence counter in struct mali_c55_isp is incremented in mali_c55_set_plane_done(), which runs once per completed buffer per capture device rather than once per frame. Hardware fitted with the downscale pipe always hits this: mali_c55_pipeline_ready() refuses to start the ISP unless both the full-resolution and the downscale queues are streaming, so the counter advances twice per frame.
Each video node then reports sequence numbers 0, 2, 4, ..., which userspace reads as a dropped frame between every pair of frames, and the two pipes never number the same frame alike, contrary to Documentation/admin-guide/media/mali-c55.rst. The V4L2_EVENT_FRAME_SYNC event and the statistics and parameters buffers only read the counter, so their sequence numbers stop identifying a frame too.
Increment the counter once per frame, when the ISP start interrupt is handled, and let mali_c55_set_plane_done() only read it. Initialise it to UINT_MAX so the first increment yields sequence zero. Hardware without the downscale pipe keeps the numbering it had before.
--- drivers/media/platform/arm/mali-c55/mali-c55-capture.c | 2 +- drivers/media/platform/arm/mali-c55/mali-c55-core.c | 1 + drivers/media/platform/arm/mali-c55/mali-c55-isp.c | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-)
drivers/media/platform/arm/mali-c55/mali-c55-capture.c +1 -1
@@ -476,7 +476,7 @@ void mali_c55_set_plane_done(struct mali_c55_cap_dev *cap_dev,/* If the other plane is also done... */ buf->vb.vb2_buf.timestamp =ktime_get_boottime_ns (); - buf->vb.sequence = isp->frame_sequence++; + buf->vb.sequence = isp->frame_sequence;vb2_buffer_done (&buf->vb.vb2_buf,VB2_BUF_STATE_DONE ); }
drivers/media/platform/arm/mali-c55/mali-c55-core.c +1 -0
@@ -573,6 +573,7 @@ static irqreturn_t mali_c55_isr(int irq, void *context)for_each_set_bit (i, &interrupt_status,MALI_C55_NUM_IRQ_BITS ) {switch (i) {case MALI_C55_IRQ_ISP_START : + mali_c55->isp.frame_sequence++;mali_c55_isp_queue_event_sof (mali_c55);mali_c55_set_next_buffer (&mali_c55->cap_devs[MALI_C55_CAP_DEV_FR ]);
drivers/media/platform/arm/mali-c55/mali-c55-isp.c +2 -1
@@ -342,7 +342,8 @@ static int mali_c55_isp_enable_streams(struct v4l2_subdev *sd, src_sd =media_entity_to_v4l2_subdev (isp->remote_src->entity); - isp->frame_sequence =0 ; +/* first pre-increment at SOF yields sequence 0 */ + isp->frame_sequence =UINT_MAX ; ret =mali_c55_isp_start (mali_c55, state);if (ret) {dev_err (mali_c55->dev,"Failed to start ISP\n" );
Signature · 1 line
2.53.0
Jacopo Mondi
maintainerHi David
Quoted 47 lines · David Carlier
The single
frame_sequencecounter in structmali_c55_ispis incremented inmali_c55_set_plane_done(), which runs once per completed buffer per capture device rather than once per frame. Hardware fitted with the downscale pipe always hits this:mali_c55_pipeline_ready()refuses to start the ISP unless both the full-resolution and the downscale queues are streaming, so the counter advances twice per frame.Each video node then reports sequence numbers 0, 2, 4, ..., which userspace reads as a dropped frame between every pair of frames, and the two pipes never number the same frame alike, contrary to
Documentation/admin-guide/media/mali-c55.rst. TheV4L2_EVENT_FRAME_SYNCevent and the statistics and parameters buffers only read the counter, so their sequence numbers stop identifying a frame too.Increment the counter once per frame, when the ISP start interrupt is handled, and let
mali_c55_set_plane_done()only read it. Initialise it toUINT_MAXso the first increment yields sequence zero. Hardware without the downscale pipe keeps the numbering it had before.Fixes d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")Assisted-by Claude:claude-opus-5Signed-off-by David Carlier <[email protected]>--- drivers/media/platform/arm/mali-c55/mali-c55-capture.c | 2 +- drivers/media/platform/arm/mali-c55/mali-c55-core.c | 1 + drivers/media/platform/arm/mali-c55/mali-c55-isp.c | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-)drivers/media/platform/arm/mali-c55/mali-c55-capture.c +1
-1@@ -476,7 +476,7 @@ void mali_c55_set_plane_done(struct mali_c55_cap_dev *cap_dev,/* If the other plane is also done... */ buf->vb.vb2_buf.timestamp =ktime_get_boottime_ns (); - buf->vb.sequence = isp->frame_sequence++; + buf->vb.sequence = isp->frame_sequence;vb2_buffer_done (&buf->vb.vb2_buf,VB2_BUF_STATE_DONE ); }drivers/media/platform/arm/mali-c55/mali-c55-core.c +0
-0
drivers/media/platform/arm/mali-c55/mali-c55-core.c static irqreturn_t mali_c55_isr(int irq, void *context) · line 576
Quoted 4 lines
drivers/media/platform/arm/mali-c55/mali-c55-core.c +1
-0@@ -573,6 +573,7 @@ static irqreturn_t mali_c55_isr(int irq, void *context)for_each_set_bit (i, &interrupt_status,MALI_C55_NUM_IRQ_BITS ) {switch (i) {case MALI_C55_IRQ_ISP_START : + mali_c55->isp.frame_sequence++;
ISP_DONE and ISP_START interrupts might be handled together, if the handling of DONE(n) interrupt is delayed enough to be handled at the same time as START(n+1).
I would rather handle this with per-ouput queue counters ?
Quoted 21 lines
drivers/media/platform/arm/mali-c55/mali-c55-core.c +0
-0mali_c55_isp_queue_event_sof (mali_c55);mali_c55_set_next_buffer (&mali_c55->cap_devs[MALI_C55_CAP_DEV_FR ]);drivers/media/platform/arm/mali-c55/mali-c55-isp.c +2
-2@@ -342,7 +342,8 @@ static int mali_c55_isp_enable_streams(struct v4l2_subdev *sd, src_sd =media_entity_to_v4l2_subdev (isp->remote_src->entity); - isp->frame_sequence =0 ; +/* first pre-increment at SOF yields sequence 0 */ + isp->frame_sequence =UINT_MAX ; ret =mali_c55_isp_start (mali_c55, state);if (ret) {dev_err (mali_c55->dev,"Failed to start ISP\n" ); --2.53.0
Hi Jacopo
Quoted 1 line · Jacopo Mondi
I would rather handle this with per-ouput queue counters ?
Right, START(n+1) is processed before the DONE(n) bits when both are pending.
A per-queue counter would not advance on underflow though, hiding the dropped frame. Would latching the sequence in mali_c55_set_next_buffer() work for you instead?
Cheers.