[PATCH 1/4] drm/i915/vlv: write the port field in the per-pipe DIP control reg

[PATCH 1/4] drm/i915/vlv: write the port field in the per-pipe DIP control reg

[Jesse Barnes]

In case we end up bouncing these around between ports.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index b0413e1..ee892a4 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -557,10 +557,12 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 			       struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
+	u32 port = VIDEO_DIP_PORT(intel_dig_port->port);
 
 	assert_hdmi_port_disabled(intel_hdmi);
 
@@ -576,6 +578,16 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	if (port != (val & VIDEO_DIP_PORT_MASK)) {
+		if (val & VIDEO_DIP_ENABLE) {
+			val &= ~VIDEO_DIP_ENABLE;
+			I915_WRITE(reg, val);
+			POSTING_READ(reg);
+		}
+		val &= ~VIDEO_DIP_PORT_MASK;
+		val |= port;
+	}
+
 	val |= VIDEO_DIP_ENABLE;
 	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
 		 VIDEO_DIP_ENABLE_GCP);
-- 
1.7.9.5

[PATCH 2/4] drm/i915/vlv: disable AVI infoframe emission when writing infoframes

[Jesse Barnes]

We also do a disable later when we write a specific infoframe, but here
we do it to prevent sending a stale one before updating the infoframes.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ee892a4..3b804fc 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -589,8 +589,8 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 	}
 
 	val |= VIDEO_DIP_ENABLE;
-	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
-		 VIDEO_DIP_ENABLE_GCP);
+	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
+		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);
 
 	I915_WRITE(reg, val);
 	POSTING_READ(reg);
-- 
1.7.9.5

Re: [PATCH 2/4] drm/i915/vlv: disable AVI infoframe emission when writing infoframes

[Ville Syrjälä]

On Wed, Apr 02, 2014 at 10:08:52AM -0700, Jesse Barnes wrote:
> We also do a disable later when we write a specific infoframe, but here
> we do it to prevent sending a stale one before updating the infoframes.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index ee892a4..3b804fc 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -589,8 +589,8 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
>  	}
>  
>  	val |= VIDEO_DIP_ENABLE;
> -	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> -		 VIDEO_DIP_ENABLE_GCP);
> +	val &= ~(VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_ENABLE_VENDOR |
> +		 VIDEO_DIP_ENABLE_GAMUT | VIDEO_DIP_ENABLE_GCP);

This goes against the rule of matching the ENABLE_AVI bit with
the DIP_ENABLE bit. But after reading the spec I think following
that rule is not actually needed as it applies only if we
enable/disable DIP while the port is already enabled. We never
do that AFAICS.

There's another rule that says we aren't even allowed to disable
the AVI infoframe while the port is enabled. The logic we have
in the write_infoframe functions goes agains that rule.

These two rules look like something that are two parts of the same
issue of always having the AVI infoframe enabled while the port is
enabled. And since the logic in the write_infoframe functions already
makes it impossible to update the AVI infoframe correctly on the fly,
I think we should try to change all platforms to do the obvious thing
that you're doing here for VLV. I don't like accumulating differences
between the platforms if we really don't have to.

But I suppose a bit of testing would be needed to make sure such
a change wouldn't break other platforms.

For this patch:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	I915_WRITE(reg, val);
>  	POSTING_READ(reg);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 3/4] drm/i915: enable HDMI mode on VLV when an HDMI sink is detected

[Jesse Barnes]

Allows sending of the null packets for conformance.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 3b804fc..fb9839b 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -650,8 +650,8 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder)
 	else
 		hdmi_val |= SDVO_COLOR_FORMAT_8bpc;
 
