| Message ID | 20210412113457.328012-28-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ti-vpe: cal: prepare for multistream support | expand |
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:56PM +0300, Tomi Valkeinen wrote: > struct cal_camerarx has fmtinfo field which is used to point to the > current active input format. The only place where the field is used is > cal_camerarx_get_ext_link_freq(). > > With multiple streams the whole concept of single input format is not > valid anymore, so lets remove the field by looking up the format in > cal_camerarx_get_ext_link_freq(), making it easier to handle the > multistream-case in the following patches. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-camerarx.c | 12 ++++++++---- > drivers/media/platform/ti-vpe/cal.h | 1 - > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index 25f4692d210e..efe6513d69e8 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -49,8 +49,15 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy) > { > struct v4l2_fwnode_bus_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2; > u32 num_lanes = mipi_csi2->num_data_lanes; > - u32 bpp = phy->fmtinfo->bpp; > + u32 bpp; > s64 freq; > + const struct cal_format_info *fmtinfo; I'd declare this after num_lanes as I like the reverse xmas tree order. > + > + fmtinfo = cal_format_by_code(phy->formats[CAL_CAMERARX_PAD_SINK].code); > + if (!fmtinfo) > + return -EINVAL; > + > + bpp = fmtinfo->bpp; I wonder if we'll end up dropping this, as falling back to V4L2_CID_PIXEL_RATE in v4l2_get_link_freq() makes less sense when using multiplexed streams. Something to worry about later. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > freq = v4l2_get_link_freq(phy->source->ctrl_handler, bpp, 2 * num_lanes); > if (freq < 0) { > @@ -723,9 +730,6 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, > format->which); > *fmt = format->format; > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > - phy->fmtinfo = fmtinfo; > - > return 0; > } > > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h > index c941d2aec79f..7f35ad5ceac2 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -163,7 +163,6 @@ struct cal_camerarx { > struct v4l2_subdev subdev; > struct media_pad pads[2]; > struct v4l2_mbus_framefmt formats[2]; > - const struct cal_format_info *fmtinfo; > }; > > struct cal_dev {
On 18/04/2021 16:24, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:56PM +0300, Tomi Valkeinen wrote: >> struct cal_camerarx has fmtinfo field which is used to point to the >> current active input format. The only place where the field is used is >> cal_camerarx_get_ext_link_freq(). >> >> With multiple streams the whole concept of single input format is not >> valid anymore, so lets remove the field by looking up the format in >> cal_camerarx_get_ext_link_freq(), making it easier to handle the >> multistream-case in the following patches. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal-camerarx.c | 12 ++++++++---- >> drivers/media/platform/ti-vpe/cal.h | 1 - >> 2 files changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c >> index 25f4692d210e..efe6513d69e8 100644 >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c >> @@ -49,8 +49,15 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy) >> { >> struct v4l2_fwnode_bus_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2; >> u32 num_lanes = mipi_csi2->num_data_lanes; >> - u32 bpp = phy->fmtinfo->bpp; >> + u32 bpp; >> s64 freq; >> + const struct cal_format_info *fmtinfo; > > I'd declare this after num_lanes as I like the reverse xmas tree order. > >> + >> + fmtinfo = cal_format_by_code(phy->formats[CAL_CAMERARX_PAD_SINK].code); >> + if (!fmtinfo) >> + return -EINVAL; >> + >> + bpp = fmtinfo->bpp; > > I wonder if we'll end up dropping this, as falling back to > V4L2_CID_PIXEL_RATE in v4l2_get_link_freq() makes less sense when using > multiplexed streams. Something to worry about later. You're right, it goes behind an if in an unposted patch: if (cal_streams_api) { bpp = 0; } else { const struct cal_format_info *fmtinfo; fmtinfo = cal_format_by_code(phy->formats[CAL_CAMERARX_PAD_SINK].code); if (!fmtinfo) return -EINVAL; bpp = fmtinfo->bpp; } and calling v4l2_get_link_freq with mul == 0 (bpp) will result in ENOENT if V4L2_CID_LINK_FREQ is not available. Tomi
diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c index 25f4692d210e..efe6513d69e8 100644 --- a/drivers/media/platform/ti-vpe/cal-camerarx.c +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c @@ -49,8 +49,15 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy) { struct v4l2_fwnode_bus_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2; u32 num_lanes = mipi_csi2->num_data_lanes; - u32 bpp = phy->fmtinfo->bpp; + u32 bpp; s64 freq; + const struct cal_format_info *fmtinfo; + + fmtinfo = cal_format_by_code(phy->formats[CAL_CAMERARX_PAD_SINK].code); + if (!fmtinfo) + return -EINVAL; + + bpp = fmtinfo->bpp; freq = v4l2_get_link_freq(phy->source->ctrl_handler, bpp, 2 * num_lanes); if (freq < 0) { @@ -723,9 +730,6 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, format->which); *fmt = format->format; - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) - phy->fmtinfo = fmtinfo; - return 0; } diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h index c941d2aec79f..7f35ad5ceac2 100644 --- a/drivers/media/platform/ti-vpe/cal.h +++ b/drivers/media/platform/ti-vpe/cal.h @@ -163,7 +163,6 @@ struct cal_camerarx { struct v4l2_subdev subdev; struct media_pad pads[2]; struct v4l2_mbus_framefmt formats[2]; - const struct cal_format_info *fmtinfo; }; struct cal_dev {
struct cal_camerarx has fmtinfo field which is used to point to the current active input format. The only place where the field is used is cal_camerarx_get_ext_link_freq(). With multiple streams the whole concept of single input format is not valid anymore, so lets remove the field by looking up the format in cal_camerarx_get_ext_link_freq(), making it easier to handle the multistream-case in the following patches. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal-camerarx.c | 12 ++++++++---- drivers/media/platform/ti-vpe/cal.h | 1 - 2 files changed, 8 insertions(+), 5 deletions(-)