| Message ID | 1479193483-136968-2-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@intel.com> > > This patch extends the support of audio sample rate > sync up to DP MST. > > Signed-off-by: Libin Yang <libin.yang@intel.com> > --- > drivers/gpu/drm/i915/intel_audio.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > index c8a1345..88ed869 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -807,7 +807,8 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port, > intel_encoder = get_saved_enc(dev_priv, port, pipe); > if (!intel_encoder || !intel_encoder->base.crtc || > (intel_encoder->type != INTEL_OUTPUT_HDMI && > - intel_encoder->type != INTEL_OUTPUT_DP)) { > + intel_encoder->type != INTEL_OUTPUT_DP && > + intel_encoder->type != INTEL_OUTPUT_DP_MST)) { I think the better option is to make absolutely sure we never store other kinds of encoders in dev_priv->av_enc_map[pipe] to begin with. I think that's true already, but please add if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI && intel_encoder->type != INTEL_OUTPUT_DP && intel_encoder->type != INTEL_OUTPUT_DP_MST)) return; near the beginning of intel_audio_codec_enable(), and remove the type checks here. This reduces the confusion about different kinds of checks after calling get_saved_enc(). BR, Jani. > DRM_DEBUG_KMS("Not valid for port %c\n", port_name(port)); > err = -ENODEV; > goto unlock;
On Tue, Nov 29, 2016 at 06:33:50PM +0200, Jani Nikula wrote: > On Tue, 15 Nov 2016, libin.yang@intel.com wrote: > > From: Libin Yang <libin.yang@intel.com> > > > > This patch extends the support of audio sample rate > > sync up to DP MST. > > > > Signed-off-by: Libin Yang <libin.yang@intel.com> > > --- > > drivers/gpu/drm/i915/intel_audio.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > > index c8a1345..88ed869 100644 > > --- a/drivers/gpu/drm/i915/intel_audio.c > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > @@ -807,7 +807,8 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port, > > intel_encoder = get_saved_enc(dev_priv, port, pipe); > > if (!intel_encoder || !intel_encoder->base.crtc || > > (intel_encoder->type != INTEL_OUTPUT_HDMI && > > - intel_encoder->type != INTEL_OUTPUT_DP)) { > > + intel_encoder->type != INTEL_OUTPUT_DP && > > + intel_encoder->type != INTEL_OUTPUT_DP_MST)) { > > I think the better option is to make absolutely sure we never store > other kinds of encoders in dev_priv->av_enc_map[pipe] to begin with. I > think that's true already, but please add > > if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI && > intel_encoder->type != INTEL_OUTPUT_DP && > intel_encoder->type != INTEL_OUTPUT_DP_MST)) > return; > > near the beginning of intel_audio_codec_enable(), and remove the type > checks here. This reduces the confusion about different kinds of checks > after calling get_saved_enc(). This while encoder->type thing is pretty broken actually. Currently we're semi-randomly flipping DDI encoders between UNKNOWN,HDMI and DP. I want to eliminate that so that their type will always be just "DDI". For iguring out what kind of signal we are driving out you should be looking at crtc_state->output_types.
>-----Original Message----- >From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >Sent: Wednesday, November 30, 2016 12:34 AM >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> >Subject: Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for >DP MST > >On Tue, 15 Nov 2016, libin.yang@intel.com wrote: >> From: Libin Yang <libin.yang@intel.com> >> >> This patch extends the support of audio sample rate sync up to DP MST. >> >> Signed-off-by: Libin Yang <libin.yang@intel.com> >> --- >> drivers/gpu/drm/i915/intel_audio.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_audio.c >> b/drivers/gpu/drm/i915/intel_audio.c >> index c8a1345..88ed869 100644 >> --- a/drivers/gpu/drm/i915/intel_audio.c >> +++ b/drivers/gpu/drm/i915/intel_audio.c >> @@ -807,7 +807,8 @@ static int >i915_audio_component_sync_audio_rate(struct device *kdev, int port, >> intel_encoder = get_saved_enc(dev_priv, port, pipe); >> if (!intel_encoder || !intel_encoder->base.crtc || >> (intel_encoder->type != INTEL_OUTPUT_HDMI && >> - intel_encoder->type != INTEL_OUTPUT_DP)) { >> + intel_encoder->type != INTEL_OUTPUT_DP && >> + intel_encoder->type != INTEL_OUTPUT_DP_MST)) { > >I think the better option is to make absolutely sure we never store other kinds >of encoders in dev_priv->av_enc_map[pipe] to begin with. I think that's true >already, but please add > > if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI && > intel_encoder->type != INTEL_OUTPUT_DP && > intel_encoder->type != INTEL_OUTPUT_DP_MST)) > return; > >near the beginning of intel_audio_codec_enable(), and remove the type >checks here. This reduces the confusion about different kinds of checks after >calling get_saved_enc(). We can't put the check at the beginning of the function. We still need the check in the same place as we need get intel_encoder at first. Regards, Libin > >BR, >Jani. > > >> DRM_DEBUG_KMS("Not valid for port %c\n", >port_name(port)); >> err = -ENODEV; >> goto unlock; > >-- >Jani Nikula, Intel Open Source Technology Center
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Wednesday, November 30, 2016 12:50 AM >To: Jani Nikula <jani.nikula@linux.intel.com> >Cc: Yang, Libin <libin.yang@intel.com>; intel-gfx@lists.freedesktop.org; Vetter, >Daniel <daniel.vetter@intel.com>; Pandiyan, Dhinakaran ><dhinakaran.pandiyan@intel.com>; Kp, Jeeja <jeeja.kp@intel.com>; >tiwai@suse.de >Subject: Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for >DP MST > >On Tue, Nov 29, 2016 at 06:33:50PM +0200, Jani Nikula wrote: >> On Tue, 15 Nov 2016, libin.yang@intel.com wrote: >> > From: Libin Yang <libin.yang@intel.com> >> > >> > This patch extends the support of audio sample rate sync up to DP >> > MST. >> > >> > Signed-off-by: Libin Yang <libin.yang@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_audio.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c >> > b/drivers/gpu/drm/i915/intel_audio.c >> > index c8a1345..88ed869 100644 >> > --- a/drivers/gpu/drm/i915/intel_audio.c >> > +++ b/drivers/gpu/drm/i915/intel_audio.c >> > @@ -807,7 +807,8 @@ static int >i915_audio_component_sync_audio_rate(struct device *kdev, int port, >> > intel_encoder = get_saved_enc(dev_priv, port, pipe); >> > if (!intel_encoder || !intel_encoder->base.crtc || >> > (intel_encoder->type != INTEL_OUTPUT_HDMI && >> > - intel_encoder->type != INTEL_OUTPUT_DP)) { >> > + intel_encoder->type != INTEL_OUTPUT_DP && >> > + intel_encoder->type != INTEL_OUTPUT_DP_MST)) { >> >> I think the better option is to make absolutely sure we never store >> other kinds of encoders in dev_priv->av_enc_map[pipe] to begin with. I >> think that's true already, but please add >> >> if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI && >> intel_encoder->type != INTEL_OUTPUT_DP && >> intel_encoder->type != INTEL_OUTPUT_DP_MST)) >> return; >> >> near the beginning of intel_audio_codec_enable(), and remove the type >> checks here. This reduces the confusion about different kinds of >> checks after calling get_saved_enc(). > >This while encoder->type thing is pretty broken actually. Currently we're semi- >randomly flipping DDI encoders between UNKNOWN,HDMI and DP. >I want to eliminate that so that their type will always be just "DDI". >For iguring out what kind of signal we are driving out you should be looking at >crtc_state->output_types. Do you mean the code should be like: @@ -807,10 +807,7 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port, /* 1. get the pipe */ intel_encoder = get_saved_enc(dev_priv, port, pipe); - if (!intel_encoder || !intel_encoder->base.crtc || - (intel_encoder->type != INTEL_OUTPUT_HDMI && - intel_encoder->type != INTEL_OUTPUT_DP && - intel_encoder->type != INTEL_OUTPUT_DP_MST)) { + if (!intel_encoder || !intel_encoder->base.crtc) { DRM_DEBUG_KMS("Not valid for port %c\n", port_name(port)); err = -ENODEV; goto unlock; @@ -819,7 +816,12 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port, /* pipe passed from the audio driver will be -1 for Non-MST case */ crtc = to_intel_crtc(intel_encoder->base.crtc); crtc_state = crtc->config; + if (WARN_ON((crtc_state->output_types & + ((1 << INTEL_OUTPUT_HDMI) | + (1 << INTEL_OUTPUT_DP) | + (1 << INTEL_OUTPUT_DP_MST))) == 0)) + return 0; + pipe = crtc->pipe; I did the test, and it seems intel_encoder->type can show the correct type on my platform. It is similar with crtc_state->output_types. Regards, Libin > >-- >Ville Syrjälä >Intel OTC
On Wed, 30 Nov 2016, "Yang, Libin" <libin.yang@intel.com> wrote: >>-----Original Message----- >>From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >>Sent: Wednesday, November 30, 2016 12:34 AM >>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> >>Subject: Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for >>DP MST >> >>On Tue, 15 Nov 2016, libin.yang@intel.com wrote: >>> From: Libin Yang <libin.yang@intel.com> >>> >>> This patch extends the support of audio sample rate sync up to DP MST. >>> >>> Signed-off-by: Libin Yang <libin.yang@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_audio.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_audio.c >>> b/drivers/gpu/drm/i915/intel_audio.c >>> index c8a1345..88ed869 100644 >>> --- a/drivers/gpu/drm/i915/intel_audio.c >>> +++ b/drivers/gpu/drm/i915/intel_audio.c >>> @@ -807,7 +807,8 @@ static int >>i915_audio_component_sync_audio_rate(struct device *kdev, int port, >>> intel_encoder = get_saved_enc(dev_priv, port, pipe); >>> if (!intel_encoder || !intel_encoder->base.crtc || >>> (intel_encoder->type != INTEL_OUTPUT_HDMI && >>> - intel_encoder->type != INTEL_OUTPUT_DP)) { >>> + intel_encoder->type != INTEL_OUTPUT_DP && >>> + intel_encoder->type != INTEL_OUTPUT_DP_MST)) { >> >>I think the better option is to make absolutely sure we never store other kinds >>of encoders in dev_priv->av_enc_map[pipe] to begin with. I think that's true >>already, but please add >> >> if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI && >> intel_encoder->type != INTEL_OUTPUT_DP && >> intel_encoder->type != INTEL_OUTPUT_DP_MST)) >> return; >> >>near the beginning of intel_audio_codec_enable(), and remove the type >>checks here. This reduces the confusion about different kinds of checks after >>calling get_saved_enc(). > > We can't put the check at the beginning of the function. > We still need the check in the same place as we need get intel_encoder at first. We do not need to duplicate the checks here if we ensure only good stuff is ever put into av_enc_map. > > Regards, > Libin > >> >>BR, >>Jani. >> >> >>> DRM_DEBUG_KMS("Not valid for port %c\n", >>port_name(port)); >>> err = -ENODEV; >>> goto unlock; >> >>-- >>Jani Nikula, Intel Open Source Technology Center
>-----Original Message----- >From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >Sent: Wednesday, November 30, 2016 11:52 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 >Subject: RE: [PATCH 2/2] drm/i915/audio: Extend audio sync rate support for >DP MST > >On Wed, 30 Nov 2016, "Yang, Libin" <libin.yang@intel.com> wrote: >>>-----Original Message----- >>>From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >>>Sent: Wednesday, November 30, 2016 12:34 AM >>>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> >>>Subject: Re: [PATCH 2/2] drm/i915/audio: Extend audio sync rate >>>support for DP MST >>> >>>On Tue, 15 Nov 2016, libin.yang@intel.com wrote: >>>> From: Libin Yang <libin.yang@intel.com> >>>> >>>> This patch extends the support of audio sample rate sync up to DP MST. >>>> >>>> Signed-off-by: Libin Yang <libin.yang@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_audio.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_audio.c >>>> b/drivers/gpu/drm/i915/intel_audio.c >>>> index c8a1345..88ed869 100644 >>>> --- a/drivers/gpu/drm/i915/intel_audio.c >>>> +++ b/drivers/gpu/drm/i915/intel_audio.c >>>> @@ -807,7 +807,8 @@ static int >>>i915_audio_component_sync_audio_rate(struct device *kdev, int port, >>>> intel_encoder = get_saved_enc(dev_priv, port, pipe); >>>> if (!intel_encoder || !intel_encoder->base.crtc || >>>> (intel_encoder->type != INTEL_OUTPUT_HDMI && >>>> - intel_encoder->type != INTEL_OUTPUT_DP)) { >>>> + intel_encoder->type != INTEL_OUTPUT_DP && >>>> + intel_encoder->type != INTEL_OUTPUT_DP_MST)) { >>> >>>I think the better option is to make absolutely sure we never store >>>other kinds of encoders in dev_priv->av_enc_map[pipe] to begin with. I >>>think that's true already, but please add >>> >>> if (WARN_ON(intel_encoder->type != INTEL_OUTPUT_HDMI && >>> intel_encoder->type != INTEL_OUTPUT_DP && >>> intel_encoder->type != INTEL_OUTPUT_DP_MST)) >>> return; >>> >>>near the beginning of intel_audio_codec_enable(), and remove the type >>>checks here. This reduces the confusion about different kinds of >>>checks after calling get_saved_enc(). >> >> We can't put the check at the beginning of the function. >> We still need the check in the same place as we need get intel_encoder at >first. > >We do not need to duplicate the checks here if we ensure only good stuff is >ever put into av_enc_map. Yes, I agree. I will remove the type judgement. Regards, Libin > >> >> Regards, >> Libin >> >>> >>>BR, >>>Jani. >>> >>> >>>> DRM_DEBUG_KMS("Not valid for port %c\n", >>>port_name(port)); >>>> err = -ENODEV; >>>> goto unlock; >>> >>>-- >>>Jani Nikula, Intel Open Source Technology Center > >-- >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 c8a1345..88ed869 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -807,7 +807,8 @@ static int i915_audio_component_sync_audio_rate(struct device *kdev, int port, intel_encoder = get_saved_enc(dev_priv, port, pipe); if (!intel_encoder || !intel_encoder->base.crtc || (intel_encoder->type != INTEL_OUTPUT_HDMI && - intel_encoder->type != INTEL_OUTPUT_DP)) { + intel_encoder->type != INTEL_OUTPUT_DP && + intel_encoder->type != INTEL_OUTPUT_DP_MST)) { DRM_DEBUG_KMS("Not valid for port %c\n", port_name(port)); err = -ENODEV; goto unlock;