-	/* Required on CPT */
-	if (intel_hdmi->has_hdmi_sink && HAS_PCH_CPT(dev))
+	if (intel_hdmi->has_hdmi_sink &&
+	    (HAS_PCH_CPT(dev) || IS_VALLEYVIEW(dev)))
 		hdmi_val |= HDMI_MODE_SELECT_HDMI;
 
 	if (intel_hdmi->has_audio) {
-- 
1.7.9.5

Re: [PATCH 3/4] drm/i915: enable HDMI mode on VLV when an HDMI sink is detected

[Ville Syrjälä]

On Wed, Apr 02, 2014 at 10:08:53AM -0700, Jesse Barnes wrote:
> Allows sending of the null packets for conformance.

Just set it for all platforms? I'm not aware of any downside of setting
it always.

Anyways seems sane enough so even for just VLV:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 3b804fc..fb9839b 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -650,8 +650,8 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder)
>  	else
>  		hdmi_val |= SDVO_COLOR_FORMAT_8bpc;
>  
> -	/* Required on CPT */
> -	if (intel_hdmi->has_hdmi_sink && HAS_PCH_CPT(dev))
> +	if (intel_hdmi->has_hdmi_sink &&
> +	    (HAS_PCH_CPT(dev) || IS_VALLEYVIEW(dev)))
>  		hdmi_val |= HDMI_MODE_SELECT_HDMI;
>  
>  	if (intel_hdmi->has_audio) {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Jesse Barnes]

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;
 
-- 
1.7.9.5

Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Paulo Zanoni]

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



-- 
Paulo Zanoni

Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: move infoframe setting to after pll enable v2

[Jesse Barnes]

Needs to happen after clock is running or it doesn't behave correctly.

v2: fix subject (Ville)
    make it clearer that this occurs in pre_enable (Paulo)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index fb9839b..05055f6 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,
@@ -1115,13 +1113,26 @@ done:
 	return 0;
 }
 
+static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
+{
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+
+	intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
+}
+
 static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 {
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(encoder->base.crtc);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
 	enum dpio_channel port = vlv_dport_to_channel(dport);
 	int pipe = intel_crtc->pipe;
 	u32 val;
@@ -1155,6 +1166,8 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 	vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW23(port), 0x00400888);
 	mutex_unlock(&dev_priv->dpio_lock);
 
+	intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
+
 	intel_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport);
@@ -1344,6 +1357,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 	intel_encoder->disable = intel_disable_hdmi;
 	intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
 	intel_encoder->get_config = intel_hdmi_get_config;
+	intel_encoder->pre_enable = intel_hdmi_pre_enable;
 	if (IS_VALLEYVIEW(dev)) {
 		intel_encoder->pre_pll_enable = vlv_hdmi_pre_pll_enable;
 		intel_encoder->pre_enable = vlv_hdmi_pre_enable;
-- 
1.7.9.5

Re: [PATCH] drm/i915: move infoframe setting to after pll enable v2

[Paulo Zanoni]

2014-04-04 18:38 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> Needs to happen after clock is running or it doesn't behave correctly.
>
> v2: fix subject (Ville)
>     make it clearer that this occurs in pre_enable (Paulo)
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index fb9839b..05055f6 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,
> @@ -1115,13 +1113,26 @@ done:
>         return 0;
>  }
>
> +static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
> +{
> +       struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +       struct drm_display_mode *adjusted_mode =
> +               &intel_crtc->config.adjusted_mode;
> +
> +       intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> +}
> +
>  static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>  {
> +       struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>         struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);

<bikeshed><OCD>
struct intel_hdmi *intel_hdmi = &dport->hdmi;

Because enc_to_intel_hdmi() just calls enc_to_dig_port() again, which
you already called on this funciton.
</OCD></bikeshed>


>         struct drm_device *dev = encoder->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crtc *intel_crtc =
>                 to_intel_crtc(encoder->base.crtc);
> +       struct drm_display_mode *adjusted_mode =
> +               &intel_crtc->config.adjusted_mode;
>         enum dpio_channel port = vlv_dport_to_channel(dport);
>         int pipe = intel_crtc->pipe;
>         u32 val;
> @@ -1155,6 +1166,8 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>         vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW23(port), 0x00400888);
>         mutex_unlock(&dev_priv->dpio_lock);
>
> +       intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> +
>         intel_enable_hdmi(encoder);
>
>         vlv_wait_port_ready(dev_priv, dport);
> @@ -1344,6 +1357,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>         intel_encoder->disable = intel_disable_hdmi;
>         intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
>         intel_encoder->get_config = intel_hdmi_get_config;
> +       intel_encoder->pre_enable = intel_hdmi_pre_enable;

<bikeshed>
Put the assignment above on the "else" case of the "if" below, to
reduce the probability that someone will quickly spot this assignment
and think it is also valid for VLV, because they will fail to notice
that pre_enable is replaced below.
</bikeshed>

We still didn't change HSW+, but I'll pretend I didn't see that :)

With or without the above: Reviewed-by: Paulo Zanoni
<paulo.r.zanoni@intel.com>. But I still think it needs testing :)

>         if (IS_VALLEYVIEW(dev)) {
>                 intel_encoder->pre_pll_enable = vlv_hdmi_pre_pll_enable;
>                 intel_encoder->pre_enable = vlv_hdmi_pre_enable;
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH] drm/i915: move infoframe setting to after pll enable v2

[Jesse Barnes]

Needs to happen after clock is running or it doesn't behave correctly.

v2: fix subject (Ville)
    make it clearer that this occurs in pre_enable (Paulo)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index fb9839b..05055f6 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,
@@ -1115,13 +1113,26 @@ done:
 	return 0;
 }
 
+static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
+{
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+
+	intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
+}
+
 static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 {
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(encoder->base.crtc);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
 	enum dpio_channel port = vlv_dport_to_channel(dport);
 	int pipe = intel_crtc->pipe;
 	u32 val;
@@ -1155,6 +1166,8 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 	vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW23(port), 0x00400888);
 	mutex_unlock(&dev_priv->dpio_lock);
 
+	intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
+
 	intel_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport);
@@ -1344,6 +1357,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 	intel_encoder->disable = intel_disable_hdmi;
 	intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
 	intel_encoder->get_config = intel_hdmi_get_config;
+	intel_encoder->pre_enable = intel_hdmi_pre_enable;
 	if (IS_VALLEYVIEW(dev)) {
 		intel_encoder->pre_pll_enable = vlv_hdmi_pre_pll_enable;
 		intel_encoder->pre_enable = vlv_hdmi_pre_enable;
-- 
1.7.9.5

Re: [PATCH] drm/i915: move infoframe setting to after pll enable v2

[Daniel Vetter]

On Fri, Apr 04, 2014 at 03:12:08PM -0700, Jesse Barnes wrote:
> Needs to happen after clock is running or it doesn't behave correctly.
> 
> v2: fix subject (Ville)
>     make it clearer that this occurs in pre_enable (Paulo)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Resent the wrong version or failed to git add?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index fb9839b..05055f6 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,
> @@ -1115,13 +1113,26 @@ done:
>  	return 0;
>  }
>  
> +static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
> +{
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct drm_display_mode *adjusted_mode =
> +		&intel_crtc->config.adjusted_mode;
> +
> +	intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> +}
> +
>  static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>  {
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc =
>  		to_intel_crtc(encoder->base.crtc);
> +	struct drm_display_mode *adjusted_mode =
> +		&intel_crtc->config.adjusted_mode;
>  	enum dpio_channel port = vlv_dport_to_channel(dport);
>  	int pipe = intel_crtc->pipe;
>  	u32 val;
> @@ -1155,6 +1166,8 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>  	vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW23(port), 0x00400888);
>  	mutex_unlock(&dev_priv->dpio_lock);
>  
> +	intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> +
>  	intel_enable_hdmi(encoder);
>  
>  	vlv_wait_port_ready(dev_priv, dport);
> @@ -1344,6 +1357,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  	intel_encoder->disable = intel_disable_hdmi;
>  	intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
>  	intel_encoder->get_config = intel_hdmi_get_config;
> +	intel_encoder->pre_enable = intel_hdmi_pre_enable;
>  	if (IS_VALLEYVIEW(dev)) {
>  		intel_encoder->pre_pll_enable = vlv_hdmi_pre_pll_enable;
>  		intel_encoder->pre_enable = vlv_hdmi_pre_enable;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: move infoframe setting to after pll enable v2

[Jesse Barnes]

On Sat, 5 Apr 2014 17:21:04 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Apr 04, 2014 at 03:12:08PM -0700, Jesse Barnes wrote:
> > Needs to happen after clock is running or it doesn't behave correctly.
> > 
> > v2: fix subject (Ville)
> >     make it clearer that this occurs in pre_enable (Paulo)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Resent the wrong version or failed to git add?

Yeah git send-email with the old sha will do that... :)

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm/i915: move infoframe setting to after pll enable v3

[Jesse Barnes]

Needs to happen after clock is running or it doesn't behave correctly.

v2: fix subject (Ville)
    make it clearer that this occurs in pre_enable (Paulo)
    misc bikesheds (Paulo)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index fb9839b..fd940a7 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,
@@ -1115,13 +1113,26 @@ done:
 	return 0;
 }
 
+static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
+{
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
+
+	intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
+}
+
 static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 {
 	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
+	struct intel_hdmi *intel_hdmi = &dport->hdmi;
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(encoder->base.crtc);
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config.adjusted_mode;
 	enum dpio_channel port = vlv_dport_to_channel(dport);
 	int pipe = intel_crtc->pipe;
 	u32 val;
@@ -1155,6 +1166,8 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 	vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW23(port), 0x00400888);
 	mutex_unlock(&dev_priv->dpio_lock);
 
+	intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
+
 	intel_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport);
@@ -1350,6 +1363,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 		intel_encoder->enable = vlv_enable_hdmi;
 		intel_encoder->post_disable = vlv_hdmi_post_disable;
 	} else {
+		intel_encoder->pre_enable = intel_hdmi_pre_enable;
 		intel_encoder->enable = intel_enable_hdmi;
 	}
 
-- 
1.7.9.5

Re: [PATCH] drm/i915: move infoframe setting to after pll enable v3

[Paulo Zanoni]

2014-04-05 15:51 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> Needs to happen after clock is running or it doesn't behave correctly.
>
> v2: fix subject (Ville)
>     make it clearer that this occurs in pre_enable (Paulo)
>     misc bikesheds (Paulo)
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index fb9839b..fd940a7 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,
> @@ -1115,13 +1113,26 @@ done:
>         return 0;
>  }
>
> +static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
> +{
> +       struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +       struct drm_display_mode *adjusted_mode =
> +               &intel_crtc->config.adjusted_mode;
> +
> +       intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> +}
> +
>  static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>  {
>         struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
> +       struct intel_hdmi *intel_hdmi = &dport->hdmi;
>         struct drm_device *dev = encoder->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crtc *intel_crtc =
>                 to_intel_crtc(encoder->base.crtc);
> +       struct drm_display_mode *adjusted_mode =
> +               &intel_crtc->config.adjusted_mode;
>         enum dpio_channel port = vlv_dport_to_channel(dport);
>         int pipe = intel_crtc->pipe;
>         u32 val;
> @@ -1155,6 +1166,8 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>         vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW23(port), 0x00400888);
>         mutex_unlock(&dev_priv->dpio_lock);
>
> +       intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> +
>         intel_enable_hdmi(encoder);
>
>         vlv_wait_port_ready(dev_priv, dport);
> @@ -1350,6 +1363,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>                 intel_encoder->enable = vlv_enable_hdmi;
>                 intel_encoder->post_disable = vlv_hdmi_post_disable;
>         } else {
> +               intel_encoder->pre_enable = intel_hdmi_pre_enable;
>                 intel_encoder->enable = intel_enable_hdmi;
>         }
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: move infoframe setting to after pll enable v3

[Daniel Vetter]

On Mon, Apr 07, 2014 at 09:54:50AM -0300, Paulo Zanoni wrote:
> 2014-04-05 15:51 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > Needs to happen after clock is running or it doesn't behave correctly.
> >
> > v2: fix subject (Ville)
> >     make it clearer that this occurs in pre_enable (Paulo)
> >     misc bikesheds (Paulo)
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

All 4 patches merged to dinq, thanks.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c |   18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index fb9839b..fd940a7 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,
> > @@ -1115,13 +1113,26 @@ done:
> >         return 0;
> >  }
> >
> > +static void intel_hdmi_pre_enable(struct intel_encoder *encoder)
> > +{
> > +       struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> > +       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > +       struct drm_display_mode *adjusted_mode =
> > +               &intel_crtc->config.adjusted_mode;
> > +
> > +       intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> > +}
> > +
> >  static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
> >  {
> >         struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
> > +       struct intel_hdmi *intel_hdmi = &dport->hdmi;
> >         struct drm_device *dev = encoder->base.dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         struct intel_crtc *intel_crtc =
> >                 to_intel_crtc(encoder->base.crtc);
> > +       struct drm_display_mode *adjusted_mode =
> > +               &intel_crtc->config.adjusted_mode;
> >         enum dpio_channel port = vlv_dport_to_channel(dport);
> >         int pipe = intel_crtc->pipe;
> >         u32 val;
> > @@ -1155,6 +1166,8 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
> >         vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW23(port), 0x00400888);
> >         mutex_unlock(&dev_priv->dpio_lock);
> >
> > +       intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> > +
> >         intel_enable_hdmi(encoder);
> >
> >         vlv_wait_port_ready(dev_priv, dport);
> > @@ -1350,6 +1363,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
> >                 intel_encoder->enable = vlv_enable_hdmi;
> >                 intel_encoder->post_disable = vlv_hdmi_post_disable;
> >         } else {
> > +               intel_encoder->pre_enable = intel_hdmi_pre_enable;
> >                 intel_encoder->enable = intel_enable_hdmi;
> >         }
> >
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915/vlv: write the port field in the per-pipe DIP control reg

[Jesse Barnes]

These all have a
Tested-by: "Joon Jung <joon.jung@intel.com>"
too.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/4] drm/i915/vlv: write the port field in the per-pipe DIP control reg

[Ville Syrjälä]

On Wed, Apr 02, 2014 at 10:08:51AM -0700, Jesse Barnes wrote:
> In case we end up bouncing these around between ports.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Whoops. Almost missed this one. I must have an spam filter for cover
letters in my brain or something,

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index b0413e1..ee892a4 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -557,10 +557,12 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
>  			       struct drm_display_mode *adjusted_mode)
>  {
>  	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> +	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>  	u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
> +	u32 port = VIDEO_DIP_PORT(intel_dig_port->port);
>  
>  	assert_hdmi_port_disabled(intel_hdmi);
>  
> @@ -576,6 +578,16 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
>  		return;
>  	}
>  
> +	if (port != (val & VIDEO_DIP_PORT_MASK)) {
> +		if (val & VIDEO_DIP_ENABLE) {
> +			val &= ~VIDEO_DIP_ENABLE;
> +			I915_WRITE(reg, val);
> +			POSTING_READ(reg);
> +		}
> +		val &= ~VIDEO_DIP_PORT_MASK;
> +		val |= port;
> +	}
> +
>  	val |= VIDEO_DIP_ENABLE;
>  	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
>  		 VIDEO_DIP_ENABLE_GCP);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC