All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
@ 2017-05-11  9:57 Jani Nikula
  2017-05-11  9:57 ` [PATCH 2/2] drm/i915: Detect USB-C specific dongles before reducing M and N Jani Nikula
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Jani Nikula @ 2017-05-11  9:57 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: jani.nikula, dhinakaran.pandiyan

Face the fact, there are Display Port sink and branch devices out there
in the wild that don't follow the Display Port specifications, or they
have bugs, or just otherwise require special treatment. Start a common
quirk database the drivers can query based on OUI (with the option of
expanding to device identification string and hardware/firmware
revisions later). At least for now, we leave the workarounds for the
drivers to implement as they see fit.

For starters, add a branch device that can't handle full 24-bit main
link M and N attributes properly. Naturally, the workaround of reducing
main link attributes for all devices ended up in regressions for other
devices. So here we are.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Clint Taylor <clinton.a.taylor@intel.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 61 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |  7 +++++
 2 files changed, 68 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3e5f52110ea1..58b2ced33151 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1208,3 +1208,64 @@ int drm_dp_stop_crc(struct drm_dp_aux *aux)
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_stop_crc);
+
+/*
+ * Display Port sink and branch devices in the wild have a variety of bugs, try
+ * to collect them here. The quirks are shared, but it's up to the drivers to
+ * implement workarounds for them.
+ */
+
+/*
+ * FIXME: Add support for device identification and hardware/firmware revision.
+ */
+struct dpcd_quirk {
+	u8 oui[3];
+	bool is_branch;
+	u32 quirks;
+};
+
+#define OUI(first, second, third) { (first), (second), (third) }
+
+static const struct dpcd_quirk dpcd_quirk_list[] = {
+	/* Analogix 7737 needs reduced M and N at HBR2 link rates */
+	{ OUI(0x00, 0x22, 0xb9), true, DP_DPCD_QUIRK_LIMITED_M_N },
+};
+
+#undef OUI
+
+/**
+ * drm_dp_get_quirks() - get quirks for the sink/branch device
+ * @oui: pointer to len bytes of DPCD at 0x400 (sink) or 0x500 (branch)
+ * @len: 3-12 bytes of DPCD data
+ * @is_branch: true for branch devices, false for sink devices
+ *
+ * Get a bit mask of DPCD quirks for the sink/branch device. The quirk data is
+ * shared but it's up to the drivers to act on the data.
+ *
+ * For now, only the OUI (first three bytes) is used, but this may be extended
+ * to device identification string and hardware/firmware revisions later.
+ */
+u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch)
+{
+	const struct dpcd_quirk *quirk;
+	u32 quirks = 0;
+	int i;
+
+	if (WARN_ON_ONCE(len < 3))
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(dpcd_quirk_list); i++) {
+		quirk = &dpcd_quirk_list[i];
+
+		if (quirk->is_branch != is_branch)
+			continue;
+
+		if (memcmp(quirk->oui, oui, 3) != 0)
+			continue;
+
+		quirks |= quirk->quirks;
+	}
+
+	return quirks;
+}
+EXPORT_SYMBOL(drm_dp_get_quirks);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index f7007e544f29..8abe9897fe59 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1079,4 +1079,11 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
 int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
 int drm_dp_stop_crc(struct drm_dp_aux *aux);
 
+/* Display Port sink and branch device quirks. */
+
+/* The sink is limited to small link M and N values. */
+#define DP_DPCD_QUIRK_LIMITED_M_N			BIT(0)
+
+u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch);
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
2.11.0

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

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

* [PATCH 2/2] drm/i915: Detect USB-C specific dongles before reducing M and N
  2017-05-11  9:57 [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Jani Nikula
@ 2017-05-11  9:57 ` Jani Nikula
  2017-05-11 16:12   ` Clint Taylor
  2017-05-11 10:44 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2017-05-11  9:57 UTC (permalink / raw)
  To: intel-gfx, dri-devel
  Cc: jani.nikula, ajax, dhinakaran.pandiyan, harry.wentland

From: Clint Taylor <clinton.a.taylor@intel.com>

The Analogix 7737 DP to HDMI converter requires reduced M and N values
when to operate correctly at HBR2. Detect this IC by its OUI value of
0x0022B9 via the DPCD quirk list.

v2 by Jani: Rebased on the DP quirk database

Fixes: 9a86cda07af2 ("drm/i915/dp: reduce link M/N parameters")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100755
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
 drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++--------
 drivers/gpu/drm/i915/intel_dp.c      | 15 +++++++++++----
 drivers/gpu/drm/i915/intel_dp_mst.c  |  4 +++-
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 5 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff3574a56812..d34412673d61 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -563,7 +563,8 @@ struct intel_link_m_n {
 
 void intel_link_compute_m_n(int bpp, int nlanes,
 			    int pixel_clock, int link_clock,
-			    struct intel_link_m_n *m_n);
+			    struct intel_link_m_n *m_n,
+			    bool reduce_m_n);
 
 /* Interface history:
  *
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bf9020da8b80..46ac46ca976c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6116,7 +6116,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 	pipe_config->fdi_lanes = lane;
 
 	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
-			       link_bw, &pipe_config->fdi_m_n);
+			       link_bw, &pipe_config->fdi_m_n, false);
 
 	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
 	if (ret == -EINVAL && pipe_config->pipe_bpp > 6*3) {
@@ -6292,7 +6292,8 @@ intel_reduce_m_n_ratio(uint32_t *num, uint32_t *den)
 }
 
 static void compute_m_n(unsigned int m, unsigned int n,
-			uint32_t *ret_m, uint32_t *ret_n)
+			uint32_t *ret_m, uint32_t *ret_n,
+			bool reduce_m_n)
 {
 	/*
 	 * Reduce M/N as much as possible without loss in precision. Several DP
@@ -6300,9 +6301,11 @@ static void compute_m_n(unsigned int m, unsigned int n,
 	 * values. The passed in values are more likely to have the least
 	 * significant bits zero than M after rounding below, so do this first.
 	 */
-	while ((m & 1) == 0 && (n & 1) == 0) {
-		m >>= 1;
-		n >>= 1;
+	if (reduce_m_n) {
+		while ((m & 1) == 0 && (n & 1) == 0) {
+			m >>= 1;
+			n >>= 1;
+		}
 	}
 
 	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
@@ -6313,16 +6316,19 @@ static void compute_m_n(unsigned int m, unsigned int n,
 void
 intel_link_compute_m_n(int bits_per_pixel, int nlanes,
 		       int pixel_clock, int link_clock,
-		       struct intel_link_m_n *m_n)
+		       struct intel_link_m_n *m_n,
+		       bool reduce_m_n)
 {
 	m_n->tu = 64;
 
 	compute_m_n(bits_per_pixel * pixel_clock,
 		    link_clock * nlanes * 8,
-		    &m_n->gmch_m, &m_n->gmch_n);
+		    &m_n->gmch_m, &m_n->gmch_n,
+		    reduce_m_n);
 
 	compute_m_n(pixel_clock, link_clock,
-		    &m_n->link_m, &m_n->link_n);
+		    &m_n->link_m, &m_n->link_n,
+		    reduce_m_n);
 }
 
 static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4a6feb6a69bd..59f3dbb242a6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1568,13 +1568,17 @@ bool intel_dp_read_desc(struct intel_dp *intel_dp)
 	if (!__intel_dp_read_desc(intel_dp, desc))
 		return false;
 
+	intel_dp->quirks = drm_dp_get_quirks(desc->oui, sizeof(desc->oui),
+					     drm_dp_is_branch(intel_dp->dpcd));
+
 	dev_id_len = strnlen(desc->device_id, sizeof(desc->device_id));
-	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %*pE HW-rev %d.%d SW-rev %d.%d\n",
+	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %*pE HW-rev %d.%d SW-rev %d.%d quirks 0x%04x\n",
 		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
 		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
 		      dev_id_len, desc->device_id,
 		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
-		      desc->sw_major_rev, desc->sw_minor_rev);
+		      desc->sw_major_rev, desc->sw_minor_rev,
+		      intel_dp->quirks);
 
 	return true;
 }
@@ -1658,6 +1662,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	int link_avail, link_clock;
 	int common_len;
 	uint8_t link_bw, rate_select;
+	bool reduce_m_n = intel_dp->quirks & DP_DPCD_QUIRK_LIMITED_M_N;
 
 	common_len = intel_dp_common_len_rate_limit(intel_dp,
 						    intel_dp->max_link_rate);
@@ -1790,7 +1795,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
-			       &pipe_config->dp_m_n);
+			       &pipe_config->dp_m_n,
+			       reduce_m_n);
 
 	if (intel_connector->panel.downclock_mode != NULL &&
 		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
@@ -1798,7 +1804,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 			intel_link_compute_m_n(bpp, lane_count,
 				intel_connector->panel.downclock_mode->clock,
 				pipe_config->port_clock,
-				&pipe_config->dp_m2_n2);
+				&pipe_config->dp_m2_n2,
+				reduce_m_n);
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 68c788eb0b95..763d490eeca9 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -44,6 +44,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	int lane_count, slots;
 	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	int mst_pbn;
+	bool reduce_m_n = intel_dp->quirks & DP_DPCD_QUIRK_LIMITED_M_N;
 
 	pipe_config->has_pch_encoder = false;
 	bpp = 24;
@@ -79,7 +80,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
-			       &pipe_config->dp_m_n);
+			       &pipe_config->dp_m_n,
+			       reduce_m_n);
 
 	pipe_config->dp_m_n.tu = slots;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bd500977b3fc..664156acff0c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -997,6 +997,8 @@ struct intel_dp {
 	int max_link_rate;
 	/* sink or branch descriptor */
 	struct intel_dp_desc desc;
+	/* Quirks as returned by drm_dp_get_quirks() */
+	u32 quirks;
 	struct drm_dp_aux aux;
 	enum intel_display_power_domain aux_power_domain;
 	uint8_t train_set[4];
-- 
2.11.0

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

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

* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
  2017-05-11  9:57 [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Jani Nikula
  2017-05-11  9:57 ` [PATCH 2/2] drm/i915: Detect USB-C specific dongles before reducing M and N Jani Nikula
@ 2017-05-11 10:44 ` Patchwork
  2017-05-11 16:11 ` [PATCH 1/2] " Clint Taylor
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2017-05-11 10:44 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
URL   : https://patchwork.freedesktop.org/series/24282/
State : warning

== Summary ==

Series 24282v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/24282/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-edid:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-load-detect:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup prune-stale-modes:
                pass       -> SKIP       (fi-ivb-3520m)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:438s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:431s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:582s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:511s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:540s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:492s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:484s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:422s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:407s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:415s
fi-ivb-3520m     total:278  pass:256  dwarn:0   dfail:0   fail:0   skip:22  time:495s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:504s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:460s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:570s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:465s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:581s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:470s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:493s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:434s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:537s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:411s

81dbd44ab4c1331c61a971b05681473a869b3039 drm-tip: 2017y-05m-11d-09h-55m-44s UTC integration manifest
4c53183 drm/i915: Detect USB-C specific dongles before reducing M and N
6072b33 drm/dp: start a DPCD based DP sink/branch device quirk database

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4672/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
  2017-05-11  9:57 [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Jani Nikula
  2017-05-11  9:57 ` [PATCH 2/2] drm/i915: Detect USB-C specific dongles before reducing M and N Jani Nikula
  2017-05-11 10:44 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Patchwork
@ 2017-05-11 16:11 ` Clint Taylor
  2017-05-11 18:17 ` [Intel-gfx] " Daniel Vetter
  2017-05-15 20:42 ` Pandiyan, Dhinakaran
  4 siblings, 0 replies; 10+ messages in thread
From: Clint Taylor @ 2017-05-11 16:11 UTC (permalink / raw)
  To: Jani Nikula, intel-gfx, dri-devel; +Cc: dhinakaran.pandiyan



On 05/11/2017 02:57 AM, Jani Nikula wrote:
> Face the fact, there are Display Port sink and branch devices out there
> in the wild that don't follow the Display Port specifications, or they
> have bugs, or just otherwise require special treatment. Start a common
> quirk database the drivers can query based on OUI (with the option of
> expanding to device identification string and hardware/firmware
> revisions later). At least for now, we leave the workarounds for the
> drivers to implement as they see fit.
>
> For starters, add a branch device that can't handle full 24-bit main
> link M and N attributes properly. Naturally, the workaround of reducing
> main link attributes for all devices ended up in regressions for other
> devices. So here we are.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>

Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/drm_dp_helper.c | 61 +++++++++++++++++++++++++++++++++++++++++
>   include/drm/drm_dp_helper.h     |  7 +++++
>   2 files changed, 68 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 3e5f52110ea1..58b2ced33151 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1208,3 +1208,64 @@ int drm_dp_stop_crc(struct drm_dp_aux *aux)
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_dp_stop_crc);
> +
> +/*
> + * Display Port sink and branch devices in the wild have a variety of bugs, try
> + * to collect them here. The quirks are shared, but it's up to the drivers to
> + * implement workarounds for them.
> + */
> +
> +/*
> + * FIXME: Add support for device identification and hardware/firmware revision.
> + */
> +struct dpcd_quirk {
> +	u8 oui[3];
> +	bool is_branch;
> +	u32 quirks;
> +};
> +
> +#define OUI(first, second, third) { (first), (second), (third) }
> +
> +static const struct dpcd_quirk dpcd_quirk_list[] = {
> +	/* Analogix 7737 needs reduced M and N at HBR2 link rates */
> +	{ OUI(0x00, 0x22, 0xb9), true, DP_DPCD_QUIRK_LIMITED_M_N },
> +};
> +
> +#undef OUI
> +
> +/**
> + * drm_dp_get_quirks() - get quirks for the sink/branch device
> + * @oui: pointer to len bytes of DPCD at 0x400 (sink) or 0x500 (branch)
> + * @len: 3-12 bytes of DPCD data
> + * @is_branch: true for branch devices, false for sink devices
> + *
> + * Get a bit mask of DPCD quirks for the sink/branch device. The quirk data is
> + * shared but it's up to the drivers to act on the data.
> + *
> + * For now, only the OUI (first three bytes) is used, but this may be extended
> + * to device identification string and hardware/firmware revisions later.
> + */
> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch)
> +{
> +	const struct dpcd_quirk *quirk;
> +	u32 quirks = 0;
> +	int i;
> +
> +	if (WARN_ON_ONCE(len < 3))
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(dpcd_quirk_list); i++) {
> +		quirk = &dpcd_quirk_list[i];
> +
> +		if (quirk->is_branch != is_branch)
> +			continue;
> +
> +		if (memcmp(quirk->oui, oui, 3) != 0)
> +			continue;
> +
> +		quirks |= quirk->quirks;
> +	}
> +
> +	return quirks;
> +}
> +EXPORT_SYMBOL(drm_dp_get_quirks);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index f7007e544f29..8abe9897fe59 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1079,4 +1079,11 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>   int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
>   int drm_dp_stop_crc(struct drm_dp_aux *aux);
>   
> +/* Display Port sink and branch device quirks. */
> +
> +/* The sink is limited to small link M and N values. */
> +#define DP_DPCD_QUIRK_LIMITED_M_N			BIT(0)
> +
> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch);
> +
>   #endif /* _DRM_DP_HELPER_H_ */

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

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

* Re: [PATCH 2/2] drm/i915: Detect USB-C specific dongles before reducing M and N
  2017-05-11  9:57 ` [PATCH 2/2] drm/i915: Detect USB-C specific dongles before reducing M and N Jani Nikula
@ 2017-05-11 16:12   ` Clint Taylor
  2017-05-15 20:24     ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 10+ messages in thread
From: Clint Taylor @ 2017-05-11 16:12 UTC (permalink / raw)
  To: Jani Nikula, intel-gfx, dri-devel; +Cc: dhinakaran.pandiyan



On 05/11/2017 02:57 AM, Jani Nikula wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
>
> The Analogix 7737 DP to HDMI converter requires reduced M and N values
> when to operate correctly at HBR2. Detect this IC by its OUI value of
> 0x0022B9 via the DPCD quirk list.
>
> v2 by Jani: Rebased on the DP quirk database
>
> Fixes: 9a86cda07af2 ("drm/i915/dp: reduce link M/N parameters")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100755
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>

> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
>   drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++--------
>   drivers/gpu/drm/i915/intel_dp.c      | 15 +++++++++++----
>   drivers/gpu/drm/i915/intel_dp_mst.c  |  4 +++-
>   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>   5 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ff3574a56812..d34412673d61 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -563,7 +563,8 @@ struct intel_link_m_n {
>   
>   void intel_link_compute_m_n(int bpp, int nlanes,
>   			    int pixel_clock, int link_clock,
> -			    struct intel_link_m_n *m_n);
> +			    struct intel_link_m_n *m_n,
> +			    bool reduce_m_n);
>   
>   /* Interface history:
>    *
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bf9020da8b80..46ac46ca976c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6116,7 +6116,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>   	pipe_config->fdi_lanes = lane;
>   
>   	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> -			       link_bw, &pipe_config->fdi_m_n);
> +			       link_bw, &pipe_config->fdi_m_n, false);
>   
>   	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
>   	if (ret == -EINVAL && pipe_config->pipe_bpp > 6*3) {
> @@ -6292,7 +6292,8 @@ intel_reduce_m_n_ratio(uint32_t *num, uint32_t *den)
>   }
>   
>   static void compute_m_n(unsigned int m, unsigned int n,
> -			uint32_t *ret_m, uint32_t *ret_n)
> +			uint32_t *ret_m, uint32_t *ret_n,
> +			bool reduce_m_n)
>   {
>   	/*
>   	 * Reduce M/N as much as possible without loss in precision. Several DP
> @@ -6300,9 +6301,11 @@ static void compute_m_n(unsigned int m, unsigned int n,
>   	 * values. The passed in values are more likely to have the least
>   	 * significant bits zero than M after rounding below, so do this first.
>   	 */
> -	while ((m & 1) == 0 && (n & 1) == 0) {
> -		m >>= 1;
> -		n >>= 1;
> +	if (reduce_m_n) {
> +		while ((m & 1) == 0 && (n & 1) == 0) {
> +			m >>= 1;
> +			n >>= 1;
> +		}
>   	}
>   
>   	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
> @@ -6313,16 +6316,19 @@ static void compute_m_n(unsigned int m, unsigned int n,
>   void
>   intel_link_compute_m_n(int bits_per_pixel, int nlanes,
>   		       int pixel_clock, int link_clock,
> -		       struct intel_link_m_n *m_n)
> +		       struct intel_link_m_n *m_n,
> +		       bool reduce_m_n)
>   {
>   	m_n->tu = 64;
>   
>   	compute_m_n(bits_per_pixel * pixel_clock,
>   		    link_clock * nlanes * 8,
> -		    &m_n->gmch_m, &m_n->gmch_n);
> +		    &m_n->gmch_m, &m_n->gmch_n,
> +		    reduce_m_n);
>   
>   	compute_m_n(pixel_clock, link_clock,
> -		    &m_n->link_m, &m_n->link_n);
> +		    &m_n->link_m, &m_n->link_n,
> +		    reduce_m_n);
>   }
>   
>   static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4a6feb6a69bd..59f3dbb242a6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1568,13 +1568,17 @@ bool intel_dp_read_desc(struct intel_dp *intel_dp)
>   	if (!__intel_dp_read_desc(intel_dp, desc))
>   		return false;
>   
> +	intel_dp->quirks = drm_dp_get_quirks(desc->oui, sizeof(desc->oui),
> +					     drm_dp_is_branch(intel_dp->dpcd));
> +
>   	dev_id_len = strnlen(desc->device_id, sizeof(desc->device_id));
> -	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %*pE HW-rev %d.%d SW-rev %d.%d\n",
> +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %*pE HW-rev %d.%d SW-rev %d.%d quirks 0x%04x\n",
>   		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
>   		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
>   		      dev_id_len, desc->device_id,
>   		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
> -		      desc->sw_major_rev, desc->sw_minor_rev);
> +		      desc->sw_major_rev, desc->sw_minor_rev,
> +		      intel_dp->quirks);
>   
>   	return true;
>   }
> @@ -1658,6 +1662,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>   	int link_avail, link_clock;
>   	int common_len;
>   	uint8_t link_bw, rate_select;
> +	bool reduce_m_n = intel_dp->quirks & DP_DPCD_QUIRK_LIMITED_M_N;
>   
>   	common_len = intel_dp_common_len_rate_limit(intel_dp,
>   						    intel_dp->max_link_rate);
> @@ -1790,7 +1795,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>   	intel_link_compute_m_n(bpp, lane_count,
>   			       adjusted_mode->crtc_clock,
>   			       pipe_config->port_clock,
> -			       &pipe_config->dp_m_n);
> +			       &pipe_config->dp_m_n,
> +			       reduce_m_n);
>   
>   	if (intel_connector->panel.downclock_mode != NULL &&
>   		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
> @@ -1798,7 +1804,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>   			intel_link_compute_m_n(bpp, lane_count,
>   				intel_connector->panel.downclock_mode->clock,
>   				pipe_config->port_clock,
> -				&pipe_config->dp_m2_n2);
> +				&pipe_config->dp_m2_n2,
> +				reduce_m_n);
>   	}
>   
>   	/*
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 68c788eb0b95..763d490eeca9 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -44,6 +44,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>   	int lane_count, slots;
>   	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>   	int mst_pbn;
> +	bool reduce_m_n = intel_dp->quirks & DP_DPCD_QUIRK_LIMITED_M_N;
>   
>   	pipe_config->has_pch_encoder = false;
>   	bpp = 24;
> @@ -79,7 +80,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>   	intel_link_compute_m_n(bpp, lane_count,
>   			       adjusted_mode->crtc_clock,
>   			       pipe_config->port_clock,
> -			       &pipe_config->dp_m_n);
> +			       &pipe_config->dp_m_n,
> +			       reduce_m_n);
>   
>   	pipe_config->dp_m_n.tu = slots;
>   
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bd500977b3fc..664156acff0c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -997,6 +997,8 @@ struct intel_dp {
>   	int max_link_rate;
>   	/* sink or branch descriptor */
>   	struct intel_dp_desc desc;
> +	/* Quirks as returned by drm_dp_get_quirks() */
> +	u32 quirks;
>   	struct drm_dp_aux aux;
>   	enum intel_display_power_domain aux_power_domain;
>   	uint8_t train_set[4];

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

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

* Re: [Intel-gfx] [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
  2017-05-11  9:57 [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Jani Nikula
                   ` (2 preceding siblings ...)
  2017-05-11 16:11 ` [PATCH 1/2] " Clint Taylor
@ 2017-05-11 18:17 ` Daniel Vetter
  2017-05-12  7:57   ` Jani Nikula
  2017-05-15 20:42 ` Pandiyan, Dhinakaran
  4 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2017-05-11 18:17 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, dhinakaran.pandiyan, dri-devel

On Thu, May 11, 2017 at 12:57:20PM +0300, Jani Nikula wrote:
> Face the fact, there are Display Port sink and branch devices out there
> in the wild that don't follow the Display Port specifications, or they
> have bugs, or just otherwise require special treatment. Start a common
> quirk database the drivers can query based on OUI (with the option of
> expanding to device identification string and hardware/firmware
> revisions later). At least for now, we leave the workarounds for the
> drivers to implement as they see fit.
> 
> For starters, add a branch device that can't handle full 24-bit main
> link M and N attributes properly. Naturally, the workaround of reducing
> main link attributes for all devices ended up in regressions for other
> devices. So here we are.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

A few small bikesheds below. Ack in principle.
-Daniel

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 61 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |  7 +++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 3e5f52110ea1..58b2ced33151 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1208,3 +1208,64 @@ int drm_dp_stop_crc(struct drm_dp_aux *aux)
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_dp_stop_crc);
> +
> +/*
> + * Display Port sink and branch devices in the wild have a variety of bugs, try
> + * to collect them here. The quirks are shared, but it's up to the drivers to
> + * implement workarounds for them.
> + */

I'd stash this in the kerneldoc for drm_dp_get_quirks().

> +
> +/*
> + * FIXME: Add support for device identification and hardware/firmware revision.
> + */
> +struct dpcd_quirk {
> +	u8 oui[3];
> +	bool is_branch;
> +	u32 quirks;
> +};
> +
> +#define OUI(first, second, third) { (first), (second), (third) }
> +
> +static const struct dpcd_quirk dpcd_quirk_list[] = {
> +	/* Analogix 7737 needs reduced M and N at HBR2 link rates */
> +	{ OUI(0x00, 0x22, 0xb9), true, DP_DPCD_QUIRK_LIMITED_M_N },
> +};
> +
> +#undef OUI
> +
> +/**
> + * drm_dp_get_quirks() - get quirks for the sink/branch device
> + * @oui: pointer to len bytes of DPCD at 0x400 (sink) or 0x500 (branch)
> + * @len: 3-12 bytes of DPCD data
> + * @is_branch: true for branch devices, false for sink devices
> + *
> + * Get a bit mask of DPCD quirks for the sink/branch device. The quirk data is
> + * shared but it's up to the drivers to act on the data.
> + *
> + * For now, only the OUI (first three bytes) is used, but this may be extended
> + * to device identification string and hardware/firmware revisions later.
> + */
> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch)

Should we either base this on drm_dp_aux (so that this function would
query for quirks), or on struct drm_dp_link (which is kinda something
tegra and msm started to use to cache some sink properties)?

This is with an eye towards handling your FIXME, and maybe even converging
the dp helpers a bit more again.

Personally (aka my own bikeshed right now) I'm leaning towards struct
drm_dp_link and extending that.

> +{
> +	const struct dpcd_quirk *quirk;
> +	u32 quirks = 0;
> +	int i;
> +
> +	if (WARN_ON_ONCE(len < 3))
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(dpcd_quirk_list); i++) {
> +		quirk = &dpcd_quirk_list[i];
> +
> +		if (quirk->is_branch != is_branch)
> +			continue;
> +
> +		if (memcmp(quirk->oui, oui, 3) != 0)
> +			continue;
> +
> +		quirks |= quirk->quirks;
> +	}
> +
> +	return quirks;
> +}
> +EXPORT_SYMBOL(drm_dp_get_quirks);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index f7007e544f29..8abe9897fe59 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1079,4 +1079,11 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
>  int drm_dp_stop_crc(struct drm_dp_aux *aux);
>  
> +/* Display Port sink and branch device quirks. */
> +
> +/* The sink is limited to small link M and N values. */
> +#define DP_DPCD_QUIRK_LIMITED_M_N			BIT(0)

Bikeshed: I much prefer enums (with bitfields) than defines for internal
stuff, allows them to be nicely kernel-doc documented and linked. Which I
think would be good to do here, to spec out very precisely what exactly
the hack is that needs to be applied. LIMITED_M_N doesn't tell you
anything without looking at the i915 code.

> +
> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch);
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

* Re: [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
  2017-05-11 18:17 ` [Intel-gfx] " Daniel Vetter
@ 2017-05-12  7:57   ` Jani Nikula
  2017-05-14 11:00     ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2017-05-12  7:57 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: harry.wentland, intel-gfx, ajax, dhinakaran.pandiyan, dri-devel

On Thu, 11 May 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, May 11, 2017 at 12:57:20PM +0300, Jani Nikula wrote:
>> Face the fact, there are Display Port sink and branch devices out there
>> in the wild that don't follow the Display Port specifications, or they
>> have bugs, or just otherwise require special treatment. Start a common
>> quirk database the drivers can query based on OUI (with the option of
>> expanding to device identification string and hardware/firmware
>> revisions later). At least for now, we leave the workarounds for the
>> drivers to implement as they see fit.
>> 
>> For starters, add a branch device that can't handle full 24-bit main
>> link M and N attributes properly. Naturally, the workaround of reducing
>> main link attributes for all devices ended up in regressions for other
>> devices. So here we are.
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>> Cc: Adam Jackson <ajax@redhat.com>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> A few small bikesheds below. Ack in principle.
> -Daniel
>
>> ---
>>  drivers/gpu/drm/drm_dp_helper.c | 61 +++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_dp_helper.h     |  7 +++++
>>  2 files changed, 68 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 3e5f52110ea1..58b2ced33151 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -1208,3 +1208,64 @@ int drm_dp_stop_crc(struct drm_dp_aux *aux)
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL(drm_dp_stop_crc);
>> +
>> +/*
>> + * Display Port sink and branch devices in the wild have a variety of bugs, try
>> + * to collect them here. The quirks are shared, but it's up to the drivers to
>> + * implement workarounds for them.
>> + */
>
> I'd stash this in the kerneldoc for drm_dp_get_quirks().
>
>> +
>> +/*
>> + * FIXME: Add support for device identification and hardware/firmware revision.
>> + */
>> +struct dpcd_quirk {
>> +	u8 oui[3];
>> +	bool is_branch;
>> +	u32 quirks;
>> +};
>> +
>> +#define OUI(first, second, third) { (first), (second), (third) }
>> +
>> +static const struct dpcd_quirk dpcd_quirk_list[] = {
>> +	/* Analogix 7737 needs reduced M and N at HBR2 link rates */
>> +	{ OUI(0x00, 0x22, 0xb9), true, DP_DPCD_QUIRK_LIMITED_M_N },
>> +};
>> +
>> +#undef OUI
>> +
>> +/**
>> + * drm_dp_get_quirks() - get quirks for the sink/branch device
>> + * @oui: pointer to len bytes of DPCD at 0x400 (sink) or 0x500 (branch)
>> + * @len: 3-12 bytes of DPCD data
>> + * @is_branch: true for branch devices, false for sink devices
>> + *
>> + * Get a bit mask of DPCD quirks for the sink/branch device. The quirk data is
>> + * shared but it's up to the drivers to act on the data.
>> + *
>> + * For now, only the OUI (first three bytes) is used, but this may be extended
>> + * to device identification string and hardware/firmware revisions later.
>> + */
>> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch)
>
> Should we either base this on drm_dp_aux (so that this function would
> query for quirks), or on struct drm_dp_link (which is kinda something
> tegra and msm started to use to cache some sink properties)?
>
> This is with an eye towards handling your FIXME, and maybe even converging
> the dp helpers a bit more again.
>
> Personally (aka my own bikeshed right now) I'm leaning towards struct
> drm_dp_link and extending that.

Except we don't use drm_dp_link in i915, and I don't see starting to use
that would help us in any way. What we do with the source/sink/link
properties is much more complicated.

As to drm_dp_aux, I think that's the wrong level of abstraction, and I'd
much prefer the drivers would be in control of the DPCD access.

I guess we could move struct intel_dp_desc and intel_dp_read_desc() to
drm helpers, and store the quirks in struct intel_dp_desc upon reading.

>
>> +{
>> +	const struct dpcd_quirk *quirk;
>> +	u32 quirks = 0;
>> +	int i;
>> +
>> +	if (WARN_ON_ONCE(len < 3))
>> +		return 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(dpcd_quirk_list); i++) {
>> +		quirk = &dpcd_quirk_list[i];
>> +
>> +		if (quirk->is_branch != is_branch)
>> +			continue;
>> +
>> +		if (memcmp(quirk->oui, oui, 3) != 0)
>> +			continue;
>> +
>> +		quirks |= quirk->quirks;
>> +	}
>> +
>> +	return quirks;
>> +}
>> +EXPORT_SYMBOL(drm_dp_get_quirks);
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index f7007e544f29..8abe9897fe59 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -1079,4 +1079,11 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>>  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
>>  int drm_dp_stop_crc(struct drm_dp_aux *aux);
>>  
>> +/* Display Port sink and branch device quirks. */
>> +
>> +/* The sink is limited to small link M and N values. */
>> +#define DP_DPCD_QUIRK_LIMITED_M_N			BIT(0)
>
> Bikeshed: I much prefer enums (with bitfields) than defines for internal
> stuff, allows them to be nicely kernel-doc documented and linked. Which I
> think would be good to do here, to spec out very precisely what exactly
> the hack is that needs to be applied. LIMITED_M_N doesn't tell you
> anything without looking at the i915 code.

Personally I don't think enums should be used for defining bits, because
they are not enumerations. The bits are usually combined to come up with
values that are not part of the enum.

If we added the quirks to struct intel_dp_desc (and moved to drm
helpers) we could use enums for defining the bit shifts and add a
function to query for a specific quirk, say, drm_dp_has_quirk(struct
drm_dp_desc *desc, enum drm_dpcd_quirk quirk).


BR,
Jani.

>
>> +
>> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch);
>> +
>>  #endif /* _DRM_DP_HELPER_H_ */
>> -- 
>> 2.11.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

* Re: [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
  2017-05-12  7:57   ` Jani Nikula
@ 2017-05-14 11:00     ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2017-05-14 11:00 UTC (permalink / raw)
  To: Jani Nikula
  Cc: Wentland, Harry, intel-gfx, Adam Jackson, Dhinakaran Pandiyan,
	dri-devel

On Fri, May 12, 2017 at 9:57 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> Personally I don't think enums should be used for defining bits, because
> they are not enumerations. The bits are usually combined to come up with
> values that are not part of the enum.
>
> If we added the quirks to struct intel_dp_desc (and moved to drm
> helpers) we could use enums for defining the bit shifts and add a
> function to query for a specific quirk, say, drm_dp_has_quirk(struct
> drm_dp_desc *desc, enum drm_dpcd_quirk quirk).

It's a bit a gnu-ism afaik, but enums-as-bitfields is very much
standard. gdb even decodes it for you into the individual bits iirc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 2/2] drm/i915: Detect USB-C specific dongles before reducing M and N
  2017-05-11 16:12   ` Clint Taylor
@ 2017-05-15 20:24     ` Pandiyan, Dhinakaran
  0 siblings, 0 replies; 10+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-05-15 20:24 UTC (permalink / raw)
  To: Taylor, Clinton A
  Cc: Nikula, Jani, harry.wentland@amd.com,
	intel-gfx@lists.freedesktop.org, ajax@redhat.com,
	dri-devel@lists.freedesktop.org

On Thu, 2017-05-11 at 09:12 -0700, Clint Taylor wrote:
> 
> On 05/11/2017 02:57 AM, Jani Nikula wrote:
> > From: Clint Taylor <clinton.a.taylor@intel.com>
> >
> > The Analogix 7737 DP to HDMI converter requires reduced M and N values
> > when to operate correctly at HBR2. Detect this IC by its OUI value of
> > 0x0022B9 via the DPCD quirk list.
> >
> > v2 by Jani: Rebased on the DP quirk database
> >
> > Fixes: 9a86cda07af2 ("drm/i915/dp: reduce link M/N parameters")
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100755
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> 
> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
> >   drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++--------
> >   drivers/gpu/drm/i915/intel_dp.c      | 15 +++++++++++----
> >   drivers/gpu/drm/i915/intel_dp_mst.c  |  4 +++-
> >   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >   5 files changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ff3574a56812..d34412673d61 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -563,7 +563,8 @@ struct intel_link_m_n {
> >   
> >   void intel_link_compute_m_n(int bpp, int nlanes,
> >   			    int pixel_clock, int link_clock,
> > -			    struct intel_link_m_n *m_n);
> > +			    struct intel_link_m_n *m_n,
> > +			    bool reduce_m_n);
> >   
> >   /* Interface history:
> >    *
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index bf9020da8b80..46ac46ca976c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6116,7 +6116,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> >   	pipe_config->fdi_lanes = lane;
> >   
> >   	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> > -			       link_bw, &pipe_config->fdi_m_n);
> > +			       link_bw, &pipe_config->fdi_m_n, false);
> >   
> >   	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
> >   	if (ret == -EINVAL && pipe_config->pipe_bpp > 6*3) {
> > @@ -6292,7 +6292,8 @@ intel_reduce_m_n_ratio(uint32_t *num, uint32_t *den)
> >   }
> >   
> >   static void compute_m_n(unsigned int m, unsigned int n,
> > -			uint32_t *ret_m, uint32_t *ret_n)
> > +			uint32_t *ret_m, uint32_t *ret_n,
> > +			bool reduce_m_n)
> >   {
> >   	/*
> >   	 * Reduce M/N as much as possible without loss in precision. Several DP
> > @@ -6300,9 +6301,11 @@ static void compute_m_n(unsigned int m, unsigned int n,
> >   	 * values. The passed in values are more likely to have the least
> >   	 * significant bits zero than M after rounding below, so do this first.
> >   	 */
> > -	while ((m & 1) == 0 && (n & 1) == 0) {
> > -		m >>= 1;
> > -		n >>= 1;
> > +	if (reduce_m_n) {
> > +		while ((m & 1) == 0 && (n & 1) == 0) {
> > +			m >>= 1;
> > +			n >>= 1;
> > +		}
> >   	}
> >   
> >   	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
> > @@ -6313,16 +6316,19 @@ static void compute_m_n(unsigned int m, unsigned int n,
> >   void
> >   intel_link_compute_m_n(int bits_per_pixel, int nlanes,
> >   		       int pixel_clock, int link_clock,
> > -		       struct intel_link_m_n *m_n)
> > +		       struct intel_link_m_n *m_n,
> > +		       bool reduce_m_n)
> >   {
> >   	m_n->tu = 64;
> >   
> >   	compute_m_n(bits_per_pixel * pixel_clock,
> >   		    link_clock * nlanes * 8,
> > -		    &m_n->gmch_m, &m_n->gmch_n);
> > +		    &m_n->gmch_m, &m_n->gmch_n,
> > +		    reduce_m_n);
> >   
> >   	compute_m_n(pixel_clock, link_clock,
> > -		    &m_n->link_m, &m_n->link_n);
> > +		    &m_n->link_m, &m_n->link_n,
> > +		    reduce_m_n);
> >   }
> >   
> >   static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 4a6feb6a69bd..59f3dbb242a6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1568,13 +1568,17 @@ bool intel_dp_read_desc(struct intel_dp *intel_dp)
> >   	if (!__intel_dp_read_desc(intel_dp, desc))
> >   		return false;
> >   
> > +	intel_dp->quirks = drm_dp_get_quirks(desc->oui, sizeof(desc->oui),
> > +					     drm_dp_is_branch(intel_dp->dpcd));
> > +
> >   	dev_id_len = strnlen(desc->device_id, sizeof(desc->device_id));
> > -	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %*pE HW-rev %d.%d SW-rev %d.%d\n",
> > +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %*pE HW-rev %d.%d SW-rev %d.%d quirks 0x%04x\n",
> >   		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
> >   		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
> >   		      dev_id_len, desc->device_id,
> >   		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
> > -		      desc->sw_major_rev, desc->sw_minor_rev);
> > +		      desc->sw_major_rev, desc->sw_minor_rev,
> > +		      intel_dp->quirks);
> >   
> >   	return true;
> >   }
> > @@ -1658,6 +1662,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >   	int link_avail, link_clock;
> >   	int common_len;
> >   	uint8_t link_bw, rate_select;
> > +	bool reduce_m_n = intel_dp->quirks & DP_DPCD_QUIRK_LIMITED_M_N;
> >   
> >   	common_len = intel_dp_common_len_rate_limit(intel_dp,
> >   						    intel_dp->max_link_rate);
> > @@ -1790,7 +1795,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >   	intel_link_compute_m_n(bpp, lane_count,
> >   			       adjusted_mode->crtc_clock,
> >   			       pipe_config->port_clock,
> > -			       &pipe_config->dp_m_n);
> > +			       &pipe_config->dp_m_n,
> > +			       reduce_m_n);
> >   
> >   	if (intel_connector->panel.downclock_mode != NULL &&
> >   		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
> > @@ -1798,7 +1804,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >   			intel_link_compute_m_n(bpp, lane_count,
> >   				intel_connector->panel.downclock_mode->clock,
> >   				pipe_config->port_clock,
> > -				&pipe_config->dp_m2_n2);
> > +				&pipe_config->dp_m2_n2,
> > +				reduce_m_n);
> >   	}
> >   
> >   	/*
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 68c788eb0b95..763d490eeca9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -44,6 +44,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >   	int lane_count, slots;
> >   	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >   	int mst_pbn;
> > +	bool reduce_m_n = intel_dp->quirks & DP_DPCD_QUIRK_LIMITED_M_N;
> >   
> >   	pipe_config->has_pch_encoder = false;
> >   	bpp = 24;
> > @@ -79,7 +80,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >   	intel_link_compute_m_n(bpp, lane_count,
> >   			       adjusted_mode->crtc_clock,
> >   			       pipe_config->port_clock,
> > -			       &pipe_config->dp_m_n);
> > +			       &pipe_config->dp_m_n,
> > +			       reduce_m_n);
> >   
> >   	pipe_config->dp_m_n.tu = slots;
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bd500977b3fc..664156acff0c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -997,6 +997,8 @@ struct intel_dp {
> >   	int max_link_rate;
> >   	/* sink or branch descriptor */
> >   	struct intel_dp_desc desc;
> > +	/* Quirks as returned by drm_dp_get_quirks() */
> > +	u32 quirks;
> >   	struct drm_dp_aux aux;
> >   	enum intel_display_power_domain aux_power_domain;
> >   	uint8_t train_set[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] 10+ messages in thread

