[Freedreno] [PATCH v2 3/6] drm/msm/dpu: split interrupt address arrays
Abhinav Kumar
quic_abhinavk at quicinc.com
Thu May 25 22:42:45 UTC 2023
On 5/25/2023 3:30 PM, Dmitry Baryshkov wrote:
> On Fri, 26 May 2023 at 00:40, Jeykumar Sankaran
> <quic_jeykumar at quicinc.com> wrote:
>>
>>
>>
>> On 5/22/2023 2:45 PM, 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 at linaro.org>
>>> ---
>>> .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 1 +
>>> .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 1 +
>>> .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 1 +
>>> .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 1 +
>>> .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 1 +
>>> .../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 ++++---
>>> 8 files changed, 79 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>>> index 3c1b2c13398d..320cfa4be633 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>>> @@ -15,6 +15,7 @@ static const struct dpu_caps sm8350_dpu_caps = {
>>> .has_dim_layer = true,
>>> .has_idle_pc = true,
>>> .has_3d_merge = true,
>>> + .has_7xxx_intr = true,
>>> .max_linewidth = 4096,
>>> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>> };
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>> index 5d894cbb0a62..9306c7a115e9 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>> @@ -13,6 +13,7 @@ static const struct dpu_caps sc7280_dpu_caps = {
>>> .qseed_type = DPU_SSPP_SCALER_QSEED4,
>>> .has_dim_layer = true,
>>> .has_idle_pc = true,
>>> + .has_7xxx_intr = true,
>>> .max_linewidth = 2400,
>>> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>> };
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>>> index c3f1ae000a21..fc1e17c495f0 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>>> @@ -15,6 +15,7 @@ static const struct dpu_caps sc8280xp_dpu_caps = {
>>> .has_dim_layer = true,
>>> .has_idle_pc = true,
>>> .has_3d_merge = true,
>>> + .has_7xxx_intr = true,
>>> .max_linewidth = 5120,
>>> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>> };
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>>> index 86c2e68ebd2c..eb72411c16db 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>>> @@ -14,6 +14,7 @@ static const struct dpu_caps sm8450_dpu_caps = {
>>> .has_src_split = true,
>>> .has_dim_layer = true,
>>> .has_idle_pc = true,
>>> + .has_7xxx_intr = true,
>>> .has_3d_merge = true,
>>> .max_linewidth = 5120,
>>> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>>> index 85dc34458b88..8209ca317bdc 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>>> @@ -15,6 +15,7 @@ static const struct dpu_caps sm8550_dpu_caps = {
>>> .has_dim_layer = true,
>>> .has_idle_pc = true,
>>> .has_3d_merge = true,
>>> + .has_7xxx_intr = true,
>>> .max_linewidth = 5120,
>>> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>> };
>>> 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
>>
>> I see the requirement to distinguish feature support based on the DPU
>> version in more than one series. Is it a good idea to bring in the DPU
>> version info in chipset catalog? This will relieve us from maintaining
>> such version flags for individual HW sub-blocks.
>
> This would not play well with the rest of the driver. The driver
> usually does not compute features by DPU revision. Instead it lists
> feature flags.
>
So I am increasingly seeing examples such as data_compress, widebus
where it looks like version based enablement in the code will be just
more efficient. For example.
if (DPU_MAJOR_VER > xxx && DPU_MAJOR_VER < yyy)
enable data_compress;
will be much easier to do than adding catalog entry for each chipset for
these bit level details of registers especially when some of these
cannot be considered as catalog features.
Thats why I am wondering that, we dont need to add the revision based
cfg picking anymore and rely on the compatible but in the dpu_mdss_cfg
perhaps add a .core_rev.
We will still stick to catalog based feature bits when its actually
indeed a feature.
Thoughts?
>>
>> Thanks and Regards,
>> Jeykumar S.
>>
>>> * @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;
>>> /* 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);
>
>
>
More information about the dri-devel
mailing list