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@xxxxxxxxxxxxxxxx>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>>>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>>>> ---
>>>> 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);
>
