Rather than remasking interrupts after a device reset in the main reset
path, allow selecting whether to do this with an additional bool parameter.

To this end, split reenabling job interrupts into two functions, one that
clears the interrupts and another one which unmasks them conditionally.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++--
 drivers/gpu/drm/panfrost/panfrost_device.h |  2 +-
 drivers/gpu/drm/panfrost/panfrost_job.c    | 34 ++++++++++++----------
 drivers/gpu/drm/panfrost/panfrost_job.h    |  3 +-
 4 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 4fe29286bbe3..7e5d39699982 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -395,20 +395,25 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
 	return false;
 }
 
-void panfrost_device_reset(struct panfrost_device *pfdev)
+void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
+	u32 irq_mask;
+
 	panfrost_gpu_soft_reset(pfdev);
 
 	panfrost_gpu_power_on(pfdev);
 	panfrost_mmu_reset(pfdev);
-	panfrost_job_enable_interrupts(pfdev);
+
+	irq_mask = panfrost_job_reset_interrupts(pfdev);
+	if (enable_job_int)
+		panfrost_enable_interrupts(pfdev, irq_mask);
 }
 
 static int panfrost_device_runtime_resume(struct device *dev)
 {
 	struct panfrost_device *pfdev = dev_get_drvdata(dev);
 
-	panfrost_device_reset(pfdev);
+	panfrost_device_reset(pfdev, true);
 	panfrost_devfreq_resume(pfdev);
 
 	return 0;
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index d9aea2c2cbe5..fc83d5e104a3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -205,7 +205,7 @@ int panfrost_unstable_ioctl_check(void);
 
 int panfrost_device_init(struct panfrost_device *pfdev);
 void panfrost_device_fini(struct panfrost_device *pfdev);
-void panfrost_device_reset(struct panfrost_device *pfdev);
+void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
 
 extern const struct dev_pm_ops panfrost_pm_ops;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index fba1a376f593..79d2ee3625b2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -27,6 +27,10 @@
 #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
 #define job_read(dev, reg) readl(dev->iomem + (reg))
 
+#define JOB_INT_ENABLE_MASK			\
+	(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |	\
+	 GENMASK(NUM_JOB_SLOTS - 1, 0))
+
 struct panfrost_queue_state {
 	struct drm_gpu_scheduler sched;
 	u64 fence_context;
@@ -421,18 +425,23 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	return fence;
 }
 
-void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
+u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev)
 {
 	int j;
 	u32 irq_mask = 0;
 
-	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
-
 	for (j = 0; j < NUM_JOB_SLOTS; j++) {
 		irq_mask |= MK_JS_MASK(j);
 	}
 
 	job_write(pfdev, JOB_INT_CLEAR, irq_mask);
+
+	return irq_mask;
+}
+
+void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask)
+{
+	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
 	job_write(pfdev, JOB_INT_MASK, irq_mask);
 }
 
@@ -725,12 +734,7 @@ panfrost_reset(struct panfrost_device *pfdev,
 	spin_unlock(&pfdev->js->job_lock);
 
 	/* Proceed with reset now. */
-	panfrost_device_reset(pfdev);
-
-	/* panfrost_device_reset() unmasks job interrupts, but we want to
-	 * keep them masked a bit longer.
-	 */
-	job_write(pfdev, JOB_INT_MASK, 0);
+	panfrost_device_reset(pfdev, false);
 
 	/* GPU has been reset, we can clear the reset pending bit. */
 	atomic_set(&pfdev->reset.pending, 0);
@@ -752,9 +756,7 @@ panfrost_reset(struct panfrost_device *pfdev,
 		drm_sched_start(&pfdev->js->queue[i].sched, 0);
 
 	/* Re-enable job interrupts now that everything has been restarted. */
-	job_write(pfdev, JOB_INT_MASK,
-		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
-		  GENMASK(NUM_JOB_SLOTS - 1, 0));
+	panfrost_enable_interrupts(pfdev, JOB_INT_ENABLE_MASK);
 
 	dma_fence_end_signalling(cookie);
 }
@@ -827,9 +829,7 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
 
 	/* Enable interrupts only if we're not about to get suspended */
 	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
-		job_write(pfdev, JOB_INT_MASK,
-			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
-			  GENMASK(NUM_JOB_SLOTS - 1, 0));
+		job_write(pfdev, JOB_INT_MASK, JOB_INT_ENABLE_MASK);
 
 	return IRQ_HANDLED;
 }
@@ -854,6 +854,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
 {
 	struct panfrost_job_slot *js;
 	unsigned int nentries = 2;
+	u32 irq_mask;
 	int ret, j;
 
 	/* All GPUs have two entries per queue, but without jobchain
@@ -905,7 +906,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
 		}
 	}
 
-	panfrost_job_enable_interrupts(pfdev);
+	irq_mask = panfrost_job_reset_interrupts(pfdev);
+	panfrost_enable_interrupts(pfdev, irq_mask);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
index ec581b97852b..c09d42ee5039 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.h
+++ b/drivers/gpu/drm/panfrost/panfrost_job.h
@@ -46,7 +46,8 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
 int panfrost_job_get_slot(struct panfrost_job *job);
 int panfrost_job_push(struct panfrost_job *job);
 void panfrost_job_put(struct panfrost_job *job);
-void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
+u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev);
+void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask);
 void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
 int panfrost_job_is_idle(struct panfrost_device *pfdev);
 
-- 
2.47.0

On Thu, 28 Nov 2024 21:06:21 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Rather than remasking interrupts after a device reset in the main reset
> path, allow selecting whether to do this with an additional bool parameter.
> 
> To this end, split reenabling job interrupts into two functions, one that
> clears the interrupts and another one which unmasks them conditionally.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++--
>  drivers/gpu/drm/panfrost/panfrost_device.h |  2 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 34 ++++++++++++----------
>  drivers/gpu/drm/panfrost/panfrost_job.h    |  3 +-
>  4 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 4fe29286bbe3..7e5d39699982 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -395,20 +395,25 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  	return false;
>  }
>  
> -void panfrost_device_reset(struct panfrost_device *pfdev)
> +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  {
> +	u32 irq_mask;
> +
>  	panfrost_gpu_soft_reset(pfdev);
>  
>  	panfrost_gpu_power_on(pfdev);
>  	panfrost_mmu_reset(pfdev);
> -	panfrost_job_enable_interrupts(pfdev);
> +
> +	irq_mask = panfrost_job_reset_interrupts(pfdev);
> +	if (enable_job_int)
> +		panfrost_enable_interrupts(pfdev, irq_mask);
>  }
>  
>  static int panfrost_device_runtime_resume(struct device *dev)
>  {
>  	struct panfrost_device *pfdev = dev_get_drvdata(dev);
>  
> -	panfrost_device_reset(pfdev);
> +	panfrost_device_reset(pfdev, true);
>  	panfrost_devfreq_resume(pfdev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index d9aea2c2cbe5..fc83d5e104a3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -205,7 +205,7 @@ int panfrost_unstable_ioctl_check(void);
>  
>  int panfrost_device_init(struct panfrost_device *pfdev);
>  void panfrost_device_fini(struct panfrost_device *pfdev);
> -void panfrost_device_reset(struct panfrost_device *pfdev);
> +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
>  
>  extern const struct dev_pm_ops panfrost_pm_ops;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index fba1a376f593..79d2ee3625b2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -27,6 +27,10 @@
>  #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
>  #define job_read(dev, reg) readl(dev->iomem + (reg))
>  
> +#define JOB_INT_ENABLE_MASK			\
> +	(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |	\
> +	 GENMASK(NUM_JOB_SLOTS - 1, 0))
> +
>  struct panfrost_queue_state {
>  	struct drm_gpu_scheduler sched;
>  	u64 fence_context;
> @@ -421,18 +425,23 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
>  	return fence;
>  }
>  
> -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
> +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev)
>  {
>  	int j;
>  	u32 irq_mask = 0;
>  
> -	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
> -
>  	for (j = 0; j < NUM_JOB_SLOTS; j++) {
>  		irq_mask |= MK_JS_MASK(j);
>  	}
>  
>  	job_write(pfdev, JOB_INT_CLEAR, irq_mask);
> +
> +	return irq_mask;
> +}

Let's define an ALL_JS_INT_MASK so we can get rid of the for loop and
can avoid returning a value that's constant.

> +
> +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask)

Let's drop the irq_mask mask (use ALL_JS_INT_MASK), and call the
function panfrost_job_enable_interrupts() instead.

> +{
> +	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
>  	job_write(pfdev, JOB_INT_MASK, irq_mask);
>  }
>  
> @@ -725,12 +734,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>  	spin_unlock(&pfdev->js->job_lock);
>  
>  	/* Proceed with reset now. */
> -	panfrost_device_reset(pfdev);
> -
> -	/* panfrost_device_reset() unmasks job interrupts, but we want to
> -	 * keep them masked a bit longer.
> -	 */
> -	job_write(pfdev, JOB_INT_MASK, 0);
> +	panfrost_device_reset(pfdev, false);
>  
>  	/* GPU has been reset, we can clear the reset pending bit. */
>  	atomic_set(&pfdev->reset.pending, 0);
> @@ -752,9 +756,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>  		drm_sched_start(&pfdev->js->queue[i].sched, 0);
>  
>  	/* Re-enable job interrupts now that everything has been restarted. */
> -	job_write(pfdev, JOB_INT_MASK,
> -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
> +	panfrost_enable_interrupts(pfdev, JOB_INT_ENABLE_MASK);
>  
>  	dma_fence_end_signalling(cookie);
>  }
> @@ -827,9 +829,7 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
>  
>  	/* Enable interrupts only if we're not about to get suspended */
>  	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
> -		job_write(pfdev, JOB_INT_MASK,
> -			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> -			  GENMASK(NUM_JOB_SLOTS - 1, 0));
> +		job_write(pfdev, JOB_INT_MASK, JOB_INT_ENABLE_MASK);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -854,6 +854,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>  {
>  	struct panfrost_job_slot *js;
>  	unsigned int nentries = 2;
> +	u32 irq_mask;
>  	int ret, j;
>  
>  	/* All GPUs have two entries per queue, but without jobchain
> @@ -905,7 +906,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>  		}
>  	}
>  
> -	panfrost_job_enable_interrupts(pfdev);
> +	irq_mask = panfrost_job_reset_interrupts(pfdev);
> +	panfrost_enable_interrupts(pfdev, irq_mask);
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> index ec581b97852b..c09d42ee5039 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> @@ -46,7 +46,8 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
>  int panfrost_job_get_slot(struct panfrost_job *job);
>  int panfrost_job_push(struct panfrost_job *job);
>  void panfrost_job_put(struct panfrost_job *job);
> -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
> +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev);
> +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask);
>  void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
>  int panfrost_job_is_idle(struct panfrost_device *pfdev);
>  

Hi Boris,

On 02.12.2024 12:20, Boris Brezillon wrote:
> On Thu, 28 Nov 2024 21:06:21 +0000
> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> 
> > Rather than remasking interrupts after a device reset in the main reset
> > path, allow selecting whether to do this with an additional bool parameter.
> > 
> > To this end, split reenabling job interrupts into two functions, one that
> > clears the interrupts and another one which unmasks them conditionally.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++--
> >  drivers/gpu/drm/panfrost/panfrost_device.h |  2 +-
> >  drivers/gpu/drm/panfrost/panfrost_job.c    | 34 ++++++++++++----------
> >  drivers/gpu/drm/panfrost/panfrost_job.h    |  3 +-
> >  4 files changed, 29 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> > index 4fe29286bbe3..7e5d39699982 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > @@ -395,20 +395,25 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
> >  	return false;
> >  }
> >  
> > -void panfrost_device_reset(struct panfrost_device *pfdev)
> > +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
> >  {
> > +	u32 irq_mask;
> > +
> >  	panfrost_gpu_soft_reset(pfdev);
> >  
> >  	panfrost_gpu_power_on(pfdev);
> >  	panfrost_mmu_reset(pfdev);
> > -	panfrost_job_enable_interrupts(pfdev);
> > +
> > +	irq_mask = panfrost_job_reset_interrupts(pfdev);
> > +	if (enable_job_int)
> > +		panfrost_enable_interrupts(pfdev, irq_mask);
> >  }
> >  
> >  static int panfrost_device_runtime_resume(struct device *dev)
> >  {
> >  	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> >  
> > -	panfrost_device_reset(pfdev);
> > +	panfrost_device_reset(pfdev, true);
> >  	panfrost_devfreq_resume(pfdev);
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> > index d9aea2c2cbe5..fc83d5e104a3 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> > @@ -205,7 +205,7 @@ int panfrost_unstable_ioctl_check(void);
> >  
> >  int panfrost_device_init(struct panfrost_device *pfdev);
> >  void panfrost_device_fini(struct panfrost_device *pfdev);
> > -void panfrost_device_reset(struct panfrost_device *pfdev);
> > +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
> >  
> >  extern const struct dev_pm_ops panfrost_pm_ops;
> >  
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index fba1a376f593..79d2ee3625b2 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -27,6 +27,10 @@
> >  #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
> >  #define job_read(dev, reg) readl(dev->iomem + (reg))
> >  
> > +#define JOB_INT_ENABLE_MASK			\
> > +	(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |	\
> > +	 GENMASK(NUM_JOB_SLOTS - 1, 0))
> > +
> >  struct panfrost_queue_state {
> >  	struct drm_gpu_scheduler sched;
> >  	u64 fence_context;
> > @@ -421,18 +425,23 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
> >  	return fence;
> >  }
> >  
> > -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
> > +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev)
> >  {
> >  	int j;
> >  	u32 irq_mask = 0;
> >  
> > -	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
> > -
> >  	for (j = 0; j < NUM_JOB_SLOTS; j++) {
> >  		irq_mask |= MK_JS_MASK(j);
> >  	}
> >  
> >  	job_write(pfdev, JOB_INT_CLEAR, irq_mask);
> > +
> > +	return irq_mask;
> > +}
> 
> Let's define an ALL_JS_INT_MASK so we can get rid of the for loop and
> can avoid returning a value that's constant.

Because ALL_JS_INT_MASK would be defined as an OR of MK_JS_MASK() applications,
and this macro is defined in panfrost_regs.h, I can't think of a nice location
for it that would be suitable for all the files where it would be called.

Maybe I could implement the same loop inside panfrost_job_init() where it would be
called only once, and store the result in a const struct panfrost_job_slot field?

> > +
> > +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask)
> 
> Let's drop the irq_mask mask (use ALL_JS_INT_MASK), and call the
> function panfrost_job_enable_interrupts() instead.

I made a mistake here naming this function, it should've been panfrost_job_enable_interrupts().

Other than that, I decided to keep the irq_mask argument because I'm also calling it from
the very end of panfrost_reset() with JOB_INT_ENABLE_MASK, where I defined it to be

(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) | GENMASK(NUM_JOB_SLOTS - 1, 0))

That raises the question, what is the functional difference between writing either
this or MK_JS_MASK(0) | MK_JS_MASK(1) | MK_JS_MASK(2) into the JOB_INT_MASK register?

It's also being done in panfrost_job_irq_handler_thread() so I'm not quite sure of this.

> > +{
> > +	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
> >  	job_write(pfdev, JOB_INT_MASK, irq_mask);
> >  }
> >  
> > @@ -725,12 +734,7 @@ panfrost_reset(struct panfrost_device *pfdev,
> >  	spin_unlock(&pfdev->js->job_lock);
> >  
> >  	/* Proceed with reset now. */
> > -	panfrost_device_reset(pfdev);
> > -
> > -	/* panfrost_device_reset() unmasks job interrupts, but we want to
> > -	 * keep them masked a bit longer.
> > -	 */
> > -	job_write(pfdev, JOB_INT_MASK, 0);
> > +	panfrost_device_reset(pfdev, false);
> >  
> >  	/* GPU has been reset, we can clear the reset pending bit. */
> >  	atomic_set(&pfdev->reset.pending, 0);
> > @@ -752,9 +756,7 @@ panfrost_reset(struct panfrost_device *pfdev,
> >  		drm_sched_start(&pfdev->js->queue[i].sched, 0);
> >  
> >  	/* Re-enable job interrupts now that everything has been restarted. */
> > -	job_write(pfdev, JOB_INT_MASK,
> > -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> > -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
> > +	panfrost_enable_interrupts(pfdev, JOB_INT_ENABLE_MASK);
> >  
> >  	dma_fence_end_signalling(cookie);
> >  }
> > @@ -827,9 +829,7 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
> >  
> >  	/* Enable interrupts only if we're not about to get suspended */
> >  	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
> > -		job_write(pfdev, JOB_INT_MASK,
> > -			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> > -			  GENMASK(NUM_JOB_SLOTS - 1, 0));
> > +		job_write(pfdev, JOB_INT_MASK, JOB_INT_ENABLE_MASK);
> >  
> >  	return IRQ_HANDLED;
> >  }
> > @@ -854,6 +854,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
> >  {
> >  	struct panfrost_job_slot *js;
> >  	unsigned int nentries = 2;
> > +	u32 irq_mask;
> >  	int ret, j;
> >  
> >  	/* All GPUs have two entries per queue, but without jobchain
> > @@ -905,7 +906,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
> >  		}
> >  	}
> >  
> > -	panfrost_job_enable_interrupts(pfdev);
> > +	irq_mask = panfrost_job_reset_interrupts(pfdev);
> > +	panfrost_enable_interrupts(pfdev, irq_mask);
> >  
> >  	return 0;
> >  
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> > index ec581b97852b..c09d42ee5039 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> > @@ -46,7 +46,8 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
> >  int panfrost_job_get_slot(struct panfrost_job *job);
> >  int panfrost_job_push(struct panfrost_job *job);
> >  void panfrost_job_put(struct panfrost_job *job);
> > -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
> > +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev);
> > +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask);
> >  void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
> >  int panfrost_job_is_idle(struct panfrost_device *pfdev);
> >  

On 04/12/2024 15:34, Adrián Larumbe wrote:
> Hi Boris,
> 
> On 02.12.2024 12:20, Boris Brezillon wrote:
>> On Thu, 28 Nov 2024 21:06:21 +0000
>> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
>>
>>> Rather than remasking interrupts after a device reset in the main reset
>>> path, allow selecting whether to do this with an additional bool parameter.
>>>
>>> To this end, split reenabling job interrupts into two functions, one that
>>> clears the interrupts and another one which unmasks them conditionally.
>>>
>>> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++--
>>>  drivers/gpu/drm/panfrost/panfrost_device.h |  2 +-
>>>  drivers/gpu/drm/panfrost/panfrost_job.c    | 34 ++++++++++++----------
>>>  drivers/gpu/drm/panfrost/panfrost_job.h    |  3 +-
>>>  4 files changed, 29 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> index 4fe29286bbe3..7e5d39699982 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> @@ -395,20 +395,25 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>>>  	return false;
>>>  }
>>>  
>>> -void panfrost_device_reset(struct panfrost_device *pfdev)
>>> +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>>>  {
>>> +	u32 irq_mask;
>>> +
>>>  	panfrost_gpu_soft_reset(pfdev);
>>>  
>>>  	panfrost_gpu_power_on(pfdev);
>>>  	panfrost_mmu_reset(pfdev);
>>> -	panfrost_job_enable_interrupts(pfdev);
>>> +
>>> +	irq_mask = panfrost_job_reset_interrupts(pfdev);
>>> +	if (enable_job_int)
>>> +		panfrost_enable_interrupts(pfdev, irq_mask);
>>>  }
>>>  
>>>  static int panfrost_device_runtime_resume(struct device *dev)
>>>  {
>>>  	struct panfrost_device *pfdev = dev_get_drvdata(dev);
>>>  
>>> -	panfrost_device_reset(pfdev);
>>> +	panfrost_device_reset(pfdev, true);
>>>  	panfrost_devfreq_resume(pfdev);
>>>  
>>>  	return 0;
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
>>> index d9aea2c2cbe5..fc83d5e104a3 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>>> @@ -205,7 +205,7 @@ int panfrost_unstable_ioctl_check(void);
>>>  
>>>  int panfrost_device_init(struct panfrost_device *pfdev);
>>>  void panfrost_device_fini(struct panfrost_device *pfdev);
>>> -void panfrost_device_reset(struct panfrost_device *pfdev);
>>> +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
>>>  
>>>  extern const struct dev_pm_ops panfrost_pm_ops;
>>>  
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>>> index fba1a376f593..79d2ee3625b2 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>>> @@ -27,6 +27,10 @@
>>>  #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
>>>  #define job_read(dev, reg) readl(dev->iomem + (reg))
>>>  
>>> +#define JOB_INT_ENABLE_MASK			\
>>> +	(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |	\
>>> +	 GENMASK(NUM_JOB_SLOTS - 1, 0))
>>> +
>>>  struct panfrost_queue_state {
>>>  	struct drm_gpu_scheduler sched;
>>>  	u64 fence_context;
>>> @@ -421,18 +425,23 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
>>>  	return fence;
>>>  }
>>>  
>>> -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
>>> +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev)
>>>  {
>>>  	int j;
>>>  	u32 irq_mask = 0;
>>>  
>>> -	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
>>> -
>>>  	for (j = 0; j < NUM_JOB_SLOTS; j++) {
>>>  		irq_mask |= MK_JS_MASK(j);
>>>  	}
>>>  
>>>  	job_write(pfdev, JOB_INT_CLEAR, irq_mask);
>>> +
>>> +	return irq_mask;
>>> +}
>>
>> Let's define an ALL_JS_INT_MASK so we can get rid of the for loop and
>> can avoid returning a value that's constant.
> 
> Because ALL_JS_INT_MASK would be defined as an OR of MK_JS_MASK() applications,
> and this macro is defined in panfrost_regs.h, I can't think of a nice location
> for it that would be suitable for all the files where it would be called.

I can't speak for Boris, but I'd be quite happy with a:

#define ALL_JS_INT_MASK 0x70007

We know NUM_JOB_SLOTS is a (hardware) constant so we can compute the
value once. That or MK_JS_MASK(0)|MK_JS_MASK(1)|MK_JS_MASK(2) if you
prefer, or of course the GENMASK() variant below.

> Maybe I could implement the same loop inside panfrost_job_init() where it would be
> called only once, and store the result in a const struct panfrost_job_slot field?

It seems silly wasting memory on a compile time constant...

>>> +
>>> +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask)
>>
>> Let's drop the irq_mask mask (use ALL_JS_INT_MASK), and call the
>> function panfrost_job_enable_interrupts() instead.
> 
> I made a mistake here naming this function, it should've been panfrost_job_enable_interrupts().
> 
> Other than that, I decided to keep the irq_mask argument because I'm also calling it from
> the very end of panfrost_reset() with JOB_INT_ENABLE_MASK, where I defined it to be
> 
> (GENMASK(16 + NUM_JOB_SLOTS - 1, 16) | GENMASK(NUM_JOB_SLOTS - 1, 0))
> 
> That raises the question, what is the functional difference between writing either
> this or MK_JS_MASK(0) | MK_JS_MASK(1) | MK_JS_MASK(2) into the JOB_INT_MASK register?

Hopefully none - the two values should be the same. Indeed the GENMASK
variant is possibly best because it encodes using NUM_JOB_SLOTS.
Although I have to admit I have to think harder to decode the GENMASK()
version than either of the other alternatives above.

Steve

> It's also being done in panfrost_job_irq_handler_thread() so I'm not quite sure of this.
> 
>>> +{
>>> +	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
>>>  	job_write(pfdev, JOB_INT_MASK, irq_mask);
>>>  }
>>>  
>>> @@ -725,12 +734,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>>>  	spin_unlock(&pfdev->js->job_lock);
>>>  
>>>  	/* Proceed with reset now. */
>>> -	panfrost_device_reset(pfdev);
>>> -
>>> -	/* panfrost_device_reset() unmasks job interrupts, but we want to
>>> -	 * keep them masked a bit longer.
>>> -	 */
>>> -	job_write(pfdev, JOB_INT_MASK, 0);
>>> +	panfrost_device_reset(pfdev, false);
>>>  
>>>  	/* GPU has been reset, we can clear the reset pending bit. */
>>>  	atomic_set(&pfdev->reset.pending, 0);
>>> @@ -752,9 +756,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>>>  		drm_sched_start(&pfdev->js->queue[i].sched, 0);
>>>  
>>>  	/* Re-enable job interrupts now that everything has been restarted. */
>>> -	job_write(pfdev, JOB_INT_MASK,
>>> -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
>>> -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
>>> +	panfrost_enable_interrupts(pfdev, JOB_INT_ENABLE_MASK);
>>>  
>>>  	dma_fence_end_signalling(cookie);
>>>  }
>>> @@ -827,9 +829,7 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
>>>  
>>>  	/* Enable interrupts only if we're not about to get suspended */
>>>  	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
>>> -		job_write(pfdev, JOB_INT_MASK,
>>> -			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
>>> -			  GENMASK(NUM_JOB_SLOTS - 1, 0));
>>> +		job_write(pfdev, JOB_INT_MASK, JOB_INT_ENABLE_MASK);
>>>  
>>>  	return IRQ_HANDLED;
>>>  }
>>> @@ -854,6 +854,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>>>  {
>>>  	struct panfrost_job_slot *js;
>>>  	unsigned int nentries = 2;
>>> +	u32 irq_mask;
>>>  	int ret, j;
>>>  
>>>  	/* All GPUs have two entries per queue, but without jobchain
>>> @@ -905,7 +906,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>>>  		}
>>>  	}
>>>  
>>> -	panfrost_job_enable_interrupts(pfdev);
>>> +	irq_mask = panfrost_job_reset_interrupts(pfdev);
>>> +	panfrost_enable_interrupts(pfdev, irq_mask);
>>>  
>>>  	return 0;
>>>  
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
>>> index ec581b97852b..c09d42ee5039 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
>>> @@ -46,7 +46,8 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
>>>  int panfrost_job_get_slot(struct panfrost_job *job);
>>>  int panfrost_job_push(struct panfrost_job *job);
>>>  void panfrost_job_put(struct panfrost_job *job);
>>> -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
>>> +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev);
>>> +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask);
>>>  void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
>>>  int panfrost_job_is_idle(struct panfrost_device *pfdev);
>>>