[PATCH v2 4/6] drm/msm/dpu: autodetect supported interrupts

[Marijn Suijten]

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 at linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten at somainline.org>

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

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