[PATCH 1/6] drm: Store the calculated vrefresh in the user mode

[PATCH 1/6] drm: Store the calculated vrefresh in the user mode

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Ignore the vrefresh in the mode the user passed in and instead
calculate the value based on the actual timings. This way we can
actually trust mode->vrefresh to some degree.

Or should we compare the user's idea of vrefresh with the one
we get from the timings and return an error if they differ? We
can't really be sure what the user is asking in that case.

v2: Set it before mode validation

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 7f552d5fa88e..11d8224535ca 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1702,7 +1702,11 @@ int drm_mode_convert_umode(struct drm_device *dev,
 	out->vsync_end = in->vsync_end;
 	out->vtotal = in->vtotal;
 	out->vscan = in->vscan;
-	out->vrefresh = in->vrefresh;
+	 /*
+	  * Ignore what the user is saying here and instead
+	  * calculate vrefresh based on the actual timings.
+	  */
+	out->vrefresh = 0;
 	out->flags = in->flags;
 	/*
 	 * Old xf86-video-vmware (possibly others too) used to
@@ -1738,6 +1742,8 @@ int drm_mode_convert_umode(struct drm_device *dev,
 		break;
 	}
 
+	out->vrefresh = drm_mode_vrefresh(out);
+
 	out->status = drm_mode_validate_driver(dev, out);
 	if (out->status != MODE_OK)
 		return -EINVAL;
-- 
2.16.4

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

[PATCH 2/6] drm: Set mode->vrefresh before mode validation

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Update mode->vrefresh before mode validation. This allows the
validation code to consult mode->vrefresh safely.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_probe_helper.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 527743394150..34409abfd061 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -490,6 +490,16 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 
 	drm_mode_connector_list_update(connector);
 
+	list_for_each_entry(mode, &connector->modes, head) {
+		/*
+		 * Clear out any potentially stale cached vrefresh
+		 * value, as otherwise drm_mode_vrefresh() would
+		 * just return it back to us.
+		 */
+		mode->vrefresh = 0;
+		mode->vrefresh = drm_mode_vrefresh(mode);
+	}
+
 	if (connector->interlace_allowed)
 		mode_flags |= DRM_MODE_FLAG_INTERLACE;
 	if (connector->doublescan_allowed)
@@ -525,9 +535,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	if (list_empty(&connector->modes))
 		return 0;
 
-	list_for_each_entry(mode, &connector->modes, head)
-		mode->vrefresh = drm_mode_vrefresh(mode);
-
 	drm_mode_sort(&connector->modes);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] probed modes :\n", connector->base.id,
-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] drm/i915/sdvo: Fix multi function encoder stuff

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

SDVO encoders can have multiple different types of outputs hanging off
them. Currently the code tries to muck around with various is_foo
flags in the encoder to figure out which type its driving. That doesn't
work with atomic and other stuff, so let's nuke those flags and just
look at which type of connector we're actually dealing with.

The is_hdmi we'll need as that's not discoverable via the output flags,
but we'll just move it under the connector.

