| Message ID | 20210412113457.328012-22-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| 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:50PM +0300, Tomi Valkeinen wrote: > cal_ctx_v4l2_register() returns an error code, but the returned error > code is not handled anywhere. However, we can't really even handle the > error in any proper way, so lets just drop the return value and make > sure all error paths have an error print. Ouch. Doesn't this call for registering the video node earlier, at probe time, instead of in the async notifier complete callback ? > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-video.c | 16 ++++++++-------- > drivers/media/platform/ti-vpe/cal.h | 2 +- > 2 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c > index 064efdc31b28..ea9b13c16a06 100644 > --- a/drivers/media/platform/ti-vpe/cal-video.c > +++ b/drivers/media/platform/ti-vpe/cal-video.c > @@ -864,14 +864,16 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx) > return 0; > } > > -int cal_ctx_v4l2_register(struct cal_ctx *ctx) > +void cal_ctx_v4l2_register(struct cal_ctx *ctx) > { > struct video_device *vfd = &ctx->vdev; > int ret; > > ret = cal_ctx_v4l2_init_formats(ctx); > - if (ret) > - return ret; > + if (ret) { > + ctx_err(ctx, "Failed to init formats: %d\n", ret); > + return; > + } > > if (!cal_mc_api) { > struct v4l2_ctrl_handler *hdl = &ctx->ctrl_handler; > @@ -880,14 +882,14 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx) > NULL, true); > if (ret < 0) { > ctx_err(ctx, "Failed to add source ctrl handler\n"); > - return ret; > + return; > } > } > > ret = video_register_device(vfd, VFL_TYPE_VIDEO, cal_video_nr); > if (ret < 0) { > ctx_err(ctx, "Failed to register video device\n"); > - return ret; > + return; > } > > ret = media_create_pad_link(&ctx->phy->subdev.entity, > @@ -899,13 +901,11 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx) > ctx_err(ctx, "Failed to create media link for context %u\n", > ctx->dma_ctx); > video_unregister_device(vfd); > - return ret; > + return; > } > > ctx_info(ctx, "V4L2 device registered as %s\n", > video_device_node_name(vfd)); > - > - return 0; > } > > void cal_ctx_v4l2_unregister(struct cal_ctx *ctx) > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h > index 8aa93c92193a..ad7d26c803eb 100644 > --- a/drivers/media/platform/ti-vpe/cal.h > +++ b/drivers/media/platform/ti-vpe/cal.h > @@ -310,7 +310,7 @@ 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); > > -int cal_ctx_v4l2_register(struct cal_ctx *ctx); > +void cal_ctx_v4l2_register(struct cal_ctx *ctx); > void cal_ctx_v4l2_unregister(struct cal_ctx *ctx); > int cal_ctx_v4l2_init(struct cal_ctx *ctx); > void cal_ctx_v4l2_cleanup(struct cal_ctx *ctx); -- Regards, Laurent Pinchart
On 18/04/2021 16:09, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:50PM +0300, Tomi Valkeinen wrote: >> cal_ctx_v4l2_register() returns an error code, but the returned error >> code is not handled anywhere. However, we can't really even handle the >> error in any proper way, so lets just drop the return value and make >> sure all error paths have an error print. > > Ouch. Doesn't this call for registering the video node earlier, at probe > time, instead of in the async notifier complete callback ? Shouldn't we only register uAPI access points after the kernel has probed (succesfully) the hardware? If we register the video nodes at probe time I presume we would have to add checks to all the cal ioctl handlers to check if we have actually probed. v4l2_async_notifier_operations.complete can return an error, but it's not quite clear to me what happens in that case and how the driver should handle it. I'll look at this a bit, maybe it's still better to handle the error in complete callback and return the error from there, than ignoring the error. Tomi
On 20/04/2021 13:50, Tomi Valkeinen wrote: > On 18/04/2021 16:09, Laurent Pinchart wrote: >> Hi Tomi, >> >> Thank you for the patch. >> >> On Mon, Apr 12, 2021 at 02:34:50PM +0300, Tomi Valkeinen wrote: >>> cal_ctx_v4l2_register() returns an error code, but the returned error >>> code is not handled anywhere. However, we can't really even handle the >>> error in any proper way, so lets just drop the return value and make >>> sure all error paths have an error print. >> >> Ouch. Doesn't this call for registering the video node earlier, at probe >> time, instead of in the async notifier complete callback ? > > Shouldn't we only register uAPI access points after the kernel has > probed (succesfully) the hardware? If we register the video nodes at > probe time I presume we would have to add checks to all the cal ioctl > handlers to check if we have actually probed. > > v4l2_async_notifier_operations.complete can return an error, but it's > not quite clear to me what happens in that case and how the driver > should handle it. > > I'll look at this a bit, maybe it's still better to handle the error in > complete callback and return the error from there, than ignoring the error. Well, handling the error in complete callback seems to work fine. I'm not sure why I didn't do that and instead went with the approach in this patch. Tomi
Hi Tomi, On Tue, Apr 20, 2021 at 02:17:36PM +0300, Tomi Valkeinen wrote: > On 20/04/2021 13:50, Tomi Valkeinen wrote: > > On 18/04/2021 16:09, Laurent Pinchart wrote: > >> On Mon, Apr 12, 2021 at 02:34:50PM +0300, Tomi Valkeinen wrote: > >>> cal_ctx_v4l2_register() returns an error code, but the returned error > >>> code is not handled anywhere. However, we can't really even handle the > >>> error in any proper way, so lets just drop the return value and make > >>> sure all error paths have an error print. > >> > >> Ouch. Doesn't this call for registering the video node earlier, at probe > >> time, instead of in the async notifier complete callback ? > > > > Shouldn't we only register uAPI access points after the kernel has > > probed (succesfully) the hardware? If we register the video nodes at > > probe time I presume we would have to add checks to all the cal ioctl > > handlers to check if we have actually probed. There's a long-lasting debate on this topic :-) The issue with registering video nodes when all the subdevs have been acquired is that you should then unregister them with a subdev is removed. The re-registration gets fairly messy, especially if userspace keeps a video device node open. It's not like V4L2 handles object life time management correctly anyway, as it's completely broken in the core, maybe we shouldn't care and just decide that unbinding a device from its driver is unsupported. > > v4l2_async_notifier_operations.complete can return an error, but it's > > not quite clear to me what happens in that case and how the driver > > should handle it. > > > > I'll look at this a bit, maybe it's still better to handle the error in > > complete callback and return the error from there, than ignoring the error. > > Well, handling the error in complete callback seems to work fine. I'm > not sure why I didn't do that and instead went with the approach in this > patch.
diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c index 064efdc31b28..ea9b13c16a06 100644 --- a/drivers/media/platform/ti-vpe/cal-video.c +++ b/drivers/media/platform/ti-vpe/cal-video.c @@ -864,14 +864,16 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx) return 0; } -int cal_ctx_v4l2_register(struct cal_ctx *ctx) +void cal_ctx_v4l2_register(struct cal_ctx *ctx) { struct video_device *vfd = &ctx->vdev; int ret; ret = cal_ctx_v4l2_init_formats(ctx); - if (ret) - return ret; + if (ret) { + ctx_err(ctx, "Failed to init formats: %d\n", ret); + return; + } if (!cal_mc_api) { struct v4l2_ctrl_handler *hdl = &ctx->ctrl_handler; @@ -880,14 +882,14 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx) NULL, true); if (ret < 0) { ctx_err(ctx, "Failed to add source ctrl handler\n"); - return ret; + return; } } ret = video_register_device(vfd, VFL_TYPE_VIDEO, cal_video_nr); if (ret < 0) { ctx_err(ctx, "Failed to register video device\n"); - return ret; + return; } ret = media_create_pad_link(&ctx->phy->subdev.entity, @@ -899,13 +901,11 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx) ctx_err(ctx, "Failed to create media link for context %u\n", ctx->dma_ctx); video_unregister_device(vfd); - return ret; + return; } ctx_info(ctx, "V4L2 device registered as %s\n", video_device_node_name(vfd)); - - return 0; } void cal_ctx_v4l2_unregister(struct cal_ctx *ctx) diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h index 8aa93c92193a..ad7d26c803eb 100644 --- a/drivers/media/platform/ti-vpe/cal.h +++ b/drivers/media/platform/ti-vpe/cal.h @@ -310,7 +310,7 @@ 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); -int cal_ctx_v4l2_register(struct cal_ctx *ctx); +void cal_ctx_v4l2_register(struct cal_ctx *ctx); void cal_ctx_v4l2_unregister(struct cal_ctx *ctx); int cal_ctx_v4l2_init(struct cal_ctx *ctx); void cal_ctx_v4l2_cleanup(struct cal_ctx *ctx);
cal_ctx_v4l2_register() returns an error code, but the returned error code is not handled anywhere. However, we can't really even handle the error in any proper way, so lets just drop the return value and make sure all error paths have an error print. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal-video.c | 16 ++++++++-------- drivers/media/platform/ti-vpe/cal.h | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-)