[PATCH v2 3/4] drm/msm/dpu: add helper to get IRQ-related data
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Thu Jul 27 19:51:32 UTC 2023
On 27/07/2023 22:41, Marijn Suijten wrote:
> On 2023-07-27 22:34:59, Dmitry Baryshkov wrote:
>> On 27/07/2023 22:29, Marijn Suijten wrote:
>>> On 2023-07-27 18:04:54, Dmitry Baryshkov wrote:
>>>> In preparation to reworking the IRQ indices, move irq_tbl access to
>>>> separate helper.
>>>
>>> I am not seeing the advantage of the helper, but making every function
>>> look up dpu_kms->hw_intr->irq_tbl[irq_idx] only once and storing that in
>>> a local dpu_hw_intr_entry pointer is much tidier.
>>
>> There was a bonus point when I tried to do a irq_idx-1 in the next
>> patch. But since that code has gone, maybe I can drop this patch too.
>
> Don't drop the whole patch though. While maybe not necessary, having
> the lookup only once is much easier to follow.
Then it's easier to keep it as is.
>
> - Marijn
>
>>> Maybe I expected it to do extra mutations to irq_idx in 4/4?
>>>
>>>> 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 | 48 +++++++++++++------
>>>> .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 12 +++--
>>>> 2 files changed, 41 insertions(+), 19 deletions(-)
>>>>
>>>> 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 eaae7f11f57f..ede7161ae904 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
>>>> @@ -199,6 +199,12 @@ static const struct dpu_intr_reg dpu_intr_set_7xxx[] = {
>>>>
>>>> #define DPU_IRQ_MASK(irq_idx) (BIT(DPU_IRQ_OFFSET(irq_idx)))
>>>>
>>>> +static inline struct dpu_hw_intr_entry *dpu_core_irq_get_entry(struct dpu_kms *dpu_kms,
>>>> + int irq_idx)
>>>> +{
>>>> + return &dpu_kms->hw_intr->irq_tbl[irq_idx];
>>>> +}
>>>> +
>>>> /**
>>>> * dpu_core_irq_callback_handler - dispatch core interrupts
>>>> * @dpu_kms: Pointer to DPU's KMS structure
>>>> @@ -206,17 +212,19 @@ static const struct dpu_intr_reg dpu_intr_set_7xxx[] = {
>>>> */
>>>> static void dpu_core_irq_callback_handler(struct dpu_kms *dpu_kms, int irq_idx)
>>>> {
>>>> + struct dpu_hw_intr_entry *irq_entry = dpu_core_irq_get_entry(dpu_kms, irq_idx);
>>>> +
>>>> VERB("irq_idx=%d\n", irq_idx);
>>>>
>>>> - if (!dpu_kms->hw_intr->irq_tbl[irq_idx].cb)
>>>> + if (!irq_entry->cb)
>>>> DRM_ERROR("no registered cb, idx:%d\n", irq_idx);
>>>>
>>>> - atomic_inc(&dpu_kms->hw_intr->irq_tbl[irq_idx].count);
>>>> + atomic_inc(&irq_entry->count);
>>>>
>>>> /*
>>>> * Perform registered function callback
>>>> */
>>>> - dpu_kms->hw_intr->irq_tbl[irq_idx].cb(dpu_kms->hw_intr->irq_tbl[irq_idx].arg);
>>>> + irq_entry->cb(irq_entry->arg);
>>>> }
>>>>
>>>> irqreturn_t dpu_core_irq(struct msm_kms *kms)
>>>> @@ -509,6 +517,7 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms, int irq_idx,
>>>> void (*irq_cb)(void *arg),
>>>> void *irq_arg)
>>>> {
>>>> + struct dpu_hw_intr_entry *irq_entry;
>>>> unsigned long irq_flags;
>>>> int ret;
>>>>
>>>> @@ -526,15 +535,16 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms, int irq_idx,
>>>>
>>>> spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags);
>>>>
>>>> - if (unlikely(WARN_ON(dpu_kms->hw_intr->irq_tbl[irq_idx].cb))) {
>>>> + irq_entry = dpu_core_irq_get_entry(dpu_kms, irq_idx);
>>>> + if (unlikely(WARN_ON(irq_entry->cb))) {
>>>> spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
>>>>
>>>> return -EBUSY;
>>>> }
>>>>
>>>> trace_dpu_core_irq_register_callback(irq_idx, irq_cb);
>>>> - dpu_kms->hw_intr->irq_tbl[irq_idx].arg = irq_arg;
>>>> - dpu_kms->hw_intr->irq_tbl[irq_idx].cb = irq_cb;
>>>> + irq_entry->arg = irq_arg;
>>>> + irq_entry->cb = irq_cb;
>>>>
>>>> ret = dpu_hw_intr_enable_irq_locked(
>>>> dpu_kms->hw_intr,
>>>> @@ -551,6 +561,7 @@ int dpu_core_irq_register_callback(struct dpu_kms *dpu_kms, int irq_idx,
>>>>
>>>> int dpu_core_irq_unregister_callback(struct dpu_kms *dpu_kms, int irq_idx)
>>>> {
>>>> + struct dpu_hw_intr_entry *irq_entry;
>>>> unsigned long irq_flags;
>>>> int ret;
>>>>
>>>> @@ -569,8 +580,9 @@ int dpu_core_irq_unregister_callback(struct dpu_kms *dpu_kms, int irq_idx)
>>>> DPU_ERROR("Fail to disable IRQ for irq_idx:%d: %d\n",
>>>> irq_idx, ret);
>>>>
>>>> - dpu_kms->hw_intr->irq_tbl[irq_idx].cb = NULL;
>>>> - dpu_kms->hw_intr->irq_tbl[irq_idx].arg = NULL;
>>>> + irq_entry = dpu_core_irq_get_entry(dpu_kms, irq_idx);
>>>> + irq_entry->cb = NULL;
>>>> + irq_entry->arg = NULL;
>>>>
>>>> spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
>>>>
>>>> @@ -583,14 +595,16 @@ int dpu_core_irq_unregister_callback(struct dpu_kms *dpu_kms, int irq_idx)
>>>> static int dpu_debugfs_core_irq_show(struct seq_file *s, void *v)
>>>> {
>>>> struct dpu_kms *dpu_kms = s->private;
>>>> + struct dpu_hw_intr_entry *irq_entry;
>>>> unsigned long irq_flags;
>>>> int i, irq_count;
>>>> void *cb;
>>>>
>>>> for (i = 0; i < dpu_kms->hw_intr->total_irqs; i++) {
>>>> spin_lock_irqsave(&dpu_kms->hw_intr->irq_lock, irq_flags);
>>>> - irq_count = atomic_read(&dpu_kms->hw_intr->irq_tbl[i].count);
>>>> - cb = dpu_kms->hw_intr->irq_tbl[i].cb;
>>>> + irq_entry = dpu_core_irq_get_entry(dpu_kms, i);
>>>> + irq_count = atomic_read(&irq_entry->count);
>>>> + cb = irq_entry->cb;
>>>> spin_unlock_irqrestore(&dpu_kms->hw_intr->irq_lock, irq_flags);
>>>>
>>>> if (irq_count || cb)
>>>> @@ -613,6 +627,7 @@ void dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms,
>>>> void dpu_core_irq_preinstall(struct msm_kms *kms)
>>>> {
>>>> struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>>> + struct dpu_hw_intr_entry *irq_entry;
>>>> int i;
>>>>
>>>> pm_runtime_get_sync(&dpu_kms->pdev->dev);
>>>> @@ -620,22 +635,27 @@ void dpu_core_irq_preinstall(struct msm_kms *kms)
>>>> dpu_disable_all_irqs(dpu_kms);
>>>> pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>>
>>>> - for (i = 0; i < dpu_kms->hw_intr->total_irqs; i++)
>>>> - atomic_set(&dpu_kms->hw_intr->irq_tbl[i].count, 0);
>>>> + for (i = 0; i < dpu_kms->hw_intr->total_irqs; i++) {
>>>> + irq_entry = dpu_core_irq_get_entry(dpu_kms, i);
>>>> + atomic_set(&irq_entry->count, 0);
>>>> + }
>>>> }
>>>>
>>>> void dpu_core_irq_uninstall(struct msm_kms *kms)
>>>> {
>>>> struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>>> + struct dpu_hw_intr_entry *irq_entry;
>>>> int i;
>>>>
>>>> if (!dpu_kms->hw_intr)
>>>> return;
>>>>
>>>> pm_runtime_get_sync(&dpu_kms->pdev->dev);
>>>> - for (i = 0; i < dpu_kms->hw_intr->total_irqs; i++)
>>>> - if (dpu_kms->hw_intr->irq_tbl[i].cb)
>>>> + for (i = 0; i < dpu_kms->hw_intr->total_irqs; i++) {
>>>> + irq_entry = dpu_core_irq_get_entry(dpu_kms, i);
>>>> + if (irq_entry->cb)
>>>> DPU_ERROR("irq_idx=%d still enabled/registered\n", i);
>>>> + }
>>>>
>>>> dpu_clear_irqs(dpu_kms);
>>>> dpu_disable_all_irqs(dpu_kms);
>>>> 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 3a988a4e4f33..59bde8ab50c8 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
>>>> @@ -40,6 +40,12 @@ enum dpu_hw_intr_reg {
>>>> #define DPU_IRQ_REG(irq_idx) ((irq_idx) / 32)
>>>> #define DPU_IRQ_OFFSET(irq_idx) ((irq_idx) % 32)
>>>>
>>>> +struct dpu_hw_intr_entry {
>>>> + void (*cb)(void *arg);
>>>> + void *arg;
>>>> + atomic_t count;
>>>> +};
>>>> +
>>>> /**
>>>> * struct dpu_hw_intr: hw interrupts handling data structure
>>>> * @hw: virtual address mapping
>>>> @@ -59,11 +65,7 @@ struct dpu_hw_intr {
>>>> unsigned long irq_mask;
>>>> const struct dpu_intr_reg *intr_set;
>>>>
>>>> - struct {
>>>> - void (*cb)(void *arg);
>>>> - void *arg;
>>>> - atomic_t count;
>>>> - } irq_tbl[];
>>>> + struct dpu_hw_intr_entry irq_tbl[];
>>>> };
>>>>
>>>> /**
>>>> --
>>>> 2.39.2
>>>>
>>
>> --
>> With best wishes
>> Dmitry
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list