[PATCH] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to rzg2l_cru_video_register()

[PATCH] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to rzg2l_cru_video_register()

[Biju Das]

Move request_irq() to rzg2l_cru_video_register(), in order to avoid
requesting IRQ during device start which happens frequently.

Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 35 ++++++++++---------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index b16b8af6e8f8..78fc5ae6c8af 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -637,13 +637,6 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 		goto assert_aresetn;
 	}
 
-	ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
-			  IRQF_SHARED, KBUILD_MODNAME, cru);
-	if (ret) {
-		dev_err(cru->dev, "failed to request irq\n");
-		goto assert_presetn;
-	}
-
 	/* Allocate scratch buffer. */
 	cru->scratch = dma_alloc_coherent(cru->dev, cru->format.sizeimage,
 					  &cru->scratch_phys, GFP_KERNEL);
@@ -651,7 +644,7 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 		return_unused_buffers(cru, VB2_BUF_STATE_QUEUED);
 		dev_err(cru->dev, "Failed to allocate scratch buffer\n");
 		ret = -ENOMEM;
-		goto free_image_conv_irq;
+		goto assert_presetn;
 	}
 
 	cru->sequence = 0;
@@ -670,9 +663,6 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 	if (ret)
 		dma_free_coherent(cru->dev, cru->format.sizeimage, cru->scratch,
 				  cru->scratch_phys);
-free_image_conv_irq:
-	free_irq(cru->image_conv_irq, cru);
-
 assert_presetn:
 	reset_control_assert(cru->presetn);
 
@@ -698,7 +688,6 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq)
 	dma_free_coherent(cru->dev, cru->format.sizeimage,
 			  cru->scratch, cru->scratch_phys);
 
-	free_irq(cru->image_conv_irq, cru);
 	return_unused_buffers(cru, VB2_BUF_STATE_ERROR);
 
 	reset_control_assert(cru->presetn);
@@ -1011,6 +1000,7 @@ void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru)
 {
 	media_device_unregister(&cru->mdev);
 	video_unregister_device(&cru->vdev);
+	free_irq(cru->image_conv_irq, cru);
 }
 
 int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
@@ -1018,6 +1008,13 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
 	struct video_device *vdev = &cru->vdev;
 	int ret;
 
+	ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
+			  IRQF_SHARED, KBUILD_MODNAME, cru);
+	if (ret) {
+		dev_err(cru->dev, "failed to request irq\n");
+		return ret;
+	}
+
 	if (video_is_registered(&cru->vdev)) {
 		struct media_entity *entity;
 
@@ -1032,14 +1029,18 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
 	if (ret) {
 		dev_err(cru->dev, "Failed to register video device\n");
-		return ret;
+		goto err_request_irq;
 	}
 
 	ret = media_device_register(&cru->mdev);
-	if (ret) {
-		video_unregister_device(&cru->vdev);
-		return ret;
-	}
+	if (ret)
+		goto err_video_unregister;
 
 	return 0;
+
+err_video_unregister:
+	video_unregister_device(&cru->vdev);
+err_request_irq:
+	free_irq(cru->image_conv_irq, cru);
+	return ret;
 }
-- 
2.25.1

Re: [PATCH] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to rzg2l_cru_video_register()

[Geert Uytterhoeven]

Hi Biju,

On Mon, Feb 19, 2024 at 7:05 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Move request_irq() to rzg2l_cru_video_register(), in order to avoid
> requesting IRQ during device start which happens frequently.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c

