Message ID | 20230228171620.330978-4-tomi.valkeinen@ideasonboard.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | media: ti: cal: Streams support | expand |
Hi Tomi On Tue, Feb 28, 2023 at 07:16:19PM +0200, Tomi Valkeinen wrote: > CAL uses get_frame_desc to get the VC and DT for the incoming CSI-2 > stream, but does it in a bit hacky way. Clean this up by implementing > .get_frame_desc to camera-rx, and calling that from cal.c. > > No functional change intended. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti/cal/cal-camerarx.c | 62 +++++++++++--------- > drivers/media/platform/ti/cal/cal.c | 30 ++++------ > drivers/media/platform/ti/cal/cal.h | 2 - > 3 files changed, 47 insertions(+), 47 deletions(-) > > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c > index 3dfcb276d367..95e0ad59a39b 100644 > --- a/drivers/media/platform/ti/cal/cal-camerarx.c > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c > @@ -589,33 +589,6 @@ 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 *desc) > -{ > - struct media_pad *pad; > - int ret; > - > - if (!phy->source) > - return -EPIPE; > - > - pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]); > - if (!pad) > - return -EPIPE; > - > - ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index, > - desc); > - if (ret) > - return ret; > - > - if (desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > - dev_err(phy->cal->dev, > - "Frame descriptor does not describe CSI-2 link"); > - return -EINVAL; > - } > - > - return 0; > -} > - > /* ------------------------------------------------------------------ > * V4L2 Subdev Operations > * ------------------------------------------------------------------ > @@ -772,6 +745,40 @@ static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd, > return cal_camerarx_sd_set_fmt(sd, state, &format); > } > > +static int cal_camerarx_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_frame_desc *fd) > +{ > + struct cal_camerarx *phy = to_cal_camerarx(sd); > + struct v4l2_mbus_frame_desc remote_desc; > + const struct media_pad *remote_pad; > + int ret; > + > + remote_pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]); > + if (!remote_pad) > + return -EPIPE; > + > + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, > + remote_pad->index, &remote_desc); > + if (ret) > + return ret; > + > + if (remote_desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > + dev_err(phy->cal->dev, > + "Frame descriptor does not describe CSI-2 link"); > + return -EINVAL; > + } > + > + if (remote_desc.num_entries > 1) > + dev_dbg(phy->cal->dev, > + "Multiple streams not supported in remote frame descriptor, using the first one\n"); Maybe WARN_ONCE, but doesn't really matter as I presume this will go away in next patch > + > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; Is the bus between CAL and camera-rx a CSI-2 bus ?? As this mostly serves to transport to CAL the DT and VC, I guess it's fine Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > + fd->num_entries = 1; > + fd->entry[0] = remote_desc.entry[0]; > + > + return 0; > +} > + > static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = { > .s_stream = cal_camerarx_sd_s_stream, > }; > @@ -782,6 +789,7 @@ static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = { > .enum_frame_size = cal_camerarx_sd_enum_frame_size, > .get_fmt = v4l2_subdev_get_fmt, > .set_fmt = cal_camerarx_sd_set_fmt, > + .get_frame_desc = cal_camerarx_get_frame_desc, > }; > > static const struct v4l2_subdev_ops cal_camerarx_subdev_ops = { > diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c > index 760c58cb3b3e..bb782193cf65 100644 > --- a/drivers/media/platform/ti/cal/cal.c > +++ b/drivers/media/platform/ti/cal/cal.c > @@ -446,30 +446,24 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx) > } > > static int > -cal_get_remote_frame_desc_entry(struct cal_camerarx *phy, > +cal_get_remote_frame_desc_entry(struct cal_ctx *ctx, > struct v4l2_mbus_frame_desc_entry *entry) > { > struct v4l2_mbus_frame_desc fd; > + struct media_pad *phy_source_pad; > 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; > - } > - > - if (fd.num_entries == 0) { > - dev_err(phy->cal->dev, > - "No streams found in the remote frame descriptor\n"); > - > + phy_source_pad = media_pad_remote_pad_first(&ctx->pad); > + if (!phy_source_pad) > return -ENODEV; > - } > > - if (fd.num_entries > 1) > - dev_dbg(phy->cal->dev, > - "Multiple streams not supported in remote frame descriptor, using the first one\n"); > + ret = v4l2_subdev_call(&ctx->phy->subdev, pad, get_frame_desc, > + phy_source_pad->index, &fd); > + if (ret) > + return ret; > + > + if (fd.num_entries != 1) > + return -EINVAL; > > *entry = fd.entry[0]; > > @@ -481,7 +475,7 @@ 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, &entry); > + ret = cal_get_remote_frame_desc_entry(ctx, &entry); > > if (ret == -ENOIOCTLCMD) { > ctx->vc = 0; > diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h > index 55d4736fed18..0856297adc0b 100644 > --- a/drivers/media/platform/ti/cal/cal.h > +++ b/drivers/media/platform/ti/cal/cal.h > @@ -319,8 +319,6 @@ 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 *desc); > 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, > -- > 2.34.1 >
On 01/03/2023 11:40, Jacopo Mondi wrote: > Hi Tomi > > On Tue, Feb 28, 2023 at 07:16:19PM +0200, Tomi Valkeinen wrote: >> CAL uses get_frame_desc to get the VC and DT for the incoming CSI-2 >> stream, but does it in a bit hacky way. Clean this up by implementing >> .get_frame_desc to camera-rx, and calling that from cal.c. >> >> No functional change intended. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti/cal/cal-camerarx.c | 62 +++++++++++--------- >> drivers/media/platform/ti/cal/cal.c | 30 ++++------ >> drivers/media/platform/ti/cal/cal.h | 2 - >> 3 files changed, 47 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c >> index 3dfcb276d367..95e0ad59a39b 100644 >> --- a/drivers/media/platform/ti/cal/cal-camerarx.c >> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c >> @@ -589,33 +589,6 @@ 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 *desc) >> -{ >> - struct media_pad *pad; >> - int ret; >> - >> - if (!phy->source) >> - return -EPIPE; >> - >> - pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]); >> - if (!pad) >> - return -EPIPE; >> - >> - ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index, >> - desc); >> - if (ret) >> - return ret; >> - >> - if (desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { >> - dev_err(phy->cal->dev, >> - "Frame descriptor does not describe CSI-2 link"); >> - return -EINVAL; >> - } >> - >> - return 0; >> -} >> - >> /* ------------------------------------------------------------------ >> * V4L2 Subdev Operations >> * ------------------------------------------------------------------ >> @@ -772,6 +745,40 @@ static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd, >> return cal_camerarx_sd_set_fmt(sd, state, &format); >> } >> >> +static int cal_camerarx_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, >> + struct v4l2_mbus_frame_desc *fd) >> +{ >> + struct cal_camerarx *phy = to_cal_camerarx(sd); >> + struct v4l2_mbus_frame_desc remote_desc; >> + const struct media_pad *remote_pad; >> + int ret; >> + >> + remote_pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]); >> + if (!remote_pad) >> + return -EPIPE; >> + >> + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, >> + remote_pad->index, &remote_desc); >> + if (ret) >> + return ret; >> + >> + if (remote_desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { >> + dev_err(phy->cal->dev, >> + "Frame descriptor does not describe CSI-2 link"); >> + return -EINVAL; >> + } >> + >> + if (remote_desc.num_entries > 1) >> + dev_dbg(phy->cal->dev, >> + "Multiple streams not supported in remote frame descriptor, using the first one\n"); > > Maybe WARN_ONCE, but doesn't really matter as I presume this will go > away in next patch > >> + >> + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > > Is the bus between CAL and camera-rx a CSI-2 bus ?? No, it's a custom internal bus. > As this mostly serves to transport to CAL the DT and VC, I guess it's > fine Yes, we use it to transport CSI-2 data, and mainly just the DT and VC. The SW architecture doesn't really match the HW, but I think it works quite fine. E.g. there's really just a single output from each of the PHYs, which go to the CAL block (so CAL would have two inputs), and CAL contains a processing pipeline, and at the end it has the DMAs and memory connection. I don't know if trying to design the SW like that would help any, and besides, CAL is an old driver so the legacy drag is there. The current design is an extension to the older designs. Here, we could of course drop the .get_frame_desc from camera-rx, and implement (more or less identical) function which the cal.c could call. In some ways that would be better, but then again, I think the .get_frame_desc works here quite nicely, as cal.c can ask the frame desc for a single output pad (i.e. the stream which the context in question is interested in). > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks! Tomi
Hi Tomi On Wed, Mar 01, 2023 at 12:05:45PM +0200, Tomi Valkeinen wrote: > On 01/03/2023 11:40, Jacopo Mondi wrote: > > Hi Tomi > > > > On Tue, Feb 28, 2023 at 07:16:19PM +0200, Tomi Valkeinen wrote: > > > CAL uses get_frame_desc to get the VC and DT for the incoming CSI-2 > > > stream, but does it in a bit hacky way. Clean this up by implementing > > > .get_frame_desc to camera-rx, and calling that from cal.c. > > > > > > No functional change intended. > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > --- > > > drivers/media/platform/ti/cal/cal-camerarx.c | 62 +++++++++++--------- > > > drivers/media/platform/ti/cal/cal.c | 30 ++++------ > > > drivers/media/platform/ti/cal/cal.h | 2 - > > > 3 files changed, 47 insertions(+), 47 deletions(-) > > > > > > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c > > > index 3dfcb276d367..95e0ad59a39b 100644 > > > --- a/drivers/media/platform/ti/cal/cal-camerarx.c > > > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c > > > @@ -589,33 +589,6 @@ 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 *desc) > > > -{ > > > - struct media_pad *pad; > > > - int ret; > > > - > > > - if (!phy->source) > > > - return -EPIPE; > > > - > > > - pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]); > > > - if (!pad) > > > - return -EPIPE; > > > - > > > - ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index, > > > - desc); > > > - if (ret) > > > - return ret; > > > - > > > - if (desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > > > - dev_err(phy->cal->dev, > > > - "Frame descriptor does not describe CSI-2 link"); > > > - return -EINVAL; > > > - } > > > - > > > - return 0; > > > -} > > > - > > > /* ------------------------------------------------------------------ > > > * V4L2 Subdev Operations > > > * ------------------------------------------------------------------ > > > @@ -772,6 +745,40 @@ static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd, > > > return cal_camerarx_sd_set_fmt(sd, state, &format); > > > } > > > > > > +static int cal_camerarx_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > > + struct v4l2_mbus_frame_desc *fd) > > > +{ > > > + struct cal_camerarx *phy = to_cal_camerarx(sd); > > > + struct v4l2_mbus_frame_desc remote_desc; > > > + const struct media_pad *remote_pad; > > > + int ret; > > > + > > > + remote_pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]); > > > + if (!remote_pad) > > > + return -EPIPE; > > > + > > > + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, > > > + remote_pad->index, &remote_desc); > > > + if (ret) > > > + return ret; > > > + > > > + if (remote_desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > > > + dev_err(phy->cal->dev, > > > + "Frame descriptor does not describe CSI-2 link"); > > > + return -EINVAL; > > > + } > > > + > > > + if (remote_desc.num_entries > 1) > > > + dev_dbg(phy->cal->dev, > > > + "Multiple streams not supported in remote frame descriptor, using the first one\n"); > > > > Maybe WARN_ONCE, but doesn't really matter as I presume this will go > > away in next patch > > > > > + > > > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > > > > Is the bus between CAL and camera-rx a CSI-2 bus ?? > > No, it's a custom internal bus. > > > As this mostly serves to transport to CAL the DT and VC, I guess it's > > fine > > Yes, we use it to transport CSI-2 data, and mainly just the DT and VC. > > The SW architecture doesn't really match the HW, but I think it works quite > fine. E.g. there's really just a single output from each of the PHYs, which > go to the CAL block (so CAL would have two inputs), and CAL contains a > processing pipeline, and at the end it has the DMAs and memory connection. > > I don't know if trying to design the SW like that would help any, and > besides, CAL is an old driver so the legacy drag is there. The current > design is an extension to the older designs. > > Here, we could of course drop the .get_frame_desc from camera-rx, and > implement (more or less identical) function which the cal.c could call. In > some ways that would be better, but then again, I think the .get_frame_desc > works here quite nicely, as cal.c can ask the frame desc for a single output > pad (i.e. the stream which the context in question is interested in). > Yeah I was thinking about some helper that explicitly fetch the DT and VC transported on the camera-rx pad connected to this CAL instance. But I agree the current design works, and if you're fine with that I am as well :) > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks! > > Tomi >
diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c index 3dfcb276d367..95e0ad59a39b 100644 --- a/drivers/media/platform/ti/cal/cal-camerarx.c +++ b/drivers/media/platform/ti/cal/cal-camerarx.c @@ -589,33 +589,6 @@ 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 *desc) -{ - struct media_pad *pad; - int ret; - - if (!phy->source) - return -EPIPE; - - pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]); - if (!pad) - return -EPIPE; - - ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index, - desc); - if (ret) - return ret; - - if (desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { - dev_err(phy->cal->dev, - "Frame descriptor does not describe CSI-2 link"); - return -EINVAL; - } - - return 0; -} - /* ------------------------------------------------------------------ * V4L2 Subdev Operations * ------------------------------------------------------------------ @@ -772,6 +745,40 @@ static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd, return cal_camerarx_sd_set_fmt(sd, state, &format); } +static int cal_camerarx_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, + struct v4l2_mbus_frame_desc *fd) +{ + struct cal_camerarx *phy = to_cal_camerarx(sd); + struct v4l2_mbus_frame_desc remote_desc; + const struct media_pad *remote_pad; + int ret; + + remote_pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]); + if (!remote_pad) + return -EPIPE; + + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, + remote_pad->index, &remote_desc); + if (ret) + return ret; + + if (remote_desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { + dev_err(phy->cal->dev, + "Frame descriptor does not describe CSI-2 link"); + return -EINVAL; + } + + if (remote_desc.num_entries > 1) + dev_dbg(phy->cal->dev, + "Multiple streams not supported in remote frame descriptor, using the first one\n"); + + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; + fd->num_entries = 1; + fd->entry[0] = remote_desc.entry[0]; + + return 0; +} + static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = { .s_stream = cal_camerarx_sd_s_stream, }; @@ -782,6 +789,7 @@ static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = { .enum_frame_size = cal_camerarx_sd_enum_frame_size, .get_fmt = v4l2_subdev_get_fmt, .set_fmt = cal_camerarx_sd_set_fmt, + .get_frame_desc = cal_camerarx_get_frame_desc, }; static const struct v4l2_subdev_ops cal_camerarx_subdev_ops = { diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c index 760c58cb3b3e..bb782193cf65 100644 --- a/drivers/media/platform/ti/cal/cal.c +++ b/drivers/media/platform/ti/cal/cal.c @@ -446,30 +446,24 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx) } static int -cal_get_remote_frame_desc_entry(struct cal_camerarx *phy, +cal_get_remote_frame_desc_entry(struct cal_ctx *ctx, struct v4l2_mbus_frame_desc_entry *entry) { struct v4l2_mbus_frame_desc fd; + struct media_pad *phy_source_pad; 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; - } - - if (fd.num_entries == 0) { - dev_err(phy->cal->dev, - "No streams found in the remote frame descriptor\n"); - + phy_source_pad = media_pad_remote_pad_first(&ctx->pad); + if (!phy_source_pad) return -ENODEV; - } - if (fd.num_entries > 1) - dev_dbg(phy->cal->dev, - "Multiple streams not supported in remote frame descriptor, using the first one\n"); + ret = v4l2_subdev_call(&ctx->phy->subdev, pad, get_frame_desc, + phy_source_pad->index, &fd); + if (ret) + return ret; + + if (fd.num_entries != 1) + return -EINVAL; *entry = fd.entry[0]; @@ -481,7 +475,7 @@ 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, &entry); + ret = cal_get_remote_frame_desc_entry(ctx, &entry); if (ret == -ENOIOCTLCMD) { ctx->vc = 0; diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h index 55d4736fed18..0856297adc0b 100644 --- a/drivers/media/platform/ti/cal/cal.h +++ b/drivers/media/platform/ti/cal/cal.h @@ -319,8 +319,6 @@ 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 *desc); 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,
CAL uses get_frame_desc to get the VC and DT for the incoming CSI-2 stream, but does it in a bit hacky way. Clean this up by implementing .get_frame_desc to camera-rx, and calling that from cal.c. No functional change intended. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti/cal/cal-camerarx.c | 62 +++++++++++--------- drivers/media/platform/ti/cal/cal.c | 30 ++++------ drivers/media/platform/ti/cal/cal.h | 2 - 3 files changed, 47 insertions(+), 47 deletions(-)