On Tue, 29 Nov 2016, "Yang, Libin" <libin.yang@xxxxxxxxx> wrote: >>-----Original Message----- >>From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >>Sent: Friday, November 25, 2016 10:46 PM >>To: Yang, Libin <libin.yang@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >>ville.syrjala@xxxxxxxxxxxxxxx; Vetter, Daniel <daniel.vetter@xxxxxxxxx>; >>Pandiyan, Dhinakaran <dhinakaran.pandiyan@xxxxxxxxx>; Kp, Jeeja >><jeeja.kp@xxxxxxxxx>; tiwai@xxxxxxx >>Cc: Yang, Libin <libin.yang@xxxxxxxxx>; Libin Yang <libin.yang@xxxxxxxxxxxxxxx> >>Subject: Re: [PATCH 1/2] drm/i915/audio: extend get_saved_enc() to support >>more scenarios >> >>On Tue, 15 Nov 2016, libin.yang@xxxxxxxxx wrote: >>> From: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> >>> >>> When bootup, audio driver may not know it is MST or not. The audio >>> driver will poll all the port & pipe combinations in either MST or >>> Non-MST mode. get_saved_enc() should handle this situation. >>> >>> Signed-off-by: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> >>> --- >>> drivers/gpu/drm/i915/intel_audio.c | 32 >>> ++++++++++++++++++++++++++++---- >>> 1 file changed, 28 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_audio.c >>> b/drivers/gpu/drm/i915/intel_audio.c >>> index 49f1053..c8a1345 100644 >>> --- a/drivers/gpu/drm/i915/intel_audio.c >>> +++ b/drivers/gpu/drm/i915/intel_audio.c >>> @@ -737,25 +737,49 @@ static int >>i915_audio_component_get_cdclk_freq(struct device *kdev) >>> return dev_priv->cdclk_freq; >>> } >>> >>> +/* >>> + * get the intel_encoder according to the parameter port and pipe >>> + * intel_encoder is saved by the index of pipe >>> + * MST & (pipe >= 0): return the av_enc_map[pipe], >>> + * when port is matched >>> + * MST & (pipe < 0): this is invalid >>> + * Non-MST & (pipe >= 0): only pipe = 0 (the first device entry) >>> + * will get the right intel_encoder with port matched >>> + * Non-MST & (pipe < 0): get the right intel_encoder with port >>> +matched >> >>This essentially describes the code in English. I can read the code. I need to >>know more about the higher level of what this does and *why*. > > In initialization, audio driver will call functions get_eld() and etc. But > at that time, audio driver may not know whether it is DP MST or not. > In the original function get_saved_enc(), if it is DP MST, it will set the pipe > to the correct value, otherwise, it will be set -1. > > Although audio driver can get the knowledge whether it is in DP MST > mode or not by reading the codec register. But it will drop performance > each time it call the get_eld and other functions. As gfx driver can easily > know whether it is in DP MST mode or not. I would like to extend the > get_saved_enc() function to handle the situation that audio driver still > send the device id info even it is in DP mode. In this situation, > get_saved_enc() will return the correct intel_encoder instead of panic. Please describe that in the commit message and/or comments in the patch. >> >>I also look at the call sites of get_saved_enc and wonder why they have >>different checks for the return values. Also patch 2/2 only modifies one of >>them, not both. Why? > > The 2 check should be similar? In sync_audio_rate(), it calls get_saved_enc() > and checked base.crtc, I think it is because it will use base.crtc later. > > And it will check the intel_encoder->type because this function only need > handle HDMI/DP/DP MST situation to setup the audio config. I think addressing my review comment on patch 2 will help this. BR, Jani. > > Regards, > Libin > >> >>So maybe I'm just too clueless about DP (MST) audio in general, and I >>wouldn't even get offended if you told me that. But I don't think the commit >>message and the comments in this patch properly help me either. >> >>BR, >>Jani. >> >> >>> + */ >>> static struct intel_encoder *get_saved_enc(struct drm_i915_private >>*dev_priv, >>> int port, int pipe) >>> { >>> + struct intel_encoder *encoder; >>> >>> if (WARN_ON(pipe >= I915_MAX_PIPES)) >>> return NULL; >>> >>> /* MST */ >>> - if (pipe >= 0) >>> - return dev_priv->av_enc_map[pipe]; >>> + if (pipe >= 0) { >>> + encoder = dev_priv->av_enc_map[pipe]; >>> + /* >>> + * when bootup, audio driver may not know it is >>> + * MST or not. So it will poll all the port & pipe >>> + * combinations >>> + */ >>> + if (encoder != NULL && encoder->port == port && >>> + encoder->type == INTEL_OUTPUT_DP_MST) >>> + return encoder; >>> + } >>> >>> /* Non-MST */ >>> - for_each_pipe(dev_priv, pipe) { >>> - struct intel_encoder *encoder; >>> + if (pipe > 0) >>> + return NULL; >>> >>> + for_each_pipe(dev_priv, pipe) { >>> encoder = dev_priv->av_enc_map[pipe]; >>> if (encoder == NULL) >>> continue; >>> >>> + if (encoder->type == INTEL_OUTPUT_DP_MST) >>> + continue; >>> + >>> if (port == encoder->port) >>> return encoder; >>> } >> >>-- >>Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
