From: Neil Armstrong <neil.armstrong@linaro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>,
Stephen Boyd <swboyd@chromium.org>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
Bjorn Andersson <andersson@kernel.org>,
linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
freedreno@lists.freedesktop.org
Subject: Re: [PATCH 3/6] drm/msm/dpu: split interrupt address arrays
Date: Mon, 22 May 2023 16:36:51 +0200 [thread overview]
Message-ID: <80da4c26-ca3f-00c9-072c-087a1ff24c74@linaro.org> (raw)
In-Reply-To: <20230522004227.134501-4-dmitry.baryshkov@linaro.org>
Hi,
On 22/05/2023 02:42, Dmitry Baryshkov wrote:
> There is no point in having a single enum (and a single array) for both
> DPU < 7.0 and DPU >= 7.0 interrupt registers. Instead define a single
> enum and two IRQ address arrays.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 +
> .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 82 +++++++++++++------
> .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 28 ++++---
> 3 files changed, 74 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index 677048cc3b7d..72530ebb0ae6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -351,6 +351,7 @@ struct dpu_rotation_cfg {
> * @has_dim_layer dim layer feature status
> * @has_idle_pc indicate if idle power collapse feature is supported
> * @has_3d_merge indicate if 3D merge is supported
> + * @has_7xxx_intr indicate that INTF/IRQs use addressing for DPU 7.0 and greater
> * @max_linewidth max linewidth for sspp
> * @pixel_ram_size size of latency hiding and de-tiling buffer in bytes
> * @max_hdeci_exp max horizontal decimation supported (max is 2^value)
> @@ -364,6 +365,7 @@ struct dpu_caps {
> bool has_dim_layer;
> bool has_idle_pc;
> bool has_3d_merge;
> + bool has_7xxx_intr;
looks good, but I can't find where has_7xxx_intr is set in the patchset
Neil
> /* SSPP limits */
> u32 max_linewidth;
> u32 pixel_ram_size;
> 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 0776b0f6df4f..a03d826bb9ad 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -51,11 +51,9 @@ struct dpu_intr_reg {
> };
>
> /*
> - * struct dpu_intr_reg - List of DPU interrupt registers
> - *
> - * When making changes be sure to sync with dpu_hw_intr_reg
> + * dpu_intr_set_legacy - List of DPU interrupt registers for DPU <= 6.x
> */
> -static const struct dpu_intr_reg dpu_intr_set[] = {
> +static const struct dpu_intr_reg dpu_intr_set_legacy[] = {
> [MDP_SSPP_TOP0_INTR] = {
> INTR_CLEAR,
> INTR_EN,
> @@ -121,57 +119,78 @@ static const struct dpu_intr_reg dpu_intr_set[] = {
> MDP_AD4_INTR_EN_OFF(1),
> MDP_AD4_INTR_STATUS_OFF(1),
> },
> - [MDP_INTF0_7xxx_INTR] = {
> +};
> +
> +/*
> + * dpu_intr_set_7xxx - List of DPU interrupt registers for DPU >= 7.0
> + */
> +static const struct dpu_intr_reg dpu_intr_set_7xxx[] = {
> + [MDP_SSPP_TOP0_INTR] = {
> + INTR_CLEAR,
> + INTR_EN,
> + INTR_STATUS
> + },
> + [MDP_SSPP_TOP0_INTR2] = {
> + INTR2_CLEAR,
> + INTR2_EN,
> + INTR2_STATUS
> + },
> + [MDP_SSPP_TOP0_HIST_INTR] = {
> + HIST_INTR_CLEAR,
> + HIST_INTR_EN,
> + HIST_INTR_STATUS
> + },
> + [MDP_INTF0_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(0),
> MDP_INTF_REV_7xxx_INTR_EN(0),
> MDP_INTF_REV_7xxx_INTR_STATUS(0)
> },
> - [MDP_INTF1_7xxx_INTR] = {
> + [MDP_INTF1_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(1),
> MDP_INTF_REV_7xxx_INTR_EN(1),
> MDP_INTF_REV_7xxx_INTR_STATUS(1)
> },
> - [MDP_INTF1_7xxx_TEAR_INTR] = {
> + [MDP_INTF1_TEAR_INTR] = {
> MDP_INTF_REV_7xxx_INTR_TEAR_CLEAR(1),
> MDP_INTF_REV_7xxx_INTR_TEAR_EN(1),
> MDP_INTF_REV_7xxx_INTR_TEAR_STATUS(1)
> },
> - [MDP_INTF2_7xxx_INTR] = {
> + [MDP_INTF2_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(2),
> MDP_INTF_REV_7xxx_INTR_EN(2),
> MDP_INTF_REV_7xxx_INTR_STATUS(2)
> },
> - [MDP_INTF2_7xxx_TEAR_INTR] = {
> + [MDP_INTF2_TEAR_INTR] = {
> MDP_INTF_REV_7xxx_INTR_TEAR_CLEAR(2),
> MDP_INTF_REV_7xxx_INTR_TEAR_EN(2),
> MDP_INTF_REV_7xxx_INTR_TEAR_STATUS(2)
> },
> - [MDP_INTF3_7xxx_INTR] = {
> + [MDP_INTF3_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(3),
> MDP_INTF_REV_7xxx_INTR_EN(3),
> MDP_INTF_REV_7xxx_INTR_STATUS(3)
> },
> - [MDP_INTF4_7xxx_INTR] = {
> + [MDP_INTF4_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(4),
> MDP_INTF_REV_7xxx_INTR_EN(4),
> MDP_INTF_REV_7xxx_INTR_STATUS(4)
> },
> - [MDP_INTF5_7xxx_INTR] = {
> + [MDP_INTF5_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(5),
> MDP_INTF_REV_7xxx_INTR_EN(5),
> MDP_INTF_REV_7xxx_INTR_STATUS(5)
> },
> - [MDP_INTF6_7xxx_INTR] = {
> + [MDP_INTF6_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(6),
> MDP_INTF_REV_7xxx_INTR_EN(6),
> MDP_INTF_REV_7xxx_INTR_STATUS(6)
> },
> - [MDP_INTF7_7xxx_INTR] = {
> + [MDP_INTF7_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(7),
> MDP_INTF_REV_7xxx_INTR_EN(7),
> MDP_INTF_REV_7xxx_INTR_STATUS(7)
> },
> - [MDP_INTF8_7xxx_INTR] = {
> + [MDP_INTF8_INTR] = {
> MDP_INTF_REV_7xxx_INTR_CLEAR(8),
> MDP_INTF_REV_7xxx_INTR_EN(8),
> MDP_INTF_REV_7xxx_INTR_STATUS(8)
> @@ -216,19 +235,19 @@ irqreturn_t dpu_core_irq(struct msm_kms *kms)
> return IRQ_NONE;
>
> spin_lock_irqsave(&intr->irq_lock, irq_flags);
> - for (reg_idx = 0; reg_idx < ARRAY_SIZE(dpu_intr_set); reg_idx++) {
> + for (reg_idx = 0; reg_idx < MDP_INTR_MAX; reg_idx++) {
> if (!test_bit(reg_idx, &intr->irq_mask))
> continue;
>
> /* Read interrupt status */
> - irq_status = DPU_REG_READ(&intr->hw, dpu_intr_set[reg_idx].status_off);
> + irq_status = DPU_REG_READ(&intr->hw, intr->intr_set[reg_idx].status_off);
>
> /* Read enable mask */
> - enable_mask = DPU_REG_READ(&intr->hw, dpu_intr_set[reg_idx].en_off);
> + enable_mask = DPU_REG_READ(&intr->hw, intr->intr_set[reg_idx].en_off);
>
> /* and clear the interrupt */
> if (irq_status)
> - DPU_REG_WRITE(&intr->hw, dpu_intr_set[reg_idx].clr_off,
> + DPU_REG_WRITE(&intr->hw, intr->intr_set[reg_idx].clr_off,
> irq_status);
>
> /* Finally update IRQ status based on enable mask */
> @@ -285,7 +304,11 @@ static int dpu_hw_intr_enable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
> assert_spin_locked(&intr->irq_lock);
>
> reg_idx = DPU_IRQ_REG(irq_idx);
> - reg = &dpu_intr_set[reg_idx];
> + reg = &intr->intr_set[reg_idx];
> +
> + /* Is this interrupt register supported on the platform */
> + if (WARN_ON(!reg->en_off))
> + return -EINVAL;
>
> cache_irq_mask = intr->cache_irq_mask[reg_idx];
> if (cache_irq_mask & DPU_IRQ_MASK(irq_idx)) {
> @@ -334,7 +357,7 @@ static int dpu_hw_intr_disable_irq_locked(struct dpu_hw_intr *intr, int irq_idx)
> assert_spin_locked(&intr->irq_lock);
>
> reg_idx = DPU_IRQ_REG(irq_idx);
> - reg = &dpu_intr_set[reg_idx];
> + reg = &intr->intr_set[reg_idx];
>
> cache_irq_mask = intr->cache_irq_mask[reg_idx];
> if ((cache_irq_mask & DPU_IRQ_MASK(irq_idx)) == 0) {
> @@ -368,10 +391,10 @@ static void dpu_clear_irqs(struct dpu_kms *dpu_kms)
> if (!intr)
> return;
>
> - for (i = 0; i < ARRAY_SIZE(dpu_intr_set); i++) {
> + for (i = 0; i < MDP_INTR_MAX; i++) {
> if (test_bit(i, &intr->irq_mask))
> DPU_REG_WRITE(&intr->hw,
> - dpu_intr_set[i].clr_off, 0xffffffff);
> + intr->intr_set[i].clr_off, 0xffffffff);
> }
>
> /* ensure register writes go through */
> @@ -386,10 +409,10 @@ static void dpu_disable_all_irqs(struct dpu_kms *dpu_kms)
> if (!intr)
> return;
>
> - for (i = 0; i < ARRAY_SIZE(dpu_intr_set); i++) {
> + for (i = 0; i < MDP_INTR_MAX; i++) {
> if (test_bit(i, &intr->irq_mask))
> DPU_REG_WRITE(&intr->hw,
> - dpu_intr_set[i].en_off, 0x00000000);
> + intr->intr_set[i].en_off, 0x00000000);
> }
>
> /* ensure register writes go through */
> @@ -421,10 +444,10 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms, int irq_idx)
>
> reg_idx = DPU_IRQ_REG(irq_idx);
> intr_status = DPU_REG_READ(&intr->hw,
> - dpu_intr_set[reg_idx].status_off) &
> + intr->intr_set[reg_idx].status_off) &
> DPU_IRQ_MASK(irq_idx);
> if (intr_status)
> - DPU_REG_WRITE(&intr->hw, dpu_intr_set[reg_idx].clr_off,
> + DPU_REG_WRITE(&intr->hw, intr->intr_set[reg_idx].clr_off,
> intr_status);
>
> /* ensure register writes go through */
> @@ -448,6 +471,11 @@ struct dpu_hw_intr *dpu_hw_intr_init(void __iomem *addr,
> if (!intr)
> return ERR_PTR(-ENOMEM);
>
> + if (m->caps->has_7xxx_intr)
> + intr->intr_set = dpu_intr_set_7xxx;
> + else
> + intr->intr_set = dpu_intr_set_legacy;
> +
> intr->hw.blk_addr = addr + m->mdp[0].base;
>
> intr->total_irqs = nirq;
> 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 1f2dabc54c22..f329d6d7f646 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
> @@ -23,24 +23,29 @@ enum dpu_hw_intr_reg {
> MDP_INTF3_INTR,
> MDP_INTF4_INTR,
> MDP_INTF5_INTR,
> + MDP_INTF6_INTR,
> + MDP_INTF7_INTR,
> + MDP_INTF8_INTR,
> MDP_INTF1_TEAR_INTR,
> MDP_INTF2_TEAR_INTR,
> MDP_AD4_0_INTR,
> MDP_AD4_1_INTR,
> - MDP_INTF0_7xxx_INTR,
> - MDP_INTF1_7xxx_INTR,
> - MDP_INTF1_7xxx_TEAR_INTR,
> - MDP_INTF2_7xxx_INTR,
> - MDP_INTF2_7xxx_TEAR_INTR,
> - MDP_INTF3_7xxx_INTR,
> - MDP_INTF4_7xxx_INTR,
> - MDP_INTF5_7xxx_INTR,
> - MDP_INTF6_7xxx_INTR,
> - MDP_INTF7_7xxx_INTR,
> - MDP_INTF8_7xxx_INTR,
> MDP_INTR_MAX,
> };
>
> +/* compatibility */
> +#define MDP_INTF0_7xxx_INTR MDP_INTF0_INTR
> +#define MDP_INTF1_7xxx_INTR MDP_INTF1_INTR
> +#define MDP_INTF2_7xxx_INTR MDP_INTF2_INTR
> +#define MDP_INTF3_7xxx_INTR MDP_INTF3_INTR
> +#define MDP_INTF4_7xxx_INTR MDP_INTF4_INTR
> +#define MDP_INTF5_7xxx_INTR MDP_INTF5_INTR
> +#define MDP_INTF6_7xxx_INTR MDP_INTF6_INTR
> +#define MDP_INTF7_7xxx_INTR MDP_INTF7_INTR
> +#define MDP_INTF8_7xxx_INTR MDP_INTF8_INTR
> +#define MDP_INTF1_7xxx_TEAR_INTR MDP_INTF1_TEAR_INTR
> +#define MDP_INTF2_7xxx_TEAR_INTR MDP_INTF2_TEAR_INTR
> +
> #define DPU_IRQ_IDX(reg_idx, offset) (reg_idx * 32 + offset)
>
> /**
> @@ -60,6 +65,7 @@ struct dpu_hw_intr {
> u32 total_irqs;
> spinlock_t irq_lock;
> unsigned long irq_mask;
> + const struct dpu_intr_reg *intr_set;
>
> struct {
> void (*cb)(void *arg, int irq_idx);
next prev parent reply other threads:[~2023-05-22 14:36 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-22 0:42 [PATCH 0/6] drm/msm/dpu: rework interrupt handling Dmitry Baryshkov
2023-05-22 0:42 ` [PATCH 1/6] drm/msm/dpu: don't set DPU_INTF_TE globally Dmitry Baryshkov
2023-05-22 14:29 ` Neil Armstrong
2023-05-22 0:42 ` [PATCH 2/6] drm/msm/dpu: inline __intr_offset Dmitry Baryshkov
2023-05-22 14:30 ` Neil Armstrong
2023-05-22 0:42 ` [PATCH 3/6] drm/msm/dpu: split interrupt address arrays Dmitry Baryshkov
2023-05-22 14:36 ` Neil Armstrong [this message]
2023-05-22 14:58 ` Dmitry Baryshkov
2023-05-22 0:42 ` [PATCH 4/6] drm/msm/dpu: autodetect supported interrupts Dmitry Baryshkov
2023-05-22 0:42 ` [PATCH 5/6] drm/msm/dpu: drop now-unused mdss_irqs field from hw catalog Dmitry Baryshkov
2023-05-22 14:37 ` Neil Armstrong
2023-05-22 0:42 ` [PATCH 6/6] drm/msm/dpu: drop compatibility INTR defines Dmitry Baryshkov
2023-05-22 14:37 ` Neil Armstrong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=80da4c26-ca3f-00c9-072c-087a1ff24c74@linaro.org \
--to=neil.armstrong@linaro.org \
--cc=airlied@gmail.com \
--cc=andersson@kernel.org \
--cc=daniel@ffwll.ch \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=quic_abhinavk@quicinc.com \
--cc=robdclark@gmail.com \
--cc=sean@poorly.run \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox