[PATCH 0/5] sanitize hda/i915 interface using the component fw

[PATCH 0/5] sanitize hda/i915 interface using the component fw

[Imre Deak]

The current hda/i915 interface to enable/disable power wells and query
the CD clock rate is based on looking up the relevant i915 module
symbols from the hda driver. By using the component framework we can get
rid of some global state tracking in the i915 driver and pave the way to
fully decouple the two drivers: once support is added to enable/disable
the HDMI functionality dynamically in the hda driver, it can bind/unbind
itself from the i915 component master, without the need to keep a
reference on the i915 module.

This also gets rid of the problem that currently the i915 driver exposes
the interface only on HSW and BDW, while it's also needed at least on
VLV/CHV.

Imre Deak (5):
  drm/i915: add dev_to_i915_priv helper
  drm/i915: add component support
  ALSA: hda: pass chip to all i915 interface functions
  ALSA: hda: add component support
  drm/i915: remove unused power_well/get_cdclk_freq api

 drivers/gpu/drm/i915/i915_dma.c         |  80 ++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.c         |  15 ++--
 drivers/gpu/drm/i915/intel_drv.h        |   8 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c |  56 --------------
 include/drm/i915_component.h            |  38 ++++++++++
 include/drm/i915_powerwell.h            |  37 ----------
 sound/pci/hda/hda_i915.c                | 126 +++++++++++++++++++++-----------
 sound/pci/hda/hda_i915.h                |  12 +--
 sound/pci/hda/hda_intel.c               |  16 ++--
 sound/pci/hda/hda_priv.h                |   7 ++
 10 files changed, 238 insertions(+), 157 deletions(-)
 create mode 100644 include/drm/i915_component.h
 delete mode 100644 include/drm/i915_powerwell.h

-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/i915: add dev_to_i915_priv helper

[Imre Deak]

This will be needed by later patches, so factor it out.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  | 15 ++++++---------
 drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 71be3c9..32c2e33 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev)
 
 static int i915_pm_suspend(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
+	struct drm_device *drm_dev = dev_priv->dev;
 
-	if (!drm_dev || !drm_dev->dev_private) {
+	if (!drm_dev || !dev_priv) {
 		dev_err(dev, "DRM not initialized, aborting suspend.\n");
 		return -ENODEV;
 	}
@@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev)
 
 static int i915_pm_suspend_late(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
 
 	/*
 	 * We have a suspedn ordering issue with the snd-hda driver also
@@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev)
 
 static int i915_pm_resume_early(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
 
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
@@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev)
 
 static int i915_pm_resume(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
 
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61a88fa..3de7a55 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
 	return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1;
 }
 
+static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+
+	return drm_dev->dev_private;
+}
+
 /* intel_fifo_underrun.c */
 bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
 					   enum pipe pipe, bool enable);
-- 
1.8.4

[PATCH 2/5] drm/i915: add component support

[Imre Deak]

Register a component master to be used to interface with the
snd_hda_intel driver. This is meant to replace the same interface that
is currently based on module symbol lookup.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 80 +++++++++++++++++++++++++++++++++++++++++
 include/drm/i915_component.h    | 38 ++++++++++++++++++++
 2 files changed, 118 insertions(+)
 create mode 100644 include/drm/i915_component.h

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 887d88f..6e0f3be 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -35,6 +35,8 @@
 #include <drm/drm_legacy.h>
 #include "intel_drv.h"
 #include <drm/i915_drm.h>
+#include <linux/component.h>
+#include <drm/i915_component.h>
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include <linux/pci.h>
@@ -600,6 +602,80 @@ static void intel_device_info_runtime_init(struct drm_device *dev)
 	}
 }
 
+static void i915_component_get_power(struct device *dev)
+{
+	intel_display_power_get(dev_to_i915_priv(dev), POWER_DOMAIN_AUDIO);
+}
+
+static void i915_component_put_power(struct device *dev)
+{
+	intel_display_power_put(dev_to_i915_priv(dev), POWER_DOMAIN_AUDIO);
+}
+
+/* Get CDCLK in kHz  */
+static int i915_component_get_cdclk_freq(struct device *dev)
+{
+	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
+	int ret;
+
+	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
+		return -ENODEV;
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+	ret = intel_ddi_get_cdclk_freq(dev_priv);
+	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
+
+	return ret;
+}
+
+static struct i915_component_ops component_ops = {
+	.owner		= THIS_MODULE,
+	.get_power	= i915_component_get_power,
+	.put_power	= i915_component_put_power,
+	.get_cdclk	= i915_component_get_cdclk_freq,
+};
+
+static int i915_component_master_bind(struct device *dev)
+{
+	return component_bind_all(dev, &component_ops);
+}
+
+static void i915_component_master_unbind(struct device *dev)
+{
+	return component_unbind_all(dev, &component_ops);
+}
+
+static const struct component_master_ops i915_component_master_ops = {
+	.bind	= i915_component_master_bind,
+	.unbind	= i915_component_master_unbind,
+};
+
+static int i915_component_master_match(struct device *dev, void *data)
+{
+	/* snd_hda_intel is the only supported component */
+	return !strcmp(dev->driver->name, "snd_hda_intel");
+}
+
+static void i915_component_master_init(struct drm_i915_private *dev_priv)
+{
+	struct device *dev = dev_priv->dev->dev;
+	struct component_match *match = NULL;
+	int ret;
+
+	component_match_add(dev, &match, i915_component_master_match, NULL);
+	ret = component_master_add_with_match(dev, &i915_component_master_ops,
+					      match);
+	if (ret < 0)
+		DRM_ERROR("failed to add component master (%d)\n", ret);
+	/* continue with reduced functionality */
+}
+
+static void i915_component_master_cleanup(struct drm_i915_private *dev_priv)
+{
+	/* the following is ok to call even if component_master_add failed */
+	component_master_del(dev_priv->dev->dev, &i915_component_master_ops);
+}
+
 /**
  * i915_driver_load - setup chip and create an initial config
  * @dev: DRM device
@@ -830,6 +906,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_runtime_pm_enable(dev_priv);
 
+	i915_component_master_init(dev_priv);
+
 	return 0;
 
 out_power_well:
@@ -870,6 +948,8 @@ int i915_driver_unload(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	i915_component_master_cleanup(dev_priv);
+
 	ret = i915_gem_suspend(dev);
 	if (ret) {
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
new file mode 100644
index 0000000..44542c2
--- /dev/null
+++ b/include/drm/i915_component.h
@@ -0,0 +1,38 @@
+/**************************************************************************
+ *
+ * Copyright 2014 Intel Inc.
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ **************************************************************************/
+
+#ifndef _I915_COMPONENT_H_
+#define _I915_COMPONENT_H_
+
+struct i915_component_ops {
+	struct module *owner;
+	void (*get_power)(struct device *);
+	void (*put_power)(struct device *);
+	int (*get_cdclk)(struct device *);
+};
+
+#endif				/* _I915_COMPONENT_H_ */
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] ALSA: hda: pass chip to all i915 interface functions

[Imre Deak]

chip is already passed to most of the i915 interface functions, unify
things by passing it also to the rest. This will be needed by an
upcoming patch adding component support.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 sound/pci/hda/hda_i915.c  |  6 +++---
 sound/pci/hda/hda_i915.h  | 12 ++++++------
 sound/pci/hda/hda_intel.c | 16 ++++++++--------
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
index d4d0375..4e4b733 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -35,7 +35,7 @@ static int (*get_power)(void);
 static int (*put_power)(void);
 static int (*get_cdclk)(void);
 
-int hda_display_power(bool enable)
+int hda_display_power(struct azx *chip, bool enable)
 {
 	if (!get_power || !put_power)
 		return -ENODEV;
@@ -85,7 +85,7 @@ void haswell_set_bclk(struct azx *chip)
 }
 
 
-int hda_i915_init(void)
+int hda_i915_init(struct azx *chip)
 {
 	int err = 0;
 
@@ -111,7 +111,7 @@ int hda_i915_init(void)
 	return err;
 }
 
-int hda_i915_exit(void)
+int hda_i915_exit(struct azx *chip)
 {
 	if (get_power) {
 		symbol_put(i915_request_power_well);
diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h
index e6072c6..4d77d73 100644
--- a/sound/pci/hda/hda_i915.h
+++ b/sound/pci/hda/hda_i915.h
@@ -17,18 +17,18 @@
 #define __SOUND_HDA_I915_H
 
 #ifdef CONFIG_SND_HDA_I915
-int hda_display_power(bool enable);
+int hda_display_power(struct azx *chip, bool enable);
 void haswell_set_bclk(struct azx *chip);
-int hda_i915_init(void);
-int hda_i915_exit(void);
+int hda_i915_init(struct azx *chip);
+int hda_i915_exit(struct azx *chip);
 #else
-static inline int hda_display_power(bool enable) { return 0; }
+static inline int hda_display_power(struct azx *chip, bool enable) { return 0; }
 static inline void haswell_set_bclk(struct azx *chip) { return; }
-static inline int hda_i915_init(void)
+static inline int hda_i915_init(struct azx *chip);
 {
 	return -ENODEV;
 }
-static inline int hda_i915_exit(void)
+static inline int hda_i915_exit(struct azx *chip)
 {
 	return 0;
 }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 5ac0d39..f3b5dcd 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -825,7 +825,7 @@ static int azx_suspend(struct device *dev)
 	pci_save_state(pci);
 	pci_set_power_state(pci, PCI_D3hot);
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		hda_display_power(false);
+		hda_display_power(chip, false);
 	return 0;
 }
 
@@ -845,7 +845,7 @@ static int azx_resume(struct device *dev)
 		return 0;
 
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		hda_display_power(true);
+		hda_display_power(chip, true);
 		haswell_set_bclk(chip);
 	}
 	pci_set_power_state(pci, PCI_D0);
@@ -898,7 +898,7 @@ static int azx_runtime_suspend(struct device *dev)
 	azx_enter_link_reset(chip);
 	azx_clear_irq_pending(chip);
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		hda_display_power(false);
+		hda_display_power(chip, false);
 
 	return 0;
 }
@@ -924,7 +924,7 @@ static int azx_runtime_resume(struct device *dev)
 		return 0;
 
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		hda_display_power(true);
+		hda_display_power(chip, true);
 		haswell_set_bclk(chip);
 	}
 
@@ -1150,8 +1150,8 @@ static int azx_free(struct azx *chip)
 	release_firmware(chip->fw);
 #endif
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		hda_display_power(false);
-		hda_i915_exit();
+		hda_display_power(chip, false);
+		hda_i915_exit(chip);
 	}
 	kfree(hda);
 
@@ -1910,13 +1910,13 @@ static int azx_probe_continue(struct azx *chip)
 	/* Request power well for Haswell HDA controller and codec */
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
 #ifdef CONFIG_SND_HDA_I915
-		err = hda_i915_init();
+		err = hda_i915_init(chip);
 		if (err < 0) {
 			dev_err(chip->card->dev,
 				"Error request power-well from i915\n");
 			goto out_free;
 		}
-		err = hda_display_power(true);
+		err = hda_display_power(chip, true);
 		if (err < 0) {
 			dev_err(chip->card->dev,
 				"Cannot turn on display power on i915\n");
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] ALSA: hda: add component support

[Imre Deak]

Register a component to be used to interface with the i915 driver. This
is meant to replace the current interface which is based on module
symbol lookups.

Note that currently we keep the existing behavior and pin the i915
module while the hda driver is loaded. Using the component interface
allows us to remove this dependency once support for dynamically
enabling / disabling the HDMI functionality from the bind/unbind hooks
is added.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 sound/pci/hda/hda_i915.c | 120 ++++++++++++++++++++++++++++++++---------------
 sound/pci/hda/hda_priv.h |   7 +++
 2 files changed, 89 insertions(+), 38 deletions(-)

diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
index 4e4b733..01f5a5d 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -18,8 +18,10 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/component.h>
+#include <drm/i915_component.h>
 #include <sound/core.h>
-#include <drm/i915_powerwell.h>
 #include "hda_priv.h"
 #include "hda_i915.h"
 
@@ -31,32 +33,33 @@
 #define AZX_REG_EM4			0x100c
 #define AZX_REG_EM5			0x1010
 
-static int (*get_power)(void);
-static int (*put_power)(void);
-static int (*get_cdclk)(void);
-
 int hda_display_power(struct azx *chip, bool enable)
 {
-	if (!get_power || !put_power)
+	struct hda_component *hdac = &chip->component;
+
+	if (!hdac->i915_ops)
 		return -ENODEV;
 
 	pr_debug("HDA display power %s \n",
 			enable ? "Enable" : "Disable");
 	if (enable)
-		return get_power();
+		hdac->i915_ops->get_power(hdac->i915_dev);
 	else
-		return put_power();
+		hdac->i915_ops->put_power(hdac->i915_dev);
+
+	return 0;
 }
 
 void haswell_set_bclk(struct azx *chip)
 {
 	int cdclk_freq;
 	unsigned int bclk_m, bclk_n;
+	struct hda_component *hdac = &chip->component;
 
-	if (!get_cdclk)
+	if (!hdac->i915_ops)
 		return;
 
-	cdclk_freq = get_cdclk();
+	cdclk_freq = hdac->i915_ops->get_cdclk(hdac->i915_dev);
 	switch (cdclk_freq) {
 	case 337500:
 		bclk_m = 16;
@@ -84,47 +87,87 @@ void haswell_set_bclk(struct azx *chip)
 	azx_writew(chip, EM5, bclk_n);
 }
 
+static int hda_component_bind(struct device *hda_dev,
+			      struct device *i915_dev, void *data)
+{
+	struct snd_card *card = dev_get_drvdata(hda_dev);
+	struct azx *chip = card->private_data;
+	struct i915_component_ops *i915_ops = data;
+	struct hda_component *hdac = &chip->component;
+
+	if (WARN_ON(!(i915_ops->get_power && i915_ops->put_power &&
+		      i915_ops->get_cdclk)))
+		return -EINVAL;
+
+	/*
+	 * Atm, we don't support dynamic unbinding initiated by the component
+	 * master, so pin its containing module until we unbind.
+	 */
+	if (!try_module_get(i915_ops->owner))
+		return -ENODEV;
+
+	hdac->i915_dev = i915_dev;
+	hdac->i915_ops = i915_ops;
+
+	return 0;
+}
+
+static void hda_component_unbind(struct device *hda_dev,
+				 struct device *i915_dev, void *data)
+{
+	struct snd_card *card = dev_get_drvdata(hda_dev);
+	struct azx *chip = card->private_data;
+	struct hda_component *hdac = &chip->component;
+
+	module_put(hdac->i915_ops->owner);
+
+	hdac->i915_dev = NULL;
+	hdac->i915_ops = NULL;
+}
+
+static struct component_ops hda_component_ops = {
+	.bind = hda_component_bind,
+	.unbind = hda_component_unbind,
+};
 
 int hda_i915_init(struct azx *chip)
 {
-	int err = 0;
+	struct device *dev = &chip->pci->dev;
+	int ret;
 
-	get_power = symbol_request(i915_request_power_well);
-	if (!get_power) {
-		pr_warn("hda-i915: get_power symbol get fail\n");
-		return -ENODEV;
-	}
-
-	put_power = symbol_request(i915_release_power_well);
-	if (!put_power) {
-		symbol_put(i915_request_power_well);
-		get_power = NULL;
-		return -ENODEV;
-	}
-
-	get_cdclk = symbol_request(i915_get_cdclk_freq);
-	if (!get_cdclk)	/* may have abnormal BCLK and audio playback rate */
-		pr_warn("hda-i915: get_cdclk symbol get fail\n");
-
+	ret = component_add(dev, &hda_component_ops);
+	if (ret < 0)
+		goto out_err;
+
+	/*
+	 * Atm, we don't support deferring the component binding, so make sure
+	 * i915 is loaded and that the binding successfully completes.
+	 */
+	ret = request_module("i915");
+	if (ret < 0)
+		goto out_err;
+
+	if (!chip->component.i915_ops) {
+		ret = -ENODEV;
+		goto out_component_del;
+	}
+
-	pr_debug("HDA driver get symbol successfully from i915 module\n");
+	pr_debug("hda-i915: bound to component master\n");
+
+	return 0;
+out_component_del:
+	component_del(dev, &hda_component_ops);
+out_err:
+	pr_warn("hda-i915: failed to bind to component master (%d)\n", ret);
 
-	return err;
+	return ret;
 }
 
 int hda_i915_exit(struct azx *chip)
 {
-	if (get_power) {
-		symbol_put(i915_request_power_well);
-		get_power = NULL;
-	}
-	if (put_power) {
-		symbol_put(i915_release_power_well);
-		put_power = NULL;
-	}
-	if (get_cdclk) {
-		symbol_put(i915_get_cdclk_freq);
-		get_cdclk = NULL;
-	}
+	struct device *dev = &chip->pci->dev;
+
+	component_del(dev, &hda_component_ops);
 
 	return 0;
 }
diff --git a/sound/pci/hda/hda_priv.h b/sound/pci/hda/hda_priv.h
index aa484fd..336d401 100644
--- a/sound/pci/hda/hda_priv.h
+++ b/sound/pci/hda/hda_priv.h
@@ -18,6 +18,7 @@
 #include <linux/clocksource.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
+#include <drm/i915_component.h>
 
 /*
  * registers
@@ -291,6 +292,12 @@ struct azx {
 	struct pci_dev *pci;
 	int dev_index;
 
+	/* i915 component interface */
+	struct hda_component {
+		struct device *i915_dev;
+		const struct i915_component_ops *i915_ops;
+	} component;
+
 	/* chip type specific */
 	int driver_type;
 	unsigned int driver_caps;
-- 
1.8.4

[PATCH 5/5] drm/i915: remove unused power_well/get_cdclk_freq api

[Imre Deak]

After switching to using the component interface this API isn't needed
any more.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 56 ---------------------------------
 include/drm/i915_powerwell.h            | 37 ----------------------
 2 files changed, 93 deletions(-)
 delete mode 100644 include/drm/i915_powerwell.h

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 8a2bd18..0c9ab32 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -31,7 +31,6 @@
 
 #include "i915_drv.h"
 #include "intel_drv.h"
-#include <drm/i915_powerwell.h>
 
 /**
  * DOC: runtime pm
@@ -50,8 +49,6 @@
  * present for a given platform.
  */
 
-static struct i915_power_domains *hsw_pwr;
-
 #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
 	for (i = 0;							\
 	     i < (power_domains)->power_well_count &&			\
@@ -1098,10 +1095,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
 	 */
 	if (IS_HASWELL(dev_priv->dev)) {
 		set_power_wells(power_domains, hsw_power_wells);
-		hsw_pwr = power_domains;
 	} else if (IS_BROADWELL(dev_priv->dev)) {
 		set_power_wells(power_domains, bdw_power_wells);
-		hsw_pwr = power_domains;
 	} else if (IS_CHERRYVIEW(dev_priv->dev)) {
 		set_power_wells(power_domains, chv_power_wells);
 	} else if (IS_VALLEYVIEW(dev_priv->dev)) {
@@ -1145,8 +1140,6 @@ void intel_power_domains_fini(struct drm_i915_private *dev_priv)
 	 * the power well is not enabled, so just enable it in case
 	 * we're going to unload/reload. */
 	intel_display_set_init_power(dev_priv, true);
-
-	hsw_pwr = NULL;
 }
 
 static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
@@ -1355,52 +1348,3 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	pm_runtime_put_autosuspend(device);
 }
 
-/* Display audio driver power well request */
-int i915_request_power_well(void)
-{
-	struct drm_i915_private *dev_priv;
-
-	if (!hsw_pwr)
-		return -ENODEV;
-
-	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
-				power_domains);
-	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(i915_request_power_well);
-
-/* Display audio driver power well release */
-int i915_release_power_well(void)
-{
-	struct drm_i915_private *dev_priv;
-
-	if (!hsw_pwr)
-		return -ENODEV;
-
-	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
-				power_domains);
-	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(i915_release_power_well);
-
-/*
- * Private interface for the audio driver to get CDCLK in kHz.
- *
- * Caller must request power well using i915_request_power_well() prior to
- * making the call.
- */
-int i915_get_cdclk_freq(void)
-{
-	struct drm_i915_private *dev_priv;
-
-	if (!hsw_pwr)
-		return -ENODEV;
-
-	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
-				power_domains);
-
-	return intel_ddi_get_cdclk_freq(dev_priv);
-}
-EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h
deleted file mode 100644
index baa6f11..0000000
--- a/include/drm/i915_powerwell.h
+++ /dev/null
@@ -1,37 +0,0 @@
-/**************************************************************************
- *
- * Copyright 2013 Intel Inc.
- * All Rights Reserved.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the
- * "Software"), to deal in the Software without restriction, including
- * without limitation the rights to use, copy, modify, merge, publish,
- * distribute, sub license, and/or sell copies of the Software, and to
- * permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- *
- * The above copyright notice and this permission notice (including the
- * next paragraph) shall be included in all copies or substantial portions
- * of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
- * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
- * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
- * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
- * USE OR OTHER DEALINGS IN THE SOFTWARE.
- *
- *
- **************************************************************************/
-
-#ifndef _I915_POWERWELL_H_
-#define _I915_POWERWELL_H_
-
-/* For use by hda_i915 driver */
-extern int i915_request_power_well(void);
-extern int i915_release_power_well(void);
-extern int i915_get_cdclk_freq(void);
-
-#endif				/* _I915_POWERWELL_H_ */
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: add dev_to_i915_priv helper

[Jani Nikula]

On Mon, 08 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> This will be needed by later patches, so factor it out.
>
> No functional change.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  | 15 ++++++---------
>  drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
>  2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 71be3c9..32c2e33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev)
>  
>  static int i915_pm_suspend(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
> +	struct drm_device *drm_dev = dev_priv->dev;

Seems like a funny back and forth pointer dance, but let's see what the
next patches bring us... ;)

>  
> -	if (!drm_dev || !drm_dev->dev_private) {
> +	if (!drm_dev || !dev_priv) {
>  		dev_err(dev, "DRM not initialized, aborting suspend.\n");
>  		return -ENODEV;
>  	}
> @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev)
>  
>  static int i915_pm_suspend_late(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
>  
>  	/*
>  	 * We have a suspedn ordering issue with the snd-hda driver also
> @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev)
>  
>  static int i915_pm_resume_early(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
>  
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
> @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev)
>  
>  static int i915_pm_resume(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
>  
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 61a88fa..3de7a55 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
>  	return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1;
>  }
>  
> +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev)

Bikeshed, I think dev_to_i915() is sufficient and in line with
to_i915().

BR,
Jani.

> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +
> +	return drm_dev->dev_private;
> +}
> +
>  /* intel_fifo_underrun.c */
>  bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
>  					   enum pipe pipe, bool enable);
> -- 
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: add component support

[Jani Nikula]

On Mon, 08 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> Register a component master to be used to interface with the
> snd_hda_intel driver. This is meant to replace the same interface that
> is currently based on module symbol lookup.

I think I'd add a whole new intel_component.c file for this.

>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 80 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/i915_component.h    | 38 ++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 include/drm/i915_component.h
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 887d88f..6e0f3be 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -35,6 +35,8 @@
>  #include <drm/drm_legacy.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
> +#include <linux/component.h>
> +#include <drm/i915_component.h>
>  #include "i915_drv.h"
>  #include "i915_trace.h"
>  #include <linux/pci.h>
> @@ -600,6 +602,80 @@ static void intel_device_info_runtime_init(struct drm_device *dev)
>  	}
>  }
>  
> +static void i915_component_get_power(struct device *dev)
> +{
> +	intel_display_power_get(dev_to_i915_priv(dev), POWER_DOMAIN_AUDIO);
> +}
> +
> +static void i915_component_put_power(struct device *dev)
> +{
> +	intel_display_power_put(dev_to_i915_priv(dev), POWER_DOMAIN_AUDIO);
> +}
> +
> +/* Get CDCLK in kHz  */
> +static int i915_component_get_cdclk_freq(struct device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
> +		return -ENODEV;
> +
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> +	ret = intel_ddi_get_cdclk_freq(dev_priv);
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> +
> +	return ret;
> +}
> +
> +static struct i915_component_ops component_ops = {
> +	.owner		= THIS_MODULE,
> +	.get_power	= i915_component_get_power,
> +	.put_power	= i915_component_put_power,
> +	.get_cdclk	= i915_component_get_cdclk_freq,

Okay, I'm thinking one step ahead, but I'm not familiar with the
component driver stuff yet. Could we use the same component model for
communicating with, say, the pmic driver in the future? Should we use
the same component ops, or add another one? If the same, then they're
all in the same "namespace", and I think we should use something more
specific than just get_power and put_power.

(The master/slave relationship may be the other way round with the pmic
driver I guess. But nonetheless, we should think about reusing this.)

> +};
> +
> +static int i915_component_master_bind(struct device *dev)
> +{
> +	return component_bind_all(dev, &component_ops);
> +}
> +
> +static void i915_component_master_unbind(struct device *dev)
> +{
> +	return component_unbind_all(dev, &component_ops);
> +}
> +
> +static const struct component_master_ops i915_component_master_ops = {
> +	.bind	= i915_component_master_bind,
> +	.unbind	= i915_component_master_unbind,
> +};
> +
> +static int i915_component_master_match(struct device *dev, void *data)
> +{
> +	/* snd_hda_intel is the only supported component */
> +	return !strcmp(dev->driver->name, "snd_hda_intel");
> +}
> +
> +static void i915_component_master_init(struct drm_i915_private *dev_priv)
> +{
> +	struct device *dev = dev_priv->dev->dev;
> +	struct component_match *match = NULL;
> +	int ret;
> +
> +	component_match_add(dev, &match, i915_component_master_match, NULL);
> +	ret = component_master_add_with_match(dev, &i915_component_master_ops,
> +					      match);
> +	if (ret < 0)
> +		DRM_ERROR("failed to add component master (%d)\n", ret);
> +	/* continue with reduced functionality */
> +}
> +
> +static void i915_component_master_cleanup(struct drm_i915_private *dev_priv)
> +{
> +	/* the following is ok to call even if component_master_add failed */
> +	component_master_del(dev_priv->dev->dev, &i915_component_master_ops);
> +}
> +
>  /**
>   * i915_driver_load - setup chip and create an initial config
>   * @dev: DRM device
> @@ -830,6 +906,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_runtime_pm_enable(dev_priv);
>  
> +	i915_component_master_init(dev_priv);
> +
>  	return 0;
>  
>  out_power_well:
> @@ -870,6 +948,8 @@ int i915_driver_unload(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	i915_component_master_cleanup(dev_priv);
> +
>  	ret = i915_gem_suspend(dev);
>  	if (ret) {
>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> new file mode 100644
> index 0000000..44542c2
> --- /dev/null
> +++ b/include/drm/i915_component.h
> @@ -0,0 +1,38 @@
> +/**************************************************************************
> + *
> + * Copyright 2014 Intel Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + **************************************************************************/
> +
> +#ifndef _I915_COMPONENT_H_
> +#define _I915_COMPONENT_H_
> +
> +struct i915_component_ops {
> +	struct module *owner;
> +	void (*get_power)(struct device *);
> +	void (*put_power)(struct device *);
> +	int (*get_cdclk)(struct device *);
> +};
> +
> +#endif				/* _I915_COMPONENT_H_ */
> -- 
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: remove unused power_well/get_cdclk_freq api

[Jani Nikula]

On Mon, 08 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> After switching to using the component interface this API isn't needed
> any more.

\o/

>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 56 ---------------------------------
>  include/drm/i915_powerwell.h            | 37 ----------------------
>  2 files changed, 93 deletions(-)
>  delete mode 100644 include/drm/i915_powerwell.h
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 8a2bd18..0c9ab32 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -31,7 +31,6 @@
>  
>  #include "i915_drv.h"
>  #include "intel_drv.h"
> -#include <drm/i915_powerwell.h>
>  
>  /**
>   * DOC: runtime pm
> @@ -50,8 +49,6 @@
>   * present for a given platform.
>   */
>  
> -static struct i915_power_domains *hsw_pwr;
> -
>  #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
>  	for (i = 0;							\
>  	     i < (power_domains)->power_well_count &&			\
> @@ -1098,10 +1095,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  	 */
>  	if (IS_HASWELL(dev_priv->dev)) {
>  		set_power_wells(power_domains, hsw_power_wells);
> -		hsw_pwr = power_domains;
>  	} else if (IS_BROADWELL(dev_priv->dev)) {
>  		set_power_wells(power_domains, bdw_power_wells);
> -		hsw_pwr = power_domains;
>  	} else if (IS_CHERRYVIEW(dev_priv->dev)) {
>  		set_power_wells(power_domains, chv_power_wells);
>  	} else if (IS_VALLEYVIEW(dev_priv->dev)) {
> @@ -1145,8 +1140,6 @@ void intel_power_domains_fini(struct drm_i915_private *dev_priv)
>  	 * the power well is not enabled, so just enable it in case
>  	 * we're going to unload/reload. */
>  	intel_display_set_init_power(dev_priv, true);
> -
> -	hsw_pwr = NULL;
>  }
>  
>  static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
> @@ -1355,52 +1348,3 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>  	pm_runtime_put_autosuspend(device);
>  }
>  
> -/* Display audio driver power well request */
> -int i915_request_power_well(void)
> -{
> -	struct drm_i915_private *dev_priv;
> -
> -	if (!hsw_pwr)
> -		return -ENODEV;
> -
> -	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
> -				power_domains);
> -	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(i915_request_power_well);
> -
> -/* Display audio driver power well release */
> -int i915_release_power_well(void)
> -{
> -	struct drm_i915_private *dev_priv;
> -
> -	if (!hsw_pwr)
> -		return -ENODEV;
> -
> -	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
> -				power_domains);
> -	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(i915_release_power_well);
> -
> -/*
> - * Private interface for the audio driver to get CDCLK in kHz.
> - *
> - * Caller must request power well using i915_request_power_well() prior to
> - * making the call.
> - */
> -int i915_get_cdclk_freq(void)
> -{
> -	struct drm_i915_private *dev_priv;
> -
> -	if (!hsw_pwr)
> -		return -ENODEV;
> -
> -	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
> -				power_domains);
> -
> -	return intel_ddi_get_cdclk_freq(dev_priv);
> -}
> -EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
> diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h
> deleted file mode 100644
> index baa6f11..0000000
> --- a/include/drm/i915_powerwell.h
> +++ /dev/null
> @@ -1,37 +0,0 @@
> -/**************************************************************************
> - *
> - * Copyright 2013 Intel Inc.
> - * All Rights Reserved.
> - *
> - * Permission is hereby granted, free of charge, to any person obtaining a
> - * copy of this software and associated documentation files (the
> - * "Software"), to deal in the Software without restriction, including
> - * without limitation the rights to use, copy, modify, merge, publish,
> - * distribute, sub license, and/or sell copies of the Software, and to
> - * permit persons to whom the Software is furnished to do so, subject to
> - * the following conditions:
> - *
> - * The above copyright notice and this permission notice (including the
> - * next paragraph) shall be included in all copies or substantial portions
> - * of the Software.
> - *
> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> - * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> - * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> - * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> - * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> - * USE OR OTHER DEALINGS IN THE SOFTWARE.
> - *
> - *
> - **************************************************************************/
> -
> -#ifndef _I915_POWERWELL_H_
> -#define _I915_POWERWELL_H_
> -
> -/* For use by hda_i915 driver */
> -extern int i915_request_power_well(void);
> -extern int i915_release_power_well(void);
> -extern int i915_get_cdclk_freq(void);
> -
> -#endif				/* _I915_POWERWELL_H_ */
> -- 
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: add dev_to_i915_priv helper

[Imre Deak]

On Mon, 2014-12-08 at 20:36 +0200, Jani Nikula wrote:
> On Mon, 08 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> > This will be needed by later patches, so factor it out.
> >
> > No functional change.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c  | 15 ++++++---------
> >  drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
> >  2 files changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 71be3c9..32c2e33 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev)
> >  
> >  static int i915_pm_suspend(struct device *dev)
> >  {
> > -	struct pci_dev *pdev = to_pci_dev(dev);
> > -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
> > +	struct drm_device *drm_dev = dev_priv->dev;
> 
> Seems like a funny back and forth pointer dance, but let's see what the
> next patches bring us... ;)

Hm, right. This hunk doesn't simplify anything and doesn't even make
sense with the below !drm_dev check. Btw if and why drm_dev can be NULL
here would be worth investigating more. I'll remove this change for now.

> 
> >  
> > -	if (!drm_dev || !drm_dev->dev_private) {
> > +	if (!drm_dev || !dev_priv) {
> >  		dev_err(dev, "DRM not initialized, aborting suspend.\n");
> >  		return -ENODEV;
> >  	}
> > @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev)
> >  
> >  static int i915_pm_suspend_late(struct device *dev)
> >  {
> > -	struct pci_dev *pdev = to_pci_dev(dev);
> > -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
> >  
> >  	/*
> >  	 * We have a suspedn ordering issue with the snd-hda driver also
> > @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev)
> >  
> >  static int i915_pm_resume_early(struct device *dev)
> >  {
> > -	struct pci_dev *pdev = to_pci_dev(dev);
> > -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
> >  
> >  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  		return 0;
> > @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev)
> >  
> >  static int i915_pm_resume(struct device *dev)
> >  {
> > -	struct pci_dev *pdev = to_pci_dev(dev);
> > -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
> >  
> >  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  		return 0;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 61a88fa..3de7a55 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
> >  	return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1;
> >  }
> >  
> > +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev)
> 
> Bikeshed, I think dev_to_i915() is sufficient and in line with
> to_i915().

Ok.

> 
> BR,
> Jani.
> 
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +
> > +	return drm_dev->dev_private;
> > +}
> > +
> >  /* intel_fifo_underrun.c */
> >  bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
> >  					   enum pipe pipe, bool enable);
> > -- 
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Daniel Vetter]

On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> The current hda/i915 interface to enable/disable power wells and query
> the CD clock rate is based on looking up the relevant i915 module
> symbols from the hda driver. By using the component framework we can get
> rid of some global state tracking in the i915 driver and pave the way to
> fully decouple the two drivers: once support is added to enable/disable
> the HDMI functionality dynamically in the hda driver, it can bind/unbind
> itself from the i915 component master, without the need to keep a
> reference on the i915 module.
> 
> This also gets rid of the problem that currently the i915 driver exposes
> the interface only on HSW and BDW, while it's also needed at least on
> VLV/CHV.

Awesome that you're tackling this, really happy to see these hacks go.
Unfortunately I think it's upside down: hda should be the component master
and i915 should only register a component.

Longer story: The main reason for the component helpers is to be able to
magically delay registering the main/master driver until all the
components are loaded. Otherwise -EDEFER doesn't work and also the
suspend/resume ordering this should result in. Master here means whatever
userspace eventually sees as a device node or similar, component is
anything really that this userspace interfaces needs to function
correctly.

I think what we need here is then:
- Put the current azx_probe into the master_bind callback. The new
  azx_probe will do nothing else than register the master component and
  the component match list. To do so it checks the chip flag and if it
  needs to cooperate with i915 it registers one component match for that.
  The master_bind (old probe) function calls component_bind_all with the
  hda_intel pointer as void *data parameter.

- i915 registers a component for the i915 gfx device. It uses the
  component device to get at i915 sturctures and fills the dev+ops into
  the hda_intel pointer it gets as void *data.

Stuff we then should do on top:
- Add deferred probing to azx_probe: Only when all components are there
  should it actually register. This will take care of all the module load
  order mess. It should also take care of system suspend/resume ordering
  and we should be able to delete all the early_resume/late_suspend
  trickery.

Imo we should have things ready up to this point to make sure this
refactoring actually works.

Then there's some cool stuff we could do on top:
- Register a i915-hda platform devices as a child of the gfx pci device.
  Besides shuffling around a bit with the interfaces/argument casting and
  the component match function this doesn't really have a functional
  impact. But it makes the relationship more clear since hda doesn't
  really need the entire pci device, but only the small part that does
  audio.

- Replace the hand-rolled power-well interface with runtime pm on that
  device node.

- If system suspend/resume doesn't work automatically with deferred
  probing (tbh I'm not sure) add pm_ops to the component master. Then add
  some functions as default implementations for pm_ops for components
  which simply refcount all component pm_ops calls and call the master
  pm_ops suspend on the first suspend call and resume on the last resume
  call. That really should take care of suspend/resume ordering for good.

Cheers, Daniel

> 
> Imre Deak (5):
>   drm/i915: add dev_to_i915_priv helper
>   drm/i915: add component support
>   ALSA: hda: pass chip to all i915 interface functions
>   ALSA: hda: add component support
>   drm/i915: remove unused power_well/get_cdclk_freq api
> 
>  drivers/gpu/drm/i915/i915_dma.c         |  80 ++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.c         |  15 ++--
>  drivers/gpu/drm/i915/intel_drv.h        |   8 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  56 --------------
>  include/drm/i915_component.h            |  38 ++++++++++
>  include/drm/i915_powerwell.h            |  37 ----------
>  sound/pci/hda/hda_i915.c                | 126 +++++++++++++++++++++-----------
>  sound/pci/hda/hda_i915.h                |  12 +--
>  sound/pci/hda/hda_intel.c               |  16 ++--
>  sound/pci/hda/hda_priv.h                |   7 ++
>  10 files changed, 238 insertions(+), 157 deletions(-)
>  create mode 100644 include/drm/i915_component.h
>  delete mode 100644 include/drm/i915_powerwell.h
> 
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: add component support

[Imre Deak]

On Mon, 2014-12-08 at 20:44 +0200, Jani Nikula wrote:
> On Mon, 08 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> > Register a component master to be used to interface with the
> > snd_hda_intel driver. This is meant to replace the same interface that
> > is currently based on module symbol lookup.
> 
> I think I'd add a whole new intel_component.c file for this.

Ok.

> 
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 80 +++++++++++++++++++++++++++++++++++++++++
> >  include/drm/i915_component.h    | 38 ++++++++++++++++++++
> >  2 files changed, 118 insertions(+)
> >  create mode 100644 include/drm/i915_component.h
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 887d88f..6e0f3be 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -35,6 +35,8 @@
> >  #include <drm/drm_legacy.h>
> >  #include "intel_drv.h"
> >  #include <drm/i915_drm.h>
> > +#include <linux/component.h>
> > +#include <drm/i915_component.h>
> >  #include "i915_drv.h"
> >  #include "i915_trace.h"
> >  #include <linux/pci.h>
> > @@ -600,6 +602,80 @@ static void intel_device_info_runtime_init(struct drm_device *dev)
> >  	}
> >  }
> >  
> > +static void i915_component_get_power(struct device *dev)
> > +{
> > +	intel_display_power_get(dev_to_i915_priv(dev), POWER_DOMAIN_AUDIO);
> > +}
> > +
> > +static void i915_component_put_power(struct device *dev)
> > +{
> > +	intel_display_power_put(dev_to_i915_priv(dev), POWER_DOMAIN_AUDIO);
> > +}
> > +
> > +/* Get CDCLK in kHz  */
> > +static int i915_component_get_cdclk_freq(struct device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
> > +	int ret;
> > +
> > +	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
> > +		return -ENODEV;
> > +
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> > +	ret = intel_ddi_get_cdclk_freq(dev_priv);
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> > +
> > +	return ret;
> > +}
> > +
> > +static struct i915_component_ops component_ops = {
> > +	.owner		= THIS_MODULE,
> > +	.get_power	= i915_component_get_power,
> > +	.put_power	= i915_component_put_power,
> > +	.get_cdclk	= i915_component_get_cdclk_freq,
> 
> Okay, I'm thinking one step ahead, but I'm not familiar with the
> component driver stuff yet. Could we use the same component model for
> communicating with, say, the pmic driver in the future?

Afaics, yes. And that would give even better justification for using the
component fw..

> Should we use the same component ops, or add another one?

It's possible to register the same master device multiple times with
distinct ops, so I think that's a better solution in terms of interface
isolation.

> If the same, then they're all in the same "namespace", and I think we
> should use something more specific than just get_power and put_power.

I could do this in any case now, for example renaming them to
display_power_get/put and display_get_cdclk_rate.

> (The master/slave relationship may be the other way round with the pmic
> driver I guess.

Yes, afaics that works too.

> But nonetheless, we should think about reusing this.)
> 
> > +};
> > +
> > +static int i915_component_master_bind(struct device *dev)
> > +{
> > +	return component_bind_all(dev, &component_ops);
> > +}
> > +
> > +static void i915_component_master_unbind(struct device *dev)
> > +{
> > +	return component_unbind_all(dev, &component_ops);
> > +}
> > +
> > +static const struct component_master_ops i915_component_master_ops = {
> > +	.bind	= i915_component_master_bind,
> > +	.unbind	= i915_component_master_unbind,
> > +};
> > +
> > +static int i915_component_master_match(struct device *dev, void *data)
> > +{
> > +	/* snd_hda_intel is the only supported component */
> > +	return !strcmp(dev->driver->name, "snd_hda_intel");
> > +}
> > +
> > +static void i915_component_master_init(struct drm_i915_private *dev_priv)
> > +{
> > +	struct device *dev = dev_priv->dev->dev;
> > +	struct component_match *match = NULL;
> > +	int ret;
> > +
> > +	component_match_add(dev, &match, i915_component_master_match, NULL);
> > +	ret = component_master_add_with_match(dev, &i915_component_master_ops,
> > +					      match);
> > +	if (ret < 0)
> > +		DRM_ERROR("failed to add component master (%d)\n", ret);
> > +	/* continue with reduced functionality */
> > +}
> > +
> > +static void i915_component_master_cleanup(struct drm_i915_private *dev_priv)
> > +{
> > +	/* the following is ok to call even if component_master_add failed */
> > +	component_master_del(dev_priv->dev->dev, &i915_component_master_ops);
> > +}
> > +
> >  /**
> >   * i915_driver_load - setup chip and create an initial config
> >   * @dev: DRM device
> > @@ -830,6 +906,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  
> >  	intel_runtime_pm_enable(dev_priv);
> >  
> > +	i915_component_master_init(dev_priv);
> > +
> >  	return 0;
> >  
> >  out_power_well:
> > @@ -870,6 +948,8 @@ int i915_driver_unload(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > +	i915_component_master_cleanup(dev_priv);
> > +
> >  	ret = i915_gem_suspend(dev);
> >  	if (ret) {
> >  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > new file mode 100644
> > index 0000000..44542c2
> > --- /dev/null
> > +++ b/include/drm/i915_component.h
> > @@ -0,0 +1,38 @@
> > +/**************************************************************************
> > + *
> > + * Copyright 2014 Intel Inc.
> > + * All Rights Reserved.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > + * "Software"), to deal in the Software without restriction, including
> > + * without limitation the rights to use, copy, modify, merge, publish,
> > + * distribute, sub license, and/or sell copies of the Software, and to
> > + * permit persons to whom the Software is furnished to do so, subject to
> > + * the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > + * next paragraph) shall be included in all copies or substantial portions
> > + * of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> > + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> > + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> > + *
> > + **************************************************************************/
> > +
> > +#ifndef _I915_COMPONENT_H_
> > +#define _I915_COMPONENT_H_
> > +
> > +struct i915_component_ops {
> > +	struct module *owner;
> > +	void (*get_power)(struct device *);
> > +	void (*put_power)(struct device *);
> > +	int (*get_cdclk)(struct device *);
> > +};
> > +
> > +#endif				/* _I915_COMPONENT_H_ */
> > -- 
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: add dev_to_i915_priv helper

[Daniel Vetter]

On Mon, Dec 08, 2014 at 06:42:05PM +0200, Imre Deak wrote:
> This will be needed by later patches, so factor it out.
> 
> No functional change.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  | 15 ++++++---------
>  drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 71be3c9..32c2e33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev)
>  
>  static int i915_pm_suspend(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
> +	struct drm_device *drm_dev = dev_priv->dev;
>  
> -	if (!drm_dev || !drm_dev->dev_private) {
> +	if (!drm_dev || !dev_priv) {

btw all checks for drm_dev and dev_private are remnants from the old ums
days and can all be killed. With fire, please.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: add dev_to_i915_priv helper

[Chris Wilson]

On Mon, Dec 08, 2014 at 06:42:05PM +0200, Imre Deak wrote:
> This will be needed by later patches, so factor it out.

The similiar function (for drm_device) is known as to_i915(), so just
call this dev_to_i915().
> 
> No functional change.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  | 15 ++++++---------
>  drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 71be3c9..32c2e33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev)
>  
>  static int i915_pm_suspend(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
> +	struct drm_device *drm_dev = dev_priv->dev;
>  
> -	if (!drm_dev || !drm_dev->dev_private) {
> +	if (!drm_dev || !dev_priv) {

This test is quite nonsensical since you've dereferenced both pointers
getting to this point. If it is required, we need a new patch.

>  		dev_err(dev, "DRM not initialized, aborting suspend.\n");
>  		return -ENODEV;
>  	}
> @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev)
>  
>  static int i915_pm_suspend_late(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
>  
>  	/*
>  	 * We have a suspedn ordering issue with the snd-hda driver also
> @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev)
>  
>  static int i915_pm_resume_early(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
>  
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
> @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev)
>  
>  static int i915_pm_resume(struct device *dev)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
>  
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 61a88fa..3de7a55 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
>  	return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1;
>  }
>  
> +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +
> +	return drm_dev->dev_private;

I think you meant 

static inline struct drm_i915_private *dev_to_i915(struct device *dev)
{
	return to_i915(pci_get_drvdata(to_pci_dev(dev)));
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: add dev_to_i915_priv helper

[Imre Deak]

On Mon, 2014-12-08 at 20:40 +0000, Chris Wilson wrote:
> On Mon, Dec 08, 2014 at 06:42:05PM +0200, Imre Deak wrote:
> > This will be needed by later patches, so factor it out.
> 
> The similiar function (for drm_device) is known as to_i915(), so just
> call this dev_to_i915().
> > 
> > No functional change.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c  | 15 ++++++---------
> >  drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
> >  2 files changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 71be3c9..32c2e33 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev)
> >  
> >  static int i915_pm_suspend(struct device *dev)
> >  {
> > -	struct pci_dev *pdev = to_pci_dev(dev);
> > -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_i915_private *dev_priv = dev_to_i915_priv(dev);
> > +	struct drm_device *drm_dev = dev_priv->dev;
> >  
> > -	if (!drm_dev || !drm_dev->dev_private) {
> > +	if (!drm_dev || !dev_priv) {
> 
> This test is quite nonsensical since you've dereferenced both pointers
> getting to this point. If it is required, we need a new patch.

Yep, I noticed this after Jani's comment. For now I would just leave
this part unchanged.

> 
> >  		dev_err(dev, "DRM not initialized, aborting suspend.\n");
> >  		return -ENODEV;
> >  	}
> > @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev)
> >  
> >  static int i915_pm_suspend_late(struct device *dev)
> >  {
> > -	struct pci_dev *pdev = to_pci_dev(dev);
> > -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
> >  
> >  	/*
> >  	 * We have a suspedn ordering issue with the snd-hda driver also
> > @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev)
> >  
> >  static int i915_pm_resume_early(struct device *dev)
> >  {
> > -	struct pci_dev *pdev = to_pci_dev(dev);
> > -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
> >  
> >  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  		return 0;
> > @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev)
> >  
> >  static int i915_pm_resume(struct device *dev)
> >  {
> > -	struct pci_dev *pdev = to_pci_dev(dev);
> > -	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev;
> >  
> >  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  		return 0;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 61a88fa..3de7a55 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
> >  	return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1;
> >  }
> >  
> > +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +
> > +	return drm_dev->dev_private;
> 
> I think you meant 
> 
> static inline struct drm_i915_private *dev_to_i915(struct device *dev)
> {
> 	return to_i915(pci_get_drvdata(to_pci_dev(dev)));
> }

Yes, looks simpler.

> -Chris
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Imre Deak]

On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > The current hda/i915 interface to enable/disable power wells and query
> > the CD clock rate is based on looking up the relevant i915 module
> > symbols from the hda driver. By using the component framework we can get
> > rid of some global state tracking in the i915 driver and pave the way to
> > fully decouple the two drivers: once support is added to enable/disable
> > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > itself from the i915 component master, without the need to keep a
> > reference on the i915 module.
> > 
> > This also gets rid of the problem that currently the i915 driver exposes
> > the interface only on HSW and BDW, while it's also needed at least on
> > VLV/CHV.
> 
> Awesome that you're tackling this, really happy to see these hacks go.
> Unfortunately I think it's upside down: hda should be the component master
> and i915 should only register a component.
> 
> Longer story: The main reason for the component helpers is to be able to
> magically delay registering the main/master driver until all the
> components are loaded. Otherwise -EDEFER doesn't work and also the
> suspend/resume ordering this should result in. Master here means whatever
> userspace eventually sees as a device node or similar, component is
> anything really that this userspace interfaces needs to function
> correctly.

EDEFER doesn't solve the suspend/resume ordering, at least not in the
general async s/r case. In any case I don't see a problem in making the
hda component the master and I think it's more logical that way as you
said, so I changed that.

> I think what we need here is then:
> - Put the current azx_probe into the master_bind callback. The new
>   azx_probe will do nothing else than register the master component and
>   the component match list. To do so it checks the chip flag and if it
>   needs to cooperate with i915 it registers one component match for that.
>   The master_bind (old probe) function calls component_bind_all with the
>   hda_intel pointer as void *data parameter.

I'm not sure this is the best way since by this the i915 module would
need to be pinned even when no HDMI functionality is used. I think a
better way would be to let the probe function run as now and
init/register all the non-HDMI functionality. Then only the HDMI
functionality would be inited/registered from the bind/unbind hooks.

But we could go either way even as a follow-up to this patchset.

> - i915 registers a component for the i915 gfx device. It uses the
>   component device to get at i915 sturctures and fills the dev+ops into
>   the hda_intel pointer it gets as void *data.
>
> Stuff we then should do on top:
> - Add deferred probing to azx_probe: Only when all components are there
>   should it actually register. This will take care of all the module load
>   order mess.

I agree that we should only register user interfaces when everything is
in place. But I'm not sure deferred probe is the best, we could do
without it by registering HDMI from the component bind hook.

>   It should also take care of system suspend/resume ordering and we
>   should be able to delete all the early_resume/late_suspend trickery.

Deferred probe doesn't solve the suspend/resume ordering, we would need
to have a separate HDMI device that is set as a child to the i915
device. Alternatively we could use device_pm_wait_for_dev().

> Imo we should have things ready up to this point to make sure this
> refactoring actually works.

I think we could go with this minimal patch with your change to have the
hda component be master. This only adds the support for components and
keeps everything else the same. We could consider the bigger changes as
a follow-up.

> Then there's some cool stuff we could do on top:
> - Register a i915-hda platform devices as a child of the gfx pci device.
>   Besides shuffling around a bit with the interfaces/argument casting and
>   the component match function this doesn't really have a functional
>   impact. But it makes the relationship more clear since hda doesn't
>   really need the entire pci device, but only the small part that does
>   audio.
> 
> - Replace the hand-rolled power-well interface with runtime pm on that
>   device node.
> 
> - If system suspend/resume doesn't work automatically with deferred
>   probing (tbh I'm not sure) add pm_ops to the component master. Then add
>   some functions as default implementations for pm_ops for components
>   which simply refcount all component pm_ops calls and call the master
>   pm_ops suspend on the first suspend call and resume on the last resume
>   call. That really should take care of suspend/resume ordering for good.

Yep, these sound good. I think having an HDMI child device is the
cleanest solution for the s/r ordering issue.

--Imre

> 
> Cheers, Daniel
> 
> > 
> > Imre Deak (5):
> >   drm/i915: add dev_to_i915_priv helper
> >   drm/i915: add component support
> >   ALSA: hda: pass chip to all i915 interface functions
> >   ALSA: hda: add component support
> >   drm/i915: remove unused power_well/get_cdclk_freq api
> > 
> >  drivers/gpu/drm/i915/i915_dma.c         |  80 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.c         |  15 ++--
> >  drivers/gpu/drm/i915/intel_drv.h        |   8 ++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  56 --------------
> >  include/drm/i915_component.h            |  38 ++++++++++
> >  include/drm/i915_powerwell.h            |  37 ----------
> >  sound/pci/hda/hda_i915.c                | 126 +++++++++++++++++++++-----------
> >  sound/pci/hda/hda_i915.h                |  12 +--
> >  sound/pci/hda/hda_intel.c               |  16 ++--
> >  sound/pci/hda/hda_priv.h                |   7 ++
> >  10 files changed, 238 insertions(+), 157 deletions(-)
> >  create mode 100644 include/drm/i915_component.h
> >  delete mode 100644 include/drm/i915_powerwell.h
> > 
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/5] drm/i915: add dev_to_i915 helper

[Imre Deak]

This will be needed by later patches, so factor it out.

No functional change.

v2:
- s/dev_to_i915_priv/dev_to_i915/ (Jani)
- don't use the helper in i915_pm_suspend (Chris)
- simplify the helper (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 9 +++------
 drivers/gpu/drm/i915/i915_drv.h | 5 +++++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 71be3c9..d2ae327 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev)
 
 static int i915_pm_suspend_late(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
 
 	/*
 	 * We have a suspedn ordering issue with the snd-hda driver also
@@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev)
 
 static int i915_pm_resume_early(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
 
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
@@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev)
 
 static int i915_pm_resume(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
 
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11e85cb..fb2616d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1800,6 +1800,11 @@ static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
 	return dev->dev_private;
 }
 
+static inline struct drm_i915_private *dev_to_i915(struct device *dev)
+{
+	return to_i915(pci_get_drvdata(to_pci_dev(dev)));
+}
+
 /* Iterate over initialised rings */
 #define for_each_ring(ring__, dev_priv__, i__) \
 	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/5] drm/i915: add component support

[Imre Deak]

Register a component to be used to interface with the snd_hda_intel
driver. This is meant to replace the same interface that is currently
based on module symbol lookup.

v2:
- change roles between the hda and i915 components (Daniel)
- add the implementation to a new file (Jani)
- use better namespacing (Jani)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/Makefile         |   3 +-
 drivers/gpu/drm/i915/i915_component.c | 109 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_dma.c       |   4 ++
 drivers/gpu/drm/i915/i915_drv.h       |   3 +
 drivers/gpu/drm/i915/intel_drv.h      |   4 ++
 include/drm/i915_component.h          |  38 ++++++++++++
 6 files changed, 160 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/i915_component.c
 create mode 100644 include/drm/i915_component.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e4083e4..6b5b082 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -7,7 +7,8 @@ ccflags-y := -Iinclude/drm
 # Please keep these build lists sorted!
 
 # core driver code
-i915-y := i915_drv.o \
+i915-y := i915_component.o \
+	  i915_drv.o \
 	  i915_params.o \
           i915_suspend.o \
 	  i915_sysfs.o \
diff --git a/drivers/gpu/drm/i915/i915_component.c b/drivers/gpu/drm/i915/i915_component.c
new file mode 100644
index 0000000..4bb49f0
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_component.c
@@ -0,0 +1,109 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <linux/component.h>
+#include <drm/i915_component.h>
+#include "intel_drv.h"
+
+static void display_component_get_power(struct device *dev)
+{
+	intel_display_power_get(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
+}
+
+static void display_component_put_power(struct device *dev)
+{
+	intel_display_power_put(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
+}
+
+/* Get CDCLK in kHz  */
+static int display_component_get_cdclk_freq(struct device *dev)
+{
+	struct drm_i915_private *dev_priv = dev_to_i915(dev);
+	int ret;
+
+	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
+		return -ENODEV;
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+	ret = intel_ddi_get_cdclk_freq(dev_priv);
+	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
+
+	return ret;
+}
+
+static const struct i915_display_component_ops display_component_ops = {
+	.owner		= THIS_MODULE,
+	.get_power	= display_component_get_power,
+	.put_power	= display_component_put_power,
+	.get_cdclk_freq	= display_component_get_cdclk_freq,
+};
+
+static int display_component_bind(struct device *i915_dev,
+				  struct device *hda_dev, void *data)
+{
+	struct i915_display_component *dcomp = data;
+
+	if (WARN_ON(dcomp->ops || dcomp->dev))
+		return -EEXIST;
+
+	dcomp->ops = &display_component_ops;
+	dcomp->dev = i915_dev;
+
+	return 0;
+}
+
+static void display_component_unbind(struct device *i915_dev,
+				     struct device *hda_dev, void *data)
+{
+	struct i915_display_component *dcomp = data;
+
+	dcomp->ops = NULL;
+	dcomp->dev = NULL;
+}
+
+static const struct component_ops display_component_bind_ops = {
+	.bind	= display_component_bind,
+	.unbind	= display_component_unbind,
+};
+
+void i915_component_init(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	ret = component_add(dev_priv->dev->dev, &display_component_bind_ops);
+	if (ret < 0) {
+		DRM_ERROR("failed to add display component (%d)\n", ret);
+		/* continue with reduced functionality */
+		return;
+	}
+
+	dev_priv->display_component_registered = true;
+}
+
+void i915_component_cleanup(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->display_component_registered) {
+		component_del(dev_priv->dev->dev, &display_component_bind_ops);
+		dev_priv->display_component_registered = false;
+	}
+}
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 887d88f..b6238bb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -830,6 +830,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_runtime_pm_enable(dev_priv);
 
+	i915_component_init(dev_priv);
+
 	return 0;
 
 out_power_well:
@@ -870,6 +872,8 @@ int i915_driver_unload(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	i915_component_cleanup(dev_priv);
+
 	ret = i915_gem_suspend(dev);
 	if (ret) {
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fb2616d..3b7ae14 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1684,6 +1684,9 @@ struct drm_i915_private {
 
 	bool mchbar_need_disable;
 
+	/* hda/i915 display component */
+	bool display_component_registered;
+
 	struct intel_l3_parity l3_parity;
 
 	/* Cannot be determined by PCIID. You must always read a register. */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61a88fa..856709e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -809,6 +809,10 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
 void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv);
 
+/* i915_component.c */
+void i915_component_init(struct drm_i915_private *dev_priv);
+void i915_component_cleanup(struct drm_i915_private *dev_priv);
+
 /* intel_crt.c */
 void intel_crt_init(struct drm_device *dev);
 
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
new file mode 100644
index 0000000..9c660ca
--- /dev/null
+++ b/include/drm/i915_component.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef _I915_COMPONENT_H_
+#define _I915_COMPONENT_H_
+
+struct i915_display_component {
+	struct device *dev;
+
+	const struct i915_display_component_ops {
+		struct module *owner;
+		void (*get_power)(struct device *);
+		void (*put_power)(struct device *);
+		int (*get_cdclk_freq)(struct device *);
+	} *ops;
+};
+
+#endif /* _I915_COMPONENT_H_ */
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/5] ALSA: hda: add component support

[Imre Deak]

Register a component master to be used to interface with the i915
driver. This is meant to replace the current interface which is based on
module symbol lookups.

Note that currently we keep the existing behavior and pin the i915
module while the hda driver is loaded. Using the component interface
allows us to remove this dependency once support for dynamically
enabling / disabling the HDMI functionality is added to the driver.

v2:
- change roles between the hda and i915 components (Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 sound/pci/hda/hda_i915.c  | 137 +++++++++++++++++++++++++++++++++-------------
 sound/pci/hda/hda_intel.c |   3 +-
 sound/pci/hda/hda_priv.h  |   4 ++
 3 files changed, 104 insertions(+), 40 deletions(-)

diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
index 4e4b733..2bf6a1b 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -18,8 +18,10 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/component.h>
+#include <drm/i915_component.h>
 #include <sound/core.h>
-#include <drm/i915_powerwell.h>
 #include "hda_priv.h"
 #include "hda_i915.h"
 
@@ -31,32 +33,33 @@
 #define AZX_REG_EM4			0x100c
 #define AZX_REG_EM5			0x1010
 
-static int (*get_power)(void);
-static int (*put_power)(void);
-static int (*get_cdclk)(void);
-
 int hda_display_power(struct azx *chip, bool enable)
 {
-	if (!get_power || !put_power)
+	struct i915_display_component *dcomp = &chip->display_component;
+
+	if (!dcomp->ops)
 		return -ENODEV;
 
 	pr_debug("HDA display power %s \n",
 			enable ? "Enable" : "Disable");
 	if (enable)
-		return get_power();
+		dcomp->ops->get_power(dcomp->dev);
 	else
-		return put_power();
+		dcomp->ops->put_power(dcomp->dev);
+
+	return 0;
 }
 
 void haswell_set_bclk(struct azx *chip)
 {
 	int cdclk_freq;
 	unsigned int bclk_m, bclk_n;
+	struct i915_display_component *dcomp = &chip->display_component;
 
-	if (!get_cdclk)
+	if (!dcomp->ops)
 		return;
 
-	cdclk_freq = get_cdclk();
+	cdclk_freq = dcomp->ops->get_cdclk_freq(dcomp->dev);
 	switch (cdclk_freq) {
 	case 337500:
 		bclk_m = 16;
@@ -84,47 +87,104 @@ void haswell_set_bclk(struct azx *chip)
 	azx_writew(chip, EM5, bclk_n);
 }
 
+static int hda_component_master_bind(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct i915_display_component *dcomp = &chip->display_component;
+	int ret;
+
+	ret = component_bind_all(dev, dcomp);
+	if (ret < 0)
+		return ret;
+
+	if (WARN_ON(!(dcomp->dev && dcomp->ops && dcomp->ops->get_power &&
+		      dcomp->ops->put_power && dcomp->ops->get_cdclk_freq))) {
+		ret = -EINVAL;
+		goto out_unbind;
+	}
+
+	/*
+	 * Atm, we don't support dynamic unbinding initiated by the child
+	 * component, so pin its containing module until we unbind.
+	 */
+	if (!try_module_get(dcomp->ops->owner)) {
+		ret = -ENODEV;
+		goto out_unbind;
+	}
+
+	return 0;
+
+out_unbind:
+	component_unbind_all(dev, dcomp);
+
+	return ret;
+}
+
+static void hda_component_master_unbind(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct i915_display_component *dcomp = &chip->display_component;
+
+	module_put(dcomp->ops->owner);
+	component_unbind_all(dev, dcomp);
+	WARN_ON(dcomp->ops || dcomp->dev);
+}
+
+static const struct component_master_ops hda_component_master_ops = {
+	.bind = hda_component_master_bind,
+	.unbind = hda_component_master_unbind,
+};
+
+static int hda_component_master_match(struct device *dev, void *data)
+{
+	/* i915 is the only supported component */
+	return !strcmp(dev->driver->name, "i915");
+}
 
 int hda_i915_init(struct azx *chip)
 {
-	int err = 0;
+	struct component_match *match = NULL;
+	struct device *dev = &chip->pci->dev;
+	struct i915_display_component *dcomp = &chip->display_component;
+	int ret;
 
-	get_power = symbol_request(i915_request_power_well);
-	if (!get_power) {
-		pr_warn("hda-i915: get_power symbol get fail\n");
-		return -ENODEV;
-	}
-
-	put_power = symbol_request(i915_release_power_well);
-	if (!put_power) {
-		symbol_put(i915_request_power_well);
-		get_power = NULL;
-		return -ENODEV;
-	}
-
-	get_cdclk = symbol_request(i915_get_cdclk_freq);
-	if (!get_cdclk)	/* may have abnormal BCLK and audio playback rate */
-		pr_warn("hda-i915: get_cdclk symbol get fail\n");
-
+	component_match_add(dev, &match, hda_component_master_match, chip);
+	ret = component_master_add_with_match(dev, &hda_component_master_ops,
+					      match);
+	if (ret < 0)
+		goto out_err;
+
+	/*
+	 * Atm, we don't support deferring the component binding, so make sure
+	 * i915 is loaded and that the binding successfully completes.
+	 */
+	ret = request_module("i915");
+	if (ret < 0)
+		goto out_master_del;
+
+	if (!dcomp->ops) {
+		ret = -ENODEV;
+		goto out_master_del;
+	}
+
-	pr_debug("HDA driver get symbol successfully from i915 module\n");
+	pr_debug("hda-i915: bound to i915 component\n");
+
+	return 0;
+out_master_del:
+	component_master_del(dev, &hda_component_master_ops);
+out_err:
+	pr_warn("hda-i915: failed to add component master (%d)\n", ret);
 
-	return err;
+	return ret;
 }
 
 int hda_i915_exit(struct azx *chip)
 {
-	if (get_power) {
-		symbol_put(i915_request_power_well);
-		get_power = NULL;
-	}
-	if (put_power) {
-		symbol_put(i915_release_power_well);
-		put_power = NULL;
-	}
-	if (get_cdclk) {
-		symbol_put(i915_get_cdclk_freq);
-		get_cdclk = NULL;
-	}
+	struct device *dev = &chip->pci->dev;
+
+	component_master_del(dev, &hda_component_master_ops);
 
 	return 0;
 }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index f3b5dcd..e15b30e 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1912,8 +1912,7 @@ static int azx_probe_continue(struct azx *chip)
 #ifdef CONFIG_SND_HDA_I915
 		err = hda_i915_init(chip);
 		if (err < 0) {
-			dev_err(chip->card->dev,
-				"Error request power-well from i915\n");
+			dev_err(chip->card->dev, "Error binding to i915\n");
 			goto out_free;
 		}
 		err = hda_display_power(chip, true);
diff --git a/sound/pci/hda/hda_priv.h b/sound/pci/hda/hda_priv.h
index aa484fd..af227f3 100644
--- a/sound/pci/hda/hda_priv.h
+++ b/sound/pci/hda/hda_priv.h
@@ -18,6 +18,7 @@
 #include <linux/clocksource.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
+#include <drm/i915_component.h>
 
 /*
  * registers
@@ -291,6 +292,9 @@ struct azx {
 	struct pci_dev *pci;
 	int dev_index;
 
+	/* i915 component interface */
+	struct i915_display_component display_component;
+
 	/* chip type specific */
 	int driver_type;
 	unsigned int driver_caps;
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Daniel Vetter]

On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > The current hda/i915 interface to enable/disable power wells and query
> > > the CD clock rate is based on looking up the relevant i915 module
> > > symbols from the hda driver. By using the component framework we can get
> > > rid of some global state tracking in the i915 driver and pave the way to
> > > fully decouple the two drivers: once support is added to enable/disable
> > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > itself from the i915 component master, without the need to keep a
> > > reference on the i915 module.
> > > 
> > > This also gets rid of the problem that currently the i915 driver exposes
> > > the interface only on HSW and BDW, while it's also needed at least on
> > > VLV/CHV.
> > 
> > Awesome that you're tackling this, really happy to see these hacks go.
> > Unfortunately I think it's upside down: hda should be the component master
> > and i915 should only register a component.
> > 
> > Longer story: The main reason for the component helpers is to be able to
> > magically delay registering the main/master driver until all the
> > components are loaded. Otherwise -EDEFER doesn't work and also the
> > suspend/resume ordering this should result in. Master here means whatever
> > userspace eventually sees as a device node or similar, component is
> > anything really that this userspace interfaces needs to function
> > correctly.
> 
> EDEFER doesn't solve the suspend/resume ordering, at least not in the
> general async s/r case. In any case I don't see a problem in making the
> hda component the master and I think it's more logical that way as you
> said, so I changed that.

Yeah for full async s/r we're screwed as-is. But see below for some crazy
ideas.

> > I think what we need here is then:
> > - Put the current azx_probe into the master_bind callback. The new
> >   azx_probe will do nothing else than register the master component and
> >   the component match list. To do so it checks the chip flag and if it
> >   needs to cooperate with i915 it registers one component match for that.
> >   The master_bind (old probe) function calls component_bind_all with the
> >   hda_intel pointer as void *data parameter.
> 
> I'm not sure this is the best way since by this the i915 module would
> need to be pinned even when no HDMI functionality is used. I think a
> better way would be to let the probe function run as now and
> init/register all the non-HDMI functionality. Then only the HDMI
> functionality would be inited/registered from the bind/unbind hooks.

Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
to be able to load the driver. But if we can defer just the hdmi part.

> But we could go either way even as a follow-up to this patchset.
> 
> > - i915 registers a component for the i915 gfx device. It uses the
> >   component device to get at i915 sturctures and fills the dev+ops into
> >   the hda_intel pointer it gets as void *data.
> >
> > Stuff we then should do on top:
> > - Add deferred probing to azx_probe: Only when all components are there
> >   should it actually register. This will take care of all the module load
> >   order mess.
> 
> I agree that we should only register user interfaces when everything is
> in place. But I'm not sure deferred probe is the best, we could do
> without it by registering HDMI from the component bind hook.

It's mostly a question whether alsa does support delayed addition of
interface parts. DRM most definitely does not (except for recently added
dp mst connector hotplug). But I guess if the current driver already
delays registering the hdmi part then we're fine. I'm not sure about
whether it's really safe - I spotted not a lot of locking really to make
sure there's no races between i915 loading and userspace trying to access
the hdmi side.

> >   It should also take care of system suspend/resume ordering and we
> >   should be able to delete all the early_resume/late_suspend trickery.
> 
> Deferred probe doesn't solve the suspend/resume ordering, we would need
> to have a separate HDMI device that is set as a child to the i915
> device. Alternatively we could use device_pm_wait_for_dev().

I'm not sure whether there's the possibility of deadlocks with
device_pm_wait_for_dev. But if that works I think a component helper to
wait for all components to resume would be really useful. Or we implement
the full-blown pm_ops idea laid out below for components.
> 
> > Imo we should have things ready up to this point to make sure this
> > refactoring actually works.
> 
> I think we could go with this minimal patch with your change to have the
> hda component be master. This only adds the support for components and
> keeps everything else the same. We could consider the bigger changes as
> a follow-up.

Yeah that was my plan - if EDEFER isn't enough then we keep the early/late
resume/suspend hooks for a bit longer.

> > Then there's some cool stuff we could do on top:
> > - Register a i915-hda platform devices as a child of the gfx pci device.
> >   Besides shuffling around a bit with the interfaces/argument casting and
> >   the component match function this doesn't really have a functional
> >   impact. But it makes the relationship more clear since hda doesn't
> >   really need the entire pci device, but only the small part that does
> >   audio.
> > 
> > - Replace the hand-rolled power-well interface with runtime pm on that
> >   device node.
> > 
> > - If system suspend/resume doesn't work automatically with deferred
> >   probing (tbh I'm not sure) add pm_ops to the component master. Then add
> >   some functions as default implementations for pm_ops for components
> >   which simply refcount all component pm_ops calls and call the master
> >   pm_ops suspend on the first suspend call and resume on the last resume
> >   call. That really should take care of suspend/resume ordering for good.
> 
> Yep, these sound good. I think having an HDMI child device is the
> cleanest solution for the s/r ordering issue.

Ok, sounds like we have a plan. And thanks again for tackling this, I'm
really happy to see this go away.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/5] drm/i915: add dev_to_i915 helper

[Daniel Vetter]

On Tue, Dec 09, 2014 at 11:41:16AM +0200, Imre Deak wrote:
> +static inline struct drm_i915_private *dev_to_i915(struct device *dev)
> +{
> +	return to_i915(pci_get_drvdata(to_pci_dev(dev)));
> +}

dev_get_drvdata. No need to upcast to pci_dev for pci_get_drvdata to
downcast to device again. Let's really simplify this ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/5] drm/i915: add component support

[Daniel Vetter]

On Tue, Dec 09, 2014 at 11:41:17AM +0200, Imre Deak wrote:
> Register a component to be used to interface with the snd_hda_intel
> driver. This is meant to replace the same interface that is currently
> based on module symbol lookup.
> 
> v2:
> - change roles between the hda and i915 components (Daniel)
> - add the implementation to a new file (Jani)

I disagree with the name here - intel_component.c is not really
descriptive since it's not really. Imo it makes much more sense to put
this into intel_audio.c. After all it's all about how we interact with the
audio side, which will be even more obvious once we have a dedicated
subdevice for this.

Also please add a bit of kerneldoc to the _init/_cleanup functions with a
few words about what this is used for. Or if you want, extract a short
DOC: section for it even to describe the interactions.
-Daniel

> - use better namespacing (Jani)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile         |   3 +-
>  drivers/gpu/drm/i915/i915_component.c | 109 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_dma.c       |   4 ++
>  drivers/gpu/drm/i915/i915_drv.h       |   3 +
>  drivers/gpu/drm/i915/intel_drv.h      |   4 ++
>  include/drm/i915_component.h          |  38 ++++++++++++
>  6 files changed, 160 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_component.c
>  create mode 100644 include/drm/i915_component.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e4083e4..6b5b082 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -7,7 +7,8 @@ ccflags-y := -Iinclude/drm
>  # Please keep these build lists sorted!
>  
>  # core driver code
> -i915-y := i915_drv.o \
> +i915-y := i915_component.o \
> +	  i915_drv.o \
>  	  i915_params.o \
>            i915_suspend.o \
>  	  i915_sysfs.o \
> diff --git a/drivers/gpu/drm/i915/i915_component.c b/drivers/gpu/drm/i915/i915_component.c
> new file mode 100644
> index 0000000..4bb49f0
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_component.c
> @@ -0,0 +1,109 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include <linux/component.h>
> +#include <drm/i915_component.h>
> +#include "intel_drv.h"
> +
> +static void display_component_get_power(struct device *dev)
> +{
> +	intel_display_power_get(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
> +}
> +
> +static void display_component_put_power(struct device *dev)
> +{
> +	intel_display_power_put(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
> +}
> +
> +/* Get CDCLK in kHz  */
> +static int display_component_get_cdclk_freq(struct device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
> +		return -ENODEV;
> +
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> +	ret = intel_ddi_get_cdclk_freq(dev_priv);
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> +
> +	return ret;
> +}
> +
> +static const struct i915_display_component_ops display_component_ops = {
> +	.owner		= THIS_MODULE,
> +	.get_power	= display_component_get_power,
> +	.put_power	= display_component_put_power,
> +	.get_cdclk_freq	= display_component_get_cdclk_freq,
> +};
> +
> +static int display_component_bind(struct device *i915_dev,
> +				  struct device *hda_dev, void *data)
> +{
> +	struct i915_display_component *dcomp = data;
> +
> +	if (WARN_ON(dcomp->ops || dcomp->dev))
> +		return -EEXIST;
> +
> +	dcomp->ops = &display_component_ops;
> +	dcomp->dev = i915_dev;
> +
> +	return 0;
> +}
> +
> +static void display_component_unbind(struct device *i915_dev,
> +				     struct device *hda_dev, void *data)
> +{
> +	struct i915_display_component *dcomp = data;
> +
> +	dcomp->ops = NULL;
> +	dcomp->dev = NULL;
> +}
> +
> +static const struct component_ops display_component_bind_ops = {
> +	.bind	= display_component_bind,
> +	.unbind	= display_component_unbind,
> +};
> +
> +void i915_component_init(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	ret = component_add(dev_priv->dev->dev, &display_component_bind_ops);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to add display component (%d)\n", ret);
> +		/* continue with reduced functionality */
> +		return;
> +	}
> +
> +	dev_priv->display_component_registered = true;
> +}
> +
> +void i915_component_cleanup(struct drm_i915_private *dev_priv)
> +{
> +	if (dev_priv->display_component_registered) {
> +		component_del(dev_priv->dev->dev, &display_component_bind_ops);
> +		dev_priv->display_component_registered = false;
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 887d88f..b6238bb 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -830,6 +830,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_runtime_pm_enable(dev_priv);
>  
> +	i915_component_init(dev_priv);
> +
>  	return 0;
>  
>  out_power_well:
> @@ -870,6 +872,8 @@ int i915_driver_unload(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	i915_component_cleanup(dev_priv);
> +
>  	ret = i915_gem_suspend(dev);
>  	if (ret) {
>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fb2616d..3b7ae14 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1684,6 +1684,9 @@ struct drm_i915_private {
>  
>  	bool mchbar_need_disable;
>  
> +	/* hda/i915 display component */
> +	bool display_component_registered;
> +
>  	struct intel_l3_parity l3_parity;
>  
>  	/* Cannot be determined by PCIID. You must always read a register. */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 61a88fa..856709e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -809,6 +809,10 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
>  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv);
>  
> +/* i915_component.c */
> +void i915_component_init(struct drm_i915_private *dev_priv);
> +void i915_component_cleanup(struct drm_i915_private *dev_priv);
> +
>  /* intel_crt.c */
>  void intel_crt_init(struct drm_device *dev);
>  
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> new file mode 100644
> index 0000000..9c660ca
> --- /dev/null
> +++ b/include/drm/i915_component.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef _I915_COMPONENT_H_
> +#define _I915_COMPONENT_H_
> +
> +struct i915_display_component {
> +	struct device *dev;
> +
> +	const struct i915_display_component_ops {
> +		struct module *owner;
> +		void (*get_power)(struct device *);
> +		void (*put_power)(struct device *);
> +		int (*get_cdclk_freq)(struct device *);
> +	} *ops;
> +};
> +
> +#endif /* _I915_COMPONENT_H_ */
> -- 
> 1.8.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/5] ALSA: hda: add component support

[Daniel Vetter]

On Tue, Dec 09, 2014 at 11:41:18AM +0200, Imre Deak wrote:
> Register a component master to be used to interface with the i915
> driver. This is meant to replace the current interface which is based on
> module symbol lookups.
> 
> Note that currently we keep the existing behavior and pin the i915
> module while the hda driver is loaded. Using the component interface
> allows us to remove this dependency once support for dynamically
> enabling / disabling the HDMI functionality is added to the driver.
> 
> v2:
> - change roles between the hda and i915 components (Daniel)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Ok, I think this is a good foundation to build on and later on add more
pieces to fix suspend/resume and similar.

But we also need to discuss how to merge this. My proposal is that I do a
special topic branch with just the patches in this series and then pull
that into drm-intel-next and send a pull request for it to Takashi for
sound-next, too. That way we don't have any depencies between drm and
sound for the 3.20 merge window.

Testing by intel QA is also solved this way since sound-next is already
pulled into drm-intel-nightly. And both gfx and sound QA use that branch.

Takashi/Dave, does this sound good?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/5] drm/i915: add component support

[Jani Nikula]

On Tue, 09 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 09, 2014 at 11:41:17AM +0200, Imre Deak wrote:
>> Register a component to be used to interface with the snd_hda_intel
>> driver. This is meant to replace the same interface that is currently
>> based on module symbol lookup.
>> 
>> v2:
>> - change roles between the hda and i915 components (Daniel)
>> - add the implementation to a new file (Jani)
>
> I disagree with the name here - intel_component.c is not really
> descriptive since it's not really. Imo it makes much more sense to put
> this into intel_audio.c. After all it's all about how we interact with the
> audio side, which will be even more obvious once we have a dedicated
> subdevice for this.

If we keep this component audio specific, then I guess I agree
intel_audio.c is the better place for it. But that means anything else
(like possibly pmic driver interaction) will need to have a component of
its own.

BR,
Jani.


>
> Also please add a bit of kerneldoc to the _init/_cleanup functions with a
> few words about what this is used for. Or if you want, extract a short
> DOC: section for it even to describe the interactions.
> -Daniel
>
>> - use better namespacing (Jani)
>> 
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/Makefile         |   3 +-
>>  drivers/gpu/drm/i915/i915_component.c | 109 ++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/i915_dma.c       |   4 ++
>>  drivers/gpu/drm/i915/i915_drv.h       |   3 +
>>  drivers/gpu/drm/i915/intel_drv.h      |   4 ++
>>  include/drm/i915_component.h          |  38 ++++++++++++
>>  6 files changed, 160 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/i915/i915_component.c
>>  create mode 100644 include/drm/i915_component.h
>> 
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index e4083e4..6b5b082 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -7,7 +7,8 @@ ccflags-y := -Iinclude/drm
>>  # Please keep these build lists sorted!
>>  
>>  # core driver code
>> -i915-y := i915_drv.o \
>> +i915-y := i915_component.o \
>> +	  i915_drv.o \
>>  	  i915_params.o \
>>            i915_suspend.o \
>>  	  i915_sysfs.o \
>> diff --git a/drivers/gpu/drm/i915/i915_component.c b/drivers/gpu/drm/i915/i915_component.c
>> new file mode 100644
>> index 0000000..4bb49f0
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_component.c
>> @@ -0,0 +1,109 @@
>> +/*
>> + * Copyright © 2014 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +#include <linux/component.h>
>> +#include <drm/i915_component.h>
>> +#include "intel_drv.h"
>> +
>> +static void display_component_get_power(struct device *dev)
>> +{
>> +	intel_display_power_get(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
>> +}
>> +
>> +static void display_component_put_power(struct device *dev)
>> +{
>> +	intel_display_power_put(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
>> +}
>> +
>> +/* Get CDCLK in kHz  */
>> +static int display_component_get_cdclk_freq(struct device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
>> +	int ret;
>> +
>> +	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
>> +		return -ENODEV;
>> +
>> +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
>> +	ret = intel_ddi_get_cdclk_freq(dev_priv);
>> +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct i915_display_component_ops display_component_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.get_power	= display_component_get_power,
>> +	.put_power	= display_component_put_power,
>> +	.get_cdclk_freq	= display_component_get_cdclk_freq,
>> +};
>> +
>> +static int display_component_bind(struct device *i915_dev,
>> +				  struct device *hda_dev, void *data)
>> +{
>> +	struct i915_display_component *dcomp = data;
>> +
>> +	if (WARN_ON(dcomp->ops || dcomp->dev))
>> +		return -EEXIST;
>> +
>> +	dcomp->ops = &display_component_ops;
>> +	dcomp->dev = i915_dev;
>> +
>> +	return 0;
>> +}
>> +
>> +static void display_component_unbind(struct device *i915_dev,
>> +				     struct device *hda_dev, void *data)
>> +{
>> +	struct i915_display_component *dcomp = data;
>> +
>> +	dcomp->ops = NULL;
>> +	dcomp->dev = NULL;
>> +}
>> +
>> +static const struct component_ops display_component_bind_ops = {
>> +	.bind	= display_component_bind,
>> +	.unbind	= display_component_unbind,
>> +};
>> +
>> +void i915_component_init(struct drm_i915_private *dev_priv)
>> +{
>> +	int ret;
>> +
>> +	ret = component_add(dev_priv->dev->dev, &display_component_bind_ops);
>> +	if (ret < 0) {
>> +		DRM_ERROR("failed to add display component (%d)\n", ret);
>> +		/* continue with reduced functionality */
>> +		return;
>> +	}
>> +
>> +	dev_priv->display_component_registered = true;
>> +}
>> +
>> +void i915_component_cleanup(struct drm_i915_private *dev_priv)
>> +{
>> +	if (dev_priv->display_component_registered) {
>> +		component_del(dev_priv->dev->dev, &display_component_bind_ops);
>> +		dev_priv->display_component_registered = false;
>> +	}
>> +}
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 887d88f..b6238bb 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -830,6 +830,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>  
>>  	intel_runtime_pm_enable(dev_priv);
>>  
>> +	i915_component_init(dev_priv);
>> +
>>  	return 0;
>>  
>>  out_power_well:
>> @@ -870,6 +872,8 @@ int i915_driver_unload(struct drm_device *dev)
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	int ret;
>>  
>> +	i915_component_cleanup(dev_priv);
>> +
>>  	ret = i915_gem_suspend(dev);
>>  	if (ret) {
>>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index fb2616d..3b7ae14 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1684,6 +1684,9 @@ struct drm_i915_private {
>>  
>>  	bool mchbar_need_disable;
>>  
>> +	/* hda/i915 display component */
>> +	bool display_component_registered;
>> +
>>  	struct intel_l3_parity l3_parity;
>>  
>>  	/* Cannot be determined by PCIID. You must always read a register. */
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 61a88fa..856709e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -809,6 +809,10 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
>>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
>>  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv);
>>  
>> +/* i915_component.c */
>> +void i915_component_init(struct drm_i915_private *dev_priv);
>> +void i915_component_cleanup(struct drm_i915_private *dev_priv);
>> +
>>  /* intel_crt.c */
>>  void intel_crt_init(struct drm_device *dev);
>>  
>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>> new file mode 100644
>> index 0000000..9c660ca
>> --- /dev/null
>> +++ b/include/drm/i915_component.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Copyright © 2014 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +#ifndef _I915_COMPONENT_H_
>> +#define _I915_COMPONENT_H_
>> +
>> +struct i915_display_component {
>> +	struct device *dev;
>> +
>> +	const struct i915_display_component_ops {
>> +		struct module *owner;
>> +		void (*get_power)(struct device *);
>> +		void (*put_power)(struct device *);
>> +		int (*get_cdclk_freq)(struct device *);
>> +	} *ops;
>> +};
>> +
>> +#endif /* _I915_COMPONENT_H_ */
>> -- 
>> 1.8.4
>> 
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/5] drm/i915: add dev_to_i915 helper

[Imre Deak]

This will be needed by later patches, so factor it out.

No functional change.

v2:
- s/dev_to_i915_priv/dev_to_i915/ (Jani)
- don't use the helper in i915_pm_suspend (Chris)
- simplify the helper (Chris)
v3:
- remove redundant upcasting in the helper (Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 9 +++------
 drivers/gpu/drm/i915/i915_drv.h | 5 +++++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 71be3c9..d2ae327 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev)
 
 static int i915_pm_suspend_late(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
 
 	/*
 	 * We have a suspedn ordering issue with the snd-hda driver also
@@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev)
 
 static int i915_pm_resume_early(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
 
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
@@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev)
 
 static int i915_pm_resume(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
 
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11e85cb..0961d7f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1800,6 +1800,11 @@ static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
 	return dev->dev_private;
 }
 
+static inline struct drm_i915_private *dev_to_i915(struct device *dev)
+{
+	return to_i915(dev_get_drvdata(dev));
+}
+
 /* Iterate over initialised rings */
 #define for_each_ring(ring__, dev_priv__, i__) \
 	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/5] drm/i915: add component support

[Imre Deak]

Register a component to be used to interface with the snd_hda_intel
driver. This is meant to replace the same interface that is currently
based on module symbol lookup.

v2:
- change roles between the hda and i915 components (Daniel)
- add the implementation to a new file (Jani)
- use better namespacing (Jani)
v3:
- move the implementation to intel_audio.c (Daniel)
- rename display_component to audio_component (Daniel)
- add kerneldoc (Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_component.c |  27 +++++++++
 drivers/gpu/drm/i915/i915_dma.c       |   4 ++
 drivers/gpu/drm/i915/i915_drv.h       |   3 +
 drivers/gpu/drm/i915/intel_audio.c    | 110 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h      |   2 +
 include/drm/i915_component.h          |  38 ++++++++++++
 6 files changed, 184 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/i915_component.c
 create mode 100644 include/drm/i915_component.h

diff --git a/drivers/gpu/drm/i915/i915_component.c b/drivers/gpu/drm/i915/i915_component.c
new file mode 100644
index 0000000..95dd087
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_component.c
@@ -0,0 +1,27 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <linux/component.h>
+#include <drm/i915_component.h>
+#include "intel_drv.h"
+
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 887d88f..f6334d0 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -830,6 +830,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_runtime_pm_enable(dev_priv);
 
+	i915_audio_component_init(dev_priv);
+
 	return 0;
 
 out_power_well:
@@ -870,6 +872,8 @@ int i915_driver_unload(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	i915_audio_component_cleanup(dev_priv);
+
 	ret = i915_gem_suspend(dev);
 	if (ret) {
 		DRM_ERROR("failed to idle hardware: %d\n", ret);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0961d7f..3c2d9c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1713,6 +1713,9 @@ struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
+	/* hda/i915 audio component */
+	bool audio_component_registered;
+
 	uint32_t hw_context_size;
 	struct list_head context_list;
 
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 2c7ed5c..ee41b88 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -22,6 +22,9 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/component.h>
+#include <drm/i915_component.h>
+#include "intel_drv.h"
 
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
@@ -461,3 +464,110 @@ void intel_init_audio(struct drm_device *dev)
 		dev_priv->display.audio_codec_disable = ilk_audio_codec_disable;
 	}
 }
+
+static void i915_audio_component_get_power(struct device *dev)
+{
+	intel_display_power_get(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
+}
+
+static void i915_audio_component_put_power(struct device *dev)
+{
+	intel_display_power_put(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
+}
+
+/* Get CDCLK in kHz  */
+static int i915_audio_component_get_cdclk_freq(struct device *dev)
+{
+	struct drm_i915_private *dev_priv = dev_to_i915(dev);
+	int ret;
+
+	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
+		return -ENODEV;
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+	ret = intel_ddi_get_cdclk_freq(dev_priv);
+	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
+
+	return ret;
+}
+
+static const struct i915_audio_component_ops i915_audio_component_ops = {
+	.owner		= THIS_MODULE,
+	.get_power	= i915_audio_component_get_power,
+	.put_power	= i915_audio_component_put_power,
+	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
+};
+
+static int i915_audio_component_bind(struct device *i915_dev,
+				     struct device *hda_dev, void *data)
+{
+	struct i915_audio_component *acomp = data;
+
+	if (WARN_ON(acomp->ops || acomp->dev))
+		return -EEXIST;
+
+	acomp->ops = &i915_audio_component_ops;
+	acomp->dev = i915_dev;
+
+	return 0;
+}
+
+static void i915_audio_component_unbind(struct device *i915_dev,
+					struct device *hda_dev, void *data)
+{
+	struct i915_audio_component *acomp = data;
+
+	acomp->ops = NULL;
+	acomp->dev = NULL;
+}
+
+static const struct component_ops i915_audio_component_bind_ops = {
+	.bind	= i915_audio_component_bind,
+	.unbind	= i915_audio_component_unbind,
+};
+
+/**
+ * i915_audio_component_init - initialize and register the audio component
+ * @dev_priv: i915 device instance
+ *
+ * This will register with the component framework a child component which
+ * will bind dynamically to the snd_hda_intel driver's corresponding master
+ * component when the latter is registered. During binding the child
+ * initializes an instance of struct i915_audio_component which it receives
+ * from the master. The master can then start to use the interface defined by
+ * this struct. Each side can break the binding at any point by deregistering
+ * its own component after which each side's component unbind callback is
+ * called.
+ *
+ * We ignore any error during registration and continue with reduced
+ * functionality (i.e. without HDMI audio).
+ */
+void i915_audio_component_init(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	ret = component_add(dev_priv->dev->dev, &i915_audio_component_bind_ops);
+	if (ret < 0) {
+		DRM_ERROR("failed to add audio component (%d)\n", ret);
+		/* continue with reduced functionality */
+		return;
+	}
+
+	dev_priv->audio_component_registered = true;
+}
+
+/**
+ * i915_audio_component_cleanup - deregister the audio component
+ * @dev_priv: i915 device instance
+ *
+ * Deregisters the audio component, breaking any existing binding to the
+ * corresponding snd_hda_intel driver's master component.
+ */
+void i915_audio_component_cleanup(struct drm_i915_private *dev_priv)
+{
+	if (!dev_priv->audio_component_registered)
+		return;
+
+	component_del(dev_priv->dev->dev, &i915_audio_component_bind_ops);
+	dev_priv->audio_component_registered = false;
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61a88fa..61701a6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -873,6 +873,8 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 void intel_init_audio(struct drm_device *dev);
 void intel_audio_codec_enable(struct intel_encoder *encoder);
 void intel_audio_codec_disable(struct intel_encoder *encoder);
+void i915_audio_component_init(struct drm_i915_private *dev_priv);
+void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
 
 /* intel_display.c */
 bool intel_has_pending_fb_unpin(struct drm_device *dev);
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
new file mode 100644
index 0000000..3e2f22e
--- /dev/null
+++ b/include/drm/i915_component.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef _I915_COMPONENT_H_
+#define _I915_COMPONENT_H_
+
+struct i915_audio_component {
+	struct device *dev;
+
+	const struct i915_audio_component_ops {
+		struct module *owner;
+		void (*get_power)(struct device *);
+		void (*put_power)(struct device *);
+		int (*get_cdclk_freq)(struct device *);
+	} *ops;
+};
+
+#endif /* _I915_COMPONENT_H_ */
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 4/5] ALSA: hda: add component support

[Imre Deak]

Register a component master to be used to interface with the i915
driver. This is meant to replace the current interface which is based on
module symbol lookups.

Note that currently we keep the existing behavior and pin the i915
module while the hda driver is loaded. Using the component interface
allows us to remove this dependency once support for dynamically
enabling / disabling the HDMI functionality is added to the driver.

v2:
- change roles between the hda and i915 components (Daniel)
v3:
- rename display_component to audio_component (Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 include/drm/i915_powerwell.h |  37 ------------
 sound/pci/hda/hda_i915.c     | 136 +++++++++++++++++++++++++++++++------------
 sound/pci/hda/hda_intel.c    |   3 +-
 sound/pci/hda/hda_priv.h     |   4 ++
 4 files changed, 103 insertions(+), 77 deletions(-)
 delete mode 100644 include/drm/i915_powerwell.h

diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h
deleted file mode 100644
index baa6f11..0000000
--- a/include/drm/i915_powerwell.h
+++ /dev/null
@@ -1,37 +0,0 @@
-/**************************************************************************
- *
- * Copyright 2013 Intel Inc.
- * All Rights Reserved.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the
- * "Software"), to deal in the Software without restriction, including
- * without limitation the rights to use, copy, modify, merge, publish,
- * distribute, sub license, and/or sell copies of the Software, and to
- * permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- *
- * The above copyright notice and this permission notice (including the
- * next paragraph) shall be included in all copies or substantial portions
- * of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
- * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
- * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
- * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
- * USE OR OTHER DEALINGS IN THE SOFTWARE.
- *
- *
- **************************************************************************/
-
-#ifndef _I915_POWERWELL_H_
-#define _I915_POWERWELL_H_
-
-/* For use by hda_i915 driver */
-extern int i915_request_power_well(void);
-extern int i915_release_power_well(void);
-extern int i915_get_cdclk_freq(void);
-
-#endif				/* _I915_POWERWELL_H_ */
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
index 4e4b733..78894ec 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -18,8 +18,10 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/component.h>
+#include <drm/i915_component.h>
 #include <sound/core.h>
-#include <drm/i915_powerwell.h>
 #include "hda_priv.h"
 #include "hda_i915.h"
 
@@ -31,32 +33,33 @@
 #define AZX_REG_EM4			0x100c
 #define AZX_REG_EM5			0x1010
 
-static int (*get_power)(void);
-static int (*put_power)(void);
-static int (*get_cdclk)(void);
-
 int hda_display_power(struct azx *chip, bool enable)
 {
-	if (!get_power || !put_power)
+	struct i915_audio_component *acomp = &chip->audio_component;
+
+	if (!acomp->ops)
 		return -ENODEV;
 
 	pr_debug("HDA display power %s \n",
 			enable ? "Enable" : "Disable");
 	if (enable)
-		return get_power();
+		acomp->ops->get_power(acomp->dev);
 	else
-		return put_power();
+		acomp->ops->put_power(acomp->dev);
+
+	return 0;
 }
 
 void haswell_set_bclk(struct azx *chip)
 {
 	int cdclk_freq;
 	unsigned int bclk_m, bclk_n;
+	struct i915_audio_component *acomp = &chip->audio_component;
 
-	if (!get_cdclk)
+	if (!acomp->ops)
 		return;
 
-	cdclk_freq = get_cdclk();
+	cdclk_freq = acomp->ops->get_cdclk_freq(acomp->dev);
 	switch (cdclk_freq) {
 	case 337500:
 		bclk_m = 16;
@@ -84,47 +87,104 @@ void haswell_set_bclk(struct azx *chip)
 	azx_writew(chip, EM5, bclk_n);
 }
 
+static int hda_component_master_bind(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct i915_audio_component *acomp = &chip->audio_component;
+	int ret;
+
+	ret = component_bind_all(dev, acomp);
+	if (ret < 0)
+		return ret;
+
+	if (WARN_ON(!(acomp->dev && acomp->ops && acomp->ops->get_power &&
+		      acomp->ops->put_power && acomp->ops->get_cdclk_freq))) {
+		ret = -EINVAL;
+		goto out_unbind;
+	}
+
+	/*
+	 * Atm, we don't support dynamic unbinding initiated by the child
+	 * component, so pin its containing module until we unbind.
+	 */
+	if (!try_module_get(acomp->ops->owner)) {
+		ret = -ENODEV;
+		goto out_unbind;
+	}
+
+	return 0;
+
+out_unbind:
+	component_unbind_all(dev, acomp);
+
+	return ret;
+}
+
+static void hda_component_master_unbind(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct i915_audio_component *acomp = &chip->audio_component;
+
+	module_put(acomp->ops->owner);
+	component_unbind_all(dev, acomp);
+	WARN_ON(acomp->ops || acomp->dev);
+}
+
+static const struct component_master_ops hda_component_master_ops = {
+	.bind = hda_component_master_bind,
+	.unbind = hda_component_master_unbind,
+};
+
+static int hda_component_master_match(struct device *dev, void *data)
+{
+	/* i915 is the only supported component */
+	return !strcmp(dev->driver->name, "i915");
+}
 
 int hda_i915_init(struct azx *chip)
 {
-	int err = 0;
+	struct component_match *match = NULL;
+	struct device *dev = &chip->pci->dev;
+	struct i915_audio_component *acomp = &chip->audio_component;
+	int ret;
 
-	get_power = symbol_request(i915_request_power_well);
-	if (!get_power) {
-		pr_warn("hda-i915: get_power symbol get fail\n");
-		return -ENODEV;
-	}
-
-	put_power = symbol_request(i915_release_power_well);
-	if (!put_power) {
-		symbol_put(i915_request_power_well);
-		get_power = NULL;
-		return -ENODEV;
-	}
-
-	get_cdclk = symbol_request(i915_get_cdclk_freq);
-	if (!get_cdclk)	/* may have abnormal BCLK and audio playback rate */
-		pr_warn("hda-i915: get_cdclk symbol get fail\n");
-
+	component_match_add(dev, &match, hda_component_master_match, chip);
+	ret = component_master_add_with_match(dev, &hda_component_master_ops,
+					      match);
+	if (ret < 0)
+		goto out_err;
+
+	/*
+	 * Atm, we don't support deferring the component binding, so make sure
+	 * i915 is loaded and that the binding successfully completes.
+	 */
+	ret = request_module("i915");
+	if (ret < 0)
+		goto out_master_del;
+
+	if (!acomp->ops) {
+		ret = -ENODEV;
+		goto out_master_del;
+	}
+
-	pr_debug("HDA driver get symbol successfully from i915 module\n");
+	pr_debug("hda-i915: bound to i915 component\n");
+
+	return 0;
+out_master_del:
+	component_master_del(dev, &hda_component_master_ops);
+out_err:
+	pr_warn("hda-i915: failed to add component master (%d)\n", ret);
 
-	return err;
+	return ret;
 }
 
 int hda_i915_exit(struct azx *chip)
 {
-	if (get_power) {
-		symbol_put(i915_request_power_well);
-		get_power = NULL;
-	}
-	if (put_power) {
-		symbol_put(i915_release_power_well);
-		put_power = NULL;
-	}
-	if (get_cdclk) {
-		symbol_put(i915_get_cdclk_freq);
-		get_cdclk = NULL;
-	}
+	struct device *dev = &chip->pci->dev;
+
+	component_master_del(dev, &hda_component_master_ops);
 
 	return 0;
 }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index f3b5dcd..e15b30e 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1912,8 +1912,7 @@ static int azx_probe_continue(struct azx *chip)
 #ifdef CONFIG_SND_HDA_I915
 		err = hda_i915_init(chip);
 		if (err < 0) {
-			dev_err(chip->card->dev,
-				"Error request power-well from i915\n");
+			dev_err(chip->card->dev, "Error binding to i915\n");
 			goto out_free;
 		}
 		err = hda_display_power(chip, true);
diff --git a/sound/pci/hda/hda_priv.h b/sound/pci/hda/hda_priv.h
index aa484fd..e6ac4e3 100644
--- a/sound/pci/hda/hda_priv.h
+++ b/sound/pci/hda/hda_priv.h
@@ -18,6 +18,7 @@
 #include <linux/clocksource.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
+#include <drm/i915_component.h>
 
 /*
  * registers
@@ -291,6 +292,9 @@ struct azx {
 	struct pci_dev *pci;
 	int dev_index;
 
+	/* i915 component interface */
+	struct i915_audio_component audio_component;
+
 	/* chip type specific */
 	int driver_type;
 	unsigned int driver_caps;
-- 
1.8.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Imre Deak]

On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > > The current hda/i915 interface to enable/disable power wells and query
> > > > the CD clock rate is based on looking up the relevant i915 module
> > > > symbols from the hda driver. By using the component framework we can get
> > > > rid of some global state tracking in the i915 driver and pave the way to
> > > > fully decouple the two drivers: once support is added to enable/disable
> > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > > itself from the i915 component master, without the need to keep a
> > > > reference on the i915 module.
> > > > 
> > > > This also gets rid of the problem that currently the i915 driver exposes
> > > > the interface only on HSW and BDW, while it's also needed at least on
> > > > VLV/CHV.
> > > 
> > > Awesome that you're tackling this, really happy to see these hacks go.
> > > Unfortunately I think it's upside down: hda should be the component master
> > > and i915 should only register a component.
> > > 
> > > Longer story: The main reason for the component helpers is to be able to
> > > magically delay registering the main/master driver until all the
> > > components are loaded. Otherwise -EDEFER doesn't work and also the
> > > suspend/resume ordering this should result in. Master here means whatever
> > > userspace eventually sees as a device node or similar, component is
> > > anything really that this userspace interfaces needs to function
> > > correctly.
> > 
> > EDEFER doesn't solve the suspend/resume ordering, at least not in the
> > general async s/r case. In any case I don't see a problem in making the
> > hda component the master and I think it's more logical that way as you
> > said, so I changed that.
> 
> Yeah for full async s/r we're screwed as-is. But see below for some crazy
> ideas.
> > > I think what we need here is then:
> > > - Put the current azx_probe into the master_bind callback. The new
> > >   azx_probe will do nothing else than register the master component and
> > >   the component match list. To do so it checks the chip flag and if it
> > >   needs to cooperate with i915 it registers one component match for that.
> > >   The master_bind (old probe) function calls component_bind_all with the
> > >   hda_intel pointer as void *data parameter.
> > 
> > I'm not sure this is the best way since by this the i915 module would
> > need to be pinned even when no HDMI functionality is used. I think a
> > better way would be to let the probe function run as now and
> > init/register all the non-HDMI functionality. Then only the HDMI
> > functionality would be inited/registered from the bind/unbind hooks.
> 
> Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
> to be able to load the driver. But if we can defer just the hdmi part.
> 
> > But we could go either way even as a follow-up to this patchset.
> > 
> > > - i915 registers a component for the i915 gfx device. It uses the
> > >   component device to get at i915 sturctures and fills the dev+ops into
> > >   the hda_intel pointer it gets as void *data.
> > >
> > > Stuff we then should do on top:
> > > - Add deferred probing to azx_probe: Only when all components are there
> > >   should it actually register. This will take care of all the module load
> > >   order mess.
> > 
> > I agree that we should only register user interfaces when everything is
> > in place. But I'm not sure deferred probe is the best, we could do
> > without it by registering HDMI from the component bind hook.
> 
> It's mostly a question whether alsa does support delayed addition of
> interface parts. DRM most definitely does not (except for recently added
> dp mst connector hotplug). But I guess if the current driver already
> delays registering the hdmi part then we're fine. I'm not sure about
> whether it's really safe - I spotted not a lot of locking really to make
> sure there's no races between i915 loading and userspace trying to access
> the hdmi side.

Ok, I'm not sure either. If that's not possible, I agree EDEFER would be
the best and with that we could also get rid of the request_module() we
need now.

> > >   It should also take care of system suspend/resume ordering and we
> > >   should be able to delete all the early_resume/late_suspend trickery.
> > 
> > Deferred probe doesn't solve the suspend/resume ordering, we would need
> > to have a separate HDMI device that is set as a child to the i915
> > device. Alternatively we could use device_pm_wait_for_dev().
> 
> I'm not sure whether there's the possibility of deadlocks with
> device_pm_wait_for_dev. But if that works I think a component helper to
> wait for all components to resume would be really useful. Or we implement
> the full-blown pm_ops idea laid out below for components.

Yes it could deadlock with pm_async=0 (a debug thing nowadays?) and
depending on the bus address of the waiter and wait-for device. At least
for us this can't happen (with the fixed PCI addresses for the gfx and
audio devices), but I don't know if you can consider this a clean
solution. Perhaps by adding a check for this in device_pm_wait_for_dev()
and avoiding the deadlock by returning some error would be safe enough.

> > 
> > > Imo we should have things ready up to this point to make sure this
> > > refactoring actually works.
> > 
> > I think we could go with this minimal patch with your change to have the
> > hda component be master. This only adds the support for components and
> > keeps everything else the same. We could consider the bigger changes as
> > a follow-up.
> 
> Yeah that was my plan - if EDEFER isn't enough then we keep the early/late
> resume/suspend hooks for a bit longer.
> 
> > > Then there's some cool stuff we could do on top:
> > > - Register a i915-hda platform devices as a child of the gfx pci device.
> > >   Besides shuffling around a bit with the interfaces/argument casting and
> > >   the component match function this doesn't really have a functional
> > >   impact. But it makes the relationship more clear since hda doesn't
> > >   really need the entire pci device, but only the small part that does
> > >   audio.
> > > 
> > > - Replace the hand-rolled power-well interface with runtime pm on that
> > >   device node.
> > > 
> > > - If system suspend/resume doesn't work automatically with deferred
> > >   probing (tbh I'm not sure) add pm_ops to the component master. Then add
> > >   some functions as default implementations for pm_ops for components
> > >   which simply refcount all component pm_ops calls and call the master
> > >   pm_ops suspend on the first suspend call and resume on the last resume
> > >   call. That really should take care of suspend/resume ordering for good.
> > 
> > Yep, these sound good. I think having an HDMI child device is the
> > cleanest solution for the s/r ordering issue.
> 
> Ok, sounds like we have a plan. And thanks again for tackling this, I'm
> really happy to see this go away.
> 
> Cheers, Daniel


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/5] ALSA: hda: add component support

[Takashi Iwai]

At Tue, 9 Dec 2014 11:19:34 +0100,
Daniel Vetter wrote:
> 
> On Tue, Dec 09, 2014 at 11:41:18AM +0200, Imre Deak wrote:
> > Register a component master to be used to interface with the i915
> > driver. This is meant to replace the current interface which is based on
> > module symbol lookups.
> > 
> > Note that currently we keep the existing behavior and pin the i915
> > module while the hda driver is loaded. Using the component interface
> > allows us to remove this dependency once support for dynamically
> > enabling / disabling the HDMI functionality is added to the driver.
> > 
> > v2:
> > - change roles between the hda and i915 components (Daniel)
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Ok, I think this is a good foundation to build on and later on add more
> pieces to fix suspend/resume and similar.
> 
> But we also need to discuss how to merge this. My proposal is that I do a
> special topic branch with just the patches in this series and then pull
> that into drm-intel-next and send a pull request for it to Takashi for
> sound-next, too. That way we don't have any depencies between drm and
> sound for the 3.20 merge window.
> 
> Testing by intel QA is also solved this way since sound-next is already
> pulled into drm-intel-nightly. And both gfx and sound QA use that branch.
> 
> Takashi/Dave, does this sound good?

Yes, this sounds good.  Maybe it'd be good to base the branch on
3.19-rc1 so that all changes are covered?


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Takashi Iwai]

At Tue, 09 Dec 2014 18:56:07 +0200,
Imre Deak wrote:
> 
> On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> > On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> > > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > > > The current hda/i915 interface to enable/disable power wells and query
> > > > > the CD clock rate is based on looking up the relevant i915 module
> > > > > symbols from the hda driver. By using the component framework we can get
> > > > > rid of some global state tracking in the i915 driver and pave the way to
> > > > > fully decouple the two drivers: once support is added to enable/disable
> > > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > > > itself from the i915 component master, without the need to keep a
> > > > > reference on the i915 module.
> > > > > 
> > > > > This also gets rid of the problem that currently the i915 driver exposes
> > > > > the interface only on HSW and BDW, while it's also needed at least on
> > > > > VLV/CHV.
> > > > 
> > > > Awesome that you're tackling this, really happy to see these hacks go.
> > > > Unfortunately I think it's upside down: hda should be the component master
> > > > and i915 should only register a component.
> > > > 
> > > > Longer story: The main reason for the component helpers is to be able to
> > > > magically delay registering the main/master driver until all the
> > > > components are loaded. Otherwise -EDEFER doesn't work and also the
> > > > suspend/resume ordering this should result in. Master here means whatever
> > > > userspace eventually sees as a device node or similar, component is
> > > > anything really that this userspace interfaces needs to function
> > > > correctly.
> > > 
> > > EDEFER doesn't solve the suspend/resume ordering, at least not in the
> > > general async s/r case. In any case I don't see a problem in making the
> > > hda component the master and I think it's more logical that way as you
> > > said, so I changed that.
> > 
> > Yeah for full async s/r we're screwed as-is. But see below for some crazy
> > ideas.
> > > > I think what we need here is then:
> > > > - Put the current azx_probe into the master_bind callback. The new
> > > >   azx_probe will do nothing else than register the master component and
> > > >   the component match list. To do so it checks the chip flag and if it
> > > >   needs to cooperate with i915 it registers one component match for that.
> > > >   The master_bind (old probe) function calls component_bind_all with the
> > > >   hda_intel pointer as void *data parameter.
> > > 
> > > I'm not sure this is the best way since by this the i915 module would
> > > need to be pinned even when no HDMI functionality is used. I think a
> > > better way would be to let the probe function run as now and
> > > init/register all the non-HDMI functionality. Then only the HDMI
> > > functionality would be inited/registered from the bind/unbind hooks.
> > 
> > Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
> > to be able to load the driver. But if we can defer just the hdmi part.
> > 
> > > But we could go either way even as a follow-up to this patchset.
> > > 
> > > > - i915 registers a component for the i915 gfx device. It uses the
> > > >   component device to get at i915 sturctures and fills the dev+ops into
> > > >   the hda_intel pointer it gets as void *data.
> > > >
> > > > Stuff we then should do on top:
> > > > - Add deferred probing to azx_probe: Only when all components are there
> > > >   should it actually register. This will take care of all the module load
> > > >   order mess.
> > > 
> > > I agree that we should only register user interfaces when everything is
> > > in place. But I'm not sure deferred probe is the best, we could do
> > > without it by registering HDMI from the component bind hook.
> > 
> > It's mostly a question whether alsa does support delayed addition of
> > interface parts. DRM most definitely does not (except for recently added
> > dp mst connector hotplug). But I guess if the current driver already
> > delays registering the hdmi part then we're fine. I'm not sure about
> > whether it's really safe - I spotted not a lot of locking really to make
> > sure there's no races between i915 loading and userspace trying to access
> > the hdmi side.
> 
> Ok, I'm not sure either. If that's not possible, I agree EDEFER would be
> the best and with that we could also get rid of the request_module() we
> need now.

The probe of HD-audio driver is done in a work anyway, so changing the
code to sync with component should be feasible. 

I'm off today (and yesterday was a Christmas party :), so had little
time for review so far.  Will check the series tomorrow.


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: remove unused power_well/get_cdclk_freq api

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1-9              364/366              356/366
SNB                                  448/450              448/450
IVB                 -1              497/498              496/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_pipe_crc_basic_bad-nb-words-3      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_setmode_invalid-clone-exclusive-crtc      PASS(3, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-wf_vblank-vs-dpms-interruptible      PASS(4, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_render_direct-render      PASS(3, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_flip-vs-dpms-interruptible      PASS(3, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-dpms      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-modeset      PASS(3, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-panning      PASS(3, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(1, M26)PASS(8, M26M37)      PASS(1, M26)
*ILK  igt_kms_flip_wf_vblank-vs-modeset-interruptible      PASS(3, M26)      DMESG_WARN(1, M26)
*IVB  igt_kms_cursor_crc_cursor-64x64-random      PASS(3, M4M21)      DMESG_WARN(1, M21)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/5] drm/i915: add component support

[Jani Nikula]

On Tue, 09 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> Register a component to be used to interface with the snd_hda_intel
> driver. This is meant to replace the same interface that is currently
> based on module symbol lookup.
>
> v2:
> - change roles between the hda and i915 components (Daniel)
> - add the implementation to a new file (Jani)
> - use better namespacing (Jani)
> v3:
> - move the implementation to intel_audio.c (Daniel)
> - rename display_component to audio_component (Daniel)
> - add kerneldoc (Daniel)
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_component.c |  27 +++++++++

Ooops?

J.

>  drivers/gpu/drm/i915/i915_dma.c       |   4 ++
>  drivers/gpu/drm/i915/i915_drv.h       |   3 +
>  drivers/gpu/drm/i915/intel_audio.c    | 110 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h      |   2 +
>  include/drm/i915_component.h          |  38 ++++++++++++
>  6 files changed, 184 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/i915_component.c
>  create mode 100644 include/drm/i915_component.h
>
> diff --git a/drivers/gpu/drm/i915/i915_component.c b/drivers/gpu/drm/i915/i915_component.c
> new file mode 100644
> index 0000000..95dd087
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_component.c
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include <linux/component.h>
> +#include <drm/i915_component.h>
> +#include "intel_drv.h"
> +
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 887d88f..f6334d0 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -830,6 +830,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_runtime_pm_enable(dev_priv);
>  
> +	i915_audio_component_init(dev_priv);
> +
>  	return 0;
>  
>  out_power_well:
> @@ -870,6 +872,8 @@ int i915_driver_unload(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	i915_audio_component_cleanup(dev_priv);
> +
>  	ret = i915_gem_suspend(dev);
>  	if (ret) {
>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0961d7f..3c2d9c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1713,6 +1713,9 @@ struct drm_i915_private {
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> +	/* hda/i915 audio component */
> +	bool audio_component_registered;
> +
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
>  
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 2c7ed5c..ee41b88 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -22,6 +22,9 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/component.h>
> +#include <drm/i915_component.h>
> +#include "intel_drv.h"
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_edid.h>
> @@ -461,3 +464,110 @@ void intel_init_audio(struct drm_device *dev)
>  		dev_priv->display.audio_codec_disable = ilk_audio_codec_disable;
>  	}
>  }
> +
> +static void i915_audio_component_get_power(struct device *dev)
> +{
> +	intel_display_power_get(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
> +}
> +
> +static void i915_audio_component_put_power(struct device *dev)
> +{
> +	intel_display_power_put(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
> +}
> +
> +/* Get CDCLK in kHz  */
> +static int i915_audio_component_get_cdclk_freq(struct device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
> +		return -ENODEV;
> +
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> +	ret = intel_ddi_get_cdclk_freq(dev_priv);
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> +
> +	return ret;
> +}
> +
> +static const struct i915_audio_component_ops i915_audio_component_ops = {
> +	.owner		= THIS_MODULE,
> +	.get_power	= i915_audio_component_get_power,
> +	.put_power	= i915_audio_component_put_power,
> +	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> +};
> +
> +static int i915_audio_component_bind(struct device *i915_dev,
> +				     struct device *hda_dev, void *data)
> +{
> +	struct i915_audio_component *acomp = data;
> +
> +	if (WARN_ON(acomp->ops || acomp->dev))
> +		return -EEXIST;
> +
> +	acomp->ops = &i915_audio_component_ops;
> +	acomp->dev = i915_dev;
> +
> +	return 0;
> +}
> +
> +static void i915_audio_component_unbind(struct device *i915_dev,
> +					struct device *hda_dev, void *data)
> +{
> +	struct i915_audio_component *acomp = data;
> +
> +	acomp->ops = NULL;
> +	acomp->dev = NULL;
> +}
> +
> +static const struct component_ops i915_audio_component_bind_ops = {
> +	.bind	= i915_audio_component_bind,
> +	.unbind	= i915_audio_component_unbind,
> +};
> +
> +/**
> + * i915_audio_component_init - initialize and register the audio component
> + * @dev_priv: i915 device instance
> + *
> + * This will register with the component framework a child component which
> + * will bind dynamically to the snd_hda_intel driver's corresponding master
> + * component when the latter is registered. During binding the child
> + * initializes an instance of struct i915_audio_component which it receives
> + * from the master. The master can then start to use the interface defined by
> + * this struct. Each side can break the binding at any point by deregistering
> + * its own component after which each side's component unbind callback is
> + * called.
> + *
> + * We ignore any error during registration and continue with reduced
> + * functionality (i.e. without HDMI audio).
> + */
> +void i915_audio_component_init(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	ret = component_add(dev_priv->dev->dev, &i915_audio_component_bind_ops);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to add audio component (%d)\n", ret);
> +		/* continue with reduced functionality */
> +		return;
> +	}
> +
> +	dev_priv->audio_component_registered = true;
> +}
> +
> +/**
> + * i915_audio_component_cleanup - deregister the audio component
> + * @dev_priv: i915 device instance
> + *
> + * Deregisters the audio component, breaking any existing binding to the
> + * corresponding snd_hda_intel driver's master component.
> + */
> +void i915_audio_component_cleanup(struct drm_i915_private *dev_priv)
> +{
> +	if (!dev_priv->audio_component_registered)
> +		return;
> +
> +	component_del(dev_priv->dev->dev, &i915_audio_component_bind_ops);
> +	dev_priv->audio_component_registered = false;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 61a88fa..61701a6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -873,6 +873,8 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
>  void intel_init_audio(struct drm_device *dev);
>  void intel_audio_codec_enable(struct intel_encoder *encoder);
>  void intel_audio_codec_disable(struct intel_encoder *encoder);
> +void i915_audio_component_init(struct drm_i915_private *dev_priv);
> +void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
>  
>  /* intel_display.c */
>  bool intel_has_pending_fb_unpin(struct drm_device *dev);
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> new file mode 100644
> index 0000000..3e2f22e
> --- /dev/null
> +++ b/include/drm/i915_component.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef _I915_COMPONENT_H_
> +#define _I915_COMPONENT_H_
> +
> +struct i915_audio_component {
> +	struct device *dev;
> +
> +	const struct i915_audio_component_ops {
> +		struct module *owner;
> +		void (*get_power)(struct device *);
> +		void (*put_power)(struct device *);
> +		int (*get_cdclk_freq)(struct device *);
> +	} *ops;
> +};
> +
> +#endif /* _I915_COMPONENT_H_ */
> -- 
> 1.8.4
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/5] drm/i915: add component support

[Daniel Vetter]

On Tue, Dec 09, 2014 at 12:33:21PM +0200, Jani Nikula wrote:
> On Tue, 09 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Dec 09, 2014 at 11:41:17AM +0200, Imre Deak wrote:
> >> Register a component to be used to interface with the snd_hda_intel
> >> driver. This is meant to replace the same interface that is currently
> >> based on module symbol lookup.
> >> 
> >> v2:
> >> - change roles between the hda and i915 components (Daniel)
> >> - add the implementation to a new file (Jani)
> >
> > I disagree with the name here - intel_component.c is not really
> > descriptive since it's not really. Imo it makes much more sense to put
> > this into intel_audio.c. After all it's all about how we interact with the
> > audio side, which will be even more obvious once we have a dedicated
> > subdevice for this.
> 
> If we keep this component audio specific, then I guess I agree
> intel_audio.c is the better place for it. But that means anything else
> (like possibly pmic driver interaction) will need to have a component of
> its own.

I guess it depends upon how we'll structure it, but if i915 needs to
access pmic then pmic needs to expose a new platform dev/component and
i915 is a master. This won't interfere I think since it's something from
the i915 device that we expose for the audio driver.

So high-level summary of component:
- master: the main part which owns the userspace/logical device (e.g.
  drm_device, snd_dev, ...)
- component: various bits&pieces all over needed for a master, but not
  part of the main device. In DT-land that's everything since the main
  device is just a fake DT node to bundle everything up with no realation
  to real hw. In acpi we'll likely always have some real acpi or pci
  device as master.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/5] ALSA: hda: add component support

[Daniel Vetter]

On Tue, Dec 09, 2014 at 06:30:40PM +0100, Takashi Iwai wrote:
> At Tue, 9 Dec 2014 11:19:34 +0100,
> Daniel Vetter wrote:
> > 
> > On Tue, Dec 09, 2014 at 11:41:18AM +0200, Imre Deak wrote:
> > > Register a component master to be used to interface with the i915
> > > driver. This is meant to replace the current interface which is based on
> > > module symbol lookups.
> > > 
> > > Note that currently we keep the existing behavior and pin the i915
> > > module while the hda driver is loaded. Using the component interface
> > > allows us to remove this dependency once support for dynamically
> > > enabling / disabling the HDMI functionality is added to the driver.
> > > 
> > > v2:
> > > - change roles between the hda and i915 components (Daniel)
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Ok, I think this is a good foundation to build on and later on add more
> > pieces to fix suspend/resume and similar.
> > 
> > But we also need to discuss how to merge this. My proposal is that I do a
> > special topic branch with just the patches in this series and then pull
> > that into drm-intel-next and send a pull request for it to Takashi for
> > sound-next, too. That way we don't have any depencies between drm and
> > sound for the 3.20 merge window.
> > 
> > Testing by intel QA is also solved this way since sound-next is already
> > pulled into drm-intel-nightly. And both gfx and sound QA use that branch.
> > 
> > Takashi/Dave, does this sound good?
> 
> Yes, this sounds good.  Maybe it'd be good to base the branch on
> 3.19-rc1 so that all changes are covered?

Guess that depends when the patches are ready for merging and whether
there's conflicts. I have sound-next in my integration tree so will
noticed and can act accordingly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/5] drm/i915: add component support

[Imre Deak]

On Wed, 2014-12-10 at 10:22 +0200, Jani Nikula wrote:
> On Tue, 09 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> > Register a component to be used to interface with the snd_hda_intel
> > driver. This is meant to replace the same interface that is currently
> > based on module symbol lookup.
> >
> > v2:
> > - change roles between the hda and i915 components (Daniel)
> > - add the implementation to a new file (Jani)
> > - use better namespacing (Jani)
> > v3:
> > - move the implementation to intel_audio.c (Daniel)
> > - rename display_component to audio_component (Daniel)
> > - add kerneldoc (Daniel)
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_component.c |  27 +++++++++
> 
> Ooops?

Yep, missed git rm:/

> 
> J.
> 
> >  drivers/gpu/drm/i915/i915_dma.c       |   4 ++
> >  drivers/gpu/drm/i915/i915_drv.h       |   3 +
> >  drivers/gpu/drm/i915/intel_audio.c    | 110 ++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h      |   2 +
> >  include/drm/i915_component.h          |  38 ++++++++++++
> >  6 files changed, 184 insertions(+)
> >  create mode 100644 drivers/gpu/drm/i915/i915_component.c
> >  create mode 100644 include/drm/i915_component.h
> >
> > diff --git a/drivers/gpu/drm/i915/i915_component.c b/drivers/gpu/drm/i915/i915_component.c
> > new file mode 100644
> > index 0000000..95dd087
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/i915_component.c
> > @@ -0,0 +1,27 @@
> > +/*
> > + * Copyright © 2014 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include <linux/component.h>
> > +#include <drm/i915_component.h>
> > +#include "intel_drv.h"
> > +
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 887d88f..f6334d0 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -830,6 +830,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  
> >  	intel_runtime_pm_enable(dev_priv);
> >  
> > +	i915_audio_component_init(dev_priv);
> > +
> >  	return 0;
> >  
> >  out_power_well:
> > @@ -870,6 +872,8 @@ int i915_driver_unload(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > +	i915_audio_component_cleanup(dev_priv);
> > +
> >  	ret = i915_gem_suspend(dev);
> >  	if (ret) {
> >  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 0961d7f..3c2d9c7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1713,6 +1713,9 @@ struct drm_i915_private {
> >  	struct drm_property *broadcast_rgb_property;
> >  	struct drm_property *force_audio_property;
> >  
> > +	/* hda/i915 audio component */
> > +	bool audio_component_registered;
> > +
> >  	uint32_t hw_context_size;
> >  	struct list_head context_list;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 2c7ed5c..ee41b88 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -22,6 +22,9 @@
> >   */
> >  
> >  #include <linux/kernel.h>
> > +#include <linux/component.h>
> > +#include <drm/i915_component.h>
> > +#include "intel_drv.h"
> >  
> >  #include <drm/drmP.h>
> >  #include <drm/drm_edid.h>
> > @@ -461,3 +464,110 @@ void intel_init_audio(struct drm_device *dev)
> >  		dev_priv->display.audio_codec_disable = ilk_audio_codec_disable;
> >  	}
> >  }
> > +
> > +static void i915_audio_component_get_power(struct device *dev)
> > +{
> > +	intel_display_power_get(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
> > +}
> > +
> > +static void i915_audio_component_put_power(struct device *dev)
> > +{
> > +	intel_display_power_put(dev_to_i915(dev), POWER_DOMAIN_AUDIO);
> > +}
> > +
> > +/* Get CDCLK in kHz  */
> > +static int i915_audio_component_get_cdclk_freq(struct device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > +	int ret;
> > +
> > +	if (WARN_ON_ONCE(!HAS_DDI(dev_priv)))
> > +		return -ENODEV;
> > +
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> > +	ret = intel_ddi_get_cdclk_freq(dev_priv);
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct i915_audio_component_ops i915_audio_component_ops = {
> > +	.owner		= THIS_MODULE,
> > +	.get_power	= i915_audio_component_get_power,
> > +	.put_power	= i915_audio_component_put_power,
> > +	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> > +};
> > +
> > +static int i915_audio_component_bind(struct device *i915_dev,
> > +				     struct device *hda_dev, void *data)
> > +{
> > +	struct i915_audio_component *acomp = data;
> > +
> > +	if (WARN_ON(acomp->ops || acomp->dev))
> > +		return -EEXIST;
> > +
> > +	acomp->ops = &i915_audio_component_ops;
> > +	acomp->dev = i915_dev;
> > +
> > +	return 0;
> > +}
> > +
> > +static void i915_audio_component_unbind(struct device *i915_dev,
> > +					struct device *hda_dev, void *data)
> > +{
> > +	struct i915_audio_component *acomp = data;
> > +
> > +	acomp->ops = NULL;
> > +	acomp->dev = NULL;
> > +}
> > +
> > +static const struct component_ops i915_audio_component_bind_ops = {
> > +	.bind	= i915_audio_component_bind,
> > +	.unbind	= i915_audio_component_unbind,
> > +};
> > +
> > +/**
> > + * i915_audio_component_init - initialize and register the audio component
> > + * @dev_priv: i915 device instance
> > + *
> > + * This will register with the component framework a child component which
> > + * will bind dynamically to the snd_hda_intel driver's corresponding master
> > + * component when the latter is registered. During binding the child
> > + * initializes an instance of struct i915_audio_component which it receives
> > + * from the master. The master can then start to use the interface defined by
> > + * this struct. Each side can break the binding at any point by deregistering
> > + * its own component after which each side's component unbind callback is
> > + * called.
> > + *
> > + * We ignore any error during registration and continue with reduced
> > + * functionality (i.e. without HDMI audio).
> > + */
> > +void i915_audio_component_init(struct drm_i915_private *dev_priv)
> > +{
> > +	int ret;
> > +
> > +	ret = component_add(dev_priv->dev->dev, &i915_audio_component_bind_ops);
> > +	if (ret < 0) {
> > +		DRM_ERROR("failed to add audio component (%d)\n", ret);
> > +		/* continue with reduced functionality */
> > +		return;
> > +	}
> > +
> > +	dev_priv->audio_component_registered = true;
> > +}
> > +
> > +/**
> > + * i915_audio_component_cleanup - deregister the audio component
> > + * @dev_priv: i915 device instance
> > + *
> > + * Deregisters the audio component, breaking any existing binding to the
> > + * corresponding snd_hda_intel driver's master component.
> > + */
> > +void i915_audio_component_cleanup(struct drm_i915_private *dev_priv)
> > +{
> > +	if (!dev_priv->audio_component_registered)
> > +		return;
> > +
> > +	component_del(dev_priv->dev->dev, &i915_audio_component_bind_ops);
> > +	dev_priv->audio_component_registered = false;
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 61a88fa..61701a6 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -873,6 +873,8 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
> >  void intel_init_audio(struct drm_device *dev);
> >  void intel_audio_codec_enable(struct intel_encoder *encoder);
> >  void intel_audio_codec_disable(struct intel_encoder *encoder);
> > +void i915_audio_component_init(struct drm_i915_private *dev_priv);
> > +void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
> >  
> >  /* intel_display.c */
> >  bool intel_has_pending_fb_unpin(struct drm_device *dev);
> > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > new file mode 100644
> > index 0000000..3e2f22e
> > --- /dev/null
> > +++ b/include/drm/i915_component.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * Copyright © 2014 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef _I915_COMPONENT_H_
> > +#define _I915_COMPONENT_H_
> > +
> > +struct i915_audio_component {
> > +	struct device *dev;
> > +
> > +	const struct i915_audio_component_ops {
> > +		struct module *owner;
> > +		void (*get_power)(struct device *);
> > +		void (*put_power)(struct device *);
> > +		int (*get_cdclk_freq)(struct device *);
> > +	} *ops;
> > +};
> > +
> > +#endif /* _I915_COMPONENT_H_ */
> > -- 
> > 1.8.4
> >
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Imre Deak]

Hi Mengdong, Takashi,

On Tue, 2014-12-09 at 18:33 +0100, Takashi Iwai wrote:
> At Tue, 09 Dec 2014 18:56:07 +0200,
> Imre Deak wrote:
> > 
> > On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> > > On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> > > > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > > > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > > > > The current hda/i915 interface to enable/disable power wells and query
> > > > > > the CD clock rate is based on looking up the relevant i915 module
> > > > > > symbols from the hda driver. By using the component framework we can get
> > > > > > rid of some global state tracking in the i915 driver and pave the way to
> > > > > > fully decouple the two drivers: once support is added to enable/disable
> > > > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > > > > itself from the i915 component master, without the need to keep a
> > > > > > reference on the i915 module.
> > > > > > 
> > > > > > This also gets rid of the problem that currently the i915 driver exposes
> > > > > > the interface only on HSW and BDW, while it's also needed at least on
> > > > > > VLV/CHV.
> > > > > 
> > > > > Awesome that you're tackling this, really happy to see these hacks go.
> > > > > Unfortunately I think it's upside down: hda should be the component master
> > > > > and i915 should only register a component.
> > > > > 
> > > > > Longer story: The main reason for the component helpers is to be able to
> > > > > magically delay registering the main/master driver until all the
> > > > > components are loaded. Otherwise -EDEFER doesn't work and also the
> > > > > suspend/resume ordering this should result in. Master here means whatever
> > > > > userspace eventually sees as a device node or similar, component is
> > > > > anything really that this userspace interfaces needs to function
> > > > > correctly.
> > > > 
> > > > EDEFER doesn't solve the suspend/resume ordering, at least not in the
> > > > general async s/r case. In any case I don't see a problem in making the
> > > > hda component the master and I think it's more logical that way as you
> > > > said, so I changed that.
> > > 
> > > Yeah for full async s/r we're screwed as-is. But see below for some crazy
> > > ideas.
> > > > > I think what we need here is then:
> > > > > - Put the current azx_probe into the master_bind callback. The new
> > > > >   azx_probe will do nothing else than register the master component and
> > > > >   the component match list. To do so it checks the chip flag and if it
> > > > >   needs to cooperate with i915 it registers one component match for that.
> > > > >   The master_bind (old probe) function calls component_bind_all with the
> > > > >   hda_intel pointer as void *data parameter.
> > > > 
> > > > I'm not sure this is the best way since by this the i915 module would
> > > > need to be pinned even when no HDMI functionality is used. I think a
> > > > better way would be to let the probe function run as now and
> > > > init/register all the non-HDMI functionality. Then only the HDMI
> > > > functionality would be inited/registered from the bind/unbind hooks.
> > > 
> > > Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
> > > to be able to load the driver. But if we can defer just the hdmi part.
> > > 
> > > > But we could go either way even as a follow-up to this patchset.
> > > > 
> > > > > - i915 registers a component for the i915 gfx device. It uses the
> > > > >   component device to get at i915 sturctures and fills the dev+ops into
> > > > >   the hda_intel pointer it gets as void *data.
> > > > >
> > > > > Stuff we then should do on top:
> > > > > - Add deferred probing to azx_probe: Only when all components are there
> > > > >   should it actually register. This will take care of all the module load
> > > > >   order mess.
> > > > 
> > > > I agree that we should only register user interfaces when everything is
> > > > in place. But I'm not sure deferred probe is the best, we could do
> > > > without it by registering HDMI from the component bind hook.
> > > 
> > > It's mostly a question whether alsa does support delayed addition of
> > > interface parts. DRM most definitely does not (except for recently added
> > > dp mst connector hotplug). But I guess if the current driver already
> > > delays registering the hdmi part then we're fine. I'm not sure about
> > > whether it's really safe - I spotted not a lot of locking really to make
> > > sure there's no races between i915 loading and userspace trying to access
> > > the hdmi side.
> > 
> > Ok, I'm not sure either. If that's not possible, I agree EDEFER would be
> > the best and with that we could also get rid of the request_module() we
> > need now.
> 
> The probe of HD-audio driver is done in a work anyway, so changing the
> code to sync with component should be feasible. 
> 
> I'm off today (and yesterday was a Christmas party :), so had little
> time for review so far.  Will check the series tomorrow.

could one of you help reviewing this patchset? Note that there is a v2
version with some individual patches having a v3 already.

Thanks,
Imre

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Takashi Iwai]

At Mon, 05 Jan 2015 17:29:34 +0200,
Imre Deak wrote:
> 
> Hi Mengdong, Takashi,
> 
> On Tue, 2014-12-09 at 18:33 +0100, Takashi Iwai wrote:
> > At Tue, 09 Dec 2014 18:56:07 +0200,
> > Imre Deak wrote:
> > > 
> > > On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> > > > On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> > > > > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > > > > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > > > > > The current hda/i915 interface to enable/disable power wells and query
> > > > > > > the CD clock rate is based on looking up the relevant i915 module
> > > > > > > symbols from the hda driver. By using the component framework we can get
> > > > > > > rid of some global state tracking in the i915 driver and pave the way to
> > > > > > > fully decouple the two drivers: once support is added to enable/disable
> > > > > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > > > > > itself from the i915 component master, without the need to keep a
> > > > > > > reference on the i915 module.
> > > > > > > 
> > > > > > > This also gets rid of the problem that currently the i915 driver exposes
> > > > > > > the interface only on HSW and BDW, while it's also needed at least on
> > > > > > > VLV/CHV.
> > > > > > 
> > > > > > Awesome that you're tackling this, really happy to see these hacks go.
> > > > > > Unfortunately I think it's upside down: hda should be the component master
> > > > > > and i915 should only register a component.
> > > > > > 
> > > > > > Longer story: The main reason for the component helpers is to be able to
> > > > > > magically delay registering the main/master driver until all the
> > > > > > components are loaded. Otherwise -EDEFER doesn't work and also the
> > > > > > suspend/resume ordering this should result in. Master here means whatever
> > > > > > userspace eventually sees as a device node or similar, component is
> > > > > > anything really that this userspace interfaces needs to function
> > > > > > correctly.
> > > > > 
> > > > > EDEFER doesn't solve the suspend/resume ordering, at least not in the
> > > > > general async s/r case. In any case I don't see a problem in making the
> > > > > hda component the master and I think it's more logical that way as you
> > > > > said, so I changed that.
> > > > 
> > > > Yeah for full async s/r we're screwed as-is. But see below for some crazy
> > > > ideas.
> > > > > > I think what we need here is then:
> > > > > > - Put the current azx_probe into the master_bind callback. The new
> > > > > >   azx_probe will do nothing else than register the master component and
> > > > > >   the component match list. To do so it checks the chip flag and if it
> > > > > >   needs to cooperate with i915 it registers one component match for that.
> > > > > >   The master_bind (old probe) function calls component_bind_all with the
> > > > > >   hda_intel pointer as void *data parameter.
> > > > > 
> > > > > I'm not sure this is the best way since by this the i915 module would
> > > > > need to be pinned even when no HDMI functionality is used. I think a
> > > > > better way would be to let the probe function run as now and
> > > > > init/register all the non-HDMI functionality. Then only the HDMI
> > > > > functionality would be inited/registered from the bind/unbind hooks.
> > > > 
> > > > Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
> > > > to be able to load the driver. But if we can defer just the hdmi part.
> > > > 
> > > > > But we could go either way even as a follow-up to this patchset.
> > > > > 
> > > > > > - i915 registers a component for the i915 gfx device. It uses the
> > > > > >   component device to get at i915 sturctures and fills the dev+ops into
> > > > > >   the hda_intel pointer it gets as void *data.
> > > > > >
> > > > > > Stuff we then should do on top:
> > > > > > - Add deferred probing to azx_probe: Only when all components are there
> > > > > >   should it actually register. This will take care of all the module load
> > > > > >   order mess.
> > > > > 
> > > > > I agree that we should only register user interfaces when everything is
> > > > > in place. But I'm not sure deferred probe is the best, we could do
> > > > > without it by registering HDMI from the component bind hook.
> > > > 
> > > > It's mostly a question whether alsa does support delayed addition of
> > > > interface parts. DRM most definitely does not (except for recently added
> > > > dp mst connector hotplug). But I guess if the current driver already
> > > > delays registering the hdmi part then we're fine. I'm not sure about
> > > > whether it's really safe - I spotted not a lot of locking really to make
> > > > sure there's no races between i915 loading and userspace trying to access
> > > > the hdmi side.
> > > 
> > > Ok, I'm not sure either. If that's not possible, I agree EDEFER would be
> > > the best and with that we could also get rid of the request_module() we
> > > need now.
> > 
> > The probe of HD-audio driver is done in a work anyway, so changing the
> > code to sync with component should be feasible. 
> > 
> > I'm off today (and yesterday was a Christmas party :), so had little
> > time for review so far.  Will check the series tomorrow.
> 
> could one of you help reviewing this patchset? Note that there is a v2
> version with some individual patches having a v3 already.

Is there any git branch containing the latest patches that is easily
applicable to 3.19-rc (or linux-next)?


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Imre Deak]

On Mon, 2015-01-05 at 16:35 +0100, Takashi Iwai wrote:
> At Mon, 05 Jan 2015 17:29:34 +0200,
> Imre Deak wrote:
> > 
> > Hi Mengdong, Takashi,
> > 
> > On Tue, 2014-12-09 at 18:33 +0100, Takashi Iwai wrote:
> > > At Tue, 09 Dec 2014 18:56:07 +0200,
> > > Imre Deak wrote:
> > > > 
> > > > On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> > > > > On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> > > > > > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > > > > > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > > > > > > The current hda/i915 interface to enable/disable power wells and query
> > > > > > > > the CD clock rate is based on looking up the relevant i915 module
> > > > > > > > symbols from the hda driver. By using the component framework we can get
> > > > > > > > rid of some global state tracking in the i915 driver and pave the way to
> > > > > > > > fully decouple the two drivers: once support is added to enable/disable
> > > > > > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > > > > > > itself from the i915 component master, without the need to keep a
> > > > > > > > reference on the i915 module.
> > > > > > > > 
> > > > > > > > This also gets rid of the problem that currently the i915 driver exposes
> > > > > > > > the interface only on HSW and BDW, while it's also needed at least on
> > > > > > > > VLV/CHV.
> > > > > > > 
> > > > > > > Awesome that you're tackling this, really happy to see these hacks go.
> > > > > > > Unfortunately I think it's upside down: hda should be the component master
> > > > > > > and i915 should only register a component.
> > > > > > > 
> > > > > > > Longer story: The main reason for the component helpers is to be able to
> > > > > > > magically delay registering the main/master driver until all the
> > > > > > > components are loaded. Otherwise -EDEFER doesn't work and also the
> > > > > > > suspend/resume ordering this should result in. Master here means whatever
> > > > > > > userspace eventually sees as a device node or similar, component is
> > > > > > > anything really that this userspace interfaces needs to function
> > > > > > > correctly.
> > > > > > 
> > > > > > EDEFER doesn't solve the suspend/resume ordering, at least not in the
> > > > > > general async s/r case. In any case I don't see a problem in making the
> > > > > > hda component the master and I think it's more logical that way as you
> > > > > > said, so I changed that.
> > > > > 
> > > > > Yeah for full async s/r we're screwed as-is. But see below for some crazy
> > > > > ideas.
> > > > > > > I think what we need here is then:
> > > > > > > - Put the current azx_probe into the master_bind callback. The new
> > > > > > >   azx_probe will do nothing else than register the master component and
> > > > > > >   the component match list. To do so it checks the chip flag and if it
> > > > > > >   needs to cooperate with i915 it registers one component match for that.
> > > > > > >   The master_bind (old probe) function calls component_bind_all with the
> > > > > > >   hda_intel pointer as void *data parameter.
> > > > > > 
> > > > > > I'm not sure this is the best way since by this the i915 module would
> > > > > > need to be pinned even when no HDMI functionality is used. I think a
> > > > > > better way would be to let the probe function run as now and
> > > > > > init/register all the non-HDMI functionality. Then only the HDMI
> > > > > > functionality would be inited/registered from the bind/unbind hooks.
> > > > > 
> > > > > Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
> > > > > to be able to load the driver. But if we can defer just the hdmi part.
> > > > > 
> > > > > > But we could go either way even as a follow-up to this patchset.
> > > > > > 
> > > > > > > - i915 registers a component for the i915 gfx device. It uses the
> > > > > > >   component device to get at i915 sturctures and fills the dev+ops into
> > > > > > >   the hda_intel pointer it gets as void *data.
> > > > > > >
> > > > > > > Stuff we then should do on top:
> > > > > > > - Add deferred probing to azx_probe: Only when all components are there
> > > > > > >   should it actually register. This will take care of all the module load
> > > > > > >   order mess.
> > > > > > 
> > > > > > I agree that we should only register user interfaces when everything is
> > > > > > in place. But I'm not sure deferred probe is the best, we could do
> > > > > > without it by registering HDMI from the component bind hook.
> > > > > 
> > > > > It's mostly a question whether alsa does support delayed addition of
> > > > > interface parts. DRM most definitely does not (except for recently added
> > > > > dp mst connector hotplug). But I guess if the current driver already
> > > > > delays registering the hdmi part then we're fine. I'm not sure about
> > > > > whether it's really safe - I spotted not a lot of locking really to make
> > > > > sure there's no races between i915 loading and userspace trying to access
> > > > > the hdmi side.
> > > > 
> > > > Ok, I'm not sure either. If that's not possible, I agree EDEFER would be
> > > > the best and with that we could also get rid of the request_module() we
> > > > need now.
> > > 
> > > The probe of HD-audio driver is done in a work anyway, so changing the
> > > code to sync with component should be feasible. 
> > > 
> > > I'm off today (and yesterday was a Christmas party :), so had little
> > > time for review so far.  Will check the series tomorrow.
> > 
> > could one of you help reviewing this patchset? Note that there is a v2
> > version with some individual patches having a v3 already.
> 
> Is there any git branch containing the latest patches that is easily
> applicable to 3.19-rc (or linux-next)?

I applied them on 3.19-rc2 and pushed the branch to:
https://github.com/ideak/linux/commits/audio-component

--Imre


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Takashi Iwai]

At Mon, 05 Jan 2015 19:25:09 +0200,
Imre Deak wrote:
> 
> On Mon, 2015-01-05 at 16:35 +0100, Takashi Iwai wrote:
> > At Mon, 05 Jan 2015 17:29:34 +0200,
> > Imre Deak wrote:
> > > 
> > > Hi Mengdong, Takashi,
> > > 
> > > On Tue, 2014-12-09 at 18:33 +0100, Takashi Iwai wrote:
> > > > At Tue, 09 Dec 2014 18:56:07 +0200,
> > > > Imre Deak wrote:
> > > > > 
> > > > > On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> > > > > > On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> > > > > > > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > > > > > > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > > > > > > > The current hda/i915 interface to enable/disable power wells and query
> > > > > > > > > the CD clock rate is based on looking up the relevant i915 module
> > > > > > > > > symbols from the hda driver. By using the component framework we can get
> > > > > > > > > rid of some global state tracking in the i915 driver and pave the way to
> > > > > > > > > fully decouple the two drivers: once support is added to enable/disable
> > > > > > > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > > > > > > > itself from the i915 component master, without the need to keep a
> > > > > > > > > reference on the i915 module.
> > > > > > > > > 
> > > > > > > > > This also gets rid of the problem that currently the i915 driver exposes
> > > > > > > > > the interface only on HSW and BDW, while it's also needed at least on
> > > > > > > > > VLV/CHV.
> > > > > > > > 
> > > > > > > > Awesome that you're tackling this, really happy to see these hacks go.
> > > > > > > > Unfortunately I think it's upside down: hda should be the component master
> > > > > > > > and i915 should only register a component.
> > > > > > > > 
> > > > > > > > Longer story: The main reason for the component helpers is to be able to
> > > > > > > > magically delay registering the main/master driver until all the
> > > > > > > > components are loaded. Otherwise -EDEFER doesn't work and also the
> > > > > > > > suspend/resume ordering this should result in. Master here means whatever
> > > > > > > > userspace eventually sees as a device node or similar, component is
> > > > > > > > anything really that this userspace interfaces needs to function
> > > > > > > > correctly.
> > > > > > > 
> > > > > > > EDEFER doesn't solve the suspend/resume ordering, at least not in the
> > > > > > > general async s/r case. In any case I don't see a problem in making the
> > > > > > > hda component the master and I think it's more logical that way as you
> > > > > > > said, so I changed that.
> > > > > > 
> > > > > > Yeah for full async s/r we're screwed as-is. But see below for some crazy
> > > > > > ideas.
> > > > > > > > I think what we need here is then:
> > > > > > > > - Put the current azx_probe into the master_bind callback. The new
> > > > > > > >   azx_probe will do nothing else than register the master component and
> > > > > > > >   the component match list. To do so it checks the chip flag and if it
> > > > > > > >   needs to cooperate with i915 it registers one component match for that.
> > > > > > > >   The master_bind (old probe) function calls component_bind_all with the
> > > > > > > >   hda_intel pointer as void *data parameter.
> > > > > > > 
> > > > > > > I'm not sure this is the best way since by this the i915 module would
> > > > > > > need to be pinned even when no HDMI functionality is used. I think a
> > > > > > > better way would be to let the probe function run as now and
> > > > > > > init/register all the non-HDMI functionality. Then only the HDMI
> > > > > > > functionality would be inited/registered from the bind/unbind hooks.
> > > > > > 
> > > > > > Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
> > > > > > to be able to load the driver. But if we can defer just the hdmi part.
> > > > > > 
> > > > > > > But we could go either way even as a follow-up to this patchset.
> > > > > > > 
> > > > > > > > - i915 registers a component for the i915 gfx device. It uses the
> > > > > > > >   component device to get at i915 sturctures and fills the dev+ops into
> > > > > > > >   the hda_intel pointer it gets as void *data.
> > > > > > > >
> > > > > > > > Stuff we then should do on top:
> > > > > > > > - Add deferred probing to azx_probe: Only when all components are there
> > > > > > > >   should it actually register. This will take care of all the module load
> > > > > > > >   order mess.
> > > > > > > 
> > > > > > > I agree that we should only register user interfaces when everything is
> > > > > > > in place. But I'm not sure deferred probe is the best, we could do
> > > > > > > without it by registering HDMI from the component bind hook.
> > > > > > 
> > > > > > It's mostly a question whether alsa does support delayed addition of
> > > > > > interface parts. DRM most definitely does not (except for recently added
> > > > > > dp mst connector hotplug). But I guess if the current driver already
> > > > > > delays registering the hdmi part then we're fine. I'm not sure about
> > > > > > whether it's really safe - I spotted not a lot of locking really to make
> > > > > > sure there's no races between i915 loading and userspace trying to access
> > > > > > the hdmi side.
> > > > > 
> > > > > Ok, I'm not sure either. If that's not possible, I agree EDEFER would be
> > > > > the best and with that we could also get rid of the request_module() we
> > > > > need now.
> > > > 
> > > > The probe of HD-audio driver is done in a work anyway, so changing the
> > > > code to sync with component should be feasible. 
> > > > 
> > > > I'm off today (and yesterday was a Christmas party :), so had little
> > > > time for review so far.  Will check the series tomorrow.
> > > 
> > > could one of you help reviewing this patchset? Note that there is a v2
> > > version with some individual patches having a v3 already.
> > 
> > Is there any git branch containing the latest patches that is easily
> > applicable to 3.19-rc (or linux-next)?
> 
> I applied them on 3.19-rc2 and pushed the branch to:
> https://github.com/ideak/linux/commits/audio-component

Thanks!  Just reading through it (but couldn't build/test it as today
is again a holiday here :)  The patches looks good but a few
nitpicking:

- audio_component shouldn't be added into struct azx;
  struct azx is a common object used by Tegra HD-audio, too, so keep
  it clean.  There is struct hda_intel defined in hda_intel.c, which
  inherits struct azx.

  That is, we need to cut struct hda_intel definition out to a header
  file (e.g. hda_intel.h) and include it in hda_i915.c.  I think we
  can merge the contents of hda_i915.h into hda_intel.h, too.

- The removal of include/drm/i915_powerwell.h should be folded into
  the last patch, so that we can separate the changes in sound/* and
  drm/i915/* in each patch.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Imre Deak]

On Tue, 2015-01-06 at 11:25 +0100, Takashi Iwai wrote:
> At Mon, 05 Jan 2015 19:25:09 +0200,
> Imre Deak wrote:
> > 
> > On Mon, 2015-01-05 at 16:35 +0100, Takashi Iwai wrote:
> > > At Mon, 05 Jan 2015 17:29:34 +0200,
> > > Imre Deak wrote:
> > > > 
> > > > Hi Mengdong, Takashi,
> > > > 
> > > > On Tue, 2014-12-09 at 18:33 +0100, Takashi Iwai wrote:
> > > > > At Tue, 09 Dec 2014 18:56:07 +0200,
> > > > > Imre Deak wrote:
> > > > > > 
> > > > > > On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> > > > > > > On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> > > > > > > > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > > > > > > > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > > > > > > > > The current hda/i915 interface to enable/disable power wells and query
> > > > > > > > > > the CD clock rate is based on looking up the relevant i915 module
> > > > > > > > > > symbols from the hda driver. By using the component framework we can get
> > > > > > > > > > rid of some global state tracking in the i915 driver and pave the way to
> > > > > > > > > > fully decouple the two drivers: once support is added to enable/disable
> > > > > > > > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > > > > > > > > itself from the i915 component master, without the need to keep a
> > > > > > > > > > reference on the i915 module.
> > > > > > > > > > 
> > > > > > > > > > This also gets rid of the problem that currently the i915 driver exposes
> > > > > > > > > > the interface only on HSW and BDW, while it's also needed at least on
> > > > > > > > > > VLV/CHV.
> > > > > > > > > 
> > > > > > > > > Awesome that you're tackling this, really happy to see these hacks go.
> > > > > > > > > Unfortunately I think it's upside down: hda should be the component master
> > > > > > > > > and i915 should only register a component.
> > > > > > > > > 
> > > > > > > > > Longer story: The main reason for the component helpers is to be able to
> > > > > > > > > magically delay registering the main/master driver until all the
> > > > > > > > > components are loaded. Otherwise -EDEFER doesn't work and also the
> > > > > > > > > suspend/resume ordering this should result in. Master here means whatever
> > > > > > > > > userspace eventually sees as a device node or similar, component is
> > > > > > > > > anything really that this userspace interfaces needs to function
> > > > > > > > > correctly.
> > > > > > > > 
> > > > > > > > EDEFER doesn't solve the suspend/resume ordering, at least not in the
> > > > > > > > general async s/r case. In any case I don't see a problem in making the
> > > > > > > > hda component the master and I think it's more logical that way as you
> > > > > > > > said, so I changed that.
> > > > > > > 
> > > > > > > Yeah for full async s/r we're screwed as-is. But see below for some crazy
> > > > > > > ideas.
> > > > > > > > > I think what we need here is then:
> > > > > > > > > - Put the current azx_probe into the master_bind callback. The new
> > > > > > > > >   azx_probe will do nothing else than register the master component and
> > > > > > > > >   the component match list. To do so it checks the chip flag and if it
> > > > > > > > >   needs to cooperate with i915 it registers one component match for that.
> > > > > > > > >   The master_bind (old probe) function calls component_bind_all with the
> > > > > > > > >   hda_intel pointer as void *data parameter.
> > > > > > > > 
> > > > > > > > I'm not sure this is the best way since by this the i915 module would
> > > > > > > > need to be pinned even when no HDMI functionality is used. I think a
> > > > > > > > better way would be to let the probe function run as now and
> > > > > > > > init/register all the non-HDMI functionality. Then only the HDMI
> > > > > > > > functionality would be inited/registered from the bind/unbind hooks.
> > > > > > > 
> > > > > > > Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
> > > > > > > to be able to load the driver. But if we can defer just the hdmi part.
> > > > > > > 
> > > > > > > > But we could go either way even as a follow-up to this patchset.
> > > > > > > > 
> > > > > > > > > - i915 registers a component for the i915 gfx device. It uses the
> > > > > > > > >   component device to get at i915 sturctures and fills the dev+ops into
> > > > > > > > >   the hda_intel pointer it gets as void *data.
> > > > > > > > >
> > > > > > > > > Stuff we then should do on top:
> > > > > > > > > - Add deferred probing to azx_probe: Only when all components are there
> > > > > > > > >   should it actually register. This will take care of all the module load
> > > > > > > > >   order mess.
> > > > > > > > 
> > > > > > > > I agree that we should only register user interfaces when everything is
> > > > > > > > in place. But I'm not sure deferred probe is the best, we could do
> > > > > > > > without it by registering HDMI from the component bind hook.
> > > > > > > 
> > > > > > > It's mostly a question whether alsa does support delayed addition of
> > > > > > > interface parts. DRM most definitely does not (except for recently added
> > > > > > > dp mst connector hotplug). But I guess if the current driver already
> > > > > > > delays registering the hdmi part then we're fine. I'm not sure about
> > > > > > > whether it's really safe - I spotted not a lot of locking really to make
> > > > > > > sure there's no races between i915 loading and userspace trying to access
> > > > > > > the hdmi side.
> > > > > > 
> > > > > > Ok, I'm not sure either. If that's not possible, I agree EDEFER would be
> > > > > > the best and with that we could also get rid of the request_module() we
> > > > > > need now.
> > > > > 
> > > > > The probe of HD-audio driver is done in a work anyway, so changing the
> > > > > code to sync with component should be feasible. 
> > > > > 
> > > > > I'm off today (and yesterday was a Christmas party :), so had little
> > > > > time for review so far.  Will check the series tomorrow.
> > > > 
> > > > could one of you help reviewing this patchset? Note that there is a v2
> > > > version with some individual patches having a v3 already.
> > > 
> > > Is there any git branch containing the latest patches that is easily
> > > applicable to 3.19-rc (or linux-next)?
> > 
> > I applied them on 3.19-rc2 and pushed the branch to:
> > https://github.com/ideak/linux/commits/audio-component
> 
> Thanks!  Just reading through it (but couldn't build/test it as today
> is again a holiday here :)  The patches looks good but a few
> nitpicking:
> 
> - audio_component shouldn't be added into struct azx;
>   struct azx is a common object used by Tegra HD-audio, too, so keep
>   it clean.  There is struct hda_intel defined in hda_intel.c, which
>   inherits struct azx.
> 
>   That is, we need to cut struct hda_intel definition out to a header
>   file (e.g. hda_intel.h) and include it in hda_i915.c.  I think we
>   can merge the contents of hda_i915.h into hda_intel.h, too.

Ok, I did the above and also changed the hda_i915 interface to accept
hda_intel instead of chip as that makes now more sense.

> - The removal of include/drm/i915_powerwell.h should be folded into
>   the last patch, so that we can separate the changes in sound/* and
>   drm/i915/* in each patch.

Oops, that would've also caused a bisect error. I moved the removal to
the last patch.

I tested this on HSW, seems to work ok. I updated the github branch, if
you don't see more issues, I will post the new version to the list.

Thanks,
Imre


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] sanitize hda/i915 interface using the component fw

[Takashi Iwai]

At Wed, 07 Jan 2015 21:49:46 +0200,
Imre Deak wrote:
> 
> On Tue, 2015-01-06 at 11:25 +0100, Takashi Iwai wrote:
> > At Mon, 05 Jan 2015 19:25:09 +0200,
> > Imre Deak wrote:
> > > 
> > > On Mon, 2015-01-05 at 16:35 +0100, Takashi Iwai wrote:
> > > > At Mon, 05 Jan 2015 17:29:34 +0200,
> > > > Imre Deak wrote:
> > > > > 
> > > > > Hi Mengdong, Takashi,
> > > > > 
> > > > > On Tue, 2014-12-09 at 18:33 +0100, Takashi Iwai wrote:
> > > > > > At Tue, 09 Dec 2014 18:56:07 +0200,
> > > > > > Imre Deak wrote:
> > > > > > > 
> > > > > > > On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
> > > > > > > > On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
> > > > > > > > > On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
> > > > > > > > > > On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
> > > > > > > > > > > The current hda/i915 interface to enable/disable power wells and query
> > > > > > > > > > > the CD clock rate is based on looking up the relevant i915 module
> > > > > > > > > > > symbols from the hda driver. By using the component framework we can get
> > > > > > > > > > > rid of some global state tracking in the i915 driver and pave the way to
> > > > > > > > > > > fully decouple the two drivers: once support is added to enable/disable
> > > > > > > > > > > the HDMI functionality dynamically in the hda driver, it can bind/unbind
> > > > > > > > > > > itself from the i915 component master, without the need to keep a
> > > > > > > > > > > reference on the i915 module.
> > > > > > > > > > > 
> > > > > > > > > > > This also gets rid of the problem that currently the i915 driver exposes
> > > > > > > > > > > the interface only on HSW and BDW, while it's also needed at least on
> > > > > > > > > > > VLV/CHV.
> > > > > > > > > > 
> > > > > > > > > > Awesome that you're tackling this, really happy to see these hacks go.
> > > > > > > > > > Unfortunately I think it's upside down: hda should be the component master
> > > > > > > > > > and i915 should only register a component.
> > > > > > > > > > 
> > > > > > > > > > Longer story: The main reason for the component helpers is to be able to
> > > > > > > > > > magically delay registering the main/master driver until all the
> > > > > > > > > > components are loaded. Otherwise -EDEFER doesn't work and also the
> > > > > > > > > > suspend/resume ordering this should result in. Master here means whatever
> > > > > > > > > > userspace eventually sees as a device node or similar, component is
> > > > > > > > > > anything really that this userspace interfaces needs to function
> > > > > > > > > > correctly.
> > > > > > > > > 
> > > > > > > > > EDEFER doesn't solve the suspend/resume ordering, at least not in the
> > > > > > > > > general async s/r case. In any case I don't see a problem in making the
> > > > > > > > > hda component the master and I think it's more logical that way as you
> > > > > > > > > said, so I changed that.
> > > > > > > > 
> > > > > > > > Yeah for full async s/r we're screwed as-is. But see below for some crazy
> > > > > > > > ideas.
> > > > > > > > > > I think what we need here is then:
> > > > > > > > > > - Put the current azx_probe into the master_bind callback. The new
> > > > > > > > > >   azx_probe will do nothing else than register the master component and
> > > > > > > > > >   the component match list. To do so it checks the chip flag and if it
> > > > > > > > > >   needs to cooperate with i915 it registers one component match for that.
> > > > > > > > > >   The master_bind (old probe) function calls component_bind_all with the
> > > > > > > > > >   hda_intel pointer as void *data parameter.
> > > > > > > > > 
> > > > > > > > > I'm not sure this is the best way since by this the i915 module would
> > > > > > > > > need to be pinned even when no HDMI functionality is used. I think a
> > > > > > > > > better way would be to let the probe function run as now and
> > > > > > > > > init/register all the non-HDMI functionality. Then only the HDMI
> > > > > > > > > functionality would be inited/registered from the bind/unbind hooks.
> > > > > > > > 
> > > > > > > > Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway
> > > > > > > > to be able to load the driver. But if we can defer just the hdmi part.
> > > > > > > > 
> > > > > > > > > But we could go either way even as a follow-up to this patchset.
> > > > > > > > > 
> > > > > > > > > > - i915 registers a component for the i915 gfx device. It uses the
> > > > > > > > > >   component device to get at i915 sturctures and fills the dev+ops into
> > > > > > > > > >   the hda_intel pointer it gets as void *data.
> > > > > > > > > >
> > > > > > > > > > Stuff we then should do on top:
> > > > > > > > > > - Add deferred probing to azx_probe: Only when all components are there
> > > > > > > > > >   should it actually register. This will take care of all the module load
> > > > > > > > > >   order mess.
> > > > > > > > > 
> > > > > > > > > I agree that we should only register user interfaces when everything is
> > > > > > > > > in place. But I'm not sure deferred probe is the best, we could do
> > > > > > > > > without it by registering HDMI from the component bind hook.
> > > > > > > > 
> > > > > > > > It's mostly a question whether alsa does support delayed addition of
> > > > > > > > interface parts. DRM most definitely does not (except for recently added
> > > > > > > > dp mst connector hotplug). But I guess if the current driver already
> > > > > > > > delays registering the hdmi part then we're fine. I'm not sure about
> > > > > > > > whether it's really safe - I spotted not a lot of locking really to make
> > > > > > > > sure there's no races between i915 loading and userspace trying to access
> > > > > > > > the hdmi side.
> > > > > > > 
> > > > > > > Ok, I'm not sure either. If that's not possible, I agree EDEFER would be
> > > > > > > the best and with that we could also get rid of the request_module() we
> > > > > > > need now.
> > > > > > 
> > > > > > The probe of HD-audio driver is done in a work anyway, so changing the
> > > > > > code to sync with component should be feasible. 
> > > > > > 
> > > > > > I'm off today (and yesterday was a Christmas party :), so had little
> > > > > > time for review so far.  Will check the series tomorrow.
> > > > > 
> > > > > could one of you help reviewing this patchset? Note that there is a v2
> > > > > version with some individual patches having a v3 already.
> > > > 
> > > > Is there any git branch containing the latest patches that is easily
> > > > applicable to 3.19-rc (or linux-next)?
> > > 
> > > I applied them on 3.19-rc2 and pushed the branch to:
> > > https://github.com/ideak/linux/commits/audio-component
> > 
> > Thanks!  Just reading through it (but couldn't build/test it as today
> > is again a holiday here :)  The patches looks good but a few
> > nitpicking:
> > 
> > - audio_component shouldn't be added into struct azx;
> >   struct azx is a common object used by Tegra HD-audio, too, so keep
> >   it clean.  There is struct hda_intel defined in hda_intel.c, which
> >   inherits struct azx.
> > 
> >   That is, we need to cut struct hda_intel definition out to a header
> >   file (e.g. hda_intel.h) and include it in hda_i915.c.  I think we
> >   can merge the contents of hda_i915.h into hda_intel.h, too.
> 
> Ok, I did the above and also changed the hda_i915 interface to accept
> hda_intel instead of chip as that makes now more sense.
> 
> > - The removal of include/drm/i915_powerwell.h should be folded into
> >   the last patch, so that we can separate the changes in sound/* and
> >   drm/i915/* in each patch.
> 
> Oops, that would've also caused a bisect error. I moved the removal to
> the last patch.
> 
> I tested this on HSW, seems to work ok. I updated the github branch, if
> you don't see more issues, I will post the new version to the list.

Thanks.  I tried pulling your branch and did a quick test.  It looks
working well.

After reading the commits again, the following minor issues came to my
mind:

- request_module("i915") may return an error if i915 is built-in.
  We can simply ignore the return value there since the binding
  condition is checked at the next sooner or later.

- pr_xxx() can be replaced with dev_xxx() as we pass the hda object to
  each function.

Other than that, all patches look good.  Feel free to take my
reviewed-by tag.
  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx