On Thu, 13 Sep 2018 15:54:37 +0200,
Imre Deak wrote:
>
> On Wed, Sep 12, 2018 at 08:18:37PM +0200, Takashi Iwai wrote:
> > On Wed, 12 Sep 2018 15:18:47 +0200,
> > Imre Deak wrote:
> > >
> > > +Takashi
> > >
> > > On Wed, Sep 12, 2018 at 04:18:12PM +0300, Imre Deak wrote:
> > > > Hi Kumar, Takashi,
> > > >
> > > > On Tue, Jun 19, 2018 at 03:01:11PM -0700, Abhay Kumar wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > >
> > > > > CDCLK has to be at least twice the BLCK regardless of audio. Audio
> > > > > driver has to probe using this hook and increase the clock even in
> > > > > absence of any display.
> > > > >
> > > > > v2: Use atomic refcount for get_power, put_power so that we can
> > > > > call each once(Abhay).
> > > > > v3: Reset power well 2 to avoid any transaction on iDisp link
> > > > > during cdclk change(Abhay).
> > > > > v4: Remove Power well 2 reset workaround(Ville).
> > > > > v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Abhay Kumar <abhay.kumar@xxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_drv.h | 3 ++
> > > > > drivers/gpu/drm/i915/intel_audio.c | 67 +++++++++++++++++++++++++++++++++---
> > > > > drivers/gpu/drm/i915/intel_cdclk.c | 29 +++++-----------
> > > > > drivers/gpu/drm/i915/intel_display.c | 7 +++-
> > > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > > > > 5 files changed, 83 insertions(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 6104d7115054..a4a386a5db69 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -1702,6 +1702,7 @@ struct drm_i915_private {
> > > > > unsigned int hpll_freq;
> > > > > unsigned int fdi_pll_freq;
> > > > > unsigned int czclk_freq;
> > > > > + u32 get_put_refcount;
> > > > >
> > > > > struct {
> > > > > /*
> > > > > @@ -1719,6 +1720,8 @@ struct drm_i915_private {
> > > > > struct intel_cdclk_state actual;
> > > > > /* The current hardware cdclk state */
> > > > > struct intel_cdclk_state hw;
> > > > > +
> > > > > + int force_min_cdclk;
> > > > > } cdclk;
> > > > >
> > > > > /**
> > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > > index 3ea566f99450..ca8f04c7cbb3 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
> > > > >
> > > > > if (!connector->eld[0])
> > > > > return;
> > > > > -
> > > > > DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> > > > > connector->base.id,
> > > > > connector->name,
> > > > > @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
> > > > > }
> > > > > }
> > > > >
> > > > > +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> > > > > + bool enable)
> > > > > +{
> > > > > + struct drm_modeset_acquire_ctx ctx;
> > > > > + struct drm_atomic_state *state;
> > > > > + int ret;
> > > > > +
> > > > > + drm_modeset_acquire_init(&ctx, 0);
> > > > > + state = drm_atomic_state_alloc(&dev_priv->drm);
> > > > > + if (WARN_ON(!state))
> > > > > + return;
> > > > > +
> > > > > + state->acquire_ctx = &ctx;
> > > > > +
> > > > > +retry:
> > > > > + to_intel_atomic_state(state)->modeset = true;
> > > > > + to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> > > > > + enable ? 2 * 96000 : 0;
> > > > > +
> > > > > + /*
> > > > > + * Protects dev_priv->cdclk.force_min_cdclk
> > > > > + * Need to lock this here in case we have no active pipes
> > > > > + * and thus wouldn't lock it during the commit otherwise.
> > > > > + */
> > > > > + ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> > > > > + if (!ret)
> > > > > + ret = drm_atomic_commit(state);
> > > >
> > > > Ville mentioned a potential dead-lock problem here: a normal modeset
> > > > enabling an HDMI/DP output with an audio sink will call the
> > > > drm_audio_component_audio_ops::pin_eld_notify() callback with the
> > > > connection_mutex already held. This callback in turn could call
> > > > drm_audio_component_ops::get_power()/put_power() callbacks and
> > > > dead-lock at the above drm_modeset_lock() call.
> > > >
> > > > Looks like that
> > > >
> > > > sound/pci/hda/patch_hdmi.c / intel_pin_eld_notify()->
> > > > check_presence_and_report()->
> > > > hdmi_present_sense()
> > > >
> > > > would prevent this, but for instance
> > > >
> > > > sound/soc/codecs/hdac_hdmi.c / hdac_hdmi_eld_notify_cb()->
> > > > hdac_hdmi_present_sense()->
> > > > hdac_hdmi_jack_report()->
> > > > snd_soc_jack_report()->
> > > > snd_soc_dapm_sync()->
> > > > snd_soc_dapm_sync_unlocked()->
> > > > dapm_power_widgets()->
> > > > dapm_pre_sequence_async()
> > > >
> > > > could call pm_runtime_get_sync() and so I guess also the aops
> > > > get_power() hook.
> > > >
> > > > Could someone in your team check if the above can indeed happen?
> >
> > Through a quick glance, I'm afraid that it's indeed possible. We need
> > to off-load either the hdac_hdmi jack handling or this new
> > force_audio_cdclk stuff, I suppose.
>
> Ok. We need the force_audio_cdclk thing whenever the audio driver needs
> power and calls the aops::get_power() hook. IIUC this happens from various
> places in the HDA driver and get_power() should enable power
> synchronously, so not sure how we could off-load that..
>
> How about the same for the jack handling, perhaps only in the case it's
> done from hdac_hdmi_eld_notify_cb()?
Yes, this shouldn't be too hard. A totally untested patch is below.
> Also could you explain why patch_hdmi.c can do without get_power()?
> Could the same thing be done in hdac_hdmi.c?
In the legacy HD-audio driver, what driver does is essentially only
the update of the jack and the ELD ctl elements. The state is already
stored in GPU driver, hence we need no GPU power up, but just read
it and set to the sound ctl values.
OTOH, on ASoC hdac_hdmi driver, it kicks off the DAPM update depending
the jack state, and it's involved with the power up because it touches
the whole graph.
Takashi
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -96,8 +96,10 @@ struct hdac_hdmi_port {
hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
struct hdac_hdmi_eld eld;
const char *jack_pin;
+ bool is_connect;
struct snd_soc_dapm_context *dapm;
const char *output_pin;
+ struct work_struct dapm_work;
};
struct hdac_hdmi_pcm {
@@ -158,16 +160,12 @@ hdac_hdmi_get_pcm_from_cvt(struct hdac_hdmi_priv *hdmi,
return pcm;
}
-static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
+static void __hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
struct hdac_hdmi_port *port, bool is_connect)
{
struct hdac_device *hdev = port->pin->hdev;
- if (is_connect)
- snd_soc_dapm_enable_pin(port->dapm, port->jack_pin);
- else
- snd_soc_dapm_disable_pin(port->dapm, port->jack_pin);
-
+ port->is_connect = is_connect;
if (is_connect) {
/*
* Report Jack connect event when a device is connected
@@ -193,10 +191,32 @@ static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
if (pcm->jack_event > 0)
pcm->jack_event--;
}
+}
+static void hdac_hdmi_port_dapm_update(struct hdac_hdmi_port *port)
+{
+ if (port->is_connect)
+ snd_soc_dapm_enable_pin(port->dapm, port->jack_pin);
+ else
+ snd_soc_dapm_disable_pin(port->dapm, port->jack_pin);
snd_soc_dapm_sync(port->dapm);
}
+static void hdac_hdmi_jack_dapm_work(struct work_struct *work)
+{
+ struct hdac_hdmi_port *port;
+
+ port = container_of(work, struct hdac_hdmi_port, dapm_work);
+ hdac_hdmi_port_dapm_update(port);
+}
+
+static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
+ struct hdac_hdmi_port *port, bool is_connect)
+{
+ __hdac_hdmi_jack_report(pcm, port, is_connect);
+ hdac_hdmi_port_dapm_update(port);
+}
+
/* MST supported verbs */
/*
* Get the no devices that can be connected to a port on the Pin widget.
@@ -1261,16 +1281,20 @@ static void hdac_hdmi_present_sense(struct hdac_hdmi_pin *pin,
* report jack here. It will be done in usermode mux
* control select.
*/
- if (pcm)
- hdac_hdmi_jack_report(pcm, port, false);
+ if (pcm) {
+ __hdac_hdmi_jack_report(pcm, port, false);
+ schedule_work(&port->dapm_work);
+ }
mutex_unlock(&hdmi->pin_mutex);
return;
}
if (port->eld.monitor_present && port->eld.eld_valid) {
- if (pcm)
- hdac_hdmi_jack_report(pcm, port, true);
+ if (pcm) {
+ __hdac_hdmi_jack_report(pcm, port, true);
+ schedule_work(&port->dapm_work);
+ }
print_hex_dump_debug("ELD: ", DUMP_PREFIX_OFFSET, 16, 1,
port->eld.eld_buffer, port->eld.eld_size, false);
@@ -1299,6 +1323,7 @@ static int hdac_hdmi_add_ports(struct hdac_hdmi_priv *hdmi,
for (i = 0; i < max_ports; i++) {
ports[i].id = i;
ports[i].pin = pin;
+ INIT_WORK(&ports[i].dapm_work, hdac_hdmi_jack_dapm_work);
}
pin->ports = ports;
pin->num_ports = max_ports;
@@ -2062,6 +2087,10 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev)
struct hdac_hdmi_port *port, *port_next;
int i;
+ list_for_each_entry(pin, &hdmi->pin_list, head)
+ for (i = 0; i < pin->num_ports; i++)
+ cancel_work_sync(& pin->ports[i].dapm_work);
+
list_for_each_entry_safe(pcm, pcm_next, &hdmi->pcm_list, head) {
pcm->cvt = NULL;
if (list_empty(&pcm->port_list))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
