| Message ID | 20210524110909.672432-38-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: ti-vpe: cal: multistream & embedded data support | expand |
Hi Tomi, Thank you for the patch. On Mon, May 24, 2021 at 02:09:08PM +0300, Tomi Valkeinen wrote: > Use get_frame_desc() to get the frame desc from the connected source, > and use the provided virtual channel and datatype instead of hardcoded > ones. > > get_frame_desc() works per stream, but as we don't support multiple > streams yet, we will just always use stream 0. > > If the source doesn't support get_frame_desc(), fall back to the > previous method of always capturing virtual channel 0 and any datatype. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-camerarx.c | 26 +++++++++++ > drivers/media/platform/ti-vpe/cal.c | 49 +++++++++++++++++++- > drivers/media/platform/ti-vpe/cal.h | 3 ++ > 3 files changed, 76 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index e3a4c20be1e6..cb6a37f47432 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -583,6 +583,32 @@ static int cal_camerarx_parse_dt(struct cal_camerarx *phy) > return ret; > } > > +int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy, > + struct v4l2_mbus_frame_desc *fd) Maybe s/fd/desc/ to avoid the confusion with file descriptor ? > +{ > + struct media_pad *pad; > + int ret; > + > + if (!phy->source) > + return -ENODEV; Would EPIPE (here and below) be a better error ? It will be returned to userspace from VIDIOC_STREAMON(), which already uses EPIPE to indicate that the links are not correctly set up. > + > + pad = media_entity_remote_pad(&phy->pads[CAL_CAMERARX_PAD_SINK]); > + if (!pad) > + return -ENODEV; > + > + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index, > + fd); > + if (ret) > + return ret; > + > + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > + dev_err(phy->cal->dev, "Frame desc do not describe CSI-2 link"); s/do not/does not/ and maybe s/desc/descriptor/ > + return -EINVAL; > + } > + > + return 0; > +} > + > /* ------------------------------------------------------------------ > * V4L2 Subdev Operations > * ------------------------------------------------------------------ > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index fcc81024ae18..7975bb449acd 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -469,10 +469,56 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx) > return stopped; > } > > +static int > +cal_get_remote_frame_desc_entry(struct cal_camerarx *phy, u32 stream, > + struct v4l2_mbus_frame_desc_entry *entry) > +{ > + struct v4l2_mbus_frame_desc fd; > + unsigned int i; > + int ret; > + > + ret = cal_camerarx_get_remote_frame_desc(phy, &fd); > + if (ret) { > + if (ret != -ENOIOCTLCMD) > + dev_err(phy->cal->dev, > + "Failed to get remote frame desc: %d\n", ret); > + return ret; > + } > + > + for (i = 0; i < fd.num_entries; i++) { > + if (stream == fd.entry[i].stream) { > + *entry = fd.entry[i]; > + return 0; > + } > + } > + A dev_dbg() would be good here, in which case you could drop the ctx_err() in cal_ctx_prepare() as all error paths will print a message. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + return -ENODEV; > +} > + > int cal_ctx_prepare(struct cal_ctx *ctx) > { > + struct v4l2_mbus_frame_desc_entry entry; > int ret; > > + ret = cal_get_remote_frame_desc_entry(ctx->phy, ctx->stream, &entry); > + > + if (ret == -ENOIOCTLCMD) { > + ctx->vc = 0; > + ctx->datatype = CAL_CSI2_CTX_DT_ANY; > + } else if (!ret) { > + ctx_dbg(2, ctx, "Framedesc: stream %u, len %u, vc %u, dt %#x\n", > + entry.stream, > + entry.length, > + entry.bus.csi2.vc, > + entry.bus.csi2.dt); You can group multiple variables on the same line. > + > + ctx->vc = entry.bus.csi2.vc; > + ctx->datatype = entry.bus.csi2.dt; > + } else { > + ctx_err(ctx, "Failed to get remote frame desc: %d\n", ret); > + return ret; > + } > + > ctx->use_pix_proc = !ctx->fmtinfo->meta; > > if (ctx->use_pix_proc) { > @@ -925,8 +971,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) > ctx->dma_ctx = inst; > ctx->csi2_ctx = inst; > ctx->cport = inst; > - ctx->vc = 0; > - ctx->datatype = CAL_CSI2_CTX_DT_ANY; > + ctx->stream = 0; > > ret = cal_ctx_v4l2_init(ctx); > if (ret) > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h > index 29b865d1a238..3aea444f8bf8 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -245,6 +245,7 @@ struct cal_ctx { > u8 pix_proc; > u8 vc; > u8 datatype; > + u32 stream; > > bool use_pix_proc; > }; > @@ -318,6 +319,8 @@ const struct cal_format_info *cal_format_by_code(u32 code); > > void cal_quickdump_regs(struct cal_dev *cal); > > +int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy, > + struct v4l2_mbus_frame_desc *fd); > void cal_camerarx_disable(struct cal_camerarx *phy); > void cal_camerarx_i913_errata(struct cal_camerarx *phy); > struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, -- Regards, Laurent Pinchart
On 04/06/2021 17:25, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, May 24, 2021 at 02:09:08PM +0300, Tomi Valkeinen wrote: >> Use get_frame_desc() to get the frame desc from the connected source, >> and use the provided virtual channel and datatype instead of hardcoded >> ones. >> >> get_frame_desc() works per stream, but as we don't support multiple >> streams yet, we will just always use stream 0. >> >> If the source doesn't support get_frame_desc(), fall back to the >> previous method of always capturing virtual channel 0 and any datatype. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal-camerarx.c | 26 +++++++++++ >> drivers/media/platform/ti-vpe/cal.c | 49 +++++++++++++++++++- >> drivers/media/platform/ti-vpe/cal.h | 3 ++ >> 3 files changed, 76 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c >> index e3a4c20be1e6..cb6a37f47432 100644 >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c >> @@ -583,6 +583,32 @@ static int cal_camerarx_parse_dt(struct cal_camerarx *phy) >> return ret; >> } >> >> +int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy, >> + struct v4l2_mbus_frame_desc *fd) > > Maybe s/fd/desc/ to avoid the confusion with file descriptor ? Good point. >> +{ >> + struct media_pad *pad; >> + int ret; >> + >> + if (!phy->source) >> + return -ENODEV; > > Would EPIPE (here and below) be a better error ? It will be returned to > userspace from VIDIOC_STREAMON(), which already uses EPIPE to indicate > that the links are not correctly set up. Sound fine to me. >> + >> + pad = media_entity_remote_pad(&phy->pads[CAL_CAMERARX_PAD_SINK]); >> + if (!pad) >> + return -ENODEV; >> + >> + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index, >> + fd); >> + if (ret) >> + return ret; >> + >> + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { >> + dev_err(phy->cal->dev, "Frame desc do not describe CSI-2 link"); > > s/do not/does not/ and maybe s/desc/descriptor/ Yep. >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> /* ------------------------------------------------------------------ >> * V4L2 Subdev Operations >> * ------------------------------------------------------------------ >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c >> index fcc81024ae18..7975bb449acd 100644 >> --- a/drivers/media/platform/ti-vpe/cal.c >> +++ b/drivers/media/platform/ti-vpe/cal.c >> @@ -469,10 +469,56 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx) >> return stopped; >> } >> >> +static int >> +cal_get_remote_frame_desc_entry(struct cal_camerarx *phy, u32 stream, >> + struct v4l2_mbus_frame_desc_entry *entry) >> +{ >> + struct v4l2_mbus_frame_desc fd; >> + unsigned int i; >> + int ret; >> + >> + ret = cal_camerarx_get_remote_frame_desc(phy, &fd); >> + if (ret) { >> + if (ret != -ENOIOCTLCMD) >> + dev_err(phy->cal->dev, >> + "Failed to get remote frame desc: %d\n", ret); >> + return ret; >> + } >> + >> + for (i = 0; i < fd.num_entries; i++) { >> + if (stream == fd.entry[i].stream) { >> + *entry = fd.entry[i]; >> + return 0; >> + } >> + } >> + > > A dev_dbg() would be good here, in which case you could drop the > ctx_err() in cal_ctx_prepare() as all error paths will print a message. Hmm, yes, or maybe the other way around: remove the dev_err above, and let the cal_ctx_prepare handle the printing (as it already does). But perhaps it's better to add a dev_err (did you mean dev_err, when you said dev_dbg?) above before the return -ENODEV to make it clear if the error is coming from get_remote_frame_desc or from trying to find the stream. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + return -ENODEV; >> +} >> + >> int cal_ctx_prepare(struct cal_ctx *ctx) >> { >> + struct v4l2_mbus_frame_desc_entry entry; >> int ret; >> >> + ret = cal_get_remote_frame_desc_entry(ctx->phy, ctx->stream, &entry); >> + >> + if (ret == -ENOIOCTLCMD) { >> + ctx->vc = 0; >> + ctx->datatype = CAL_CSI2_CTX_DT_ANY; >> + } else if (!ret) { >> + ctx_dbg(2, ctx, "Framedesc: stream %u, len %u, vc %u, dt %#x\n", >> + entry.stream, >> + entry.length, >> + entry.bus.csi2.vc, >> + entry.bus.csi2.dt); > > You can group multiple variables on the same line. Yep. Thanks! Tomi
Hi Tomi, On Mon, Jun 07, 2021 at 03:07:42PM +0300, Tomi Valkeinen wrote: > On 04/06/2021 17:25, Laurent Pinchart wrote: > > On Mon, May 24, 2021 at 02:09:08PM +0300, Tomi Valkeinen wrote: > >> Use get_frame_desc() to get the frame desc from the connected source, > >> and use the provided virtual channel and datatype instead of hardcoded > >> ones. > >> > >> get_frame_desc() works per stream, but as we don't support multiple > >> streams yet, we will just always use stream 0. > >> > >> If the source doesn't support get_frame_desc(), fall back to the > >> previous method of always capturing virtual channel 0 and any datatype. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> drivers/media/platform/ti-vpe/cal-camerarx.c | 26 +++++++++++ > >> drivers/media/platform/ti-vpe/cal.c | 49 +++++++++++++++++++- > >> drivers/media/platform/ti-vpe/cal.h | 3 ++ > >> 3 files changed, 76 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > >> index e3a4c20be1e6..cb6a37f47432 100644 > >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > >> @@ -583,6 +583,32 @@ static int cal_camerarx_parse_dt(struct cal_camerarx *phy) > >> return ret; > >> } > >> > >> +int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy, > >> + struct v4l2_mbus_frame_desc *fd) > > > > Maybe s/fd/desc/ to avoid the confusion with file descriptor ? > > Good point. > > >> +{ > >> + struct media_pad *pad; > >> + int ret; > >> + > >> + if (!phy->source) > >> + return -ENODEV; > > > > Would EPIPE (here and below) be a better error ? It will be returned to > > userspace from VIDIOC_STREAMON(), which already uses EPIPE to indicate > > that the links are not correctly set up. > > Sound fine to me. > > >> + > >> + pad = media_entity_remote_pad(&phy->pads[CAL_CAMERARX_PAD_SINK]); > >> + if (!pad) > >> + return -ENODEV; > >> + > >> + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index, > >> + fd); > >> + if (ret) > >> + return ret; > >> + > >> + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > >> + dev_err(phy->cal->dev, "Frame desc do not describe CSI-2 link"); > > > > s/do not/does not/ and maybe s/desc/descriptor/ > > Yep. > > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> /* ------------------------------------------------------------------ > >> * V4L2 Subdev Operations > >> * ------------------------------------------------------------------ > >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > >> index fcc81024ae18..7975bb449acd 100644 > >> --- a/drivers/media/platform/ti-vpe/cal.c > >> +++ b/drivers/media/platform/ti-vpe/cal.c > >> @@ -469,10 +469,56 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx) > >> return stopped; > >> } > >> > >> +static int > >> +cal_get_remote_frame_desc_entry(struct cal_camerarx *phy, u32 stream, > >> + struct v4l2_mbus_frame_desc_entry *entry) > >> +{ > >> + struct v4l2_mbus_frame_desc fd; > >> + unsigned int i; > >> + int ret; > >> + > >> + ret = cal_camerarx_get_remote_frame_desc(phy, &fd); > >> + if (ret) { > >> + if (ret != -ENOIOCTLCMD) > >> + dev_err(phy->cal->dev, > >> + "Failed to get remote frame desc: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + for (i = 0; i < fd.num_entries; i++) { > >> + if (stream == fd.entry[i].stream) { > >> + *entry = fd.entry[i]; > >> + return 0; > >> + } > >> + } > >> + > > > > A dev_dbg() would be good here, in which case you could drop the > > ctx_err() in cal_ctx_prepare() as all error paths will print a message. > > Hmm, yes, or maybe the other way around: remove the dev_err above, and > let the cal_ctx_prepare handle the printing (as it already does). > > But perhaps it's better to add a dev_err (did you mean dev_err, when you > said dev_dbg?) above before the return -ENODEV to make it clear if the > error is coming from get_remote_frame_desc or from trying to find the > stream. I usually prefer debug messages for anything that can be triggered by userspace and isn't a kernel error. Not sure which one it is here :-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + return -ENODEV; > >> +} > >> + > >> int cal_ctx_prepare(struct cal_ctx *ctx) > >> { > >> + struct v4l2_mbus_frame_desc_entry entry; > >> int ret; > >> > >> + ret = cal_get_remote_frame_desc_entry(ctx->phy, ctx->stream, &entry); > >> + > >> + if (ret == -ENOIOCTLCMD) { > >> + ctx->vc = 0; > >> + ctx->datatype = CAL_CSI2_CTX_DT_ANY; > >> + } else if (!ret) { > >> + ctx_dbg(2, ctx, "Framedesc: stream %u, len %u, vc %u, dt %#x\n", > >> + entry.stream, > >> + entry.length, > >> + entry.bus.csi2.vc, > >> + entry.bus.csi2.dt); > > > > You can group multiple variables on the same line. > > Yep. -- Regards, Laurent Pinchart
diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c index e3a4c20be1e6..cb6a37f47432 100644 --- a/drivers/media/platform/ti-vpe/cal-camerarx.c +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c @@ -583,6 +583,32 @@ static int cal_camerarx_parse_dt(struct cal_camerarx *phy) return ret; } +int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy, + struct v4l2_mbus_frame_desc *fd) +{ + struct media_pad *pad; + int ret; + + if (!phy->source) + return -ENODEV; + + pad = media_entity_remote_pad(&phy->pads[CAL_CAMERARX_PAD_SINK]); + if (!pad) + return -ENODEV; + + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index, + fd); + if (ret) + return ret; + + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { + dev_err(phy->cal->dev, "Frame desc do not describe CSI-2 link"); + return -EINVAL; + } + + return 0; +} + /* ------------------------------------------------------------------ * V4L2 Subdev Operations * ------------------------------------------------------------------ diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index fcc81024ae18..7975bb449acd 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -469,10 +469,56 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx) return stopped; } +static int +cal_get_remote_frame_desc_entry(struct cal_camerarx *phy, u32 stream, + struct v4l2_mbus_frame_desc_entry *entry) +{ + struct v4l2_mbus_frame_desc fd; + unsigned int i; + int ret; + + ret = cal_camerarx_get_remote_frame_desc(phy, &fd); + if (ret) { + if (ret != -ENOIOCTLCMD) + dev_err(phy->cal->dev, + "Failed to get remote frame desc: %d\n", ret); + return ret; + } + + for (i = 0; i < fd.num_entries; i++) { + if (stream == fd.entry[i].stream) { + *entry = fd.entry[i]; + return 0; + } + } + + return -ENODEV; +} + int cal_ctx_prepare(struct cal_ctx *ctx) { + struct v4l2_mbus_frame_desc_entry entry; int ret; + ret = cal_get_remote_frame_desc_entry(ctx->phy, ctx->stream, &entry); + + if (ret == -ENOIOCTLCMD) { + ctx->vc = 0; + ctx->datatype = CAL_CSI2_CTX_DT_ANY; + } else if (!ret) { + ctx_dbg(2, ctx, "Framedesc: stream %u, len %u, vc %u, dt %#x\n", + entry.stream, + entry.length, + entry.bus.csi2.vc, + entry.bus.csi2.dt); + + ctx->vc = entry.bus.csi2.vc; + ctx->datatype = entry.bus.csi2.dt; + } else { + ctx_err(ctx, "Failed to get remote frame desc: %d\n", ret); + return ret; + } + ctx->use_pix_proc = !ctx->fmtinfo->meta; if (ctx->use_pix_proc) { @@ -925,8 +971,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) ctx->dma_ctx = inst; ctx->csi2_ctx = inst; ctx->cport = inst; - ctx->vc = 0; - ctx->datatype = CAL_CSI2_CTX_DT_ANY; + ctx->stream = 0; ret = cal_ctx_v4l2_init(ctx); if (ret) diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h index 29b865d1a238..3aea444f8bf8 100644 --- a/drivers/media/platform/ti-vpe/cal.h +++ b/drivers/media/platform/ti-vpe/cal.h @@ -245,6 +245,7 @@ struct cal_ctx { u8 pix_proc; u8 vc; u8 datatype; + u32 stream; bool use_pix_proc; }; @@ -318,6 +319,8 @@ const struct cal_format_info *cal_format_by_code(u32 code); void cal_quickdump_regs(struct cal_dev *cal); +int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy, + struct v4l2_mbus_frame_desc *fd); void cal_camerarx_disable(struct cal_camerarx *phy); void cal_camerarx_i913_errata(struct cal_camerarx *phy); struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
Use get_frame_desc() to get the frame desc from the connected source, and use the provided virtual channel and datatype instead of hardcoded ones. get_frame_desc() works per stream, but as we don't support multiple streams yet, we will just always use stream 0. If the source doesn't support get_frame_desc(), fall back to the previous method of always capturing virtual channel 0 and any datatype. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal-camerarx.c | 26 +++++++++++ drivers/media/platform/ti-vpe/cal.c | 49 +++++++++++++++++++- drivers/media/platform/ti-vpe/cal.h | 3 ++ 3 files changed, 76 insertions(+), 2 deletions(-)