| Message ID | 20221121222456.437815-1-marijn.suijten@somainline.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm/dpu: Print interrupt index in addition to the mask | expand |
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>
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;
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 --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;
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(-)