> @@ -1011,6 +1000,7 @@ void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru)
>  {
>         media_device_unregister(&cru->mdev);
>         video_unregister_device(&cru->vdev);
> +       free_irq(cru->image_conv_irq, cru);
>  }
>
>  int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> @@ -1018,6 +1008,13 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
>         struct video_device *vdev = &cru->vdev;
>         int ret;
>
> +       ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
> +                         IRQF_SHARED, KBUILD_MODNAME, cru);
> +       if (ret) {
> +               dev_err(cru->dev, "failed to request irq\n");
> +               return ret;
> +       }
> +
>         if (video_is_registered(&cru->vdev)) {

How can this happen? Perhaps rzg2l_cru_video_register() can be called
multiple times through the rzg2l_cru_group_notify_complete() notifier?

If that is true, the request_irq() should be moved after this block,
just before the call to video_register_device() below.

>                 struct media_entity *entity;
>
> @@ -1032,14 +1029,18 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
>         ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>         if (ret) {
>                 dev_err(cru->dev, "Failed to register video device\n");
> -               return ret;
> +               goto err_request_irq;
>         }
>
>         ret = media_device_register(&cru->mdev);
> -       if (ret) {
> -               video_unregister_device(&cru->vdev);
> -               return ret;
> -       }
> +       if (ret)
> +               goto err_video_unregister;
>
>         return 0;
> +
> +err_video_unregister:
> +       video_unregister_device(&cru->vdev);
> +err_request_irq:
> +       free_irq(cru->image_conv_irq, cru);
> +       return ret;
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to rzg2l_cru_video_register()

[Laurent Pinchart]

On Thu, Apr 18, 2024 at 11:06:27AM +0200, Geert Uytterhoeven wrote:
> Hi Biju,
> 
> On Mon, Feb 19, 2024 at 7:05 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Move request_irq() to rzg2l_cru_video_register(), in order to avoid
> > requesting IRQ during device start which happens frequently.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> 
> > @@ -1011,6 +1000,7 @@ void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru)
> >  {
> >         media_device_unregister(&cru->mdev);
> >         video_unregister_device(&cru->vdev);
> > +       free_irq(cru->image_conv_irq, cru);
> >  }
> >
> >  int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > @@ -1018,6 +1008,13 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> >         struct video_device *vdev = &cru->vdev;
> >         int ret;
> >
> > +       ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
> > +                         IRQF_SHARED, KBUILD_MODNAME, cru);
> > +       if (ret) {
> > +               dev_err(cru->dev, "failed to request irq\n");
> > +               return ret;
> > +       }
> > +
> >         if (video_is_registered(&cru->vdev)) {
> 
> How can this happen? Perhaps rzg2l_cru_video_register() can be called
> multiple times through the rzg2l_cru_group_notify_complete() notifier?

The notifier completion handler shouldn't be called multiple times, no.
There's however a possibility (I think) that a subdev could disappear of
the device is unbound from its driver. If the device is later rebound,
the notifier completion handler could be called again.

The issue is that rzg2l_cru_video_unregister() is called from .remove().
I think a better fix would be to request the IRQ at probe time (or did
we discuss that previously and concluded it could cause issues ?). I
would also argue that the video devices should be registered at probe
time, not in the notifier completion handler.

> If that is true, the request_irq() should be moved after this block,
> just before the call to video_register_device() below.
> 
> >                 struct media_entity *entity;
> >
> > @@ -1032,14 +1029,18 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> >         ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> >         if (ret) {
> >                 dev_err(cru->dev, "Failed to register video device\n");
> > -               return ret;
> > +               goto err_request_irq;
> >         }
> >
> >         ret = media_device_register(&cru->mdev);
> > -       if (ret) {
> > -               video_unregister_device(&cru->vdev);
> > -               return ret;
> > -       }
> > +       if (ret)
> > +               goto err_video_unregister;
> >
> >         return 0;
> > +
> > +err_video_unregister:
> > +       video_unregister_device(&cru->vdev);
> > +err_request_irq:
> > +       free_irq(cru->image_conv_irq, cru);
> > +       return ret;
> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to rzg2l_cru_video_register()

[Niklas Söderlund]

Hello,

On 2024-04-21 16:46:30 +0300, Laurent Pinchart wrote:
> On Thu, Apr 18, 2024 at 11:06:27AM +0200, Geert Uytterhoeven wrote:
> > Hi Biju,
> > 
> > On Mon, Feb 19, 2024 at 7:05 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Move request_irq() to rzg2l_cru_video_register(), in order to avoid
> > > requesting IRQ during device start which happens frequently.
> > >
> > > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > 
> > Thanks for your patch!
> > 
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > 
> > > @@ -1011,6 +1000,7 @@ void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru)
> > >  {
> > >         media_device_unregister(&cru->mdev);
> > >         video_unregister_device(&cru->vdev);
> > > +       free_irq(cru->image_conv_irq, cru);
> > >  }
> > >
> > >  int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > > @@ -1018,6 +1008,13 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > >         struct video_device *vdev = &cru->vdev;
> > >         int ret;
> > >
> > > +       ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
> > > +                         IRQF_SHARED, KBUILD_MODNAME, cru);
> > > +       if (ret) {
> > > +               dev_err(cru->dev, "failed to request irq\n");
> > > +               return ret;
> > > +       }
> > > +
> > >         if (video_is_registered(&cru->vdev)) {
> > 
> > How can this happen? Perhaps rzg2l_cru_video_register() can be called
> > multiple times through the rzg2l_cru_group_notify_complete() notifier?
> 
> The notifier completion handler shouldn't be called multiple times, no.
> There's however a possibility (I think) that a subdev could disappear of
> the device is unbound from its driver. If the device is later rebound,
> the notifier completion handler could be called again.
> 
> The issue is that rzg2l_cru_video_unregister() is called from .remove().
> I think a better fix would be to request the IRQ at probe time (or did
> we discuss that previously and concluded it could cause issues ?). I
> would also argue that the video devices should be registered at probe
> time, not in the notifier completion handler.

FWIW, I intend to have another go at moving video device registration to 
probe time for VIN once the current cleanup-patches on the list are 
processed. Last time I tired a few years ago it was rejected with the 
reason video devices should be register when all subdevices are 
available.

Now days other drivers upstream register video devices at probe time so 
I hope this is now (finally) OK.

> 
> > If that is true, the request_irq() should be moved after this block,
> > just before the call to video_register_device() below.
> > 
> > >                 struct media_entity *entity;
> > >
> > > @@ -1032,14 +1029,18 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > >         ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > >         if (ret) {
> > >                 dev_err(cru->dev, "Failed to register video device\n");
> > > -               return ret;
> > > +               goto err_request_irq;
> > >         }
> > >
> > >         ret = media_device_register(&cru->mdev);
> > > -       if (ret) {
> > > -               video_unregister_device(&cru->vdev);
> > > -               return ret;
> > > -       }
> > > +       if (ret)
> > > +               goto err_video_unregister;
> > >
> > >         return 0;
> > > +
> > > +err_video_unregister:
> > > +       video_unregister_device(&cru->vdev);
> > > +err_request_irq:
> > > +       free_irq(cru->image_conv_irq, cru);
> > > +       return ret;
> > >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Kind Regards,
Niklas Söderlund