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.
>
> - 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
