All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff
@ 2018-09-17 15:15 Ville Syrjala
  2018-09-17 15:15 ` [PATCH 2/2] drm/i915/sdvo: Utilize intel_panel for fixed_mode Ville Syrjala
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Ville Syrjala @ 2018-09-17 15:15 UTC (permalink / raw)
  To: intel-gfx

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 812fe7b06f87..701372e512a8 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",
@@ -1630,6 +1625,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)
@@ -1644,7 +1641,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;
 
@@ -1759,6 +1756,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;
 
@@ -1797,7 +1796,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 =
@@ -1875,17 +1874,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;
 }
 
@@ -2054,16 +2042,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)
@@ -2555,7 +2533,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) {
@@ -2563,7 +2541,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;
@@ -2591,8 +2569,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;
@@ -2654,6 +2630,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);
 
@@ -2682,6 +2659,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:
@@ -2692,9 +2682,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

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] drm/i915/sdvo: Utilize intel_panel for fixed_mode
  2018-09-17 15:15 [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff Ville Syrjala
@ 2018-09-17 15:15 ` Ville Syrjala
  2018-10-09 14:44   ` Jani Nikula
  2018-09-17 15:21 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjala @ 2018-09-17 15:15 UTC (permalink / raw)
  To: intel-gfx

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 701372e512a8..11e8d598702e 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))
@@ -1642,10 +1641,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;
 	}
 
@@ -2063,6 +2065,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);
 }
 
@@ -2267,10 +2270,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);
 }
@@ -2663,13 +2662,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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff
  2018-09-17 15:15 [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff Ville Syrjala
  2018-09-17 15:15 ` [PATCH 2/2] drm/i915/sdvo: Utilize intel_panel for fixed_mode Ville Syrjala
@ 2018-09-17 15:21 ` Patchwork
  2018-09-17 15:41 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-09-17 15:21 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff
URL   : https://patchwork.freedesktop.org/series/49795/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
61a7c71555be 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
8a795501a8cf 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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff
  2018-09-17 15:15 [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff Ville Syrjala
  2018-09-17 15:15 ` [PATCH 2/2] drm/i915/sdvo: Utilize intel_panel for fixed_mode Ville Syrjala
  2018-09-17 15:21 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff Patchwork
@ 2018-09-17 15:41 ` Patchwork
  2018-09-17 16:41 ` ✓ Fi.CI.IGT: " Patchwork
  2018-09-17 17:16 ` [PATCH 1/2] " Rodrigo Vivi
  4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-09-17 15:41 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff
URL   : https://patchwork.freedesktop.org/series/49795/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4833 -> Patchwork_10204 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-skl-caroline:    NOTRUN -> INCOMPLETE (fdo#107556, fdo#104108)

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-skl-6700k2:      PASS -> FAIL (fdo#103191)

    igt@kms_psr@primary_mmap_gtt:
      {fi-cnl-u}:         NOTRUN -> FAIL (fdo#107383) +3

    igt@kms_psr@primary_page_flip:
      fi-kbl-7560u:       PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@drv_selftest@live_hangcheck:
      fi-glk-j4005:       INCOMPLETE (fdo#103359, k.org#198133) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-icl-u:           INCOMPLETE (fdo#107713) -> PASS

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

    igt@kms_psr@primary_page_flip:
      fi-kbl-r:           FAIL (fdo#107336) -> PASS

    igt@kms_setmode@basic-clone-single-crtc:
      fi-snb-2520m:       DMESG-WARN (fdo#103713) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107383 https://bugs.freedesktop.org/show_bug.cgi?id=107383
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (46 -> 42) ==

  Additional (2): fi-cnl-u fi-skl-caroline 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-bdw-gvtdvm fi-byt-squawks fi-bsw-cyan fi-bsw-kefka 


== Build changes ==

    * Linux: CI_DRM_4833 -> Patchwork_10204

  CI_DRM_4833: 75bb460b367a614d10b0fba220143bee42657d7e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4644: 0b59bb3231ab481959528c5c7b3a98762772e1b0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10204: 8a795501a8cf58a8abb713bf74e454271e7fd6c1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8a795501a8cf drm/i915/sdvo: Utilize intel_panel for fixed_mode
61a7c71555be drm/i915/sdvo: Fix multi function encoder stuff

== Logs ==

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff
  2018-09-17 15:15 [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff Ville Syrjala
                   ` (2 preceding siblings ...)
  2018-09-17 15:41 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-09-17 16:41 ` Patchwork
  2018-09-17 17:16 ` [PATCH 1/2] " Rodrigo Vivi
  4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-09-17 16:41 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff
URL   : https://patchwork.freedesktop.org/series/49795/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4833_full -> Patchwork_10204_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
      shard-apl:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105363)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@fbc-stridechange:
      shard-glk:          FAIL (fdo#103167) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


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

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4833 -> Patchwork_10204

  CI_DRM_4833: 75bb460b367a614d10b0fba220143bee42657d7e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4644: 0b59bb3231ab481959528c5c7b3a98762772e1b0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10204: 8a795501a8cf58a8abb713bf74e454271e7fd6c1 @ 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_10204/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff
  2018-09-17 15:15 [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff Ville Syrjala
                   ` (3 preceding siblings ...)
  2018-09-17 16:41 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-09-17 17:16 ` Rodrigo Vivi
  2018-09-17 17:34   ` Ville Syrjälä
  4 siblings, 1 reply; 9+ messages in thread
From: Rodrigo Vivi @ 2018-09-17 17:16 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

On Mon, Sep 17, 2018 at 06:15:03PM +0300, Ville Syrjala wrote:
> 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.

I wondered if this last part didn't deserved a separated patch. But not sure
how chicken-egg issue this could cause so maybe the same patch is better
indeed...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 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 812fe7b06f87..701372e512a8 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",
> @@ -1630,6 +1625,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)
> @@ -1644,7 +1641,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;
>  
> @@ -1759,6 +1756,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;
>  
> @@ -1797,7 +1796,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 =
> @@ -1875,17 +1874,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;
>  }
>  
> @@ -2054,16 +2042,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)
> @@ -2555,7 +2533,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) {
> @@ -2563,7 +2541,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;
> @@ -2591,8 +2569,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;
> @@ -2654,6 +2630,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);
>  
> @@ -2682,6 +2659,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:
> @@ -2692,9 +2682,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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff
  2018-09-17 17:16 ` [PATCH 1/2] " Rodrigo Vivi
@ 2018-09-17 17:34   ` Ville Syrjälä
  2018-09-18 18:10     ` Ville Syrjälä
  0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2018-09-17 17:34 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

On Mon, Sep 17, 2018 at 10:16:05AM -0700, Rodrigo Vivi wrote:
> On Mon, Sep 17, 2018 at 06:15:03PM +0300, Ville Syrjala wrote:
> > 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.
> 
> I wondered if this last part didn't deserved a separated patch. But not sure
> how chicken-egg issue this could cause so maybe the same patch is better
> indeed...

There are both chickens and eggs in there for sure. I suppose it might
have been possible to handle it by clearing the LVDS bit from
output_flag if we fail to find the mode. But then we'd be left with
an LVDS connector without the flag and I'm not quite sure where the
code would lead if we ever tried to use it. Although one can argue
that the current code already has that same issue, maybe.

> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> > 
> > 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 812fe7b06f87..701372e512a8 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",
> > @@ -1630,6 +1625,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)
> > @@ -1644,7 +1641,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;
> >  
> > @@ -1759,6 +1756,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;
> >  
> > @@ -1797,7 +1796,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 =
> > @@ -1875,17 +1874,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;
> >  }
> >  
> > @@ -2054,16 +2042,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)
> > @@ -2555,7 +2533,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) {
> > @@ -2563,7 +2541,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;
> > @@ -2591,8 +2569,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;
> > @@ -2654,6 +2630,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);
> >  
> > @@ -2682,6 +2659,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:
> > @@ -2692,9 +2682,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
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff
  2018-09-17 17:34   ` Ville Syrjälä
@ 2018-09-18 18:10     ` Ville Syrjälä
  0 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2018-09-18 18:10 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

On Mon, Sep 17, 2018 at 08:34:36PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 17, 2018 at 10:16:05AM -0700, Rodrigo Vivi wrote:
> > On Mon, Sep 17, 2018 at 06:15:03PM +0300, Ville Syrjala wrote:
> > > 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.
> > 
> > I wondered if this last part didn't deserved a separated patch. But not sure
> > how chicken-egg issue this could cause so maybe the same patch is better
> > indeed...
> 
> There are both chickens and eggs in there for sure. I suppose it might
> have been possible to handle it by clearing the LVDS bit from
> output_flag if we fail to find the mode. But then we'd be left with
> an LVDS connector without the flag and I'm not quite sure where the
> code would lead if we ever tried to use it. Although one can argue
> that the current code already has that same issue, maybe.

Decided to keep this patch as is, and pushed it. Thanks for the review.

> 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > > 
> > > 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 812fe7b06f87..701372e512a8 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",
> > > @@ -1630,6 +1625,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)
> > > @@ -1644,7 +1641,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;
> > >  
> > > @@ -1759,6 +1756,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;
> > >  
> > > @@ -1797,7 +1796,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 =
> > > @@ -1875,17 +1874,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;
> > >  }
> > >  
> > > @@ -2054,16 +2042,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)
> > > @@ -2555,7 +2533,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) {
> > > @@ -2563,7 +2541,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;
> > > @@ -2591,8 +2569,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;
> > > @@ -2654,6 +2630,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);
> > >  
> > > @@ -2682,6 +2659,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:
> > > @@ -2692,9 +2682,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
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] drm/i915/sdvo: Utilize intel_panel for fixed_mode
  2018-09-17 15:15 ` [PATCH 2/2] drm/i915/sdvo: Utilize intel_panel for fixed_mode Ville Syrjala
@ 2018-10-09 14:44   ` Jani Nikula
  0 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2018-10-09 14:44 UTC (permalink / raw)
  To: Ville Syrjala, intel-gfx

On Mon, 17 Sep 2018, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> 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>

Reviewed-by: Jani Nikula <jani.nikula@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 701372e512a8..11e8d598702e 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))
> @@ -1642,10 +1641,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;
>  	}
>  
> @@ -2063,6 +2065,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);
>  }
>  
> @@ -2267,10 +2270,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);
>  }
> @@ -2663,13 +2662,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;

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-10-09 14:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-17 15:15 [PATCH 1/2] drm/i915/sdvo: Fix multi function encoder stuff Ville Syrjala
2018-09-17 15:15 ` [PATCH 2/2] drm/i915/sdvo: Utilize intel_panel for fixed_mode Ville Syrjala
2018-10-09 14:44   ` Jani Nikula
2018-09-17 15:21 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/sdvo: Fix multi function encoder stuff Patchwork
2018-09-17 15:41 ` ✓ Fi.CI.BAT: success " Patchwork
2018-09-17 16:41 ` ✓ Fi.CI.IGT: " Patchwork
2018-09-17 17:16 ` [PATCH 1/2] " Rodrigo Vivi
2018-09-17 17:34   ` Ville Syrjälä
2018-09-18 18:10     ` Ville Syrjälä

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.