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

[Daniel Vetter <daniel@ffwll.ch>]

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