| Message ID | 20230728233200.151735-6-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: change interrupts code to make 0 be the no IRQ | expand |
Title suggestion: replace "stop using" with "Replace RAW IRQ indices in prints with tuple" or something else that describes what the new case is that we can expect after this PR was applied (at your discretion, it might be hard to fit that in 72 chars). Otherwise this reads as if you just dropped the index from printk's and systfs altogether. On 2023-07-29 02:31:59, Dmitry Baryshkov wrote: > In preparation to reworking IRQ indcies, stop using raw indices in > kernel output (both printk and debugfs). Instead use a pair of register > index and bit. This corresponds closer to the values in HW catalog. This is rather painful to read: irq=[%d, %d] IRQ:[%d, %d] IRQ: [%d, %d] IRQ [%d, %d] Four variations within the first **four** hunks. It looks like you tried to consistenize some of them, but can we please go all the way? Perhaps we also want to correct the format in dpu_trace.h? > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 49 ++++++++++++------- > 1 file changed, 31 insertions(+), 18 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 308b122059cd..6071d3f05b0c 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c > @@ -199,6 +199,7 @@ static const struct dpu_intr_reg dpu_intr_set_7xxx[] = { > > #define DPU_IRQ_REG(irq_idx) (irq_idx / 32) > #define DPU_IRQ_MASK(irq_idx) (BIT(irq_idx % 32)) > +#define DPU_IRQ_BIT(irq_idx) (ffs(DPU_IRQ_MASK(irq_idx)) - 1) > > static inline bool dpu_core_irq_is_valid(struct dpu_hw_intr *intr, > int irq_idx) > @@ -221,10 +222,11 @@ static void dpu_core_irq_callback_handler(struct dpu_kms *dpu_kms, int irq_idx) > { > struct dpu_hw_intr_entry *irq_entry = dpu_core_irq_get_entry(dpu_kms->hw_intr, irq_idx); > > - VERB("irq_idx=%d\n", irq_idx); > + VERB("irq=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > > if (!irq_entry->cb) > - DRM_ERROR("no registered cb, idx:%d\n", irq_idx); > + DRM_ERROR("no registered cb, IRQ:[%d, %d]\n", > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > > atomic_inc(&irq_entry->count); > > @@ -306,7 +308,8 @@ static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx) > return -EINVAL; > > if (!dpu_core_irq_is_valid(intr, irq_idx)) { > - pr_err("invalid IRQ index: [%d]\n", irq_idx); > + pr_err("invalid IRQ: [%d, %d]\n", > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > return -EINVAL; > } > > @@ -342,7 +345,8 @@ 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("DPU IRQ %d %senabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr, > + pr_debug("DPU IRQ [%d, %d] %senabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), dbgstr, > DPU_IRQ_MASK(irq_idx), cache_irq_mask); > > return 0; > @@ -359,7 +363,8 @@ static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx) > return -EINVAL; > > if (!dpu_core_irq_is_valid(intr, irq_idx)) { > - pr_err("invalid IRQ index: [%d]\n", irq_idx); > + pr_err("invalid IRQ: [%d, %d]\n", > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > return -EINVAL; > } > > @@ -391,7 +396,8 @@ 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("DPU IRQ %d %sdisabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr, > + pr_debug("DPU IRQ [%d, %d] %sdisabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), dbgstr, > DPU_IRQ_MASK(irq_idx), cache_irq_mask); > > return 0; > @@ -444,7 +450,7 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms, int irq_idx) > return 0; > > if (!dpu_core_irq_is_valid(intr, irq_idx)) { > - pr_err("invalid IRQ index: [%d]\n", irq_idx); > + pr_err("invalid IRQ: [%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > return 0; > } > > @@ -520,16 +526,19 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms, int irq_idx, > int ret; > > if (!irq_cb) { > - DPU_ERROR("invalid ird_idx:%d irq_cb:%ps\n", irq_idx, irq_cb); > + DPU_ERROR("invalid ird_IRQ:[%d, %d] irq_cb:%ps\n", I think this was a typo that should have been irq_idx in the original code, so this just becomes IRQ (with one of the variations to settle on, as brought up above). > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), irq_cb); > return -EINVAL; > } > > if (!dpu_core_irq_is_valid(dpu_kms->hw_intr, irq_idx)) { > - DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx); > + DPU_ERROR("invalid IRQ: [%d, %d]\n", > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > return -EINVAL; > } > > - VERB("[%pS] irq_idx=%d\n", __builtin_return_address(0), irq_idx); > + VERB("[%pS] irq=[%d, %d]\n", __builtin_return_address(0), > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > > spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags); > > @@ -548,8 +557,8 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms, int irq_idx, > dpu_kms->hw_intr, > irq_idx); > if (ret) > - DPU_ERROR("Fail to enable IRQ for irq_idx:%d\n", > - irq_idx); > + DPU_ERROR("Fail to enable IRQ for [%d, %d]\n", And let's just make this `Failed to enable IRQ:[%d, %d]`, following one of the variations discussed above. > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags); > > trace_dpu_irq_register_success(irq_idx); > @@ -564,19 +573,21 @@ int dpu_core_irq_unregister_callback(struct dpu_kms *dpu_kms, int irq_idx) > int ret; > > if (!dpu_core_irq_is_valid(dpu_kms->hw_intr, irq_idx)) { > - DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx); > + DPU_ERROR("invalid IRQ: [%d, %d]\n", > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > return -EINVAL; > } > > - VERB("[%pS] irq_idx=%d\n", __builtin_return_address(0), irq_idx); > + VERB("[%pS] irq=[%d, %d]\n", __builtin_return_address(0), > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); > > spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags); > trace_dpu_core_irq_unregister_callback(irq_idx); > > ret = dpu_hw_intr_disable_irq_locked(dpu_kms->hw_intr, irq_idx); > if (ret) > - DPU_ERROR("Fail to disable IRQ for irq_idx:%d: %d\n", > - irq_idx, ret); > + DPU_ERROR("Fail to disable IRQ for [%d, %d]: %d\n", And let's drop "for" here too, it's unreadable. - Marijn > + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), ret); > > irq_entry = dpu_core_irq_get_entry(dpu_kms->hw_intr, irq_idx); > irq_entry->cb = NULL; > @@ -606,7 +617,8 @@ static int dpu_debugfs_core_irq_show(struct seq_file *s, void *v) > spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags); > > if (irq_count || cb) > - seq_printf(s, "idx:%d irq:%d cb:%ps\n", i, irq_count, cb); > + seq_printf(s, "IRQ:[%d, %d] count:%d cb:%ps\n", > + DPU_IRQ_REG(i), DPU_IRQ_BIT(i), irq_count, cb); > } > > return 0; > @@ -652,7 +664,8 @@ void dpu_core_irq_uninstall(struct msm_kms *kms) > for (i = 0; i < DPU_NUM_IRQS; i++) { > irq_entry = dpu_core_irq_get_entry(dpu_kms->hw_intr, i); > if (irq_entry->cb) > - DPU_ERROR("irq_idx=%d still enabled/registered\n", i); > + DPU_ERROR("irq=[%d, %d] still enabled/registered\n", > + DPU_IRQ_REG(i), DPU_IRQ_BIT(i)); > } > > dpu_clear_irqs(dpu_kms); > -- > 2.39.2 >
On 2023-07-29 02:31:59, Dmitry Baryshkov wrote: > In preparation to reworking IRQ indcies, stop using raw indices in > kernel output (both printk and debugfs). Instead use a pair of register > index and bit. This corresponds closer to the values in HW catalog. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 49 ++++++++++++------- > 1 file changed, 31 insertions(+), 18 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 308b122059cd..6071d3f05b0c 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c > @@ -199,6 +199,7 @@ static const struct dpu_intr_reg dpu_intr_set_7xxx[] = { > > #define DPU_IRQ_REG(irq_idx) (irq_idx / 32) > #define DPU_IRQ_MASK(irq_idx) (BIT(irq_idx % 32)) > +#define DPU_IRQ_BIT(irq_idx) (ffs(DPU_IRQ_MASK(irq_idx)) - 1) Another nit: I'd find it clearer to define DPU_IRQ_MASK in terms of DPU_IRQ_BIT[_INDEX...] than the other way around, because this is just irq_idx%32. - Marijn
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 308b122059cd..6071d3f05b0c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c @@ -199,6 +199,7 @@ static const struct dpu_intr_reg dpu_intr_set_7xxx[] = { #define DPU_IRQ_REG(irq_idx) (irq_idx / 32) #define DPU_IRQ_MASK(irq_idx) (BIT(irq_idx % 32)) +#define DPU_IRQ_BIT(irq_idx) (ffs(DPU_IRQ_MASK(irq_idx)) - 1) static inline bool dpu_core_irq_is_valid(struct dpu_hw_intr *intr, int irq_idx) @@ -221,10 +222,11 @@ static void dpu_core_irq_callback_handler(struct dpu_kms *dpu_kms, int irq_idx) { struct dpu_hw_intr_entry *irq_entry = dpu_core_irq_get_entry(dpu_kms->hw_intr, irq_idx); - VERB("irq_idx=%d\n", irq_idx); + VERB("irq=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); if (!irq_entry->cb) - DRM_ERROR("no registered cb, idx:%d\n", irq_idx); + DRM_ERROR("no registered cb, IRQ:[%d, %d]\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); atomic_inc(&irq_entry->count); @@ -306,7 +308,8 @@ static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx) return -EINVAL; if (!dpu_core_irq_is_valid(intr, irq_idx)) { - pr_err("invalid IRQ index: [%d]\n", irq_idx); + pr_err("invalid IRQ: [%d, %d]\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); return -EINVAL; } @@ -342,7 +345,8 @@ 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("DPU IRQ %d %senabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr, + pr_debug("DPU IRQ [%d, %d] %senabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), dbgstr, DPU_IRQ_MASK(irq_idx), cache_irq_mask); return 0; @@ -359,7 +363,8 @@ static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx) return -EINVAL; if (!dpu_core_irq_is_valid(intr, irq_idx)) { - pr_err("invalid IRQ index: [%d]\n", irq_idx); + pr_err("invalid IRQ: [%d, %d]\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); return -EINVAL; } @@ -391,7 +396,8 @@ 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("DPU IRQ %d %sdisabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", irq_idx, dbgstr, + pr_debug("DPU IRQ [%d, %d] %sdisabled: MASK:0x%.8lx, CACHE-MASK:0x%.8x\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), dbgstr, DPU_IRQ_MASK(irq_idx), cache_irq_mask); return 0; @@ -444,7 +450,7 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms, int irq_idx) return 0; if (!dpu_core_irq_is_valid(intr, irq_idx)) { - pr_err("invalid IRQ index: [%d]\n", irq_idx); + pr_err("invalid IRQ: [%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); return 0; } @@ -520,16 +526,19 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms, int irq_idx, int ret; if (!irq_cb) { - DPU_ERROR("invalid ird_idx:%d irq_cb:%ps\n", irq_idx, irq_cb); + DPU_ERROR("invalid ird_IRQ:[%d, %d] irq_cb:%ps\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), irq_cb); return -EINVAL; } if (!dpu_core_irq_is_valid(dpu_kms->hw_intr, irq_idx)) { - DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx); + DPU_ERROR("invalid IRQ: [%d, %d]\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); return -EINVAL; } - VERB("[%pS] irq_idx=%d\n", __builtin_return_address(0), irq_idx); + VERB("[%pS] irq=[%d, %d]\n", __builtin_return_address(0), + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags); @@ -548,8 +557,8 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms, int irq_idx, dpu_kms->hw_intr, irq_idx); if (ret) - DPU_ERROR("Fail to enable IRQ for irq_idx:%d\n", - irq_idx); + DPU_ERROR("Fail to enable IRQ for [%d, %d]\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags); trace_dpu_irq_register_success(irq_idx); @@ -564,19 +573,21 @@ int dpu_core_irq_unregister_callback(struct dpu_kms *dpu_kms, int irq_idx) int ret; if (!dpu_core_irq_is_valid(dpu_kms->hw_intr, irq_idx)) { - DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx); + DPU_ERROR("invalid IRQ: [%d, %d]\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); return -EINVAL; } - VERB("[%pS] irq_idx=%d\n", __builtin_return_address(0), irq_idx); + VERB("[%pS] irq=[%d, %d]\n", __builtin_return_address(0), + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx)); spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags); trace_dpu_core_irq_unregister_callback(irq_idx); ret = dpu_hw_intr_disable_irq_locked(dpu_kms->hw_intr, irq_idx); if (ret) - DPU_ERROR("Fail to disable IRQ for irq_idx:%d: %d\n", - irq_idx, ret); + DPU_ERROR("Fail to disable IRQ for [%d, %d]: %d\n", + DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx), ret); irq_entry = dpu_core_irq_get_entry(dpu_kms->hw_intr, irq_idx); irq_entry->cb = NULL; @@ -606,7 +617,8 @@ static int dpu_debugfs_core_irq_show(struct seq_file *s, void *v) spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags); if (irq_count || cb) - seq_printf(s, "idx:%d irq:%d cb:%ps\n", i, irq_count, cb); + seq_printf(s, "IRQ:[%d, %d] count:%d cb:%ps\n", + DPU_IRQ_REG(i), DPU_IRQ_BIT(i), irq_count, cb); } return 0; @@ -652,7 +664,8 @@ void dpu_core_irq_uninstall(struct msm_kms *kms) for (i = 0; i < DPU_NUM_IRQS; i++) { irq_entry = dpu_core_irq_get_entry(dpu_kms->hw_intr, i); if (irq_entry->cb) - DPU_ERROR("irq_idx=%d still enabled/registered\n", i); + DPU_ERROR("irq=[%d, %d] still enabled/registered\n", + DPU_IRQ_REG(i), DPU_IRQ_BIT(i)); } dpu_clear_irqs(dpu_kms);
In preparation to reworking IRQ indcies, stop using raw indices in kernel output (both printk and debugfs). Instead use a pair of register index and bit. This corresponds closer to the values in HW catalog. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 49 ++++++++++++------- 1 file changed, 31 insertions(+), 18 deletions(-)