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

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

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

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_ */

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

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;

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_ */

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

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

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