[alsa-devel] [RFC PATCH 1/2] ALSA: hda - divide controller and codec dependency on i915 gfx power well

[Takashi Iwai]

At Fri, 24 Apr 2015 21:01:54 +0800,
mengdong.lin at intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin at intel.com>
> 
> This patch can improve power saving for Intel platforms on which only the
> display audio codec is in the shared i915 power well:
> - divide the controller and codec dependency on i915 power well by adding
>   flag "need_i915_power" for the chip (controller) and codec respectively.
> 
> - If the controller does not need the power well, the driver will release the
>   power after probe. And if only the codec needs the power, its runtime PM ops
>   will request/release the power.
> 
> Background:
> - For Haswell/Broadwell, which has a separate HD-A controller for display audio,
>   both the controller and the display codec are in the i915 power well.
> 
> - For Baytrail/Braswell, the display and analog audio share the same HDA
>   controller and link, and only the display codec is in the i915 power well.
> 
> - For Skylake, the display and analog audio share the same HDA controller but
>   use separate links. Only the display codec is in the i915 power well. And in
>   legacy mode we take the two links as one. So it can follow Baytrail/Braswell.
> 
> Open questions:
> - Move the hda_i915 to sound/hda at first? So both legacy and new drivers can
>   use it as Takashi suggested?
>   The PCI ID check in haswell_set_bclk will no longer be needed, since it's
>   only called when chip's "need_i915_power" flag is set and so only called for
>   Haswell & Broadwell.

I don't mind which comes first.

> - move flag "need_i915_power" from struct hda_codec to hdac_device, for the
>   same reason as above?

Yes, it makes sense, if the change shall happen sooner or later.

> - If the audio component moves from struct hda_intel to hdac_bus, how can the
>   controller access the component? Need to add "bus" to struct azx?

There are more changes.  See my patch series.  The hda_bus itself is
embedded into hda_bus.

Also, the hda_bus_ops was dropped.  It's just calling the
corresponding function instead.

> - implement codec resume/suspend ops to get/put i915 power if only the codec
>   needs the power?

I don't understand the question here.  Could you elaborate?


One concern is that you'd likely need to implement a refcount for
power switch.  It makes our life easier.

A few more things:


> Signed-off-by: Mengdong Lin <mengdong.lin at intel.com>
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 2a8aa9d..baa01a0 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -169,6 +169,8 @@ struct hdac_bus_ops {
>  	/* get a response from the last command */
>  	int (*get_response)(struct hdac_bus *bus, unsigned int addr,
>  			    unsigned int *res);
> +	/* request/release display power */
> +	int (*display_power)(struct hdac_bus *bus, bool enable);
>  };
>  
>  #define HDA_UNSOL_QUEUE_SIZE	64
> @@ -222,6 +224,11 @@ static inline void snd_hdac_codec_link_down(struct hdac_device *codec)
>  	clear_bit(codec->addr, &codec->bus->codec_powered);
>  }
>  
> +static inline int snd_hdac_display_power(struct hdac_device *codec, bool enable)
> +{
> +	return codec->bus->ops->display_power(codec->bus, enable);
> +}

Please give the function description in kerneldoc style.


> +
>  /*
>   * generic array helpers
>   */
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> old mode 100755
> new mode 100644
> index 18a021a..034488e
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -524,9 +524,21 @@ static int _hda_bus_get_response(struct hdac_bus *_bus, unsigned int addr,
>  	return bus->rirb_error ? -EIO : 0;
>  }
>  
> +static int _hda_bus_display_power(struct hdac_bus *_bus, bool enable)
> +{
> +	struct hda_bus *bus = container_of(_bus, struct hda_bus, core);
> +
> +	if (bus->ops.display_power)
> +		return bus->ops.display_power(bus, enable);

As mentioned, the bus_ops was dropped.  Rebase your patch with the
latest topic/hda branch of sound git tree.


> +	else
> +		return -EINVAL;
> +
> +}
> +
>  static const struct hdac_bus_ops bus_ops = {
>  	.command = _hda_bus_command,
>  	.get_response = _hda_bus_get_response,
> +	.display_power = _hda_bus_display_power,
>  };
>  
>  /**
> @@ -949,6 +961,10 @@ void snd_hda_codec_register(struct hda_codec *codec)
>  		return;
>  	if (device_is_registered(hda_codec_dev(codec))) {
>  		snd_hda_register_beep_device(codec);
> +
> +		if (codec->need_i915_power)
> +			snd_hdac_display_power(&codec->core, true);

The question is whether doing here is appropriate.
This assumes that the power well already enabled in the controller
side during probing?  The verb accesses (reading through the whole
topology, widget caps, etc) happened already before this point.  So
the power well has to be enabled before this.

>  		pm_runtime_enable(hda_codec_dev(codec));
>  		/* it was powered up in snd_hda_codec_new(), now all done */
>  		snd_hda_power_down(codec);
> @@ -3196,6 +3212,9 @@ static int hda_codec_runtime_suspend(struct device *dev)
>  	if (codec_has_clkstop(codec) && codec_has_epss(codec) &&
>  	    (state & AC_PWRST_CLK_STOP_OK))
>  		snd_hdac_codec_link_down(&codec->core);
> +
> +	if (codec->need_i915_power)
> +		snd_hdac_display_power(&codec->core, false);
>  	return 0;
>  }
>  
> @@ -3205,6 +3224,9 @@ static int hda_codec_runtime_resume(struct device *dev)
>  
>  	printk("amanda: hda_codec_runtime_resume, codec addr %d \n", codec->addr);
>  

I suppose you're working on a local modified tree?

Overall, the code should work well without CONFIG_SND_HDA_I915.
Please check each commit whether it builds and works with and without
it.


thanks,

Takashi