[Intel-gfx] [PATCH] drm/i915: Ensure proper HDA suspend/resume ordering with a device link

[Imre Deak]

On Mon, Oct 22, 2018 at 07:22:30PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 18, 2018 at 05:25:58PM +0300, Imre Deak wrote:
> > In order to ensure that our system suspend and resume callbacks are
> > called in the correct order wrt. those of the HDA driver add a device
> > link to the HDA driver during audio component binding time. With i915 as
> > the supplier and HDA as the consumer the PM framework will guarantee
> > the HDA->i915 suspend (and shutdown) and i915->HDA resume order.
> > 
> > Atm, the lack of this ordering is not a problem, since all the i915
> > suspend/resume steps that need to be ordered wrt. the HDA driver's
> > suspend/resume steps are separated out to the i915
> > suspend_late/resume_early hooks. That will change in a follow-up
> > patchset where we'll need this ordering guarantee for steps that are in
> > the i915 suspend/resume hooks (and which can't be moved to
> > suspend_late/resume_early for other reasons). So this patch is a
> > preparation for that follow-up patchset.
> > 
> > The change also allows us to move towards removing the i915
> > suspend_late/resume_early hooks alltogether.
> > 
> > Since we only need to ensure the ordering during suspend/resume and not
> > during driver probing create the link with DL_FLAG_STATELESS. Since the
> > probe time ordering has to be optional we use the component framework
> > for that.
> 
> And we manage runtime pm explicitly too so don't need/want
> DL_FLAG_PM_RUNTIME.

Yes, will add that too to the commit message.

> > 
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Takashi Iwai <tiwai at suse.de>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 769f3f586661..391b88701610 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -921,6 +921,8 @@ static int i915_audio_component_bind(struct device *i915_kdev,
> >  	dev_priv->audio_component = acomp;
> >  	drm_modeset_unlock_all(&dev_priv->drm);
> >  
> > +	WARN_ON(!device_link_add(hda_kdev, i915_kdev, DL_FLAG_STATELESS));
> > +
> 
> I guess it's just the kzalloc() that could theoretically fail. Probably
> not a practical concern.

Yep, missed that. For clarity we should still fail the component bind,
which will make the HDA driver's bind time out and continue without HDMI
functionality. For simpler error handling I'll move the
device_link_add() call before initing acomp (moving the call to 
device_link_remove() accordingly).

> 
> Didn't read through the entire device_link code, but this seems to
> agree with the docs.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Thanks will respin with the above changes.

> 
> >  	return 0;
> >  }
> >  
> > @@ -930,6 +932,8 @@ static void i915_audio_component_unbind(struct device *i915_kdev,
> >  	struct i915_audio_component *acomp = data;
> >  	struct drm_i915_private *dev_priv = kdev_to_i915(i915_kdev);
> >  
> > +	device_link_remove(hda_kdev, i915_kdev);
> > +
> >  	drm_modeset_lock_all(&dev_priv->drm);
> >  	acomp->base.ops = NULL;
> >  	acomp->base.dev = NULL;
> > -- 
> > 2.13.2
> 
> -- 
> Ville Syrjälä
> Intel