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.laru...@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.laru...@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);
>>>
