| Message ID | 20240824182120.320751-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [v3] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to probe() | expand |
Hi, Biju, On 24.08.2024 21:21, Biju Das wrote: > Move request_irq() to probe(), in order to avoid requesting IRQ during > device start which happens frequently. As this function is in probe(), it > is better to replace it with its devm variant for managing the resource > efficiently. > > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Dropped wrapper function rzg2l_cru_process_irq() and made > rzg2l_cru_irq() global. > * Added Rb tag from Laurent. > v1->v2: > * Updated commit header and description. > * Moved rzg2l_cru_irq from rzg2l-video.c->rzg2l-core.c and introduced > rzg2l_cru_process_irq() in video.c to process irq. > * Dropped image_conv_irq from struct rzg2l_cru_dev > * Replaced request_irq with its devm variant. > --- > .../media/platform/renesas/rzg2l-cru/rzg2l-core.c | 13 +++++++++---- > .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h | 6 ++---- > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 15 ++------------- > 3 files changed, 13 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > index 280efd2a8185..2a2907beb722 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > @@ -242,7 +242,7 @@ static int rzg2l_cru_media_init(struct rzg2l_cru_dev *cru) > static int rzg2l_cru_probe(struct platform_device *pdev) > { > struct rzg2l_cru_dev *cru; > - int ret; > + int irq, ret; > > cru = devm_kzalloc(&pdev->dev, sizeof(*cru), GFP_KERNEL); > if (!cru) > @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev) > cru->dev = &pdev->dev; > cru->info = of_device_get_match_data(&pdev->dev); > > - cru->image_conv_irq = platform_get_irq(pdev, 0); > - if (cru->image_conv_irq < 0) > - return cru->image_conv_irq; > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED, > + KBUILD_MODNAME, cru); Because this is requested w/ IRQF_SHARED the free_irq() -> __free_irq() [1] will call the IRQ handler to simulate an IRQ SHARE scenario where other device generate an interrupt. AFAICT, with the current code, because the driver has runtime PM disabled (and the clocks at that time should be disabled), the first register access in the IRQ handler will generate an abort (when called through free_irq() -> __free_irq()). Have you tried to unbind the driver with CONFIG_DEBUG_SHIRQ=y ? Thank you, Claudiu Beznea [1] https://elixir.bootlin.com/linux/v6.10.6/source/kernel/irq/manage.c#L1970 > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "failed to request irq\n"); > > platform_set_drvdata(pdev, cru); > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > index a5a99b004322..174760239548 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > @@ -8,6 +8,7 @@ > #ifndef __RZG2L_CRU__ > #define __RZG2L_CRU__ > > +#include <linux/irqreturn.h> > #include <linux/reset.h> > > #include <media/v4l2-async.h> > @@ -68,8 +69,6 @@ struct rzg2l_cru_ip { > * > * @vclk: CRU Main clock > * > - * @image_conv_irq: Holds image conversion interrupt number > - * > * @vdev: V4L2 video device associated with CRU > * @v4l2_dev: V4L2 device > * @num_buf: Holds the current number of buffers enabled > @@ -105,8 +104,6 @@ struct rzg2l_cru_dev { > > struct clk *vclk; > > - int image_conv_irq; > - > struct video_device vdev; > struct v4l2_device v4l2_dev; > u8 num_buf; > @@ -141,6 +138,7 @@ void rzg2l_cru_dma_unregister(struct rzg2l_cru_dev *cru); > > int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru); > void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru); > +irqreturn_t rzg2l_cru_irq(int irq, void *data); > > const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format); > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > index b16b8af6e8f8..e80bfb9fc1af 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > @@ -527,7 +527,7 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru) > rzg2l_cru_set_stream(cru, 0); > } > > -static irqreturn_t rzg2l_cru_irq(int irq, void *data) > +irqreturn_t rzg2l_cru_irq(int irq, void *data) > { > struct rzg2l_cru_dev *cru = data; > unsigned int handled = 0; > @@ -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);
Hi Claudiu, > -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > Sent: Monday, August 26, 2024 8:27 AM > Subject: Re: [PATCH v3] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to probe() > > Hi, Biju, > > On 24.08.2024 21:21, Biju Das wrote: > > Move request_irq() to probe(), in order to avoid requesting IRQ during > > device start which happens frequently. As this function is in probe(), > > it is better to replace it with its devm variant for managing the > > resource efficiently. > > > > Suggested-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > Reviewed-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v2->v3: > > * Dropped wrapper function rzg2l_cru_process_irq() and made > > rzg2l_cru_irq() global. > > * Added Rb tag from Laurent. > > v1->v2: > > * Updated commit header and description. > > * Moved rzg2l_cru_irq from rzg2l-video.c->rzg2l-core.c and introduced > > rzg2l_cru_process_irq() in video.c to process irq. > > * Dropped image_conv_irq from struct rzg2l_cru_dev > > * Replaced request_irq with its devm variant. > > --- > > .../media/platform/renesas/rzg2l-cru/rzg2l-core.c | 13 +++++++++---- > > .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h | 6 ++---- > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 15 ++------------- > > 3 files changed, 13 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > index 280efd2a8185..2a2907beb722 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > @@ -242,7 +242,7 @@ static int rzg2l_cru_media_init(struct > > rzg2l_cru_dev *cru) static int rzg2l_cru_probe(struct platform_device > > *pdev) { > > struct rzg2l_cru_dev *cru; > > - int ret; > > + int irq, ret; > > > > cru = devm_kzalloc(&pdev->dev, sizeof(*cru), GFP_KERNEL); > > if (!cru) > > @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev) > > cru->dev = &pdev->dev; > > cru->info = of_device_get_match_data(&pdev->dev); > > > > - cru->image_conv_irq = platform_get_irq(pdev, 0); > > - if (cru->image_conv_irq < 0) > > - return cru->image_conv_irq; > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) > > + return irq; > > + > > + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED, > > + KBUILD_MODNAME, cru); > > Because this is requested w/ IRQF_SHARED the free_irq() -> __free_irq() [1] will call the IRQ handler > to simulate an IRQ SHARE scenario where other device generate an interrupt. Currently CSI driver is not registered any interrupts and CRU is the single user. Cheers, Biju > > > + if (ret) > > + return dev_err_probe(&pdev->dev, ret, "failed to request irq\n"); > > > > platform_set_drvdata(pdev, cru); > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > index a5a99b004322..174760239548 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > @@ -8,6 +8,7 @@ > > #ifndef __RZG2L_CRU__ > > #define __RZG2L_CRU__ > > > > +#include <linux/irqreturn.h> > > #include <linux/reset.h> > > > > #include <media/v4l2-async.h> > > @@ -68,8 +69,6 @@ struct rzg2l_cru_ip { > > * > > * @vclk: CRU Main clock > > * > > - * @image_conv_irq: Holds image conversion interrupt number > > - * > > * @vdev: V4L2 video device associated with CRU > > * @v4l2_dev: V4L2 device > > * @num_buf: Holds the current number of buffers enabled > > @@ -105,8 +104,6 @@ struct rzg2l_cru_dev { > > > > struct clk *vclk; > > > > - int image_conv_irq; > > - > > struct video_device vdev; > > struct v4l2_device v4l2_dev; > > u8 num_buf; > > @@ -141,6 +138,7 @@ void rzg2l_cru_dma_unregister(struct rzg2l_cru_dev > > *cru); > > > > int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru); void > > rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru); > > +irqreturn_t rzg2l_cru_irq(int irq, void *data); > > > > const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 > > format); > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > index b16b8af6e8f8..e80bfb9fc1af 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > @@ -527,7 +527,7 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru) > > rzg2l_cru_set_stream(cru, 0); > > } > > > > -static irqreturn_t rzg2l_cru_irq(int irq, void *data) > > +irqreturn_t rzg2l_cru_irq(int irq, void *data) > > { > > struct rzg2l_cru_dev *cru = data; > > unsigned int handled = 0; > > @@ -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);
On Mon, Aug 26, 2024 at 08:08:33AM +0000, Biju Das wrote: > On Monday, August 26, 2024 8:27 AM, claudiu beznea wrote: > > On 24.08.2024 21:21, Biju Das wrote: > > > Move request_irq() to probe(), in order to avoid requesting IRQ during > > > device start which happens frequently. As this function is in probe(), > > > it is better to replace it with its devm variant for managing the > > > resource efficiently. > > > > > > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > v2->v3: > > > * Dropped wrapper function rzg2l_cru_process_irq() and made > > > rzg2l_cru_irq() global. > > > * Added Rb tag from Laurent. > > > v1->v2: > > > * Updated commit header and description. > > > * Moved rzg2l_cru_irq from rzg2l-video.c->rzg2l-core.c and introduced > > > rzg2l_cru_process_irq() in video.c to process irq. > > > * Dropped image_conv_irq from struct rzg2l_cru_dev > > > * Replaced request_irq with its devm variant. > > > --- > > > .../media/platform/renesas/rzg2l-cru/rzg2l-core.c | 13 +++++++++---- > > > .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h | 6 ++---- > > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 15 ++------------- > > > 3 files changed, 13 insertions(+), 21 deletions(-) > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > > index 280efd2a8185..2a2907beb722 100644 > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > > @@ -242,7 +242,7 @@ static int rzg2l_cru_media_init(struct rzg2l_cru_dev *cru) > > > static int rzg2l_cru_probe(struct platform_device *pdev) > > > { > > > struct rzg2l_cru_dev *cru; > > > - int ret; > > > + int irq, ret; > > > > > > cru = devm_kzalloc(&pdev->dev, sizeof(*cru), GFP_KERNEL); > > > if (!cru) > > > @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev) > > > cru->dev = &pdev->dev; > > > cru->info = of_device_get_match_data(&pdev->dev); > > > > > > - cru->image_conv_irq = platform_get_irq(pdev, 0); > > > - if (cru->image_conv_irq < 0) > > > - return cru->image_conv_irq; > > > + irq = platform_get_irq(pdev, 0); > > > + if (irq < 0) > > > + return irq; > > > + > > > + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED, > > > + KBUILD_MODNAME, cru); > > > > Because this is requested w/ IRQF_SHARED the free_irq() -> > > __free_irq() [1] will call the IRQ handler to simulate an IRQ SHARE > > scenario where other device generate an interrupt. Good point, I had missed that. > Currently CSI driver is not registered any interrupts and CRU is the single user. Regardless, the fact that the IRQ is requested with IRQF_SHARED means that the IRQ handler needs to be prepared to be called at any time from the point of registration to the point the IRQ is freed. This is tested by CONFIG_DEBUG_SHIRQ=y, which you should enable for testing. If you don't need to share the interrupt with any other device, you can drop the IRQF_SHARED. Otherwise, you will need to fix the issue properly. You can probably wrap the interrupt handling with pm_runtime_get_if_in_use() and pm_runtime_put() (hoping those functions can be called from interrupt context). On a side note, I also I wonder if this issue precludesusage of devm_request_irq() for shared interrupts, requiring calling free_irq() manually at remove time to control the sequence of cleanup operations. > > > + if (ret) > > > + return dev_err_probe(&pdev->dev, ret, "failed to request irq\n"); > > > > > > platform_set_drvdata(pdev, cru); > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > index a5a99b004322..174760239548 100644 > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > @@ -8,6 +8,7 @@ > > > #ifndef __RZG2L_CRU__ > > > #define __RZG2L_CRU__ > > > > > > +#include <linux/irqreturn.h> > > > #include <linux/reset.h> > > > > > > #include <media/v4l2-async.h> > > > @@ -68,8 +69,6 @@ struct rzg2l_cru_ip { > > > * > > > * @vclk: CRU Main clock > > > * > > > - * @image_conv_irq: Holds image conversion interrupt number > > > - * > > > * @vdev: V4L2 video device associated with CRU > > > * @v4l2_dev: V4L2 device > > > * @num_buf: Holds the current number of buffers enabled > > > @@ -105,8 +104,6 @@ struct rzg2l_cru_dev { > > > > > > struct clk *vclk; > > > > > > - int image_conv_irq; > > > - > > > struct video_device vdev; > > > struct v4l2_device v4l2_dev; > > > u8 num_buf; > > > @@ -141,6 +138,7 @@ void rzg2l_cru_dma_unregister(struct rzg2l_cru_dev *cru); > > > > > > int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru); > > > void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru); > > > +irqreturn_t rzg2l_cru_irq(int irq, void *data); > > > > > > const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format); > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > index b16b8af6e8f8..e80bfb9fc1af 100644 > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > @@ -527,7 +527,7 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru) > > > rzg2l_cru_set_stream(cru, 0); > > > } > > > > > > -static irqreturn_t rzg2l_cru_irq(int irq, void *data) > > > +irqreturn_t rzg2l_cru_irq(int irq, void *data) > > > { > > > struct rzg2l_cru_dev *cru = data; > > > unsigned int handled = 0; > > > @@ -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);
Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Monday, August 26, 2024 10:43 AM > Subject: Re: [PATCH v3] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to probe() > > On Mon, Aug 26, 2024 at 08:08:33AM +0000, Biju Das wrote: > > On Monday, August 26, 2024 8:27 AM, claudiu beznea wrote: > > > On 24.08.2024 21:21, Biju Das wrote: > > > > Move request_irq() to probe(), in order to avoid requesting IRQ > > > > during device start which happens frequently. As this function is > > > > in probe(), it is better to replace it with its devm variant for > > > > managing the resource efficiently. > > > > > > > > Suggested-by: Laurent Pinchart > > > > <laurent.pinchart+renesas@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart > > > > <laurent.pinchart+renesas@ideasonboard.com> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > --- > > > > v2->v3: > > > > * Dropped wrapper function rzg2l_cru_process_irq() and made > > > > rzg2l_cru_irq() global. > > > > * Added Rb tag from Laurent. > > > > v1->v2: > > > > * Updated commit header and description. > > > > * Moved rzg2l_cru_irq from rzg2l-video.c->rzg2l-core.c and introduced > > > > rzg2l_cru_process_irq() in video.c to process irq. > > > > * Dropped image_conv_irq from struct rzg2l_cru_dev > > > > * Replaced request_irq with its devm variant. > > > > --- > > > > .../media/platform/renesas/rzg2l-cru/rzg2l-core.c | 13 > > > > +++++++++---- .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h | 6 ++---- > > > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 15 ++------------- > > > > 3 files changed, 13 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > > > index 280efd2a8185..2a2907beb722 100644 > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > > > > @@ -242,7 +242,7 @@ static int rzg2l_cru_media_init(struct > > > > rzg2l_cru_dev *cru) static int rzg2l_cru_probe(struct > > > > platform_device *pdev) { > > > > struct rzg2l_cru_dev *cru; > > > > - int ret; > > > > + int irq, ret; > > > > > > > > cru = devm_kzalloc(&pdev->dev, sizeof(*cru), GFP_KERNEL); > > > > if (!cru) > > > > @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev) > > > > cru->dev = &pdev->dev; > > > > cru->info = of_device_get_match_data(&pdev->dev); > > > > > > > > - cru->image_conv_irq = platform_get_irq(pdev, 0); > > > > - if (cru->image_conv_irq < 0) > > > > - return cru->image_conv_irq; > > > > + irq = platform_get_irq(pdev, 0); > > > > + if (irq < 0) > > > > + return irq; > > > > + > > > > + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED, > > > > + KBUILD_MODNAME, cru); > > > > > > Because this is requested w/ IRQF_SHARED the free_irq() -> > > > __free_irq() [1] will call the IRQ handler to simulate an IRQ SHARE > > > scenario where other device generate an interrupt. > > Good point, I had missed that. > > > Currently CSI driver is not registered any interrupts and CRU is the single user. > > Regardless, the fact that the IRQ is requested with IRQF_SHARED means that the IRQ handler needs to be > prepared to be called at any time from the point of registration to the point the IRQ is freed. This > is tested by CONFIG_DEBUG_SHIRQ=y, which you should enable for testing. For single user, testing CONFIG_DEBUG_SHIRQ=y this does not make any sense. See [1] [1] https://elixir.bootlin.com/linux/v6.11-rc5/source/kernel/irq/manage.c#L1965 > > If you don't need to share the interrupt with any other device, you can drop the IRQF_SHARED. I will drop IRQF_SHARED flags instead as there is single user and will send RFC for dropping calling IRQ handler for single user with CONFIG_DEBUG_SHIRQ=y Please let me know is it fine for you? Cheers, Biju > Otherwise, you will need to fix the issue properly. You can probably wrap the interrupt handling with > pm_runtime_get_if_in_use() and pm_runtime_put() (hoping those functions can be called from interrupt > context). > > On a side note, I also I wonder if this issue precludesusage of > devm_request_irq() for shared interrupts, requiring calling free_irq() manually at remove time to > control the sequence of cleanup operations. > > > > > + if (ret) > > > > + return dev_err_probe(&pdev->dev, ret, "failed to request > > > > +irq\n"); > > > > > > > > platform_set_drvdata(pdev, cru); > > > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > > index a5a99b004322..174760239548 100644 > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > > @@ -8,6 +8,7 @@ > > > > #ifndef __RZG2L_CRU__ > > > > #define __RZG2L_CRU__ > > > > > > > > +#include <linux/irqreturn.h> > > > > #include <linux/reset.h> > > > > > > > > #include <media/v4l2-async.h> > > > > @@ -68,8 +69,6 @@ struct rzg2l_cru_ip { > > > > * > > > > * @vclk: CRU Main clock > > > > * > > > > - * @image_conv_irq: Holds image conversion interrupt number > > > > - * > > > > * @vdev: V4L2 video device associated with CRU > > > > * @v4l2_dev: V4L2 device > > > > * @num_buf: Holds the current number of buffers enabled > > > > @@ -105,8 +104,6 @@ struct rzg2l_cru_dev { > > > > > > > > struct clk *vclk; > > > > > > > > - int image_conv_irq; > > > > - > > > > struct video_device vdev; > > > > struct v4l2_device v4l2_dev; > > > > u8 num_buf; > > > > @@ -141,6 +138,7 @@ void rzg2l_cru_dma_unregister(struct > > > > rzg2l_cru_dev *cru); > > > > > > > > int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru); void > > > > rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru); > > > > +irqreturn_t rzg2l_cru_irq(int irq, void *data); > > > > > > > > const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 > > > > format); > > > > > > > > diff --git > > > > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > index b16b8af6e8f8..e80bfb9fc1af 100644 > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > @@ -527,7 +527,7 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru) > > > > rzg2l_cru_set_stream(cru, 0); > > > > } > > > > > > > > -static irqreturn_t rzg2l_cru_irq(int irq, void *data) > > > > +irqreturn_t rzg2l_cru_irq(int irq, void *data) > > > > { > > > > struct rzg2l_cru_dev *cru = data; > > > > unsigned int handled = 0; > > > > @@ -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); > > -- > Regards, > > Laurent Pinchart
Hi Biju, On Mon, Aug 26, 2024 at 11:50 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > On Mon, Aug 26, 2024 at 08:08:33AM +0000, Biju Das wrote: > > > On Monday, August 26, 2024 8:27 AM, claudiu beznea wrote: > > > > On 24.08.2024 21:21, Biju Das wrote: > > > > > @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev) > > > > > cru->dev = &pdev->dev; > > > > > cru->info = of_device_get_match_data(&pdev->dev); > > > > > > > > > > - cru->image_conv_irq = platform_get_irq(pdev, 0); > > > > > - if (cru->image_conv_irq < 0) > > > > > - return cru->image_conv_irq; > > > > > + irq = platform_get_irq(pdev, 0); > > > > > + if (irq < 0) > > > > > + return irq; > > > > > + > > > > > + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED, > > > > > + KBUILD_MODNAME, cru); > > > > > > > > Because this is requested w/ IRQF_SHARED the free_irq() -> > > > > __free_irq() [1] will call the IRQ handler to simulate an IRQ SHARE > > > > scenario where other device generate an interrupt. > > > > Good point, I had missed that. > > > > > Currently CSI driver is not registered any interrupts and CRU is the single user. > > > > Regardless, the fact that the IRQ is requested with IRQF_SHARED means that the IRQ handler needs to be > > prepared to be called at any time from the point of registration to the point the IRQ is freed. This > > is tested by CONFIG_DEBUG_SHIRQ=y, which you should enable for testing. > > For single user, testing CONFIG_DEBUG_SHIRQ=y this does not make any sense. See [1] > [1] https://elixir.bootlin.com/linux/v6.11-rc5/source/kernel/irq/manage.c#L1965 > > > > > If you don't need to share the interrupt with any other device, you can drop the IRQF_SHARED. > > I will drop IRQF_SHARED flags instead as there is single user > and will send RFC for dropping calling IRQ handler for single user > with CONFIG_DEBUG_SHIRQ=y > > Please let me know is it fine for you? IMHO the latter is not a good idea: if you register an interrupt handler with IRQF_SHARED, you should be prepared for the handler being called at any time, and test that. Regardless of whether there is only a single user or not. Gr{oetje,eeting}s, Geert
On 26.08.2024 12:43, Laurent Pinchart wrote: > On Mon, Aug 26, 2024 at 08:08:33AM +0000, Biju Das wrote: >> On Monday, August 26, 2024 8:27 AM, claudiu beznea wrote: >>> On 24.08.2024 21:21, Biju Das wrote: >>>> Move request_irq() to probe(), in order to avoid requesting IRQ during >>>> device start which happens frequently. As this function is in probe(), >>>> it is better to replace it with its devm variant for managing the >>>> resource efficiently. >>>> >>>> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>>> --- >>>> v2->v3: >>>> * Dropped wrapper function rzg2l_cru_process_irq() and made >>>> rzg2l_cru_irq() global. >>>> * Added Rb tag from Laurent. >>>> v1->v2: >>>> * Updated commit header and description. >>>> * Moved rzg2l_cru_irq from rzg2l-video.c->rzg2l-core.c and introduced >>>> rzg2l_cru_process_irq() in video.c to process irq. >>>> * Dropped image_conv_irq from struct rzg2l_cru_dev >>>> * Replaced request_irq with its devm variant. >>>> --- >>>> .../media/platform/renesas/rzg2l-cru/rzg2l-core.c | 13 +++++++++---- >>>> .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h | 6 ++---- >>>> .../platform/renesas/rzg2l-cru/rzg2l-video.c | 15 ++------------- >>>> 3 files changed, 13 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c >>>> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c >>>> index 280efd2a8185..2a2907beb722 100644 >>>> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c >>>> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c >>>> @@ -242,7 +242,7 @@ static int rzg2l_cru_media_init(struct rzg2l_cru_dev *cru) >>>> static int rzg2l_cru_probe(struct platform_device *pdev) >>>> { >>>> struct rzg2l_cru_dev *cru; >>>> - int ret; >>>> + int irq, ret; >>>> >>>> cru = devm_kzalloc(&pdev->dev, sizeof(*cru), GFP_KERNEL); >>>> if (!cru) >>>> @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev) >>>> cru->dev = &pdev->dev; >>>> cru->info = of_device_get_match_data(&pdev->dev); >>>> >>>> - cru->image_conv_irq = platform_get_irq(pdev, 0); >>>> - if (cru->image_conv_irq < 0) >>>> - return cru->image_conv_irq; >>>> + irq = platform_get_irq(pdev, 0); >>>> + if (irq < 0) >>>> + return irq; >>>> + >>>> + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED, >>>> + KBUILD_MODNAME, cru); >>> >>> Because this is requested w/ IRQF_SHARED the free_irq() -> >>> __free_irq() [1] will call the IRQ handler to simulate an IRQ SHARE >>> scenario where other device generate an interrupt. > > Good point, I had missed that. > >> Currently CSI driver is not registered any interrupts and CRU is the single user. > > Regardless, the fact that the IRQ is requested with IRQF_SHARED means > that the IRQ handler needs to be prepared to be called at any time from > the point of registration to the point the IRQ is freed. This is tested > by CONFIG_DEBUG_SHIRQ=y, which you should enable for testing. > > If you don't need to share the interrupt with any other device, you can > drop the IRQF_SHARED. Otherwise, you will need to fix the issue > properly. You can probably wrap the interrupt handling with > pm_runtime_get_if_in_use() and pm_runtime_put() (hoping those functions > can be called from interrupt context). As of my current investigation on this, pm_runtime_suspended() check is enough in interrupt handler (but... see below how this works). > > On a side note, I also I wonder if this issue precludesusage of > devm_request_irq() for shared interrupts, requiring calling free_irq() > manually at remove time to control the sequence of cleanup operations. for pm_runtime_suspended() to work in interrupt handler (and to cover the CONFIG_DEBUG_SHIRQ=y) one need to: 1/ either devm_pm_runtime_enable() in probe before devm_request_irq() 2/ or use pm_runtime_enable() + request_irq() in probe and pm_runtime_disable() + free_irq() in remove Because pm_runtime_suspended() checks also for !dev->power.disable_depth. Thank you, Claudiu Benea > >>>> + if (ret) >>>> + return dev_err_probe(&pdev->dev, ret, "failed to request irq\n"); >>>> >>>> platform_set_drvdata(pdev, cru); >>>> >>>> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h >>>> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h >>>> index a5a99b004322..174760239548 100644 >>>> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h >>>> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h >>>> @@ -8,6 +8,7 @@ >>>> #ifndef __RZG2L_CRU__ >>>> #define __RZG2L_CRU__ >>>> >>>> +#include <linux/irqreturn.h> >>>> #include <linux/reset.h> >>>> >>>> #include <media/v4l2-async.h> >>>> @@ -68,8 +69,6 @@ struct rzg2l_cru_ip { >>>> * >>>> * @vclk: CRU Main clock >>>> * >>>> - * @image_conv_irq: Holds image conversion interrupt number >>>> - * >>>> * @vdev: V4L2 video device associated with CRU >>>> * @v4l2_dev: V4L2 device >>>> * @num_buf: Holds the current number of buffers enabled >>>> @@ -105,8 +104,6 @@ struct rzg2l_cru_dev { >>>> >>>> struct clk *vclk; >>>> >>>> - int image_conv_irq; >>>> - >>>> struct video_device vdev; >>>> struct v4l2_device v4l2_dev; >>>> u8 num_buf; >>>> @@ -141,6 +138,7 @@ void rzg2l_cru_dma_unregister(struct rzg2l_cru_dev *cru); >>>> >>>> int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru); >>>> void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru); >>>> +irqreturn_t rzg2l_cru_irq(int irq, void *data); >>>> >>>> const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format); >>>> >>>> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c >>>> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c >>>> index b16b8af6e8f8..e80bfb9fc1af 100644 >>>> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c >>>> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c >>>> @@ -527,7 +527,7 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru) >>>> rzg2l_cru_set_stream(cru, 0); >>>> } >>>> >>>> -static irqreturn_t rzg2l_cru_irq(int irq, void *data) >>>> +irqreturn_t rzg2l_cru_irq(int irq, void *data) >>>> { >>>> struct rzg2l_cru_dev *cru = data; >>>> unsigned int handled = 0; >>>> @@ -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); >
Hi Geert, > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Monday, August 26, 2024 10:56 AM > Subject: Re: [PATCH v3] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to probe() > > Hi Biju, > > On Mon, Aug 26, 2024 at 11:50 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > On Mon, Aug 26, 2024 at 08:08:33AM +0000, Biju Das wrote: > > > > On Monday, August 26, 2024 8:27 AM, claudiu beznea wrote: > > > > > On 24.08.2024 21:21, Biju Das wrote: > > > > > > @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev) > > > > > > cru->dev = &pdev->dev; > > > > > > cru->info = of_device_get_match_data(&pdev->dev); > > > > > > > > > > > > - cru->image_conv_irq = platform_get_irq(pdev, 0); > > > > > > - if (cru->image_conv_irq < 0) > > > > > > - return cru->image_conv_irq; > > > > > > + irq = platform_get_irq(pdev, 0); > > > > > > + if (irq < 0) > > > > > > + return irq; > > > > > > + > > > > > > + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED, > > > > > > + KBUILD_MODNAME, cru); > > > > > > > > > > Because this is requested w/ IRQF_SHARED the free_irq() -> > > > > > __free_irq() [1] will call the IRQ handler to simulate an IRQ > > > > > SHARE scenario where other device generate an interrupt. > > > > > > Good point, I had missed that. > > > > > > > Currently CSI driver is not registered any interrupts and CRU is the single user. > > > > > > Regardless, the fact that the IRQ is requested with IRQF_SHARED > > > means that the IRQ handler needs to be prepared to be called at any > > > time from the point of registration to the point the IRQ is freed. This is tested by > CONFIG_DEBUG_SHIRQ=y, which you should enable for testing. > > > > For single user, testing CONFIG_DEBUG_SHIRQ=y this does not make any > > sense. See [1] [1] > > https://elixir.bootlin.com/linux/v6.11-rc5/source/kernel/irq/manage.c# > > L1965 > > > > > > > > If you don't need to share the interrupt with any other device, you can drop the IRQF_SHARED. > > > > I will drop IRQF_SHARED flags instead as there is single user and will > > send RFC for dropping calling IRQ handler for single user with > > CONFIG_DEBUG_SHIRQ=y > > > > Please let me know is it fine for you? > > IMHO the latter is not a good idea: if you register an interrupt handler with IRQF_SHARED, you should > be prepared for the handler being called at any time, and test that. Regardless of whether there is > only a single user or not. OK, will drop sending RFC. Cheers, Biju
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c index 280efd2a8185..2a2907beb722 100644 --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c @@ -242,7 +242,7 @@ static int rzg2l_cru_media_init(struct rzg2l_cru_dev *cru) static int rzg2l_cru_probe(struct platform_device *pdev) { struct rzg2l_cru_dev *cru; - int ret; + int irq, ret; cru = devm_kzalloc(&pdev->dev, sizeof(*cru), GFP_KERNEL); if (!cru) @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev) cru->dev = &pdev->dev; cru->info = of_device_get_match_data(&pdev->dev); - cru->image_conv_irq = platform_get_irq(pdev, 0); - if (cru->image_conv_irq < 0) - return cru->image_conv_irq; + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED, + KBUILD_MODNAME, cru); + if (ret) + return dev_err_probe(&pdev->dev, ret, "failed to request irq\n"); platform_set_drvdata(pdev, cru); diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h index a5a99b004322..174760239548 100644 --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h @@ -8,6 +8,7 @@ #ifndef __RZG2L_CRU__ #define __RZG2L_CRU__ +#include <linux/irqreturn.h> #include <linux/reset.h> #include <media/v4l2-async.h> @@ -68,8 +69,6 @@ struct rzg2l_cru_ip { * * @vclk: CRU Main clock * - * @image_conv_irq: Holds image conversion interrupt number - * * @vdev: V4L2 video device associated with CRU * @v4l2_dev: V4L2 device * @num_buf: Holds the current number of buffers enabled @@ -105,8 +104,6 @@ struct rzg2l_cru_dev { struct clk *vclk; - int image_conv_irq; - struct video_device vdev; struct v4l2_device v4l2_dev; u8 num_buf; @@ -141,6 +138,7 @@ void rzg2l_cru_dma_unregister(struct rzg2l_cru_dev *cru); int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru); void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru); +irqreturn_t rzg2l_cru_irq(int irq, void *data); const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format); diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c index b16b8af6e8f8..e80bfb9fc1af 100644 --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c @@ -527,7 +527,7 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru) rzg2l_cru_set_stream(cru, 0); } -static irqreturn_t rzg2l_cru_irq(int irq, void *data) +irqreturn_t rzg2l_cru_irq(int irq, void *data) { struct rzg2l_cru_dev *cru = data; unsigned int handled = 0; @@ -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);