| Message ID | 20170107012109.25744-3-jerome.anand@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Same here, missing fixes on agreed comments? 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? > > /** > 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 > + 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. > > #define HDMI_MAX_ELD_BYTES 128 > >
> -----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 > > > >
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); } /** 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; + } + 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> #define HDMI_MAX_ELD_BYTES 128