| Message ID | 20210412113457.328012-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| 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:30PM +0300, Tomi Valkeinen wrote: > v4l2-compliance complains about g/s_parm when using the non-MC API. Fix > it by adding the functions and just call v4l2_s/g_parm_cap for the > phy subdev. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-video.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c > index 7b7436a355ee..86d7cdd27224 100644 > --- a/drivers/media/platform/ti-vpe/cal-video.c > +++ b/drivers/media/platform/ti-vpe/cal-video.c > @@ -388,6 +388,20 @@ static int cal_enum_frameintervals(struct file *file, void *priv, > return 0; > } > > +static int cal_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > +{ > + struct cal_ctx *ctx = video_drvdata(file); > + > + return v4l2_g_parm_cap(video_devdata(file), ctx->phy->sensor, a); > +} > + > +static int cal_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > +{ > + struct cal_ctx *ctx = video_drvdata(file); > + > + return v4l2_s_parm_cap(video_devdata(file), ctx->phy->sensor, a); > +} > + > static const struct v4l2_ioctl_ops cal_ioctl_video_ops = { > .vidioc_querycap = cal_querycap, > .vidioc_enum_fmt_vid_cap = cal_enum_fmt_vid_cap, > @@ -411,6 +425,8 @@ static const struct v4l2_ioctl_ops cal_ioctl_video_ops = { > .vidioc_log_status = v4l2_ctrl_log_status, > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > + .vidioc_g_parm = cal_g_parm, > + .vidioc_s_parm = cal_s_parm, > }; > > /* ------------------------------------------------------------------ -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:33PM +0300, Tomi Valkeinen wrote: > CAL driver uses "sensor" name to refer to the subdev connected to CAL. > As the subdev can also be a bridge, the naming is misleading and might > cause the reader to think it refers to the actual sensor at the end of > the pipeline. > > Rename "sensor" to "source". > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-camerarx.c | 34 ++++++++++---------- > drivers/media/platform/ti-vpe/cal-video.c | 26 +++++++-------- > drivers/media/platform/ti-vpe/cal.c | 20 ++++++------ > drivers/media/platform/ti-vpe/cal.h | 6 ++-- > 4 files changed, 43 insertions(+), 43 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index 820fb483c402..603405824738 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -50,15 +50,15 @@ static s64 cal_camerarx_get_external_rate(struct cal_camerarx *phy) > struct v4l2_ctrl *ctrl; > s64 rate; > > - ctrl = v4l2_ctrl_find(phy->sensor->ctrl_handler, V4L2_CID_PIXEL_RATE); > + ctrl = v4l2_ctrl_find(phy->source->ctrl_handler, V4L2_CID_PIXEL_RATE); > if (!ctrl) { > phy_err(phy, "no pixel rate control in subdev: %s\n", > - phy->sensor->name); > + phy->source->name); > return -EPIPE; > } > > rate = v4l2_ctrl_g_ctrl_int64(ctrl); > - phy_dbg(3, phy, "sensor Pixel Rate: %llu\n", rate); > + phy_dbg(3, phy, "Source Pixel Rate: %llu\n", rate); > > return rate; > } > @@ -279,7 +279,7 @@ static int cal_camerarx_start(struct cal_camerarx *phy) > if (external_rate < 0) > return external_rate; > > - ret = v4l2_subdev_call(phy->sensor, core, s_power, 1); > + ret = v4l2_subdev_call(phy->source, core, s_power, 1); > if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV) { > phy_err(phy, "power on failed in subdev\n"); > return ret; > @@ -311,7 +311,7 @@ static int cal_camerarx_start(struct cal_camerarx *phy) > * 2. CSI PHY and link initialization sequence. > * > * a. Deassert the CSI-2 PHY reset. Do not wait for reset completion > - * at this point, as it requires the external sensor to send the > + * at this point, as it requires the external source to send the > * CSI-2 HS clock. > */ > cal_write_field(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance), > @@ -370,12 +370,12 @@ static int cal_camerarx_start(struct cal_camerarx *phy) > cal_camerarx_power(phy, true); > > /* > - * Start the sensor to enable the CSI-2 HS clock. We can now wait for > + * Start the source to enable the CSI-2 HS clock. We can now wait for > * CSI-2 PHY reset to complete. > */ > - ret = v4l2_subdev_call(phy->sensor, video, s_stream, 1); > + ret = v4l2_subdev_call(phy->source, video, s_stream, 1); > if (ret) { > - v4l2_subdev_call(phy->sensor, core, s_power, 0); > + v4l2_subdev_call(phy->source, core, s_power, 0); > cal_camerarx_disable_irqs(phy); > phy_err(phy, "stream on failed in subdev\n"); > return ret; > @@ -435,10 +435,10 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) > /* Disable the phy */ > cal_camerarx_disable(phy); > > - if (v4l2_subdev_call(phy->sensor, video, s_stream, 0)) > + if (v4l2_subdev_call(phy->source, video, s_stream, 0)) > phy_err(phy, "stream off failed in subdev\n"); > > - ret = v4l2_subdev_call(phy->sensor, core, s_power, 0); > + ret = v4l2_subdev_call(phy->source, core, s_power, 0); > if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV) > phy_err(phy, "power off failed in subdev\n"); > } > @@ -558,16 +558,16 @@ static int cal_camerarx_parse_dt(struct cal_camerarx *phy) > endpoint->bus.mipi_csi2.flags); > > /* Retrieve the connected device and store it for later use. */ > - phy->sensor_ep_node = of_graph_get_remote_endpoint(ep_node); > - phy->sensor_node = of_graph_get_port_parent(phy->sensor_ep_node); > - if (!phy->sensor_node) { > + phy->source_ep_node = of_graph_get_remote_endpoint(ep_node); > + phy->source_node = of_graph_get_port_parent(phy->source_ep_node); > + if (!phy->source_node) { > phy_dbg(3, phy, "Can't get remote parent\n"); > - of_node_put(phy->sensor_ep_node); > + of_node_put(phy->source_ep_node); > ret = -EINVAL; > goto done; > } > > - phy_dbg(1, phy, "Found connected device %pOFn\n", phy->sensor_node); > + phy_dbg(1, phy, "Found connected device %pOFn\n", phy->source_node); > > done: > of_node_put(ep_node); > @@ -866,7 +866,7 @@ void cal_camerarx_destroy(struct cal_camerarx *phy) > > v4l2_device_unregister_subdev(&phy->subdev); > media_entity_cleanup(&phy->subdev.entity); > - of_node_put(phy->sensor_ep_node); > - of_node_put(phy->sensor_node); > + of_node_put(phy->source_ep_node); > + of_node_put(phy->source_node); > kfree(phy); > } > diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c > index 86d7cdd27224..cf603cc9114c 100644 > --- a/drivers/media/platform/ti-vpe/cal-video.c > +++ b/drivers/media/platform/ti-vpe/cal-video.c > @@ -128,7 +128,7 @@ static int __subdev_get_format(struct cal_ctx *ctx, > sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > sd_fmt.pad = 0; > > - ret = v4l2_subdev_call(ctx->phy->sensor, pad, get_fmt, NULL, &sd_fmt); > + ret = v4l2_subdev_call(ctx->phy->source, pad, get_fmt, NULL, &sd_fmt); > if (ret) > return ret; > > @@ -151,7 +151,7 @@ static int __subdev_set_format(struct cal_ctx *ctx, > sd_fmt.pad = 0; > *mbus_fmt = *fmt; > > - ret = v4l2_subdev_call(ctx->phy->sensor, pad, set_fmt, NULL, &sd_fmt); > + ret = v4l2_subdev_call(ctx->phy->source, pad, set_fmt, NULL, &sd_fmt); > if (ret) > return ret; > > @@ -216,7 +216,7 @@ static int cal_try_fmt_vid_cap(struct file *file, void *priv, > fse.code = fmtinfo->code; > fse.which = V4L2_SUBDEV_FORMAT_ACTIVE; > for (fse.index = 0; ; fse.index++) { > - ret = v4l2_subdev_call(ctx->phy->sensor, pad, enum_frame_size, > + ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size, > NULL, &fse); > if (ret) > break; > @@ -321,7 +321,7 @@ static int cal_enum_framesizes(struct file *file, void *fh, > fse.code = fmtinfo->code; > fse.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > - ret = v4l2_subdev_call(ctx->phy->sensor, pad, enum_frame_size, NULL, > + ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size, NULL, > &fse); > if (ret) > return ret; > @@ -378,7 +378,7 @@ static int cal_enum_frameintervals(struct file *file, void *priv, > return -EINVAL; > > fie.code = fmtinfo->code; > - ret = v4l2_subdev_call(ctx->phy->sensor, pad, enum_frame_interval, > + ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_interval, > NULL, &fie); > if (ret) > return ret; > @@ -392,14 +392,14 @@ static int cal_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > { > struct cal_ctx *ctx = video_drvdata(file); > > - return v4l2_g_parm_cap(video_devdata(file), ctx->phy->sensor, a); > + return v4l2_g_parm_cap(video_devdata(file), ctx->phy->source, a); > } > > static int cal_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > { > struct cal_ctx *ctx = video_drvdata(file); > > - return v4l2_s_parm_cap(video_devdata(file), ctx->phy->sensor, a); > + return v4l2_s_parm_cap(video_devdata(file), ctx->phy->source, a); > } > > static const struct v4l2_ioctl_ops cal_ioctl_video_ops = { > @@ -799,20 +799,20 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx) > memset(&mbus_code, 0, sizeof(mbus_code)); > mbus_code.index = j; > mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE; > - ret = v4l2_subdev_call(ctx->phy->sensor, pad, enum_mbus_code, > + ret = v4l2_subdev_call(ctx->phy->source, pad, enum_mbus_code, > NULL, &mbus_code); > if (ret == -EINVAL) > break; > > if (ret) { > ctx_err(ctx, "Error enumerating mbus codes in subdev %s: %d\n", > - ctx->phy->sensor->name, ret); > + ctx->phy->source->name, ret); > return ret; > } > > ctx_dbg(2, ctx, > "subdev %s: code: %04x idx: %u\n", > - ctx->phy->sensor->name, mbus_code.code, j); > + ctx->phy->source->name, mbus_code.code, j); > > for (k = 0; k < cal_num_formats; k++) { > fmtinfo = &cal_formats[k]; > @@ -830,7 +830,7 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx) > > if (i == 0) { > ctx_err(ctx, "No suitable format reported by subdev %s\n", > - ctx->phy->sensor->name); > + ctx->phy->source->name); > return -EINVAL; > } > > @@ -867,10 +867,10 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx) > if (!cal_mc_api) { > struct v4l2_ctrl_handler *hdl = &ctx->ctrl_handler; > > - ret = v4l2_ctrl_add_handler(hdl, ctx->phy->sensor->ctrl_handler, > + ret = v4l2_ctrl_add_handler(hdl, ctx->phy->source->ctrl_handler, > NULL, true); > if (ret < 0) { > - ctx_err(ctx, "Failed to add sensor ctrl handler\n"); > + ctx_err(ctx, "Failed to add source ctrl handler\n"); > return ret; > } > } > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 2e2bef91b2b0..a5340b583592 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -635,20 +635,20 @@ static int cal_async_notifier_bound(struct v4l2_async_notifier *notifier, > int pad; > int ret; > > - if (phy->sensor) { > + if (phy->source) { > phy_info(phy, "Rejecting subdev %s (Already set!!)", > subdev->name); > return 0; > } > > - phy->sensor = subdev; > - phy_dbg(1, phy, "Using sensor %s for capture\n", subdev->name); > + phy->source = subdev; > + phy_dbg(1, phy, "Using source %s for capture\n", subdev->name); > > pad = media_entity_get_fwnode_pad(&subdev->entity, > - of_fwnode_handle(phy->sensor_ep_node), > + of_fwnode_handle(phy->source_ep_node), > MEDIA_PAD_FL_SOURCE); > if (pad < 0) { > - phy_err(phy, "Sensor %s has no connected source pad\n", > + phy_err(phy, "Source %s has no connected source pad\n", > subdev->name); > return pad; > } > @@ -658,7 +658,7 @@ static int cal_async_notifier_bound(struct v4l2_async_notifier *notifier, > MEDIA_LNK_FL_IMMUTABLE | > MEDIA_LNK_FL_ENABLED); > if (ret) { > - phy_err(phy, "Failed to create media link for sensor %s\n", > + phy_err(phy, "Failed to create media link for source %s\n", > subdev->name); > return ret; > } > @@ -701,10 +701,10 @@ static int cal_async_notifier_register(struct cal_dev *cal) > struct cal_v4l2_async_subdev *casd; > struct fwnode_handle *fwnode; > > - if (!phy->sensor_node) > + if (!phy->source_node) > continue; > > - fwnode = of_fwnode_handle(phy->sensor_node); > + fwnode = of_fwnode_handle(phy->source_node); > casd = v4l2_async_notifier_add_fwnode_subdev(&cal->notifier, > fwnode, > struct cal_v4l2_async_subdev); > @@ -1045,7 +1045,7 @@ static int cal_probe(struct platform_device *pdev) > goto error_camerarx; > } > > - if (cal->phy[i]->sensor_node) > + if (cal->phy[i]->source_node) > connected = true; > } > > @@ -1057,7 +1057,7 @@ static int cal_probe(struct platform_device *pdev) > > /* Create contexts. */ > for (i = 0; i < cal->data->num_csi2_phy; ++i) { > - if (!cal->phy[i]->sensor_node) > + if (!cal->phy[i]->source_node) > continue; > > cal->ctx[i] = cal_ctx_create(cal, i); > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h > index e079c6a9f93f..af46084580bd 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -155,9 +155,9 @@ struct cal_camerarx { > unsigned int instance; > > struct v4l2_fwnode_endpoint endpoint; > - struct device_node *sensor_ep_node; > - struct device_node *sensor_node; > - struct v4l2_subdev *sensor; > + struct device_node *source_ep_node; > + struct device_node *source_node; > + struct v4l2_subdev *source; > struct media_pipeline pipe; > > struct v4l2_subdev subdev; -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:32PM +0300, Tomi Valkeinen wrote: > cal_camerarx->dev field is not used, remove it. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h > index db0e408eaa94..e079c6a9f93f 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -149,7 +149,6 @@ struct cal_data { > struct cal_camerarx { > void __iomem *base; > struct resource *res; > - struct device *dev; > struct regmap_field *fields[F_MAX_FIELDS]; > > struct cal_dev *cal; -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:38PM +0300, Tomi Valkeinen wrote: > CAL has 8 PPI contexts per PHY, which are used to tag the incoming data. > The current driver only uses the first PPI, but we need to support all > of them to implement multi-stream support. > > Add a ppi_ctx field to cal_ctx, which indicates which of the 8 PPI > contexts is used for the particular cal_ctx. Also clean up the PPI > context register macros to take the PPI context number as a parameter. As far as I can tell, the TRMs don't mention "PPI contexts". PPI stands for PHY Protocol Interface, it does identify a particular physical interface. Would it be better to rename ppi_ctx to csi2_ctx ? This would match the register names too. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal.c | 10 ++++++---- > drivers/media/platform/ti-vpe/cal.h | 1 + > drivers/media/platform/ti-vpe/cal_regs.h | 18 ++---------------- > 3 files changed, 9 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 3d57aedbee0a..c550eeb27e79 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -294,7 +294,7 @@ static void cal_ctx_csi2_config(struct cal_ctx *ctx) > { > u32 val; > > - val = cal_read(ctx->cal, CAL_CSI2_CTX0(ctx->index)); > + val = cal_read(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx)); > cal_set_field(&val, ctx->cport, CAL_CSI2_CTX_CPORT_MASK); > /* > * DT type: MIPI CSI-2 Specs > @@ -310,9 +310,10 @@ static void cal_ctx_csi2_config(struct cal_ctx *ctx) > cal_set_field(&val, CAL_CSI2_CTX_ATT_PIX, CAL_CSI2_CTX_ATT_MASK); > cal_set_field(&val, CAL_CSI2_CTX_PACK_MODE_LINE, > CAL_CSI2_CTX_PACK_MODE_MASK); > - cal_write(ctx->cal, CAL_CSI2_CTX0(ctx->index), val); > - ctx_dbg(3, ctx, "CAL_CSI2_CTX0(%d) = 0x%08x\n", ctx->index, > - cal_read(ctx->cal, CAL_CSI2_CTX0(ctx->index))); > + cal_write(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx), val); > + ctx_dbg(3, ctx, "CAL_CSI2_CTX%d(%d) = 0x%08x\n", > + ctx->phy->instance, ctx->ppi_ctx, > + cal_read(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx))); > } > > static void cal_ctx_pix_proc_config(struct cal_ctx *ctx) > @@ -854,6 +855,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) > ctx->cal = cal; > ctx->phy = cal->phy[inst]; > ctx->index = inst; > + ctx->ppi_ctx = inst; To avoid a functional change in this patch, should this be = 0 ? > ctx->cport = inst; > > ret = cal_ctx_v4l2_init(ctx); > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h > index 251bb0ba7b3b..6eb63268f916 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -219,6 +219,7 @@ struct cal_ctx { > struct vb2_queue vb_vidq; > u8 index; > u8 cport; > + u8 ppi_ctx; > }; > > extern unsigned int cal_debug; > diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h > index f752096dcf7f..5c4f9e642185 100644 > --- a/drivers/media/platform/ti-vpe/cal_regs.h > +++ b/drivers/media/platform/ti-vpe/cal_regs.h > @@ -72,22 +72,8 @@ > #define CAL_CSI2_TIMING(m) (0x314U + (m) * 0x80U) > #define CAL_CSI2_VC_IRQENABLE(m) (0x318U + (m) * 0x80U) > #define CAL_CSI2_VC_IRQSTATUS(m) (0x328U + (m) * 0x80U) > -#define CAL_CSI2_CTX0(m) (0x330U + (m) * 0x80U) > -#define CAL_CSI2_CTX1(m) (0x334U + (m) * 0x80U) > -#define CAL_CSI2_CTX2(m) (0x338U + (m) * 0x80U) > -#define CAL_CSI2_CTX3(m) (0x33cU + (m) * 0x80U) > -#define CAL_CSI2_CTX4(m) (0x340U + (m) * 0x80U) > -#define CAL_CSI2_CTX5(m) (0x344U + (m) * 0x80U) > -#define CAL_CSI2_CTX6(m) (0x348U + (m) * 0x80U) > -#define CAL_CSI2_CTX7(m) (0x34cU + (m) * 0x80U) > -#define CAL_CSI2_STATUS0(m) (0x350U + (m) * 0x80U) > -#define CAL_CSI2_STATUS1(m) (0x354U + (m) * 0x80U) > -#define CAL_CSI2_STATUS2(m) (0x358U + (m) * 0x80U) > -#define CAL_CSI2_STATUS3(m) (0x35cU + (m) * 0x80U) > -#define CAL_CSI2_STATUS4(m) (0x360U + (m) * 0x80U) > -#define CAL_CSI2_STATUS5(m) (0x364U + (m) * 0x80U) > -#define CAL_CSI2_STATUS6(m) (0x368U + (m) * 0x80U) > -#define CAL_CSI2_STATUS7(m) (0x36cU + (m) * 0x80U) > +#define CAL_CSI2_CTX(phy, ppi_ctx) (0x330U + (phy) * 0x80U + (ppi_ctx) * 4) > +#define CAL_CSI2_STATUS(phy, ppi_ctx) (0x350U + (phy) * 0x80U + (ppi_ctx) * 4) > > /* CAL CSI2 PHY register offsets */ > #define CAL_CSI2_PHY_REG0 0x000 -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:40PM +0300, Tomi Valkeinen wrote: > Rename cal_ctx->index to dma_ctx to clarify that the field refers to the > DMA context number. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-video.c | 4 +-- > drivers/media/platform/ti-vpe/cal.c | 38 +++++++++++------------ > drivers/media/platform/ti-vpe/cal.h | 8 ++--- > 3 files changed, 25 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c > index 8e9bbe6beb23..064efdc31b28 100644 > --- a/drivers/media/platform/ti-vpe/cal-video.c > +++ b/drivers/media/platform/ti-vpe/cal-video.c > @@ -897,7 +897,7 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx) > MEDIA_LNK_FL_ENABLED); > if (ret) { > ctx_err(ctx, "Failed to create media link for context %u\n", > - ctx->index); > + ctx->dma_ctx); > video_unregister_device(vfd); > return ret; > } > @@ -949,7 +949,7 @@ int cal_ctx_v4l2_init(struct cal_ctx *ctx) > | (cal_mc_api ? V4L2_CAP_IO_MC : 0); > vfd->v4l2_dev = &ctx->cal->v4l2_dev; > vfd->queue = q; > - snprintf(vfd->name, sizeof(vfd->name), "CAL output %u", ctx->index); > + snprintf(vfd->name, sizeof(vfd->name), "CAL output %u", ctx->dma_ctx); > vfd->release = video_device_release_empty; > vfd->ioctl_ops = cal_mc_api ? &cal_ioctl_mc_ops : &cal_ioctl_video_ops; > vfd->lock = &ctx->mutex; > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 3dc83c66fd96..daa0c1ab94e7 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -372,7 +372,7 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) > unsigned int stride = ctx->v_fmt.fmt.pix.bytesperline; > u32 val; > > - val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->index)); > + val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); > cal_set_field(&val, ctx->cport, CAL_WR_DMA_CTRL_CPORT_MASK); > cal_set_field(&val, ctx->v_fmt.fmt.pix.height, > CAL_WR_DMA_CTRL_YSIZE_MASK); > @@ -383,16 +383,16 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) > cal_set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR, > CAL_WR_DMA_CTRL_PATTERN_MASK); > cal_set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK); > - cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->index), val); > - ctx_dbg(3, ctx, "CAL_WR_DMA_CTRL(%d) = 0x%08x\n", ctx->index, > - cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->index))); > + cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx), val); > + ctx_dbg(3, ctx, "CAL_WR_DMA_CTRL(%d) = 0x%08x\n", ctx->dma_ctx, > + cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx))); > > - cal_write_field(ctx->cal, CAL_WR_DMA_OFST(ctx->index), > + cal_write_field(ctx->cal, CAL_WR_DMA_OFST(ctx->dma_ctx), > stride / 16, CAL_WR_DMA_OFST_MASK); > - ctx_dbg(3, ctx, "CAL_WR_DMA_OFST(%d) = 0x%08x\n", ctx->index, > - cal_read(ctx->cal, CAL_WR_DMA_OFST(ctx->index))); > + ctx_dbg(3, ctx, "CAL_WR_DMA_OFST(%d) = 0x%08x\n", ctx->dma_ctx, > + cal_read(ctx->cal, CAL_WR_DMA_OFST(ctx->dma_ctx))); > > - val = cal_read(ctx->cal, CAL_WR_DMA_XSIZE(ctx->index)); > + val = cal_read(ctx->cal, CAL_WR_DMA_XSIZE(ctx->dma_ctx)); > /* 64 bit word means no skipping */ > cal_set_field(&val, 0, CAL_WR_DMA_XSIZE_XSKIP_MASK); > /* > @@ -401,23 +401,23 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) > * written per line. > */ > cal_set_field(&val, stride / 8, CAL_WR_DMA_XSIZE_MASK); > - cal_write(ctx->cal, CAL_WR_DMA_XSIZE(ctx->index), val); > - ctx_dbg(3, ctx, "CAL_WR_DMA_XSIZE(%d) = 0x%08x\n", ctx->index, > - cal_read(ctx->cal, CAL_WR_DMA_XSIZE(ctx->index))); > + cal_write(ctx->cal, CAL_WR_DMA_XSIZE(ctx->dma_ctx), val); > + ctx_dbg(3, ctx, "CAL_WR_DMA_XSIZE(%d) = 0x%08x\n", ctx->dma_ctx, > + cal_read(ctx->cal, CAL_WR_DMA_XSIZE(ctx->dma_ctx))); > } > > void cal_ctx_set_dma_addr(struct cal_ctx *ctx, dma_addr_t addr) > { > - cal_write(ctx->cal, CAL_WR_DMA_ADDR(ctx->index), addr); > + cal_write(ctx->cal, CAL_WR_DMA_ADDR(ctx->dma_ctx), addr); > } > > static void cal_ctx_wr_dma_disable(struct cal_ctx *ctx) > { > - u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->index)); > + u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); > > cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_DIS, > CAL_WR_DMA_CTRL_MODE_MASK); > - cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->index), val); > + cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx), val); > } > > static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx) > @@ -453,9 +453,9 @@ void cal_ctx_start(struct cal_ctx *ctx) > > /* Enable IRQ_WDMA_END and IRQ_WDMA_START. */ > cal_write(ctx->cal, CAL_HL_IRQENABLE_SET(1), > - CAL_HL_IRQ_MASK(ctx->index)); > + CAL_HL_IRQ_MASK(ctx->dma_ctx)); > cal_write(ctx->cal, CAL_HL_IRQENABLE_SET(2), > - CAL_HL_IRQ_MASK(ctx->index)); > + CAL_HL_IRQ_MASK(ctx->dma_ctx)); > } > > void cal_ctx_stop(struct cal_ctx *ctx) > @@ -479,9 +479,9 @@ void cal_ctx_stop(struct cal_ctx *ctx) > > /* Disable IRQ_WDMA_END and IRQ_WDMA_START. */ > cal_write(ctx->cal, CAL_HL_IRQENABLE_CLR(1), > - CAL_HL_IRQ_MASK(ctx->index)); > + CAL_HL_IRQ_MASK(ctx->dma_ctx)); > cal_write(ctx->cal, CAL_HL_IRQENABLE_CLR(2), > - CAL_HL_IRQ_MASK(ctx->index)); > + CAL_HL_IRQ_MASK(ctx->dma_ctx)); > > ctx->dma.state = CAL_DMA_STOPPED; > } > @@ -854,7 +854,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) > > ctx->cal = cal; > ctx->phy = cal->phy[inst]; > - ctx->index = inst; > + ctx->dma_ctx = inst; > ctx->ppi_ctx = inst; > ctx->cport = inst; > ctx->pix_proc = inst; > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h > index 03c71763f1a5..c34b843d2019 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -217,7 +217,7 @@ struct cal_ctx { > > unsigned int sequence; > struct vb2_queue vb_vidq; > - u8 index; > + u8 dma_ctx; > u8 cport; > u8 ppi_ctx; > u8 pix_proc; > @@ -238,11 +238,11 @@ extern bool cal_mc_api; > dev_err((cal)->dev, fmt, ##arg) > > #define ctx_dbg(level, ctx, fmt, arg...) \ > - cal_dbg(level, (ctx)->cal, "ctx%u: " fmt, (ctx)->index, ##arg) > + cal_dbg(level, (ctx)->cal, "ctx%u: " fmt, (ctx)->dma_ctx, ##arg) > #define ctx_info(ctx, fmt, arg...) \ > - cal_info((ctx)->cal, "ctx%u: " fmt, (ctx)->index, ##arg) > + cal_info((ctx)->cal, "ctx%u: " fmt, (ctx)->dma_ctx, ##arg) > #define ctx_err(ctx, fmt, arg...) \ > - cal_err((ctx)->cal, "ctx%u: " fmt, (ctx)->index, ##arg) > + cal_err((ctx)->cal, "ctx%u: " fmt, (ctx)->dma_ctx, ##arg) > > #define phy_dbg(level, phy, fmt, arg...) \ > cal_dbg(level, (phy)->cal, "phy%u: " fmt, (phy)->instance, ##arg) -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:42PM +0300, Tomi Valkeinen wrote: > The macros related to CAL_CSI2_VC_IRQ can be handled better by having > the VC number as a macro parameter. > > Note that the macros are not used anywhere yet, so no other changes are > needed. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal_regs.h | 30 +++++------------------- > 1 file changed, 6 insertions(+), 24 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h > index 93d9bf1f3c00..ed658175a444 100644 > --- a/drivers/media/platform/ti-vpe/cal_regs.h > +++ b/drivers/media/platform/ti-vpe/cal_regs.h > @@ -406,30 +406,12 @@ > #define CAL_CSI2_TIMING_STOP_STATE_X16_IO1_MASK BIT(14) > #define CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK BIT(15) > > -#define CAL_CSI2_VC_IRQ_FS_IRQ_0_MASK BIT(0) > -#define CAL_CSI2_VC_IRQ_FE_IRQ_0_MASK BIT(1) > -#define CAL_CSI2_VC_IRQ_LS_IRQ_0_MASK BIT(2) > -#define CAL_CSI2_VC_IRQ_LE_IRQ_0_MASK BIT(3) > -#define CAL_CSI2_VC_IRQ_CS_IRQ_0_MASK BIT(4) > -#define CAL_CSI2_VC_IRQ_ECC_CORRECTION0_IRQ_0_MASK BIT(5) > -#define CAL_CSI2_VC_IRQ_FS_IRQ_1_MASK BIT(8) > -#define CAL_CSI2_VC_IRQ_FE_IRQ_1_MASK BIT(9) > -#define CAL_CSI2_VC_IRQ_LS_IRQ_1_MASK BIT(10) > -#define CAL_CSI2_VC_IRQ_LE_IRQ_1_MASK BIT(11) > -#define CAL_CSI2_VC_IRQ_CS_IRQ_1_MASK BIT(12) > -#define CAL_CSI2_VC_IRQ_ECC_CORRECTION0_IRQ_1_MASK BIT(13) > -#define CAL_CSI2_VC_IRQ_FS_IRQ_2_MASK BIT(16) > -#define CAL_CSI2_VC_IRQ_FE_IRQ_2_MASK BIT(17) > -#define CAL_CSI2_VC_IRQ_LS_IRQ_2_MASK BIT(18) > -#define CAL_CSI2_VC_IRQ_LE_IRQ_2_MASK BIT(19) > -#define CAL_CSI2_VC_IRQ_CS_IRQ_2_MASK BIT(20) > -#define CAL_CSI2_VC_IRQ_ECC_CORRECTION0_IRQ_2_MASK BIT(21) > -#define CAL_CSI2_VC_IRQ_FS_IRQ_3_MASK BIT(24) > -#define CAL_CSI2_VC_IRQ_FE_IRQ_3_MASK BIT(25) > -#define CAL_CSI2_VC_IRQ_LS_IRQ_3_MASK BIT(26) > -#define CAL_CSI2_VC_IRQ_LE_IRQ_3_MASK BIT(27) > -#define CAL_CSI2_VC_IRQ_CS_IRQ_3_MASK BIT(28) > -#define CAL_CSI2_VC_IRQ_ECC_CORRECTION0_IRQ_3_MASK BIT(29) > +#define CAL_CSI2_VC_IRQ_FS_IRQ_MASK(n) BIT(0 + (n * 8)) This should be BIT(0 + (n) * 8). Same below. As they're single bits, I would have dropped the _MASK suffix. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +#define CAL_CSI2_VC_IRQ_FE_IRQ_MASK(n) BIT(1 + (n * 8)) > +#define CAL_CSI2_VC_IRQ_LS_IRQ_MASK(n) BIT(2 + (n * 8)) > +#define CAL_CSI2_VC_IRQ_LE_IRQ_MASK(n) BIT(3 + (n * 8)) > +#define CAL_CSI2_VC_IRQ_CS_IRQ_MASK(n) BIT(4 + (n * 8)) > +#define CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(n) BIT(5 + (n * 8)) > > #define CAL_CSI2_CTX_DT_MASK GENMASK(5, 0) > #define CAL_CSI2_CTX_VC_MASK GENMASK(7, 6) -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:43PM +0300, Tomi Valkeinen wrote: > CAL driver currently ignores VC related errors. To help catch error > conditions, enable all the VC error interrupts and handle them in the > interrupt handler by printing an error. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-camerarx.c | 23 ++++++++++++++++---- > drivers/media/platform/ti-vpe/cal.c | 9 ++++++++ > 2 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index 974fcbb19547..0354f311c5d2 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -226,24 +226,39 @@ static void cal_camerarx_enable_irqs(struct cal_camerarx *phy) > CAL_CSI2_COMPLEXIO_IRQ_FIFO_OVR_MASK | > CAL_CSI2_COMPLEXIO_IRQ_SHORT_PACKET_MASK | > CAL_CSI2_COMPLEXIO_IRQ_ECC_NO_CORRECTION_MASK; > - > - /* Enable CIO error IRQs. */ > + const u32 vc_err_mask = > + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(0) | > + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(1) | > + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(2) | > + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(3) | > + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(0) | > + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(1) | > + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(2) | > + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(3); > + > + /* Enable CIO & VC error IRQs. */ > cal_write(phy->cal, CAL_HL_IRQENABLE_SET(0), > - CAL_HL_IRQ_CIO_MASK(phy->instance)); > + CAL_HL_IRQ_CIO_MASK(phy->instance) | CAL_HL_IRQ_VC_MASK(phy->instance)); Line wrap ? Same in multiple places below. I know there's no strict 80 columns limit anymore, but I don't think longer lines help with readability in this patch (not to mention the coding style inconsistency with the rest of the driver). Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > cal_write(phy->cal, CAL_CSI2_COMPLEXIO_IRQENABLE(phy->instance), > cio_err_mask); > + cal_write(phy->cal, CAL_CSI2_VC_IRQENABLE(phy->instance), > + vc_err_mask); > } > > static void cal_camerarx_disable_irqs(struct cal_camerarx *phy) > { > /* Disable CIO error irqs */ > cal_write(phy->cal, CAL_HL_IRQENABLE_CLR(0), > - CAL_HL_IRQ_CIO_MASK(phy->instance)); > + CAL_HL_IRQ_CIO_MASK(phy->instance) | CAL_HL_IRQ_VC_MASK(phy->instance)); > cal_write(phy->cal, CAL_CSI2_COMPLEXIO_IRQENABLE(phy->instance), 0); > + cal_write(phy->cal, CAL_CSI2_VC_IRQENABLE(phy->instance), 0); > } > > static void cal_camerarx_ppi_enable(struct cal_camerarx *phy) > { > + cal_write_field(phy->cal, CAL_CSI2_PPI_CTRL(phy->instance), > + 1, CAL_CSI2_PPI_CTRL_ECC_EN_MASK); > + > cal_write_field(phy->cal, CAL_CSI2_PPI_CTRL(phy->instance), > 1, CAL_CSI2_PPI_CTRL_IF_EN_MASK); > } > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 0abcc83841c6..092041ddbcfb 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -577,6 +577,15 @@ static irqreturn_t cal_irq(int irq_cal, void *data) > cal_write(cal, CAL_CSI2_COMPLEXIO_IRQSTATUS(i), > cio_stat); > } > + > + if (status & CAL_HL_IRQ_VC_MASK(i)) { > + u32 vc_stat = cal_read(cal, CAL_CSI2_VC_IRQSTATUS(i)); > + > + dev_err_ratelimited(cal->dev, > + "CIO%u VC error: %#08x\n", i, vc_stat); > + > + cal_write(cal, CAL_CSI2_VC_IRQSTATUS(i), vc_stat); > + } > } > } > -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:45PM +0300, Tomi Valkeinen wrote: > Disable PPI and pix proc in cal_ctx_stop() to ensure they are not used > if the same context is used later on a different PHY or without pix > proc. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 092041ddbcfb..a6ca341c98bd 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -484,6 +484,12 @@ void cal_ctx_stop(struct cal_ctx *ctx) > CAL_HL_IRQ_WDMA_START_MASK(ctx->dma_ctx)); > > ctx->dma.state = CAL_DMA_STOPPED; > + > + /* Disable PPI context */ > + cal_write(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx), 0); > + > + /* Disable pix proc */ > + cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), 0); > } > > /* ------------------------------------------------------------------ -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:48PM +0300, Tomi Valkeinen wrote: > I have not noticed any errors due to this, but the DMA configuration > looks racy. Setting the DMA mode bitfield in CAL_WR_DMA_CTRL supposedly > enables the DMA. However, the driver currently a) continues configuring > the DMA after setting the mode, and b) enables the DMA interrupts only > after setting the mode. > > This probably doesn't cause any issues as there should be no data coming > in to the DMA yet, but it's still better to fix this. > > Add a new function, cal_ctx_wr_dma_enable(), to set the DMA mode field, > and call that function only after the DMA config and the irq enabling > has been done. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index a1d173bd4613..0fef892854ef 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -409,8 +409,6 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) > CAL_WR_DMA_CTRL_YSIZE_MASK); > cal_set_field(&val, CAL_WR_DMA_CTRL_DTAG_PIX_DAT, > CAL_WR_DMA_CTRL_DTAG_MASK); > - cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, > - CAL_WR_DMA_CTRL_MODE_MASK); > cal_set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR, > CAL_WR_DMA_CTRL_PATTERN_MASK); > cal_set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK); > @@ -442,6 +440,15 @@ void cal_ctx_set_dma_addr(struct cal_ctx *ctx, dma_addr_t addr) > cal_write(ctx->cal, CAL_WR_DMA_ADDR(ctx->dma_ctx), addr); > } > > +static void cal_ctx_wr_dma_enable(struct cal_ctx *ctx) > +{ > + u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); > + > + cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, > + CAL_WR_DMA_CTRL_MODE_MASK); > + cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx), val); > +} > + > static void cal_ctx_wr_dma_disable(struct cal_ctx *ctx) > { > u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); > @@ -500,6 +507,8 @@ void cal_ctx_start(struct cal_ctx *ctx) > CAL_HL_IRQ_WDMA_END_MASK(ctx->dma_ctx)); > cal_write(ctx->cal, CAL_HL_IRQENABLE_SET(2), > CAL_HL_IRQ_WDMA_START_MASK(ctx->dma_ctx)); > + > + cal_ctx_wr_dma_enable(ctx); > } You could also move the IRQ enabling before the call to cal_ctx_wr_dma_config(), and reorder the configuration in cal_ctx_wr_dma_config() to write CAL_WR_DMA_CTRL last. That would save a read-modify-write cycle. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > void cal_ctx_stop(struct cal_ctx *ctx) -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:49PM +0300, Tomi Valkeinen wrote: > In preparation for supporting multiple virtual channels and datatypes, > add vc and datatype fields to cal_ctx, initialize them to the currently > used values, and use those fields when writing to the register. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal.c | 6 ++++-- > drivers/media/platform/ti-vpe/cal.h | 2 ++ > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 0fef892854ef..91d2139adc9b 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -335,8 +335,8 @@ static void cal_ctx_csi2_config(struct cal_ctx *ctx) > * 0x2A: RAW8 1 pixel = 1 byte > * 0x1E: YUV422 2 pixels = 4 bytes > */ > - cal_set_field(&val, 0x1, CAL_CSI2_CTX_DT_MASK); > - cal_set_field(&val, 0, CAL_CSI2_CTX_VC_MASK); > + cal_set_field(&val, ctx->datatype, CAL_CSI2_CTX_DT_MASK); > + cal_set_field(&val, ctx->vc, CAL_CSI2_CTX_VC_MASK); > cal_set_field(&val, ctx->v_fmt.fmt.pix.height, CAL_CSI2_CTX_LINES_MASK); > cal_set_field(&val, CAL_CSI2_CTX_ATT_PIX, CAL_CSI2_CTX_ATT_MASK); > cal_set_field(&val, CAL_CSI2_CTX_PACK_MODE_LINE, > @@ -926,6 +926,8 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) > ctx->dma_ctx = inst; > ctx->ppi_ctx = inst; > ctx->cport = inst; > + ctx->vc = 0; > + ctx->datatype = 1; /* datatype filter disabled */ Could you define a macro in cal_regs.h for this ? You can then drop the comment. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > 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 409b7276a1fa..8aa93c92193a 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -223,6 +223,8 @@ struct cal_ctx { > u8 cport; > u8 ppi_ctx; > u8 pix_proc; > + u8 vc; > + u8 datatype; > > bool use_pix_proc; > }; -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:52PM +0300, Tomi Valkeinen wrote: > Fix a typo in a comment in cal_camerarx_sd_set_fmt(). > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-camerarx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index 880261d53a1d..25f4692d210e 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -695,7 +695,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, > return cal_camerarx_sd_get_fmt(sd, cfg, format); > > /* > - * Default to the first format is the requested media bus code isn't > + * Default to the first format if the requested media bus code isn't > * supported. > */ > fmtinfo = cal_format_by_code(format->format.code); -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:54PM +0300, Tomi Valkeinen wrote: > To make it more obvious if the function in question is dealing with > media-controller API or the legacy API, rename legacy API functions to > cal_legacy_*. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-video.c | 46 +++++++++++------------ > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c > index 1d9c0fce4b03..0494cd04b9a5 100644 > --- a/drivers/media/platform/ti-vpe/cal-video.c > +++ b/drivers/media/platform/ti-vpe/cal-video.c > @@ -102,7 +102,7 @@ static const struct cal_format_info *find_format_by_code(struct cal_ctx *ctx, > return NULL; > } > > -static int cal_enum_fmt_vid_cap(struct file *file, void *priv, > +static int cal_legacy_enum_fmt_vid_cap(struct file *file, void *priv, > struct v4l2_fmtdesc *f) Could you update the indentation ? Same below. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > { > struct cal_ctx *ctx = video_drvdata(file); > @@ -189,7 +189,7 @@ static void cal_calc_format_size(struct cal_ctx *ctx, > f->fmt.pix.bytesperline, f->fmt.pix.sizeimage); > } > > -static int cal_try_fmt_vid_cap(struct file *file, void *priv, > +static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv, > struct v4l2_format *f) > { > struct cal_ctx *ctx = video_drvdata(file); > @@ -249,7 +249,7 @@ static int cal_try_fmt_vid_cap(struct file *file, void *priv, > return 0; > } > > -static int cal_s_fmt_vid_cap(struct file *file, void *priv, > +static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv, > struct v4l2_format *f) > { > struct cal_ctx *ctx = video_drvdata(file); > @@ -266,7 +266,7 @@ static int cal_s_fmt_vid_cap(struct file *file, void *priv, > return -EBUSY; > } > > - ret = cal_try_fmt_vid_cap(file, priv, f); > + ret = cal_legacy_try_fmt_vid_cap(file, priv, f); > if (ret < 0) > return ret; > > @@ -300,7 +300,7 @@ static int cal_s_fmt_vid_cap(struct file *file, void *priv, > return 0; > } > > -static int cal_enum_framesizes(struct file *file, void *fh, > +static int cal_legacy_enum_framesizes(struct file *file, void *fh, > struct v4l2_frmsizeenum *fsize) > { > struct cal_ctx *ctx = video_drvdata(file); > @@ -337,7 +337,7 @@ static int cal_enum_framesizes(struct file *file, void *fh, > return 0; > } > > -static int cal_enum_input(struct file *file, void *priv, > +static int cal_legacy_enum_input(struct file *file, void *priv, > struct v4l2_input *inp) > { > if (inp->index > 0) > @@ -348,19 +348,19 @@ static int cal_enum_input(struct file *file, void *priv, > return 0; > } > > -static int cal_g_input(struct file *file, void *priv, unsigned int *i) > +static int cal_legacy_g_input(struct file *file, void *priv, unsigned int *i) > { > *i = 0; > return 0; > } > > -static int cal_s_input(struct file *file, void *priv, unsigned int i) > +static int cal_legacy_s_input(struct file *file, void *priv, unsigned int i) > { > return i > 0 ? -EINVAL : 0; > } > > /* timeperframe is arbitrary and continuous */ > -static int cal_enum_frameintervals(struct file *file, void *priv, > +static int cal_legacy_enum_frameintervals(struct file *file, void *priv, > struct v4l2_frmivalenum *fival) > { > struct cal_ctx *ctx = video_drvdata(file); > @@ -388,27 +388,27 @@ static int cal_enum_frameintervals(struct file *file, void *priv, > return 0; > } > > -static int cal_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > +static int cal_legacy_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > { > struct cal_ctx *ctx = video_drvdata(file); > > return v4l2_g_parm_cap(video_devdata(file), ctx->phy->source, a); > } > > -static int cal_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > +static int cal_legacy_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > { > struct cal_ctx *ctx = video_drvdata(file); > > return v4l2_s_parm_cap(video_devdata(file), ctx->phy->source, a); > } > > -static const struct v4l2_ioctl_ops cal_ioctl_video_ops = { > +static const struct v4l2_ioctl_ops cal_ioctl_legacy_ops = { > .vidioc_querycap = cal_querycap, > - .vidioc_enum_fmt_vid_cap = cal_enum_fmt_vid_cap, > + .vidioc_enum_fmt_vid_cap = cal_legacy_enum_fmt_vid_cap, > .vidioc_g_fmt_vid_cap = cal_g_fmt_vid_cap, > - .vidioc_try_fmt_vid_cap = cal_try_fmt_vid_cap, > - .vidioc_s_fmt_vid_cap = cal_s_fmt_vid_cap, > - .vidioc_enum_framesizes = cal_enum_framesizes, > + .vidioc_try_fmt_vid_cap = cal_legacy_try_fmt_vid_cap, > + .vidioc_s_fmt_vid_cap = cal_legacy_s_fmt_vid_cap, > + .vidioc_enum_framesizes = cal_legacy_enum_framesizes, > .vidioc_reqbufs = vb2_ioctl_reqbufs, > .vidioc_create_bufs = vb2_ioctl_create_bufs, > .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > @@ -416,17 +416,17 @@ static const struct v4l2_ioctl_ops cal_ioctl_video_ops = { > .vidioc_qbuf = vb2_ioctl_qbuf, > .vidioc_dqbuf = vb2_ioctl_dqbuf, > .vidioc_expbuf = vb2_ioctl_expbuf, > - .vidioc_enum_input = cal_enum_input, > - .vidioc_g_input = cal_g_input, > - .vidioc_s_input = cal_s_input, > - .vidioc_enum_frameintervals = cal_enum_frameintervals, > + .vidioc_enum_input = cal_legacy_enum_input, > + .vidioc_g_input = cal_legacy_g_input, > + .vidioc_s_input = cal_legacy_s_input, > + .vidioc_enum_frameintervals = cal_legacy_enum_frameintervals, > .vidioc_streamon = vb2_ioctl_streamon, > .vidioc_streamoff = vb2_ioctl_streamoff, > .vidioc_log_status = v4l2_ctrl_log_status, > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > - .vidioc_g_parm = cal_g_parm, > - .vidioc_s_parm = cal_s_parm, > + .vidioc_g_parm = cal_legacy_g_parm, > + .vidioc_s_parm = cal_legacy_s_parm, > }; > > /* ------------------------------------------------------------------ > @@ -966,7 +966,7 @@ int cal_ctx_v4l2_init(struct cal_ctx *ctx) > vfd->queue = q; > snprintf(vfd->name, sizeof(vfd->name), "CAL output %u", ctx->dma_ctx); > vfd->release = video_device_release_empty; > - vfd->ioctl_ops = cal_mc_api ? &cal_ioctl_mc_ops : &cal_ioctl_video_ops; > + vfd->ioctl_ops = cal_mc_api ? &cal_ioctl_mc_ops : &cal_ioctl_legacy_ops; > vfd->lock = &ctx->mutex; > video_set_drvdata(vfd, ctx); > -- Regards, Laurent Pinchart
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 { -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:36PM +0300, Tomi Valkeinen wrote: > In the following patches we need to do context configuration which might > fail. Add new functions, cal_ctx_prepare and cal_ctx_unprepare, to > handle such configuration. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-video.c | 9 +++++++++ > drivers/media/platform/ti-vpe/cal.c | 10 ++++++++++ > drivers/media/platform/ti-vpe/cal.h | 2 ++ > 3 files changed, 21 insertions(+) > > diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c > index cf603cc9114c..8e9bbe6beb23 100644 > --- a/drivers/media/platform/ti-vpe/cal-video.c > +++ b/drivers/media/platform/ti-vpe/cal-video.c > @@ -708,6 +708,12 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count) > goto error_pipeline; > } > > + ret = cal_ctx_prepare(ctx); > + if (ret) { > + ctx_err(ctx, "Failed to prepare context: %d\n", ret); > + goto error_pipeline; > + } > + > spin_lock_irq(&ctx->dma.lock); > buf = list_first_entry(&ctx->dma.queue, struct cal_buffer, list); > ctx->dma.pending = buf; > @@ -733,6 +739,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count) > error_stop: > cal_ctx_stop(ctx); > pm_runtime_put_sync(ctx->cal->dev); > + cal_ctx_unprepare(ctx); > > error_pipeline: > media_pipeline_stop(&ctx->vdev.entity); > @@ -752,6 +759,8 @@ static void cal_stop_streaming(struct vb2_queue *vq) > > pm_runtime_put_sync(ctx->cal->dev); > > + cal_ctx_unprepare(ctx); > + > cal_release_buffers(ctx, VB2_BUF_STATE_ERROR); > > media_pipeline_stop(&ctx->vdev.entity); > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index b539a9afb3f5..3d57aedbee0a 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -430,6 +430,16 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx) > return stopped; > } > > +int cal_ctx_prepare(struct cal_ctx *ctx) > +{ > + return 0; > +} > + > +void cal_ctx_unprepare(struct cal_ctx *ctx) > +{ > + > +} > + > void cal_ctx_start(struct cal_ctx *ctx) > { > ctx->sequence = 0; > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h > index af46084580bd..09ad20faa9e1 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -296,6 +296,8 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, > unsigned int instance); > void cal_camerarx_destroy(struct cal_camerarx *phy); > > +int cal_ctx_prepare(struct cal_ctx *ctx); > +void cal_ctx_unprepare(struct cal_ctx *ctx); > void cal_ctx_set_dma_addr(struct cal_ctx *ctx, dma_addr_t addr); > void cal_ctx_start(struct cal_ctx *ctx); > void cal_ctx_stop(struct cal_ctx *ctx); -- Regards, Laurent Pinchart
On 18/04/2021 15:17, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:38PM +0300, Tomi Valkeinen wrote: >> CAL has 8 PPI contexts per PHY, which are used to tag the incoming data. >> The current driver only uses the first PPI, but we need to support all >> of them to implement multi-stream support. >> >> Add a ppi_ctx field to cal_ctx, which indicates which of the 8 PPI >> contexts is used for the particular cal_ctx. Also clean up the PPI >> context register macros to take the PPI context number as a parameter. > > As far as I can tell, the TRMs don't mention "PPI contexts". PPI stands > for PHY Protocol Interface, it does identify a particular physical > interface. Would it be better to rename ppi_ctx to csi2_ctx ? This > would match the register names too. At least DRA76 TRM says, for example, these: "Number of contexts for PPI interface #0" "The PPI context IDs of interleaved streams must match." "Each PPI FSM has 8 copies of the CAL_CSI2_CTXy_l (y = [0 to 7], CAL_CSI2_CTX0_l through CAL_CSI2_CTX7_l) state registers (that is, contexts)." It's true changing ppi_ctx to csi2_ctx would match the register names neatly. But then again, the TRM doesn't mention CSI2 context, as far as I can tell. However, I think csi2_ctx is more understandable than ppi_ctx. So perhaps I'll change it. >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal.c | 10 ++++++---- >> drivers/media/platform/ti-vpe/cal.h | 1 + >> drivers/media/platform/ti-vpe/cal_regs.h | 18 ++---------------- >> 3 files changed, 9 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c >> index 3d57aedbee0a..c550eeb27e79 100644 >> --- a/drivers/media/platform/ti-vpe/cal.c >> +++ b/drivers/media/platform/ti-vpe/cal.c >> @@ -294,7 +294,7 @@ static void cal_ctx_csi2_config(struct cal_ctx *ctx) >> { >> u32 val; >> >> - val = cal_read(ctx->cal, CAL_CSI2_CTX0(ctx->index)); >> + val = cal_read(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx)); >> cal_set_field(&val, ctx->cport, CAL_CSI2_CTX_CPORT_MASK); >> /* >> * DT type: MIPI CSI-2 Specs >> @@ -310,9 +310,10 @@ static void cal_ctx_csi2_config(struct cal_ctx *ctx) >> cal_set_field(&val, CAL_CSI2_CTX_ATT_PIX, CAL_CSI2_CTX_ATT_MASK); >> cal_set_field(&val, CAL_CSI2_CTX_PACK_MODE_LINE, >> CAL_CSI2_CTX_PACK_MODE_MASK); >> - cal_write(ctx->cal, CAL_CSI2_CTX0(ctx->index), val); >> - ctx_dbg(3, ctx, "CAL_CSI2_CTX0(%d) = 0x%08x\n", ctx->index, >> - cal_read(ctx->cal, CAL_CSI2_CTX0(ctx->index))); >> + cal_write(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx), val); >> + ctx_dbg(3, ctx, "CAL_CSI2_CTX%d(%d) = 0x%08x\n", >> + ctx->phy->instance, ctx->ppi_ctx, >> + cal_read(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx))); >> } >> >> static void cal_ctx_pix_proc_config(struct cal_ctx *ctx) >> @@ -854,6 +855,7 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) >> ctx->cal = cal; >> ctx->phy = cal->phy[inst]; >> ctx->index = inst; >> + ctx->ppi_ctx = inst; > > To avoid a functional change in this patch, should this be = 0 ? Define "functional change" =). The context used may be different after this patch, but there's no change in how the HW functions. I changed it to 'inst' here, instead of keeping it as 0, as this works fine for both the legacy and new functionality and thus I don't need to remember to change it later. I should mention this in the commit desc, though. Tomi
On 18/04/2021 15:32, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:42PM +0300, Tomi Valkeinen wrote: >> The macros related to CAL_CSI2_VC_IRQ can be handled better by having >> the VC number as a macro parameter. >> >> Note that the macros are not used anywhere yet, so no other changes are >> needed. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal_regs.h | 30 +++++------------------- >> 1 file changed, 6 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h >> index 93d9bf1f3c00..ed658175a444 100644 >> --- a/drivers/media/platform/ti-vpe/cal_regs.h >> +++ b/drivers/media/platform/ti-vpe/cal_regs.h >> @@ -406,30 +406,12 @@ >> #define CAL_CSI2_TIMING_STOP_STATE_X16_IO1_MASK BIT(14) >> #define CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK BIT(15) >> >> -#define CAL_CSI2_VC_IRQ_FS_IRQ_0_MASK BIT(0) >> -#define CAL_CSI2_VC_IRQ_FE_IRQ_0_MASK BIT(1) >> -#define CAL_CSI2_VC_IRQ_LS_IRQ_0_MASK BIT(2) >> -#define CAL_CSI2_VC_IRQ_LE_IRQ_0_MASK BIT(3) >> -#define CAL_CSI2_VC_IRQ_CS_IRQ_0_MASK BIT(4) >> -#define CAL_CSI2_VC_IRQ_ECC_CORRECTION0_IRQ_0_MASK BIT(5) >> -#define CAL_CSI2_VC_IRQ_FS_IRQ_1_MASK BIT(8) >> -#define CAL_CSI2_VC_IRQ_FE_IRQ_1_MASK BIT(9) >> -#define CAL_CSI2_VC_IRQ_LS_IRQ_1_MASK BIT(10) >> -#define CAL_CSI2_VC_IRQ_LE_IRQ_1_MASK BIT(11) >> -#define CAL_CSI2_VC_IRQ_CS_IRQ_1_MASK BIT(12) >> -#define CAL_CSI2_VC_IRQ_ECC_CORRECTION0_IRQ_1_MASK BIT(13) >> -#define CAL_CSI2_VC_IRQ_FS_IRQ_2_MASK BIT(16) >> -#define CAL_CSI2_VC_IRQ_FE_IRQ_2_MASK BIT(17) >> -#define CAL_CSI2_VC_IRQ_LS_IRQ_2_MASK BIT(18) >> -#define CAL_CSI2_VC_IRQ_LE_IRQ_2_MASK BIT(19) >> -#define CAL_CSI2_VC_IRQ_CS_IRQ_2_MASK BIT(20) >> -#define CAL_CSI2_VC_IRQ_ECC_CORRECTION0_IRQ_2_MASK BIT(21) >> -#define CAL_CSI2_VC_IRQ_FS_IRQ_3_MASK BIT(24) >> -#define CAL_CSI2_VC_IRQ_FE_IRQ_3_MASK BIT(25) >> -#define CAL_CSI2_VC_IRQ_LS_IRQ_3_MASK BIT(26) >> -#define CAL_CSI2_VC_IRQ_LE_IRQ_3_MASK BIT(27) >> -#define CAL_CSI2_VC_IRQ_CS_IRQ_3_MASK BIT(28) >> -#define CAL_CSI2_VC_IRQ_ECC_CORRECTION0_IRQ_3_MASK BIT(29) >> +#define CAL_CSI2_VC_IRQ_FS_IRQ_MASK(n) BIT(0 + (n * 8)) > > This should be BIT(0 + (n) * 8). Same below. > > As they're single bits, I would have dropped the _MASK suffix. All the BIT(x) macros use _MASK suffix in the driver. Tomi
On 18/04/2021 15:38, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:43PM +0300, Tomi Valkeinen wrote: >> CAL driver currently ignores VC related errors. To help catch error >> conditions, enable all the VC error interrupts and handle them in the >> interrupt handler by printing an error. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal-camerarx.c | 23 ++++++++++++++++---- >> drivers/media/platform/ti-vpe/cal.c | 9 ++++++++ >> 2 files changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c >> index 974fcbb19547..0354f311c5d2 100644 >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c >> @@ -226,24 +226,39 @@ static void cal_camerarx_enable_irqs(struct cal_camerarx *phy) >> CAL_CSI2_COMPLEXIO_IRQ_FIFO_OVR_MASK | >> CAL_CSI2_COMPLEXIO_IRQ_SHORT_PACKET_MASK | >> CAL_CSI2_COMPLEXIO_IRQ_ECC_NO_CORRECTION_MASK; >> - >> - /* Enable CIO error IRQs. */ >> + const u32 vc_err_mask = >> + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(0) | >> + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(1) | >> + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(2) | >> + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(3) | >> + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(0) | >> + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(1) | >> + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(2) | >> + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(3); >> + >> + /* Enable CIO & VC error IRQs. */ >> cal_write(phy->cal, CAL_HL_IRQENABLE_SET(0), >> - CAL_HL_IRQ_CIO_MASK(phy->instance)); >> + CAL_HL_IRQ_CIO_MASK(phy->instance) | CAL_HL_IRQ_VC_MASK(phy->instance)); > > Line wrap ? Same in multiple places below. I know there's no strict 80 > columns limit anymore, but I don't think longer lines help with > readability in this patch (not to mention the coding style inconsistency > with the rest of the driver). Well, I disagree, but I guess that's in the eye of the beholder. If we have a coding style with strict 80 column limit, then there are other places I need to start fixing too. My personal coding style is such that around 80 columns I start thinking about splitting if it can be done without any messiness, around 100 I seriously try to split it, and around 120 I think it's broken. I can change this and the other similar line, the end result is only slightly messier, but... >> + >> + if (status & CAL_HL_IRQ_VC_MASK(i)) { >> + u32 vc_stat = cal_read(cal, CAL_CSI2_VC_IRQSTATUS(i)); >> + >> + dev_err_ratelimited(cal->dev, >> + "CIO%u VC error: %#08x\n", i, vc_stat); >> + >> + cal_write(cal, CAL_CSI2_VC_IRQSTATUS(i), vc_stat); >> + } ...especially for this part sticking to 80 columns uglifies the code. u32 vc_stat = cal_read(cal, CAL_CSI2_VC_IRQSTATUS(i)); or u32 cio_stat = cal_read(cal, CAL_CSI2_COMPLEXIO_IRQSTATUS(i)); I could split parts to a separate function, but I don't think the end result would be better. I think we discuss the 80-column problem almost in every series. Maybe we need to agree to some clear predefined rules to avoid future discussions about this? =) Tomi
On 18/04/2021 16:04, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:48PM +0300, Tomi Valkeinen wrote: >> I have not noticed any errors due to this, but the DMA configuration >> looks racy. Setting the DMA mode bitfield in CAL_WR_DMA_CTRL supposedly >> enables the DMA. However, the driver currently a) continues configuring >> the DMA after setting the mode, and b) enables the DMA interrupts only >> after setting the mode. >> >> This probably doesn't cause any issues as there should be no data coming >> in to the DMA yet, but it's still better to fix this. >> >> Add a new function, cal_ctx_wr_dma_enable(), to set the DMA mode field, >> and call that function only after the DMA config and the irq enabling >> has been done. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c >> index a1d173bd4613..0fef892854ef 100644 >> --- a/drivers/media/platform/ti-vpe/cal.c >> +++ b/drivers/media/platform/ti-vpe/cal.c >> @@ -409,8 +409,6 @@ static void cal_ctx_wr_dma_config(struct cal_ctx *ctx) >> CAL_WR_DMA_CTRL_YSIZE_MASK); >> cal_set_field(&val, CAL_WR_DMA_CTRL_DTAG_PIX_DAT, >> CAL_WR_DMA_CTRL_DTAG_MASK); >> - cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, >> - CAL_WR_DMA_CTRL_MODE_MASK); >> cal_set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR, >> CAL_WR_DMA_CTRL_PATTERN_MASK); >> cal_set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK); >> @@ -442,6 +440,15 @@ void cal_ctx_set_dma_addr(struct cal_ctx *ctx, dma_addr_t addr) >> cal_write(ctx->cal, CAL_WR_DMA_ADDR(ctx->dma_ctx), addr); >> } >> >> +static void cal_ctx_wr_dma_enable(struct cal_ctx *ctx) >> +{ >> + u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); >> + >> + cal_set_field(&val, CAL_WR_DMA_CTRL_MODE_CONST, >> + CAL_WR_DMA_CTRL_MODE_MASK); >> + cal_write(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx), val); >> +} >> + >> static void cal_ctx_wr_dma_disable(struct cal_ctx *ctx) >> { >> u32 val = cal_read(ctx->cal, CAL_WR_DMA_CTRL(ctx->dma_ctx)); >> @@ -500,6 +507,8 @@ void cal_ctx_start(struct cal_ctx *ctx) >> CAL_HL_IRQ_WDMA_END_MASK(ctx->dma_ctx)); >> cal_write(ctx->cal, CAL_HL_IRQENABLE_SET(2), >> CAL_HL_IRQ_WDMA_START_MASK(ctx->dma_ctx)); >> + >> + cal_ctx_wr_dma_enable(ctx); >> } > > You could also move the IRQ enabling before the call to > cal_ctx_wr_dma_config(), and reorder the configuration in > cal_ctx_wr_dma_config() to write CAL_WR_DMA_CTRL last. That would save a > read-modify-write cycle. Yes, I did that initially, but then ended up with a separate dma_enable call for clarity (I find it a bit misleading that cal_ctx_wr_dma_config would start the dma) and to have matching dma_enable and dma_disable calls. Tomi
On 18/04/2021 16:06, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:49PM +0300, Tomi Valkeinen wrote: >> In preparation for supporting multiple virtual channels and datatypes, >> add vc and datatype fields to cal_ctx, initialize them to the currently >> used values, and use those fields when writing to the register. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal.c | 6 ++++-- >> drivers/media/platform/ti-vpe/cal.h | 2 ++ >> 2 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c >> index 0fef892854ef..91d2139adc9b 100644 >> --- a/drivers/media/platform/ti-vpe/cal.c >> +++ b/drivers/media/platform/ti-vpe/cal.c >> @@ -335,8 +335,8 @@ static void cal_ctx_csi2_config(struct cal_ctx *ctx) >> * 0x2A: RAW8 1 pixel = 1 byte >> * 0x1E: YUV422 2 pixels = 4 bytes >> */ >> - cal_set_field(&val, 0x1, CAL_CSI2_CTX_DT_MASK); >> - cal_set_field(&val, 0, CAL_CSI2_CTX_VC_MASK); >> + cal_set_field(&val, ctx->datatype, CAL_CSI2_CTX_DT_MASK); >> + cal_set_field(&val, ctx->vc, CAL_CSI2_CTX_VC_MASK); >> cal_set_field(&val, ctx->v_fmt.fmt.pix.height, CAL_CSI2_CTX_LINES_MASK); >> cal_set_field(&val, CAL_CSI2_CTX_ATT_PIX, CAL_CSI2_CTX_ATT_MASK); >> cal_set_field(&val, CAL_CSI2_CTX_PACK_MODE_LINE, >> @@ -926,6 +926,8 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst) >> ctx->dma_ctx = inst; >> ctx->ppi_ctx = inst; >> ctx->cport = inst; >> + ctx->vc = 0; >> + ctx->datatype = 1; /* datatype filter disabled */ > > Could you define a macro in cal_regs.h for this ? You can then drop the > comment. Yes, good idea. I added: #define CAL_CSI2_CTX_DT_DISABLED 0 #define CAL_CSI2_CTX_DT_ANY 1 Tomi
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
Hi Tomi, On Mon, Apr 19, 2021 at 02:19:01PM +0300, Tomi Valkeinen wrote: > On 18/04/2021 15:38, Laurent Pinchart wrote: > > On Mon, Apr 12, 2021 at 02:34:43PM +0300, Tomi Valkeinen wrote: > >> CAL driver currently ignores VC related errors. To help catch error > >> conditions, enable all the VC error interrupts and handle them in the > >> interrupt handler by printing an error. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> drivers/media/platform/ti-vpe/cal-camerarx.c | 23 ++++++++++++++++---- > >> drivers/media/platform/ti-vpe/cal.c | 9 ++++++++ > >> 2 files changed, 28 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > >> index 974fcbb19547..0354f311c5d2 100644 > >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > >> @@ -226,24 +226,39 @@ static void cal_camerarx_enable_irqs(struct cal_camerarx *phy) > >> CAL_CSI2_COMPLEXIO_IRQ_FIFO_OVR_MASK | > >> CAL_CSI2_COMPLEXIO_IRQ_SHORT_PACKET_MASK | > >> CAL_CSI2_COMPLEXIO_IRQ_ECC_NO_CORRECTION_MASK; > >> - > >> - /* Enable CIO error IRQs. */ > >> + const u32 vc_err_mask = > >> + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(0) | > >> + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(1) | > >> + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(2) | > >> + CAL_CSI2_VC_IRQ_CS_IRQ_MASK(3) | > >> + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(0) | > >> + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(1) | > >> + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(2) | > >> + CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(3); > >> + > >> + /* Enable CIO & VC error IRQs. */ > >> cal_write(phy->cal, CAL_HL_IRQENABLE_SET(0), > >> - CAL_HL_IRQ_CIO_MASK(phy->instance)); > >> + CAL_HL_IRQ_CIO_MASK(phy->instance) | CAL_HL_IRQ_VC_MASK(phy->instance)); > > > > Line wrap ? Same in multiple places below. I know there's no strict 80 > > columns limit anymore, but I don't think longer lines help with > > readability in this patch (not to mention the coding style inconsistency > > with the rest of the driver). > > Well, I disagree, but I guess that's in the eye of the beholder. > > If we have a coding style with strict 80 column limit, then there are > other places I need to start fixing too. My personal coding style is > such that around 80 columns I start thinking about splitting if it can > be done without any messiness, around 100 I seriously try to split it, > and around 120 I think it's broken. It then all depends on your messiness gauge :-) For the code above, cal_write(phy->cal, CAL_HL_IRQENABLE_SET(0), CAL_HL_IRQ_CIO_MASK(phy->instance) | CAL_HL_IRQ_VC_MASK(phy->instance)); doesn't seem messy at all to me (quite the contrary actually). > I can change this and the other similar line, the end result is only > slightly messier, but... > > >> + > >> + if (status & CAL_HL_IRQ_VC_MASK(i)) { > >> + u32 vc_stat = cal_read(cal, CAL_CSI2_VC_IRQSTATUS(i)); > >> + > >> + dev_err_ratelimited(cal->dev, > >> + "CIO%u VC error: %#08x\n", i, vc_stat); > >> + > >> + cal_write(cal, CAL_CSI2_VC_IRQSTATUS(i), vc_stat); > >> + } > > ...especially for this part sticking to 80 columns uglifies the code. > > u32 vc_stat = > cal_read(cal, > CAL_CSI2_VC_IRQSTATUS(i)); > > or > > u32 cio_stat = cal_read(cal, > CAL_CSI2_COMPLEXIO_IRQSTATUS(i)); That would be messy, and I think it should either stay as-is, or the function should be refactored with code broken out in multiple functions (probably overkill for this specific example). The next line, however, I would have written as dev_err_ratelimited(cal->dev, "CIO%u VC error: %#08x\n", i, vc_stat); > I could split parts to a separate function, but I don't think the end > result would be better. > > I think we discuss the 80-column problem almost in every series. Maybe > we need to agree to some clear predefined rules to avoid future > discussions about this? =) So all we need is a metric that measure code mess, right ? :-) -- Regards, Laurent Pinchart
Hi, I'm working on adding multistream support to TI CAL driver. This series is a preparation series: it does not add any new features, but adds code needed for multistreaming. In other words, with this series the driver works just like it used to. The multistream code is not ready yet, but I'd like to start cleaning up my work branch by getting the "probably ok" patches reviewed and possibly merged. Tomi Tomi Valkeinen (28): media: ti-vpe: cal: add g/s_parm for legacy API media: ti-vpe: cal: fix error handling in cal_camerarx_create media: ti-vpe: cal: remove unused cal_camerarx->dev field media: ti-vpe: cal: rename "sensor" to "source" media: ti-vpe: cal: move global config from cal_ctx_wr_dma_config to runtime resume media: ti-vpe: cal: use v4l2_get_link_freq media: ti-vpe: cal: add cal_ctx_prepare/unprepare media: ti-vpe: cal: change index and cport to u8 media: ti-vpe: cal: Add PPI context media: ti-vpe: cal: Add pixel processing context media: ti-vpe: cal: rename cal_ctx->index to dma_ctx media: ti-vpe: cal: rename CAL_HL_IRQ_MASK media: ti-vpe: cal: clean up CAL_CSI2_VC_IRQ_* macros media: ti-vpe: cal: catch VC errors media: ti-vpe: cal: remove wait when stopping camerarx media: ti-vpe: cal: disable ppi and pix proc at ctx_stop media: ti-vpe: cal: allocate pix proc dynamically media: ti-vpe: cal: add 'use_pix_proc' field media: ti-vpe: cal: add cal_ctx_wr_dma_enable and fix a race media: ti-vpe: cal: add vc and datatype fields to cal_ctx media: ti-vpe: cal: fix cal_ctx_v4l2_register error handling media: ti-vpe: cal: set field always to V4L2_FIELD_NONE media: ti-vpe: cal: fix typo in a comment media: ti-vpe: cal: add mbus_code support to cal_mc_enum_fmt_vid_cap media: ti-vpe: cal: rename non-MC funcs to cal_legacy_* media: ti-vpe: cal: init ctx->v_fmt correctly in MC mode media: ti-vpe: cal: remove cal_camerarx->fmtinfo media: ti-vpe: cal: support 8 DMA contexts drivers/media/platform/ti-vpe/cal-camerarx.c | 157 ++++++------- drivers/media/platform/ti-vpe/cal-video.c | 163 +++++++++---- drivers/media/platform/ti-vpe/cal.c | 232 ++++++++++++------- drivers/media/platform/ti-vpe/cal.h | 37 +-- drivers/media/platform/ti-vpe/cal_regs.h | 51 +--- 5 files changed, 381 insertions(+), 259 deletions(-)