Re: [Intel-gfx] [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI

2012-05-28 Thread Paulo Zanoni
2012/5/28 Chris Wilson :
> On Mon, 28 May 2012 16:42:49 -0300, Paulo Zanoni  wrote:
>> This patch adds the code to fix the problem, but it depends on the
>> removal of some code that can't be removed right now and will come
>> later in the patch series. The patch that we need is:
>>   - drm/i915: don't write 0 to DIP control at HDMI init
>
> I was going to grumble a bit more and ask if these could be split into
> generational patches so that a bisect doesn't land on a commit affecting
> them all. However, it sounds like bisecting through this series is going
> to be problematic anyway... Just to be sure, we are not introducing
> issues to be resolved in later patches?

No. The idea of depending on a patch that comes later in the series is
because I tried to avoid any regressions in the middle of the series
and also tried to avoid moving code to the "set_infoframes" function
only to remove it later. The "don't write 0 to DIP" patch needs the
set_infoframes function to completely set the state of the control
register, so it basically needed all the previous patches. Any
regression introduced in the middle of the series and then removed
later is an accident.

I could split patches into generational patches, but then we'd have
like 60 patches, most of them one-liners...


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI

2012-05-28 Thread Chris Wilson
On Mon, 28 May 2012 16:42:49 -0300, Paulo Zanoni  wrote:
> This patch adds the code to fix the problem, but it depends on the
> removal of some code that can't be removed right now and will come
> later in the patch series. The patch that we need is:
>   - drm/i915: don't write 0 to DIP control at HDMI init

I was going to grumble a bit more and ask if these could be split into
generational patches so that a bisect doesn't land on a commit affecting
them all. However, it sounds like bisecting through this series is going
to be problematic anyway... Just to be sure, we are not introducing
issues to be resolved in later patches?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI

2012-05-28 Thread Paulo Zanoni
From: Paulo Zanoni 

This solves problems that happen when you alternate between HDMI and
DVI on the same port. I can reproduce these problems using DP->HDMI
and DP->DVI adapters on a DP port.

When you first plug HDMI and then plug DVI, you need to stop sending
DIPs, even if the port is in DVI mode (see the HDMI register spec). If
you don't stop sending DIPs, you'll see a pink vertical line on the
left side of the screen, some modes will give you a black screen, some
modes won't work correctly.

When you first plug DVI and then plug HDMI, you need to properly
enable the DIPs, otherwise the HW won't send them. After spending a
lot of time investigating this, I concluded that if the DIPs are
disabled, we should not write to the DIP register again because when
we do this, we also set the AVI InfoFrame frequency to "once", and
this seems to really confuse our hardware. Since this problem was not
exactly easy to debug, I'm adopting the defensive behavior and not
just avoing the "disable twice" sequence, but also explicitly
selecting the AVI InfoFrame and setting its frequency to a correct
one.

Also, move the "is_dvi" check from intel_set_infoframe to the
set_infoframes functions since now they're going to be the first ones
to deal with the DIP registers.

This patch adds the code to fix the problem, but it depends on the
removal of some code that can't be removed right now and will come
later in the patch series. The patch that we need is:
  - drm/i915: don't write 0 to DIP control at HDMI init

V2: Be even more defensive by selecting AVI and setting its frequency
outside the "is_dvi" check.

Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/intel_hdmi.c |   88 +++--
 1 file changed, 85 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
b/drivers/gpu/drm/i915/intel_hdmi.c
index 8d892b0..2f2adc4 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -308,9 +308,6 @@ static void intel_set_infoframe(struct drm_encoder *encoder,
 {
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 
-   if (!intel_hdmi->has_hdmi_sink)
-   return;
-
intel_dip_infoframe_csum(frame);
intel_hdmi->write_infoframe(encoder, frame);
 }
@@ -348,6 +345,30 @@ static void intel_hdmi_set_spd_infoframe(struct 
drm_encoder *encoder)
 static void g4x_set_infoframes(struct drm_encoder *encoder,
   struct drm_display_mode *adjusted_mode)
 {
+   struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+   struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+   u32 reg = VIDEO_DIP_CTL;
+   u32 val = I915_READ(reg);
+
+   /* If the registers were not initialized yet, they might be zeroes,
+* which means we're selecting the AVI DIP and we're setting its
+* frequency to once. This seems to really confuse the HW and make
+* things stop working (the register spec says the AVI always needs to
+* be sent every VSync). So here we avoid writing to the register more
+* than we need and also explicitly select the AVI DIP and explicitly
+* set its frequency to every VSync. Avoiding to write it twice seems to
+* be enough to solve the problem, but being defensive shouldn't hurt us
+* either. */
+   val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
+
+   if (!intel_hdmi->has_hdmi_sink) {
+   if (!(val & VIDEO_DIP_ENABLE))
+   return;
+   val &= ~VIDEO_DIP_ENABLE;
+   I915_WRITE(reg, val);
+   return;
+   }
+
intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
intel_hdmi_set_spd_infoframe(encoder);
 }
@@ -355,6 +376,23 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 static void ibx_set_infoframes(struct drm_encoder *encoder,
   struct drm_display_mode *adjusted_mode)
 {
+   struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+   struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+   struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+   u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
+   u32 val = I915_READ(reg);
+
+   /* See the big comment in g4x_set_infoframes() */
+   val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
+
+   if (!intel_hdmi->has_hdmi_sink) {
+   if (!(val & VIDEO_DIP_ENABLE))
+   return;
+   val &= ~VIDEO_DIP_ENABLE;
+   I915_WRITE(reg, val);
+   return;
+   }
+
intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
intel_hdmi_set_spd_infoframe(encoder);
 }
@@ -362,6 +400,23 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 static void cpt_set_infoframes(struct drm_encoder *encoder,
   struct drm_display_mode *adjusted_mode)
 {
+   struct drm_