| Message ID | 20220421143449.552312-5-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ti: cal: misc fixes | expand |
Hi Tomi, Thank you for the patch. On Thu, Apr 21, 2022 at 05:34:47PM +0300, Tomi Valkeinen wrote: > The userspace needs a way to match received metadata buffers to pixel > data buffers. The obvious way to do this is to use the CSI-2 frame > number, as both the metadata and the pixel data have the same frame > number as they come from the same frame. > > However, we don't have means to convey the frame number to userspace. We > do have the 'sequence' field, which with a few tricks can be used for > this purpose. > > To achieve this, track the frame number for each virtual channel and > increase the sequence for each virtual channel by frame-number - > previous-frame-number, also taking into account the eventual wrap of the > CSI-2 frame number. > > This way we get a monotonically increasing sequence number which is > common to all streams using the same virtual channel. I'd agree in principle, if it wasn't for the fact that sensors are not required to produce a frame number :-S Quoting CSI-2 v1.01.00: For FS and FE synchronization packets the Short Packet Data Field shall contain a 16-bit frame number. This frame number shall be the same for the FS and FE synchronization packets corresponding to a given frame. The 16-bit frame number, when used, shall be non-zero to distinguish it from the use-case where frame number is inoperative and remains set to zero. The behavior of the 16-bit frame number shall be as one of the following - Frame number is always zero – frame number is inoperative. - Frame number increments by 1 for every FS packet with the same Virtual Channel and is periodically reset to one e.g. 1, 2, 1, 2, 1, 2, 1, 2 or 1, 2, 3, 4, 1, 2, 3, 4 The frame number must be a non-zero value. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti/cal/cal-camerarx.c | 1 + > drivers/media/platform/ti/cal/cal.c | 57 +++++++++++++++++++- > drivers/media/platform/ti/cal/cal.h | 7 ++- > 3 files changed, 62 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c > index 34b8542133b6..96adbf7d8b65 100644 > --- a/drivers/media/platform/ti/cal/cal-camerarx.c > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c > @@ -844,6 +844,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, > phy->cal = cal; > phy->instance = instance; > > + spin_lock_init(&phy->vc_lock); > mutex_init(&phy->mutex); > > phy->res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c > index 4a4a6c5983f7..783ce5a8cd79 100644 > --- a/drivers/media/platform/ti/cal/cal.c > +++ b/drivers/media/platform/ti/cal/cal.c > @@ -496,7 +496,22 @@ void cal_ctx_unprepare(struct cal_ctx *ctx) > > void cal_ctx_start(struct cal_ctx *ctx) > { > - ctx->sequence = 0; > + struct cal_camerarx *phy = ctx->phy; > + > + /* > + * Reset the frame number & sequence number, but only if the > + * virtual channel is not already in use. > + */ > + > + spin_lock(&phy->vc_lock); > + > + if (phy->vc_enable_count[ctx->vc]++ == 0) { > + phy->vc_frame_number[ctx->vc] = 0; > + phy->vc_sequence[ctx->vc] = 0; > + } > + > + spin_unlock(&phy->vc_lock); > + > ctx->dma.state = CAL_DMA_RUNNING; > > /* Configure the CSI-2, pixel processing and write DMA contexts. */ > @@ -516,8 +531,15 @@ void cal_ctx_start(struct cal_ctx *ctx) > > void cal_ctx_stop(struct cal_ctx *ctx) > { > + struct cal_camerarx *phy = ctx->phy; > long timeout; > > + WARN_ON(phy->vc_enable_count[ctx->vc] == 0); > + > + spin_lock(&phy->vc_lock); > + phy->vc_enable_count[ctx->vc]--; > + spin_unlock(&phy->vc_lock); > + > /* > * Request DMA stop and wait until it completes. If completion times > * out, forcefully disable the DMA. > @@ -554,6 +576,34 @@ void cal_ctx_stop(struct cal_ctx *ctx) > * ------------------------------------------------------------------ > */ > > +/* > + * Track a sequence number for each virtual channel, which is shared by > + * all contexts using the same virtual channel. This is done using the > + * CSI-2 frame number as a base. > + */ > +static void cal_update_seq_number(struct cal_ctx *ctx) > +{ > + struct cal_dev *cal = ctx->cal; > + struct cal_camerarx *phy = ctx->phy; > + u16 prev_frame_num, frame_num; > + u8 vc = ctx->vc; > + > + frame_num = > + cal_read(cal, CAL_CSI2_STATUS(phy->instance, ctx->csi2_ctx)) & > + 0xffff; > + > + if (phy->vc_frame_number[vc] != frame_num) { > + prev_frame_num = phy->vc_frame_number[vc]; > + > + if (prev_frame_num >= frame_num) > + phy->vc_sequence[vc] += 1; > + else > + phy->vc_sequence[vc] += frame_num - prev_frame_num; > + > + phy->vc_frame_number[vc] = frame_num; > + } > +} > + > static inline void cal_irq_wdma_start(struct cal_ctx *ctx) > { > spin_lock(&ctx->dma.lock); > @@ -584,6 +634,8 @@ static inline void cal_irq_wdma_start(struct cal_ctx *ctx) > } > > spin_unlock(&ctx->dma.lock); > + > + cal_update_seq_number(ctx); > } > > static inline void cal_irq_wdma_end(struct cal_ctx *ctx) > @@ -610,7 +662,8 @@ static inline void cal_irq_wdma_end(struct cal_ctx *ctx) > if (buf) { > buf->vb.vb2_buf.timestamp = ktime_get_ns(); > buf->vb.field = ctx->v_fmt.fmt.pix.field; > - buf->vb.sequence = ctx->sequence++; > + buf->vb.sequence = ctx->phy->vc_sequence[ctx->vc]; > + > vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE); > } > } > diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h > index 527e22d022f3..dfb628cd3bdd 100644 > --- a/drivers/media/platform/ti/cal/cal.h > +++ b/drivers/media/platform/ti/cal/cal.h > @@ -180,6 +180,12 @@ struct cal_camerarx { > struct media_pad pads[CAL_CAMERARX_NUM_PADS]; > struct v4l2_mbus_framefmt formats[CAL_CAMERARX_NUM_PADS]; > > + /* protects the vc_* fields below */ > + spinlock_t vc_lock; > + u8 vc_enable_count[4]; > + u16 vc_frame_number[4]; > + u32 vc_sequence[4]; > + > /* > * Lock for camerarx ops. Protects: > * - formats > @@ -242,7 +248,6 @@ struct cal_ctx { > const struct cal_format_info **active_fmt; > unsigned int num_active_fmt; > > - unsigned int sequence; > struct vb2_queue vb_vidq; > u8 dma_ctx; > u8 cport;
On 22/04/2022 02:52, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Thu, Apr 21, 2022 at 05:34:47PM +0300, Tomi Valkeinen wrote: >> The userspace needs a way to match received metadata buffers to pixel >> data buffers. The obvious way to do this is to use the CSI-2 frame >> number, as both the metadata and the pixel data have the same frame >> number as they come from the same frame. >> >> However, we don't have means to convey the frame number to userspace. We >> do have the 'sequence' field, which with a few tricks can be used for >> this purpose. >> >> To achieve this, track the frame number for each virtual channel and >> increase the sequence for each virtual channel by frame-number - >> previous-frame-number, also taking into account the eventual wrap of the >> CSI-2 frame number. >> >> This way we get a monotonically increasing sequence number which is >> common to all streams using the same virtual channel. > > I'd agree in principle, if it wasn't for the fact that sensors are not > required to produce a frame number :-S In that case the CAL hardware will increment the register every frame. From CAL doc: Frame number. Matches the frame number sent by the camera when the camera transmits it. Otherwise, incremented by one on every FS short packet for this context. Reset when the context is enabled. I'll add a note about that to the desc. Tomi
Hi Tomi, On Fri, Apr 22, 2022 at 09:41:51AM +0300, Tomi Valkeinen wrote: > On 22/04/2022 02:52, Laurent Pinchart wrote: > > On Thu, Apr 21, 2022 at 05:34:47PM +0300, Tomi Valkeinen wrote: > >> The userspace needs a way to match received metadata buffers to pixel > >> data buffers. The obvious way to do this is to use the CSI-2 frame > >> number, as both the metadata and the pixel data have the same frame > >> number as they come from the same frame. > >> > >> However, we don't have means to convey the frame number to userspace. We > >> do have the 'sequence' field, which with a few tricks can be used for > >> this purpose. > >> > >> To achieve this, track the frame number for each virtual channel and > >> increase the sequence for each virtual channel by frame-number - > >> previous-frame-number, also taking into account the eventual wrap of the > >> CSI-2 frame number. > >> > >> This way we get a monotonically increasing sequence number which is > >> common to all streams using the same virtual channel. > > > > I'd agree in principle, if it wasn't for the fact that sensors are not > > required to produce a frame number :-S > > In that case the CAL hardware will increment the register every frame. > From CAL doc: > > Frame number. > Matches the frame number sent by the camera when the > camera transmits it. > Otherwise, incremented by one on every FS short packet > for this context. Is that only when the FS packet contains a frame number equal to 0 ? How about the extreme case where the frame number counts up to 2 and resets to 1, effectively toggling between 1 and 2 ? Does your patch support this correctly ? > Reset when the context is enabled. > > I'll add a note about that to the desc.
On 22/04/2022 19:53, Laurent Pinchart wrote: > Hi Tomi, > > On Fri, Apr 22, 2022 at 09:41:51AM +0300, Tomi Valkeinen wrote: >> On 22/04/2022 02:52, Laurent Pinchart wrote: >>> On Thu, Apr 21, 2022 at 05:34:47PM +0300, Tomi Valkeinen wrote: >>>> The userspace needs a way to match received metadata buffers to pixel >>>> data buffers. The obvious way to do this is to use the CSI-2 frame >>>> number, as both the metadata and the pixel data have the same frame >>>> number as they come from the same frame. >>>> >>>> However, we don't have means to convey the frame number to userspace. We >>>> do have the 'sequence' field, which with a few tricks can be used for >>>> this purpose. >>>> >>>> To achieve this, track the frame number for each virtual channel and >>>> increase the sequence for each virtual channel by frame-number - >>>> previous-frame-number, also taking into account the eventual wrap of the >>>> CSI-2 frame number. >>>> >>>> This way we get a monotonically increasing sequence number which is >>>> common to all streams using the same virtual channel. >>> >>> I'd agree in principle, if it wasn't for the fact that sensors are not >>> required to produce a frame number :-S >> >> In that case the CAL hardware will increment the register every frame. >> From CAL doc: >> >> Frame number. >> Matches the frame number sent by the camera when the >> camera transmits it. >> Otherwise, incremented by one on every FS short packet >> for this context. > > Is that only when the FS packet contains a frame number equal to 0 ? How I think so, although the doc doesn't explicitly say it. > about the extreme case where the frame number counts up to 2 and resets > to 1, effectively toggling between 1 and 2 ? Does your patch support > this correctly ? I think so, but I didn't try that one. I do have a setup where the counts go up to 4 (or was it 3), and it seems to work fine. The code increments the seq number by new_frame_number - prev_frame_number, except when the frame number has wrapped, in which case the seq number is incremented by 1. Tomi
diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c index 34b8542133b6..96adbf7d8b65 100644 --- a/drivers/media/platform/ti/cal/cal-camerarx.c +++ b/drivers/media/platform/ti/cal/cal-camerarx.c @@ -844,6 +844,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, phy->cal = cal; phy->instance = instance; + spin_lock_init(&phy->vc_lock); mutex_init(&phy->mutex); phy->res = platform_get_resource_byname(pdev, IORESOURCE_MEM, diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c index 4a4a6c5983f7..783ce5a8cd79 100644 --- a/drivers/media/platform/ti/cal/cal.c +++ b/drivers/media/platform/ti/cal/cal.c @@ -496,7 +496,22 @@ void cal_ctx_unprepare(struct cal_ctx *ctx) void cal_ctx_start(struct cal_ctx *ctx) { - ctx->sequence = 0; + struct cal_camerarx *phy = ctx->phy; + + /* + * Reset the frame number & sequence number, but only if the + * virtual channel is not already in use. + */ + + spin_lock(&phy->vc_lock); + + if (phy->vc_enable_count[ctx->vc]++ == 0) { + phy->vc_frame_number[ctx->vc] = 0; + phy->vc_sequence[ctx->vc] = 0; + } + + spin_unlock(&phy->vc_lock); + ctx->dma.state = CAL_DMA_RUNNING; /* Configure the CSI-2, pixel processing and write DMA contexts. */ @@ -516,8 +531,15 @@ void cal_ctx_start(struct cal_ctx *ctx) void cal_ctx_stop(struct cal_ctx *ctx) { + struct cal_camerarx *phy = ctx->phy; long timeout; + WARN_ON(phy->vc_enable_count[ctx->vc] == 0); + + spin_lock(&phy->vc_lock); + phy->vc_enable_count[ctx->vc]--; + spin_unlock(&phy->vc_lock); + /* * Request DMA stop and wait until it completes. If completion times * out, forcefully disable the DMA. @@ -554,6 +576,34 @@ void cal_ctx_stop(struct cal_ctx *ctx) * ------------------------------------------------------------------ */ +/* + * Track a sequence number for each virtual channel, which is shared by + * all contexts using the same virtual channel. This is done using the + * CSI-2 frame number as a base. + */ +static void cal_update_seq_number(struct cal_ctx *ctx) +{ + struct cal_dev *cal = ctx->cal; + struct cal_camerarx *phy = ctx->phy; + u16 prev_frame_num, frame_num; + u8 vc = ctx->vc; + + frame_num = + cal_read(cal, CAL_CSI2_STATUS(phy->instance, ctx->csi2_ctx)) & + 0xffff; + + if (phy->vc_frame_number[vc] != frame_num) { + prev_frame_num = phy->vc_frame_number[vc]; + + if (prev_frame_num >= frame_num) + phy->vc_sequence[vc] += 1; + else + phy->vc_sequence[vc] += frame_num - prev_frame_num; + + phy->vc_frame_number[vc] = frame_num; + } +} + static inline void cal_irq_wdma_start(struct cal_ctx *ctx) { spin_lock(&ctx->dma.lock); @@ -584,6 +634,8 @@ static inline void cal_irq_wdma_start(struct cal_ctx *ctx) } spin_unlock(&ctx->dma.lock); + + cal_update_seq_number(ctx); } static inline void cal_irq_wdma_end(struct cal_ctx *ctx) @@ -610,7 +662,8 @@ static inline void cal_irq_wdma_end(struct cal_ctx *ctx) if (buf) { buf->vb.vb2_buf.timestamp = ktime_get_ns(); buf->vb.field = ctx->v_fmt.fmt.pix.field; - buf->vb.sequence = ctx->sequence++; + buf->vb.sequence = ctx->phy->vc_sequence[ctx->vc]; + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE); } } diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h index 527e22d022f3..dfb628cd3bdd 100644 --- a/drivers/media/platform/ti/cal/cal.h +++ b/drivers/media/platform/ti/cal/cal.h @@ -180,6 +180,12 @@ struct cal_camerarx { struct media_pad pads[CAL_CAMERARX_NUM_PADS]; struct v4l2_mbus_framefmt formats[CAL_CAMERARX_NUM_PADS]; + /* protects the vc_* fields below */ + spinlock_t vc_lock; + u8 vc_enable_count[4]; + u16 vc_frame_number[4]; + u32 vc_sequence[4]; + /* * Lock for camerarx ops. Protects: * - formats @@ -242,7 +248,6 @@ struct cal_ctx { const struct cal_format_info **active_fmt; unsigned int num_active_fmt; - unsigned int sequence; struct vb2_queue vb_vidq; u8 dma_ctx; u8 cport;
The userspace needs a way to match received metadata buffers to pixel data buffers. The obvious way to do this is to use the CSI-2 frame number, as both the metadata and the pixel data have the same frame number as they come from the same frame. However, we don't have means to convey the frame number to userspace. We do have the 'sequence' field, which with a few tricks can be used for this purpose. To achieve this, track the frame number for each virtual channel and increase the sequence for each virtual channel by frame-number - previous-frame-number, also taking into account the eventual wrap of the CSI-2 frame number. This way we get a monotonically increasing sequence number which is common to all streams using the same virtual channel. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti/cal/cal-camerarx.c | 1 + drivers/media/platform/ti/cal/cal.c | 57 +++++++++++++++++++- drivers/media/platform/ti/cal/cal.h | 7 ++- 3 files changed, 62 insertions(+), 3 deletions(-)