From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vinod Koul Subject: Re: HDMI hotplug on Skylake when power well is off Date: Thu, 16 Jul 2015 15:32:26 +0530 Message-ID: <20150716100226.GK5086@localhost> References: <55A62252.1050506@canonical.com> <20150716083908.GI5086@localhost> <55A77A8A.2080101@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by alsa0.perex.cz (Postfix) with ESMTP id ED9F12661B0 for ; Thu, 16 Jul 2015 12:00:40 +0200 (CEST) Content-Disposition: inline In-Reply-To: <55A77A8A.2080101@canonical.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: David Henningsson Cc: Takashi Iwai , libin.yang@intel.com, "alsa-devel@alsa-project.org" , "Lin, Mengdong" List-Id: alsa-devel@alsa-project.org On Thu, Jul 16, 2015 at 11:34:02AM +0200, David Henningsson wrote: > > > On 2015-07-16 10:39, Vinod Koul wrote: > >On Wed, Jul 15, 2015 at 12:59:02PM +0200, Takashi Iwai wrote: > >>On Wed, 15 Jul 2015 11:05:22 +0200, > >>David Henningsson wrote: > >>> > >>>Hi, > >>> > >>>I'm trying to debug an issue here where the HDMI hotplug events are not > >>>delivered to the audio side when the power well is off. This is on a > >>>Skylake machine (running in HDA mode). > >>> > >>>I'm not sure whether the problem is upstream or due to my own patches > >>>while testing, so I was wondering how this is supposed to be working, so > >>>I can troubleshoot further? > >>> > >>>Should there be an IRQ on the HDA controller even if the power well is > >>>off, and if not, how should the audio driver be notified that an HDMI > >>>hotplug event has happened? > >> > >>I thought this has been always a problem when the runtime PM is > >>enabled, no matter whether the power well state is. > >Shouldn't the hotplug action turn on the power well? Then notification for > >audio side should get propagated as power well is On > > While the video side can turn the power well on, maybe there are > other things that needs to be turned on from the audio driver? Hmmm that is intresting question, Takashi, Mengdong do you ahve ay idea on this one? > > >>IMO, a cleaner solution would be rather the notifier implementation in > >>software, e.g. extend the i915 component to pass the audio side ops > >>for notification. > >Yes that should be added but I would prefer we have hw do that as well > > So I took a quick stab at this and tried to write down a draft, but > I got stuck trying to figure out how to wake up the audio codecs > from the hdac_i915.c file. I'm not sure how to do this with the > recent reorg as I don't want to break the ASoC version of the driver > by including the wrong header files. > > See attached patch (which is a very rough draft, not even compile > tested), maybe you or Takashi could offer some insight w r t whether > I'm on the right track, and how to proceed? > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 8ae6f7f..0eaac41 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -45,6 +45,7 @@ > #include > #include /* for struct drm_dma_handle */ > #include > +#include > #include > #include > #include > @@ -1752,6 +1753,7 @@ struct drm_i915_private { > struct drm_property *force_audio_property; > > /* hda/i915 audio component */ > + struct i915_audio_component *audio_component; > bool audio_component_registered; > > uint32_t hw_context_size; > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > index ef34257..3799d88 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -424,6 +424,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder) > > if (dev_priv->display.audio_codec_enable) > dev_priv->display.audio_codec_enable(connector, intel_encoder, mode); > + > + if (acomp && acomp->cb_ops && acomp->cb_ops->hotplug_notify) > + acomp->cb_ops->hotplug_notify(acomp->hdac_bus, true); > } > > /** > @@ -437,9 +440,13 @@ void intel_audio_codec_disable(struct intel_encoder *encoder) > { > struct drm_device *dev = encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct i915_audio_component *acomp = dev_priv->audio_component; > > if (dev_priv->display.audio_codec_disable) > dev_priv->display.audio_codec_disable(encoder); > + > + if (acomp && acomp->cb_ops && acomp->cb_ops->hotplug_notify) > + acomp->cb_ops->hotplug_notify(acomp->hdac_bus, false); > } > > /** > @@ -529,12 +536,14 @@ static int i915_audio_component_bind(struct device *i915_dev, > struct device *hda_dev, void *data) > { > struct i915_audio_component *acomp = data; > + struct drm_i915_private *dev_priv = dev_to_i915(i915_dev); > > if (WARN_ON(acomp->ops || acomp->dev)) > return -EEXIST; > > acomp->ops = &i915_audio_component_ops; > acomp->dev = i915_dev; > + dev_priv->audio_component = acomp; > > return 0; > } > @@ -543,9 +552,11 @@ static void i915_audio_component_unbind(struct device *i915_dev, > struct device *hda_dev, void *data) > { > struct i915_audio_component *acomp = data; > + struct drm_i915_private *dev_priv = dev_to_i915(i915_dev); > > acomp->ops = NULL; > acomp->dev = NULL; > + dev_priv->audio_component = NULL; > } > > static const struct component_ops i915_audio_component_bind_ops = { > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h > index c9a8b64..3feab48 100644 > --- a/include/drm/i915_component.h > +++ b/include/drm/i915_component.h > @@ -24,8 +24,11 @@ > #ifndef _I915_COMPONENT_H_ > #define _I915_COMPONENT_H_ > > +struct hdac_bus; > + > struct i915_audio_component { > struct device *dev; > + struct hdac_bus *hdac_bus; > > const struct i915_audio_component_ops { > struct module *owner; > @@ -34,6 +37,11 @@ struct i915_audio_component { > void (*codec_wake_override)(struct device *, bool enable); > int (*get_cdclk_freq)(struct device *); > } *ops; > + > + const struct i915_audio_component_cb_ops { > + struct module *owner; > + void (*hotplug_notify)(struct hdac_bus *, bool enable); > + } *cb_ops; > }; > > #endif /* _I915_COMPONENT_H_ */ > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c > index 442500e..0ec49a6 100644 > --- a/sound/hda/hdac_i915.c > +++ b/sound/hda/hdac_i915.c > @@ -115,6 +115,8 @@ static void hdac_component_master_unbind(struct device *dev) > { > struct i915_audio_component *acomp = hdac_acomp; > > + acomp->cb_ops = NULL; > + acomp->hdac_bus = NULL; > module_put(acomp->ops->owner); > component_unbind_all(dev, acomp); > WARN_ON(acomp->ops || acomp->dev); > @@ -125,6 +127,31 @@ static const struct component_master_ops hdac_component_master_ops = { > .unbind = hdac_component_master_unbind, > }; > > +static const struct i915_ hdac_component_master_ops = { > + .bind = hdac_component_master_bind, > + .unbind = hdac_component_master_unbind, > +}; > + > +static const struct i915_audio_component_cb_ops i915_audio_component_cb_ops = { > + .owner = THIS_MODULE, > + .hotplug_notify = i915_audio_component_hotplug_notify, > +}; do we need these two, why not add .hotplug_notify in the i915_audio_component. During bind the hdac_i915 sets this up and the display can invoke this callback (assuming here the binding takes care of passing this value from audio to display component) > + > + > +static void i915_audio_component_hotplug_notify(struct hdac_bus *bus, bool enable) > +{ > + struct hda_codec *codec; > + > + codec_dbg("Received HDMI hotplug callback (enable = %d)", (int) enable); > + > + // TODO: Not sure about this part - with the new reorg, maybe I can't access > + // codec->jackpoll_work from this file? Here i feel that we should add notify in hdac_device. Driver can set that up and bus can invoke -- ~Vinod > + list_for_each_codec(codec, bus) > + schedule_delayed_work(&codec->jackpoll_work, > + codec->jackpoll_interval); > + > +} > + > static int hdac_component_master_match(struct device *dev, void *data) > { > /* i915 is the only supported component */ > @@ -160,6 +187,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus) > ret = -ENODEV; > goto out_master_del; > } > + acomp->cb_ops = &i915_audio_component_cb_ops; > + acomp->hdac_bus = bus; > + > dev_dbg(dev, "bound to i915 component master\n"); > > return 0; --