* Re: [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database
  2017-05-11  9:57 [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Jani Nikula
                   ` (3 preceding siblings ...)
  2017-05-11 18:17 ` [Intel-gfx] " Daniel Vetter
@ 2017-05-15 20:42 ` Pandiyan, Dhinakaran
  4 siblings, 0 replies; 10+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-05-15 20:42 UTC (permalink / raw)
  To: Nikula, Jani
  Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
	ajax@redhat.com, dri-devel@lists.freedesktop.org,
	harry.wentland@amd.com

On Thu, 2017-05-11 at 12:57 +0300, Jani Nikula wrote:
> Face the fact, there are Display Port sink and branch devices out there
> in the wild that don't follow the Display Port specifications, or they
> have bugs, or just otherwise require special treatment. Start a common
> quirk database the drivers can query based on OUI (with the option of
> expanding to device identification string and hardware/firmware
> revisions later). At least for now, we leave the workarounds for the
> drivers to implement as they see fit.
> 
> For starters, add a branch device that can't handle full 24-bit main
> link M and N attributes properly. Naturally, the workaround of reducing

Please see nit below.

> main link attributes for all devices ended up in regressions for other
> devices. So here we are.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 61 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |  7 +++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 3e5f52110ea1..58b2ced33151 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1208,3 +1208,64 @@ int drm_dp_stop_crc(struct drm_dp_aux *aux)
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_dp_stop_crc);
> +
> +/*
> + * Display Port sink and branch devices in the wild have a variety of bugs, try
> + * to collect them here. The quirks are shared, but it's up to the drivers to
> + * implement workarounds for them.
> + */
> +
> +/*
> + * FIXME: Add support for device identification and hardware/firmware revision.
> + */
> +struct dpcd_quirk {
> +	u8 oui[3];
> +	bool is_branch;
> +	u32 quirks;
> +};
> +
> +#define OUI(first, second, third) { (first), (second), (third) }
> +
> +static const struct dpcd_quirk dpcd_quirk_list[] = {
> +	/* Analogix 7737 needs reduced M and N at HBR2 link rates */
> +	{ OUI(0x00, 0x22, 0xb9), true, DP_DPCD_QUIRK_LIMITED_M_N },
> +};
> +
> +#undef OUI
> +
> +/**
> + * drm_dp_get_quirks() - get quirks for the sink/branch device
> + * @oui: pointer to len bytes of DPCD at 0x400 (sink) or 0x500 (branch)
> + * @len: 3-12 bytes of DPCD data
> + * @is_branch: true for branch devices, false for sink devices
> + *
> + * Get a bit mask of DPCD quirks for the sink/branch device. The quirk data is
> + * shared but it's up to the drivers to act on the data.
> + *
> + * For now, only the OUI (first three bytes) is used, but this may be extended
> + * to device identification string and hardware/firmware revisions later.
> + */
> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch)
> +{
> +	const struct dpcd_quirk *quirk;
> +	u32 quirks = 0;
> +	int i;
> +
> +	if (WARN_ON_ONCE(len < 3))
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(dpcd_quirk_list); i++) {
> +		quirk = &dpcd_quirk_list[i];
> +
> +		if (quirk->is_branch != is_branch)
> +			continue;
> +
> +		if (memcmp(quirk->oui, oui, 3) != 0)
> +			continue;
> +
> +		quirks |= quirk->quirks;
> +	}
> +
> +	return quirks;
> +}
> +EXPORT_SYMBOL(drm_dp_get_quirks);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index f7007e544f29..8abe9897fe59 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1079,4 +1079,11 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
>  int drm_dp_stop_crc(struct drm_dp_aux *aux);
>  
> +/* Display Port sink and branch device quirks. */
> +
> +/* The sink is limited to small link M and N values. */


I believe these are called mvid and nvid timestamps in the DP spec.
There's no reference link m and n outside i915 afaict. 

I'd prefer "small" to be quantified here or documentation that 24 bits
of link m/n doesn't work. It'll clarify how to use this quirk a little
bit.


With or without these
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

-DK

> +#define DP_DPCD_QUIRK_LIMITED_M_N			BIT(0)
> +
> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch);
> +
>  #endif /* _DRM_DP_HELPER_H_ */

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

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

end of thread, other threads:[~2017-05-15 20:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-11  9:57 [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Jani Nikula
2017-05-11  9:57 ` [PATCH 2/2] drm/i915: Detect USB-C specific dongles before reducing M and N Jani Nikula
2017-05-11 16:12   ` Clint Taylor
2017-05-15 20:24     ` Pandiyan, Dhinakaran
2017-05-11 10:44 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/dp: start a DPCD based DP sink/branch device quirk database Patchwork
2017-05-11 16:11 ` [PATCH 1/2] " Clint Taylor
2017-05-11 18:17 ` [Intel-gfx] " Daniel Vetter
2017-05-12  7:57   ` Jani Nikula
2017-05-14 11:00     ` Daniel Vetter
2017-05-15 20:42 ` Pandiyan, Dhinakaran

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.