[PATCH] drm/i915: Ignore pipe B active state when enabling pipe C

[PATCH] drm/i915: Ignore pipe B active state when enabling pipe C

[Ander Conselvan de Oliveira]

When enabling pipe C, the check for the number of lanes pipe B uses was
ignored in case pipe B wasn't active. This would allow pipe C to be
configured while pipe B is in DPMS off state even if it used more than 2
lanes. Making pipe B active again while pipe C was also active would
then fail.

Tested-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 597c10b..4008bf4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3150,8 +3150,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
 
 static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
 {
-	return crtc->base.state->enable && crtc->active &&
-		crtc->config->has_pch_encoder;
+	return crtc->base.state->enable && crtc->config->has_pch_encoder;
 }
 
 static void ivb_modeset_global_resources(struct drm_device *dev)
-- 
2.1.0

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

Re: [PATCH] drm/i915: Ignore pipe B active state when enabling pipe C

[Jani Nikula]

On Mon, 09 Mar 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> When enabling pipe C, the check for the number of lanes pipe B uses was
> ignored in case pipe B wasn't active. This would allow pipe C to be
> configured while pipe B is in DPMS off state even if it used more than 2
> lanes. Making pipe B active again while pipe C was also active would
> then fail.

Seems like a good catch. Broken when, or since forever? Cc: stable?
Bugzillas?

BR,
Jani.

>
> Tested-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 597c10b..4008bf4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3150,8 +3150,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  
>  static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  {
> -	return crtc->base.state->enable && crtc->active &&
> -		crtc->config->has_pch_encoder;
> +	return crtc->base.state->enable && crtc->config->has_pch_encoder;
>  }
>  
>  static void ivb_modeset_global_resources(struct drm_device *dev)
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Ignore pipe B active state when enabling pipe C

[Ander Conselvan De Oliveira]

On Mon, 2015-03-09 at 11:24 +0200, Jani Nikula wrote:
> On Mon, 09 Mar 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> > When enabling pipe C, the check for the number of lanes pipe B uses was
> > ignored in case pipe B wasn't active. This would allow pipe C to be
> > configured while pipe B is in DPMS off state even if it used more than 2
> > lanes. Making pipe B active again while pipe C was also active would
> > then fail.
> 
> Seems like a good catch. Broken when, or since forever? Cc: stable?
> Bugzillas?

I had to touch this code in the last patch series I submitted, and I
raised a concern that this might do the wrong thing. Daniel suggested a
tried the test case I described above, which indeed does fail. I haven't
done the actual history digging until a minute ago, which turns out
quite interesting. The exact opposite of this patch was done in the
following patch:

commit 1fbc0d789d12fec313c91912fc11733fdfbab863
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Oct 29 12:04:08 2013 +0100

    drm/i915: Fix the PPT fdi lane bifurcate state handling on ivb

I'm not sure how much has changed since then, or if the comments on that
commit's message are still relevant. Particularly, if the unifying of
mode set and dpms on code was ever done, and if it has any effect here.

Ander.


> >
> > Tested-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 597c10b..4008bf4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3150,8 +3150,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
> >  
> >  static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> >  {
> > -	return crtc->base.state->enable && crtc->active &&
> > -		crtc->config->has_pch_encoder;
> > +	return crtc->base.state->enable && crtc->config->has_pch_encoder;
> >  }
> >  
> >  static void ivb_modeset_global_resources(struct drm_device *dev)
> > -- 
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


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

Re: [PATCH] drm/i915: Ignore pipe B active state when enabling pipe C

[Daniel Vetter]

On Mon, Mar 09, 2015 at 11:33:57AM +0200, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-03-09 at 11:24 +0200, Jani Nikula wrote:
> > On Mon, 09 Mar 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> > > When enabling pipe C, the check for the number of lanes pipe B uses was
> > > ignored in case pipe B wasn't active. This would allow pipe C to be
> > > configured while pipe B is in DPMS off state even if it used more than 2
> > > lanes. Making pipe B active again while pipe C was also active would
> > > then fail.
> > 
> > Seems like a good catch. Broken when, or since forever? Cc: stable?
> > Bugzillas?
> 
> I had to touch this code in the last patch series I submitted, and I
> raised a concern that this might do the wrong thing. Daniel suggested a
> tried the test case I described above, which indeed does fail. I haven't
> done the actual history digging until a minute ago, which turns out
> quite interesting. The exact opposite of this patch was done in the
> following patch:
> 
> commit 1fbc0d789d12fec313c91912fc11733fdfbab863
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue Oct 29 12:04:08 2013 +0100
> 
>     drm/i915: Fix the PPT fdi lane bifurcate state handling on ivb
> 
> I'm not sure how much has changed since then, or if the comments on that
> commit's message are still relevant. Particularly, if the unifying of
> mode set and dpms on code was ever done, and if it has any effect here.

Indeed your patch would break things again I think for the case. What we
essentially want to know is whether we've had to do a modeset change on a
given pipe that might require us to update the bifurcate state. We abuse
intel->active as a cheap way to tell since for the case we're interested
in we have crtc->base.enabled == true and crtc->active == false. That's
the case where the pipe will be enabled again, but has been shut down
to change the mode.

With atomic we need to probably look at crtc_state->mode_changed. As an
interim solution we have the same information available in the
modeset_pipes bitmask. I think replacing the check for intel_crtc->active
with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.

The way to reproduce the original bug should be:
- Light up pipe B with something requiring more than 2 lanes.
- Do an immediate modeset on pipe B to a mode requiring at most 2 lanes.
  Not that SNA/X always does an interim modeset to everything off, you'd
  need to code up an igt I think. Or vt-switch between X servers with
  different modes. Without the intel_crtc->active check we won't set the
  bifurcate bit.
- Try to light up pipe C and go boom on the bifurcate consistency checks.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Ander Conselvan de Oliveira]

Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling or
disabling the pch transcoder. The checks before the mode set should
ensure that the configuration is valid, so it should be safe to do it
so.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---

On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> With atomic we need to probably look at crtc_state->mode_changed. As an
> interim solution we have the same information available in the
> modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.

I looked into that solution, but involves passing modeset_pipes way down
into the call stack, so I started looking for a different solution. Do you
see any caveat with this approach?

Thanks,
Ander

---
 drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
 1 file changed, 21 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..eca5a41 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -101,6 +101,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
 static void intel_begin_crtc_commit(struct drm_crtc *crtc);
 static void intel_finish_crtc_commit(struct drm_crtc *crtc);
+static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
+						bool pipe_active);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -1956,6 +1958,8 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
 					    enum pipe pipe)
 {
 	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc *crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 	uint32_t reg, val;
 
 	/* FDI relies on the transcoder */
@@ -1980,6 +1984,9 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
 		val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
 		I915_WRITE(reg, val);
 	}
+
+	if (IS_IVYBRIDGE(dev))
+		ivybridge_update_fdi_bc_bifurcation(crtc, false);
 }
 
 static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
@@ -3153,32 +3160,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
 	return crtc->base.state->enable && crtc->config->has_pch_encoder;
 }
 
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *pipe_B_crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
-	struct intel_crtc *pipe_C_crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
-	uint32_t temp;
-
-	/*
-	 * When everything is off disable fdi C so that we could enable fdi B
-	 * with all lanes. Note that we don't care about enabled pipes without
-	 * an enabled pch encoder.
-	 */
-	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
-	    !pipe_has_enabled_pch(pipe_C_crtc)) {
-		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
-		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
-
-		temp = I915_READ(SOUTH_CHICKEN1);
-		temp &= ~FDI_BC_BIFURCATION_SELECT;
-		DRM_DEBUG_KMS("disabling fdi C rx\n");
-		I915_WRITE(SOUTH_CHICKEN1, temp);
-	}
-}
-
 /* The FDI link training functions for ILK/Ibexpeak. */
 static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 {
@@ -3834,41 +3815,44 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
 		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
 }
 
-static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t temp;
 
 	temp = I915_READ(SOUTH_CHICKEN1);
-	if (temp & FDI_BC_BIFURCATION_SELECT)
+	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
 		return;
 
 	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
 	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
 
-	temp |= FDI_BC_BIFURCATION_SELECT;
-	DRM_DEBUG_KMS("enabling fdi C rx\n");
+	temp &= ~FDI_BC_BIFURCATION_SELECT;
+	if (enable)
+		temp |= FDI_BC_BIFURCATION_SELECT;
+
+	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
 	I915_WRITE(SOUTH_CHICKEN1, temp);
 	POSTING_READ(SOUTH_CHICKEN1);
 }
 
-static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
+static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
+						bool pipe_active)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	switch (intel_crtc->pipe) {
 	case PIPE_A:
 		break;
 	case PIPE_B:
-		if (intel_crtc->config->fdi_lanes > 2)
-			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
+		if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
+			cpt_enable_fdi_bc_bifurcation(dev, false);
 		else
-			cpt_enable_fdi_bc_bifurcation(dev);
+			cpt_enable_fdi_bc_bifurcation(dev, true);
 
 		break;
 	case PIPE_C:
-		cpt_enable_fdi_bc_bifurcation(dev);
+		cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
 
 		break;
 	default:
@@ -3895,7 +3879,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 	assert_pch_transcoder_disabled(dev_priv, pipe);
 
 	if (IS_IVYBRIDGE(dev))
-		ivybridge_update_fdi_bc_bifurcation(intel_crtc);
+		ivybridge_update_fdi_bc_bifurcation(intel_crtc, true);
 
 	/* Write the TU size bits before fdi link training, so that error
 	 * detection works. */
@@ -13056,8 +13040,6 @@ static void intel_init_display(struct drm_device *dev)
 	} else if (IS_IVYBRIDGE(dev)) {
 		/* FIXME: detect B0+ stepping and use auto training */
 		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
-		dev_priv->display.modeset_global_resources =
-			ivb_modeset_global_resources;
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 	} else if (IS_VALLEYVIEW(dev)) {
-- 
2.1.0

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

[PATCH] tests: Add test for pipe B and C interactions in IVB

[Ander Conselvan de Oliveira]

This actually only works if the machine is setup properly. It requires
that:

- the hardware to have at least two connected outpus;
- the defatult mode on those outputs needs to be big enough;
- the attached monitors need to support 10 bpc.

This might generate spurius results if the connected output doesn't support
10bpc or a large enough mode.

---
I used this to test the changes that affect pipe B and C interactions on
IVB. I'm not really sure how to turn into a proper igt test though, because
of the requirements on the outputs in use.


---
 tests/Makefile.sources    |   1 +
 tests/kms_pipe_b_c_dpms.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 174 insertions(+)
 create mode 100644 tests/kms_pipe_b_c_dpms.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 9ab0ff4..9e43a64 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -73,6 +73,7 @@ TESTS_progs_M = \
 	kms_nuclear \
 	kms_pipe_crc_basic \
 	kms_plane \
+	kms_pipe_b_c_dpms \
 	kms_psr_sink_crc \
 	kms_render \
 	kms_rotation_crc \
diff --git a/tests/kms_pipe_b_c_dpms.c b/tests/kms_pipe_b_c_dpms.c
new file mode 100644
index 0000000..a5ed761
--- /dev/null
+++ b/tests/kms_pipe_b_c_dpms.c
@@ -0,0 +1,173 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *   Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
+ */
+
+#include "drmtest.h"
+#include "igt_kms.h"
+#include "intel_chipset.h"
+
+typedef struct {
+	int drm_fd;
+	igt_display_t display;
+} data_t;
+
+static int
+set_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output, int bpc)
+{
+	igt_plane_t *primary;
+	drmModeModeInfo *mode;
+	struct igt_fb fb;
+	int fb_id;
+	uint32_t format;
+
+	if (bpc == 8)
+		format = DRM_FORMAT_XRGB8888;
+	else if (bpc == 10)
+		format = DRM_FORMAT_XRGB2101010;
+	else
+		igt_fail(1);
+
+	igt_output_set_pipe(output, pipe);
+
+	/* FIXME: we need a big mode */
+	mode = igt_output_get_mode(output);
+	primary = igt_output_get_plane(output, 0);
+
+	fb_id = igt_create_color_fb(data->drm_fd,
+				    mode->hdisplay, mode->vdisplay,
+				    format, I915_TILING_NONE,
+				    1.0, 1.0, 1.0, &fb);
+	igt_assert(fb_id >= 0);
+
+	igt_plane_set_fb(primary, &fb);
+	return igt_display_try_commit2(&data->display, COMMIT_LEGACY);
+}
+
+static int
+set_big_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	return set_mode_on_pipe(data, pipe, output, 10);
+}
+
+static int
+set_normal_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	return set_mode_on_pipe(data, pipe, output, 8);
+}
+
+static void
+find_outputs(data_t *data, igt_output_t **output1, igt_output_t **output2)
+{
+	int count = 0;
+	igt_output_t *output;
+
+	*output1 = NULL;
+	*output2 = NULL;
+
+	for_each_connected_output(&data->display, output) {
+		if (!(*output1))
+			*output1 = output;
+		else if (!(*output2))
+			*output2 = output;
+
+		igt_output_set_pipe(output, PIPE_ANY);
+		count++;
+	}
+
+	igt_skip_on_f(count < 2, "Not enough connected outputs\n");
+}
+
+static void
+test_dpms(data_t *data)
+{
+	igt_output_t *output1, *output2;
+	int ret;
+
+	find_outputs(data, &output1, &output2);
+
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
+
+	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	kmstest_set_connector_dpms(data->drm_fd, output1->config.connector, DRM_MODE_DPMS_OFF);
+
+	ret = set_big_mode_on_pipe(data, PIPE_C, output2);
+	igt_assert(ret != 0);
+}
+
+static void
+test_lane_reduction(data_t *data)
+{
+	igt_output_t *output1, *output2;
+	int ret;
+
+	find_outputs(data, &output1, &output2);
+
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
+
+	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	ret = set_normal_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
+	igt_assert(ret == 0);
+}
+
+static data_t data;
+igt_main
+{
+	int devid;
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		data.drm_fd = drm_open_any_master();
+		devid = intel_get_drm_devid(data.drm_fd);
+
+		kmstest_set_vt_graphics_mode();
+		igt_display_init(&data.display, data.drm_fd);
+	}
+
+	igt_skip_on(!IS_IVYBRIDGE(devid));
+
+	igt_subtest("pipe-B-dpms-off-modeset-pipe-C")
+		test_dpms(&data);
+
+	igt_subtest("pipe-B-double-modeset-then-modeset-pipe-C")
+		test_lane_reduction(&data);
+
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
+}
-- 
2.1.0

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

Re: [PATCH] tests: Add test for pipe B and C interactions in IVB

[Daniel Vetter]

On Tue, Mar 10, 2015 at 02:35:56PM +0200, Ander Conselvan de Oliveira wrote:
> This actually only works if the machine is setup properly. It requires
> that:
> 
> - the hardware to have at least two connected outpus;
> - the defatult mode on those outputs needs to be big enough;
> - the attached monitors need to support 10 bpc.
> 
> This might generate spurius results if the connected output doesn't support
> 10bpc or a large enough mode.
> 
> ---
> I used this to test the changes that affect pipe B and C interactions on
> IVB. I'm not really sure how to turn into a proper igt test though, because
> of the requirements on the outputs in use.

Sprinkle lots of igt_require(condition) over the testcase. In this case a
bit tricky since we'd need to look at the edid to check that there's a
10bpc screeen. Maybe use as an alternative plan just a higher resolution
and only check for the fdi per-lane dotclock limit? That should be more
reliable since for external outputs we start out with at least 8bpc
everywhere before deciding to dither down.
-Daniel

