Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 25, 2016 at 05:25:43AM +0530, Jerome Anand wrote:
> Notifiations like mode change, hot plug and edid to
> the audio driver are added. This is inturn used by the
> audio driver for its functionality.
> 
> A new interface file capturing the notifications needed by the
> audio driver is added
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
>  drivers/gpu/drm/i915/intel_audio.c     |  8 ++++++
>  drivers/gpu/drm/i915/intel_hdmi.c      |  1 +
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 49 ++++++++++++++++++++++++++++++++++
>  include/drm/intel_lpe_audio.h          |  1 +
>  5 files changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2a79048..33bc44c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3561,6 +3561,9 @@ int  intel_lpe_audio_setup(struct drm_i915_private *dev_priv);
>  void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv);
>  void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv);
>  bool intel_lpe_audio_detect(struct drm_i915_private *dev_priv);
> +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> +			void *eld, int port, int tmds_clk_speed,
> +			bool connected);
>  
>  /* intel_i2c.c */
>  extern int intel_setup_gmbus(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 1c509f7..55a6831 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -24,6 +24,7 @@
>  #include <linux/kernel.h>
>  #include <linux/component.h>
>  #include <drm/i915_component.h>
> +#include <drm/intel_lpe_audio.h>
>  #include "intel_drv.h"
>  
>  #include <drm/drmP.h>
> @@ -627,6 +628,10 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
>  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
>  						 (int) port, (int) pipe);
> +
> +	if (HAS_LPE_AUDIO(dev_priv))
> +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> +			crtc_state->port_clock, true);
>  }
>  
>  /**
> @@ -660,6 +665,9 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
>  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
>  						 (int) port, (int) pipe);
> +
> +	if (HAS_LPE_AUDIO(dev_priv))
> +		intel_lpe_audio_notify(dev_priv, NULL, port, 0, true);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index fb88e32..02d50e3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -36,6 +36,7 @@
>  #include <drm/drm_edid.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
> +#include <drm/intel_lpe_audio.h>
>  #include "i915_drv.h"
>  
>  static struct drm_device *intel_hdmi_to_dev(struct intel_hdmi *intel_hdmi)
> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> index 5335fc6..93f83cb 100644
> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -367,3 +367,52 @@ void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
> +
> +
> +/**
> + * intel_lpe_audio_notify() - notify lpe audio event
> + * audio driver and i915
> + * @dev_priv: the i915 drm device private data
> + * @eld : ELD data
> + * @port: port id
> + * @tmds_clk_speed: tmds clock frequency in Hz
> + * @connected: hdmi connected/disconnected
> + *
> + * Notify lpe audio driver of eld change.
> + */
> +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> +			void *eld, int port, int tmds_clk_speed,
> +			bool connected)
> +{
> +	unsigned long irq_flags;
> +
> +	if (HAS_LPE_AUDIO(dev_priv)) {
> +		struct intel_hdmi_lpe_audio_pdata *pdata = dev_get_platdata(
> +			&(dev_priv->lpe_audio.platdev->dev));
> +
> +		if (pdata) {

How could the !pdata case happen? If it can't we shouldn't cater for it.

> +			spin_lock_irqsave(&pdata->lpe_audio_slock,
> +				irq_flags);
> +
> +			if (eld != NULL) {
> +				memcpy(pdata->eld.eld_data, eld,
> +					HDMI_MAX_ELD_BYTES);
> +				pdata->eld.port_id = port;
> +
> +				if (tmds_clk_speed)

Why the if?

> +					pdata->tmds_clock_speed =
> +						tmds_clk_speed;
> +			}
> +			pdata->hdmi_connected = connected;

Also can't really see much need for this. Any one of the port/tmds
clock/eld should be sufficient.

> +			if (pdata->notify_audio_lpe)
> +				pdata->notify_audio_lpe(
> +					(eld != NULL) ? &pdata->eld : NULL);
> +			else
> +				pdata->notify_pending = true;

Still not sure why the "pending" thing is useful. Can't the audio driver
just do its thing (whatever it is) unconditionally?

When disabling just clear the port to INVALID, eld to zero, and tmds
clock to 0, and it should all be fine no?

> +
> +			spin_unlock_irqrestore(&pdata->lpe_audio_slock,
> +				irq_flags);
> +		} else
> +			DRM_DEBUG_DRIVER("no audio notification\n");
> +	}
> +}
> diff --git a/include/drm/intel_lpe_audio.h b/include/drm/intel_lpe_audio.h
> index a64c449..952de05 100644
> --- a/include/drm/intel_lpe_audio.h
> +++ b/include/drm/intel_lpe_audio.h
> @@ -25,6 +25,7 @@
>  #define _INTEL_LPE_AUDIO_H_
>  
>  #include <linux/types.h>
> +#include <linux/spinlock_types.h>
>  
>  #define HDMI_MAX_ELD_BYTES	128
>  
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx