[PATCH 1/2] accel/ivpu: Do not trigger extra VPU reset if the VPU is idle

[PATCH 1/2] accel/ivpu: Do not trigger extra VPU reset if the VPU is idle

[Stanislaw Gruszka]

From: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>

Turning off the PLL and entering D0i3 will reset the VPU so
an explicit IP reset is redundant.
But if the VPU is active, it may interfere with PLL disabling
and to avoid that, we have to issue an additional IP reset
to silence the VPU before turning off the PLL.

Fixes: a8fed6d1e0b9 ("accel/ivpu: Fix power down sequence")
Signed-off-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_hw_mtl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_hw_mtl.c b/drivers/accel/ivpu/ivpu_hw_mtl.c
index 3210f1b4a7dd..012a25fae16d 100644
--- a/drivers/accel/ivpu/ivpu_hw_mtl.c
+++ b/drivers/accel/ivpu/ivpu_hw_mtl.c
@@ -743,9 +743,8 @@ static int ivpu_hw_mtl_power_down(struct ivpu_device *vdev)
 {
 	int ret = 0;
 
-	if (ivpu_hw_mtl_reset(vdev)) {
+	if (!ivpu_hw_mtl_is_idle(vdev) && ivpu_hw_mtl_reset(vdev)) {
 		ivpu_err(vdev, "Failed to reset the VPU\n");
-		ret = -EIO;
 	}
 
 	if (ivpu_pll_disable(vdev)) {
@@ -753,8 +752,10 @@ static int ivpu_hw_mtl_power_down(struct ivpu_device *vdev)
 		ret = -EIO;
 	}
 
-	if (ivpu_hw_mtl_d0i3_enable(vdev))
-		ivpu_warn(vdev, "Failed to enable D0I3\n");
+	if (ivpu_hw_mtl_d0i3_enable(vdev)) {
+		ivpu_err(vdev, "Failed to enter D0I3\n");
+		ret = -EIO;
+	}
 
 	return ret;
 }
-- 
2.25.1

[PATCH 2/2] accel/ivpu: Do not use mutex_lock_interruptible

[Stanislaw Gruszka]

If we get signal when waiting for the mmu->lock we do not invalidate
current MMU configuration what might result on undefined behavior.

Additionally there is little or no benefit on break waiting for
ipc->lock. In current code base, we keep this lock for short periods.

Fixes: 263b2ba5fc93 ("accel/ivpu: Add Intel VPU MMU support")
Reviewed-by: Krystian Pradzynski <krystian.pradzynski@linux.intel.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_ipc.c |  4 +---
 drivers/accel/ivpu/ivpu_mmu.c | 22 ++++++----------------
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_ipc.c b/drivers/accel/ivpu/ivpu_ipc.c
index 3adcfa80fc0e..fa0af59e39ab 100644
--- a/drivers/accel/ivpu/ivpu_ipc.c
+++ b/drivers/accel/ivpu/ivpu_ipc.c
@@ -183,9 +183,7 @@ ivpu_ipc_send(struct ivpu_device *vdev, struct ivpu_ipc_consumer *cons, struct v
 	struct ivpu_ipc_info *ipc = vdev->ipc;
 	int ret;
 
-	ret = mutex_lock_interruptible(&ipc->lock);
-	if (ret)
-		return ret;
+	mutex_lock(&ipc->lock);
 
 	if (!ipc->on) {
 		ret = -EAGAIN;
diff --git a/drivers/accel/ivpu/ivpu_mmu.c b/drivers/accel/ivpu/ivpu_mmu.c
index fa9a9ad59643..53878e77aad3 100644
--- a/drivers/accel/ivpu/ivpu_mmu.c
+++ b/drivers/accel/ivpu/ivpu_mmu.c
@@ -597,16 +597,11 @@ static int ivpu_mmu_strtab_init(struct ivpu_device *vdev)
 int ivpu_mmu_invalidate_tlb(struct ivpu_device *vdev, u16 ssid)
 {
 	struct ivpu_mmu_info *mmu = vdev->mmu;
-	int ret;
-
-	ret = mutex_lock_interruptible(&mmu->lock);
-	if (ret)
-		return ret;
+	int ret = 0;
 
-	if (!mmu->on) {
-		ret = 0;
+	mutex_lock(&mmu->lock);
+	if (!mmu->on)
 		goto unlock;
-	}
 
 	ret = ivpu_mmu_cmdq_write_tlbi_nh_asid(vdev, ssid);
 	if (ret)
@@ -624,7 +619,7 @@ static int ivpu_mmu_cd_add(struct ivpu_device *vdev, u32 ssid, u64 cd_dma)
 	struct ivpu_mmu_cdtab *cdtab = &mmu->cdtab;
 	u64 *entry;
 	u64 cd[4];
-	int ret;
+	int ret = 0;
 
 	if (ssid > IVPU_MMU_CDTAB_ENT_COUNT)
 		return -EINVAL;
@@ -665,14 +660,9 @@ static int ivpu_mmu_cd_add(struct ivpu_device *vdev, u32 ssid, u64 cd_dma)
 	ivpu_dbg(vdev, MMU, "CDTAB %s entry (SSID=%u, dma=%pad): 0x%llx, 0x%llx, 0x%llx, 0x%llx\n",
 		 cd_dma ? "write" : "clear", ssid, &cd_dma, cd[0], cd[1], cd[2], cd[3]);
 
-	ret = mutex_lock_interruptible(&mmu->lock);
-	if (ret)
-		return ret;
-
-	if (!mmu->on) {
-		ret = 0;
+	mutex_lock(&mmu->lock);
+	if (!mmu->on)
 		goto unlock;
-	}
 
 	ret = ivpu_mmu_cmdq_write_cfgi_all(vdev);
 	if (ret)
-- 
2.25.1

Re: [PATCH 1/2] accel/ivpu: Do not trigger extra VPU reset if the VPU is idle

[Jeffrey Hugo]

On 5/25/2023 4:38 AM, Stanislaw Gruszka wrote:
> From: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
> 
> Turning off the PLL and entering D0i3 will reset the VPU so
> an explicit IP reset is redundant.
> But if the VPU is active, it may interfere with PLL disabling
> and to avoid that, we have to issue an additional IP reset
> to silence the VPU before turning off the PLL.
> 
> Fixes: a8fed6d1e0b9 ("accel/ivpu: Fix power down sequence")
> Signed-off-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 2/2] accel/ivpu: Do not use mutex_lock_interruptible

[Jeffrey Hugo]

On 5/25/2023 4:38 AM, Stanislaw Gruszka wrote:
> If we get signal when waiting for the mmu->lock we do not invalidate
> current MMU configuration what might result on undefined behavior.

"that might result in"

> Additionally there is little or no benefit on break waiting for
> ipc->lock. In current code base, we keep this lock for short periods.

What about error cases?  Nothing where say the hardware can be 
unresponsive and a process from userspace is blocked?  Without 
interruptible(), ctrl+c will have no effect.

> Fixes: 263b2ba5fc93 ("accel/ivpu: Add Intel VPU MMU support")
> Reviewed-by: Krystian Pradzynski <krystian.pradzynski@linux.intel.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Re: [PATCH 2/2] accel/ivpu: Do not use mutex_lock_interruptible

[Stanislaw Gruszka]

Hi

On Fri, Jun 02, 2023 at 11:30:31AM -0600, Jeffrey Hugo wrote:
> On 5/25/2023 4:38 AM, Stanislaw Gruszka wrote:
> > If we get signal when waiting for the mmu->lock we do not invalidate
> > current MMU configuration what might result on undefined behavior.
> 
> "that might result in"
> 
> > Additionally there is little or no benefit on break waiting for
> > ipc->lock. In current code base, we keep this lock for short periods.
> 
> What about error cases?  Nothing where say the hardware can be unresponsive
> and a process from userspace is blocked?  Without interruptible(), ctrl+c
> will have no effect.

I believe we do not have any infinite loops while holding the mutexe's,
all loops will end with timeout on unresponsive hardware and sooner or
later SIGINT will be delivered. This time can take quite long on simulated
environment, but in such case we can just break the simulation.

Regards
Stanislaw

Re: [PATCH 2/2] accel/ivpu: Do not use mutex_lock_interruptible

[Jeffrey Hugo]

On 6/6/2023 7:44 AM, Stanislaw Gruszka wrote:
> Hi
> 
> On Fri, Jun 02, 2023 at 11:30:31AM -0600, Jeffrey Hugo wrote:
>> On 5/25/2023 4:38 AM, Stanislaw Gruszka wrote:
>>> If we get signal when waiting for the mmu->lock we do not invalidate
>>> current MMU configuration what might result on undefined behavior.
>>
>> "that might result in"
>>
>>> Additionally there is little or no benefit on break waiting for
>>> ipc->lock. In current code base, we keep this lock for short periods.
>>
>> What about error cases?  Nothing where say the hardware can be unresponsive
>> and a process from userspace is blocked?  Without interruptible(), ctrl+c
>> will have no effect.
> 
> I believe we do not have any infinite loops while holding the mutexe's,
> all loops will end with timeout on unresponsive hardware and sooner or
> later SIGINT will be delivered. This time can take quite long on simulated
> environment, but in such case we can just break the simulation.

Ok, then I'm not missing anything.  It does look like all the hardware 
interactions have short timeouts.  Feels odd to me to avoid 
interruptible() in user context, but I don't see anything that is wrong 
based on the code today.

With the commit text spelling fixes,
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 2/2] accel/ivpu: Do not use mutex_lock_interruptible

[Stanislaw Gruszka]

On Fri, Jun 02, 2023 at 11:30:31AM -0600, Jeffrey Hugo wrote:
> On 5/25/2023 4:38 AM, Stanislaw Gruszka wrote:
> > If we get signal when waiting for the mmu->lock we do not invalidate
> > current MMU configuration what might result on undefined behavior.
> 
> "that might result in"
Fixed this and applied 2 patches to drm-misc-fixes

Thanks
Stanislaw

Re: [PATCH 2/2] accel/ivpu: Do not use mutex_lock_interruptible

[Stanislaw Gruszka]

On Tue, Jun 06, 2023 at 08:50:52AM -0600, Jeffrey Hugo wrote:
> On 6/6/2023 7:44 AM, Stanislaw Gruszka wrote:
> > Hi
> > 
> > On Fri, Jun 02, 2023 at 11:30:31AM -0600, Jeffrey Hugo wrote:
> > > On 5/25/2023 4:38 AM, Stanislaw Gruszka wrote:
> > > > If we get signal when waiting for the mmu->lock we do not invalidate
> > > > current MMU configuration what might result on undefined behavior.
> > > 
> > > "that might result in"
> > > 
> > > > Additionally there is little or no benefit on break waiting for
> > > > ipc->lock. In current code base, we keep this lock for short periods.
> > > 
> > > What about error cases?  Nothing where say the hardware can be unresponsive
> > > and a process from userspace is blocked?  Without interruptible(), ctrl+c
> > > will have no effect.
> > 
> > I believe we do not have any infinite loops while holding the mutexe's,
> > all loops will end with timeout on unresponsive hardware and sooner or
> > later SIGINT will be delivered. This time can take quite long on simulated
> > environment, but in such case we can just break the simulation.
> 
> Ok, then I'm not missing anything.  It does look like all the hardware
> interactions have short timeouts.  Feels odd to me to avoid interruptible()
> in user context, but I don't see anything that is wrong based on the code
> today.

In this context it should not matter much, because we hold locks for
short periods But we have also wait_event_interruptible_timeout(),
which I want to get rid of as well, because we can free and reuse
memory that could still be used by FW, if we break that wait_event. 
And solving this differently will require much complication, which I
don't really want goto into. I will need to think about that ...

Anyways thanks for the insights, appreciated.

Regards
Stanislaw