[Intel-gfx] [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 at 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 at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>