| Message ID | 20240829130954.2439316-2-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/v3d: Enable Big and Super Pages | expand |
Hi Maira, El jue, 29-08-2024 a las 10:05 -0300, Maíra Canal escribió: > We must first flush the MMU cache and then, flush the TLB, not the > other > way around. Currently, we can see a race condition between the MMU > cache > and the TLB when running multiple rendering processes at the same > time. > This is evidenced by MMU errors triggered by the IRQ. > > Fix the MMU flush order by flushing the MMU cache and then the TLB. the patch is making 2 changes, it is changing the ordering of the flushes but also the fact that now we wait for the first flush to commplete before starting the second while the previous version would start both flushes and then wait for both to complete. The commit log seems to suggest that the first change is the one that fixes the issue but I wonder if that is really what is happening. Also, have you tested keeping the original order of operations but with interleaved waits like we do here? Either way, I think we probably should emphasized more in the committ log the fact that we are now waiting for each flush to complete before starting the other flush. > > Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for > Broadcom V3D V3.x+") > Signed-off-by: Maíra Canal <mcanal@igalia.com> > --- > drivers/gpu/drm/v3d/v3d_mmu.c | 29 ++++++++++------------------- > 1 file changed, 10 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/v3d/v3d_mmu.c > b/drivers/gpu/drm/v3d/v3d_mmu.c > index 14f3af40d6f6..06bb44c7f35d 100644 > --- a/drivers/gpu/drm/v3d/v3d_mmu.c > +++ b/drivers/gpu/drm/v3d/v3d_mmu.c > @@ -32,32 +32,23 @@ static int v3d_mmu_flush_all(struct v3d_dev *v3d) > { > int ret; > > - /* Make sure that another flush isn't already running when > we > - * start this one. > - */ > - ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & > - V3D_MMU_CTL_TLB_CLEARING), 100); > - if (ret) > - dev_err(v3d->drm.dev, "TLB clear wait idle pre-wait > failed\n"); > - are we certain we can't have a flush in flux when a new one comes in? > - V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL) | > - V3D_MMU_CTL_TLB_CLEAR); > - > - V3D_WRITE(V3D_MMUC_CONTROL, > - V3D_MMUC_CONTROL_FLUSH | > + V3D_WRITE(V3D_MMUC_CONTROL, V3D_MMUC_CONTROL_FLUSH | > V3D_MMUC_CONTROL_ENABLE); > > - ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & > - V3D_MMU_CTL_TLB_CLEARING), 100); > + ret = wait_for(!(V3D_READ(V3D_MMUC_CONTROL) & > + V3D_MMUC_CONTROL_FLUSHING), 100); > if (ret) { > - dev_err(v3d->drm.dev, "TLB clear wait idle > failed\n"); > + dev_err(v3d->drm.dev, "MMUC flush wait idle > failed\n"); > return ret; > } > > - ret = wait_for(!(V3D_READ(V3D_MMUC_CONTROL) & > - V3D_MMUC_CONTROL_FLUSHING), 100); > + V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL) | > + V3D_MMU_CTL_TLB_CLEAR); > + > + ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & > + V3D_MMU_CTL_TLB_CLEARING), 100); > if (ret) > - dev_err(v3d->drm.dev, "MMUC flush wait idle > failed\n"); > + dev_err(v3d->drm.dev, "TLB clear wait idle > failed\n"); I'd maybe use "MMU TLB clear wait idle failed", so we can more easily identify this message comes from the MMU implementation. Iago > > return ret; > }
Hi Iago, On 9/4/24 05:24, Iago Toral wrote: > Hi Maira, > > El jue, 29-08-2024 a las 10:05 -0300, Maíra Canal escribió: >> We must first flush the MMU cache and then, flush the TLB, not the >> other >> way around. Currently, we can see a race condition between the MMU >> cache >> and the TLB when running multiple rendering processes at the same >> time. >> This is evidenced by MMU errors triggered by the IRQ. >> >> Fix the MMU flush order by flushing the MMU cache and then the TLB. > > the patch is making 2 changes, it is changing the ordering of the > flushes but also the fact that now we wait for the first flush to > commplete before starting the second while the previous version would > start both flushes and then wait for both to complete. The commit log > seems to suggest that the first change is the one that fixes the issue > but I wonder if that is really what is happening. > > Also, have you tested keeping the original order of operations but with > interleaved waits like we do here? Either way, I think we probably I just tested keeping the order of the operations but using interleaved waits and I end up having MMU errors that crash the GPU. Changing the order of the operations was a deliberated choice due the GPU design and and waiting for each of those to finish is needed to avoid race conditions. > should emphasized more in the committ log the fact that we are now > waiting for each flush to complete before starting the other flush. > But you are absolutely right. I need to mention it on the commit log. I'll add this information in the next version. > >> >> Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for >> Broadcom V3D V3.x+") >> Signed-off-by: Maíra Canal <mcanal@igalia.com> >> --- >> drivers/gpu/drm/v3d/v3d_mmu.c | 29 ++++++++++------------------- >> 1 file changed, 10 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/gpu/drm/v3d/v3d_mmu.c >> b/drivers/gpu/drm/v3d/v3d_mmu.c >> index 14f3af40d6f6..06bb44c7f35d 100644 >> --- a/drivers/gpu/drm/v3d/v3d_mmu.c >> +++ b/drivers/gpu/drm/v3d/v3d_mmu.c >> @@ -32,32 +32,23 @@ static int v3d_mmu_flush_all(struct v3d_dev *v3d) >> { >> int ret; >> >> - /* Make sure that another flush isn't already running when >> we >> - * start this one. >> - */ >> - ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & >> - V3D_MMU_CTL_TLB_CLEARING), 100); >> - if (ret) >> - dev_err(v3d->drm.dev, "TLB clear wait idle pre-wait >> failed\n"); >> - > > are we certain we can't have a flush in flux when a new one comes in? We can, but it doesn't really matter, as by the end of both requests, the TLB will be flushed. Best Regards, - Maíra > >> - V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL) | >> - V3D_MMU_CTL_TLB_CLEAR); >> - >> - V3D_WRITE(V3D_MMUC_CONTROL, >> - V3D_MMUC_CONTROL_FLUSH | >> + V3D_WRITE(V3D_MMUC_CONTROL, V3D_MMUC_CONTROL_FLUSH | >> V3D_MMUC_CONTROL_ENABLE); >> >> - ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & >> - V3D_MMU_CTL_TLB_CLEARING), 100); >> + ret = wait_for(!(V3D_READ(V3D_MMUC_CONTROL) & >> + V3D_MMUC_CONTROL_FLUSHING), 100); >> if (ret) { >> - dev_err(v3d->drm.dev, "TLB clear wait idle >> failed\n"); >> + dev_err(v3d->drm.dev, "MMUC flush wait idle >> failed\n"); >> return ret; >> } >> >> - ret = wait_for(!(V3D_READ(V3D_MMUC_CONTROL) & >> - V3D_MMUC_CONTROL_FLUSHING), 100); >> + V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL) | >> + V3D_MMU_CTL_TLB_CLEAR); >> + >> + ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & >> + V3D_MMU_CTL_TLB_CLEARING), 100); >> if (ret) >> - dev_err(v3d->drm.dev, "MMUC flush wait idle >> failed\n"); >> + dev_err(v3d->drm.dev, "TLB clear wait idle >> failed\n"); > > I'd maybe use "MMU TLB clear wait idle failed", so we can more easily > identify this message comes from the MMU implementation. > > Iago > >> >> return ret; >> } >
diff --git a/drivers/gpu/drm/v3d/v3d_mmu.c b/drivers/gpu/drm/v3d/v3d_mmu.c index 14f3af40d6f6..06bb44c7f35d 100644 --- a/drivers/gpu/drm/v3d/v3d_mmu.c +++ b/drivers/gpu/drm/v3d/v3d_mmu.c @@ -32,32 +32,23 @@ static int v3d_mmu_flush_all(struct v3d_dev *v3d) { int ret; - /* Make sure that another flush isn't already running when we - * start this one. - */ - ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & - V3D_MMU_CTL_TLB_CLEARING), 100); - if (ret) - dev_err(v3d->drm.dev, "TLB clear wait idle pre-wait failed\n"); - - V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL) | - V3D_MMU_CTL_TLB_CLEAR); - - V3D_WRITE(V3D_MMUC_CONTROL, - V3D_MMUC_CONTROL_FLUSH | + V3D_WRITE(V3D_MMUC_CONTROL, V3D_MMUC_CONTROL_FLUSH | V3D_MMUC_CONTROL_ENABLE); - ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & - V3D_MMU_CTL_TLB_CLEARING), 100); + ret = wait_for(!(V3D_READ(V3D_MMUC_CONTROL) & + V3D_MMUC_CONTROL_FLUSHING), 100); if (ret) { - dev_err(v3d->drm.dev, "TLB clear wait idle failed\n"); + dev_err(v3d->drm.dev, "MMUC flush wait idle failed\n"); return ret; } - ret = wait_for(!(V3D_READ(V3D_MMUC_CONTROL) & - V3D_MMUC_CONTROL_FLUSHING), 100); + V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL) | + V3D_MMU_CTL_TLB_CLEAR); + + ret = wait_for(!(V3D_READ(V3D_MMU_CTL) & + V3D_MMU_CTL_TLB_CLEARING), 100); if (ret) - dev_err(v3d->drm.dev, "MMUC flush wait idle failed\n"); + dev_err(v3d->drm.dev, "TLB clear wait idle failed\n"); return ret; }
We must first flush the MMU cache and then, flush the TLB, not the other way around. Currently, we can see a race condition between the MMU cache and the TLB when running multiple rendering processes at the same time. This is evidenced by MMU errors triggered by the IRQ. Fix the MMU flush order by flushing the MMU cache and then the TLB. Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+") Signed-off-by: Maíra Canal <mcanal@igalia.com> --- drivers/gpu/drm/v3d/v3d_mmu.c | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-)