Re: [alsa-devel] [PATCH v4] drm/i915/dp: DP audio API changes for MST

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

 



> -----Original Message-----
> From: alsa-devel-bounces@xxxxxxxxxxxxxxxx [mailto:alsa-devel-
> bounces@xxxxxxxxxxxxxxxx] On Behalf Of Pandiyan, Dhinakaran
> Sent: Thursday, September 1, 2016 3:48 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; jim.bride@xxxxxxxxxxxxxxx;
> Kp, Jeeja <jeeja.kp@xxxxxxxxx>; libin.yang@xxxxxxxxxxxxxxx;
> ville.syrjala@xxxxxxxxxxxxxxx
> Subject: Re: [alsa-devel] [PATCH v4] drm/i915/dp: DP audio API changes for
> MST
> 
> The changes in this version are primarily in i915. I have carried over Takashi's
> R-B from the previous version and removed Ville's.
> 
> ________________________________________
> From: Pandiyan, Dhinakaran
> Sent: Thursday, September 01, 2016 12:50 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: libin.yang@xxxxxxxxxxxxxxx; jim.bride@xxxxxxxxxxxxxxx;
> ville.syrjala@xxxxxxxxxxxxxxx; tiwai@xxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Kp,
> Jeeja; Pandiyan, Dhinakaran
> Subject: [PATCH v4] drm/i915/dp: DP audio API changes for MST
> 
> DP MST provides the capability to send multiple video and audio streams
> through a single port. This requires the API's between i915 and audio drivers
> to distinguish between multiple audio capable displays that can be connected
> to a port. Currently only the port identity is shared in the APIs. This patch
> adds support for MST with an additional parameter 'int pipe'. The existing
> parameter 'port' does not change it's meaning.
> 
> pipe =
>         MST     : display 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 pipe,
> +            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 pipe,
> +                      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 pipe);
> 
> This patch makes dummy changes in the audio drivers (thanks Libin) for build
> to succeed. The audio side drivers will send the right 'pipe' values for MST in
> patches that will follow.
> 
> v2:
> Renamed the new API parameter from 'dev_id' to 'pipe'. (Jim, Ville) Included
> Asoc driver API compatibility changes from Jeeja.
> Added WARN_ON() for invalid pipe in get_saved_encoder(). (Takashi) Added
> comment for av_enc_map[] definition. (Takashi)
> 
> v3:
> Fixed logic error introduced while renaming 'dev_id' as 'pipe' (Ville) Renamed
> get_saved_encoder() to get_saved_enc() to reduce line length
> 
> v4:
> Rebased.
> Parameter check for pipe < -1 values in get_saved_enc() (Ville) Switched to
> for_each_pipe() in get_saved_enc() (Ville) Renamed 'pipe' to 'dev_id' in
> audio side code (Takashi)
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Reviewed-by: Takashi Iwai <tiwai@xxxxxxx>
> 
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  3 +-
>  drivers/gpu/drm/i915/intel_audio.c | 94 ++++++++++++++++++++++++++-----
> -------
>  include/drm/i915_component.h       |  6 +--
>  include/sound/hda_i915.h           | 11 +++--
>  sound/hda/hdac_i915.c              | 16 ++++---
>  sound/pci/hda/patch_hdmi.c         |  7 +--
>  sound/soc/codecs/hdac_hdmi.c       |  2 +-
>  7 files changed, 92 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index aaa9c60..dac81c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2056,7 +2056,8 @@ struct drm_i915_private {
>         /* perform PHY state sanity checks? */
>         bool chv_phy_assert[2];
> 
> -       struct intel_encoder *dig_port_map[I915_MAX_PORTS];
> +       /* Used to save the pipe-to-encoder mapping for audio */
> +       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 40fbdd8..9583f43 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -491,6 +491,7 @@ void intel_audio_codec_enable(struct intel_encoder
> *intel_encoder)
>         struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>         struct i915_audio_component *acomp = dev_priv->audio_component;
>         enum port port = intel_encoder->port;
> +       enum pipe pipe = crtc->pipe;
> 
>         connector = drm_select_eld(encoder);
>         if (!connector)
> @@ -515,12 +516,18 @@ void intel_audio_codec_enable(struct
> intel_encoder *intel_encoder)
> 
>         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);
> 
> +       /* audio drivers expect pipe = -1 to indicate Non-MST cases */
> +       if (intel_encoder->type != INTEL_OUTPUT_DP_MST)
> +               pipe = -1;
> +
>         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, (int)
> + pipe);
>  }
> 
>  /**
> @@ -536,17 +543,24 @@ void intel_audio_codec_disable(struct
> intel_encoder *intel_encoder)
>         struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>         struct i915_audio_component *acomp = dev_priv->audio_component;
>         enum port port = intel_encoder->port;
> +       struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> +       enum pipe pipe = crtc->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);
> 
> +       /* audio drivers expect pipe = -1 to indicate Non-MST cases */
> +       if (intel_encoder->type != INTEL_OUTPUT_DP_MST)
> +               pipe = -1;
> +
>         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, (int)
> + pipe);
>  }
> 
>  /**
> @@ -621,15 +635,40 @@ static int
> i915_audio_component_get_cdclk_freq(struct device *kdev)
>         return dev_priv->cdclk_freq;
>  }
> 
> -static int i915_audio_component_sync_audio_rate(struct device *kdev,
> -                                               int port, int rate)
> +static struct intel_encoder *get_saved_enc(struct drm_i915_private
> *dev_priv,
> +                                              int port, int pipe) {
> +
> +       if (WARN_ON(pipe >= I915_MAX_PIPES))
> +               return NULL;
> +
> +       /* MST */
> +       if (pipe >= 0)
> +               return dev_priv->av_enc_map[pipe];
> +
> +       /* Non-MST */
> +       for_each_pipe(dev_priv, pipe) {
> +               struct intel_encoder *encoder;
> +
> +               encoder = dev_priv->av_enc_map[pipe];
> +               if (encoder == NULL)
> +                       continue;
> +
> +               if (port == encoder->port)
> +                       return encoder;
> +       }
> +
> +       return NULL;
> +}
> +
> +static int i915_audio_component_sync_audio_rate(struct device *kdev, int
> port,
> +                                               int pipe, int rate)
>  {
>         struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
>         struct intel_encoder *intel_encoder;
>         struct intel_crtc *crtc;
>         struct drm_display_mode *mode;
>         struct i915_audio_component *acomp = dev_priv->audio_component;
> -       enum pipe pipe = INVALID_PIPE;
>         u32 tmp;
>         int n;
>         int err = 0;
> @@ -643,25 +682,20 @@ static int
> i915_audio_component_sync_audio_rate(struct device *kdev,
> 
>         i915_audio_component_get_power(kdev);
>         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_enc(dev_priv, port, pipe);
>         if (!intel_encoder || !intel_encoder->base.crtc ||
>             intel_encoder->type != INTEL_OUTPUT_HDMI) {
> -               DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> +               DRM_DEBUG_KMS("Not valid for port %c\n",
> + port_name(port));
>                 err = -ENODEV;
>                 goto unlock;
>         }
> +
> +       /* pipe passed from the audio driver will be -1 for Non-MST case
> + */
>         crtc = to_intel_crtc(intel_encoder->base.crtc);
>         pipe = crtc->pipe;
> -       if (pipe == INVALID_PIPE) {
> -               DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
> -               err = -ENODEV;
> -               goto unlock;
> -       }
> 
> -       DRM_DEBUG_KMS("pipe %c connects port %c\n",
> -                                 pipe_name(pipe), port_name(port));
>         mode = &crtc->config->base.adjusted_mode;
> 
>         /* port must be valid now, otherwise the pipe will be invalid */ @@ -
> 697,7 +731,7 @@ static int i915_audio_component_sync_audio_rate(struct
> device *kdev,  }
> 
>  static int i915_audio_component_get_eld(struct device *kdev, int port,
> -                                       bool *enabled,
> +                                       int pipe, bool *enabled,
>                                         unsigned char *buf, int max_bytes)  {
>         struct drm_i915_private *dev_priv = kdev_to_i915(kdev); @@ -706,16
> +740,20 @@ static int i915_audio_component_get_eld(struct device *kdev,
> int port,
>         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_enc(dev_priv, port, pipe);
> +       if (!intel_encoder) {
> +               DRM_DEBUG_KMS("Not valid for port %c\n", port_name(port));
> +               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..545c6e0 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 pipe, 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 pipe, 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 pipe);
>  };
> 
>  /**
> 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..1979b49 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -201,18 +201,20 @@ 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)

Suppose need to add the comments on parameter dev_id in the 
upper kernel doc

>  {
>         struct hdac_bus *bus = codec->bus;
>         struct i915_audio_component *acomp = bus->audio_component;
> -       int port;
> +       int port, pipe;
> 
>         if (!acomp || !acomp->ops || !acomp->ops->sync_audio_rate)
>                 return -ENODEV;
>         port = pin2port(codec, nid);
>         if (port < 0)
>                 return -EINVAL;
> -       return acomp->ops->sync_audio_rate(acomp->dev, port, rate);
> +       pipe = dev_id;
> +       return acomp->ops->sync_audio_rate(acomp->dev, port, pipe,
> + rate);
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate);
> 
> @@ -236,12 +238,12 @@
> 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)  {

Ditto

Regards,
Libin

>         struct hdac_bus *bus = codec->bus;
>         struct i915_audio_component *acomp = bus->audio_component;
> -       int port;
> +       int port, pipe;
> 
>         if (!acomp || !acomp->ops || !acomp->ops->get_eld)
>                 return -ENODEV;
> @@ -249,7 +251,9 @@ 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,
> +
> +       pipe = dev_id;
> +       return acomp->ops->get_eld(acomp->dev, port, pipe,
> + 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 56e5204..cf9bc042 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) {
> @@ -1744,7 +1744,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);
> @@ -2290,7 +2291,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 pipe)
>  {
>         struct hda_codec *codec = audio_ptr;
>         int pin_nid;
> diff --git a/sound/soc/codecs/hdac_hdmi.c
> b/sound/soc/codecs/hdac_hdmi.c index 4e181b2..dc0129b 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1368,7 +1368,7 @@ static int hdac_hdmi_parse_and_map_nid(struct
> hdac_ext_device *edev,
>         return hdac_hdmi_init_dai_map(edev);  }
> 
> -static void hdac_hdmi_eld_notify_cb(void *aptr, int port)
> +static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
>  {
>         struct hdac_ext_device *edev = aptr;
>         struct hdac_hdmi_priv *hdmi = edev->private_data;
> --
> 2.5.0
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
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