Re: [Freedreno] [PATCH v2 1/6] drm/msm/dpu: merge dpu_hw_intr_get_interrupt_statuses into dpu_hw_intr_dispatch_irqs

[abhinavk@codeaurora.org]

On 2021-05-16 13:29, 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.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  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