| Message ID | 20210105152852.5733-32-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: Miscellaneous fixes and cleanups for i.MX7 | expand |
Hi Laurent, On 1/5/21 7:28 AM, Laurent Pinchart wrote: > When the subdevice connected to the capture device has a single possible > sink, there's no point in making the link mutable. Support creating > immutable links. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +- > drivers/staging/media/imx/imx-media-capture.c | 7 +++++-- > drivers/staging/media/imx/imx-media-csi.c | 2 +- > drivers/staging/media/imx/imx-media.h | 3 ++- > drivers/staging/media/imx/imx7-media-csi.c | 2 +- > 5 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c > index 88d69425e1b3..6c9c75ffb30c 100644 > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c > @@ -1269,7 +1269,7 @@ static int prp_registered(struct v4l2_subdev *sd) > if (IS_ERR(priv->vdev)) > return PTR_ERR(priv->vdev); > > - ret = imx_media_capture_device_register(priv->vdev); > + ret = imx_media_capture_device_register(priv->vdev, false); Might as well go ahead and pass true here now, to make the prpenc and prpvf links to the capture device immutable, since there is only one source and sink in this case. Steve > if (ret) > goto remove_vdev; > > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c > index 04eb612ff1fa..c6991e8f151c 100644 > --- a/drivers/staging/media/imx/imx-media-capture.c > +++ b/drivers/staging/media/imx/imx-media-capture.c > @@ -898,12 +898,14 @@ static int capture_init_format(struct capture_priv *priv) > return 0; > } > > -int imx_media_capture_device_register(struct imx_media_video_dev *vdev) > +int imx_media_capture_device_register(struct imx_media_video_dev *vdev, > + bool immutable) > { > struct capture_priv *priv = to_capture_priv(vdev); > struct v4l2_subdev *sd = priv->src_sd; > struct v4l2_device *v4l2_dev = sd->v4l2_dev; > struct video_device *vfd = vdev->vfd; > + u32 flags; > int ret; > > /* get media device */ > @@ -927,8 +929,9 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev) > video_device_node_name(vfd)); > > /* Create the link from the src_sd devnode pad to device node. */ > + flags = immutable ? MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE : 0; > ret = media_create_pad_link(&sd->entity, priv->src_sd_pad, > - &vfd->entity, 0, 0); > + &vfd->entity, 0, flags); > if (ret) { > dev_err(priv->dev, "failed to create link to device node\n"); > video_unregister_device(vfd); > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index 436f3d7160fa..d54d2a3789c0 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -1796,7 +1796,7 @@ static int csi_registered(struct v4l2_subdev *sd) > goto free_fim; > } > > - ret = imx_media_capture_device_register(priv->vdev); > + ret = imx_media_capture_device_register(priv->vdev, false); > if (ret) > goto remove_vdev; > > diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h > index 16ab879e0084..4efc4d186c0a 100644 > --- a/drivers/staging/media/imx/imx-media.h > +++ b/drivers/staging/media/imx/imx-media.h > @@ -288,7 +288,8 @@ struct imx_media_video_dev * > imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd, > int pad, bool legacy_api); > void imx_media_capture_device_remove(struct imx_media_video_dev *vdev); > -int imx_media_capture_device_register(struct imx_media_video_dev *vdev); > +int imx_media_capture_device_register(struct imx_media_video_dev *vdev, > + bool immutable); > void imx_media_capture_device_unregister(struct imx_media_video_dev *vdev); > struct imx_media_buffer * > imx_media_capture_device_next_buf(struct imx_media_video_dev *vdev); > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > index afd1a7e35bfe..c087a212efdd 100644 > --- a/drivers/staging/media/imx/imx7-media-csi.c > +++ b/drivers/staging/media/imx/imx7-media-csi.c > @@ -1093,7 +1093,7 @@ static int imx7_csi_registered(struct v4l2_subdev *sd) > if (IS_ERR(csi->vdev)) > return PTR_ERR(csi->vdev); > > - ret = imx_media_capture_device_register(csi->vdev); > + ret = imx_media_capture_device_register(csi->vdev, false); > if (ret) > imx_media_capture_device_remove(csi->vdev); >
Hi Laurent, On Tue, 2021-01-05 at 17:28 +0200, Laurent Pinchart wrote: > When the subdevice connected to the capture device has a single possible > sink, there's no point in making the link mutable. Support creating > immutable links. > Does this apply to csi2_notify_bound as welll that is, is there any point in making the sensor link mutable? > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +- > drivers/staging/media/imx/imx-media-capture.c | 7 +++++-- > drivers/staging/media/imx/imx-media-csi.c | 2 +- > drivers/staging/media/imx/imx-media.h | 3 ++- > drivers/staging/media/imx/imx7-media-csi.c | 2 +- > 5 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c > index 88d69425e1b3..6c9c75ffb30c 100644 > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c > @@ -1269,7 +1269,7 @@ static int prp_registered(struct v4l2_subdev *sd) > if (IS_ERR(priv->vdev)) > return PTR_ERR(priv->vdev); > > - ret = imx_media_capture_device_register(priv->vdev); > + ret = imx_media_capture_device_register(priv->vdev, false); Maybe it's just me, but I dislike this boolean parameter pattern, as opposed to passing the flags directly, or some other MUTABLE/IMMUTABLE enum value. Cheers, Ezequiel
Hi Steve, On Wed, Jan 06, 2021 at 09:44:37AM -0800, Steve Longerbeam wrote: > On 1/5/21 7:28 AM, Laurent Pinchart wrote: > > When the subdevice connected to the capture device has a single possible > > sink, there's no point in making the link mutable. Support creating > > immutable links. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +- > > drivers/staging/media/imx/imx-media-capture.c | 7 +++++-- > > drivers/staging/media/imx/imx-media-csi.c | 2 +- > > drivers/staging/media/imx/imx-media.h | 3 ++- > > drivers/staging/media/imx/imx7-media-csi.c | 2 +- > > 5 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c > > index 88d69425e1b3..6c9c75ffb30c 100644 > > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c > > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c > > @@ -1269,7 +1269,7 @@ static int prp_registered(struct v4l2_subdev *sd) > > if (IS_ERR(priv->vdev)) > > return PTR_ERR(priv->vdev); > > > > - ret = imx_media_capture_device_register(priv->vdev); > > + ret = imx_media_capture_device_register(priv->vdev, false); > > Might as well go ahead and pass true here now, to make the prpenc and > prpvf links to the capture device immutable, since there is only one > source and sink in this case. This patch only adds the infrastructure to create immutable links, but doesn't make use of it. Subsequent patches change that. I can add another patch for the prpenc and prpvf. > > if (ret) > > goto remove_vdev; > > > > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c > > index 04eb612ff1fa..c6991e8f151c 100644 > > --- a/drivers/staging/media/imx/imx-media-capture.c > > +++ b/drivers/staging/media/imx/imx-media-capture.c > > @@ -898,12 +898,14 @@ static int capture_init_format(struct capture_priv *priv) > > return 0; > > } > > > > -int imx_media_capture_device_register(struct imx_media_video_dev *vdev) > > +int imx_media_capture_device_register(struct imx_media_video_dev *vdev, > > + bool immutable) > > { > > struct capture_priv *priv = to_capture_priv(vdev); > > struct v4l2_subdev *sd = priv->src_sd; > > struct v4l2_device *v4l2_dev = sd->v4l2_dev; > > struct video_device *vfd = vdev->vfd; > > + u32 flags; > > int ret; > > > > /* get media device */ > > @@ -927,8 +929,9 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev) > > video_device_node_name(vfd)); > > > > /* Create the link from the src_sd devnode pad to device node. */ > > + flags = immutable ? MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE : 0; > > ret = media_create_pad_link(&sd->entity, priv->src_sd_pad, > > - &vfd->entity, 0, 0); > > + &vfd->entity, 0, flags); > > if (ret) { > > dev_err(priv->dev, "failed to create link to device node\n"); > > video_unregister_device(vfd); > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > > index 436f3d7160fa..d54d2a3789c0 100644 > > --- a/drivers/staging/media/imx/imx-media-csi.c > > +++ b/drivers/staging/media/imx/imx-media-csi.c > > @@ -1796,7 +1796,7 @@ static int csi_registered(struct v4l2_subdev *sd) > > goto free_fim; > > } > > > > - ret = imx_media_capture_device_register(priv->vdev); > > + ret = imx_media_capture_device_register(priv->vdev, false); > > if (ret) > > goto remove_vdev; > > > > diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h > > index 16ab879e0084..4efc4d186c0a 100644 > > --- a/drivers/staging/media/imx/imx-media.h > > +++ b/drivers/staging/media/imx/imx-media.h > > @@ -288,7 +288,8 @@ struct imx_media_video_dev * > > imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd, > > int pad, bool legacy_api); > > void imx_media_capture_device_remove(struct imx_media_video_dev *vdev); > > -int imx_media_capture_device_register(struct imx_media_video_dev *vdev); > > +int imx_media_capture_device_register(struct imx_media_video_dev *vdev, > > + bool immutable); > > void imx_media_capture_device_unregister(struct imx_media_video_dev *vdev); > > struct imx_media_buffer * > > imx_media_capture_device_next_buf(struct imx_media_video_dev *vdev); > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > > index afd1a7e35bfe..c087a212efdd 100644 > > --- a/drivers/staging/media/imx/imx7-media-csi.c > > +++ b/drivers/staging/media/imx/imx7-media-csi.c > > @@ -1093,7 +1093,7 @@ static int imx7_csi_registered(struct v4l2_subdev *sd) > > if (IS_ERR(csi->vdev)) > > return PTR_ERR(csi->vdev); > > > > - ret = imx_media_capture_device_register(csi->vdev); > > + ret = imx_media_capture_device_register(csi->vdev, false); > > if (ret) > > imx_media_capture_device_remove(csi->vdev); > >
Hi Ezequiel, On Fri, Jan 08, 2021 at 02:37:54PM -0300, Ezequiel Garcia wrote: > Hi Laurent, > > On Tue, 2021-01-05 at 17:28 +0200, Laurent Pinchart wrote: > > When the subdevice connected to the capture device has a single possible > > sink, there's no point in making the link mutable. Support creating > > immutable links. > > Does this apply to csi2_notify_bound as welll > that is, is there any point in making the sensor link mutable? We could have multiple sensors connected to the same CSI-2 receiver, with all but one always in reset (not a recommended hardware design, but it has been done and can work in some cases). > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +- > > drivers/staging/media/imx/imx-media-capture.c | 7 +++++-- > > drivers/staging/media/imx/imx-media-csi.c | 2 +- > > drivers/staging/media/imx/imx-media.h | 3 ++- > > drivers/staging/media/imx/imx7-media-csi.c | 2 +- > > 5 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c > > index 88d69425e1b3..6c9c75ffb30c 100644 > > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c > > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c > > @@ -1269,7 +1269,7 @@ static int prp_registered(struct v4l2_subdev *sd) > > if (IS_ERR(priv->vdev)) > > return PTR_ERR(priv->vdev); > > > > - ret = imx_media_capture_device_register(priv->vdev); > > + ret = imx_media_capture_device_register(priv->vdev, false); > > Maybe it's just me, but I dislike this boolean parameter pattern, > as opposed to passing the flags directly, or some other MUTABLE/IMMUTABLE > enum value. Something along these lines ? diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c index 88d69425e1b3..d990553de87b 100644 --- a/drivers/staging/media/imx/imx-ic-prpencvf.c +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c @@ -1269,7 +1269,7 @@ static int prp_registered(struct v4l2_subdev *sd) if (IS_ERR(priv->vdev)) return PTR_ERR(priv->vdev); - ret = imx_media_capture_device_register(priv->vdev); + ret = imx_media_capture_device_register(priv->vdev, 0); if (ret) goto remove_vdev; diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c index 04eb612ff1fa..a17e087a0d02 100644 --- a/drivers/staging/media/imx/imx-media-capture.c +++ b/drivers/staging/media/imx/imx-media-capture.c @@ -898,7 +898,9 @@ static int capture_init_format(struct capture_priv *priv) return 0; } -int imx_media_capture_device_register(struct imx_media_video_dev *vdev) +int imx_media_capture_device_register(struct imx_media_video_dev *vdev, + u32 link_flags) + bool immutable) { struct capture_priv *priv = to_capture_priv(vdev); struct v4l2_subdev *sd = priv->src_sd; @@ -927,8 +929,10 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev) video_device_node_name(vfd)); /* Create the link from the src_sd devnode pad to device node. */ + if (link_flags & MEDIA_LNK_FL_IMMUTABLE) + link_flags |= MEDIA_LNK_FL_ENABLED; ret = media_create_pad_link(&sd->entity, priv->src_sd_pad, - &vfd->entity, 0, 0); + &vfd->entity, 0, link_flags); if (ret) { dev_err(priv->dev, "failed to create link to device node\n"); video_unregister_device(vfd); diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index cfad000d9352..ce64f3da81f7 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -1788,7 +1788,7 @@ static int csi_registered(struct v4l2_subdev *sd) goto free_fim; } - ret = imx_media_capture_device_register(priv->vdev); + ret = imx_media_capture_device_register(priv->vdev, 0); if (ret) goto remove_vdev; diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h index 2ea2e62ddf86..492d9a64e704 100644 --- a/drivers/staging/media/imx/imx-media.h +++ b/drivers/staging/media/imx/imx-media.h @@ -288,7 +288,8 @@ struct imx_media_video_dev * imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd, int pad, bool legacy_api); void imx_media_capture_device_remove(struct imx_media_video_dev *vdev); -int imx_media_capture_device_register(struct imx_media_video_dev *vdev); +int imx_media_capture_device_register(struct imx_media_video_dev *vdev, + u32 link_flags); void imx_media_capture_device_unregister(struct imx_media_video_dev *vdev); struct imx_media_buffer * imx_media_capture_device_next_buf(struct imx_media_video_dev *vdev); diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c index afd1a7e35bfe..1b13347bd0a0 100644 --- a/drivers/staging/media/imx/imx7-media-csi.c +++ b/drivers/staging/media/imx/imx7-media-csi.c @@ -1093,7 +1093,7 @@ static int imx7_csi_registered(struct v4l2_subdev *sd) if (IS_ERR(csi->vdev)) return PTR_ERR(csi->vdev); - ret = imx_media_capture_device_register(csi->vdev); + ret = imx_media_capture_device_register(csi->vdev, 0); if (ret) imx_media_capture_device_remove(csi->vdev);
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c index 88d69425e1b3..6c9c75ffb30c 100644 --- a/drivers/staging/media/imx/imx-ic-prpencvf.c +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c @@ -1269,7 +1269,7 @@ static int prp_registered(struct v4l2_subdev *sd) if (IS_ERR(priv->vdev)) return PTR_ERR(priv->vdev); - ret = imx_media_capture_device_register(priv->vdev); + ret = imx_media_capture_device_register(priv->vdev, false); if (ret) goto remove_vdev; diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c index 04eb612ff1fa..c6991e8f151c 100644 --- a/drivers/staging/media/imx/imx-media-capture.c +++ b/drivers/staging/media/imx/imx-media-capture.c @@ -898,12 +898,14 @@ static int capture_init_format(struct capture_priv *priv) return 0; } -int imx_media_capture_device_register(struct imx_media_video_dev *vdev) +int imx_media_capture_device_register(struct imx_media_video_dev *vdev, + bool immutable) { struct capture_priv *priv = to_capture_priv(vdev); struct v4l2_subdev *sd = priv->src_sd; struct v4l2_device *v4l2_dev = sd->v4l2_dev; struct video_device *vfd = vdev->vfd; + u32 flags; int ret; /* get media device */ @@ -927,8 +929,9 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev) video_device_node_name(vfd)); /* Create the link from the src_sd devnode pad to device node. */ + flags = immutable ? MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE : 0; ret = media_create_pad_link(&sd->entity, priv->src_sd_pad, - &vfd->entity, 0, 0); + &vfd->entity, 0, flags); if (ret) { dev_err(priv->dev, "failed to create link to device node\n"); video_unregister_device(vfd); diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index 436f3d7160fa..d54d2a3789c0 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -1796,7 +1796,7 @@ static int csi_registered(struct v4l2_subdev *sd) goto free_fim; } - ret = imx_media_capture_device_register(priv->vdev); + ret = imx_media_capture_device_register(priv->vdev, false); if (ret) goto remove_vdev; diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h index 16ab879e0084..4efc4d186c0a 100644 --- a/drivers/staging/media/imx/imx-media.h +++ b/drivers/staging/media/imx/imx-media.h @@ -288,7 +288,8 @@ struct imx_media_video_dev * imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd, int pad, bool legacy_api); void imx_media_capture_device_remove(struct imx_media_video_dev *vdev); -int imx_media_capture_device_register(struct imx_media_video_dev *vdev); +int imx_media_capture_device_register(struct imx_media_video_dev *vdev, + bool immutable); void imx_media_capture_device_unregister(struct imx_media_video_dev *vdev); struct imx_media_buffer * imx_media_capture_device_next_buf(struct imx_media_video_dev *vdev); diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c index afd1a7e35bfe..c087a212efdd 100644 --- a/drivers/staging/media/imx/imx7-media-csi.c +++ b/drivers/staging/media/imx/imx7-media-csi.c @@ -1093,7 +1093,7 @@ static int imx7_csi_registered(struct v4l2_subdev *sd) if (IS_ERR(csi->vdev)) return PTR_ERR(csi->vdev); - ret = imx_media_capture_device_register(csi->vdev); + ret = imx_media_capture_device_register(csi->vdev, false); if (ret) imx_media_capture_device_remove(csi->vdev);
When the subdevice connected to the capture device has a single possible sink, there's no point in making the link mutable. Support creating immutable links. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +- drivers/staging/media/imx/imx-media-capture.c | 7 +++++-- drivers/staging/media/imx/imx-media-csi.c | 2 +- drivers/staging/media/imx/imx-media.h | 3 ++- drivers/staging/media/imx/imx7-media-csi.c | 2 +- 5 files changed, 10 insertions(+), 6 deletions(-)