Re: [PATCH] drm/i915/dp: DP audio API changes for MST

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

On Wed, Aug 03, 2016 at 09:42:53PM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2016-08-03 at 23:28 +0300, Ville Syrjälä wrote:
> > On Wed, Aug 03, 2016 at 07:43:06PM +0000, Pandiyan, Dhinakaran wrote:
> > > On Wed, 2016-08-03 at 22:08 +0300, Ville Syrjälä wrote:
> > > > On Tue, Aug 02, 2016 at 07:14:30PM -0700, Dhinakaran Pandiyan wrote:
> > > > > DP MST provides the capability to send multiple video and audio streams via
> > > > > one single port. This requires the API's between i915 and audio drivers to
> > > > > distinguish between audio capable displays connected to a port. This patch
> > > > > adds this support via an additional parameter 'int dev_id'. The existing
> > > > > parameter 'port' does not change it's meaning.
> > > > > 
> > > > > dev_id =
> > > > > 	MST	: pipe that the stream originates from
> > > > > 	Non-MST	: -1
> > > > > 
> > > > > Affected APIs:
> > > > > struct i915_audio_component_ops
> > > > > -       int (*sync_audio_rate)(struct device *, int port, int rate);
> > > > > +	int (*sync_audio_rate)(struct device *, int port, int dev_id,
> > > > 
> > > > Does the term 'dev_id' have some special meaning on the audio side? On
> > > > the i915 side things would be less confusing if we just called it
> > > > 'pipe'.
> > > > 
> > > 
> > > There was quite a bit of back and forth on this. "pipe" seemed to tie us
> > > to the gfx hardware architecture. In case, there is no 1:1 mapping from
> > > pipes to encoders, this will get confusing as i915 audio registers
> > > (E.g.HSW_AUD_PIN_ELD_CP_VLD) are encoder based.
> > 
> > I'm not following. Looking at the current i915 audio code, on HSW+
> > everything is pipe based.
> > 
> Although we pass "pipe" to these macros, the definition here and in the
> BSpec seem to indicate they are based on the encoder.
> 
> Eg: #define HSW_AUD_PIN_ELD_CP_VLD          _MMIO(0x650c0)
> #define   AUDIO_INACTIVE(trans)         ((1 << 3) << ((trans) * 4))
> #define   AUDIO_OUTPUT_ENABLE(trans)    ((1 << 2) << ((trans) * 4))
> #define   AUDIO_CP_READY(trans)         ((1 << 1) << ((trans) * 4))
> #define   AUDIO_ELD_VALID(trans)        ((1 << 0) << ((trans) * 4))

That's transcoder, not encoder. transcoder is the tail end of the pipe.
For all the external ports transcoder==pipe, and for port A there is the
special edp transcoder which can be fed by any pipe. The edp transcoder
doesn't support audio though, and I can't see anything in the spec to
suggest that it would gain audio capabilities, in the near future at
least. So from that point of view passing around the pipe would seem
like a safe approach.

We could of course pass around transcoder instead of pipe. Since we
don't ever need to enable audio on the edp transcoder, we don't even
have to worry about what number we assign to it. And hence it is
actually the same as passing the pipe, except in name.

> 
> 
> > > Whereas, dev_id is
> > > generic - HDA spec calls them "devices" or "device entry"
> > 
> > Generic and totally unclear what it means from graphics side.
> > 
> > pipe and port are the only things we really have in i915. If those aren't
> > good enough for the audio side, then I think we should have some decent
> > code to remap between the pipe+port vs. whatever audio side id is needed.
> > I don't really care on which side of the border such code would live, as
> > long as it's clear what it's doing.
> > 
> 
> I agree, dev_id can be misleading within i915.
> > > 
> > > > > +	     int rate);
> > > > > 
> > > > > -       int (*get_eld)(struct device *, int port, bool *enabled,
> > > > > -                       unsigned char *buf, int max_bytes);
> > > > > +       int (*get_eld)(struct device *, int port, int dev_id,
> > > > > +		       bool *enabled, unsigned char *buf, int max_bytes);
> > > > > 
> > > > > struct i915_audio_component_audio_ops
> > > > > -       void (*pin_eld_notify)(void *audio_ptr, int port);
> > > > > +       void (*pin_eld_notify)(void *audio_ptr, int port, int dev_id);
> > > > > 
> > > > > This patch makes dummy changes in the audio drivers for build to succeed.
> > > > > 
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h    |  2 +-
> > > > >  drivers/gpu/drm/i915/intel_audio.c | 82 +++++++++++++++++++++++++++++---------
> > > > >  include/drm/i915_component.h       |  6 +--
> > > > >  include/sound/hda_i915.h           | 11 ++---
> > > > >  sound/hda/hdac_i915.c              |  9 +++--
> > > > >  sound/pci/hda/patch_hdmi.c         |  7 ++--
> > > > >  6 files changed, 82 insertions(+), 35 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 65ada5d..8e4c8c8 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -2054,7 +2054,7 @@ struct drm_i915_private {
> > > > >  	/* perform PHY state sanity checks? */
> > > > >  	bool chv_phy_assert[2];
> > > > >  
> > > > > -	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
> > > > > +	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
> > > > >  
> > > > >  	/*
> > > > >  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
> > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > > index 8c493de..cbe44c8 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > @@ -500,6 +500,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> > > > >  	struct i915_audio_component *acomp = dev_priv->audio_component;
> > > > >  	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
> > > > >  	enum port port = intel_dig_port->port;
> > > > > +	enum pipe pipe = crtc->pipe;
> > > > > +	int dev_id = -1;
> > > > > +
> > > > >  
> > > > >  	connector = drm_select_eld(encoder);
> > > > >  	if (!connector)
> > > > > @@ -522,14 +525,19 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> > > > >  		dev_priv->display.audio_codec_enable(connector, intel_encoder,
> > > > >  						     adjusted_mode);
> > > > >  
> > > > > +	if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
> > > > > +		dev_id = pipe;
> > > > > +
> > > > >  	mutex_lock(&dev_priv->av_mutex);
> > > > >  	intel_encoder->audio_connector = connector;
> > > > > +
> > > > >  	/* referred in audio callbacks */
> > > > > -	dev_priv->dig_port_map[port] = intel_encoder;
> > > > > +	dev_priv->av_enc_map[pipe] = intel_encoder;
> > > > >  	mutex_unlock(&dev_priv->av_mutex);
> > > > >  
> > > > >  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
> > > > > -		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
> > > > > +		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
> > > > > +						 (int) port, dev_id);
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > @@ -542,22 +550,29 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> > > > >  void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
> > > > >  {
> > > > >  	struct drm_encoder *encoder = &intel_encoder->base;
> > > > > +	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > > >  	struct drm_device *dev = encoder->dev;
> > > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > >  	struct i915_audio_component *acomp = dev_priv->audio_component;
> > > > >  	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
> > > > >  	enum port port = intel_dig_port->port;
> > > > > +	enum pipe pipe = crtc->pipe;
> > > > > +	int dev_id = -1;
> > > > > +
> > > > > +	if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
> > > > > +		dev_id = pipe;
> > > > >  
> > > > >  	if (dev_priv->display.audio_codec_disable)
> > > > >  		dev_priv->display.audio_codec_disable(intel_encoder);
> > > > >  
> > > > >  	mutex_lock(&dev_priv->av_mutex);
> > > > >  	intel_encoder->audio_connector = NULL;
> > > > > -	dev_priv->dig_port_map[port] = NULL;
> > > > > +	dev_priv->av_enc_map[pipe] = NULL;
> > > > >  	mutex_unlock(&dev_priv->av_mutex);
> > > > >  
> > > > >  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
> > > > > -		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
> > > > > +		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
> > > > > +						 (int) port, dev_id);
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > @@ -628,8 +643,32 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev)
> > > > >  	return dev_priv->cdclk_freq;
> > > > >  }
> > > > >  
> > > > > -static int i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > -						int port, int rate)
> > > > > +static struct intel_encoder *get_saved_encoder(struct intel_encoder *av_enc_map[],
> > > > > +					       int port, int dev_id)
> > > > > +{
> > > > > +
> > > > > +	enum pipe pipe;
> > > > > +	struct drm_encoder *encoder;
> > > > > +
> > > > > +	if (dev_id >= 0 && dev_id < I915_MAX_PIPES)
> > > > > +		return av_enc_map[dev_id];
> > > > > +
> > > > > +	for (pipe = PIPE_A; pipe < I915_MAX_PIPES; pipe++) {
> > > > > +
> > > > > +		if (!av_enc_map[pipe])
> > > > > +			continue;
> > > > > +
> > > > > +		encoder = &av_enc_map[pipe]->base;
> > > > > +		if (port == enc_to_dig_port(encoder)->port)
> > > > > +			return av_enc_map[pipe];
> > > > > +	}
> > > > > +
> > > > > +	return NULL;
> > > > > +
> > > > > +}
> > > > > +
> > > > > +static int i915_audio_component_sync_audio_rate(struct device *dev, int port,
> > > > > +						int dev_id, int rate)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > > >  	struct intel_encoder *intel_encoder;
> > > > > @@ -649,15 +688,16 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> > > > >  		return 0;
> > > > >  
> > > > >  	mutex_lock(&dev_priv->av_mutex);
> > > > > +
> > > > >  	/* 1. get the pipe */
> > > > > -	intel_encoder = dev_priv->dig_port_map[port];
> > > > > -	/* intel_encoder might be NULL for DP MST */
> > > > > +	intel_encoder = get_saved_encoder(dev_priv->av_enc_map, port, dev_id);
> > > > >  	if (!intel_encoder || !intel_encoder->base.crtc ||
> > > > >  	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
> > > > >  		DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > > > >  		err = -ENODEV;
> > > > >  		goto unlock;
> > > > >  	}
> > > > > +
> > > > >  	crtc = to_intel_crtc(intel_encoder->base.crtc);
> > > > >  	pipe = crtc->pipe;
> > > > >  	if (pipe == INVALID_PIPE) {
> > > > > @@ -702,7 +742,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> > > > >  }
> > > > >  
> > > > >  static int i915_audio_component_get_eld(struct device *dev, int port,
> > > > > -					bool *enabled,
> > > > > +					int dev_id, bool *enabled,
> > > > >  					unsigned char *buf, int max_bytes)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > > > @@ -710,17 +750,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
> > > > >  	const u8 *eld;
> > > > >  	int ret = -EINVAL;
> > > > >  
> > > > > +
> > > > >  	mutex_lock(&dev_priv->av_mutex);
> > > > > -	intel_encoder = dev_priv->dig_port_map[port];
> > > > > -	/* intel_encoder might be NULL for DP MST */
> > > > > -	if (intel_encoder) {
> > > > > -		ret = 0;
> > > > > -		*enabled = intel_encoder->audio_connector != NULL;
> > > > > -		if (*enabled) {
> > > > > -			eld = intel_encoder->audio_connector->eld;
> > > > > -			ret = drm_eld_size(eld);
> > > > > -			memcpy(buf, eld, min(max_bytes, ret));
> > > > > -		}
> > > > > +
> > > > > +	intel_encoder = get_saved_encoder(dev_priv->av_enc_map, port, dev_id);
> > > > > +	if (!intel_encoder) {
> > > > > +		mutex_unlock(&dev_priv->av_mutex);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	ret = 0;
> > > > > +	*enabled = intel_encoder->audio_connector != NULL;
> > > > > +	if (*enabled) {
> > > > > +		eld = intel_encoder->audio_connector->eld;
> > > > > +		ret = drm_eld_size(eld);
> > > > > +		memcpy(buf, eld, min(max_bytes, ret));
> > > > >  	}
> > > > >  
> > > > >  	mutex_unlock(&dev_priv->av_mutex);
> > > > > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > > > > index b46fa0e..1419c98 100644
> > > > > --- a/include/drm/i915_component.h
> > > > > +++ b/include/drm/i915_component.h
> > > > > @@ -64,7 +64,7 @@ struct i915_audio_component_ops {
> > > > >  	 * Called from audio driver. After audio driver sets the
> > > > >  	 * sample rate, it will call this function to set n/cts
> > > > >  	 */
> > > > > -	int (*sync_audio_rate)(struct device *, int port, int rate);
> > > > > +	int (*sync_audio_rate)(struct device *, int port, int dev_id, int rate);
> > > > >  	/**
> > > > >  	 * @get_eld: fill the audio state and ELD bytes for the given port
> > > > >  	 *
> > > > > @@ -77,7 +77,7 @@ struct i915_audio_component_ops {
> > > > >  	 * Note that the returned size may be over @max_bytes.  Then it
> > > > >  	 * implies that only a part of ELD has been copied to the buffer.
> > > > >  	 */
> > > > > -	int (*get_eld)(struct device *, int port, bool *enabled,
> > > > > +	int (*get_eld)(struct device *, int port, int dev_id, bool *enabled,
> > > > >  		       unsigned char *buf, int max_bytes);
> > > > >  };
> > > > >  
> > > > > @@ -97,7 +97,7 @@ struct i915_audio_component_audio_ops {
> > > > >  	 * status accordingly (even when the HDA controller is in power save
> > > > >  	 * mode).
> > > > >  	 */
> > > > > -	void (*pin_eld_notify)(void *audio_ptr, int port);
> > > > > +	void (*pin_eld_notify)(void *audio_ptr, int port, int dev_id);
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
> > > > > index 796cabf..5ab972e 100644
> > > > > --- a/include/sound/hda_i915.h
> > > > > +++ b/include/sound/hda_i915.h
> > > > > @@ -10,8 +10,9 @@
> > > > >  int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
> > > > >  int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
> > > > >  void snd_hdac_i915_set_bclk(struct hdac_bus *bus);
> > > > > -int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid, int rate);
> > > > > -int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid,
> > > > > +int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid,
> > > > > +			     int dev_id, int rate);
> > > > > +int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid, int dev_id,
> > > > >  			   bool *audio_enabled, char *buffer, int max_bytes);
> > > > >  int snd_hdac_i915_init(struct hdac_bus *bus);
> > > > >  int snd_hdac_i915_exit(struct hdac_bus *bus);
> > > > > @@ -29,13 +30,13 @@ static inline void snd_hdac_i915_set_bclk(struct hdac_bus *bus)
> > > > >  {
> > > > >  }
> > > > >  static inline int snd_hdac_sync_audio_rate(struct hdac_device *codec,
> > > > > -					   hda_nid_t nid, int rate)
> > > > > +					   hda_nid_t nid, int dev_id, int rate)
> > > > >  {
> > > > >  	return 0;
> > > > >  }
> > > > >  static inline int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid,
> > > > > -					 bool *audio_enabled, char *buffer,
> > > > > -					 int max_bytes)
> > > > > +					 int dev_id, bool *audio_enabled,
> > > > > +					 char *buffer, int max_bytes)
> > > > >  {
> > > > >  	return -ENODEV;
> > > > >  }
> > > > > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > > > > index c9af022..6ea63ac 100644
> > > > > --- a/sound/hda/hdac_i915.c
> > > > > +++ b/sound/hda/hdac_i915.c
> > > > > @@ -201,7 +201,8 @@ static int pin2port(struct hdac_device *codec, hda_nid_t pin_nid)
> > > > >   * This function sets N/CTS value based on the given sample rate.
> > > > >   * Returns zero for success, or a negative error code.
> > > > >   */
> > > > > -int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid, int rate)
> > > > > +int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid,
> > > > > +			     int dev_id, int rate)
> > > > >  {
> > > > >  	struct hdac_bus *bus = codec->bus;
> > > > >  	struct i915_audio_component *acomp = bus->audio_component;
> > > > > @@ -212,7 +213,7 @@ int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid, int rate)
> > > > >  	port = pin2port(codec, nid);
> > > > >  	if (port < 0)
> > > > >  		return -EINVAL;
> > > > > -	return acomp->ops->sync_audio_rate(acomp->dev, port, rate);
> > > > > +	return acomp->ops->sync_audio_rate(acomp->dev, port, dev_id, rate);
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate);
> > > > >  
> > > > > @@ -236,7 +237,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate);
> > > > >   * thus it may be over @max_bytes.  If it's over @max_bytes, it implies
> > > > >   * that only a part of ELD bytes have been fetched.
> > > > >   */
> > > > > -int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid,
> > > > > +int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid, int dev_id,
> > > > >  			   bool *audio_enabled, char *buffer, int max_bytes)
> > > > >  {
> > > > >  	struct hdac_bus *bus = codec->bus;
> > > > > @@ -249,7 +250,7 @@ int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid,
> > > > >  	port = pin2port(codec, nid);
> > > > >  	if (port < 0)
> > > > >  		return -EINVAL;
> > > > > -	return acomp->ops->get_eld(acomp->dev, port, audio_enabled,
> > > > > +	return acomp->ops->get_eld(acomp->dev, port, dev_id, audio_enabled,
> > > > >  				   buffer, max_bytes);
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_eld);
> > > > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > > > index d0d5ad8..077d48a 100644
> > > > > --- a/sound/pci/hda/patch_hdmi.c
> > > > > +++ b/sound/pci/hda/patch_hdmi.c
> > > > > @@ -1485,7 +1485,7 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
> > > > >  
> > > > >  	mutex_lock(&per_pin->lock);
> > > > >  	eld->monitor_present = false;
> > > > > -	size = snd_hdac_acomp_get_eld(&codec->core, per_pin->pin_nid,
> > > > > +	size = snd_hdac_acomp_get_eld(&codec->core, per_pin->pin_nid, -1,
> > > > >  				      &eld->monitor_present, eld->eld_buffer,
> > > > >  				      ELD_MAX_SIZE);
> > > > >  	if (size > 0) {
> > > > > @@ -1739,7 +1739,8 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> > > > >  	/* Call sync_audio_rate to set the N/CTS/M manually if necessary */
> > > > >  	/* Todo: add DP1.2 MST audio support later */
> > > > >  	if (codec_has_acomp(codec))
> > > > > -		snd_hdac_sync_audio_rate(&codec->core, pin_nid, runtime->rate);
> > > > > +		snd_hdac_sync_audio_rate(&codec->core, pin_nid, -1,
> > > > > +					 runtime->rate);
> > > > >  
> > > > >  	non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
> > > > >  	mutex_lock(&per_pin->lock);
> > > > > @@ -2285,7 +2286,7 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
> > > > >  	snd_hda_codec_set_power_to_all(codec, fg, power_state);
> > > > >  }
> > > > >  
> > > > > -static void intel_pin_eld_notify(void *audio_ptr, int port)
> > > > > +static void intel_pin_eld_notify(void *audio_ptr, int port, int dev_id)
> > > > >  {
> > > > >  	struct hda_codec *codec = audio_ptr;
> > > > >  	int pin_nid;
> > > > > -- 
> > > > > 2.5.0
> > > > 
> > > 
> > 
> 

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