| Message ID | 1396458534-23108-4-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 02 Apr 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > Needs to happen after clock is running or it doesn't behave correctly. Do you think this might fix some HDMI audio related bugs we have? Jani. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index fb9839b..a4ca63b6 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -669,8 +669,6 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder) > > I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val); > POSTING_READ(intel_hdmi->hdmi_reg); > - > - intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > } > > static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, > @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); > + struct drm_display_mode *adjusted_mode = > + &intel_crtc->config.adjusted_mode; > u32 temp; > u32 enable_bits = SDVO_ENABLE; > > + intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > + > if (intel_hdmi->has_audio) > enable_bits |= SDVO_AUDIO_ENABLE; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Apr 02, 2014 at 10:08:54AM -0700, Jesse Barnes wrote: > Needs to happen after clock is running or it doesn't behave correctly. Subject of the patch isn't correct. You enable it after the PLL, but still before the port gets enabled. Which I believe is the order we want. So you should just fix the patch subject. Apart from that I think it should work out just fine: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> BTW now that we do this, we should also disable the DIP after disabling the port. Otherwise when we move a pipe from a HDMI port to some other type of port we leave the DIP enabled and might continue sending infoframes to some unsuspecting sink. That bug has been present since forever, but now it could actually be fixed. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index fb9839b..a4ca63b6 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -669,8 +669,6 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder) > > I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val); > POSTING_READ(intel_hdmi->hdmi_reg); > - > - intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > } > > static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, > @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); > + struct drm_display_mode *adjusted_mode = > + &intel_crtc->config.adjusted_mode; > u32 temp; > u32 enable_bits = SDVO_ENABLE; > > + intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > + > if (intel_hdmi->has_audio) > enable_bits |= SDVO_AUDIO_ENABLE; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Apr 02, 2014 at 10:08:54AM -0700, Jesse Barnes wrote: > Needs to happen after clock is running or it doesn't behave correctly. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index fb9839b..a4ca63b6 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -669,8 +669,6 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder) > > I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val); > POSTING_READ(intel_hdmi->hdmi_reg); > - > - intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > } > > static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, > @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); > + struct drm_display_mode *adjusted_mode = > + &intel_crtc->config.adjusted_mode; > u32 temp; > u32 enable_bits = SDVO_ENABLE; > > + intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > + > if (intel_hdmi->has_audio) > enable_bits |= SDVO_AUDIO_ENABLE; That kind of change tends to freak out Paulo, our master of infoframes. Do doecs really state that this is how stuff should work in general, or is this just a gm45/vlv thing? Or vlv only? /me remembers how often we've burnt our hands here Thanks, Daniel
On Thu, 3 Apr 2014 17:19:56 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Apr 02, 2014 at 10:08:54AM -0700, Jesse Barnes wrote: > > Needs to happen after clock is running or it doesn't behave correctly. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > index fb9839b..a4ca63b6 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -669,8 +669,6 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder) > > > > I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val); > > POSTING_READ(intel_hdmi->hdmi_reg); > > - > > - intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > > } > > > > static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, > > @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder) > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); > > + struct drm_display_mode *adjusted_mode = > > + &intel_crtc->config.adjusted_mode; > > u32 temp; > > u32 enable_bits = SDVO_ENABLE; > > > > + intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > > + > > if (intel_hdmi->has_audio) > > enable_bits |= SDVO_AUDIO_ENABLE; > > That kind of change tends to freak out Paulo, our master of infoframes. Do > doecs really state that this is how stuff should work in general, or is > this just a gm45/vlv thing? Or vlv only? > > /me remembers how often we've burnt our hands here Hey infoframe emission was totally broken for awhile due to a generic change, and we didn't notice that right away. :) But yeah I'd prefer to test this on multiple platforms first, but don't have that capability. It does pass on BYT though, and the logic should be similar to IBX, so this change ought to be safe. It's easy to revert too and make platform specific if we get regression reports, but I expect it to fix weird issues instead of introducing new ones, based on the infoframe analyzer results we have from BYT.
On Thu, Apr 3, 2014 at 6:49 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, >> > @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder) >> > struct drm_i915_private *dev_priv = dev->dev_private; >> > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); >> > + struct drm_display_mode *adjusted_mode = >> > + &intel_crtc->config.adjusted_mode; >> > u32 temp; >> > u32 enable_bits = SDVO_ENABLE; >> > >> > + intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); >> > + >> > if (intel_hdmi->has_audio) >> > enable_bits |= SDVO_AUDIO_ENABLE; >> >> That kind of change tends to freak out Paulo, our master of infoframes. Do >> doecs really state that this is how stuff should work in general, or is >> this just a gm45/vlv thing? Or vlv only? >> >> /me remembers how often we've burnt our hands here > > Hey infoframe emission was totally broken for awhile due to a generic > change, and we didn't notice that right away. :) > > But yeah I'd prefer to test this on multiple platforms first, but don't > have that capability. It does pass on BYT though, and the logic should > be similar to IBX, so this change ought to be safe. It's easy to > revert too and make platform specific if we get regression reports, but > I expect it to fix weird issues instead of introducing new ones, based > on the infoframe analyzer results we have from BYT. Yeah I guess the number of users who actual use a gm45 or so with a TV is probably still bigger than all the byt platforms out there :( If Paulo can ack this I'll happily merge. Paulo, can you please take a quick look? Also you make this sound like it's a regression, but the patch is missing cc:stable and a sha1 citation of the offending commit. Jesse, can you please fix this? Thanks, Daniel
On Thu, 3 Apr 2014 22:55:24 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Apr 3, 2014 at 6:49 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > >> > static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, > >> > @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder) > >> > struct drm_i915_private *dev_priv = dev->dev_private; > >> > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > >> > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); > >> > + struct drm_display_mode *adjusted_mode = > >> > + &intel_crtc->config.adjusted_mode; > >> > u32 temp; > >> > u32 enable_bits = SDVO_ENABLE; > >> > > >> > + intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > >> > + > >> > if (intel_hdmi->has_audio) > >> > enable_bits |= SDVO_AUDIO_ENABLE; > >> > >> That kind of change tends to freak out Paulo, our master of infoframes. Do > >> doecs really state that this is how stuff should work in general, or is > >> this just a gm45/vlv thing? Or vlv only? > >> > >> /me remembers how often we've burnt our hands here > > > > Hey infoframe emission was totally broken for awhile due to a generic > > change, and we didn't notice that right away. :) > > > > But yeah I'd prefer to test this on multiple platforms first, but don't > > have that capability. It does pass on BYT though, and the logic should > > be similar to IBX, so this change ought to be safe. It's easy to > > revert too and make platform specific if we get regression reports, but > > I expect it to fix weird issues instead of introducing new ones, based > > on the infoframe analyzer results we have from BYT. > > Yeah I guess the number of users who actual use a gm45 or so with a TV > is probably still bigger than all the byt platforms out there :( If > Paulo can ack this I'll happily merge. Paulo, can you please take a > quick look? > > Also you make this sound like it's a regression, but the patch is > missing cc:stable and a sha1 citation of the offending commit. Jesse, > can you please fix this? No it's not a regression, we had an earlier regression on infoframes though that seemed to have gone unnoticed for awhile when looking at the logs and testing here... It's fixed now though.
2014-04-02 14:08 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > Needs to happen after clock is running or it doesn't behave correctly. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index fb9839b..a4ca63b6 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -669,8 +669,6 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder) > > I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val); > POSTING_READ(intel_hdmi->hdmi_reg); > - > - intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > } > > static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, > @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); > + struct drm_display_mode *adjusted_mode = > + &intel_crtc->config.adjusted_mode; > u32 temp; > u32 enable_bits = SDVO_ENABLE; > > + intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > + > if (intel_hdmi->has_audio) > enable_bits |= SDVO_AUDIO_ENABLE; > Due to my past, I am not a person who should be reviewing these patches because I have a tendency to fear that a single variable rename will break everybody's machines at this point of the code :) My problem with this patch is that now we do the same thing at 3 different points depending on the platform: - For VLV, we will enable infoframes at encoder->pre_enable - For ILK and friends, we will enable infoframes at encoder->enable - For HSW+, we will still enable infoframes at encoder->modeset I'd really like to have a standard behavior here :) Also, for ILK/SNB/IVB, we run encoder->enable after the very end of the modeset sequence, and I suspect the pipe is already running at that point (since the we already called intel_enable_pipe, we already ran intel_enable_planes, and we also called ironlake_pch_enable). For that case, we probably need to implement all those "wait for the exact VSYNC moment before touching register" restrictions that are described on the spec. Or we find a way to also call set_infoframes at pre_enable on these platforms. Did we test these patches on the ILK+ family? I also remember a lot of bugs that would only be seen after suspend/resume, so I suggest testing it too :) The good thing is that moving register writes away from the mode_set callbacks is good IMHO since at some point we'll want crtc_enable to fully setup the HW, so runtime PM will be able to work without requiring a crtc_mode_set call. But you need to patch HSW+ too for that :P This is not an ack, nack nor a reviewed-by :) If you are still confident, feel free to go ahead. Thanks, Paulo > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Apr 4, 2014 at 11:11 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > Due to my past, I am not a person who should be reviewing these > patches because I have a tendency to fear that a single variable > rename will break everybody's machines at this point of the code :) That's kinda why I want your opinion ;-) > My problem with this patch is that now we do the same thing at 3 > different points depending on the platform: > - For VLV, we will enable infoframes at encoder->pre_enable > - For ILK and friends, we will enable infoframes at encoder->enable > - For HSW+, we will still enable infoframes at encoder->modeset > > I'd really like to have a standard behavior here :) Longterm we want to get rid of all the ->mode_set callbacks anyway and move this stuff into enable hooks. > Also, for ILK/SNB/IVB, we run encoder->enable after the very end of > the modeset sequence, and I suspect the pipe is already running at > that point (since the we already called intel_enable_pipe, we already > ran intel_enable_planes, and we also called ironlake_pch_enable). For > that case, we probably need to implement all those "wait for the exact > VSYNC moment before touching register" restrictions that are described > on the spec. Or we find a way to also call set_infoframes at > pre_enable on these platforms. Did we test these patches on the ILK+ > family? Well ilk/snb/ivb are special since hdmi is only on the pch, so I think it matters when we throw the switch on the pch transcoder. But the encoder->enable hook is indeed _after_ the pch enable call, so I guess we could move it to a pre_enable hook too. Same for hsw, it shouldn't really matter there really since it's currently in the ->mode_set callback. I guess patches on top for those platforms would be good? We could then also ditch the remaining vblank waits we have I think ... > I also remember a lot of bugs that would only be seen after > suspend/resume, so I suggest testing it too :) > > The good thing is that moving register writes away from the mode_set > callbacks is good IMHO since at some point we'll want crtc_enable to > fully setup the HW, so runtime PM will be able to work without > requiring a crtc_mode_set call. But you need to patch HSW+ too for > that :P Yeah, that's always a good plan. > This is not an ack, nack nor a reviewed-by :) If you are still > confident, feel free to go ahead. I think I'll go ahead and see what happens ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index fb9839b..a4ca63b6 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -669,8 +669,6 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder) I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val); POSTING_READ(intel_hdmi->hdmi_reg); - - intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); } static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder) struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); + struct drm_display_mode *adjusted_mode = + &intel_crtc->config.adjusted_mode; u32 temp; u32 enable_bits = SDVO_ENABLE; + intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); + if (intel_hdmi->has_audio) enable_bits |= SDVO_AUDIO_ENABLE;
Needs to happen after clock is running or it doesn't behave correctly. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)