On Sun 11 Apr 19:09 CDT 2021, Dmitry Baryshkov wrote:
> There is little sense in reading interrupt statuses and right after that
> going after the array of statuses to dispatch them. Merge both loops
> into single function doing read and dispatch.
>
Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
Regards,
Bjorn
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c | 10 +--
> .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 66 ++++++-------------
> .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 8 ---
> 3 files changed, 20 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
> index cdec3fbe6ff4..54b34746a587 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c
> @@ -376,15 +376,6 @@ void dpu_core_irq_uninstall(struct dpu_kms *dpu_kms)
>
> irqreturn_t dpu_core_irq(struct dpu_kms *dpu_kms)
> {
> - /*
> - * Read interrupt status from all sources. Interrupt status are
> - * stored within hw_intr.
> - * Function will also clear the interrupt status after reading.
> - * Individual interrupt status bit will only get stored if it
> - * is enabled.
> - */
> - dpu_kms->hw_intr->ops.get_interrupt_statuses(dpu_kms->hw_intr);
> -
> /*
> * Dispatch to HW driver to handle interrupt lookup that is being
> * fired. When matching interrupt is located, HW driver will call to
> @@ -392,6 +383,7 @@ irqreturn_t dpu_core_irq(struct dpu_kms *dpu_kms)
> * dpu_core_irq_callback_handler will perform the registered function
> * callback, and do the interrupt status clearing once the registered
> * callback is finished.
> + * Function will also clear the interrupt status after reading.
> */
> dpu_kms->hw_intr->ops.dispatch_irqs(
> dpu_kms->hw_intr,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> index 48c96b812126..cf9bfd45aa59 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -1371,6 +1371,7 @@ static void dpu_hw_intr_dispatch_irq(struct dpu_hw_intr *intr,
> int start_idx;
> int end_idx;
> u32 irq_status;
> + u32 enable_mask;
> unsigned long irq_flags;
>
> if (!intr)
> @@ -1383,8 +1384,6 @@ static void dpu_hw_intr_dispatch_irq(struct dpu_hw_intr *intr,
> */
> spin_lock_irqsave(&intr->irq_lock, irq_flags);
> for (reg_idx = 0; reg_idx < ARRAY_SIZE(dpu_intr_set); reg_idx++) {
> - irq_status = intr->save_irq_status[reg_idx];
> -
> /*
> * Each Interrupt register has a range of 64 indexes, and
> * that is static for dpu_irq_map.
> @@ -1396,6 +1395,20 @@ static void dpu_hw_intr_dispatch_irq(struct dpu_hw_intr *intr,
> start_idx >= ARRAY_SIZE(dpu_irq_map))
> continue;
>
> + /* Read interrupt status */
> + irq_status = DPU_REG_READ(&intr->hw, dpu_intr_set[reg_idx].status_off);
> +
> + /* Read enable mask */
> + enable_mask = DPU_REG_READ(&intr->hw, dpu_intr_set[reg_idx].en_off);
> +
> + /* and clear the interrupt */
> + if (irq_status)
> + DPU_REG_WRITE(&intr->hw, dpu_intr_set[reg_idx].clr_off,
> + irq_status);
> +
> + /* Finally update IRQ status based on enable mask */
> + irq_status &= enable_mask;
> +
> /*
> * Search through matching intr status from irq map.
> * start_idx and end_idx defined the search range in
> @@ -1429,6 +1442,10 @@ static void dpu_hw_intr_dispatch_irq(struct dpu_hw_intr *intr,
> irq_status &= ~dpu_irq_map[irq_idx].irq_mask;
> }
> }
> +
> + /* ensure register writes go through */
> + wmb();
> +
> spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
> }
>
> @@ -1580,41 +1597,6 @@ static int dpu_hw_intr_disable_irqs(struct dpu_hw_intr *intr)
> return 0;
> }
>
> -static void dpu_hw_intr_get_interrupt_statuses(struct dpu_hw_intr *intr)
> -{
> - int i;
> - u32 enable_mask;
> - unsigned long irq_flags;
> -
> - if (!intr)
> - return;
> -
> - spin_lock_irqsave(&intr->irq_lock, irq_flags);
> - for (i = 0; i < ARRAY_SIZE(dpu_intr_set); i++) {
> - if (!test_bit(i, &intr->irq_mask))
> - continue;
> -
> - /* Read interrupt status */
> - intr->save_irq_status[i] = DPU_REG_READ(&intr->hw,
> - dpu_intr_set[i].status_off);
> -
> - /* Read enable mask */
> - enable_mask = DPU_REG_READ(&intr->hw, dpu_intr_set[i].en_off);
> -
> - /* and clear the interrupt */
> - if (intr->save_irq_status[i])
> - DPU_REG_WRITE(&intr->hw, dpu_intr_set[i].clr_off,
> - intr->save_irq_status[i]);
> -
> - /* Finally update IRQ status based on enable mask */
> - intr->save_irq_status[i] &= enable_mask;
> - }
> -
> - /* ensure register writes go through */
> - wmb();
> -
> - spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
> -}
>
> static void dpu_hw_intr_clear_intr_status_nolock(struct dpu_hw_intr *intr,
> int irq_idx)
> @@ -1673,7 +1655,6 @@ static void __setup_intr_ops(struct dpu_hw_intr_ops *ops)
> ops->dispatch_irqs = dpu_hw_intr_dispatch_irq;
> ops->clear_all_irqs = dpu_hw_intr_clear_irqs;
> ops->disable_all_irqs = dpu_hw_intr_disable_irqs;
> - ops->get_interrupt_statuses = dpu_hw_intr_get_interrupt_statuses;
> ops->clear_intr_status_nolock = dpu_hw_intr_clear_intr_status_nolock;
> ops->get_interrupt_status = dpu_hw_intr_get_interrupt_status;
> }
> @@ -1710,14 +1691,6 @@ struct dpu_hw_intr *dpu_hw_intr_init(void __iomem *addr,
> return ERR_PTR(-ENOMEM);
> }
>
> - intr->save_irq_status = kcalloc(ARRAY_SIZE(dpu_intr_set), sizeof(u32),
> - GFP_KERNEL);
> - if (intr->save_irq_status == NULL) {
> - kfree(intr->cache_irq_mask);
> - kfree(intr);
> - return ERR_PTR(-ENOMEM);
> - }
> -
> intr->irq_mask = m->mdss_irqs;
> intr->obsolete_irq = m->obsolete_irq;
>
> @@ -1730,7 +1703,6 @@ void dpu_hw_intr_destroy(struct dpu_hw_intr *intr)
> {
> if (intr) {
> kfree(intr->cache_irq_mask);
> - kfree(intr->save_irq_status);
> kfree(intr);
> }
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
> index 5d6f9a7a5195..5a1c304ba93f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
> @@ -142,14 +142,6 @@ struct dpu_hw_intr_ops {
> void (*cbfunc)(void *arg, int irq_idx),
> void *arg);
>
> - /**
> - * get_interrupt_statuses - Gets and store value from all interrupt
> - * status registers that are currently fired.
> - * @intr: HW interrupt handle
> - */
> - void (*get_interrupt_statuses)(
> - struct dpu_hw_intr *intr);
> -
> /**
> * clear_intr_status_nolock() - clears the HW interrupts without lock
> * @intr: HW interrupt handle
> --
> 2.30.2
>
