| Message ID | 20220310234611.424743-4-robdclark@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/gpu: More system suspend fixes | expand |
On 3/11/2022 5:16 AM, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > The mutex wasn't really protecting anything before. Before the previous > patch we could still be racing with the scheduler's kthread, as that is > not necessarily frozen yet. Now that we've parked the sched threads, > the only race is with jobs retiring, and that is harmless, ie. > > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/msm/adreno/adreno_device.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > index 0440a98988fc..661dfa7681fb 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > @@ -607,15 +607,6 @@ static int adreno_runtime_resume(struct device *dev) > return gpu->funcs->pm_resume(gpu); > } > > -static int active_submits(struct msm_gpu *gpu) > -{ > - int active_submits; > - mutex_lock(&gpu->active_lock); > - active_submits = gpu->active_submits; > - mutex_unlock(&gpu->active_lock); I assumed that this lock here was to ensure proper barriers while reading active_submits. Is that not required? -Akhil. > - return active_submits; > -} > - > static int adreno_runtime_suspend(struct device *dev) > { > struct msm_gpu *gpu = dev_to_gpu(dev); > @@ -669,7 +660,7 @@ static int adreno_system_suspend(struct device *dev) > suspend_scheduler(gpu); > > remaining = wait_event_timeout(gpu->retire_event, > - active_submits(gpu) == 0, > + gpu->active_submits == 0, > msecs_to_jiffies(1000)); > if (remaining == 0) { > dev_err(dev, "Timeout waiting for GPU to suspend\n");
On Thu, Mar 17, 2022 at 1:45 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > > On 3/11/2022 5:16 AM, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > The mutex wasn't really protecting anything before. Before the previous > > patch we could still be racing with the scheduler's kthread, as that is > > not necessarily frozen yet. Now that we've parked the sched threads, > > the only race is with jobs retiring, and that is harmless, ie. > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > drivers/gpu/drm/msm/adreno/adreno_device.c | 11 +---------- > > 1 file changed, 1 insertion(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c > > index 0440a98988fc..661dfa7681fb 100644 > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c > > @@ -607,15 +607,6 @@ static int adreno_runtime_resume(struct device *dev) > > return gpu->funcs->pm_resume(gpu); > > } > > > > -static int active_submits(struct msm_gpu *gpu) > > -{ > > - int active_submits; > > - mutex_lock(&gpu->active_lock); > > - active_submits = gpu->active_submits; > > - mutex_unlock(&gpu->active_lock); > I assumed that this lock here was to ensure proper barriers while > reading active_submits. Is that not required? There is a spinlock in prepare_to_wait_event() ahead of checking the condition, which AFAIU is a sufficient barrier BR, -R > > -Akhil. > > - return active_submits; > > -} > > - > > static int adreno_runtime_suspend(struct device *dev) > > { > > struct msm_gpu *gpu = dev_to_gpu(dev); > > @@ -669,7 +660,7 @@ static int adreno_system_suspend(struct device *dev) > > suspend_scheduler(gpu); > > > > remaining = wait_event_timeout(gpu->retire_event, > > - active_submits(gpu) == 0, > > + gpu->active_submits == 0, > > msecs_to_jiffies(1000)); > > if (remaining == 0) { > > dev_err(dev, "Timeout waiting for GPU to suspend\n"); >
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index 0440a98988fc..661dfa7681fb 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -607,15 +607,6 @@ static int adreno_runtime_resume(struct device *dev) return gpu->funcs->pm_resume(gpu); } -static int active_submits(struct msm_gpu *gpu) -{ - int active_submits; - mutex_lock(&gpu->active_lock); - active_submits = gpu->active_submits; - mutex_unlock(&gpu->active_lock); - return active_submits; -} - static int adreno_runtime_suspend(struct device *dev) { struct msm_gpu *gpu = dev_to_gpu(dev); @@ -669,7 +660,7 @@ static int adreno_system_suspend(struct device *dev) suspend_scheduler(gpu); remaining = wait_event_timeout(gpu->retire_event, - active_submits(gpu) == 0, + gpu->active_submits == 0, msecs_to_jiffies(1000)); if (remaining == 0) { dev_err(dev, "Timeout waiting for GPU to suspend\n");