Re: [Freedreno] [PATCH v1 1/3] drm/msm/dpu: merge dpu_hw_intr_get_interrupt_statuses into dpu_hw_intr_dispatch_irqs
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
Regards,
Bjorn
> Signed-off-by: Dmitry Baryshkov
> ---
> 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_s
[Freedreno] [PATCH v1 1/3] drm/msm/dpu: merge dpu_hw_intr_get_interrupt_statuses into dpu_hw_intr_dispatch_irqs
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
---
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,
-
