-----Original Message----- From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] Sent: Saturday, January 7, 2017 1:50 AM To: Anand, Jerome jerome.anand@intel.com; intel- gfx@lists.freedesktop.org; alsa-devel@alsa-project.org Cc: tiwai@suse.de; broonie@kernel.org; Ughreja, Rakesh A rakesh.a.ughreja@intel.com; ville.syrjala@linux.intel.com Subject: Re: [alsa-devel] [PATCH V2 2/7] drm/i915: Add support for audio driver notifications
Same here, missing fixes on agreed comments?
I have left some since it was addressed already. Comments inline.
On 1/6/17 7:21 PM, 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 | 46
++++++++++++++++++++++++++++++++++
include/drm/intel_lpe_audio.h | 1 + 5 files changed, 59 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2f8165e..263bc48 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3634,6 +3634,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_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 16c2027..aeb37c2 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> @@ -630,6 +631,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);}
/** @@ -663,6 +668,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, false);}
From Ville: The entire 'connected' parameter seems superfluous. Also why aren't we passing 'pipe' along here? How is the audio driver supposed to find the right thing to use?
I thought we discussed this already. Audio need not know about the PIPE/ gfx design. I prefer to keep this 'connected' parameter.
/** diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 0bcfead..377584e1 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 05f5e4e..2a3c1e8 100644 --- a/drivers/gpu/drm/i915/intel_lpe_audio.c +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c @@ -353,3 +353,49 @@ 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;
- struct intel_hdmi_lpe_audio_pdata *pdata = NULL;
- if (!HAS_LPE_AUDIO(dev_priv))
return;- pdata = dev_get_platdata(
&(dev_priv->lpe_audio.platdev->dev));- 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)pdata->tmds_clock_speed = tmds_clk_speed;- }
From Takashi: If eld==NULL means that no ELD is found (or disconnected), it's better to clear pdata eld as well, so that we don't leak the previous ELD.
JA: OK
Eld is sent as NULL if unavailable. So leak is not possible. Hence ignored it.
- pdata->hdmi_connected = connected;
- if (pdata->notify_audio_lpe)
pdata->notify_audio_lpe((eld != NULL) ? &pdata->eld : NULL);- else
pdata->notify_pending = true;- spin_unlock_irqrestore(&pdata->lpe_audio_slock,
irq_flags);+} 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>
From Takashi: Why do we need to add a header at this point out of sudden?
JA: It was for some warning removal. I'll try to remove this if not applicable now.
This is needed since we use spin lock in the code.
#define HDMI_MAX_ELD_BYTES 128