Re: [alsa-devel] [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
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);elsepdata->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);} elseDRM_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
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Thursday, November 24, 2016 7:03 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
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.
It can happen if there is a notification before audio driver is initialized
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?
Tmds of zero is sent in codec_disable. Hence added, just to avoid unnecessary notification to audio driver
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.
Eld and tmds are needed to cater to hotplug and display mode change notifications. Port can be avoided but kept for debug
if (pdata->notify_audio_lpe)pdata->notify_audio_lpe((eld != NULL) ? &pdata->eld : NULL);elsepdata->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?
This is added to avoid race when audio driver loads late and the notification from display has already passed.
When disabling just clear the port to INVALID, eld to zero, and tmds clock to 0, and it should all be fine no?
Yes, that's what is being done.
spin_unlock_irqrestore(&pdata->lpe_audio_slock,irq_flags);} elseDRM_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
On Fri, Nov 25, 2016 at 05:51:00AM +0000, Anand, Jerome wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Thursday, November 24, 2016 7:03 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
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.
It can happen if there is a notification before audio driver is initialized
You're setting the platform_data in i915 code.
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?
Tmds of zero is sent in codec_disable. Hence added, just to avoid unnecessary notification to audio driver
I have no idea why hanging on to a bogus tmds clock would prevert some notification call.
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.
Eld and tmds are needed to cater to hotplug and display mode change notifications. Port can be avoided but kept for debug
I don't know what you mean. The ELD/tmds clock should only be needed when the display is on.
if (pdata->notify_audio_lpe)pdata->notify_audio_lpe((eld != NULL) ? &pdata->eld : NULL);elsepdata->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?
This is added to avoid race when audio driver loads late and the notification from display has already passed.
You keep saying that but I can't see it.
When disabling just clear the port to INVALID, eld to zero, and tmds clock to 0, and it should all be fine no?
Yes, that's what is being done.
Where?
spin_unlock_irqrestore(&pdata->lpe_audio_slock,irq_flags);} elseDRM_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
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, November 28, 2016 7:30 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
On Fri, Nov 25, 2016 at 05:51:00AM +0000, Anand, Jerome wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Thursday, November 24, 2016 7:03 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
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.
It can happen if there is a notification before audio driver is initialized
You're setting the platform_data in i915 code.
OK
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?
Tmds of zero is sent in codec_disable. Hence added, just to avoid unnecessary notification to audio driver
I have no idea why hanging on to a bogus tmds clock would prevert some notification call.
Am not sure what advantage is gained here by removing it
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.
Eld and tmds are needed to cater to hotplug and display mode change notifications. Port can be avoided but kept for debug
I don't know what you mean. The ELD/tmds clock should only be needed when the display is on.
I don't get your point. We are doing this whole exercise to cater to display ON scenarios.
if (pdata->notify_audio_lpe)pdata->notify_audio_lpe((eld != NULL) ? &pdata->eld : NULL);elsepdata->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?
This is added to avoid race when audio driver loads late and the notification
from display has already passed.
You keep saying that but I can't see it.
I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver. Since the audio callbacks are not initialized, notification gets missed.
When disabling just clear the port to INVALID, eld to zero, and tmds clock to 0, and it should all be fine no?
Yes, that's what is being done.
Where?
Notify callback will have eld to NULL and tmds to zero sent in codec_disable
spin_unlock_irqrestore(&pdata->lpe_audio_slock,irq_flags);} elseDRM_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
-- Ville Syrjälä Intel OTC
On Mon, Nov 28, 2016 at 04:50:15PM +0000, Anand, Jerome wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, November 28, 2016 7:30 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
On Fri, Nov 25, 2016 at 05:51:00AM +0000, Anand, Jerome wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Thursday, November 24, 2016 7:03 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
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.
It can happen if there is a notification before audio driver is initialized
You're setting the platform_data in i915 code.
OK
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?
Tmds of zero is sent in codec_disable. Hence added, just to avoid unnecessary notification to audio driver
I have no idea why hanging on to a bogus tmds clock would prevert some notification call.
Am not sure what advantage is gained here by removing it
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.
Eld and tmds are needed to cater to hotplug and display mode change notifications. Port can be avoided but kept for debug
I don't know what you mean. The ELD/tmds clock should only be needed when the display is on.
I don't get your point. We are doing this whole exercise to cater to display ON scenarios.
Yes, but the hdmi_connected thing seems totally redundant. It can be derived from any of the eld/tmds_clock/port being set to 0/INVALID.
if (pdata->notify_audio_lpe)pdata->notify_audio_lpe((eld != NULL) ? &pdata->eld : NULL);elsepdata->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?
This is added to avoid race when audio driver loads late and the notification
from display has already passed.
You keep saying that but I can't see it.
I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver. Since the audio callbacks are not initialized, notification gets missed.
Sure. But what does the extra notification_pending flag buy us? The audio driver could just check the eld/tmds_clock/port directly.
When disabling just clear the port to INVALID, eld to zero, and tmds clock to 0, and it should all be fine no?
Yes, that's what is being done.
Where?
Notify callback will have eld to NULL and tmds to zero sent in codec_disable
But the driver can look those thigns up directly as well it seems. So this whole thing is a bit of a mess on account of sharing the platform as a communication channel and also trying to pass the things as paraameters to the notify hook. I think we need to pick one or the other approach, not some mismash of both.
spin_unlock_irqrestore(&pdata->lpe_audio_slock,irq_flags);} elseDRM_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
-- Ville Syrjälä Intel OTC
On 11/28/16 11:01 AM, Ville Syrjälä wrote:
if (pdata->notify_audio_lpe)pdata->notify_audio_lpe((eld != NULL) ? &pdata->eld : NULL);elsepdata->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?
This is added to avoid race when audio driver loads late and the notification
from display has already passed.
You keep saying that but I can't see it.
I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver. Since the audio callbacks are not initialized, notification gets missed.
Sure. But what does the extra notification_pending flag buy us? The audio driver could just check the eld/tmds_clock/port directly.
When disabling just clear the port to INVALID, eld to zero, and tmds clock to 0, and it should all be fine no?
Yes, that's what is being done.
Where?
Notify callback will have eld to NULL and tmds to zero sent in codec_disable
But the driver can look those thigns up directly as well it seems. So this whole thing is a bit of a mess on account of sharing the platform as a communication channel and also trying to pass the things as paraameters to the notify hook. I think we need to pick one or the other approach, not some mismash of both.
Indeed it looks weird to have both a parameter for tmds_clock in the pdata AND the notify parameter, this can probably be cleaned-up.
That said, I am not sure I completely understand the feedback that the audio driver can get all the eld/tmds/port information directly. We are trying to avoid accessing the data structures of the i915 driver. Are you suggesting a scheme where the i915 driver would just provide a door-bell like notification and the audio driver would use a get_eld/tmds/port interface exposed by the i915 driver on startup and upon receiving this notification?
On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
On 11/28/16 11:01 AM, Ville Syrjälä wrote:
> + 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?
This is added to avoid race when audio driver loads late and the notification
from display has already passed.
You keep saying that but I can't see it.
I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver. Since the audio callbacks are not initialized, notification gets missed.
Sure. But what does the extra notification_pending flag buy us? The audio driver could just check the eld/tmds_clock/port directly.
When disabling just clear the port to INVALID, eld to zero, and tmds clock to 0, and it should all be fine no?
Yes, that's what is being done.
Where?
Notify callback will have eld to NULL and tmds to zero sent in codec_disable
But the driver can look those thigns up directly as well it seems. So this whole thing is a bit of a mess on account of sharing the platform as a communication channel and also trying to pass the things as paraameters to the notify hook. I think we need to pick one or the other approach, not some mismash of both.
Indeed it looks weird to have both a parameter for tmds_clock in the pdata AND the notify parameter, this can probably be cleaned-up.
That said, I am not sure I completely understand the feedback that the audio driver can get all the eld/tmds/port information directly. We are trying to avoid accessing the data structures of the i915 driver.
IIRC what I proposed originally didn't even expose the same structure to both sides, but that's not what we seem to have atm.
Are you suggesting a scheme where the i915 driver would just provide a door-bell like notification and the audio driver would use a get_eld/tmds/port interface exposed by the i915 driver on startup and upon receiving this notification?
Well, we could do it that way, or we'd do it the other way that the audio driver just calls i915 to triggers a single i915->audio notify call after the audio driver has finished its probe. Or we could do whatever we seem to have now is where the audio driver can just root around directly in the structure (at which point passing any parameters in the notify calls seems redundant as well).
On 11/28/16 1:30 PM, Ville Syrjälä wrote:
On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
On 11/28/16 11:01 AM, Ville Syrjälä wrote:
>> + 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? > This is added to avoid race when audio driver loads late and the notification
from display has already passed.
You keep saying that but I can't see it.
I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver. Since the audio callbacks are not initialized, notification gets missed.
Sure. But what does the extra notification_pending flag buy us? The audio driver could just check the eld/tmds_clock/port directly.
> When disabling just clear the port to INVALID, eld to zero, and tmds > clock to 0, and it should all be fine no? > Yes, that's what is being done.
Where?
Notify callback will have eld to NULL and tmds to zero sent in codec_disable
But the driver can look those thigns up directly as well it seems. So this whole thing is a bit of a mess on account of sharing the platform as a communication channel and also trying to pass the things as paraameters to the notify hook. I think we need to pick one or the other approach, not some mismash of both.
Indeed it looks weird to have both a parameter for tmds_clock in the pdata AND the notify parameter, this can probably be cleaned-up.
That said, I am not sure I completely understand the feedback that the audio driver can get all the eld/tmds/port information directly. We are trying to avoid accessing the data structures of the i915 driver.
IIRC what I proposed originally didn't even expose the same structure to both sides, but that's not what we seem to have atm.
Are you suggesting a scheme where the i915 driver would just provide a door-bell like notification and the audio driver would use a get_eld/tmds/port interface exposed by the i915 driver on startup and upon receiving this notification?
Well, we could do it that way, or we'd do it the other way that the audio driver just calls i915 to triggers a single i915->audio notify call after the audio driver has finished its probe. Or we could do whatever we seem to have now is where the audio driver can just root around directly in the structure (at which point passing any parameters in the notify calls seems redundant as well).
Looking at some older emails, i think you recommended a 'register' callback to let the audio driver signal to the i915 side it completed its initialization, with a single notify generated if needed (what you described just above as 'the other way')
If you look at the path 4 of the series, Jerome was trying to implement this with a 'notify_pending' field in the platform data set by the i915 side and used during the audio driver probe
+ if (pdata->notify_pending) { + + pr_debug("%s: handle pending notification\n", __func__); + notify_audio_lpe(&pdata->eld); + pdata->notify_pending = false; + }
Maybe an explicit handshake is more self-explanatory and safer?
On Mon, 28 Nov 2016 22:56:24 +0100, Pierre-Louis Bossart wrote:
On 11/28/16 1:30 PM, Ville Syrjälä wrote:
On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
On 11/28/16 11:01 AM, Ville Syrjälä wrote:
>>> + 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? >> > This is added to avoid race when audio driver loads late and the notification from display has already passed.
You keep saying that but I can't see it.
I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver. Since the audio callbacks are not initialized, notification gets missed.
Sure. But what does the extra notification_pending flag buy us? The audio driver could just check the eld/tmds_clock/port directly.
>> When disabling just clear the port to INVALID, eld to zero, and tmds >> clock to 0, and it should all be fine no? >> > Yes, that's what is being done. Where?
Notify callback will have eld to NULL and tmds to zero sent in codec_disable
But the driver can look those thigns up directly as well it seems. So this whole thing is a bit of a mess on account of sharing the platform as a communication channel and also trying to pass the things as paraameters to the notify hook. I think we need to pick one or the other approach, not some mismash of both.
Indeed it looks weird to have both a parameter for tmds_clock in the pdata AND the notify parameter, this can probably be cleaned-up.
That said, I am not sure I completely understand the feedback that the audio driver can get all the eld/tmds/port information directly. We are trying to avoid accessing the data structures of the i915 driver.
IIRC what I proposed originally didn't even expose the same structure to both sides, but that's not what we seem to have atm.
Are you suggesting a scheme where the i915 driver would just provide a door-bell like notification and the audio driver would use a get_eld/tmds/port interface exposed by the i915 driver on startup and upon receiving this notification?
Well, we could do it that way, or we'd do it the other way that the audio driver just calls i915 to triggers a single i915->audio notify call after the audio driver has finished its probe. Or we could do whatever we seem to have now is where the audio driver can just root around directly in the structure (at which point passing any parameters in the notify calls seems redundant as well).
Looking at some older emails, i think you recommended a 'register' callback to let the audio driver signal to the i915 side it completed its initialization, with a single notify generated if needed (what you described just above as 'the other way')
If you look at the path 4 of the series,
I seem to have missed the whole series by some reason, both to my inbox and to ML. Could you resubmit later?
Jerome was trying to implement this with a 'notify_pending' field in the platform data set by the i915 side and used during the audio driver probe
- if (pdata->notify_pending) {
pr_debug("%s: handle pending notification\n", __func__);notify_audio_lpe(&pdata->eld);pdata->notify_pending = false;- }
Maybe an explicit handshake is more self-explanatory and safer?
IMO yes, the pending notification flag isn't intrusive.
There are cases where the audio driver wants to inquire the status from gfx side; at least, the HD-audio driver needed to recheck after PM.
Also, isn't the code above racy without a proper mutex?
thanks,
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, November 29, 2016 1:58 PM To: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com; Anand, Jerome jerome.anand@intel.com; intel-gfx@lists.freedesktop.org; alsa- devel@alsa-project.org; broonie@kernel.org; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
On Mon, 28 Nov 2016 22:56:24 +0100, Pierre-Louis Bossart wrote:
On 11/28/16 1:30 PM, Ville Syrjälä wrote:
On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
On 11/28/16 11:01 AM, Ville Syrjälä wrote:
>>>> + 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? >>> >> This is added to avoid race when audio driver loads late and >> the notification > from display has already passed. > > You keep saying that but I can't see it. > I have seen this happen - before audio driver is loaded, codec enable
completes and notification is sent to the audio driver.
Since the audio callbacks are not initialized, notification gets missed.
Sure. But what does the extra notification_pending flag buy us? The audio driver could just check the eld/tmds_clock/port directly.
>>> When disabling just clear the port to INVALID, eld to zero, >>> and tmds clock to 0, and it should all be fine no? >>> >> Yes, that's what is being done. > Where? > Notify callback will have eld to NULL and tmds to zero sent in codec_disable
But the driver can look those thigns up directly as well it seems. So this whole thing is a bit of a mess on account of sharing the platform as a communication channel and also trying to pass the things as paraameters to the notify hook. I think we need to pick one or the other approach, not some mismash of both.
Indeed it looks weird to have both a parameter for tmds_clock in the pdata AND the notify parameter, this can probably be cleaned-up.
That said, I am not sure I completely understand the feedback that the audio driver can get all the eld/tmds/port information directly. We are trying to avoid accessing the data structures of the i915
driver.
IIRC what I proposed originally didn't even expose the same structure to both sides, but that's not what we seem to have atm.
Are you suggesting a scheme where the i915 driver would just provide a door-bell like notification and the audio driver would use a get_eld/tmds/port interface exposed by the i915 driver on startup and upon receiving this notification?
Well, we could do it that way, or we'd do it the other way that the audio driver just calls i915 to triggers a single i915->audio notify call after the audio driver has finished its probe. Or we could do whatever we seem to have now is where the audio driver can just root around directly in the structure (at which point passing any parameters in the notify calls seems redundant as well).
Looking at some older emails, i think you recommended a 'register' callback to let the audio driver signal to the i915 side it completed its initialization, with a single notify generated if needed (what you described just above as 'the other way')
If you look at the path 4 of the series,
I seem to have missed the whole series by some reason, both to my inbox and to ML. Could you resubmit later?
Yes - I will resend rfc v4 after addressing some of Ville's comment.
Jerome was trying to implement this with a 'notify_pending' field in the platform data set by the i915 side and used during the audio driver probe
Yes - thanks Pierre. I believe Ville's comment was addressed with this change. Am not sure if there is anything critically missed out leading to a flaw
- if (pdata->notify_pending) {
pr_debug("%s: handle pending notification\n", __func__);notify_audio_lpe(&pdata->eld);pdata->notify_pending = false;- }
Maybe an explicit handshake is more self-explanatory and safer?
IMO yes, the pending notification flag isn't intrusive.
Yes - I thought it was simpler to handle the notification in this manner
There are cases where the audio driver wants to inquire the status from gfx side; at least, the HD-audio driver needed to recheck after PM.
Am not sure if there is any case like that to be handled here. Hence a simpler interface was created and implemented
Also, isn't the code above racy without a proper mutex?
No
thanks,
Takashi
participants (4)
-
Anand, Jerome -
Pierre-Louis Bossart -
Takashi Iwai -
Ville Syrjälä