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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


> > 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@xxxxxxxxx>
> > > > ---
> > > >  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
> > > 
> > 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux