Re: [PATCH v2 6/8] drm/panfrost: Make re-enabling job interrupts at device reset optional

[Steven Price <steven.price@arm.com>]

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);
>>>