| Message ID | 20241003124107.39153-3-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: add PM suspend/resume support | expand |
Hi Stefan, On 10/3/24 09:41, Stefan Wahren wrote: > Add suspend/resume support for the VC4 V3D component in order > to handle suspend to idle properly. > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > drivers/gpu/drm/vc4/vc4_v3d.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c > index 2423826c89eb..8057b06c1f16 100644 > --- a/drivers/gpu/drm/vc4/vc4_v3d.c > +++ b/drivers/gpu/drm/vc4/vc4_v3d.c > @@ -368,7 +368,6 @@ void vc4_v3d_bin_bo_put(struct vc4_dev *vc4) > mutex_unlock(&vc4->bin_bo_lock); > } > > -#ifdef CONFIG_PM > static int vc4_v3d_runtime_suspend(struct device *dev) > { > struct vc4_v3d *v3d = dev_get_drvdata(dev); > @@ -397,7 +396,6 @@ static int vc4_v3d_runtime_resume(struct device *dev) > > return 0; > } > -#endif > > int vc4_v3d_debugfs_init(struct drm_minor *minor) > { > @@ -507,7 +505,8 @@ static void vc4_v3d_unbind(struct device *dev, struct device *master, > } > > static const struct dev_pm_ops vc4_v3d_pm_ops = { > - SET_RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, vc4_v3d_runtime_resume, NULL) > + RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, vc4_v3d_runtime_resume, NULL) > + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) I have a question: How can we guarantee that no jobs are running when the system is forced to suspend? Best Regards, - Maíra > }; > > static const struct component_ops vc4_v3d_ops = { > @@ -538,6 +537,6 @@ struct platform_driver vc4_v3d_driver = { > .driver = { > .name = "vc4_v3d", > .of_match_table = vc4_v3d_dt_match, > - .pm = &vc4_v3d_pm_ops, > + .pm = pm_ptr(&vc4_v3d_pm_ops), > }, > }; > -- > 2.34.1 >
Hi Maíra, Am 08.10.24 um 15:49 schrieb Maíra Canal: > Hi Stefan, > > On 10/3/24 09:41, Stefan Wahren wrote: >> Add suspend/resume support for the VC4 V3D component in order >> to handle suspend to idle properly. >> >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >> --- >> drivers/gpu/drm/vc4/vc4_v3d.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c >> b/drivers/gpu/drm/vc4/vc4_v3d.c >> index 2423826c89eb..8057b06c1f16 100644 >> --- a/drivers/gpu/drm/vc4/vc4_v3d.c >> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c >> @@ -368,7 +368,6 @@ void vc4_v3d_bin_bo_put(struct vc4_dev *vc4) >> mutex_unlock(&vc4->bin_bo_lock); >> } >> >> -#ifdef CONFIG_PM >> static int vc4_v3d_runtime_suspend(struct device *dev) >> { >> struct vc4_v3d *v3d = dev_get_drvdata(dev); >> @@ -397,7 +396,6 @@ static int vc4_v3d_runtime_resume(struct device >> *dev) >> >> return 0; >> } >> -#endif >> >> int vc4_v3d_debugfs_init(struct drm_minor *minor) >> { >> @@ -507,7 +505,8 @@ static void vc4_v3d_unbind(struct device *dev, >> struct device *master, >> } >> >> static const struct dev_pm_ops vc4_v3d_pm_ops = { >> - SET_RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, >> vc4_v3d_runtime_resume, NULL) >> + RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, vc4_v3d_runtime_resume, >> NULL) >> + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >> pm_runtime_force_resume) > > I have a question: How can we guarantee that no jobs are running when > the system is forced to suspend? Not sure what do you mean with job. userspace task or v3d job within the driver? Do you have something specific in mind. Why is there a difference between runtime pm and system pm? I must confess that i didn't test a system sleep while running a v3d application. Best regards Stefan > > Best Regards, > - Maíra > >> }; >> >> static const struct component_ops vc4_v3d_ops = { >> @@ -538,6 +537,6 @@ struct platform_driver vc4_v3d_driver = { >> .driver = { >> .name = "vc4_v3d", >> .of_match_table = vc4_v3d_dt_match, >> - .pm = &vc4_v3d_pm_ops, >> + .pm = pm_ptr(&vc4_v3d_pm_ops), >> }, >> }; >> -- >> 2.34.1 >>
Hi Stefan, On 10/8/24 13:05, Stefan Wahren wrote: > Hi Maíra, > > Am 08.10.24 um 15:49 schrieb Maíra Canal: >> Hi Stefan, >> >> On 10/3/24 09:41, Stefan Wahren wrote: >>> Add suspend/resume support for the VC4 V3D component in order >>> to handle suspend to idle properly. >>> >>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >>> --- >>> drivers/gpu/drm/vc4/vc4_v3d.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c >>> b/drivers/gpu/drm/vc4/vc4_v3d.c >>> index 2423826c89eb..8057b06c1f16 100644 >>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c >>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c >>> @@ -368,7 +368,6 @@ void vc4_v3d_bin_bo_put(struct vc4_dev *vc4) >>> mutex_unlock(&vc4->bin_bo_lock); >>> } >>> >>> -#ifdef CONFIG_PM >>> static int vc4_v3d_runtime_suspend(struct device *dev) >>> { >>> struct vc4_v3d *v3d = dev_get_drvdata(dev); >>> @@ -397,7 +396,6 @@ static int vc4_v3d_runtime_resume(struct device >>> *dev) >>> >>> return 0; >>> } >>> -#endif >>> >>> int vc4_v3d_debugfs_init(struct drm_minor *minor) >>> { >>> @@ -507,7 +505,8 @@ static void vc4_v3d_unbind(struct device *dev, >>> struct device *master, >>> } >>> >>> static const struct dev_pm_ops vc4_v3d_pm_ops = { >>> - SET_RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, >>> vc4_v3d_runtime_resume, NULL) >>> + RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, vc4_v3d_runtime_resume, >>> NULL) >>> + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>> pm_runtime_force_resume) >> >> I have a question: How can we guarantee that no jobs are running when >> the system is forced to suspend? > Not sure what do you mean with job. userspace task or v3d job within the > driver? I mean a V3D job. See, when we submit a CL to the GPU, we only know when it's done through a IRQ. I'm thinking in the case where: 1. We submitted a CL to the GPU 2. We suspend but the job wasn't ended yet What happens to this job? Is the GPU going to be in a unstable state when we resume? > > Do you have something specific in mind. > > Why is there a difference between runtime pm and system pm? > As far as I can see, for system PM, we need at least to suspend V3D in a stable state, without any jobs running and with IRQs disabled. Best Regards, - Maíra > I must confess that i didn't test a system sleep while running a v3d > application. > > Best regards > Stefan >> >> Best Regards, >> - Maíra >> >>> }; >>> >>> static const struct component_ops vc4_v3d_ops = { >>> @@ -538,6 +537,6 @@ struct platform_driver vc4_v3d_driver = { >>> .driver = { >>> .name = "vc4_v3d", >>> .of_match_table = vc4_v3d_dt_match, >>> - .pm = &vc4_v3d_pm_ops, >>> + .pm = pm_ptr(&vc4_v3d_pm_ops), >>> }, >>> }; >>> -- >>> 2.34.1 >>> >
Hi Maíra, Am 09.10.24 um 14:16 schrieb Maíra Canal: > Hi Stefan, > > On 10/8/24 13:05, Stefan Wahren wrote: >> Hi Maíra, >> >> Am 08.10.24 um 15:49 schrieb Maíra Canal: >>> Hi Stefan, >>> >>> On 10/3/24 09:41, Stefan Wahren wrote: >>>> Add suspend/resume support for the VC4 V3D component in order >>>> to handle suspend to idle properly. >>>> >>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >>>> --- >>>> drivers/gpu/drm/vc4/vc4_v3d.c | 7 +++---- >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c >>>> b/drivers/gpu/drm/vc4/vc4_v3d.c >>>> index 2423826c89eb..8057b06c1f16 100644 >>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c >>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c >>>> @@ -368,7 +368,6 @@ void vc4_v3d_bin_bo_put(struct vc4_dev *vc4) >>>> mutex_unlock(&vc4->bin_bo_lock); >>>> } >>>> >>>> -#ifdef CONFIG_PM >>>> static int vc4_v3d_runtime_suspend(struct device *dev) >>>> { >>>> struct vc4_v3d *v3d = dev_get_drvdata(dev); >>>> @@ -397,7 +396,6 @@ static int vc4_v3d_runtime_resume(struct device >>>> *dev) >>>> >>>> return 0; >>>> } >>>> -#endif >>>> >>>> int vc4_v3d_debugfs_init(struct drm_minor *minor) >>>> { >>>> @@ -507,7 +505,8 @@ static void vc4_v3d_unbind(struct device *dev, >>>> struct device *master, >>>> } >>>> >>>> static const struct dev_pm_ops vc4_v3d_pm_ops = { >>>> - SET_RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, >>>> vc4_v3d_runtime_resume, NULL) >>>> + RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, vc4_v3d_runtime_resume, >>>> NULL) >>>> + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>>> pm_runtime_force_resume) >>> >>> I have a question: How can we guarantee that no jobs are running when >>> the system is forced to suspend? >> Not sure what do you mean with job. userspace task or v3d job within the >> driver? > > I mean a V3D job. See, when we submit a CL to the GPU, we only know when > it's done through a IRQ. I'm thinking in the case where: > > 1. We submitted a CL to the GPU > 2. We suspend but the job wasn't ended yet okay, now i understand. I was under the naive impression that everything is handled in the ARM cores. Sorry, i don't really have a deeper understanding of the V3D stuff. > > What happens to this job? Is the GPU going to be in a unstable state > when we resume? I don't know what happens to the GPU, but during suspend most of the IRQs would be disabled so the ARM core(s) won't serve the interrupt handler. At least i need some guidance here, how to prevent sending new jobs to the GPU. I assume such a logic must be already part of the driver removal. Best regards > >> >> Do you have something specific in mind. >> >> Why is there a difference between runtime pm and system pm? >> > As far as I can see, for system PM, we need at least to suspend V3D in > a stable state, without any jobs running and with IRQs disabled. > > Best Regards, > - Maíra > >> I must confess that i didn't test a system sleep while running a v3d >> application. >> >> Best regards >> Stefan >>> >>> Best Regards, >>> - Maíra >>> >>>> }; >>>> >>>> static const struct component_ops vc4_v3d_ops = { >>>> @@ -538,6 +537,6 @@ struct platform_driver vc4_v3d_driver = { >>>> .driver = { >>>> .name = "vc4_v3d", >>>> .of_match_table = vc4_v3d_dt_match, >>>> - .pm = &vc4_v3d_pm_ops, >>>> + .pm = pm_ptr(&vc4_v3d_pm_ops), >>>> }, >>>> }; >>>> -- >>>> 2.34.1 >>>> >>
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c index 2423826c89eb..8057b06c1f16 100644 --- a/drivers/gpu/drm/vc4/vc4_v3d.c +++ b/drivers/gpu/drm/vc4/vc4_v3d.c @@ -368,7 +368,6 @@ void vc4_v3d_bin_bo_put(struct vc4_dev *vc4) mutex_unlock(&vc4->bin_bo_lock); } -#ifdef CONFIG_PM static int vc4_v3d_runtime_suspend(struct device *dev) { struct vc4_v3d *v3d = dev_get_drvdata(dev); @@ -397,7 +396,6 @@ static int vc4_v3d_runtime_resume(struct device *dev) return 0; } -#endif int vc4_v3d_debugfs_init(struct drm_minor *minor) { @@ -507,7 +505,8 @@ static void vc4_v3d_unbind(struct device *dev, struct device *master, } static const struct dev_pm_ops vc4_v3d_pm_ops = { - SET_RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, vc4_v3d_runtime_resume, NULL) + RUNTIME_PM_OPS(vc4_v3d_runtime_suspend, vc4_v3d_runtime_resume, NULL) + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) }; static const struct component_ops vc4_v3d_ops = { @@ -538,6 +537,6 @@ struct platform_driver vc4_v3d_driver = { .driver = { .name = "vc4_v3d", .of_match_table = vc4_v3d_dt_match, - .pm = &vc4_v3d_pm_ops, + .pm = pm_ptr(&vc4_v3d_pm_ops), }, };
Add suspend/resume support for the VC4 V3D component in order to handle suspend to idle properly. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/gpu/drm/vc4/vc4_v3d.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) -- 2.34.1