| Message ID | 1527199339-7724-6-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jacopo, Thanks for your work. I really like what you did with this patch in v4. On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote: > The rcar-vin driver so far had a mutually exclusive code path for > handling parallel and CSI-2 video input subdevices, with only the CSI-2 > use case supporting media-controller. As we add support for parallel > inputs to Gen3 media-controller compliant code path now parse both port@0 > and port@1, handling the media-controller use case in the parallel > bound/unbind notifier operations. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > v3 -> v4: > - Change the mc/parallel initialization order. Initialize mc first, then > parallel > - As a consequence no need to delay parallel notifiers registration, the > media controller is set up already when parallel input got parsed, > this greatly simplify the group notifier complete callback. > --- > drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++----------- > 1 file changed, 35 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index a799684..29619c2 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); > vin->parallel->sink_pad = ret < 0 ? 0 : ret; > > + if (vin->info->use_mc) { > + vin->parallel->subdev = subdev; > + return 0; > + } > + > /* Find compatible subdevices mbus format */ > vin->mbus_code = 0; > code.index = 0; > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > { > rvin_v4l2_unregister(vin); > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - > - vin->vdev.ctrl_handler = NULL; > vin->parallel->subdev = NULL; > + > + if (!vin->info->use_mc) { > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > + vin->vdev.ctrl_handler = NULL; > + } > } > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev, > return 0; > } > > -static int rvin_parallel_graph_init(struct rvin_dev *vin) > +static int rvin_parallel_init(struct rvin_dev *vin) > { > int ret; > > - ret = v4l2_async_notifier_parse_fwnode_endpoints( > - vin->dev, &vin->notifier, > - sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2); > + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( > + vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity), > + 0, rvin_parallel_parse_v4l2); > if (ret) > return ret; > > if (!vin->parallel) > - return -ENODEV; > + return -ENOTCONN; I think you still should return -ENODEV here if !vin->info->use_mc to preserve Gen2 which runs without media controller behavior. How about: return vin->info->use_mc ? -ENOTCONN : -ENODEV; > > vin_dbg(vin, "Found parallel subdevice %pOF\n", > to_of_node(vin->parallel->asd.match.fwnode)); > @@ -784,14 +791,8 @@ static int rvin_mc_init(struct rvin_dev *vin) > { > int ret; > > - vin->pad.flags = MEDIA_PAD_FL_SINK; > - ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); > - if (ret) > - return ret; > - > - ret = rvin_group_get(vin); > - if (ret) > - return ret; > + if (!vin->info->use_mc) > + return 0; > > ret = rvin_mc_parse_of_graph(vin); > if (ret) > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev) > return ret; > > platform_set_drvdata(pdev, vin); > - if (vin->info->use_mc) > - ret = rvin_mc_init(vin); > - else > - ret = rvin_parallel_graph_init(vin); > - if (ret < 0) > + > + if (vin->info->use_mc) { > + vin->pad.flags = MEDIA_PAD_FL_SINK; > + ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); > + if (ret) > + return ret; > + > + ret = rvin_group_get(vin); > + if (ret) > + return ret; > + } I don't see why you need to move the media pad creation out of rvin_mc_init(). With the reorder of the rvin_mc_init() rvin_parallel_init() I would keep this in rvin_mc_init(). > + > + ret = rvin_mc_init(vin); > + if (ret) > + return ret; > + > + ret = rvin_parallel_init(vin); > + if (ret < 0 && ret != -ENOTCONN) > goto error; > > pm_suspend_ignore_children(&pdev->dev, true); > -- > 2.7.4 >
Hi Niklas, On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > I really like what you did with this patch in v4. Thanks for review and suggestions, what's there comes mostly from your comments and guidance. > > On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote: > > The rcar-vin driver so far had a mutually exclusive code path for > > handling parallel and CSI-2 video input subdevices, with only the CSI-2 > > use case supporting media-controller. As we add support for parallel > > inputs to Gen3 media-controller compliant code path now parse both port@0 > > and port@1, handling the media-controller use case in the parallel > > bound/unbind notifier operations. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > --- > > v3 -> v4: > > - Change the mc/parallel initialization order. Initialize mc first, then > > parallel > > - As a consequence no need to delay parallel notifiers registration, the > > media controller is set up already when parallel input got parsed, > > this greatly simplify the group notifier complete callback. > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++----------- > > 1 file changed, 35 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index a799684..29619c2 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); > > vin->parallel->sink_pad = ret < 0 ? 0 : ret; > > > > + if (vin->info->use_mc) { > > + vin->parallel->subdev = subdev; > > + return 0; > > + } > > + > > /* Find compatible subdevices mbus format */ > > vin->mbus_code = 0; > > code.index = 0; > > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > { > > rvin_v4l2_unregister(vin); > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - > > - vin->vdev.ctrl_handler = NULL; > > vin->parallel->subdev = NULL; > > + > > + if (!vin->info->use_mc) { > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + vin->vdev.ctrl_handler = NULL; > > + } > > } > > > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev, > > return 0; > > } > > > > -static int rvin_parallel_graph_init(struct rvin_dev *vin) > > +static int rvin_parallel_init(struct rvin_dev *vin) > > { > > int ret; > > > > - ret = v4l2_async_notifier_parse_fwnode_endpoints( > > - vin->dev, &vin->notifier, > > - sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2); > > + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( > > + vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity), > > + 0, rvin_parallel_parse_v4l2); > > if (ret) > > return ret; > > > > if (!vin->parallel) > > - return -ENODEV; > > + return -ENOTCONN; > > I think you still should return -ENODEV here if !vin->info->use_mc to > preserve Gen2 which runs without media controller behavior. How about: > > return vin->info->use_mc ? -ENOTCONN : -ENODEV; Right, I wish I had some gen2 board to test. I wonder if that's not better handled in probe though... I'll see how it looks like. > > > > > vin_dbg(vin, "Found parallel subdevice %pOF\n", > > to_of_node(vin->parallel->asd.match.fwnode)); > > @@ -784,14 +791,8 @@ static int rvin_mc_init(struct rvin_dev *vin) > > { > > int ret; > > > > - vin->pad.flags = MEDIA_PAD_FL_SINK; > > - ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); > > - if (ret) > > - return ret; > > - > > - ret = rvin_group_get(vin); > > - if (ret) > > - return ret; > > + if (!vin->info->use_mc) > > + return 0; > > > > ret = rvin_mc_parse_of_graph(vin); > > if (ret) > > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev) > > return ret; > > > > platform_set_drvdata(pdev, vin); > > - if (vin->info->use_mc) > > - ret = rvin_mc_init(vin); > > - else > > - ret = rvin_parallel_graph_init(vin); > > - if (ret < 0) > > + > > + if (vin->info->use_mc) { > > + vin->pad.flags = MEDIA_PAD_FL_SINK; > > + ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); > > + if (ret) > > + return ret; > > + > > + ret = rvin_group_get(vin); > > + if (ret) > > + return ret; > > + } > > I don't see why you need to move the media pad creation out of > rvin_mc_init(). With the reorder of the rvin_mc_init() > rvin_parallel_init() I would keep this in rvin_mc_init(). > Just because I've been lazy re-structuring that part of code I broke up in previous versions. I'll move that part back to mc_init() and possibly also remove the + if (!vin->info->use_mc) + return 0; in that function and handle this in probe(). Thanks j > > + > > + ret = rvin_mc_init(vin); > > + if (ret) > > + return ret; > > + > > + ret = rvin_parallel_init(vin); > > + if (ret < 0 && ret != -ENOTCONN) > > goto error; > > > > pm_suspend_ignore_children(&pdev->dev, true); > > -- > > 2.7.4 > > > > -- > Regards, > Niklas Söderlund
Hi Niklas, I might have another question before sending v5. On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > I really like what you did with this patch in v4. > > On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote: > > The rcar-vin driver so far had a mutually exclusive code path for > > handling parallel and CSI-2 video input subdevices, with only the CSI-2 > > use case supporting media-controller. As we add support for parallel > > inputs to Gen3 media-controller compliant code path now parse both port@0 > > and port@1, handling the media-controller use case in the parallel > > bound/unbind notifier operations. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > --- > > v3 -> v4: > > - Change the mc/parallel initialization order. Initialize mc first, then > > parallel > > - As a consequence no need to delay parallel notifiers registration, the > > media controller is set up already when parallel input got parsed, > > this greatly simplify the group notifier complete callback. > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++----------- > > 1 file changed, 35 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index a799684..29619c2 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); > > vin->parallel->sink_pad = ret < 0 ? 0 : ret; > > > > + if (vin->info->use_mc) { > > + vin->parallel->subdev = subdev; > > + return 0; > > + } > > + > > /* Find compatible subdevices mbus format */ > > vin->mbus_code = 0; > > code.index = 0; > > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > { > > rvin_v4l2_unregister(vin); > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > - > > - vin->vdev.ctrl_handler = NULL; > > vin->parallel->subdev = NULL; > > + > > + if (!vin->info->use_mc) { > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > > + vin->vdev.ctrl_handler = NULL; > > + } > > } > > > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev, > > return 0; > > } > > > > -static int rvin_parallel_graph_init(struct rvin_dev *vin) > > +static int rvin_parallel_init(struct rvin_dev *vin) > > { > > int ret; > > > > - ret = v4l2_async_notifier_parse_fwnode_endpoints( > > - vin->dev, &vin->notifier, > > - sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2); > > + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( > > + vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity), > > + 0, rvin_parallel_parse_v4l2); > > if (ret) > > return ret; > > > > if (!vin->parallel) > > - return -ENODEV; > > + return -ENOTCONN; > > I think you still should return -ENODEV here if !vin->info->use_mc to > preserve Gen2 which runs without media controller behavior. How about: > > return vin->info->use_mc ? -ENOTCONN : -ENODEV; > > > > > vin_dbg(vin, "Found parallel subdevice %pOF\n", > > to_of_node(vin->parallel->asd.match.fwnode)); > > @@ -784,14 +791,8 @@ static int rvin_mc_init(struct rvin_dev *vin) > > { > > int ret; > > > > - vin->pad.flags = MEDIA_PAD_FL_SINK; > > - ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); > > - if (ret) > > - return ret; > > - > > - ret = rvin_group_get(vin); > > - if (ret) > > - return ret; > > + if (!vin->info->use_mc) > > + return 0; > > > > ret = rvin_mc_parse_of_graph(vin); > > if (ret) > > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev) > > return ret; > > > > platform_set_drvdata(pdev, vin); > > - if (vin->info->use_mc) > > - ret = rvin_mc_init(vin); > > - else > > - ret = rvin_parallel_graph_init(vin); > > - if (ret < 0) > > + > > + if (vin->info->use_mc) { > > + vin->pad.flags = MEDIA_PAD_FL_SINK; > > + ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); > > + if (ret) > > + return ret; > > + > > + ret = rvin_group_get(vin); > > + if (ret) > > + return ret; > > + } > > I don't see why you need to move the media pad creation out of > rvin_mc_init(). With the reorder of the rvin_mc_init() > rvin_parallel_init() I would keep this in rvin_mc_init(). > > > + > > + ret = rvin_mc_init(vin); > > + if (ret) > > + return ret; > > + > > + ret = rvin_parallel_init(vin); > > + if (ret < 0 && ret != -ENOTCONN) > > goto error; > > > > pm_suspend_ignore_children(&pdev->dev, true); I've been looking at the error path handling now that the code looks like this in the forthcoming v5: if (vin->info->use_mc) { ret = rvin_mc_init(vin); if (ret) goto error_dma_unregister; } ret = rvin_parallel_init(vin); if (ret) goto error_group_unregister; ... error_group_unreg: if (vin->info->use_mc) { mutex_lock(&vin->group->lock); if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { v4l2_async_notifier_unregister(&vin->group->notifier); v4l2_async_notifier_cleanup(&vin->group->notifier); } mutex_unlock(&vin->group->lock); rvin_group_put(vin); } error_dma_unreg: rvin_dma_unregister(vin); return ret; Question is, do you think the group notifier should be unregistered and cleaned up if the parallel input initialization of the VIN instance whose v4l2_dev is used to register the group notifier fails? I feel like it should, as the VIN instance whose v4l2_dev is used is always the last probing one, so there should be no issues with other VINs registering after it and finding themselves without a valid notifier. I felt like it was better anticipating this to you instead of adding this part out of the blue in v5. Also, I think in both parallel input and mc notifier registration, the notifier should be cleaned up to release the parsed async subdevices memory allocated by v4l2_async_notifier_parse_fwnode_endpoints_by_port() if the sub-sequent v4l2_async_notifier_register() fails. That would be: @@ -781,18 +782,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) &vin->group->notifier); if (ret < 0) { vin_err(vin, "Notifier registration failed\n"); - return ret; + goto error_clean_up_notifier; } return 0; + +error_clean_up_notifier: + v4l2_async_notifier_cleanup(&vin->group->notifier); + + return ret; } in both mc and parallel initialization functions. With your ack I can send two patches on top of the currently merged VIN version, and rebase my series on top eventually. Sorry for the long email. Thanks j > > -- > > 2.7.4 > > > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2018-05-25 13:50:08 +0200, Jacopo Mondi wrote: > Hi Niklas, > I might have another question before sending v5. > > On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote: > > Hi Jacopo, > > > > Thanks for your work. > > > > I really like what you did with this patch in v4. > > > > On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote: > > > The rcar-vin driver so far had a mutually exclusive code path for > > > handling parallel and CSI-2 video input subdevices, with only the CSI-2 > > > use case supporting media-controller. As we add support for parallel > > > inputs to Gen3 media-controller compliant code path now parse both port@0 > > > and port@1, handling the media-controller use case in the parallel > > > bound/unbind notifier operations. > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > > > --- > > > v3 -> v4: > > > - Change the mc/parallel initialization order. Initialize mc first, then > > > parallel > > > - As a consequence no need to delay parallel notifiers registration, the > > > media controller is set up already when parallel input got parsed, > > > this greatly simplify the group notifier complete callback. > > > --- > > > drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++----------- > > > 1 file changed, 35 insertions(+), 21 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > > index a799684..29619c2 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > > ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); > > > vin->parallel->sink_pad = ret < 0 ? 0 : ret; > > > > > > + if (vin->info->use_mc) { > > > + vin->parallel->subdev = subdev; > > > + return 0; > > > + } > > > + > > > /* Find compatible subdevices mbus format */ > > > vin->mbus_code = 0; > > > code.index = 0; > > > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > > > static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > > > { > > > rvin_v4l2_unregister(vin); > > > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > - > > > - vin->vdev.ctrl_handler = NULL; > > > vin->parallel->subdev = NULL; > > > + > > > + if (!vin->info->use_mc) { > > > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > > > + vin->vdev.ctrl_handler = NULL; > > > + } > > > } > > > > > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > > > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev, > > > return 0; > > > } > > > > > > -static int rvin_parallel_graph_init(struct rvin_dev *vin) > > > +static int rvin_parallel_init(struct rvin_dev *vin) > > > { > > > int ret; > > > > > > - ret = v4l2_async_notifier_parse_fwnode_endpoints( > > > - vin->dev, &vin->notifier, > > > - sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2); > > > + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( > > > + vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity), > > > + 0, rvin_parallel_parse_v4l2); > > > if (ret) > > > return ret; > > > > > > if (!vin->parallel) > > > - return -ENODEV; > > > + return -ENOTCONN; > > > > I think you still should return -ENODEV here if !vin->info->use_mc to > > preserve Gen2 which runs without media controller behavior. How about: > > > > return vin->info->use_mc ? -ENOTCONN : -ENODEV; > > > > > > > > vin_dbg(vin, "Found parallel subdevice %pOF\n", > > > to_of_node(vin->parallel->asd.match.fwnode)); > > > @@ -784,14 +791,8 @@ static int rvin_mc_init(struct rvin_dev *vin) > > > { > > > int ret; > > > > > > - vin->pad.flags = MEDIA_PAD_FL_SINK; > > > - ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); > > > - if (ret) > > > - return ret; > > > - > > > - ret = rvin_group_get(vin); > > > - if (ret) > > > - return ret; > > > + if (!vin->info->use_mc) > > > + return 0; > > > > > > ret = rvin_mc_parse_of_graph(vin); > > > if (ret) > > > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev) > > > return ret; > > > > > > platform_set_drvdata(pdev, vin); > > > - if (vin->info->use_mc) > > > - ret = rvin_mc_init(vin); > > > - else > > > - ret = rvin_parallel_graph_init(vin); > > > - if (ret < 0) > > > + > > > + if (vin->info->use_mc) { > > > + vin->pad.flags = MEDIA_PAD_FL_SINK; > > > + ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); > > > + if (ret) > > > + return ret; > > > + > > > + ret = rvin_group_get(vin); > > > + if (ret) > > > + return ret; > > > + } > > > > I don't see why you need to move the media pad creation out of > > rvin_mc_init(). With the reorder of the rvin_mc_init() > > rvin_parallel_init() I would keep this in rvin_mc_init(). > > > > > + > > > + ret = rvin_mc_init(vin); > > > + if (ret) > > > + return ret; > > > + > > > + ret = rvin_parallel_init(vin); > > > + if (ret < 0 && ret != -ENOTCONN) > > > goto error; > > > > > > pm_suspend_ignore_children(&pdev->dev, true); > > I've been looking at the error path handling now that the code looks > like this in the forthcoming v5: > > if (vin->info->use_mc) { > ret = rvin_mc_init(vin); > if (ret) > goto error_dma_unregister; > } > > ret = rvin_parallel_init(vin); > if (ret) > goto error_group_unregister; > > > ... > > > error_group_unreg: > if (vin->info->use_mc) { > mutex_lock(&vin->group->lock); > if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { > v4l2_async_notifier_unregister(&vin->group->notifier); > v4l2_async_notifier_cleanup(&vin->group->notifier); > } > mutex_unlock(&vin->group->lock); > rvin_group_put(vin); > } > > error_dma_unreg: > rvin_dma_unregister(vin); > > return ret; > > Question is, do you think the group notifier should be unregistered > and cleaned up if the parallel input initialization of the VIN > instance whose v4l2_dev is used to register the group notifier fails? > > I feel like it should, as the VIN instance whose v4l2_dev is used is > always the last probing one, so there should be no issues with other > VINs registering after it and finding themselves without a valid > notifier. I agree with you. If the parallel initialization fails everything done by that particular VIN probe should be undone. So if it have registered the group notifier it should unregister it. > > I felt like it was better anticipating this to you instead of adding > this part out of the blue in v5. > > Also, I think in both parallel input and mc notifier registration, the > notifier should be cleaned up to release the parsed async subdevices > memory allocated by > v4l2_async_notifier_parse_fwnode_endpoints_by_port() if the > sub-sequent v4l2_async_notifier_register() fails. I agree with you here as well. That this memory should be cleaned up, nice catch. > > That would be: > > @@ -781,18 +782,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) > &vin->group->notifier); > if (ret < 0) { > vin_err(vin, "Notifier registration failed\n"); > - return ret; > + goto error_clean_up_notifier; > } > > return 0; > + > +error_clean_up_notifier: > + v4l2_async_notifier_cleanup(&vin->group->notifier); > + > + return ret; > } > > in both mc and parallel initialization functions. > > With your ack I can send two patches on top of the currently merged VIN > version, and rebase my series on top eventually. > > Sorry for the long email. > > Thanks > j > > > > > -- > > > 2.7.4 > > > > > > > -- > > Regards, > > Niklas Söderlund
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index a799684..29619c2 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); vin->parallel->sink_pad = ret < 0 ? 0 : ret; + if (vin->info->use_mc) { + vin->parallel->subdev = subdev; + return 0; + } + /* Find compatible subdevices mbus format */ vin->mbus_code = 0; code.index = 0; @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) { rvin_v4l2_unregister(vin); - v4l2_ctrl_handler_free(&vin->ctrl_handler); - - vin->vdev.ctrl_handler = NULL; vin->parallel->subdev = NULL; + + if (!vin->info->use_mc) { + v4l2_ctrl_handler_free(&vin->ctrl_handler); + vin->vdev.ctrl_handler = NULL; + } } static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev, return 0; } -static int rvin_parallel_graph_init(struct rvin_dev *vin) +static int rvin_parallel_init(struct rvin_dev *vin) { int ret; - ret = v4l2_async_notifier_parse_fwnode_endpoints( - vin->dev, &vin->notifier, - sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2); + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( + vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity), + 0, rvin_parallel_parse_v4l2); if (ret) return ret; if (!vin->parallel) - return -ENODEV; + return -ENOTCONN; vin_dbg(vin, "Found parallel subdevice %pOF\n", to_of_node(vin->parallel->asd.match.fwnode)); @@ -784,14 +791,8 @@ static int rvin_mc_init(struct rvin_dev *vin) { int ret; - vin->pad.flags = MEDIA_PAD_FL_SINK; - ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); - if (ret) - return ret; - - ret = rvin_group_get(vin); - if (ret) - return ret; + if (!vin->info->use_mc) + return 0; ret = rvin_mc_parse_of_graph(vin); if (ret) @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev) return ret; platform_set_drvdata(pdev, vin); - if (vin->info->use_mc) - ret = rvin_mc_init(vin); - else - ret = rvin_parallel_graph_init(vin); - if (ret < 0) + + if (vin->info->use_mc) { + vin->pad.flags = MEDIA_PAD_FL_SINK; + ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad); + if (ret) + return ret; + + ret = rvin_group_get(vin); + if (ret) + return ret; + } + + ret = rvin_mc_init(vin); + if (ret) + return ret; + + ret = rvin_parallel_init(vin); + if (ret < 0 && ret != -ENOTCONN) goto error; pm_suspend_ignore_children(&pdev->dev, true);
The rcar-vin driver so far had a mutually exclusive code path for handling parallel and CSI-2 video input subdevices, with only the CSI-2 use case supporting media-controller. As we add support for parallel inputs to Gen3 media-controller compliant code path now parse both port@0 and port@1, handling the media-controller use case in the parallel bound/unbind notifier operations. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- v3 -> v4: - Change the mc/parallel initialization order. Initialize mc first, then parallel - As a consequence no need to delay parallel notifiers registration, the media controller is set up already when parallel input got parsed, this greatly simplify the group notifier complete callback. --- drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++----------- 1 file changed, 35 insertions(+), 21 deletions(-)