diff mbox series

drm/msm/dpu: Print interrupt index in addition to the mask

Message ID 20221121222456.437815-1-marijn.suijten@somainline.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/dpu: Print interrupt index in addition to the mask | expand

Commit Message

Marijn Suijten Nov. 21, 2022, 10:24 p.m. UTC 
The mask only describes the `irq_idx % 32` part, making it generally
impossible to deduce what interrupt is being enabled/disabled.  Since
`debug/core_irq` in debugfs (and other prints) also include the full
`DPU_IRQ_IDX()` value, print the same full value here for easier
correlation instead of only adding the `irq_idx / 32` part.

Furthermore, make the dbgstr messages more consistent.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Martin Botka Nov. 21, 2022, 10:35 p.m. UTC | #1 
On November 21, 2022 11:24:55 PM GMT+01:00, Marijn Suijten <marijn.suijten@somainline.org> wrote:
>The mask only describes the `irq_idx % 32` part, making it generally
>impossible to deduce what interrupt is being enabled/disabled.  Since
>`debug/core_irq` in debugfs (and other prints) also include the full
>`DPU_IRQ_IDX()` value, print the same full value here for easier
>correlation instead of only adding the `irq_idx / 32` part.
>
>Furthermore, make the dbgstr messages more consistent.
>
>Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>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 cf1b6d84c18a..64589a9c2c51 100644
>--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
>+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
>@@ -252,9 +252,9 @@ static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
> 
> 	cache_irq_mask = intr->cache_irq_mask[reg_idx];
> 	if (cache_irq_mask & DPU_IRQ_MASK(irq_idx)) {
>-		dbgstr = "DPU IRQ already set:";
>+		dbgstr = "already ";
> 	} else {
>-		dbgstr = "DPU IRQ enabled:";
>+		dbgstr = "";
> 
> 		cache_irq_mask |= DPU_IRQ_MASK(irq_idx);
> 		/* Cleaning any pending interrupt */
>@@ -268,7 +268,7 @@ static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
> 		intr->cache_irq_mask[reg_idx] = cache_irq_mask;
> 	}
> 
>-	pr_debug("%s MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", dbgstr,
>+	pr_debug("DPU IRQ %d %senabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr,
> 			DPU_IRQ_MASK(irq_idx), cache_irq_mask);
> 
> 	return 0;
>@@ -301,9 +301,9 @@ static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
> 
> 	cache_irq_mask = intr->cache_irq_mask[reg_idx];
> 	if ((cache_irq_mask & DPU_IRQ_MASK(irq_idx)) == 0) {
>-		dbgstr = "DPU IRQ is already cleared:";
>+		dbgstr = "already ";
> 	} else {
>-		dbgstr = "DPU IRQ mask disable:";
>+		dbgstr = "";
> 
> 		cache_irq_mask &= ~DPU_IRQ_MASK(irq_idx);
> 		/* Disable interrupts based on the new mask */
>@@ -317,7 +317,7 @@ static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
> 		intr->cache_irq_mask[reg_idx] = cache_irq_mask;
> 	}
> 
>-	pr_debug("%s MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", dbgstr,
>+	pr_debug("DPU IRQ %d %sdisabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr,
> 			DPU_IRQ_MASK(irq_idx), cache_irq_mask);
> 
> 	return 0;

Looks good to me.

Reviewed-by: Martin Botka <martin.botka@somainline.org>
Abhinav Kumar Nov. 25, 2022, 6:21 a.m. UTC | #2 
On 11/21/2022 2:24 PM, Marijn Suijten wrote:
> The mask only describes the `irq_idx % 32` part, making it generally
> impossible to deduce what interrupt is being enabled/disabled.  Since
> `debug/core_irq` in debugfs (and other prints) also include the full
> `DPU_IRQ_IDX()` value, print the same full value here for easier
> correlation instead of only adding the `irq_idx / 32` part.
> 
> Furthermore, make the dbgstr messages more consistent.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> 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 cf1b6d84c18a..64589a9c2c51 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -252,9 +252,9 @@ static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
>   
>   	cache_irq_mask = intr->cache_irq_mask[reg_idx];
>   	if (cache_irq_mask & DPU_IRQ_MASK(irq_idx)) {
> -		dbgstr = "DPU IRQ already set:";
> +		dbgstr = "already ";
>   	} else {
> -		dbgstr = "DPU IRQ enabled:";
> +		dbgstr = "";
>   
>   		cache_irq_mask |= DPU_IRQ_MASK(irq_idx);
>   		/* Cleaning any pending interrupt */
> @@ -268,7 +268,7 @@ static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
>   		intr->cache_irq_mask[reg_idx] = cache_irq_mask;
>   	}
>   
> -	pr_debug("%s MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", dbgstr,
> +	pr_debug("DPU IRQ %d %senabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr,
>   			DPU_IRQ_MASK(irq_idx), cache_irq_mask);
>   
>   	return 0;
> @@ -301,9 +301,9 @@ static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
>   
>   	cache_irq_mask = intr->cache_irq_mask[reg_idx];
>   	if ((cache_irq_mask & DPU_IRQ_MASK(irq_idx)) == 0) {
> -		dbgstr = "DPU IRQ is already cleared:";
> +		dbgstr = "already ";
>   	} else {
> -		dbgstr = "DPU IRQ mask disable:";
> +		dbgstr = "";
>   
>   		cache_irq_mask &= ~DPU_IRQ_MASK(irq_idx);
>   		/* Disable interrupts based on the new mask */
> @@ -317,7 +317,7 @@ static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
>   		intr->cache_irq_mask[reg_idx] = cache_irq_mask;
>   	}
>   
> -	pr_debug("%s MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", dbgstr,
> +	pr_debug("DPU IRQ %d %sdisabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr,
>   			DPU_IRQ_MASK(irq_idx), cache_irq_mask);
>   
>   	return 0;
Dmitry Baryshkov Jan. 9, 2023, 11:43 p.m. UTC | #3 
On Mon, 21 Nov 2022 23:24:55 +0100, Marijn Suijten wrote:
> The mask only describes the `irq_idx % 32` part, making it generally
> impossible to deduce what interrupt is being enabled/disabled.  Since
> `debug/core_irq` in debugfs (and other prints) also include the full
> `DPU_IRQ_IDX()` value, print the same full value here for easier
> correlation instead of only adding the `irq_idx / 32` part.
> 
> Furthermore, make the dbgstr messages more consistent.
> 
> [...]

Applied, thanks!

[1/1] drm/msm/dpu: Print interrupt index in addition to the mask
      https://gitlab.freedesktop.org/lumag/msm/-/commit/d45e5744ab39

Best regards,
diff mbox series

Patch

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 cf1b6d84c18a..64589a9c2c51 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -252,9 +252,9 @@  static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
 
 	cache_irq_mask = intr->cache_irq_mask[reg_idx];
 	if (cache_irq_mask & DPU_IRQ_MASK(irq_idx)) {
-		dbgstr = "DPU IRQ already set:";
+		dbgstr = "already ";
 	} else {
-		dbgstr = "DPU IRQ enabled:";
+		dbgstr = "";
 
 		cache_irq_mask |= DPU_IRQ_MASK(irq_idx);
 		/* Cleaning any pending interrupt */
@@ -268,7 +268,7 @@  static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
 		intr->cache_irq_mask[reg_idx] = cache_irq_mask;
 	}
 
-	pr_debug("%s MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", dbgstr,
+	pr_debug("DPU IRQ %d %senabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr,
 			DPU_IRQ_MASK(irq_idx), cache_irq_mask);
 
 	return 0;
@@ -301,9 +301,9 @@  static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
 
 	cache_irq_mask = intr->cache_irq_mask[reg_idx];
 	if ((cache_irq_mask & DPU_IRQ_MASK(irq_idx)) == 0) {
-		dbgstr = "DPU IRQ is already cleared:";
+		dbgstr = "already ";
 	} else {
-		dbgstr = "DPU IRQ mask disable:";
+		dbgstr = "";
 
 		cache_irq_mask &= ~DPU_IRQ_MASK(irq_idx);
 		/* Disable interrupts based on the new mask */
@@ -317,7 +317,7 @@  static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
 		intr->cache_irq_mask[reg_idx] = cache_irq_mask;
 	}
 
-	pr_debug("%s MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", dbgstr,
+	pr_debug("DPU IRQ %d %sdisabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr,
 			DPU_IRQ_MASK(irq_idx), cache_irq_mask);
 
 	return 0;