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