> 
> 
> ---
>  tests/Makefile.sources    |   1 +
>  tests/kms_pipe_b_c_dpms.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 174 insertions(+)
>  create mode 100644 tests/kms_pipe_b_c_dpms.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 9ab0ff4..9e43a64 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -73,6 +73,7 @@ TESTS_progs_M = \
>  	kms_nuclear \
>  	kms_pipe_crc_basic \
>  	kms_plane \
> +	kms_pipe_b_c_dpms \
>  	kms_psr_sink_crc \
>  	kms_render \
>  	kms_rotation_crc \
> diff --git a/tests/kms_pipe_b_c_dpms.c b/tests/kms_pipe_b_c_dpms.c
> new file mode 100644
> index 0000000..a5ed761
> --- /dev/null
> +++ b/tests/kms_pipe_b_c_dpms.c
> @@ -0,0 +1,173 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *   Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> + */
> +
> +#include "drmtest.h"
> +#include "igt_kms.h"
> +#include "intel_chipset.h"
> +
> +typedef struct {
> +	int drm_fd;
> +	igt_display_t display;
> +} data_t;
> +
> +static int
> +set_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output, int bpc)
> +{
> +	igt_plane_t *primary;
> +	drmModeModeInfo *mode;
> +	struct igt_fb fb;
> +	int fb_id;
> +	uint32_t format;
> +
> +	if (bpc == 8)
> +		format = DRM_FORMAT_XRGB8888;
> +	else if (bpc == 10)
> +		format = DRM_FORMAT_XRGB2101010;
> +	else
> +		igt_fail(1);
> +
> +	igt_output_set_pipe(output, pipe);
> +
> +	/* FIXME: we need a big mode */
> +	mode = igt_output_get_mode(output);
> +	primary = igt_output_get_plane(output, 0);
> +
> +	fb_id = igt_create_color_fb(data->drm_fd,
> +				    mode->hdisplay, mode->vdisplay,
> +				    format, I915_TILING_NONE,
> +				    1.0, 1.0, 1.0, &fb);
> +	igt_assert(fb_id >= 0);
> +
> +	igt_plane_set_fb(primary, &fb);
> +	return igt_display_try_commit2(&data->display, COMMIT_LEGACY);
> +}
> +
> +static int
> +set_big_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> +{
> +	return set_mode_on_pipe(data, pipe, output, 10);
> +}
> +
> +static int
> +set_normal_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> +{
> +	return set_mode_on_pipe(data, pipe, output, 8);
> +}
> +
> +static void
> +find_outputs(data_t *data, igt_output_t **output1, igt_output_t **output2)
> +{
> +	int count = 0;
> +	igt_output_t *output;
> +
> +	*output1 = NULL;
> +	*output2 = NULL;
> +
> +	for_each_connected_output(&data->display, output) {
> +		if (!(*output1))
> +			*output1 = output;
> +		else if (!(*output2))
> +			*output2 = output;
> +
> +		igt_output_set_pipe(output, PIPE_ANY);
> +		count++;
> +	}
> +
> +	igt_skip_on_f(count < 2, "Not enough connected outputs\n");
> +}
> +
> +static void
> +test_dpms(data_t *data)
> +{
> +	igt_output_t *output1, *output2;
> +	int ret;
> +
> +	find_outputs(data, &output1, &output2);
> +
> +	igt_info("Pipe %s will use connector %s\n",
> +		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
> +	igt_info("Pipe %s will use connector %s\n",
> +		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
> +
> +	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
> +	igt_assert(ret == 0);
> +
> +	kmstest_set_connector_dpms(data->drm_fd, output1->config.connector, DRM_MODE_DPMS_OFF);
> +
> +	ret = set_big_mode_on_pipe(data, PIPE_C, output2);
> +	igt_assert(ret != 0);
> +}
> +
> +static void
> +test_lane_reduction(data_t *data)
> +{
> +	igt_output_t *output1, *output2;
> +	int ret;
> +
> +	find_outputs(data, &output1, &output2);
> +
> +	igt_info("Pipe %s will use connector %s\n",
> +		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
> +	igt_info("Pipe %s will use connector %s\n",
> +		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
> +
> +	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
> +	igt_assert(ret == 0);
> +
> +	ret = set_normal_mode_on_pipe(data, PIPE_B, output1);
> +	igt_assert(ret == 0);
> +
> +	ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
> +	igt_assert(ret == 0);
> +}
> +
> +static data_t data;
> +igt_main
> +{
> +	int devid;
> +
> +	igt_skip_on_simulation();
> +
> +	igt_fixture {
> +		data.drm_fd = drm_open_any_master();
> +		devid = intel_get_drm_devid(data.drm_fd);
> +
> +		kmstest_set_vt_graphics_mode();
> +		igt_display_init(&data.display, data.drm_fd);
> +	}
> +
> +	igt_skip_on(!IS_IVYBRIDGE(devid));
> +
> +	igt_subtest("pipe-B-dpms-off-modeset-pipe-C")
> +		test_dpms(&data);
> +
> +	igt_subtest("pipe-B-double-modeset-then-modeset-pipe-C")
> +		test_lane_reduction(&data);
> +
> +	igt_fixture {
> +		igt_display_fini(&data.display);
> +	}
> +}
> -- 
> 2.1.0
> 
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH igt 1/2] lib/kms: Add a way to override an output's mode

[Ander Conselvan de Oliveira]

So that it is possible to use a custom mode with the simplified mode set API.
---
 lib/igt_kms.c | 9 +++++++++
 lib/igt_kms.h | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 26e4913..0dccd2d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -895,6 +895,9 @@ static void igt_output_refresh(igt_output_t *output)
 	if (!output->valid)
 		return;
 
+	if (output->use_override_mode)
+		output->config.default_mode = output->override_mode;
+
 	if (!output->name) {
 		drmModeConnector *c = output->config.connector;
 
@@ -1797,6 +1800,12 @@ drmModeModeInfo *igt_output_get_mode(igt_output_t *output)
 	return &output->config.default_mode;
 }
 
+void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode)
+{
+	output->override_mode = *mode;
+	output->use_override_mode = true;
+}
+
 void igt_output_set_pipe(igt_output_t *output, enum pipe pipe)
 {
 	igt_display_t *display = output->display;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 2fab30e..ddf4432 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -228,6 +228,9 @@ typedef struct {
 	bool valid;
 	unsigned long pending_crtc_idx_mask;
 
+	bool use_override_mode;
+	drmModeModeInfo override_mode;
+
 #ifdef HAVE_ATOMIC
 	/* Property set for nuclear pageflip */
 	drmModePropertySetPtr set;
@@ -255,6 +258,7 @@ int  igt_display_get_n_pipes(igt_display_t *display);
 
 const char *igt_output_name(igt_output_t *output);
 drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
+void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode);
 void igt_output_set_pipe(igt_output_t *output, enum pipe pipe);
 igt_plane_t *igt_output_get_plane(igt_output_t *output, enum igt_plane plane);
 
-- 
2.1.0

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

[PATCH igt 2/2] tests: Add test for pipe B and C interactions in IVB

[Ander Conselvan de Oliveira]

The tests exercise different combinations of enabling pipe B with modes
that require more than 2 lanes and then enabling pipe C.

v2: Added a couple more tests for different pipe transitions. (Ander)
    Use custom modes to make the test reliable. (Daniel)
---
 tests/Makefile.sources    |   1 +
 tests/kms_pipe_b_c_dpms.c | 286 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 287 insertions(+)
 create mode 100644 tests/kms_pipe_b_c_dpms.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 9ab0ff4..9e43a64 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -73,6 +73,7 @@ TESTS_progs_M = \
 	kms_nuclear \
 	kms_pipe_crc_basic \
 	kms_plane \
+	kms_pipe_b_c_dpms \
 	kms_psr_sink_crc \
 	kms_render \
 	kms_rotation_crc \
diff --git a/tests/kms_pipe_b_c_dpms.c b/tests/kms_pipe_b_c_dpms.c
new file mode 100644
index 0000000..69394f4
--- /dev/null
+++ b/tests/kms_pipe_b_c_dpms.c
@@ -0,0 +1,286 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *   Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
+ */
+
+#include "drmtest.h"
+#include "igt_kms.h"
+#include "intel_chipset.h"
+
+typedef struct {
+	int drm_fd;
+	igt_display_t display;
+} data_t;
+
+drmModeModeInfo mode_3_lanes = {
+	.clock = 173000,
+	.hdisplay = 1920,
+	.hsync_start = 2048,
+	.hsync_end = 2248,
+	.htotal = 2576,
+	.vdisplay = 1080,
+	.vsync_start = 1083,
+	.vsync_end = 1088,
+	.vtotal = 1120,
+	.vrefresh = 60,
+	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC,
+	.name = "3_lanes",
+};
+
+drmModeModeInfo mode_2_lanes = {
+	.clock = 138500,
+	.hdisplay = 1920,
+	.hsync_start = 1968,
+	.hsync_end = 2000,
+	.htotal = 2080,
+	.vdisplay = 1080,
+	.vsync_start = 1083,
+	.vsync_end = 1088,
+	.vtotal = 1111,
+	.vrefresh = 60,
+	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC,
+	.name = "2_lanes",
+};
+
+static int
+disable_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	igt_plane_t *primary;
+
+	igt_output_set_pipe(output, pipe);
+	primary = igt_output_get_plane(output, 0);
+	igt_plane_set_fb(primary, NULL);
+	return igt_display_commit(&data->display);
+}
+
+static int
+set_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	igt_plane_t *primary;
+	drmModeModeInfo *mode;
+	struct igt_fb fb;
+	int fb_id;
+
+	igt_output_set_pipe(output, pipe);
+
+	mode = igt_output_get_mode(output);
+
+	primary = igt_output_get_plane(output, 0);
+
+	fb_id = igt_create_color_fb(data->drm_fd,
+				    mode->hdisplay, mode->vdisplay,
+				    DRM_FORMAT_XRGB8888, I915_TILING_NONE,
+				    1.0, 1.0, 1.0, &fb);
+	igt_assert(fb_id >= 0);
+
+	igt_plane_set_fb(primary, &fb);
+	return igt_display_try_commit2(&data->display, COMMIT_LEGACY);
+}
+
+static int
+set_big_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	igt_output_override_mode(output, &mode_3_lanes);
+	return set_mode_on_pipe(data, pipe, output);
+}
+
+static int
+set_normal_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	igt_output_override_mode(output, &mode_2_lanes);
+	return set_mode_on_pipe(data, pipe, output);
+}
+
+static void
+find_outputs(data_t *data, igt_output_t **output1, igt_output_t **output2)
+{
+	int count = 0;
+	igt_output_t *output;
+
+	*output1 = NULL;
+	*output2 = NULL;
+
+	for_each_connected_output(&data->display, output) {
+		if (!(*output1))
+			*output1 = output;
+		else if (!(*output2))
+			*output2 = output;
+
+		igt_output_set_pipe(output, PIPE_ANY);
+		count++;
+	}
+
+	igt_skip_on_f(count < 2, "Not enough connected outputs\n");
+}
+
+static void
+test_dpms(data_t *data)
+{
+	igt_output_t *output1, *output2;
+	int ret;
+
+	find_outputs(data, &output1, &output2);
+
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
+
+	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	kmstest_set_connector_dpms(data->drm_fd, output1->config.connector, DRM_MODE_DPMS_OFF);
+
+	ret = set_big_mode_on_pipe(data, PIPE_C, output2);
+	igt_assert(ret != 0);
+}
+
+static void
+test_lane_reduction(data_t *data)
+{
+	igt_output_t *output1, *output2;
+	int ret;
+
+	find_outputs(data, &output1, &output2);
+
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
+
+	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	ret = set_normal_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
+	igt_assert(ret == 0);
+}
+
+static void
+test_disable_pipe_B(data_t *data)
+{
+	igt_output_t *output1, *output2;
+	int ret;
+
+	find_outputs(data, &output1, &output2);
+
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
+
+	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	ret = disable_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
+	igt_assert(ret == 0);
+
+	ret = set_normal_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+}
+
+static void
+test_from_C_to_B_with_3_lanes(data_t *data)
+{
+	igt_output_t *output1, *output2;
+	int ret;
+
+	find_outputs(data, &output1, &output2);
+
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
+
+	ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
+	igt_assert(ret == 0);
+
+	ret = disable_pipe(data, PIPE_C, output2);
+	igt_assert(ret == 0);
+
+	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+}
+
+static void
+test_fail_enable_pipe_C_while_B_has_3_lanes(data_t *data)
+{
+	igt_output_t *output1, *output2;
+	int ret;
+
+	find_outputs(data, &output1, &output2);
+
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_B), igt_output_name(output1));
+	igt_info("Pipe %s will use connector %s\n",
+		 kmstest_pipe_name(PIPE_C), igt_output_name(output2));
+
+	ret = set_big_mode_on_pipe(data, PIPE_B, output1);
+	igt_assert(ret == 0);
+
+	ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
+	igt_assert(ret != 0);
+}
+
+static data_t data;
+igt_main
+{
+	int devid;
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		data.drm_fd = drm_open_any_master();
+		devid = intel_get_drm_devid(data.drm_fd);
+
+		kmstest_set_vt_graphics_mode();
+		igt_display_init(&data.display, data.drm_fd);
+	}
+
+	igt_skip_on(!IS_IVYBRIDGE(devid));
+
+	igt_subtest("pipe-B-dpms-off-modeset-pipe-C")
+		test_dpms(&data);
+
+	igt_subtest("pipe-B-double-modeset-then-modeset-pipe-C")
+		test_lane_reduction(&data);
+
+	igt_subtest("disable-pipe-B-enable-pipe-C")
+		test_disable_pipe_B(&data);
+
+	igt_subtest("from-pipe-C-to-B-with-3-lanes")
+		test_from_C_to_B_with_3_lanes(&data);
+
+	igt_subtest("enable-pipe-C-while-B-has-3-lanes")
+		test_fail_enable_pipe_C_while_B_has_3_lanes(&data);
+
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
+}
-- 
2.1.0

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

Re: [PATCH igt 2/2] tests: Add test for pipe B and C interactions in IVB

[Thomas Wood]

On 11 March 2015 at 11:33, Ander Conselvan de Oliveira
<ander.conselvan.de.oliveira@intel.com> wrote:
> The tests exercise different combinations of enabling pipe B with modes
> that require more than 2 lanes and then enabling pipe C.
>
> v2: Added a couple more tests for different pipe transitions. (Ander)
>     Use custom modes to make the test reliable. (Daniel)
> ---
>  tests/Makefile.sources    |   1 +
>  tests/kms_pipe_b_c_dpms.c | 286 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 287 insertions(+)
>  create mode 100644 tests/kms_pipe_b_c_dpms.c


Please add the test binary to .gitignore and add a short description
of the test using the IGT_TEST_DESCRIPTION macro.

>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 9ab0ff4..9e43a64 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -73,6 +73,7 @@ TESTS_progs_M = \
>         kms_nuclear \
>         kms_pipe_crc_basic \
>         kms_plane \
> +       kms_pipe_b_c_dpms \
>         kms_psr_sink_crc \
>         kms_render \
>         kms_rotation_crc \
> diff --git a/tests/kms_pipe_b_c_dpms.c b/tests/kms_pipe_b_c_dpms.c
> new file mode 100644
> index 0000000..69394f4
> --- /dev/null
> +++ b/tests/kms_pipe_b_c_dpms.c
> @@ -0,0 +1,286 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *   Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> + */
> +
> +#include "drmtest.h"
> +#include "igt_kms.h"
> +#include "intel_chipset.h"
> +
> +typedef struct {
> +       int drm_fd;
> +       igt_display_t display;
> +} data_t;
> +
> +drmModeModeInfo mode_3_lanes = {
> +       .clock = 173000,
> +       .hdisplay = 1920,
> +       .hsync_start = 2048,
> +       .hsync_end = 2248,
> +       .htotal = 2576,
> +       .vdisplay = 1080,
> +       .vsync_start = 1083,
> +       .vsync_end = 1088,
> +       .vtotal = 1120,
> +       .vrefresh = 60,
> +       .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC,
> +       .name = "3_lanes",
> +};
> +
> +drmModeModeInfo mode_2_lanes = {
> +       .clock = 138500,
> +       .hdisplay = 1920,
> +       .hsync_start = 1968,
> +       .hsync_end = 2000,
> +       .htotal = 2080,
> +       .vdisplay = 1080,
> +       .vsync_start = 1083,
> +       .vsync_end = 1088,
> +       .vtotal = 1111,
> +       .vrefresh = 60,
> +       .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC,
> +       .name = "2_lanes",
> +};
> +
> +static int
> +disable_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> +{
> +       igt_plane_t *primary;
> +
> +       igt_output_set_pipe(output, pipe);
> +       primary = igt_output_get_plane(output, 0);
> +       igt_plane_set_fb(primary, NULL);
> +       return igt_display_commit(&data->display);
> +}
> +
> +static int
> +set_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> +{
> +       igt_plane_t *primary;
> +       drmModeModeInfo *mode;
> +       struct igt_fb fb;
> +       int fb_id;
> +
> +       igt_output_set_pipe(output, pipe);
> +
> +       mode = igt_output_get_mode(output);
> +
> +       primary = igt_output_get_plane(output, 0);
> +
> +       fb_id = igt_create_color_fb(data->drm_fd,
> +                                   mode->hdisplay, mode->vdisplay,
> +                                   DRM_FORMAT_XRGB8888, I915_TILING_NONE,
> +                                   1.0, 1.0, 1.0, &fb);
> +       igt_assert(fb_id >= 0);
> +
> +       igt_plane_set_fb(primary, &fb);
> +       return igt_display_try_commit2(&data->display, COMMIT_LEGACY);
> +}
> +
> +static int
> +set_big_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> +{
> +       igt_output_override_mode(output, &mode_3_lanes);
> +       return set_mode_on_pipe(data, pipe, output);
> +}
> +
> +static int
> +set_normal_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> +{
> +       igt_output_override_mode(output, &mode_2_lanes);
> +       return set_mode_on_pipe(data, pipe, output);
> +}
> +
> +static void
> +find_outputs(data_t *data, igt_output_t **output1, igt_output_t **output2)
> +{
> +       int count = 0;
> +       igt_output_t *output;
> +
> +       *output1 = NULL;
> +       *output2 = NULL;
> +
> +       for_each_connected_output(&data->display, output) {
> +               if (!(*output1))
> +                       *output1 = output;
> +               else if (!(*output2))
> +                       *output2 = output;
> +
> +               igt_output_set_pipe(output, PIPE_ANY);
> +               count++;
> +       }
> +
> +       igt_skip_on_f(count < 2, "Not enough connected outputs\n");
> +}
> +
> +static void
> +test_dpms(data_t *data)
> +{
> +       igt_output_t *output1, *output2;
> +       int ret;
> +
> +       find_outputs(data, &output1, &output2);
> +
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_B), igt_output_name(output1));
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_C), igt_output_name(output2));
> +
> +       ret = set_big_mode_on_pipe(data, PIPE_B, output1);
> +       igt_assert(ret == 0);
> +
> +       kmstest_set_connector_dpms(data->drm_fd, output1->config.connector, DRM_MODE_DPMS_OFF);
> +
> +       ret = set_big_mode_on_pipe(data, PIPE_C, output2);
> +       igt_assert(ret != 0);
> +}
> +
> +static void
> +test_lane_reduction(data_t *data)
> +{
> +       igt_output_t *output1, *output2;
> +       int ret;
> +
> +       find_outputs(data, &output1, &output2);
> +
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_B), igt_output_name(output1));
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_C), igt_output_name(output2));
> +
> +       ret = set_big_mode_on_pipe(data, PIPE_B, output1);
> +       igt_assert(ret == 0);
> +
> +       ret = set_normal_mode_on_pipe(data, PIPE_B, output1);
> +       igt_assert(ret == 0);
> +
> +       ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
> +       igt_assert(ret == 0);
> +}
> +
> +static void
> +test_disable_pipe_B(data_t *data)
> +{
> +       igt_output_t *output1, *output2;
> +       int ret;
> +
> +       find_outputs(data, &output1, &output2);
> +
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_B), igt_output_name(output1));
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_C), igt_output_name(output2));
> +
> +       ret = set_big_mode_on_pipe(data, PIPE_B, output1);
> +       igt_assert(ret == 0);
> +
> +       ret = disable_pipe(data, PIPE_B, output1);
> +       igt_assert(ret == 0);
> +
> +       ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
> +       igt_assert(ret == 0);
> +
> +       ret = set_normal_mode_on_pipe(data, PIPE_B, output1);
> +       igt_assert(ret == 0);
> +}
> +
> +static void
> +test_from_C_to_B_with_3_lanes(data_t *data)
> +{
> +       igt_output_t *output1, *output2;
> +       int ret;
> +
> +       find_outputs(data, &output1, &output2);
> +
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_B), igt_output_name(output1));
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_C), igt_output_name(output2));
> +
> +       ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
> +       igt_assert(ret == 0);
> +
> +       ret = disable_pipe(data, PIPE_C, output2);
> +       igt_assert(ret == 0);
> +
> +       ret = set_big_mode_on_pipe(data, PIPE_B, output1);
> +       igt_assert(ret == 0);
> +}
> +
> +static void
> +test_fail_enable_pipe_C_while_B_has_3_lanes(data_t *data)
> +{
> +       igt_output_t *output1, *output2;
> +       int ret;
> +
> +       find_outputs(data, &output1, &output2);
> +
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_B), igt_output_name(output1));
> +       igt_info("Pipe %s will use connector %s\n",
> +                kmstest_pipe_name(PIPE_C), igt_output_name(output2));
> +
> +       ret = set_big_mode_on_pipe(data, PIPE_B, output1);
> +       igt_assert(ret == 0);
> +
> +       ret = set_normal_mode_on_pipe(data, PIPE_C, output2);
> +       igt_assert(ret != 0);
> +}
> +
> +static data_t data;
> +igt_main
> +{
> +       int devid;
> +
> +       igt_skip_on_simulation();
> +
> +       igt_fixture {
> +               data.drm_fd = drm_open_any_master();
> +               devid = intel_get_drm_devid(data.drm_fd);
> +
> +               kmstest_set_vt_graphics_mode();
> +               igt_display_init(&data.display, data.drm_fd);
> +       }
> +
> +       igt_skip_on(!IS_IVYBRIDGE(devid));
> +
> +       igt_subtest("pipe-B-dpms-off-modeset-pipe-C")
> +               test_dpms(&data);
> +
> +       igt_subtest("pipe-B-double-modeset-then-modeset-pipe-C")
> +               test_lane_reduction(&data);
> +
> +       igt_subtest("disable-pipe-B-enable-pipe-C")
> +               test_disable_pipe_B(&data);
> +
> +       igt_subtest("from-pipe-C-to-B-with-3-lanes")
> +               test_from_C_to_B_with_3_lanes(&data);
> +
> +       igt_subtest("enable-pipe-C-while-B-has-3-lanes")
> +               test_fail_enable_pipe_C_while_B_has_3_lanes(&data);
> +
> +       igt_fixture {
> +               igt_display_fini(&data.display);
> +       }
> +}
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt 1/2] lib/kms: Add a way to override an output's mode

[Damien Lespiau]

On Wed, Mar 11, 2015 at 01:33:00PM +0200, Ander Conselvan de Oliveira wrote:
> So that it is possible to use a custom mode with the simplified mode set API.

Maybe just igt_output_set_mode()?

-- 
Damien

> ---
>  lib/igt_kms.c | 9 +++++++++
>  lib/igt_kms.h | 4 ++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 26e4913..0dccd2d 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -895,6 +895,9 @@ static void igt_output_refresh(igt_output_t *output)
>  	if (!output->valid)
>  		return;
>  
> +	if (output->use_override_mode)
> +		output->config.default_mode = output->override_mode;
> +
>  	if (!output->name) {
>  		drmModeConnector *c = output->config.connector;
>  
> @@ -1797,6 +1800,12 @@ drmModeModeInfo *igt_output_get_mode(igt_output_t *output)
>  	return &output->config.default_mode;
>  }
>  
> +void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode)
> +{
> +	output->override_mode = *mode;
> +	output->use_override_mode = true;
> +}
> +
>  void igt_output_set_pipe(igt_output_t *output, enum pipe pipe)
>  {
>  	igt_display_t *display = output->display;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 2fab30e..ddf4432 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -228,6 +228,9 @@ typedef struct {
>  	bool valid;
>  	unsigned long pending_crtc_idx_mask;
>  
> +	bool use_override_mode;
> +	drmModeModeInfo override_mode;
> +
>  #ifdef HAVE_ATOMIC
>  	/* Property set for nuclear pageflip */
>  	drmModePropertySetPtr set;
> @@ -255,6 +258,7 @@ int  igt_display_get_n_pipes(igt_display_t *display);
>  
>  const char *igt_output_name(igt_output_t *output);
>  drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
> +void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode);
>  void igt_output_set_pipe(igt_output_t *output, enum pipe pipe);
>  igt_plane_t *igt_output_get_plane(igt_output_t *output, enum igt_plane plane);
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt 1/2] lib/kms: Add a way to override an output's mode

[Ander Conselvan De Oliveira]

On Wed, 2015-03-11 at 13:26 +0000, Damien Lespiau wrote:
> On Wed, Mar 11, 2015 at 01:33:00PM +0200, Ander Conselvan de Oliveira wrote:
> > So that it is possible to use a custom mode with the simplified mode set API.
> 
> Maybe just igt_output_set_mode()?

That works too. I used override since there's a chance the desired mode
won't produce the expected results. But now that I think about it "force
mode" would sound more like that. In any case, I don't mind either way.

Ander



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

Re: [PATCH igt 1/2] lib/kms: Add a way to override an output's mode

[Damien Lespiau]

On Wed, Mar 11, 2015 at 03:48:00PM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2015-03-11 at 13:26 +0000, Damien Lespiau wrote:
> > On Wed, Mar 11, 2015 at 01:33:00PM +0200, Ander Conselvan de Oliveira wrote:
> > > So that it is possible to use a custom mode with the simplified mode set API.
> > 
> > Maybe just igt_output_set_mode()?
> 
> That works too. I used override since there's a chance the desired mode
> won't produce the expected results. But now that I think about it "force
> mode" would sound more like that. In any case, I don't mind either way.

No, me neither, can go as is anyway.

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt 1/2] lib/kms: Add a way to override an output's mode

[Thomas Wood]

On 11 March 2015 at 11:33, Ander Conselvan de Oliveira
<ander.conselvan.de.oliveira@intel.com> wrote:
> So that it is possible to use a custom mode with the simplified mode set API.
> ---
>  lib/igt_kms.c | 9 +++++++++
>  lib/igt_kms.h | 4 ++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 26e4913..0dccd2d 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -895,6 +895,9 @@ static void igt_output_refresh(igt_output_t *output)
>         if (!output->valid)
>                 return;
>
> +       if (output->use_override_mode)
> +               output->config.default_mode = output->override_mode;
> +
>         if (!output->name) {
>                 drmModeConnector *c = output->config.connector;
>
> @@ -1797,6 +1800,12 @@ drmModeModeInfo *igt_output_get_mode(igt_output_t *output)
>         return &output->config.default_mode;
>  }
>


Please add some API documentation for the new function here.

> +void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode)
> +{
> +       output->override_mode = *mode;
> +       output->use_override_mode = true;
> +}
> +
>  void igt_output_set_pipe(igt_output_t *output, enum pipe pipe)
>  {
>         igt_display_t *display = output->display;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 2fab30e..ddf4432 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -228,6 +228,9 @@ typedef struct {
>         bool valid;
>         unsigned long pending_crtc_idx_mask;
>
> +       bool use_override_mode;
> +       drmModeModeInfo override_mode;
> +
>  #ifdef HAVE_ATOMIC
>         /* Property set for nuclear pageflip */
>         drmModePropertySetPtr set;
> @@ -255,6 +258,7 @@ int  igt_display_get_n_pipes(igt_display_t *display);
>
>  const char *igt_output_name(igt_output_t *output);
>  drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
> +void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode);
>  void igt_output_set_pipe(igt_output_t *output, enum pipe pipe);
>  igt_plane_t *igt_output_get_plane(igt_output_t *output, enum igt_plane plane);
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Ville Syrjälä]

On Tue, Mar 10, 2015 at 02:32:34PM +0200, Ander Conselvan de Oliveira wrote:
> Remove the global modeset resource function that would disable the
> bifurcation bit, and instead enable/disable it when enabling or
> disabling the pch transcoder. The checks before the mode set should
> ensure that the configuration is valid, so it should be safe to do it
> so.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
> 
> On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> > With atomic we need to probably look at crtc_state->mode_changed. As an
> > interim solution we have the same information available in the
> > modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> > with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.
> 
> I looked into that solution, but involves passing modeset_pipes way down
> into the call stack, so I started looking for a different solution. Do you
> see any caveat with this approach?
> 
> Thanks,
> Ander
> 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4008bf4..eca5a41 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -101,6 +101,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config);
>  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> +						bool pipe_active);
>  
>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>  {
> @@ -1956,6 +1958,8 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  					    enum pipe pipe)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> +	struct intel_crtc *crtc =
> +		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  	uint32_t reg, val;
>  
>  	/* FDI relies on the transcoder */
> @@ -1980,6 +1984,9 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  		val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
>  		I915_WRITE(reg, val);
>  	}
> +
> +	if (IS_IVYBRIDGE(dev))
> +		ivybridge_update_fdi_bc_bifurcation(crtc, false);
>  }
>  
>  static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> @@ -3153,32 +3160,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
>  }
>  
> -static void ivb_modeset_global_resources(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *pipe_B_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> -	struct intel_crtc *pipe_C_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> -	uint32_t temp;
> -
> -	/*
> -	 * When everything is off disable fdi C so that we could enable fdi B
> -	 * with all lanes. Note that we don't care about enabled pipes without
> -	 * an enabled pch encoder.
> -	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> -
> -		temp = I915_READ(SOUTH_CHICKEN1);
> -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> -		I915_WRITE(SOUTH_CHICKEN1, temp);
> -	}
> -}
> -
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -3834,41 +3815,44 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
>  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
>  }
>  
> -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t temp;
>  
>  	temp = I915_READ(SOUTH_CHICKEN1);
> -	if (temp & FDI_BC_BIFURCATION_SELECT)
> +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
>  		return;
>  
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
>  
> -	temp |= FDI_BC_BIFURCATION_SELECT;
> -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> +	if (enable)
> +		temp |= FDI_BC_BIFURCATION_SELECT;
> +
> +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
>  	I915_WRITE(SOUTH_CHICKEN1, temp);
>  	POSTING_READ(SOUTH_CHICKEN1);
>  }
>  
> -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> +						bool pipe_active)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	switch (intel_crtc->pipe) {
>  	case PIPE_A:
>  		break;
>  	case PIPE_B:
> -		if (intel_crtc->config->fdi_lanes > 2)
> -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> +		if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
> +			cpt_enable_fdi_bc_bifurcation(dev, false);
>  		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> +			cpt_enable_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> +		cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
>  

It sees could now change the bifurcation while pipe B is active (but only
using two lanes). Not sure if the old code did that too, or if it might
cause problems.

Also depending on the order you disable the pipes you might end up with
bifurcation enabled or disabled.

It seems to me that the simplest solution should be to have bifurcation
enabled at all times, except when pipe B really needs the four lanes.
Then the hardware state would only need to be changed when
enabling/disabling pipe B with four lanes. Rest of the time
crtc->enabled and fdi_lanes should be able to tell us which
configuration is possible and which not. Or am I missing something?

Well, eventually we want to tie this into the atomic state so that we
can actaully have pipe B drop into two lane mode if pipe C needs the
lanes (and pipe B can still operate with two lanes). But I guess that's
still not achievable with the current code.


>  		break;
>  	default:
> @@ -3895,7 +3879,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  	assert_pch_transcoder_disabled(dev_priv, pipe);
>  
>  	if (IS_IVYBRIDGE(dev))
> -		ivybridge_update_fdi_bc_bifurcation(intel_crtc);
> +		ivybridge_update_fdi_bc_bifurcation(intel_crtc, true);
>  
>  	/* Write the TU size bits before fdi link training, so that error
>  	 * detection works. */
> @@ -13056,8 +13040,6 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_IVYBRIDGE(dev)) {
>  		/* FIXME: detect B0+ stepping and use auto training */
>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> -		dev_priv->display.modeset_global_resources =
> -			ivb_modeset_global_resources;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>  	} else if (IS_VALLEYVIEW(dev)) {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Daniel Vetter]

On Tue, Mar 10, 2015 at 03:03:59PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 10, 2015 at 02:32:34PM +0200, Ander Conselvan de Oliveira wrote:
> > Remove the global modeset resource function that would disable the
> > bifurcation bit, and instead enable/disable it when enabling or
> > disabling the pch transcoder. The checks before the mode set should
> > ensure that the configuration is valid, so it should be safe to do it
> > so.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > ---
> > 
> > On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> > > With atomic we need to probably look at crtc_state->mode_changed. As an
> > > interim solution we have the same information available in the
> > > modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> > > with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.
> > 
> > I looked into that solution, but involves passing modeset_pipes way down
> > into the call stack, so I started looking for a different solution. Do you
> > see any caveat with this approach?
> > 
> > Thanks,
> > Ander
> > 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
> >  1 file changed, 21 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4008bf4..eca5a41 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -101,6 +101,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
> >  			    const struct intel_crtc_state *pipe_config);
> >  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
> >  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
> > +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> > +						bool pipe_active);
> >  
> >  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
> >  {
> > @@ -1956,6 +1958,8 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
> >  					    enum pipe pipe)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> > +	struct intel_crtc *crtc =
> > +		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >  	uint32_t reg, val;
> >  
> >  	/* FDI relies on the transcoder */
> > @@ -1980,6 +1984,9 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
> >  		val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
> >  		I915_WRITE(reg, val);
> >  	}
> > +
> > +	if (IS_IVYBRIDGE(dev))
> > +		ivybridge_update_fdi_bc_bifurcation(crtc, false);
> >  }
> >  
> >  static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> > @@ -3153,32 +3160,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> >  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
> >  }
> >  
> > -static void ivb_modeset_global_resources(struct drm_device *dev)
> > -{
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *pipe_B_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> > -	struct intel_crtc *pipe_C_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> > -	uint32_t temp;
> > -
> > -	/*
> > -	 * When everything is off disable fdi C so that we could enable fdi B
> > -	 * with all lanes. Note that we don't care about enabled pipes without
> > -	 * an enabled pch encoder.
> > -	 */
> > -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> > -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> > -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> > -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> > -
> > -		temp = I915_READ(SOUTH_CHICKEN1);
> > -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> > -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> > -		I915_WRITE(SOUTH_CHICKEN1, temp);
> > -	}
> > -}
> > -
> >  /* The FDI link training functions for ILK/Ibexpeak. */
> >  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
> >  {
> > @@ -3834,41 +3815,44 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
> >  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
> >  }
> >  
> > -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> > +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t temp;
> >  
> >  	temp = I915_READ(SOUTH_CHICKEN1);
> > -	if (temp & FDI_BC_BIFURCATION_SELECT)
> > +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
> >  		return;
> >  
> >  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> >  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> >  
> > -	temp |= FDI_BC_BIFURCATION_SELECT;
> > -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> > +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> > +	if (enable)
> > +		temp |= FDI_BC_BIFURCATION_SELECT;
> > +
> > +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
> >  	I915_WRITE(SOUTH_CHICKEN1, temp);
> >  	POSTING_READ(SOUTH_CHICKEN1);
> >  }
> >  
> > -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> > +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> > +						bool pipe_active)
> >  {
> >  	struct drm_device *dev = intel_crtc->base.dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	switch (intel_crtc->pipe) {
> >  	case PIPE_A:
> >  		break;
> >  	case PIPE_B:
> > -		if (intel_crtc->config->fdi_lanes > 2)
> > -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> > +		if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
> > +			cpt_enable_fdi_bc_bifurcation(dev, false);
> >  		else
> > -			cpt_enable_fdi_bc_bifurcation(dev);
> > +			cpt_enable_fdi_bc_bifurcation(dev, true);
> >  
> >  		break;
> >  	case PIPE_C:
> > -		cpt_enable_fdi_bc_bifurcation(dev);
> > +		cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
> >  
> 
> It sees could now change the bifurcation while pipe B is active (but only
> using two lanes). Not sure if the old code did that too, or if it might
> cause problems.

It shouldn't be possible and it'll anger the hw.
cpt_enable_fdi_bc_bifurcation has WARN_ONs to make sure the fdi rx for
both pch transcoder B & C are off to make sure we don't get this wrong.

> Also depending on the order you disable the pipes you might end up with
> bifurcation enabled or disabled.
> 
> It seems to me that the simplest solution should be to have bifurcation
> enabled at all times, except when pipe B really needs the four lanes.
> Then the hardware state would only need to be changed when
> enabling/disabling pipe B with four lanes. Rest of the time
> crtc->enabled and fdi_lanes should be able to tell us which
> configuration is possible and which not. Or am I missing something?
> 
> Well, eventually we want to tie this into the atomic state so that we
> can actaully have pipe B drop into two lane mode if pipe C needs the
> lanes (and pipe B can still operate with two lanes). But I guess that's
> still not achievable with the current code.

Checking fdi lane constraints is already done in the compute_config code.
And it assumes that it can always get the max, i.e. if pipe B is only
using 2 lanes it'll just enable pipe C. Which implies that we indeed have
to aggressive enable the bifurcate bit (since atm we don't support a
modeset on pipe B). For atomic modeset we'd just need to extend that code
to work with all pipes (like all the other compute_config code). That
shouldn't be a lot fo trouble (since we can always throw the crtc for the
other pipe into the update in atomic_check functions).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Ander Conselvan de Oliveira]

Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling the pch
transcoder. The mode set consistency check should prevent us from
disabling the bit if pipe C is enabled so the change should be safe.

Note that this doens't affect the logic that prevents the bit being
set while a pipe is active, since the patch retains the behavior of
only chaging the bit if necessary. Because of the checks during mode
set, the first change would necessarily happen with both pipes B and
C disabled, and any subsequent write would be skipped.

v2: Only change the bit during pch trancoder enable. (Ville)
---
 drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
 1 file changed, 10 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..bfbd829 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
 	return crtc->base.state->enable && crtc->config->has_pch_encoder;
 }
 
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *pipe_B_crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
-	struct intel_crtc *pipe_C_crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
-	uint32_t temp;
-
-	/*
-	 * When everything is off disable fdi C so that we could enable fdi B
-	 * with all lanes. Note that we don't care about enabled pipes without
-	 * an enabled pch encoder.
-	 */
-	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
-	    !pipe_has_enabled_pch(pipe_C_crtc)) {
-		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
-		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
-
-		temp = I915_READ(SOUTH_CHICKEN1);
-		temp &= ~FDI_BC_BIFURCATION_SELECT;
-		DRM_DEBUG_KMS("disabling fdi C rx\n");
-		I915_WRITE(SOUTH_CHICKEN1, temp);
-	}
-}
-
 /* The FDI link training functions for ILK/Ibexpeak. */
 static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 {
@@ -3834,20 +3808,23 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
 		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
 }
 
-static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t temp;
 
 	temp = I915_READ(SOUTH_CHICKEN1);
-	if (temp & FDI_BC_BIFURCATION_SELECT)
+	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
 		return;
 
 	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
 	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
 
-	temp |= FDI_BC_BIFURCATION_SELECT;
-	DRM_DEBUG_KMS("enabling fdi C rx\n");
+	temp &= ~FDI_BC_BIFURCATION_SELECT;
+	if (enable)
+		temp |= FDI_BC_BIFURCATION_SELECT;
+
+	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
 	I915_WRITE(SOUTH_CHICKEN1, temp);
 	POSTING_READ(SOUTH_CHICKEN1);
 }
@@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
 static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	switch (intel_crtc->pipe) {
 	case PIPE_A:
 		break;
 	case PIPE_B:
 		if (intel_crtc->config->fdi_lanes > 2)
-			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
+			cpt_set_fdi_bc_bifurcation(dev, false);
 		else
-			cpt_enable_fdi_bc_bifurcation(dev);
+			cpt_set_fdi_bc_bifurcation(dev, true);
 
 		break;
 	case PIPE_C:
-		cpt_enable_fdi_bc_bifurcation(dev);
+		cpt_set_fdi_bc_bifurcation(dev, true);
 
 		break;
 	default:
@@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
 	} else if (IS_IVYBRIDGE(dev)) {
 		/* FIXME: detect B0+ stepping and use auto training */
 		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
-		dev_priv->display.modeset_global_resources =
-			ivb_modeset_global_resources;
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 	} else if (IS_VALLEYVIEW(dev)) {
-- 
2.1.0

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

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Conselvan De Oliveira, Ander]

On Wed, 2015-03-11 at 13:35 +0200, Ander Conselvan de Oliveira wrote:
> Remove the global modeset resource function that would disable the
> bifurcation bit, and instead enable/disable it when enabling the pch
> transcoder. The mode set consistency check should prevent us from
> disabling the bit if pipe C is enabled so the change should be safe.
> 
> Note that this doens't affect the logic that prevents the bit being
> set while a pipe is active, since the patch retains the behavior of
> only chaging the bit if necessary. Because of the checks during mode
> set, the first change would necessarily happen with both pipes B and
> C disabled, and any subsequent write would be skipped.
> 
> v2: Only change the bit during pch trancoder enable. (Ville)

Oops, I forgot the sob line.

Signed-off-by: Ander Conselvan de Oliveira
<ander.conselvan.de.oliveira@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
>  1 file changed, 10 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4008bf4..bfbd829 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
>  }
>  
> -static void ivb_modeset_global_resources(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *pipe_B_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> -	struct intel_crtc *pipe_C_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> -	uint32_t temp;
> -
> -	/*
> -	 * When everything is off disable fdi C so that we could enable fdi B
> -	 * with all lanes. Note that we don't care about enabled pipes without
> -	 * an enabled pch encoder.
> -	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> -
> -		temp = I915_READ(SOUTH_CHICKEN1);
> -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> -		I915_WRITE(SOUTH_CHICKEN1, temp);
> -	}
> -}
> -
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -3834,20 +3808,23 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
>  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
>  }
>  
> -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> +static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t temp;
>  
>  	temp = I915_READ(SOUTH_CHICKEN1);
> -	if (temp & FDI_BC_BIFURCATION_SELECT)
> +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
>  		return;
>  
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
>  
> -	temp |= FDI_BC_BIFURCATION_SELECT;
> -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> +	if (enable)
> +		temp |= FDI_BC_BIFURCATION_SELECT;
> +
> +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
>  	I915_WRITE(SOUTH_CHICKEN1, temp);
>  	POSTING_READ(SOUTH_CHICKEN1);
>  }
> @@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
>  static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	switch (intel_crtc->pipe) {
>  	case PIPE_A:
>  		break;
>  	case PIPE_B:
>  		if (intel_crtc->config->fdi_lanes > 2)
> -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> +			cpt_set_fdi_bc_bifurcation(dev, false);
>  		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> +			cpt_set_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> +		cpt_set_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	default:
> @@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_IVYBRIDGE(dev)) {
>  		/* FIXME: detect B0+ stepping and use auto training */
>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> -		dev_priv->display.modeset_global_resources =
> -			ivb_modeset_global_resources;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>  	} else if (IS_VALLEYVIEW(dev)) {

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Ville Syrjälä]

On Wed, Mar 11, 2015 at 11:37:54AM +0000, Conselvan De Oliveira, Ander wrote:
> On Wed, 2015-03-11 at 13:35 +0200, Ander Conselvan de Oliveira wrote:
> > Remove the global modeset resource function that would disable the
> > bifurcation bit, and instead enable/disable it when enabling the pch
> > transcoder. The mode set consistency check should prevent us from
> > disabling the bit if pipe C is enabled so the change should be safe.
> > 
> > Note that this doens't affect the logic that prevents the bit being
> > set while a pipe is active, since the patch retains the behavior of
> > only chaging the bit if necessary. Because of the checks during mode
> > set, the first change would necessarily happen with both pipes B and
> > C disabled, and any subsequent write would be skipped.
> > 
> > v2: Only change the bit during pch trancoder enable. (Ville)
> 
> Oops, I forgot the sob line.
> 
> Signed-off-by: Ander Conselvan de Oliveira
> <ander.conselvan.de.oliveira@intel.com>


So I was staring at this stuff for a while and I believe it should be
fine. We don't keep the bifurcation state entirely consistent when
neither of the the pipes B/C are actually driving a PCH transcoder, but
that shouldn't really matter. If we want to make it consistent then I
suggest that we go with my earlier idea of only changing the state at
transcoder B with >2 lanes enable/disable, and otherwise keep it enabled
all the time. The slight complication there is the initial state we get
from the BIOS which might not match that, so we'd need to sanitize it
or something.

Anyway, I also posted a couple of patches on top that try to sort out
ironlake_check_fdi_lanes() [1]. With those and this one I think things
should work even better than before.

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

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-March/061828.html

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
> >  1 file changed, 10 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4008bf4..bfbd829 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> >  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
> >  }
> >  
> > -static void ivb_modeset_global_resources(struct drm_device *dev)
> > -{
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *pipe_B_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> > -	struct intel_crtc *pipe_C_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> > -	uint32_t temp;
> > -
> > -	/*
> > -	 * When everything is off disable fdi C so that we could enable fdi B
> > -	 * with all lanes. Note that we don't care about enabled pipes without
> > -	 * an enabled pch encoder.
> > -	 */
> > -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> > -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> > -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> > -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> > -
> > -		temp = I915_READ(SOUTH_CHICKEN1);
> > -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> > -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> > -		I915_WRITE(SOUTH_CHICKEN1, temp);
> > -	}
> > -}
> > -
> >  /* The FDI link training functions for ILK/Ibexpeak. */
> >  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
> >  {
> > @@ -3834,20 +3808,23 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
> >  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
> >  }
> >  
> > -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> > +static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t temp;
> >  
> >  	temp = I915_READ(SOUTH_CHICKEN1);
> > -	if (temp & FDI_BC_BIFURCATION_SELECT)
> > +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
> >  		return;
> >  
> >  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> >  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> >  
> > -	temp |= FDI_BC_BIFURCATION_SELECT;
> > -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> > +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> > +	if (enable)
> > +		temp |= FDI_BC_BIFURCATION_SELECT;
> > +
> > +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
> >  	I915_WRITE(SOUTH_CHICKEN1, temp);
> >  	POSTING_READ(SOUTH_CHICKEN1);
> >  }
> > @@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> >  static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> >  {
> >  	struct drm_device *dev = intel_crtc->base.dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	switch (intel_crtc->pipe) {
> >  	case PIPE_A:
> >  		break;
> >  	case PIPE_B:
> >  		if (intel_crtc->config->fdi_lanes > 2)
> > -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> > +			cpt_set_fdi_bc_bifurcation(dev, false);
> >  		else
> > -			cpt_enable_fdi_bc_bifurcation(dev);
> > +			cpt_set_fdi_bc_bifurcation(dev, true);
> >  
> >  		break;
> >  	case PIPE_C:
> > -		cpt_enable_fdi_bc_bifurcation(dev);
> > +		cpt_set_fdi_bc_bifurcation(dev, true);
> >  
> >  		break;
> >  	default:
> > @@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
> >  	} else if (IS_IVYBRIDGE(dev)) {
> >  		/* FIXME: detect B0+ stepping and use auto training */
> >  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> > -		dev_priv->display.modeset_global_resources =
> > -			ivb_modeset_global_resources;
> >  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> >  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> >  	} else if (IS_VALLEYVIEW(dev)) {
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Daniel Vetter]

On Wed, Mar 11, 2015 at 06:58:12PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 11, 2015 at 11:37:54AM +0000, Conselvan De Oliveira, Ander wrote:
> > On Wed, 2015-03-11 at 13:35 +0200, Ander Conselvan de Oliveira wrote:
> > > Remove the global modeset resource function that would disable the
> > > bifurcation bit, and instead enable/disable it when enabling the pch
> > > transcoder. The mode set consistency check should prevent us from
> > > disabling the bit if pipe C is enabled so the change should be safe.
> > > 
> > > Note that this doens't affect the logic that prevents the bit being
> > > set while a pipe is active, since the patch retains the behavior of
> > > only chaging the bit if necessary. Because of the checks during mode
> > > set, the first change would necessarily happen with both pipes B and
> > > C disabled, and any subsequent write would be skipped.
> > > 
> > > v2: Only change the bit during pch trancoder enable. (Ville)
> > 
> > Oops, I forgot the sob line.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira
> > <ander.conselvan.de.oliveira@intel.com>
> 
> 
> So I was staring at this stuff for a while and I believe it should be
> fine. We don't keep the bifurcation state entirely consistent when
> neither of the the pipes B/C are actually driving a PCH transcoder, but
> that shouldn't really matter. If we want to make it consistent then I
> suggest that we go with my earlier idea of only changing the state at
> transcoder B with >2 lanes enable/disable, and otherwise keep it enabled
> all the time. The slight complication there is the initial state we get
> from the BIOS which might not match that, so we'd need to sanitize it
> or something.
> 
> Anyway, I also posted a couple of patches on top that try to sort out
> ironlake_check_fdi_lanes() [1]. With those and this one I think things
> should work even better than before.
> 
> So for this patch:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel
>-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Ville Syrjälä]

On Wed, Mar 11, 2015 at 01:35:43PM +0200, Ander Conselvan de Oliveira wrote:
> Remove the global modeset resource function that would disable the
> bifurcation bit, and instead enable/disable it when enabling the pch
> transcoder. The mode set consistency check should prevent us from
> disabling the bit if pipe C is enabled so the change should be safe.
> 
> Note that this doens't affect the logic that prevents the bit being
> set while a pipe is active, since the patch retains the behavior of
> only chaging the bit if necessary. Because of the checks during mode
> set, the first change would necessarily happen with both pipes B and
> C disabled, and any subsequent write would be skipped.
> 
> v2: Only change the bit during pch trancoder enable. (Ville)


Actually what I had inb mind was something like this:

pch_enable()
{
	if (pipe == B && fdi_lanes > 2)
		disable_bifurcation()
	...
}

pch_disable()
{
	...
	if (pipe == B && fdi_lanes > 2)
		enable_bifurcation()
}

So we only change it when enabling/disabling pipe B, never for pipe C.
Hence it remains enabled whenever pipe B doesn't need >2 FDI lanes.


> ---
>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
>  1 file changed, 10 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4008bf4..bfbd829 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
>  }
>  
> -static void ivb_modeset_global_resources(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *pipe_B_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> -	struct intel_crtc *pipe_C_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> -	uint32_t temp;
> -
> -	/*
> -	 * When everything is off disable fdi C so that we could enable fdi B
> -	 * with all lanes. Note that we don't care about enabled pipes without
> -	 * an enabled pch encoder.
> -	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> -
> -		temp = I915_READ(SOUTH_CHICKEN1);
> -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> -		I915_WRITE(SOUTH_CHICKEN1, temp);
> -	}
> -}
> -
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -3834,20 +3808,23 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
>  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
>  }
>  
> -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> +static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t temp;
>  
>  	temp = I915_READ(SOUTH_CHICKEN1);
> -	if (temp & FDI_BC_BIFURCATION_SELECT)
> +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
>  		return;
>  
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
>  
> -	temp |= FDI_BC_BIFURCATION_SELECT;
> -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> +	if (enable)
> +		temp |= FDI_BC_BIFURCATION_SELECT;
> +
> +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
>  	I915_WRITE(SOUTH_CHICKEN1, temp);
>  	POSTING_READ(SOUTH_CHICKEN1);
>  }
> @@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
>  static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	switch (intel_crtc->pipe) {
>  	case PIPE_A:
>  		break;
>  	case PIPE_B:
>  		if (intel_crtc->config->fdi_lanes > 2)
> -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> +			cpt_set_fdi_bc_bifurcation(dev, false);
>  		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> +			cpt_set_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> +		cpt_set_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	default:
> @@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_IVYBRIDGE(dev)) {
>  		/* FIXME: detect B0+ stepping and use auto training */
>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> -		dev_priv->display.modeset_global_resources =
> -			ivb_modeset_global_resources;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>  	} else if (IS_VALLEYVIEW(dev)) {
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Ville Syrjälä]

On Wed, Mar 11, 2015 at 01:35:43PM +0200, Ander Conselvan de Oliveira wrote:
> Remove the global modeset resource function that would disable the
> bifurcation bit, and instead enable/disable it when enabling the pch
> transcoder. The mode set consistency check should prevent us from
> disabling the bit if pipe C is enabled so the change should be safe.
> 
> Note that this doens't affect the logic that prevents the bit being
> set while a pipe is active, since the patch retains the behavior of
> only chaging the bit if necessary. Because of the checks during mode
> set, the first change would necessarily happen with both pipes B and
> C disabled, and any subsequent write would be skipped.
> 
> v2: Only change the bit during pch trancoder enable. (Ville)
> ---
>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
>  1 file changed, 10 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4008bf4..bfbd829 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
>  }
>  
> -static void ivb_modeset_global_resources(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *pipe_B_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> -	struct intel_crtc *pipe_C_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> -	uint32_t temp;
> -
> -	/*
> -	 * When everything is off disable fdi C so that we could enable fdi B
> -	 * with all lanes. Note that we don't care about enabled pipes without
> -	 * an enabled pch encoder.
> -	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> -
> -		temp = I915_READ(SOUTH_CHICKEN1);
> -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> -		I915_WRITE(SOUTH_CHICKEN1, temp);
> -	}
> -}
> -
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -3834,20 +3808,23 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
>  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
>  }
>  
> -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> +static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t temp;
>  
>  	temp = I915_READ(SOUTH_CHICKEN1);
> -	if (temp & FDI_BC_BIFURCATION_SELECT)
> +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
>  		return;
>  
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
>  
> -	temp |= FDI_BC_BIFURCATION_SELECT;
> -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> +	if (enable)
> +		temp |= FDI_BC_BIFURCATION_SELECT;
> +
> +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
>  	I915_WRITE(SOUTH_CHICKEN1, temp);
>  	POSTING_READ(SOUTH_CHICKEN1);
>  }
> @@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
>  static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	switch (intel_crtc->pipe) {
>  	case PIPE_A:
>  		break;
>  	case PIPE_B:
>  		if (intel_crtc->config->fdi_lanes > 2)
> -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> +			cpt_set_fdi_bc_bifurcation(dev, false);

So we just replace the globla_resources enforced assumed state with an
explicit state change here. Should be perfectly fine since pipe is not
active at this point.


What really confuses me about the whole FDI bifurcation code is
ironlake_check_fdi_lanes(). First of all I would rewrite it a bit like
so:

--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5615,14 +5615,13 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
                }
                return true;
        case PIPE_C:
-               if (!pipe_has_enabled_pch(pipe_B_crtc) ||
-                   pipe_B_crtc->config->fdi_lanes <= 2) {
-                       if (pipe_config->fdi_lanes > 2) {
-                               DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %c: %i lanes\n",
-                                             pipe_name(pipe), pipe_config->fdi_lanes);
-                               return false;
-                       }
-               } else {
+               if (pipe_config->fdi_lanes > 2) {
+                       DRM_DEBUG_KMS("only 2 lanes on pipe %c: required %i lanes\n",
+                                     pipe_name(pipe), pipe_config->fdi_lanes);
+                       return false;
+               }
+               if (pipe_has_enabled_pch(pipe_B_crtc) &&
+                   pipe_B_crtc->config->fdi_lanes > 2) {
                        DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
                        return false;
                }

And the next thing confusing me is why we use pipe_has_enabled_pch()
to check pipe B state, but just pipe->enabled to check pipe C state?

Consider the following scenario:
1. enable pipe B with PCH using >2 FDI lanes
2. set pipe B to DPMS off
3. enable pipe C with PCH

The crtc->active check in pipe_has_enabled_pch() will indicate that pipe
B does not need FDI, so step 3 will succeed. Or am I missing something
subtle here?

Also if we do things this way:
1. enable pipe C with eDP
2. enable pipe B with PCH using >2 FDI lanes

Now step 2 will fail even though pipe C doesn't need any FDI lanes.

So to fix that it would seem that we should remove the crtc->active
check from pipe_has_enabled_pch() and use that to check for conlicts
in both cases. Now that .global_resources() is no longer in the picture
the crtc->active check not needed anyway, I think.

Thoughts?

>  		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> +			cpt_set_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> +		cpt_set_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	default:
> @@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_IVYBRIDGE(dev)) {
>  		/* FIXME: detect B0+ stepping and use auto training */
>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> -		dev_priv->display.modeset_global_resources =
> -			ivb_modeset_global_resources;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>  	} else if (IS_VALLEYVIEW(dev)) {
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Conselvan De Oliveira, Ander]

On Wed, 2015-03-11 at 15:10 +0200, Ville Syrjälä wrote:
> On Wed, Mar 11, 2015 at 01:35:43PM +0200, Ander Conselvan de Oliveira wrote:
> > Remove the global modeset resource function that would disable the
> > bifurcation bit, and instead enable/disable it when enabling the pch
> > transcoder. The mode set consistency check should prevent us from
> > disabling the bit if pipe C is enabled so the change should be safe.
> > 
> > Note that this doens't affect the logic that prevents the bit being
> > set while a pipe is active, since the patch retains the behavior of
> > only chaging the bit if necessary. Because of the checks during mode
> > set, the first change would necessarily happen with both pipes B and
> > C disabled, and any subsequent write would be skipped.
> > 
> > v2: Only change the bit during pch trancoder enable. (Ville)
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++----------------------------
> >  1 file changed, 10 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4008bf4..bfbd829 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> >  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
> >  }
> >  
> > -static void ivb_modeset_global_resources(struct drm_device *dev)
> > -{
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *pipe_B_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> > -	struct intel_crtc *pipe_C_crtc =
> > -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> > -	uint32_t temp;
> > -
> > -	/*
> > -	 * When everything is off disable fdi C so that we could enable fdi B
> > -	 * with all lanes. Note that we don't care about enabled pipes without
> > -	 * an enabled pch encoder.
> > -	 */
> > -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> > -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> > -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> > -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> > -
> > -		temp = I915_READ(SOUTH_CHICKEN1);
> > -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> > -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> > -		I915_WRITE(SOUTH_CHICKEN1, temp);
> > -	}
> > -}
> > -
> >  /* The FDI link training functions for ILK/Ibexpeak. */
> >  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
> >  {
> > @@ -3834,20 +3808,23 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
> >  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
> >  }
> >  
> > -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> > +static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t temp;
> >  
> >  	temp = I915_READ(SOUTH_CHICKEN1);
> > -	if (temp & FDI_BC_BIFURCATION_SELECT)
> > +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
> >  		return;
> >  
> >  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> >  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> >  
> > -	temp |= FDI_BC_BIFURCATION_SELECT;
> > -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> > +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> > +	if (enable)
> > +		temp |= FDI_BC_BIFURCATION_SELECT;
> > +
> > +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
> >  	I915_WRITE(SOUTH_CHICKEN1, temp);
> >  	POSTING_READ(SOUTH_CHICKEN1);
> >  }
> > @@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> >  static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> >  {
> >  	struct drm_device *dev = intel_crtc->base.dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	switch (intel_crtc->pipe) {
> >  	case PIPE_A:
> >  		break;
> >  	case PIPE_B:
> >  		if (intel_crtc->config->fdi_lanes > 2)
> > -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> > +			cpt_set_fdi_bc_bifurcation(dev, false);
> 
> So we just replace the globla_resources enforced assumed state with an
> explicit state change here. Should be perfectly fine since pipe is not
> active at this point.
> 
> 
> What really confuses me about the whole FDI bifurcation code is
> ironlake_check_fdi_lanes(). First of all I would rewrite it a bit like
> so:
> 
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5615,14 +5615,13 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>                 }
>                 return true;
>         case PIPE_C:
> -               if (!pipe_has_enabled_pch(pipe_B_crtc) ||
> -                   pipe_B_crtc->config->fdi_lanes <= 2) {
> -                       if (pipe_config->fdi_lanes > 2) {
> -                               DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %c: %i lanes\n",
> -                                             pipe_name(pipe), pipe_config->fdi_lanes);
> -                               return false;
> -                       }
> -               } else {
> +               if (pipe_config->fdi_lanes > 2) {
> +                       DRM_DEBUG_KMS("only 2 lanes on pipe %c: required %i lanes\n",
> +                                     pipe_name(pipe), pipe_config->fdi_lanes);
> +                       return false;
> +               }
> +               if (pipe_has_enabled_pch(pipe_B_crtc) &&
> +                   pipe_B_crtc->config->fdi_lanes > 2) {
>                         DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n");
>                         return false;
>                 }
> 
> And the next thing confusing me is why we use pipe_has_enabled_pch()
> to check pipe B state, but just pipe->enabled to check pipe C state?
> 
> Consider the following scenario:
> 1. enable pipe B with PCH using >2 FDI lanes
> 2. set pipe B to DPMS off
> 3. enable pipe C with PCH
> 
> The crtc->active check in pipe_has_enabled_pch() will indicate that pipe
> B does not need FDI, so step 3 will succeed. Or am I missing something
> subtle here?
> 
> Also if we do things this way:
> 1. enable pipe C with eDP
> 2. enable pipe B with PCH using >2 FDI lanes
> 
> Now step 2 will fail even though pipe C doesn't need any FDI lanes.
> 
> So to fix that it would seem that we should remove the crtc->active
> check from pipe_has_enabled_pch() and use that to check for conlicts
> in both cases. Now that .global_resources() is no longer in the picture
> the crtc->active check not needed anyway, I think.

This is actually the motivation for this patch. I tried to fix that in 

    http://patchwork.freedesktop.org/patch/44281/ ,

but then it became clear that patch broke the case of pipe B going from
a >2 lanes mode to a <=2 mode. But the combination of the two patches
works for both cases.

I haven't thought about the eDP on pipe C case before, but it seems we
want the change you mentioned above too to fix that.

Ander


> 
> >  		else
> > -			cpt_enable_fdi_bc_bifurcation(dev);
> > +			cpt_set_fdi_bc_bifurcation(dev, true);
> >  
> >  		break;
> >  	case PIPE_C:
> > -		cpt_enable_fdi_bc_bifurcation(dev);
> > +		cpt_set_fdi_bc_bifurcation(dev, true);
> >  
> >  		break;
> >  	default:
> > @@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
> >  	} else if (IS_IVYBRIDGE(dev)) {
> >  		/* FIXME: detect B0+ stepping and use auto training */
> >  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> > -		dev_priv->display.modeset_global_resources =
> > -			ivb_modeset_global_resources;
> >  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> >  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> >  	} else if (IS_VALLEYVIEW(dev)) {
> > -- 
> > 2.1.0
> 

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5933
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -3              281/281              278/281
ILK                                  308/308              308/308
SNB                 -1              284/284              283/284
IVB                                  375/375              375/375
BYT                                  294/294              294/294
HSW                                  384/384              384/384
BDW                                  315/315              315/315
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt_gem_userptr_blits_coherency-sync      CRASH(3)PASS(2)      CRASH(1)PASS(1)
 PNV  igt_gen3_render_tiledy_blits      FAIL(3)PASS(1)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-threaded-none      PASS(4)      CRASH(1)PASS(1)
*SNB  igt_gem_exec_params_sol-reset-not-gen7      PASS(2)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[Daniel Vetter]

On Tue, Mar 10, 2015 at 02:32:34PM +0200, Ander Conselvan de Oliveira wrote:
> Remove the global modeset resource function that would disable the
> bifurcation bit, and instead enable/disable it when enabling or
> disabling the pch transcoder. The checks before the mode set should
> ensure that the configuration is valid, so it should be safe to do it
> so.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
> 
> On Mon, 2015-03-09 at 17:21 +0100, Daniel Vetter wrote:
> > With atomic we need to probably look at crtc_state->mode_changed. As an
> > interim solution we have the same information available in the
> > modeset_pipes bitmask. I think replacing the check for intel_crtc->active
> > with !(modeset_pipes & (1<<intel_crtc->pipe)) should work.
> 
> I looked into that solution, but involves passing modeset_pipes way down
> into the call stack, so I started looking for a different solution. Do you
> see any caveat with this approach?

Moving things to the disable hook would be great since that's yet another
special case remove. It wasnt' possible back when I've done this, and I
think the reason was that we still had a ->mode_set callback before the
crtc_enable hook. But that's all gone now, so as long as the bifurcate
update is done early enough I think this'll work.

Maybe discuss this with Ville locally - he provided all the feedback for
my original patch too? Random bikesheds below, I definitely need to think
about this more.

Cheers, Daniel

> 
> Thanks,
> Ander
> 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4008bf4..eca5a41 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -101,6 +101,8 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config);
>  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> +						bool pipe_active);

Imo just move the functions instead of forward declarations.

>  
>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>  {
> @@ -1956,6 +1958,8 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  					    enum pipe pipe)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> +	struct intel_crtc *crtc =
> +		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  	uint32_t reg, val;
>  
>  	/* FDI relies on the transcoder */
> @@ -1980,6 +1984,9 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  		val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE;
>  		I915_WRITE(reg, val);
>  	}
> +
> +	if (IS_IVYBRIDGE(dev))
> +		ivybridge_update_fdi_bc_bifurcation(crtc, false);
>  }
>  
>  static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> @@ -3153,32 +3160,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  	return crtc->base.state->enable && crtc->config->has_pch_encoder;
>  }
>  
> -static void ivb_modeset_global_resources(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *pipe_B_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> -	struct intel_crtc *pipe_C_crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> -	uint32_t temp;
> -
> -	/*
> -	 * When everything is off disable fdi C so that we could enable fdi B
> -	 * with all lanes. Note that we don't care about enabled pipes without
> -	 * an enabled pch encoder.
> -	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
> -		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
> -
> -		temp = I915_READ(SOUTH_CHICKEN1);
> -		temp &= ~FDI_BC_BIFURCATION_SELECT;
> -		DRM_DEBUG_KMS("disabling fdi C rx\n");
> -		I915_WRITE(SOUTH_CHICKEN1, temp);
> -	}
> -}
> -
>  /* The FDI link training functions for ILK/Ibexpeak. */
>  static void ironlake_fdi_link_train(struct drm_crtc *crtc)
>  {
> @@ -3834,41 +3815,44 @@ static void ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
>  		   I915_READ(VSYNCSHIFT(cpu_transcoder)));
>  }
>  
> -static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
> +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev, bool enable)

enable feels misnamed now. Maybe s/enable/set/ instead?

>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t temp;
>  
>  	temp = I915_READ(SOUTH_CHICKEN1);
> -	if (temp & FDI_BC_BIFURCATION_SELECT)
> +	if (!!(temp & FDI_BC_BIFURCATION_SELECT) == enable)
>  		return;
>  
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
>  	WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
>  
> -	temp |= FDI_BC_BIFURCATION_SELECT;
> -	DRM_DEBUG_KMS("enabling fdi C rx\n");
> +	temp &= ~FDI_BC_BIFURCATION_SELECT;
> +	if (enable)
> +		temp |= FDI_BC_BIFURCATION_SELECT;
> +
> +	DRM_DEBUG_KMS("%sabling fdi C rx\n", enable ? "en" : "dis");
>  	I915_WRITE(SOUTH_CHICKEN1, temp);
>  	POSTING_READ(SOUTH_CHICKEN1);
>  }
>  
> -static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
> +static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc,
> +						bool pipe_active)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	switch (intel_crtc->pipe) {
>  	case PIPE_A:
>  		break;
>  	case PIPE_B:
> -		if (intel_crtc->config->fdi_lanes > 2)
> -			WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT);
> +		if (intel_crtc->config->fdi_lanes > 2 && pipe_active)
> +			cpt_enable_fdi_bc_bifurcation(dev, false);
>  		else
> -			cpt_enable_fdi_bc_bifurcation(dev);
> +			cpt_enable_fdi_bc_bifurcation(dev, true);
>  
>  		break;
>  	case PIPE_C:
> -		cpt_enable_fdi_bc_bifurcation(dev);
> +		cpt_enable_fdi_bc_bifurcation(dev, pipe_active);
>  
>  		break;
>  	default:
> @@ -3895,7 +3879,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  	assert_pch_transcoder_disabled(dev_priv, pipe);
>  
>  	if (IS_IVYBRIDGE(dev))
> -		ivybridge_update_fdi_bc_bifurcation(intel_crtc);
> +		ivybridge_update_fdi_bc_bifurcation(intel_crtc, true);
>  
>  	/* Write the TU size bits before fdi link training, so that error
>  	 * detection works. */
> @@ -13056,8 +13040,6 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_IVYBRIDGE(dev)) {
>  		/* FIXME: detect B0+ stepping and use auto training */
>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> -		dev_priv->display.modeset_global_resources =
> -			ivb_modeset_global_resources;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>  	} else if (IS_VALLEYVIEW(dev)) {
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5925
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              282/282              280/282
ILK                                  308/308              308/308
SNB                                  307/307              307/307
IVB                                  375/375              375/375
BYT                                  294/294              294/294
HSW                                  385/385              385/385
BDW                                  315/315              315/315
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt_gem_userptr_blits_minor-normal-sync      DMESG_WARN(2)PASS(3)      DMESG_WARN(2)
*PNV  igt_gem_userptr_blits_minor-unsync-normal      PASS(4)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Ignore pipe B active state when enabling pipe C

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5915
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              282/282              281/282
ILK                                  308/308              308/308
SNB                                  307/307              307/307
IVB                 -2              375/375              373/375
BYT                                  294/294              294/294
HSW                                  385/385              385/385
BDW                                  315/315              315/315
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_userptr_blits_minor-unsync-normal      PASS(2)      DMESG_WARN(1)PASS(1)
*IVB  igt_drv_debugfs_reader      PASS(2)      DMESG_WARN(2)
*IVB  igt_drv_hangman_error-state-sysfs-entry      PASS(2)      TIMEOUT(2)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx