On Thu, 2016-08-04 at 15:49 +0300, Ville Syrjälä wrote: > 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. >
Thanks for the detailed explanation, makes sense to change it to "pipe". I will send out a new version. Do you recommend I send this patch with "Prep. for DP audio MST support" series (https://lists.freedesktop.org/archives/intel-gfx/2016-August/102399.html)? This patch depends on the some of the patches in that. I had separated this patch out because of the audio driver changes in this. > > > > > > > > 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 <[email protected]> > > > > > > --- > > > > > > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
