> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > Sent: Monday, November 28, 2016 7:30 PM > To: Anand, Jerome <jerome.anand@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; > broonie@xxxxxxxxxx; tiwai@xxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; > Ughreja, Rakesh A <rakesh.a.ughreja@xxxxxxxxx> > 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@xxxxxxxxxxxxxxx] > > > Sent: Thursday, November 24, 2016 7:03 PM > > > To: Anand, Jerome <jerome.anand@xxxxxxxxx> > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; > > > broonie@xxxxxxxxxx; tiwai@xxxxxxx; > > > pierre-louis.bossart@xxxxxxxxxxxxxxx; > > > Ughreja, Rakesh A <rakesh.a.ughreja@xxxxxxxxx> > > > 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@xxxxxxxxxxxxxxx> > > > > Signed-off-by: Jerome Anand <jerome.anand@xxxxxxxxx> > > > > --- > > > > 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); > > > > + 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. > > > > > 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); > > > > + } 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 > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
