On 2023-05-23 01:17:50, Dmitry Baryshkov wrote: > On Tue, 23 May 2023 at 01:12, Marijn Suijten > <marijn.suijten@xxxxxxxxxxxxxx> wrote: > > > > On 2023-05-23 00:45:25, Dmitry Baryshkov wrote: > > > Declaring the mask of supported interrupts proved to be error-prone. It > > > is very easy to add a bit with no corresponding backing block or to miss > > > the INTF TE bit. Replace this with looping over the enabled INTF blocks > > > to setup the irq mask. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > > > Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx> > > > > > --- > > > .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 20 ++++++++++++++++++- > > > .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 6 ++++++ > > > 2 files changed, 25 insertions(+), 1 deletion(-) > > > > > > 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 a03d826bb9ad..01f2660a2354 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c > > > @@ -463,6 +463,7 @@ struct dpu_hw_intr *dpu_hw_intr_init(void __iomem *addr, > > > { > > > struct dpu_hw_intr *intr; > > > int nirq = MDP_INTR_MAX * 32; > > > + unsigned int i; > > > > > > if (!addr || !m) > > > return ERR_PTR(-EINVAL); > > > @@ -480,7 +481,24 @@ struct dpu_hw_intr *dpu_hw_intr_init(void __iomem *addr, > > > > > > intr->total_irqs = nirq; > > > > > > - intr->irq_mask = m->mdss_irqs; > > > + intr->irq_mask = BIT(MDP_SSPP_TOP0_INTR) | > > > + BIT(MDP_SSPP_TOP0_INTR2) | > > > + BIT(MDP_SSPP_TOP0_HIST_INTR); > > > + for (i = 0; i < m->intf_count; i++) { > > > + const struct dpu_intf_cfg *intf = &m->intf[i]; > > > + > > > + if (intf->type == INTF_NONE) > > > + continue; > > > + > > > + intr->irq_mask |= BIT(MDP_INTFn_INTR(intf->id)); > > > + > > > + if (test_bit(DPU_INTF_TE, &intf->features)) { > > > + unsigned idx = MDP_INTFn_TEAR_INTR(intf->id); > > > + > > > + if (!WARN_ON(idx == -1)) > > > > We don't need to validate the catalog? But warning users about this > > (and accidentally turning on all interrupt bits hiding the issue anyway) > > is a nice side effect though, as you showed it was already going wrong > > in patch 1/6. > > > > OTOH you might have inlined the macro and provided a more useful warning > > message (DPU_INTF_TE can only be present on INTF1/2)... and then one > > could assert on INTF_DSI etc etc etc... > > I'd prefer to keep it, as a safeguard for submission being in > progress, newer generations gaining TE blocks on other interfaces, > etc. > I was selecting between having explicit intf->id == INTF_1 || == > INTF_2 condition and this kind of macro. Being explicit in-line here has my preference. Maybe the same for the above bit, not sure about that one yet (e.g. have an upper bound on INTF_8). - Marijn > > > > > - Marijn > > > > > + intr->irq_mask |= BIT(idx); > > > + } > > > + } > > > > > > spin_lock_init(&intr->irq_lock); > > > > > > 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 f329d6d7f646..f0b92c9e3b09 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h > > > @@ -17,6 +17,7 @@ enum dpu_hw_intr_reg { > > > MDP_SSPP_TOP0_INTR, > > > MDP_SSPP_TOP0_INTR2, > > > MDP_SSPP_TOP0_HIST_INTR, > > > + /* All MDP_INTFn_INTR should come sequentially */ > > > MDP_INTF0_INTR, > > > MDP_INTF1_INTR, > > > MDP_INTF2_INTR, > > > @@ -33,6 +34,11 @@ enum dpu_hw_intr_reg { > > > MDP_INTR_MAX, > > > }; > > > > > > +#define MDP_INTFn_INTR(intf) (MDP_INTF0_INTR + (intf - INTF_0)) > > > +#define MDP_INTFn_TEAR_INTR(intf) (intf == INTF_1 ? MDP_INTF1_TEAR_INTR : \ > > > + intf == INTF_2 ? MDP_INTF2_TEAR_INTR : \ > > > + -1) > > > + > > > /* compatibility */ > > > #define MDP_INTF0_7xxx_INTR MDP_INTF0_INTR > > > #define MDP_INTF1_7xxx_INTR MDP_INTF1_INTR > > > -- > > > 2.39.2 > > > > > > > -- > With best wishes > Dmitry