Re: [PATCH v2 3/4] drm/msm/dpu: add helper to get IRQ-related data

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

- Marijn

> > Maybe I expected it to do extra mutations to irq_idx in 4/4?
> > 
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> > 
> >> ---
> >>   .../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
> 



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux