| Message ID | 1479193483-136968-1-git-send-email-libin.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 15 Nov 2016, libin.yang@intel.com wrote: > From: Libin Yang <libin.yang@linux.intel.com> > > 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@linux.intel.com> > --- > 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*. 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? 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; > }
>-----Original Message----- >From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >Sent: Friday, November 25, 2016 10:46 PM >To: Yang, Libin <libin.yang@intel.com>; intel-gfx@lists.freedesktop.org; >ville.syrjala@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>; >Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>; Kp, Jeeja ><jeeja.kp@intel.com>; tiwai@suse.de >Cc: Yang, Libin <libin.yang@intel.com>; Libin Yang <libin.yang@linux.intel.com> >Subject: Re: [PATCH 1/2] drm/i915/audio: extend get_saved_enc() to support >more scenarios > >On Tue, 15 Nov 2016, libin.yang@intel.com wrote: >> From: Libin Yang <libin.yang@linux.intel.com> >> >> 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@linux.intel.com> >> --- >> 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. > >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. 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
On Tue, 29 Nov 2016, "Yang, Libin" <libin.yang@intel.com> wrote: >>-----Original Message----- >>From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >>Sent: Friday, November 25, 2016 10:46 PM >>To: Yang, Libin <libin.yang@intel.com>; intel-gfx@lists.freedesktop.org; >>ville.syrjala@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>; >>Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>; Kp, Jeeja >><jeeja.kp@intel.com>; tiwai@suse.de >>Cc: Yang, Libin <libin.yang@intel.com>; Libin Yang <libin.yang@linux.intel.com> >>Subject: Re: [PATCH 1/2] drm/i915/audio: extend get_saved_enc() to support >>more scenarios >> >>On Tue, 15 Nov 2016, libin.yang@intel.com wrote: >>> From: Libin Yang <libin.yang@linux.intel.com> >>> >>> 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@linux.intel.com> >>> --- >>> 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
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 + */ 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; }