We'll also move the sdvo fixed mode handling out from the .get_modes()
hook into the sdvo lvds init function so that we can bail out properly
if there is no fixed mode to be found.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 101 +++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e6a64b3ecd91..fc4cec60a60c 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -99,31 +99,12 @@ struct intel_sdvo {
 	 */
 	uint16_t hotplug_active;
 
-	/**
-	 * This is set if we're going to treat the device as TV-out.
-	 *
-	 * While we have these nice friendly flags for output types that ought
-	 * to decide this for us, the S-Video output on our HDMI+S-Video card
-	 * shows up as RGB1 (VGA).
-	 */
-	bool is_tv;
-
 	enum port port;
 
-	/**
-	 * This is set if we treat the device as HDMI, instead of DVI.
-	 */
-	bool is_hdmi;
 	bool has_hdmi_monitor;
 	bool has_hdmi_audio;
 	bool rgb_quant_range_selectable;
 
-	/**
-	 * This is set if we detect output of sdvo device as LVDS and
-	 * have a valid fixed mode to use with the panel.
-	 */
-	bool is_lvds;
-
 	/**
 	 * This is sdvo fixed pannel mode pointer
 	 */
@@ -172,6 +153,11 @@ struct intel_sdvo_connector {
 
 	/* this is to get the range of margin.*/
 	u32 max_hscan, max_vscan;
+
+	/**
+	 * This is set if we treat the device as HDMI, instead of DVI.
+	 */
+	bool is_hdmi;
 };
 
 struct intel_sdvo_connector_state {
@@ -766,6 +752,7 @@ static bool intel_sdvo_get_input_timing(struct intel_sdvo *intel_sdvo,
 
 static bool
 intel_sdvo_create_preferred_input_timing(struct intel_sdvo *intel_sdvo,
+					 struct intel_sdvo_connector *intel_sdvo_connector,
 					 uint16_t clock,
 					 uint16_t width,
 					 uint16_t height)
@@ -778,7 +765,7 @@ intel_sdvo_create_preferred_input_timing(struct intel_sdvo *intel_sdvo,
 	args.height = height;
 	args.interlace = 0;
 
-	if (intel_sdvo->is_lvds &&
+	if (IS_LVDS(intel_sdvo_connector) &&
 	   (intel_sdvo->sdvo_lvds_fixed_mode->hdisplay != width ||
 	    intel_sdvo->sdvo_lvds_fixed_mode->vdisplay != height))
 		args.scaled = 1;
@@ -1067,6 +1054,7 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
  */
 static bool
 intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
+				    struct intel_sdvo_connector *intel_sdvo_connector,
 				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adjusted_mode)
 {
@@ -1077,6 +1065,7 @@ intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
 		return false;
 
 	if (!intel_sdvo_create_preferred_input_timing(intel_sdvo,
+						      intel_sdvo_connector,
 						      mode->clock / 10,
 						      mode->hdisplay,
 						      mode->vdisplay))
@@ -1127,6 +1116,8 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
 	struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
 	struct intel_sdvo_connector_state *intel_sdvo_state =
 		to_intel_sdvo_connector_state(conn_state);
+	struct intel_sdvo_connector *intel_sdvo_connector =
+		to_intel_sdvo_connector(conn_state->connector);
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct drm_display_mode *mode = &pipe_config->base.mode;
 
@@ -1142,20 +1133,22 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
 	 * timings, even though this isn't really the right place in
 	 * the sequence to do it. Oh well.
 	 */
-	if (intel_sdvo->is_tv) {
+	if (IS_TV(intel_sdvo_connector)) {
 		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
 			return false;
 
 		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
+							   intel_sdvo_connector,
 							   mode,
 							   adjusted_mode);
 		pipe_config->sdvo_tv_clock = true;
-	} else if (intel_sdvo->is_lvds) {
+	} else if (IS_LVDS(intel_sdvo_connector)) {
 		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
 							     intel_sdvo->sdvo_lvds_fixed_mode))
 			return false;
 
 		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
+							   intel_sdvo_connector,
 							   mode,
 							   adjusted_mode);
 	}
@@ -1194,11 +1187,11 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
 	}
 
 	/* Clock computation needs to happen after pixel multiplier. */
-	if (intel_sdvo->is_tv)
+	if (IS_TV(intel_sdvo_connector))
 		i9xx_adjust_sdvo_tv_clock(pipe_config);
 
 	/* Set user selected PAR to incoming mode's member */
-	if (intel_sdvo->is_hdmi)
+	if (intel_sdvo_connector->is_hdmi)
 		adjusted_mode->picture_aspect_ratio = conn_state->picture_aspect_ratio;
 
 	return true;
@@ -1275,6 +1268,8 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
 	const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;
 	const struct intel_sdvo_connector_state *sdvo_state =
 		to_intel_sdvo_connector_state(conn_state);
+	const struct intel_sdvo_connector *intel_sdvo_connector =
+		to_intel_sdvo_connector(conn_state->connector);
 	const struct drm_display_mode *mode = &crtc_state->base.mode;
 	struct intel_sdvo *intel_sdvo = to_sdvo(intel_encoder);
 	u32 sdvox;
@@ -1304,7 +1299,7 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
 		return;
 
 	/* lvds has a special fixed output timing. */
-	if (intel_sdvo->is_lvds)
+	if (IS_LVDS(intel_sdvo_connector))
 		intel_sdvo_get_dtd_from_mode(&output_dtd,
 					     intel_sdvo->sdvo_lvds_fixed_mode);
 	else
@@ -1325,13 +1320,13 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
 	} else
 		intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_DVI);
 
-	if (intel_sdvo->is_tv &&
+	if (IS_TV(intel_sdvo_connector) &&
 	    !intel_sdvo_set_tv_format(intel_sdvo, conn_state))
 		return;
 
 	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
 
-	if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
+	if (IS_TV(intel_sdvo_connector) || IS_LVDS(intel_sdvo_connector))
 		input_dtd.part2.sdvo_flags = intel_sdvo->dtd_sdvo_flags;
 	if (!intel_sdvo_set_input_timing(intel_sdvo, &input_dtd))
 		DRM_INFO("Setting input timings on %s failed\n",
@@ -1632,6 +1627,8 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
 		      struct drm_display_mode *mode)
 {
 	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
+	struct intel_sdvo_connector *intel_sdvo_connector =
+		to_intel_sdvo_connector(connector);
 	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
 
 	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
@@ -1646,7 +1643,7 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
 	if (mode->clock > max_dotclk)
 		return MODE_CLOCK_HIGH;
 
-	if (intel_sdvo->is_lvds) {
+	if (IS_LVDS(intel_sdvo_connector)) {
 		if (mode->hdisplay > intel_sdvo->sdvo_lvds_fixed_mode->hdisplay)
 			return MODE_PANEL;
 
@@ -1761,6 +1758,8 @@ static enum drm_connector_status
 intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
 {
 	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
+	struct intel_sdvo_connector *intel_sdvo_connector =
+		to_intel_sdvo_connector(connector);
 	enum drm_connector_status status;
 	struct edid *edid;
 
@@ -1799,7 +1798,7 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
 		/* DDC bus is shared, match EDID to connector type */
 		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
 			status = connector_status_connected;
-			if (intel_sdvo->is_hdmi) {
+			if (intel_sdvo_connector->is_hdmi) {
 				intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid);
 				intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid);
 				intel_sdvo->rgb_quant_range_selectable =
@@ -1877,17 +1876,6 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
 			ret = connector_status_connected;
 	}
 
-	/* May update encoder flag for like clock for SDVO TV, etc.*/
-	if (ret == connector_status_connected) {
-		intel_sdvo->is_tv = false;
-		intel_sdvo->is_lvds = false;
-
-		if (response & SDVO_TV_MASK)
-			intel_sdvo->is_tv = true;
-		if (response & SDVO_LVDS_MASK)
-			intel_sdvo->is_lvds = intel_sdvo->sdvo_lvds_fixed_mode != NULL;
-	}
-
 	return ret;
 }
 
@@ -2056,16 +2044,6 @@ static void intel_sdvo_get_lvds_modes(struct drm_connector *connector)
 	 * arranged in priority order.
 	 */
 	intel_ddc_get_modes(connector, &intel_sdvo->ddc);
-
-	list_for_each_entry(newmode, &connector->probed_modes, head) {
-		if (newmode->type & DRM_MODE_TYPE_PREFERRED) {
-			intel_sdvo->sdvo_lvds_fixed_mode =
-				drm_mode_duplicate(connector->dev, newmode);
-
-			intel_sdvo->is_lvds = true;
-			break;
-		}
-	}
 }
 
 static int intel_sdvo_get_modes(struct drm_connector *connector)
@@ -2552,7 +2530,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 	if (INTEL_GEN(dev_priv) >= 4 &&
 	    intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
 		connector->connector_type = DRM_MODE_CONNECTOR_HDMIA;
-		intel_sdvo->is_hdmi = true;
+		intel_sdvo_connector->is_hdmi = true;
 	}
 
 	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
@@ -2560,7 +2538,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 		return false;
 	}
 
-	if (intel_sdvo->is_hdmi)
+	if (intel_sdvo_connector->is_hdmi)
 		intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
 
 	return true;
@@ -2588,8 +2566,6 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 	intel_sdvo->controlled_output |= type;
 	intel_sdvo_connector->output_flag = type;
 
-	intel_sdvo->is_tv = true;
-
 	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
 		kfree(intel_sdvo_connector);
 		return false;
@@ -2651,6 +2627,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 	struct drm_connector *connector;
 	struct intel_connector *intel_connector;
 	struct intel_sdvo_connector *intel_sdvo_connector;
+	struct drm_display_mode *mode;
 
 	DRM_DEBUG_KMS("initialising LVDS device %d\n", device);
 
@@ -2679,6 +2656,19 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 	if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
 		goto err;
 
+	intel_sdvo_get_lvds_modes(connector);
+
+	list_for_each_entry(mode, &connector->probed_modes, head) {
+		if (mode->type & DRM_MODE_TYPE_PREFERRED) {
+			intel_sdvo->sdvo_lvds_fixed_mode =
+				drm_mode_duplicate(connector->dev, mode);
+			break;
+		}
+	}
+
+	if (!intel_sdvo->sdvo_lvds_fixed_mode)
+		goto err;
+
 	return true;
 
 err:
@@ -2689,9 +2679,6 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 static bool
 intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags)
 {
-	intel_sdvo->is_tv = false;
-	intel_sdvo->is_lvds = false;
-
 	/* SDVO requires XXX1 function may not exist unless it has XXX0 function.*/
 
 	if (flags & SDVO_OUTPUT_TMDS0)
-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/i915/sdvo: Utilize intel_panel for fixed_mode

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Remove the local lvds fixed mode pointer from the sdvo encoder
structure and instead utilize intel_panel like everyone else.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 40 ++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index fc4cec60a60c..cd5d5e800486 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -105,11 +105,6 @@ struct intel_sdvo {
 	bool has_hdmi_audio;
 	bool rgb_quant_range_selectable;
 
-	/**
-	 * This is sdvo fixed pannel mode pointer
-	 */
-	struct drm_display_mode *sdvo_lvds_fixed_mode;
-
 	/* DDC bus used by this SDVO encoder */
 	uint8_t ddc_bus;
 
@@ -765,10 +760,14 @@ intel_sdvo_create_preferred_input_timing(struct intel_sdvo *intel_sdvo,
 	args.height = height;
 	args.interlace = 0;
 
-	if (IS_LVDS(intel_sdvo_connector) &&
-	   (intel_sdvo->sdvo_lvds_fixed_mode->hdisplay != width ||
-	    intel_sdvo->sdvo_lvds_fixed_mode->vdisplay != height))
-		args.scaled = 1;
+	if (IS_LVDS(intel_sdvo_connector)) {
+		const struct drm_display_mode *fixed_mode =
+			intel_sdvo_connector->base.panel.fixed_mode;
+
+		if (fixed_mode->hdisplay != width ||
+		    fixed_mode->vdisplay != height)
+			args.scaled = 1;
+	}
 
 	return intel_sdvo_set_value(intel_sdvo,
 				    SDVO_CMD_CREATE_PREFERRED_INPUT_TIMING,
@@ -1144,7 +1143,7 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
 		pipe_config->sdvo_tv_clock = true;
 	} else if (IS_LVDS(intel_sdvo_connector)) {
 		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
-							     intel_sdvo->sdvo_lvds_fixed_mode))
+							     intel_sdvo_connector->base.panel.fixed_mode))
 			return false;
 
 		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
@@ -1301,7 +1300,7 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
 	/* lvds has a special fixed output timing. */
 	if (IS_LVDS(intel_sdvo_connector))
 		intel_sdvo_get_dtd_from_mode(&output_dtd,
-					     intel_sdvo->sdvo_lvds_fixed_mode);
+					     intel_sdvo_connector->base.panel.fixed_mode);
 	else
 		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
 	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
@@ -1644,10 +1643,13 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
 		return MODE_CLOCK_HIGH;
 
 	if (IS_LVDS(intel_sdvo_connector)) {
-		if (mode->hdisplay > intel_sdvo->sdvo_lvds_fixed_mode->hdisplay)
+		const struct drm_display_mode *fixed_mode =
+			intel_sdvo_connector->base.panel.fixed_mode;
+
+		if (mode->hdisplay > fixed_mode->hdisplay)
 			return MODE_PANEL;
 
-		if (mode->vdisplay > intel_sdvo->sdvo_lvds_fixed_mode->vdisplay)
+		if (mode->vdisplay > fixed_mode->vdisplay)
 			return MODE_PANEL;
 	}
 
@@ -2065,6 +2067,7 @@ static void intel_sdvo_destroy(struct drm_connector *connector)
 	struct intel_sdvo_connector *intel_sdvo_connector = to_intel_sdvo_connector(connector);
 
 	drm_connector_cleanup(connector);
+	intel_panel_fini(&intel_sdvo_connector->base.panel);
 	kfree(intel_sdvo_connector);
 }
 
@@ -2269,10 +2272,6 @@ static void intel_sdvo_enc_destroy(struct drm_encoder *encoder)
 {
 	struct intel_sdvo *intel_sdvo = to_sdvo(to_intel_encoder(encoder));
 
-	if (intel_sdvo->sdvo_lvds_fixed_mode != NULL)
-		drm_mode_destroy(encoder->dev,
-				 intel_sdvo->sdvo_lvds_fixed_mode);
-
 	i2c_del_adapter(&intel_sdvo->ddc);
 	intel_encoder_destroy(encoder);
 }
@@ -2660,13 +2659,16 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 
 	list_for_each_entry(mode, &connector->probed_modes, head) {
 		if (mode->type & DRM_MODE_TYPE_PREFERRED) {
-			intel_sdvo->sdvo_lvds_fixed_mode =
+			struct drm_display_mode *fixed_mode =
 				drm_mode_duplicate(connector->dev, mode);
+
+			intel_panel_init(&intel_connector->panel,
+					 fixed_mode, NULL);
 			break;
 		}
 	}
 
-	if (!intel_sdvo->sdvo_lvds_fixed_mode)
+	if (!intel_connector->panel.fixed_mode)
 		goto err;
 
 	return true;
-- 
2.16.4

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

[PATCH 5/6] drm/i915: Make sure panel fixed_mode has vrefresh populated

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We want to use fixed_mode->vrefresh during mode validation, so first
make sure it's set correctly.

And we'll do the same for the downclock mode for consistency.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 14b827ec5427..c784e772b6aa 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1932,6 +1932,15 @@ int intel_panel_init(struct intel_panel *panel,
 {
 	intel_panel_init_backlight_funcs(panel);
 
+	if (fixed_mode) {
+		fixed_mode->vrefresh = 0;
+		fixed_mode->vrefresh = drm_mode_vrefresh(fixed_mode);
+	}
+	if (downclock_mode) {
+		downclock_mode->vrefresh = 0;
+		downclock_mode->vrefresh = drm_mode_vrefresh(downclock_mode);
+	}
+
 	panel->fixed_mode = fixed_mode;
 	panel->downclock_mode = downclock_mode;
 
-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] drm/i915: Filter out modes that don't match the fixed mode vrefresh

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We only ever drive the panel with the fixed mode, hence we don't
want to advertize any modes that have a different vertical refresh rate.

We tried to allow a second lower clocked mode to used for eDP but
that was reverted in commit d93fa1b47b8f ("Revert "drm/i915/edp:
Allow alternate fixed mode for eDP if available.""). To do that properly
I think we should probably just keep all (or just some?) of the probed
modes around (presumably all of them actually work if the panel
advertizes them), and we'd just pick the closest match based on the
user mode.

For now we don't have any of that so we shouldn't lie to the user
that they can drive the panel at different refresh rate. Note that
we still don't reject any user mode with bad refresh rate. That's
going to be step two.

TODO: Should we allow for a larger error margin?

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   | 3 +++
 drivers/gpu/drm/i915/intel_dsi.c  | 2 ++
 drivers/gpu/drm/i915/intel_dvo.c  | 2 ++
 drivers/gpu/drm/i915/intel_lvds.c | 2 ++
 drivers/gpu/drm/i915/intel_sdvo.c | 2 ++
 5 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5be07e1d816d..22feb0b144f6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -446,6 +446,9 @@ intel_dp_mode_valid(struct drm_connector *connector,
 		if (mode->vdisplay > fixed_mode->vdisplay)
 			return MODE_PANEL;
 
+		if (mode->vrefresh != fixed_mode->vrefresh)
+			return MODE_PANEL;
+
 		target_clock = fixed_mode->clock;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 3b7acb5a70b3..3d0758cec85a 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1277,6 +1277,8 @@ intel_dsi_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 		if (mode->vdisplay > fixed_mode->vdisplay)
 			return MODE_PANEL;
+		if (mode->vrefresh != fixed_mode->vrefresh)
+			return MODE_PANEL;
 		if (fixed_mode->clock > max_dotclk)
 			return MODE_CLOCK_HIGH;
 	}
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 4e142ff49708..078bb848a49a 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -225,6 +225,8 @@ intel_dvo_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 		if (mode->vdisplay > fixed_mode->vdisplay)
 			return MODE_PANEL;
+		if (mode->vrefresh != fixed_mode->vrefresh)
+			return MODE_PANEL;
 
 		target_clock = fixed_mode->clock;
 	}
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index bb06744d28a4..d312a8911b89 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -384,6 +384,8 @@ intel_lvds_mode_valid(struct drm_connector *connector,
 		return MODE_PANEL;
 	if (mode->vdisplay > fixed_mode->vdisplay)
 		return MODE_PANEL;
+	if (mode->vrefresh != fixed_mode->vrefresh)
+		return MODE_PANEL;
 	if (fixed_mode->clock > max_pixclk)
 		return MODE_CLOCK_HIGH;
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index cd5d5e800486..4be4bae51652 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1650,6 +1650,8 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 
 		if (mode->vdisplay > fixed_mode->vdisplay)
+
+		if (mode->vrefresh != fixed_mode->vrefresh)
 			return MODE_PANEL;
 	}
 
-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/6] drm: Store the calculated vrefresh in the user mode

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm: Store the calculated vrefresh in the user mode
URL   : https://patchwork.freedesktop.org/series/45610/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
2a77f40fd19a drm: Store the calculated vrefresh in the user mode
ef213bd1f5e6 drm: Set mode->vrefresh before mode validation
5c5a934387d6 drm/i915/sdvo: Fix multi function encoder stuff
-:86: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#86: FILE: drivers/gpu/drm/i915/intel_sdvo.c:769:
+	if (IS_LVDS(intel_sdvo_connector) &&
 	   (intel_sdvo->sdvo_lvds_fixed_mode->hdisplay != width ||

total: 0 errors, 0 warnings, 1 checks, 271 lines checked
e191e1a7ccd5 drm/i915/sdvo: Utilize intel_panel for fixed_mode
-:54: WARNING:LONG_LINE: line over 100 characters
#54: FILE: drivers/gpu/drm/i915/intel_sdvo.c:1146:
+							     intel_sdvo_connector->base.panel.fixed_mode))

total: 0 errors, 1 warnings, 0 checks, 95 lines checked
3b94f6dc43fa drm/i915: Make sure panel fixed_mode has vrefresh populated
3fa2a42678d1 drm/i915: Filter out modes that don't match the fixed mode vrefresh
-:14: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit d93fa1b47b8f ("Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available."")'
#14: 
that was reverted in commit d93fa1b47b8f ("Revert "drm/i915/edp:

-:90: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (16, 16)
#90: FILE: drivers/gpu/drm/i915/intel_sdvo.c:1652:
 		if (mode->vdisplay > fixed_mode->vdisplay)
[...]
+		if (mode->vrefresh != fixed_mode->vrefresh)

total: 1 errors, 1 warnings, 0 checks, 41 lines checked

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

✓ Fi.CI.BAT: success for series starting with [1/6] drm: Store the calculated vrefresh in the user mode

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm: Store the calculated vrefresh in the user mode
URL   : https://patchwork.freedesktop.org/series/45610/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4398 -> Patchwork_9476 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45610/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9476 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387) +2

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#106103, fdo#102614)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387


== Participating hosts (44 -> 39) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4398 -> Patchwork_9476

  CI_DRM_4398: 66df3b4e8f63d2918d202cd0802ce389cfe36b0e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9476: 3fa2a42678d166119450fcce74b8b944f74cc4a7 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3fa2a42678d1 drm/i915: Filter out modes that don't match the fixed mode vrefresh
3b94f6dc43fa drm/i915: Make sure panel fixed_mode has vrefresh populated
e191e1a7ccd5 drm/i915/sdvo: Utilize intel_panel for fixed_mode
5c5a934387d6 drm/i915/sdvo: Fix multi function encoder stuff
ef213bd1f5e6 drm: Set mode->vrefresh before mode validation
2a77f40fd19a drm: Store the calculated vrefresh in the user mode

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9476/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/6] drm: Store the calculated vrefresh in the user mode

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm: Store the calculated vrefresh in the user mode
URL   : https://patchwork.freedesktop.org/series/45610/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4398_full -> Patchwork_9476_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9476_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9476_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9476_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-blt:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9476_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_gtt:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@kms_flip@plain-flip-fb-recreate:
      shard-glk:          PASS -> FAIL (fdo#100368) +1

    
    ==== Possible fixes ====

    igt@gem_ctx_isolation@rcs0-s3:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          FAIL (fdo#106509) -> PASS

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          FAIL (fdo#106509, fdo#105454) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#105363, fdo#102887) -> PASS

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-msflip-blt:
      shard-glk:          FAIL (fdo#104724, fdo#103167) -> PASS

    igt@kms_rotation_crc@primary-rotation-90:
      shard-kbl:          FAIL (fdo#104724, fdo#103925) -> PASS

    
    ==== Warnings ====

    igt@drv_selftest@live_gtt:
      shard-glk:          INCOMPLETE (fdo#103359, k.org#198133) -> FAIL (fdo#105347)

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4398 -> Patchwork_9476

  CI_DRM_4398: 66df3b4e8f63d2918d202cd0802ce389cfe36b0e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9476: 3fa2a42678d166119450fcce74b8b944f74cc4a7 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9476/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Filter out modes that don't match the fixed mode vrefresh

[Daniel Vetter]

On Thu, Jun 28, 2018 at 10:43:01PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We only ever drive the panel with the fixed mode, hence we don't
> want to advertize any modes that have a different vertical refresh rate.
> 
> We tried to allow a second lower clocked mode to used for eDP but
> that was reverted in commit d93fa1b47b8f ("Revert "drm/i915/edp:
> Allow alternate fixed mode for eDP if available.""). To do that properly
> I think we should probably just keep all (or just some?) of the probed
> modes around (presumably all of them actually work if the panel
> advertizes them), and we'd just pick the closest match based on the
> user mode.
> 
> For now we don't have any of that so we shouldn't lie to the user
> that they can drive the panel at different refresh rate. Note that
> we still don't reject any user mode with bad refresh rate. That's
> going to be step two.
> 
> TODO: Should we allow for a larger error margin?
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Not sure the current vrefresh is any good, since it's HZ resolution only.
That's not precise enough for the video aficionados, who insist on that
entire 1001/1000 business.

I did look around a bit with a quick git grep, and most places only use
->vrefresh for debug printing. Then there's a few that use it as an index
(e.g. i915 DRRS code), and then there's the totally misguided code in
adv7511 which thinks vrefresh is in kHz.

There's also quite a pile of mode->vrefresh ? mode->vrefresh :
drm_mode_vrefresh(mode) around, which really doesn't inspire confidence
that we'll ever catch them all. It feels like those "recompute vrefresh"
things have been cargo-culted for ages.

I think we need to clean this up harder:
- switch everyone over to drm_mode_vrefresh()
- nuke drm_display_mode->vrefresh
- add a new drm_mode_vrefresh_mHz (for milli-Hz, i.e. fixed point math
  ftw)
- use that new function in the mode filtering, with a bit of fudge that
  keeps the 1001/1000 modes separate, but papers over rounding errors. I
  think we could even do a drm_mode_compare_vrefresh for that, we're
  probably not the only ones who'd like to have such a function.

Probably a bit more work to type (but cocci might help here), definitely
less painful to make sure it stays correct I think. Thoughts?

Cheers, Daniel


> ---
>  drivers/gpu/drm/i915/intel_dp.c   | 3 +++
>  drivers/gpu/drm/i915/intel_dsi.c  | 2 ++
>  drivers/gpu/drm/i915/intel_dvo.c  | 2 ++
>  drivers/gpu/drm/i915/intel_lvds.c | 2 ++
>  drivers/gpu/drm/i915/intel_sdvo.c | 2 ++
>  5 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5be07e1d816d..22feb0b144f6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -446,6 +446,9 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  		if (mode->vdisplay > fixed_mode->vdisplay)
>  			return MODE_PANEL;
>  
> +		if (mode->vrefresh != fixed_mode->vrefresh)
> +			return MODE_PANEL;
> +
>  		target_clock = fixed_mode->clock;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 3b7acb5a70b3..3d0758cec85a 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -1277,6 +1277,8 @@ intel_dsi_mode_valid(struct drm_connector *connector,
>  			return MODE_PANEL;
>  		if (mode->vdisplay > fixed_mode->vdisplay)
>  			return MODE_PANEL;
> +		if (mode->vrefresh != fixed_mode->vrefresh)
> +			return MODE_PANEL;
>  		if (fixed_mode->clock > max_dotclk)
>  			return MODE_CLOCK_HIGH;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 4e142ff49708..078bb848a49a 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -225,6 +225,8 @@ intel_dvo_mode_valid(struct drm_connector *connector,
>  			return MODE_PANEL;
>  		if (mode->vdisplay > fixed_mode->vdisplay)
>  			return MODE_PANEL;
> +		if (mode->vrefresh != fixed_mode->vrefresh)
> +			return MODE_PANEL;
>  
>  		target_clock = fixed_mode->clock;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index bb06744d28a4..d312a8911b89 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -384,6 +384,8 @@ intel_lvds_mode_valid(struct drm_connector *connector,
>  		return MODE_PANEL;
>  	if (mode->vdisplay > fixed_mode->vdisplay)
>  		return MODE_PANEL;
> +	if (mode->vrefresh != fixed_mode->vrefresh)
> +		return MODE_PANEL;
>  	if (fixed_mode->clock > max_pixclk)
>  		return MODE_CLOCK_HIGH;
>  
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index cd5d5e800486..4be4bae51652 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1650,6 +1650,8 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
>  			return MODE_PANEL;
>  
>  		if (mode->vdisplay > fixed_mode->vdisplay)
> +
> +		if (mode->vrefresh != fixed_mode->vrefresh)
>  			return MODE_PANEL;
>  	}
>  
> -- 
> 2.16.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Filter out modes that don't match the fixed mode vrefresh

[Ville Syrjälä]

On Mon, Jul 02, 2018 at 09:46:23AM +0200, Daniel Vetter wrote:
> On Thu, Jun 28, 2018 at 10:43:01PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We only ever drive the panel with the fixed mode, hence we don't
> > want to advertize any modes that have a different vertical refresh rate.
> > 
> > We tried to allow a second lower clocked mode to used for eDP but
> > that was reverted in commit d93fa1b47b8f ("Revert "drm/i915/edp:
> > Allow alternate fixed mode for eDP if available.""). To do that properly
> > I think we should probably just keep all (or just some?) of the probed
> > modes around (presumably all of them actually work if the panel
> > advertizes them), and we'd just pick the closest match based on the
> > user mode.
> > 
> > For now we don't have any of that so we shouldn't lie to the user
> > that they can drive the panel at different refresh rate. Note that
> > we still don't reject any user mode with bad refresh rate. That's
> > going to be step two.
> > 
> > TODO: Should we allow for a larger error margin?
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Not sure the current vrefresh is any good, since it's HZ resolution only.
> That's not precise enough for the video aficionados, who insist on that
> entire 1001/1000 business.
> 
> I did look around a bit with a quick git grep, and most places only use
> ->vrefresh for debug printing. Then there's a few that use it as an index
> (e.g. i915 DRRS code), and then there's the totally misguided code in
> adv7511 which thinks vrefresh is in kHz.
> 
> There's also quite a pile of mode->vrefresh ? mode->vrefresh :
> drm_mode_vrefresh(mode) around, which really doesn't inspire confidence
> that we'll ever catch them all. It feels like those "recompute vrefresh"
> things have been cargo-culted for ages.
> 
> I think we need to clean this up harder:
> - switch everyone over to drm_mode_vrefresh()
> - nuke drm_display_mode->vrefresh
> - add a new drm_mode_vrefresh_mHz (for milli-Hz, i.e. fixed point math
>   ftw)
> - use that new function in the mode filtering, with a bit of fudge that
>   keeps the 1001/1000 modes separate, but papers over rounding errors. I
>   think we could even do a drm_mode_compare_vrefresh for that, we're
>   probably not the only ones who'd like to have such a function.
> 
> Probably a bit more work to type (but cocci might help here), definitely
> less painful to make sure it stays correct I think. Thoughts?

Yeah, would probably make sense. But apart from the "make sure this
don't break" angle it doesn't matter for this case unless we want
to make the filtering even more strict than the 1 Hz resolution.

> 
> Cheers, Daniel
> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c   | 3 +++
> >  drivers/gpu/drm/i915/intel_dsi.c  | 2 ++
> >  drivers/gpu/drm/i915/intel_dvo.c  | 2 ++
> >  drivers/gpu/drm/i915/intel_lvds.c | 2 ++
> >  drivers/gpu/drm/i915/intel_sdvo.c | 2 ++
> >  5 files changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 5be07e1d816d..22feb0b144f6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -446,6 +446,9 @@ intel_dp_mode_valid(struct drm_connector *connector,
> >  		if (mode->vdisplay > fixed_mode->vdisplay)
> >  			return MODE_PANEL;
> >  
> > +		if (mode->vrefresh != fixed_mode->vrefresh)
> > +			return MODE_PANEL;
> > +
> >  		target_clock = fixed_mode->clock;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index 3b7acb5a70b3..3d0758cec85a 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -1277,6 +1277,8 @@ intel_dsi_mode_valid(struct drm_connector *connector,
> >  			return MODE_PANEL;
> >  		if (mode->vdisplay > fixed_mode->vdisplay)
> >  			return MODE_PANEL;
> > +		if (mode->vrefresh != fixed_mode->vrefresh)
> > +			return MODE_PANEL;
> >  		if (fixed_mode->clock > max_dotclk)
> >  			return MODE_CLOCK_HIGH;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > index 4e142ff49708..078bb848a49a 100644
> > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > @@ -225,6 +225,8 @@ intel_dvo_mode_valid(struct drm_connector *connector,
> >  			return MODE_PANEL;
> >  		if (mode->vdisplay > fixed_mode->vdisplay)
> >  			return MODE_PANEL;
> > +		if (mode->vrefresh != fixed_mode->vrefresh)
> > +			return MODE_PANEL;
> >  
> >  		target_clock = fixed_mode->clock;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index bb06744d28a4..d312a8911b89 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -384,6 +384,8 @@ intel_lvds_mode_valid(struct drm_connector *connector,
> >  		return MODE_PANEL;
> >  	if (mode->vdisplay > fixed_mode->vdisplay)
> >  		return MODE_PANEL;
> > +	if (mode->vrefresh != fixed_mode->vrefresh)
> > +		return MODE_PANEL;
> >  	if (fixed_mode->clock > max_pixclk)
> >  		return MODE_CLOCK_HIGH;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index cd5d5e800486..4be4bae51652 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1650,6 +1650,8 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
> >  			return MODE_PANEL;
> >  
> >  		if (mode->vdisplay > fixed_mode->vdisplay)
> > +
> > +		if (mode->vrefresh != fixed_mode->vrefresh)
> >  			return MODE_PANEL;
> >  	}
> >  
> > -- 
> > 2.16.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 6/6] drm/i915: Filter out modes that don't match the fixed mode vrefresh

[Daniel Vetter]

On Mon, Jul 02, 2018 at 12:52:31PM +0300, Ville Syrjälä wrote:
> On Mon, Jul 02, 2018 at 09:46:23AM +0200, Daniel Vetter wrote:
> > On Thu, Jun 28, 2018 at 10:43:01PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We only ever drive the panel with the fixed mode, hence we don't
> > > want to advertize any modes that have a different vertical refresh rate.
> > > 
> > > We tried to allow a second lower clocked mode to used for eDP but
> > > that was reverted in commit d93fa1b47b8f ("Revert "drm/i915/edp:
> > > Allow alternate fixed mode for eDP if available.""). To do that properly
> > > I think we should probably just keep all (or just some?) of the probed
> > > modes around (presumably all of them actually work if the panel
> > > advertizes them), and we'd just pick the closest match based on the
> > > user mode.
> > > 
> > > For now we don't have any of that so we shouldn't lie to the user
> > > that they can drive the panel at different refresh rate. Note that
> > > we still don't reject any user mode with bad refresh rate. That's
> > > going to be step two.
> > > 
> > > TODO: Should we allow for a larger error margin?
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Not sure the current vrefresh is any good, since it's HZ resolution only.
> > That's not precise enough for the video aficionados, who insist on that
> > entire 1001/1000 business.
> > 
> > I did look around a bit with a quick git grep, and most places only use
> > ->vrefresh for debug printing. Then there's a few that use it as an index
> > (e.g. i915 DRRS code), and then there's the totally misguided code in
> > adv7511 which thinks vrefresh is in kHz.
> > 
> > There's also quite a pile of mode->vrefresh ? mode->vrefresh :
> > drm_mode_vrefresh(mode) around, which really doesn't inspire confidence
> > that we'll ever catch them all. It feels like those "recompute vrefresh"
> > things have been cargo-culted for ages.
> > 
> > I think we need to clean this up harder:
> > - switch everyone over to drm_mode_vrefresh()
> > - nuke drm_display_mode->vrefresh
> > - add a new drm_mode_vrefresh_mHz (for milli-Hz, i.e. fixed point math
> >   ftw)
> > - use that new function in the mode filtering, with a bit of fudge that
> >   keeps the 1001/1000 modes separate, but papers over rounding errors. I
> >   think we could even do a drm_mode_compare_vrefresh for that, we're
> >   probably not the only ones who'd like to have such a function.
> > 
> > Probably a bit more work to type (but cocci might help here), definitely
> > less painful to make sure it stays correct I think. Thoughts?
> 
> Yeah, would probably make sense. But apart from the "make sure this
> don't break" angle it doesn't matter for this case unless we want
> to make the filtering even more strict than the 1 Hz resolution.

Well it's generally video folks who care about this, and those folks will
spot the 1 Hz mismatch. I just realized that you can implement the
filtering without all the refactoring:

- Add drm_mode_vrefresh_mHz.
- Use it to filter modes.

Since it's entirely new there's no need to clean up the entire confusion
around mode->vrefresh first at all :-)
-Daniel
> 
> > 
> > Cheers, Daniel
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c   | 3 +++
> > >  drivers/gpu/drm/i915/intel_dsi.c  | 2 ++
> > >  drivers/gpu/drm/i915/intel_dvo.c  | 2 ++
> > >  drivers/gpu/drm/i915/intel_lvds.c | 2 ++
> > >  drivers/gpu/drm/i915/intel_sdvo.c | 2 ++
> > >  5 files changed, 11 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 5be07e1d816d..22feb0b144f6 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -446,6 +446,9 @@ intel_dp_mode_valid(struct drm_connector *connector,
> > >  		if (mode->vdisplay > fixed_mode->vdisplay)
> > >  			return MODE_PANEL;
> > >  
> > > +		if (mode->vrefresh != fixed_mode->vrefresh)
> > > +			return MODE_PANEL;
> > > +
> > >  		target_clock = fixed_mode->clock;
> > >  	}
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > > index 3b7acb5a70b3..3d0758cec85a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > @@ -1277,6 +1277,8 @@ intel_dsi_mode_valid(struct drm_connector *connector,
> > >  			return MODE_PANEL;
> > >  		if (mode->vdisplay > fixed_mode->vdisplay)
> > >  			return MODE_PANEL;
> > > +		if (mode->vrefresh != fixed_mode->vrefresh)
> > > +			return MODE_PANEL;
> > >  		if (fixed_mode->clock > max_dotclk)
> > >  			return MODE_CLOCK_HIGH;
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > > index 4e142ff49708..078bb848a49a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > > @@ -225,6 +225,8 @@ intel_dvo_mode_valid(struct drm_connector *connector,
> > >  			return MODE_PANEL;
> > >  		if (mode->vdisplay > fixed_mode->vdisplay)
> > >  			return MODE_PANEL;
> > > +		if (mode->vrefresh != fixed_mode->vrefresh)
> > > +			return MODE_PANEL;
> > >  
> > >  		target_clock = fixed_mode->clock;
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > > index bb06744d28a4..d312a8911b89 100644
> > > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > > @@ -384,6 +384,8 @@ intel_lvds_mode_valid(struct drm_connector *connector,
> > >  		return MODE_PANEL;
> > >  	if (mode->vdisplay > fixed_mode->vdisplay)
> > >  		return MODE_PANEL;
> > > +	if (mode->vrefresh != fixed_mode->vrefresh)
> > > +		return MODE_PANEL;
> > >  	if (fixed_mode->clock > max_pixclk)
> > >  		return MODE_CLOCK_HIGH;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > > index cd5d5e800486..4be4bae51652 100644
> > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > @@ -1650,6 +1650,8 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
> > >  			return MODE_PANEL;
> > >  
> > >  		if (mode->vdisplay > fixed_mode->vdisplay)
> > > +
> > > +		if (mode->vrefresh != fixed_mode->vrefresh)
> > >  			return MODE_PANEL;
> > >  	}
> > >  
> > > -- 
> > > 2.16.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Filter out modes that don't match the fixed mode vrefresh

[Chris Wilson]

Quoting Daniel Vetter (2018-07-03 08:03:09)
> On Mon, Jul 02, 2018 at 12:52:31PM +0300, Ville Syrjälä wrote:
> > On Mon, Jul 02, 2018 at 09:46:23AM +0200, Daniel Vetter wrote:
> > > On Thu, Jun 28, 2018 at 10:43:01PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > We only ever drive the panel with the fixed mode, hence we don't
> > > > want to advertize any modes that have a different vertical refresh rate.
> > > > 
> > > > We tried to allow a second lower clocked mode to used for eDP but
> > > > that was reverted in commit d93fa1b47b8f ("Revert "drm/i915/edp:
> > > > Allow alternate fixed mode for eDP if available.""). To do that properly
> > > > I think we should probably just keep all (or just some?) of the probed
> > > > modes around (presumably all of them actually work if the panel
> > > > advertizes them), and we'd just pick the closest match based on the
> > > > user mode.
> > > > 
> > > > For now we don't have any of that so we shouldn't lie to the user
> > > > that they can drive the panel at different refresh rate. Note that
> > > > we still don't reject any user mode with bad refresh rate. That's
> > > > going to be step two.
> > > > 
> > > > TODO: Should we allow for a larger error margin?
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Not sure the current vrefresh is any good, since it's HZ resolution only.
> > > That's not precise enough for the video aficionados, who insist on that
> > > entire 1001/1000 business.
> > > 
> > > I did look around a bit with a quick git grep, and most places only use
> > > ->vrefresh for debug printing. Then there's a few that use it as an index
> > > (e.g. i915 DRRS code), and then there's the totally misguided code in
> > > adv7511 which thinks vrefresh is in kHz.
> > > 
> > > There's also quite a pile of mode->vrefresh ? mode->vrefresh :
> > > drm_mode_vrefresh(mode) around, which really doesn't inspire confidence
> > > that we'll ever catch them all. It feels like those "recompute vrefresh"
> > > things have been cargo-culted for ages.
> > > 
> > > I think we need to clean this up harder:
> > > - switch everyone over to drm_mode_vrefresh()
> > > - nuke drm_display_mode->vrefresh
> > > - add a new drm_mode_vrefresh_mHz (for milli-Hz, i.e. fixed point math
> > >   ftw)
> > > - use that new function in the mode filtering, with a bit of fudge that
> > >   keeps the 1001/1000 modes separate, but papers over rounding errors. I
> > >   think we could even do a drm_mode_compare_vrefresh for that, we're
> > >   probably not the only ones who'd like to have such a function.
> > > 
> > > Probably a bit more work to type (but cocci might help here), definitely
> > > less painful to make sure it stays correct I think. Thoughts?
> > 
> > Yeah, would probably make sense. But apart from the "make sure this
> > don't break" angle it doesn't matter for this case unless we want
> > to make the filtering even more strict than the 1 Hz resolution.
> 
> Well it's generally video folks who care about this, and those folks will
> spot the 1 Hz mismatch. I just realized that you can implement the
> filtering without all the refactoring:

You mean the same ones who complain about 24Hz != 23.976Hz, and the one
frame out of sync every 40s.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx