| Message ID | 20211115102610.3141720-2-tilak.tangudu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/rpm: Enable runtime pm autosuspend by default | expand |
On Mon, 15 Nov 2021, Tilak Tangudu <tilak.tangudu@intel.com> wrote: The actual commit message with explanations why it will work now and didn't work before go here. Just the changelog will not be enough. BR, Jani. > v1: Enable runtime pm autosuspend by default for Gen12 > and later versions. > > v2: Enable runtime pm autosuspend by default for all > platforms(Syrjala Ville) > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com> > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index eaf7688f517d..f26ed1427fdc 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -600,6 +600,9 @@ void intel_runtime_pm_enable(struct intel_runtime_pm *rpm) > pm_runtime_use_autosuspend(kdev); > } > > + /* Enable by default */ > + pm_runtime_allow(kdev); > + > /* > * The core calls the driver load handler with an RPM reference held. > * We drop that here and will reacquire it during unloading in
On Mon, Nov 15, 2021 at 01:44:57PM +0200, Jani Nikula wrote: > On Mon, 15 Nov 2021, Tilak Tangudu <tilak.tangudu@intel.com> wrote: > > The actual commit message with explanations why it will work now and > didn't work before go here. The truth is that: 1. We don't have a good track of *all* the issues with the past attempts. 2. But apparently in every attempt we hit some other bug, like the latest one with GuC PM... 3. All the attempts we also tried to do multiple changes at the same time, including reducing the autosuspend delay. > Just the changelog will not be enough. But yes, you are absolutely right here. changelogs are not enough and we need some explanation in the commit itself. I'd suggest something like: """ Let's enable runtime pm autosuspend by default everywhere. So we can allow D3hot and bigger power savings on idle scenarios. But at this time let's not touch the autosuspend_delay time, what caused some regression on our previous attempt. Also, the latest identified issue on GuC PM has been fixed by 1a52faed3131 ("drm/i915/guc: Take GT PM ref when deregistering context") """ While writing that I remembered that we cannot do this just yet. We need to do a further work and block the d3cold on discrete. D3cold is not ready yet and enabling this autosuspend by default will blow up some discrete experimental usages of upstream i915 out there. Let's protect that first. Thanks, Rodrigo. > > BR, > Jani. > > > > v1: Enable runtime pm autosuspend by default for Gen12 > > and later versions. > > > > v2: Enable runtime pm autosuspend by default for all > > platforms(Syrjala Ville) > > > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com> > > --- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index eaf7688f517d..f26ed1427fdc 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -600,6 +600,9 @@ void intel_runtime_pm_enable(struct intel_runtime_pm *rpm) > > pm_runtime_use_autosuspend(kdev); > > } > > > > + /* Enable by default */ > > + pm_runtime_allow(kdev); > > + > > /* > > * The core calls the driver load handler with an RPM reference held. > > * We drop that here and will reacquire it during unloading in > > -- > Jani Nikula, Intel Open Source Graphics Center
On Mon, Nov 15, 2021 at 09:58:56AM -0500, Rodrigo Vivi wrote: > On Mon, Nov 15, 2021 at 01:44:57PM +0200, Jani Nikula wrote: > > On Mon, 15 Nov 2021, Tilak Tangudu <tilak.tangudu@intel.com> wrote: > > > > The actual commit message with explanations why it will work now and > > didn't work before go here. > > The truth is that: > > 1. We don't have a good track of *all* the issues with the past attempts. > 2. But apparently in every attempt we hit some other bug, like the latest > one with GuC PM... > 3. All the attempts we also tried to do multiple changes at the same time, > including reducing the autosuspend delay. > > > Just the changelog will not be enough. > > But yes, you are absolutely right here. changelogs are not enough and > we need some explanation in the commit itself. > > I'd suggest something like: > > """ > Let's enable runtime pm autosuspend by default everywhere. So we can > allow D3hot and bigger power savings on idle scenarios. > > But at this time let's not touch the autosuspend_delay time, > what caused some regression on our previous attempt. > > Also, the latest identified issue on GuC PM has been fixed by > 1a52faed3131 ("drm/i915/guc: Take GT PM ref when deregistering context") > > """ > > While writing that I remembered that we cannot do this just yet. > We need to do a further work and block the d3cold on discrete. > D3cold is not ready yet and enabling this autosuspend by default > will blow up some discrete experimental usages of upstream i915 > out there. Let's protect that first. we now have the d3cold in place. Please proceed with the spin on the patch with the commit message improvement. Thanks, Rodrigo. > > Thanks, > Rodrigo. > > > > > BR, > > Jani. > > > > > > > v1: Enable runtime pm autosuspend by default for Gen12 > > > and later versions. > > > > > > v2: Enable runtime pm autosuspend by default for all > > > platforms(Syrjala Ville) > > > > > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > index eaf7688f517d..f26ed1427fdc 100644 > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > @@ -600,6 +600,9 @@ void intel_runtime_pm_enable(struct intel_runtime_pm *rpm) > > > pm_runtime_use_autosuspend(kdev); > > > } > > > > > > + /* Enable by default */ > > > + pm_runtime_allow(kdev); > > > + > > > /* > > > * The core calls the driver load handler with an RPM reference held. > > > * We drop that here and will reacquire it during unloading in > > > > -- > > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index eaf7688f517d..f26ed1427fdc 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -600,6 +600,9 @@ void intel_runtime_pm_enable(struct intel_runtime_pm *rpm) pm_runtime_use_autosuspend(kdev); } + /* Enable by default */ + pm_runtime_allow(kdev); + /* * The core calls the driver load handler with an RPM reference held. * We drop that here and will reacquire it during unloading in
v1: Enable runtime pm autosuspend by default for Gen12 and later versions. v2: Enable runtime pm autosuspend by default for all platforms(Syrjala Ville) Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++ 1 file changed, 3 insertions(+)