Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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);
> > > > > +			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.

> 
> > >
> > > > > +
> > > > > +			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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx