From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
Biju Das <biju.das.jz@bp.renesas.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Benjamin Gaignard <benjamin.gaignard@collabora.com>,
linux-media@vger.kernel.org,
Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
Biju Das <biju.das.au@gmail.com>,
linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to rzg2l_cru_video_register()
Date: Sun, 21 Apr 2024 16:30:33 +0200 [thread overview]
Message-ID: <20240421143033.GA1105869@ragnatech.se> (raw)
In-Reply-To: <20240421134630.GA29222@pendragon.ideasonboard.com>
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
prev parent reply other threads:[~2024-04-21 14:30 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-19 18:05 [PATCH] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to rzg2l_cru_video_register() Biju Das
2024-04-18 9:06 ` Geert Uytterhoeven
2024-04-21 13:46 ` Laurent Pinchart
2024-04-21 14:30 ` Niklas Söderlund [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240421143033.GA1105869@ragnatech.se \
--to=niklas.soderlund@ragnatech.se \
--cc=benjamin.gaignard@collabora.com \
--cc=biju.das.au@gmail.com \
--cc=biju.das.jz@bp.renesas.com \
--cc=geert@linux-m68k.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).