drm/i915/ilk: Wait one vblank before enabling audio

Submitted by cpaul@redhat.com on May 20, 2016, 9:36 p.m.

Details

Message ID 1463780200-22813-1-git-send-email-cpaul@redhat.com
State New
Headers show
Series "drm/i915/ilk: Wait one vblank before enabling audio" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

cpaul@redhat.com May 20, 2016, 9:36 p.m.
We no longer call ilk_audio_codec_enable() while we have vblanks
disabled. As such, we can finally fix this and stop the occasional pipe
underruns I'm seeing on this Dell OptiPlex 990.

Cc: stable@vger.kernel.org
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 7d281b4..0d685fe 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -423,12 +423,8 @@  static void ilk_audio_codec_enable(struct drm_connector *connector,
 	if (WARN_ON(port == PORT_A))
 		return;
 
-	/*
-	 * FIXME: We're supposed to wait for vblank here, but we have vblanks
-	 * disabled during the mode set. The proper fix would be to push the
-	 * rest of the setup into a vblank work item, queued here, but the
-	 * infrastructure is not there yet.
-	 */
+	/* Need to wait one vblank before enabling audio */
+	intel_wait_for_vblank(connector->dev, pipe);
 
 	if (HAS_PCH_IBX(connector->dev)) {
 		hdmiw_hdmiedid = IBX_HDMIW_HDMIEDID(pipe);

Comments

On Sat, 21 May 2016, Lyude <cpaul@redhat.com> wrote:
> We no longer call ilk_audio_codec_enable() while we have vblanks
> disabled. As such, we can finally fix this and stop the occasional pipe
> underruns I'm seeing on this Dell OptiPlex 990.
>
> Cc: stable@vger.kernel.org

Even if this were the right fix now, I'd be wary of adding a blanket cc:
stable to kernels where we still have vblanks disabled when this
function gets called.

Whoever pushes this, please ensure the cc: stable gets dropped from the
commit.

Thanks,
Jani.

> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 7d281b4..0d685fe 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -423,12 +423,8 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>  	if (WARN_ON(port == PORT_A))
>  		return;
>  
> -	/*
> -	 * FIXME: We're supposed to wait for vblank here, but we have vblanks
> -	 * disabled during the mode set. The proper fix would be to push the
> -	 * rest of the setup into a vblank work item, queued here, but the
> -	 * infrastructure is not there yet.
> -	 */
> +	/* Need to wait one vblank before enabling audio */
> +	intel_wait_for_vblank(connector->dev, pipe);
>  
>  	if (HAS_PCH_IBX(connector->dev)) {
>  		hdmiw_hdmiedid = IBX_HDMIW_HDMIEDID(pipe);
On Fri, May 20, 2016 at 05:36:40PM -0400, Lyude wrote:
> We no longer call ilk_audio_codec_enable() while we have vblanks
> disabled. As such, we can finally fix this and stop the occasional pipe
> underruns I'm seeing on this Dell OptiPlex 990.

Hmm. Are you still getting underruns on -nightly?

I basically tried this same thing (+ a bunch of other tweaks to the
audio enable sequence) when I was hunting the remaining underruns on
my ILK, but in the end audio was a red herring. So I never found
any real benefit from extra vblank waits in the audio enable sequence.
They did appear to help sometimes, but with a enough repetitions it
still failed.

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 7d281b4..0d685fe 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -423,12 +423,8 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>  	if (WARN_ON(port == PORT_A))
>  		return;
>  
> -	/*
> -	 * FIXME: We're supposed to wait for vblank here, but we have vblanks
> -	 * disabled during the mode set. The proper fix would be to push the
> -	 * rest of the setup into a vblank work item, queued here, but the
> -	 * infrastructure is not there yet.
> -	 */
> +	/* Need to wait one vblank before enabling audio */
> +	intel_wait_for_vblank(connector->dev, pipe);
>  
>  	if (HAS_PCH_IBX(connector->dev)) {
>  		hdmiw_hdmiedid = IBX_HDMIW_HDMIEDID(pipe);
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, 2016-05-23 at 21:06 +0300, Ville Syrjälä wrote:
> On Fri, May 20, 2016 at 05:36:40PM -0400, Lyude wrote:
> > 
> > We no longer call ilk_audio_codec_enable() while we have vblanks
> > disabled. As such, we can finally fix this and stop the occasional pipe
> > underruns I'm seeing on this Dell OptiPlex 990.
> Hmm. Are you still getting underruns on -nightly?
For me the problem isn't just underruns, a lot of times the modeset results in a
blank monitor without this fix… afaict waiting for 1 vblank seems to fix the
issue entirely here, but I'll check more thoroughly in a bit and update you if I
manage to get it to underrun.

Cheers,
Lyude

> 
> I basically tried this same thing (+ a bunch of other tweaks to the
> audio enable sequence) when I was hunting the remaining underruns on
> my ILK, but in the end audio was a red herring. So I never found
> any real benefit from extra vblank waits in the audio enable sequence.
> They did appear to help sometimes, but with a enough repetitions it
> still failed.
> 
> > 
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Lyude <cpaul@redhat.com>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 7d281b4..0d685fe 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -423,12 +423,8 @@ static void ilk_audio_codec_enable(struct drm_connector
> > *connector,
> >  	if (WARN_ON(port == PORT_A))
> >  		return;
> >  
> > -	/*
> > -	 * FIXME: We're supposed to wait for vblank here, but we have
> > vblanks
> > -	 * disabled during the mode set. The proper fix would be to push
> > the
> > -	 * rest of the setup into a vblank work item, queued here, but the
> > -	 * infrastructure is not there yet.
> > -	 */
> > +	/* Need to wait one vblank before enabling audio */
> > +	intel_wait_for_vblank(connector->dev, pipe);
> >  
> >  	if (HAS_PCH_IBX(connector->dev)) {
> >  		hdmiw_hdmiedid = IBX_HDMIW_HDMIEDID(pipe);
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, 23 May 2016, Lyude Paul <cpaul@redhat.com> wrote:
> On Mon, 2016-05-23 at 21:06 +0300, Ville Syrjälä wrote:
>> On Fri, May 20, 2016 at 05:36:40PM -0400, Lyude wrote:
>> > 
>> > We no longer call ilk_audio_codec_enable() while we have vblanks
>> > disabled. As such, we can finally fix this and stop the occasional pipe
>> > underruns I'm seeing on this Dell OptiPlex 990.
>> Hmm. Are you still getting underruns on -nightly?
> For me the problem isn't just underruns, a lot of times the modeset
> results in a blank monitor without this fix… afaict waiting for 1
> vblank seems to fix the issue entirely here, but I'll check more
> thoroughly in a bit and update you if I manage to get it to underrun.

I suppose the question is, do you see the problem also when you rule out
the audio stuff altogether? I.e. does the vblank wait happen to help
independent of the audio?

BR,
Jani.


>
> Cheers,
> Lyude
>
>> 
>> I basically tried this same thing (+ a bunch of other tweaks to the
>> audio enable sequence) when I was hunting the remaining underruns on
>> my ILK, but in the end audio was a red herring. So I never found
>> any real benefit from extra vblank waits in the audio enable sequence.
>> They did appear to help sometimes, but with a enough repetitions it
>> still failed.
>> 
>> > 
>> > 
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Lyude <cpaul@redhat.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_audio.c | 8 ++------
>> >  1 file changed, 2 insertions(+), 6 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> > b/drivers/gpu/drm/i915/intel_audio.c
>> > index 7d281b4..0d685fe 100644
>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > @@ -423,12 +423,8 @@ static void ilk_audio_codec_enable(struct drm_connector
>> > *connector,
>> >  	if (WARN_ON(port == PORT_A))
>> >  		return;
>> >  
>> > -	/*
>> > -	 * FIXME: We're supposed to wait for vblank here, but we have
>> > vblanks
>> > -	 * disabled during the mode set. The proper fix would be to push
>> > the
>> > -	 * rest of the setup into a vblank work item, queued here, but the
>> > -	 * infrastructure is not there yet.
>> > -	 */
>> > +	/* Need to wait one vblank before enabling audio */
>> > +	intel_wait_for_vblank(connector->dev, pipe);
>> >  
>> >  	if (HAS_PCH_IBX(connector->dev)) {
>> >  		hdmiw_hdmiedid = IBX_HDMIW_HDMIEDID(pipe);
>> > -- 
>> > 2.5.5
>> > 
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel