Re: [Intel-gfx] [PATCH v2 1/1] drm/i915/rpm: Enable runtime pm autosuspend by default

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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