diff mbox series

[v2,01/17] media: rkisp1: capture: Initialize entity before video device

Message ID 20220304171925.1592-2-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: rkisp1: Misc bug fixes and cleanups | expand

Commit Message

Laurent Pinchart March 4, 2022, 5:19 p.m. UTC 
The media_entity embedded in the video_device needs to be initialized
before registering the video_device. Do so.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>
---
 .../platform/rockchip/rkisp1/rkisp1-capture.c   | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi March 7, 2022, 11:42 a.m. UTC | #1 
Hi Laurent

On Fri, Mar 04, 2022 at 07:19:09PM +0200, Laurent Pinchart wrote:
> The media_entity embedded in the video_device needs to be initialized
> before registering the video_device. Do so.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c   | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index c95c00a91180..9c11f2b8e5f5 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -1372,22 +1372,25 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
>
>  	vdev->queue = q;
>
> +	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> +	if (ret)
> +		return ret;
> +
>  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>  	if (ret) {
>  		dev_err(cap->rkisp1->dev,
>  			"failed to register %s, ret=%d\n", vdev->name, ret);
> -		return ret;
> +		goto error;
>  	}
> +
>  	v4l2_info(v4l2_dev, "registered %s as /dev/video%d\n", vdev->name,
>  		  vdev->num);
>
> -	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> -	if (ret) {
> -		video_unregister_device(vdev);
> -		return ret;
> -	}
> -
>  	return 0;
> +
> +error:
> +	media_entity_cleanup(&vdev->entity);
> +	return ret;
>  }
>
>  static void
> --
> Regards,
>
> Laurent Pinchart
>
Nicolas Dufresne March 7, 2022, 7:15 p.m. UTC | #2 
Hi Laurent,

Le vendredi 04 mars 2022 à 19:19 +0200, Laurent Pinchart a écrit :
> The media_entity embedded in the video_device needs to be initialized
> before registering the video_device. Do so.

We've seen the same bug in MTK vcodec topology. Any idea if that could be catch
this at lower level to prevent invalid path being populated in the media
controller over and over ?

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c   | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index c95c00a91180..9c11f2b8e5f5 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -1372,22 +1372,25 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
>  
>  	vdev->queue = q;
>  
> +	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> +	if (ret)
> +		return ret;
> +
>  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>  	if (ret) {
>  		dev_err(cap->rkisp1->dev,
>  			"failed to register %s, ret=%d\n", vdev->name, ret);
> -		return ret;
> +		goto error;
>  	}
> +
>  	v4l2_info(v4l2_dev, "registered %s as /dev/video%d\n", vdev->name,
>  		  vdev->num);
>  
> -	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> -	if (ret) {
> -		video_unregister_device(vdev);
> -		return ret;
> -	}
> -
>  	return 0;
> +
> +error:
> +	media_entity_cleanup(&vdev->entity);
> +	return ret;
>  }
>  
>  static void
Laurent Pinchart March 7, 2022, 7:25 p.m. UTC | #3 
Hi Nicolas,

On Mon, Mar 07, 2022 at 02:15:32PM -0500, Nicolas Dufresne wrote:
> Hi Laurent,
> 
> Le vendredi 04 mars 2022 à 19:19 +0200, Laurent Pinchart a écrit :
> > The media_entity embedded in the video_device needs to be initialized
> > before registering the video_device. Do so.
> 
> We've seen the same bug in MTK vcodec topology. Any idea if that could be catch
> this at lower level to prevent invalid path being populated in the media
> controller over and over ?

I think the pad should be moved to the video_device structure and
initialized unconditionally internally. It makes little sense to force
drivers to do this manually.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-capture.c   | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > index c95c00a91180..9c11f2b8e5f5 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -1372,22 +1372,25 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
> >  
> >  	vdev->queue = q;
> >  
> > +	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> >  	if (ret) {
> >  		dev_err(cap->rkisp1->dev,
> >  			"failed to register %s, ret=%d\n", vdev->name, ret);
> > -		return ret;
> > +		goto error;
> >  	}
> > +
> >  	v4l2_info(v4l2_dev, "registered %s as /dev/video%d\n", vdev->name,
> >  		  vdev->num);
> >  
> > -	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> > -	if (ret) {
> > -		video_unregister_device(vdev);
> > -		return ret;
> > -	}
> > -
> >  	return 0;
> > +
> > +error:
> > +	media_entity_cleanup(&vdev->entity);
> > +	return ret;
> >  }
> >  
> >  static void
diff mbox series

Patch

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index c95c00a91180..9c11f2b8e5f5 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -1372,22 +1372,25 @@  static int rkisp1_register_capture(struct rkisp1_capture *cap)
 
 	vdev->queue = q;
 
+	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
+	if (ret)
+		return ret;
+
 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
 	if (ret) {
 		dev_err(cap->rkisp1->dev,
 			"failed to register %s, ret=%d\n", vdev->name, ret);
-		return ret;
+		goto error;
 	}
+
 	v4l2_info(v4l2_dev, "registered %s as /dev/video%d\n", vdev->name,
 		  vdev->num);
 
-	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
-	if (ret) {
-		video_unregister_device(vdev);
-		return ret;
-	}
-
 	return 0;
+
+error:
+	media_entity_cleanup(&vdev->entity);
+	return ret;
 }
 
 static void