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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Anand, Jerome" <jerome.anand@intel.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	Takashi Iwai <tiwai@suse.de>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"Ughreja, Rakesh A" <rakesh.a.ughreja@intel.com>
Subject: Re: [PATCH 2/7] drm/i915: Add support for audio driver notifications
Date: Thu, 15 Dec 2016 13:48:14 +0200	[thread overview]
Message-ID: <20161215114814.GX31595@intel.com> (raw)
In-Reply-To: <F6867404AABACB4D8EC371BF802A4B7C149345F8@fmsmsx101.amr.corp.intel.com>

On Thu, Dec 15, 2016 at 10:18:13AM +0000, Anand, Jerome wrote:
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, December 14, 2016 5:21 PM
> > To: Anand, Jerome <jerome.anand@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> > ville.syrjala@linux.intel.com; broonie@kernel.org; pierre-
> > louis.bossart@linux.intel.com; Ughreja, Rakesh A
> > <rakesh.a.ughreja@intel.com>
> > Subject: Re: [PATCH 2/7] drm/i915: Add support for audio driver notifications
> > 
> > On Mon, 12 Dec 2016 19:10:38 +0100,
> > 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 1317834..0dbe91a 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3696,6 +3696,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 3bbc96c..77cd086 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, true);
> > 
> > Does it still notify as connected?
> > 
> 
> OK - this will be changed

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 e12e5f7..a141a9c 100644
> > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > @@ -361,3 +361,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;
> > > +
> > > +	if (HAS_LPE_AUDIO(dev_priv)) {
> > 
> > Just bail out here when !HAS_LPE_AUDIO().  Then the rest needs less
> > indentation and will be more readable.
> > 
> 
> OK
> 
> > > +		struct intel_hdmi_lpe_audio_pdata *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;
> > > +		}
> > 
> > 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.
> > 
> 
> 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>
> > 
> > Why do we need to add a header at this point out of sudden?
> > 
> 
> It was for some warning removal. I'll try to remove this if not applicable now.
> 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > 
> > >
> > >  #define HDMI_MAX_ELD_BYTES	128
> > >
> > > --
> > > 2.9.3
> > >

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