[PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

Adaptive Sync is a VESA feature so add a DRM core helper to parse
the EDID's detailed descritors to obtain the adaptive sync monitor range.
Store this info as part fo drm_display_info so it can be used
across all drivers.
This part of the code is stripped out of amdgpu's function
amdgpu_dm_update_freesync_caps() to make it generic and be used
across all DRM drivers

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h | 25 +++++++++++++++++++
 include/drm/drm_edid.h      |  2 ++
 3 files changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 474ac04d5600..97dd1200773e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 	}
 }
 
+void drm_get_adaptive_sync_limits(struct drm_connector *connector,
+				  const struct edid *edid)
+{
+	struct drm_display_info *info = &connector->display_info;
+	const struct detailed_timing *timing;
+	const struct detailed_non_pixel *data;
+	const struct detailed_data_monitor_range *range;
+	int i;
+
+	/*
+	 * Restrict Adaptive Sync only for dp and edp
+	 */
+	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
+	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
+		return;
+
+	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
+		return;
+
+	for (i = 0; i < 4; i++) {
+		timing  = &edid->detailed_timings[i];
+		data    = &timing->data.other_data;
+		range   = &data->data.range;
+		/*
+		 * Check if monitor has continuous frequency mode
+		 */
+		if (data->type != EDID_DETAIL_MONITOR_RANGE)
+			continue;
+		/*
+		 * Check for flag range limits only. If flag == 1 then
+		 * no additional timing information provided.
+		 * Default GTF, GTF Secondary curve and CVT are not
+		 * supported
+		 */
+		if (range->flags != 1)
+			continue;
+
+		info->adaptive_sync.min_vfreq = range->min_vfreq;
+		info->adaptive_sync.max_vfreq = range->max_vfreq;
+		info->adaptive_sync.pixel_clock_mhz =
+			range->pixel_clock_mhz * 10;
+		break;
+	}
+}
+EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
+
 /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
  * all of the values which would have been set from EDID
  */
@@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
 	memset(&info->hdmi, 0, sizeof(info->hdmi));
 
 	info->non_desktop = 0;
+	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
 }
 
 u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
@@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
 
 	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
 
+	drm_get_adaptive_sync_limits(connector, edid);
+
 	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
 
 	if (edid->revision < 3)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 5f8c3389d46f..a27a84270d8d 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -254,6 +254,26 @@ enum drm_panel_orientation {
 	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
 };
 
+/**
+ * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
+ * &drm_display_info
+ *
+ * This struct is used to store a Panel's Adaptive Sync capabilities
+ * as parsed from EDID's detailed monitor range descriptor block.
+ *
+ * @min_vfreq: This is the min supported refresh rate in Hz from
+ *             EDID's detailed monitor range.
+ * @max_vfreq: This is the max supported refresh rate in Hz from
+ *             EDID's detailed monitor range
+ * @pixel_clock_mhz: This is the dotclock in MHz from
+ *                   EDID's detailed monitor range
+ */
+struct drm_adaptive_sync_info {
+	int min_vfreq;
+	int max_vfreq;
+	int pixel_clock_mhz;
+};
+
 /*
  * This is a consolidated colorimetry list supported by HDMI and
  * DP protocol standard. The respective connectors will register
@@ -465,6 +485,11 @@ struct drm_display_info {
 	 * @non_desktop: Non desktop display (HMD).
 	 */
 	bool non_desktop;
+
+	/**
+	 * @adaptive_sync: Adaptive Sync capabilities of the DP/eDP sink
+	 */
+	struct drm_adaptive_sync_info adaptive_sync;
 };
 
 int drm_display_info_set_bus_formats(struct drm_display_info *info,
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index f0b03d401c27..b9a230aa3e69 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -503,4 +503,6 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
 					   bool rb);
+void drm_get_adaptive_sync_limits(struct drm_connector *connector,
+				  const struct edid *edid);
 #endif /* __DRM_EDID_H__ */
-- 
2.19.1

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

[Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

Adaptive Sync is a VESA feature so add a DRM core helper to parse
the EDID's detailed descritors to obtain the adaptive sync monitor range.
Store this info as part fo drm_display_info so it can be used
across all drivers.
This part of the code is stripped out of amdgpu's function
amdgpu_dm_update_freesync_caps() to make it generic and be used
across all DRM drivers

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h | 25 +++++++++++++++++++
 include/drm/drm_edid.h      |  2 ++
 3 files changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 474ac04d5600..97dd1200773e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 	}
 }
 
+void drm_get_adaptive_sync_limits(struct drm_connector *connector,
+				  const struct edid *edid)
+{
+	struct drm_display_info *info = &connector->display_info;
+	const struct detailed_timing *timing;
+	const struct detailed_non_pixel *data;
+	const struct detailed_data_monitor_range *range;
+	int i;
+
+	/*
+	 * Restrict Adaptive Sync only for dp and edp
+	 */
+	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
+	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
+		return;
+
+	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
+		return;
+
+	for (i = 0; i < 4; i++) {
+		timing  = &edid->detailed_timings[i];
+		data    = &timing->data.other_data;
+		range   = &data->data.range;
+		/*
+		 * Check if monitor has continuous frequency mode
+		 */
+		if (data->type != EDID_DETAIL_MONITOR_RANGE)
+			continue;
+		/*
+		 * Check for flag range limits only. If flag == 1 then
+		 * no additional timing information provided.
+		 * Default GTF, GTF Secondary curve and CVT are not
+		 * supported
+		 */
+		if (range->flags != 1)
+			continue;
+
+		info->adaptive_sync.min_vfreq = range->min_vfreq;
+		info->adaptive_sync.max_vfreq = range->max_vfreq;
+		info->adaptive_sync.pixel_clock_mhz =
+			range->pixel_clock_mhz * 10;
+		break;
+	}
+}
+EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
+
 /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
  * all of the values which would have been set from EDID
  */
@@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
 	memset(&info->hdmi, 0, sizeof(info->hdmi));
 
 	info->non_desktop = 0;
+	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
 }
 
 u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
@@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
 
 	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
 
+	drm_get_adaptive_sync_limits(connector, edid);
+
 	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
 
 	if (edid->revision < 3)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 5f8c3389d46f..a27a84270d8d 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -254,6 +254,26 @@ enum drm_panel_orientation {
 	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
 };
 
+/**
+ * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
+ * &drm_display_info
+ *
+ * This struct is used to store a Panel's Adaptive Sync capabilities
+ * as parsed from EDID's detailed monitor range descriptor block.
+ *
+ * @min_vfreq: This is the min supported refresh rate in Hz from
+ *             EDID's detailed monitor range.
+ * @max_vfreq: This is the max supported refresh rate in Hz from
+ *             EDID's detailed monitor range
+ * @pixel_clock_mhz: This is the dotclock in MHz from
+ *                   EDID's detailed monitor range
+ */
+struct drm_adaptive_sync_info {
+	int min_vfreq;
+	int max_vfreq;
+	int pixel_clock_mhz;
+};
+
 /*
  * This is a consolidated colorimetry list supported by HDMI and
  * DP protocol standard. The respective connectors will register
@@ -465,6 +485,11 @@ struct drm_display_info {
 	 * @non_desktop: Non desktop display (HMD).
 	 */
 	bool non_desktop;
+
+	/**
+	 * @adaptive_sync: Adaptive Sync capabilities of the DP/eDP sink
+	 */
+	struct drm_adaptive_sync_info adaptive_sync;
 };
 
 int drm_display_info_set_bus_formats(struct drm_display_info *info,
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index f0b03d401c27..b9a230aa3e69 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -503,4 +503,6 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
 					   bool rb);
+void drm_get_adaptive_sync_limits(struct drm_connector *connector,
+				  const struct edid *edid);
 #endif /* __DRM_EDID_H__ */
-- 
2.19.1

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

✗ Fi.CI.BAT: failure for drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Patchwork]

== Series Details ==

Series: drm/dp: Add function to parse EDID descriptors for adaptive sync limits
URL   : https://patchwork.freedesktop.org/series/68488/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7167 -> Patchwork_14959
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/index.html

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_execlists:
    - fi-cfl-8109u:       [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-cfl-8109u/igt@i915_selftest@live_execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-cfl-8109u/igt@i915_selftest@live_execlists.html

  
Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap_gtt@basic-read-write-distinct:
    - fi-icl-u3:          [PASS][3] -> [DMESG-WARN][4] ([fdo#107724])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-icl-u3/igt@gem_mmap_gtt@basic-read-write-distinct.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-icl-u3/igt@gem_mmap_gtt@basic-read-write-distinct.html

  * igt@kms_chamelium@dp-edid-read:
    - fi-kbl-7500u:       [PASS][5] -> [WARN][6] ([fdo#109483])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-kbl-7500u/igt@kms_chamelium@dp-edid-read.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-kbl-7500u/igt@kms_chamelium@dp-edid-read.html

  
#### Possible fixes ####

  * igt@gem_sync@basic-store-all:
    - {fi-tgl-u}:         [INCOMPLETE][7] ([fdo#111880]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-tgl-u/igt@gem_sync@basic-store-all.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-tgl-u/igt@gem_sync@basic-store-all.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-skl-6600u:       [FAIL][9] ([fdo#107707]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html

  * {igt@i915_selftest@live_gt_heartbeat}:
    - {fi-icl-dsi}:       [DMESG-FAIL][11] -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-icl-dsi/igt@i915_selftest@live_gt_heartbeat.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-icl-dsi/igt@i915_selftest@live_gt_heartbeat.html

  * igt@kms_busy@basic-flip-a:
    - {fi-tgl-u2}:        [DMESG-WARN][13] ([fdo#111600]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-tgl-u2/igt@kms_busy@basic-flip-a.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-tgl-u2/igt@kms_busy@basic-flip-a.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][15] ([fdo#111407]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@prime_vgem@basic-fence-wait-default:
    - fi-icl-u3:          [DMESG-WARN][17] ([fdo#107724]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-icl-u3/igt@prime_vgem@basic-fence-wait-default.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-icl-u3/igt@prime_vgem@basic-fence-wait-default.html

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

  [fdo#107707]: https://bugs.freedesktop.org/show_bug.cgi?id=107707
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [fdo#111600]: https://bugs.freedesktop.org/show_bug.cgi?id=111600
  [fdo#111747]: https://bugs.freedesktop.org/show_bug.cgi?id=111747
  [fdo#111880]: https://bugs.freedesktop.org/show_bug.cgi?id=111880


Participating hosts (52 -> 45)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7167 -> Patchwork_14959

  CI-20190529: 20190529
  CI_DRM_7167: a62b1c4e7739c6777d51e7b2d66406b935131451 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5236: 8153b95b53bdef26d2c3e318197d174e982b4265 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14959: daf43f3a6b3cb9e60c17fc0ad2e270ae082792eb @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

daf43f3a6b3c drm/dp: Add function to parse EDID descriptors for adaptive sync limits

== Logs ==

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Patchwork]

== Series Details ==

Series: drm/dp: Add function to parse EDID descriptors for adaptive sync limits
URL   : https://patchwork.freedesktop.org/series/68488/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7167 -> Patchwork_14959
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/index.html

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_execlists:
    - fi-cfl-8109u:       [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-cfl-8109u/igt@i915_selftest@live_execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-cfl-8109u/igt@i915_selftest@live_execlists.html

  
Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap_gtt@basic-read-write-distinct:
    - fi-icl-u3:          [PASS][3] -> [DMESG-WARN][4] ([fdo#107724])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-icl-u3/igt@gem_mmap_gtt@basic-read-write-distinct.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-icl-u3/igt@gem_mmap_gtt@basic-read-write-distinct.html

  * igt@kms_chamelium@dp-edid-read:
    - fi-kbl-7500u:       [PASS][5] -> [WARN][6] ([fdo#109483])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-kbl-7500u/igt@kms_chamelium@dp-edid-read.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-kbl-7500u/igt@kms_chamelium@dp-edid-read.html

  
#### Possible fixes ####

  * igt@gem_sync@basic-store-all:
    - {fi-tgl-u}:         [INCOMPLETE][7] ([fdo#111880]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-tgl-u/igt@gem_sync@basic-store-all.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-tgl-u/igt@gem_sync@basic-store-all.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-skl-6600u:       [FAIL][9] ([fdo#107707]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html

  * {igt@i915_selftest@live_gt_heartbeat}:
    - {fi-icl-dsi}:       [DMESG-FAIL][11] -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-icl-dsi/igt@i915_selftest@live_gt_heartbeat.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-icl-dsi/igt@i915_selftest@live_gt_heartbeat.html

  * igt@kms_busy@basic-flip-a:
    - {fi-tgl-u2}:        [DMESG-WARN][13] ([fdo#111600]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-tgl-u2/igt@kms_busy@basic-flip-a.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-tgl-u2/igt@kms_busy@basic-flip-a.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][15] ([fdo#111407]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@prime_vgem@basic-fence-wait-default:
    - fi-icl-u3:          [DMESG-WARN][17] ([fdo#107724]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7167/fi-icl-u3/igt@prime_vgem@basic-fence-wait-default.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14959/fi-icl-u3/igt@prime_vgem@basic-fence-wait-default.html

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

  [fdo#107707]: https://bugs.freedesktop.org/show_bug.cgi?id=107707
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [fdo#111600]: https://bugs.freedesktop.org/show_bug.cgi?id=111600
  [fdo#111747]: https://bugs.freedesktop.org/show_bug.cgi?id=111747
  [fdo#111880]: https://bugs.freedesktop.org/show_bug.cgi?id=111880


Participating hosts (52 -> 45)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7167 -> Patchwork_14959

  CI-20190529: 20190529
  CI_DRM_7167: a62b1c4e7739c6777d51e7b2d66406b935131451 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5236: 8153b95b53bdef26d2c3e318197d174e982b4265 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14959: daf43f3a6b3cb9e60c17fc0ad2e270ae082792eb @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

daf43f3a6b3c drm/dp: Add function to parse EDID descriptors for adaptive sync limits

== Logs ==

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

Re: [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4695 bytes --]

On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> Adaptive Sync is a VESA feature so add a DRM core helper to parse
> the EDID's detailed descritors to obtain the adaptive sync monitor range.
> Store this info as part fo drm_display_info so it can be used
> across all drivers.
> This part of the code is stripped out of amdgpu's function
> amdgpu_dm_update_freesync_caps() to make it generic and be used
> across all DRM drivers
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h | 25 +++++++++++++++++++
>  include/drm/drm_edid.h      |  2 ++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 474ac04d5600..97dd1200773e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  	}
>  }
>  
> +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> +				  const struct edid *edid)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	const struct detailed_timing *timing;
> +	const struct detailed_non_pixel *data;
> +	const struct detailed_data_monitor_range *range;
> +	int i;

This can be unsigned int.

> +
> +	/*
> +	 * Restrict Adaptive Sync only for dp and edp
> +	 */
> +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> +		return;
> +
> +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> +		return;
> +
> +	for (i = 0; i < 4; i++) {
> +		timing  = &edid->detailed_timings[i];
> +		data    = &timing->data.other_data;
> +		range   = &data->data.range;
> +		/*
> +		 * Check if monitor has continuous frequency mode
> +		 */
> +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> +			continue;
> +		/*
> +		 * Check for flag range limits only. If flag == 1 then
> +		 * no additional timing information provided.
> +		 * Default GTF, GTF Secondary curve and CVT are not
> +		 * supported
> +		 */
> +		if (range->flags != 1)
> +			continue;
> +
> +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> +		info->adaptive_sync.pixel_clock_mhz =
> +			range->pixel_clock_mhz * 10;
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> +
>  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
>   * all of the values which would have been set from EDID
>   */
> @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
>  	memset(&info->hdmi, 0, sizeof(info->hdmi));
>  
>  	info->non_desktop = 0;
> +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
>  }
>  
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>  
>  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
>  
> +	drm_get_adaptive_sync_limits(connector, edid);
> +
>  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
>  
>  	if (edid->revision < 3)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5f8c3389d46f..a27a84270d8d 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -254,6 +254,26 @@ enum drm_panel_orientation {
>  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>  };
>  
> +/**
> + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> + * &drm_display_info
> + *
> + * This struct is used to store a Panel's Adaptive Sync capabilities
> + * as parsed from EDID's detailed monitor range descriptor block.
> + *
> + * @min_vfreq: This is the min supported refresh rate in Hz from
> + *             EDID's detailed monitor range.
> + * @max_vfreq: This is the max supported refresh rate in Hz from
> + *             EDID's detailed monitor range
> + * @pixel_clock_mhz: This is the dotclock in MHz from
> + *                   EDID's detailed monitor range
> + */
> +struct drm_adaptive_sync_info {
> +	int min_vfreq;
> +	int max_vfreq;
> +	int pixel_clock_mhz;

Any reason why these can't be unsigned? Also, does it perhaps make sense
to store the pixel clock as kHz like we do everywhere else?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4695 bytes --]

On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> Adaptive Sync is a VESA feature so add a DRM core helper to parse
> the EDID's detailed descritors to obtain the adaptive sync monitor range.
> Store this info as part fo drm_display_info so it can be used
> across all drivers.
> This part of the code is stripped out of amdgpu's function
> amdgpu_dm_update_freesync_caps() to make it generic and be used
> across all DRM drivers
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h | 25 +++++++++++++++++++
>  include/drm/drm_edid.h      |  2 ++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 474ac04d5600..97dd1200773e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  	}
>  }
>  
> +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> +				  const struct edid *edid)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	const struct detailed_timing *timing;
> +	const struct detailed_non_pixel *data;
> +	const struct detailed_data_monitor_range *range;
> +	int i;

This can be unsigned int.

> +
> +	/*
> +	 * Restrict Adaptive Sync only for dp and edp
> +	 */
> +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> +		return;
> +
> +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> +		return;
> +
> +	for (i = 0; i < 4; i++) {
> +		timing  = &edid->detailed_timings[i];
> +		data    = &timing->data.other_data;
> +		range   = &data->data.range;
> +		/*
> +		 * Check if monitor has continuous frequency mode
> +		 */
> +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> +			continue;
> +		/*
> +		 * Check for flag range limits only. If flag == 1 then
> +		 * no additional timing information provided.
> +		 * Default GTF, GTF Secondary curve and CVT are not
> +		 * supported
> +		 */
> +		if (range->flags != 1)
> +			continue;
> +
> +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> +		info->adaptive_sync.pixel_clock_mhz =
> +			range->pixel_clock_mhz * 10;
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> +
>  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
>   * all of the values which would have been set from EDID
>   */
> @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
>  	memset(&info->hdmi, 0, sizeof(info->hdmi));
>  
>  	info->non_desktop = 0;
> +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
>  }
>  
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>  
>  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
>  
> +	drm_get_adaptive_sync_limits(connector, edid);
> +
>  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
>  
>  	if (edid->revision < 3)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5f8c3389d46f..a27a84270d8d 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -254,6 +254,26 @@ enum drm_panel_orientation {
>  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>  };
>  
> +/**
> + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> + * &drm_display_info
> + *
> + * This struct is used to store a Panel's Adaptive Sync capabilities
> + * as parsed from EDID's detailed monitor range descriptor block.
> + *
> + * @min_vfreq: This is the min supported refresh rate in Hz from
> + *             EDID's detailed monitor range.
> + * @max_vfreq: This is the max supported refresh rate in Hz from
> + *             EDID's detailed monitor range
> + * @pixel_clock_mhz: This is the dotclock in MHz from
> + *                   EDID's detailed monitor range
> + */
> +struct drm_adaptive_sync_info {
> +	int min_vfreq;
> +	int max_vfreq;
> +	int pixel_clock_mhz;

Any reason why these can't be unsigned? Also, does it perhaps make sense
to store the pixel clock as kHz like we do everywhere else?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4695 bytes --]

On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> Adaptive Sync is a VESA feature so add a DRM core helper to parse
> the EDID's detailed descritors to obtain the adaptive sync monitor range.
> Store this info as part fo drm_display_info so it can be used
> across all drivers.
> This part of the code is stripped out of amdgpu's function
> amdgpu_dm_update_freesync_caps() to make it generic and be used
> across all DRM drivers
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h | 25 +++++++++++++++++++
>  include/drm/drm_edid.h      |  2 ++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 474ac04d5600..97dd1200773e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  	}
>  }
>  
> +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> +				  const struct edid *edid)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	const struct detailed_timing *timing;
> +	const struct detailed_non_pixel *data;
> +	const struct detailed_data_monitor_range *range;
> +	int i;

This can be unsigned int.

> +
> +	/*
> +	 * Restrict Adaptive Sync only for dp and edp
> +	 */
> +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> +		return;
> +
> +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> +		return;
> +
> +	for (i = 0; i < 4; i++) {
> +		timing  = &edid->detailed_timings[i];
> +		data    = &timing->data.other_data;
> +		range   = &data->data.range;
> +		/*
> +		 * Check if monitor has continuous frequency mode
> +		 */
> +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> +			continue;
> +		/*
> +		 * Check for flag range limits only. If flag == 1 then
> +		 * no additional timing information provided.
> +		 * Default GTF, GTF Secondary curve and CVT are not
> +		 * supported
> +		 */
> +		if (range->flags != 1)
> +			continue;
> +
> +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> +		info->adaptive_sync.pixel_clock_mhz =
> +			range->pixel_clock_mhz * 10;
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> +
>  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
>   * all of the values which would have been set from EDID
>   */
> @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
>  	memset(&info->hdmi, 0, sizeof(info->hdmi));
>  
>  	info->non_desktop = 0;
> +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
>  }
>  
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>  
>  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
>  
> +	drm_get_adaptive_sync_limits(connector, edid);
> +
>  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
>  
>  	if (edid->revision < 3)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5f8c3389d46f..a27a84270d8d 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -254,6 +254,26 @@ enum drm_panel_orientation {
>  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>  };
>  
> +/**
> + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> + * &drm_display_info
> + *
> + * This struct is used to store a Panel's Adaptive Sync capabilities
> + * as parsed from EDID's detailed monitor range descriptor block.
> + *
> + * @min_vfreq: This is the min supported refresh rate in Hz from
> + *             EDID's detailed monitor range.
> + * @max_vfreq: This is the max supported refresh rate in Hz from
> + *             EDID's detailed monitor range
> + * @pixel_clock_mhz: This is the dotclock in MHz from
> + *                   EDID's detailed monitor range
> + */
> +struct drm_adaptive_sync_info {
> +	int min_vfreq;
> +	int max_vfreq;
> +	int pixel_clock_mhz;

Any reason why these can't be unsigned? Also, does it perhaps make sense
to store the pixel clock as kHz like we do everywhere else?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Ville Syrjälä]

On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> 
> This can be unsigned int.

Please no. A loop iterator called 'i' should always be a normal signed int.

> 
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> 
> Any reason why these can't be unsigned? Also, does it perhaps make sense
> to store the pixel clock as kHz like we do everywhere else?

Aye, all typical clock frequencies should be in khz.

Also the vfreqs are only u8 in the EDID, so can be u8 here as well.

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Ville Syrjälä]

On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> 
> This can be unsigned int.

Please no. A loop iterator called 'i' should always be a normal signed int.

> 
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> 
> Any reason why these can't be unsigned? Also, does it perhaps make sense
> to store the pixel clock as kHz like we do everywhere else?

Aye, all typical clock frequencies should be in khz.

Also the vfreqs are only u8 in the EDID, so can be u8 here as well.

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Ville Syrjälä]

On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> 
> This can be unsigned int.

Please no. A loop iterator called 'i' should always be a normal signed int.

> 
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> 
> Any reason why these can't be unsigned? Also, does it perhaps make sense
> to store the pixel clock as kHz like we do everywhere else?

Aye, all typical clock frequencies should be in khz.

Also the vfreqs are only u8 in the EDID, so can be u8 here as well.

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

Re: [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5933 bytes --]

On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > Store this info as part fo drm_display_info so it can be used
> > > across all drivers.
> > > This part of the code is stripped out of amdgpu's function
> > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > across all DRM drivers
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > >  include/drm/drm_edid.h      |  2 ++
> > >  3 files changed, 76 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 474ac04d5600..97dd1200773e 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > >  	}
> > >  }
> > >  
> > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > +				  const struct edid *edid)
> > > +{
> > > +	struct drm_display_info *info = &connector->display_info;
> > > +	const struct detailed_timing *timing;
> > > +	const struct detailed_non_pixel *data;
> > > +	const struct detailed_data_monitor_range *range;
> > > +	int i;
> > 
> > This can be unsigned int.
> 
> Please no. A loop iterator called 'i' should always be a normal signed int.

What? Where's that rule written down? In my experience it's always
better to use as restrictive a type as possible. It's really annoying
when GCC suddenly starts complaining about comparison between signed and
unsigned. So if a variable can never contain a signed value, why risk
the ambiguity? The value goes from 0 to 4, the sign bit is useless.

> > > +	/*
> > > +	 * Restrict Adaptive Sync only for dp and edp
> > > +	 */
> > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > +		return;
> > > +
> > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > +		return;
> > > +
> > > +	for (i = 0; i < 4; i++) {
> > > +		timing  = &edid->detailed_timings[i];
> > > +		data    = &timing->data.other_data;
> > > +		range   = &data->data.range;
> > > +		/*
> > > +		 * Check if monitor has continuous frequency mode
> > > +		 */
> > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > +			continue;
> > > +		/*
> > > +		 * Check for flag range limits only. If flag == 1 then
> > > +		 * no additional timing information provided.
> > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > +		 * supported
> > > +		 */
> > > +		if (range->flags != 1)
> > > +			continue;
> > > +
> > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > +		info->adaptive_sync.pixel_clock_mhz =
> > > +			range->pixel_clock_mhz * 10;
> > > +		break;
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > +
> > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > >   * all of the values which would have been set from EDID
> > >   */
> > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > >  
> > >  	info->non_desktop = 0;
> > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > >  }
> > >  
> > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > >  
> > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > >  
> > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > +
> > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > >  
> > >  	if (edid->revision < 3)
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 5f8c3389d46f..a27a84270d8d 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > >  };
> > >  
> > > +/**
> > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > + * &drm_display_info
> > > + *
> > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > + *
> > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > + *             EDID's detailed monitor range.
> > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > + *             EDID's detailed monitor range
> > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > + *                   EDID's detailed monitor range
> > > + */
> > > +struct drm_adaptive_sync_info {
> > > +	int min_vfreq;
> > > +	int max_vfreq;
> > > +	int pixel_clock_mhz;
> > 
> > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > to store the pixel clock as kHz like we do everywhere else?
> 
> Aye, all typical clock frequencies should be in khz.
> 
> Also the vfreqs are only u8 in the EDID, so can be u8 here as well.

Not if you store them in kHz, they can't.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5933 bytes --]

On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > Store this info as part fo drm_display_info so it can be used
> > > across all drivers.
> > > This part of the code is stripped out of amdgpu's function
> > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > across all DRM drivers
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > >  include/drm/drm_edid.h      |  2 ++
> > >  3 files changed, 76 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 474ac04d5600..97dd1200773e 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > >  	}
> > >  }
> > >  
> > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > +				  const struct edid *edid)
> > > +{
> > > +	struct drm_display_info *info = &connector->display_info;
> > > +	const struct detailed_timing *timing;
> > > +	const struct detailed_non_pixel *data;
> > > +	const struct detailed_data_monitor_range *range;
> > > +	int i;
> > 
> > This can be unsigned int.
> 
> Please no. A loop iterator called 'i' should always be a normal signed int.

What? Where's that rule written down? In my experience it's always
better to use as restrictive a type as possible. It's really annoying
when GCC suddenly starts complaining about comparison between signed and
unsigned. So if a variable can never contain a signed value, why risk
the ambiguity? The value goes from 0 to 4, the sign bit is useless.

> > > +	/*
> > > +	 * Restrict Adaptive Sync only for dp and edp
> > > +	 */
> > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > +		return;
> > > +
> > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > +		return;
> > > +
> > > +	for (i = 0; i < 4; i++) {
> > > +		timing  = &edid->detailed_timings[i];
> > > +		data    = &timing->data.other_data;
> > > +		range   = &data->data.range;
> > > +		/*
> > > +		 * Check if monitor has continuous frequency mode
> > > +		 */
> > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > +			continue;
> > > +		/*
> > > +		 * Check for flag range limits only. If flag == 1 then
> > > +		 * no additional timing information provided.
> > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > +		 * supported
> > > +		 */
> > > +		if (range->flags != 1)
> > > +			continue;
> > > +
> > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > +		info->adaptive_sync.pixel_clock_mhz =
> > > +			range->pixel_clock_mhz * 10;
> > > +		break;
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > +
> > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > >   * all of the values which would have been set from EDID
> > >   */
> > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > >  
> > >  	info->non_desktop = 0;
> > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > >  }
> > >  
> > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > >  
> > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > >  
> > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > +
> > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > >  
> > >  	if (edid->revision < 3)
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 5f8c3389d46f..a27a84270d8d 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > >  };
> > >  
> > > +/**
> > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > + * &drm_display_info
> > > + *
> > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > + *
> > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > + *             EDID's detailed monitor range.
> > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > + *             EDID's detailed monitor range
> > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > + *                   EDID's detailed monitor range
> > > + */
> > > +struct drm_adaptive_sync_info {
> > > +	int min_vfreq;
> > > +	int max_vfreq;
> > > +	int pixel_clock_mhz;
> > 
> > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > to store the pixel clock as kHz like we do everywhere else?
> 
> Aye, all typical clock frequencies should be in khz.
> 
> Also the vfreqs are only u8 in the EDID, so can be u8 here as well.

Not if you store them in kHz, they can't.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5933 bytes --]

On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > Store this info as part fo drm_display_info so it can be used
> > > across all drivers.
> > > This part of the code is stripped out of amdgpu's function
> > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > across all DRM drivers
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > >  include/drm/drm_edid.h      |  2 ++
> > >  3 files changed, 76 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 474ac04d5600..97dd1200773e 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > >  	}
> > >  }
> > >  
> > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > +				  const struct edid *edid)
> > > +{
> > > +	struct drm_display_info *info = &connector->display_info;
> > > +	const struct detailed_timing *timing;
> > > +	const struct detailed_non_pixel *data;
> > > +	const struct detailed_data_monitor_range *range;
> > > +	int i;
> > 
> > This can be unsigned int.
> 
> Please no. A loop iterator called 'i' should always be a normal signed int.

What? Where's that rule written down? In my experience it's always
better to use as restrictive a type as possible. It's really annoying
when GCC suddenly starts complaining about comparison between signed and
unsigned. So if a variable can never contain a signed value, why risk
the ambiguity? The value goes from 0 to 4, the sign bit is useless.

> > > +	/*
> > > +	 * Restrict Adaptive Sync only for dp and edp
> > > +	 */
> > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > +		return;
> > > +
> > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > +		return;
> > > +
> > > +	for (i = 0; i < 4; i++) {
> > > +		timing  = &edid->detailed_timings[i];
> > > +		data    = &timing->data.other_data;
> > > +		range   = &data->data.range;
> > > +		/*
> > > +		 * Check if monitor has continuous frequency mode
> > > +		 */
> > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > +			continue;
> > > +		/*
> > > +		 * Check for flag range limits only. If flag == 1 then
> > > +		 * no additional timing information provided.
> > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > +		 * supported
> > > +		 */
> > > +		if (range->flags != 1)
> > > +			continue;
> > > +
> > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > +		info->adaptive_sync.pixel_clock_mhz =
> > > +			range->pixel_clock_mhz * 10;
> > > +		break;
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > +
> > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > >   * all of the values which would have been set from EDID
> > >   */
> > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > >  
> > >  	info->non_desktop = 0;
> > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > >  }
> > >  
> > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > >  
> > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > >  
> > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > +
> > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > >  
> > >  	if (edid->revision < 3)
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 5f8c3389d46f..a27a84270d8d 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > >  };
> > >  
> > > +/**
> > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > + * &drm_display_info
> > > + *
> > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > + *
> > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > + *             EDID's detailed monitor range.
> > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > + *             EDID's detailed monitor range
> > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > + *                   EDID's detailed monitor range
> > > + */
> > > +struct drm_adaptive_sync_info {
> > > +	int min_vfreq;
> > > +	int max_vfreq;
> > > +	int pixel_clock_mhz;
> > 
> > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > to store the pixel clock as kHz like we do everywhere else?
> 
> Aye, all typical clock frequencies should be in khz.
> 
> Also the vfreqs are only u8 in the EDID, so can be u8 here as well.

Not if you store them in kHz, they can't.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Ville Syrjälä]

On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > > Store this info as part fo drm_display_info so it can be used
> > > > across all drivers.
> > > > This part of the code is stripped out of amdgpu's function
> > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > across all DRM drivers
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > > >  include/drm/drm_edid.h      |  2 ++
> > > >  3 files changed, 76 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index 474ac04d5600..97dd1200773e 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > > >  	}
> > > >  }
> > > >  
> > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > +				  const struct edid *edid)
> > > > +{
> > > > +	struct drm_display_info *info = &connector->display_info;
> > > > +	const struct detailed_timing *timing;
> > > > +	const struct detailed_non_pixel *data;
> > > > +	const struct detailed_data_monitor_range *range;
> > > > +	int i;
> > > 
> > > This can be unsigned int.
> > 
> > Please no. A loop iterator called 'i' should always be a normal signed int.
> 
> What? Where's that rule written down? In my experience it's always
> better to use as restrictive a type as possible. It's really annoying
> when GCC suddenly starts complaining about comparison between signed and
> unsigned. So if a variable can never contain a signed value, why risk
> the ambiguity? The value goes from 0 to 4, the sign bit is useless.

Dunno if it's really written down anywhere. It's just something
experience has taught. IIRC there's also a rant from Linus about this
somewhere. Hm, can't find that one right now, but Andrew Morton also
seems to agree: https://lwn.net/Articles/309279/
Ah, here is one Linus rant about unsigned array indexes:
https://yarchive.net/comp/linux/gcc.html

My opinion: unsigned is an very *dangerous* keyword in C that demands
your respect. You should never use it without thinking first what the
ramifications are. You always have to have the promotion rules clear 
in you mind when you do any kind of arithmetic with >= unsigned int
type. And common idioms such as 'int i' should be respected so as to
not cause unexpected hair loss to other developers when they decide
to make the loop iterate backwards.

> 
> > > > +	/*
> > > > +	 * Restrict Adaptive Sync only for dp and edp
> > > > +	 */
> > > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > +		return;
> > > > +
> > > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > > +		return;
> > > > +
> > > > +	for (i = 0; i < 4; i++) {
> > > > +		timing  = &edid->detailed_timings[i];
> > > > +		data    = &timing->data.other_data;
> > > > +		range   = &data->data.range;
> > > > +		/*
> > > > +		 * Check if monitor has continuous frequency mode
> > > > +		 */
> > > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > > +			continue;
> > > > +		/*
> > > > +		 * Check for flag range limits only. If flag == 1 then
> > > > +		 * no additional timing information provided.
> > > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > > +		 * supported
> > > > +		 */
> > > > +		if (range->flags != 1)
> > > > +			continue;
> > > > +
> > > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > > +		info->adaptive_sync.pixel_clock_mhz =
> > > > +			range->pixel_clock_mhz * 10;
> > > > +		break;
> > > > +	}
> > > > +}
> > > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > > +
> > > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > > >   * all of the values which would have been set from EDID
> > > >   */
> > > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > > >  
> > > >  	info->non_desktop = 0;
> > > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > >  }
> > > >  
> > > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > > >  
> > > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > > >  
> > > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > > +
> > > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > > >  
> > > >  	if (edid->revision < 3)
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index 5f8c3389d46f..a27a84270d8d 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > > >  };
> > > >  
> > > > +/**
> > > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > > + * &drm_display_info
> > > > + *
> > > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > > + *
> > > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > > + *             EDID's detailed monitor range.
> > > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > > + *             EDID's detailed monitor range
> > > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > > + *                   EDID's detailed monitor range
> > > > + */
> > > > +struct drm_adaptive_sync_info {
> > > > +	int min_vfreq;
> > > > +	int max_vfreq;
> > > > +	int pixel_clock_mhz;
> > > 
> > > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > > to store the pixel clock as kHz like we do everywhere else?
> > 
> > Aye, all typical clock frequencies should be in khz.
> > 
> > Also the vfreqs are only u8 in the EDID, so can be u8 here as well.
> 
> Not if you store them in kHz, they can't.

IMO those can stay in Hz. That's what drm_mode_vrefresh() gives you as well.

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Ville Syrjälä]

On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > > Store this info as part fo drm_display_info so it can be used
> > > > across all drivers.
> > > > This part of the code is stripped out of amdgpu's function
> > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > across all DRM drivers
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > > >  include/drm/drm_edid.h      |  2 ++
> > > >  3 files changed, 76 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index 474ac04d5600..97dd1200773e 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > > >  	}
> > > >  }
> > > >  
> > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > +				  const struct edid *edid)
> > > > +{
> > > > +	struct drm_display_info *info = &connector->display_info;
> > > > +	const struct detailed_timing *timing;
> > > > +	const struct detailed_non_pixel *data;
> > > > +	const struct detailed_data_monitor_range *range;
> > > > +	int i;
> > > 
> > > This can be unsigned int.
> > 
> > Please no. A loop iterator called 'i' should always be a normal signed int.
> 
> What? Where's that rule written down? In my experience it's always
> better to use as restrictive a type as possible. It's really annoying
> when GCC suddenly starts complaining about comparison between signed and
> unsigned. So if a variable can never contain a signed value, why risk
> the ambiguity? The value goes from 0 to 4, the sign bit is useless.

Dunno if it's really written down anywhere. It's just something
experience has taught. IIRC there's also a rant from Linus about this
somewhere. Hm, can't find that one right now, but Andrew Morton also
seems to agree: https://lwn.net/Articles/309279/
Ah, here is one Linus rant about unsigned array indexes:
https://yarchive.net/comp/linux/gcc.html

My opinion: unsigned is an very *dangerous* keyword in C that demands
your respect. You should never use it without thinking first what the
ramifications are. You always have to have the promotion rules clear 
in you mind when you do any kind of arithmetic with >= unsigned int
type. And common idioms such as 'int i' should be respected so as to
not cause unexpected hair loss to other developers when they decide
to make the loop iterate backwards.

> 
> > > > +	/*
> > > > +	 * Restrict Adaptive Sync only for dp and edp
> > > > +	 */
> > > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > +		return;
> > > > +
> > > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > > +		return;
> > > > +
> > > > +	for (i = 0; i < 4; i++) {
> > > > +		timing  = &edid->detailed_timings[i];
> > > > +		data    = &timing->data.other_data;
> > > > +		range   = &data->data.range;
> > > > +		/*
> > > > +		 * Check if monitor has continuous frequency mode
> > > > +		 */
> > > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > > +			continue;
> > > > +		/*
> > > > +		 * Check for flag range limits only. If flag == 1 then
> > > > +		 * no additional timing information provided.
> > > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > > +		 * supported
> > > > +		 */
> > > > +		if (range->flags != 1)
> > > > +			continue;
> > > > +
> > > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > > +		info->adaptive_sync.pixel_clock_mhz =
> > > > +			range->pixel_clock_mhz * 10;
> > > > +		break;
> > > > +	}
> > > > +}
> > > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > > +
> > > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > > >   * all of the values which would have been set from EDID
> > > >   */
> > > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > > >  
> > > >  	info->non_desktop = 0;
> > > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > >  }
> > > >  
> > > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > > >  
> > > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > > >  
> > > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > > +
> > > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > > >  
> > > >  	if (edid->revision < 3)
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index 5f8c3389d46f..a27a84270d8d 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > > >  };
> > > >  
> > > > +/**
> > > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > > + * &drm_display_info
> > > > + *
> > > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > > + *
> > > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > > + *             EDID's detailed monitor range.
> > > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > > + *             EDID's detailed monitor range
> > > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > > + *                   EDID's detailed monitor range
> > > > + */
> > > > +struct drm_adaptive_sync_info {
> > > > +	int min_vfreq;
> > > > +	int max_vfreq;
> > > > +	int pixel_clock_mhz;
> > > 
> > > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > > to store the pixel clock as kHz like we do everywhere else?
> > 
> > Aye, all typical clock frequencies should be in khz.
> > 
> > Also the vfreqs are only u8 in the EDID, so can be u8 here as well.
> 
> Not if you store them in kHz, they can't.

IMO those can stay in Hz. That's what drm_mode_vrefresh() gives you as well.

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Ville Syrjälä]

On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > > Store this info as part fo drm_display_info so it can be used
> > > > across all drivers.
> > > > This part of the code is stripped out of amdgpu's function
> > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > across all DRM drivers
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > > >  include/drm/drm_edid.h      |  2 ++
> > > >  3 files changed, 76 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index 474ac04d5600..97dd1200773e 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > > >  	}
> > > >  }
> > > >  
> > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > +				  const struct edid *edid)
> > > > +{
> > > > +	struct drm_display_info *info = &connector->display_info;
> > > > +	const struct detailed_timing *timing;
> > > > +	const struct detailed_non_pixel *data;
> > > > +	const struct detailed_data_monitor_range *range;
> > > > +	int i;
> > > 
> > > This can be unsigned int.
> > 
> > Please no. A loop iterator called 'i' should always be a normal signed int.
> 
> What? Where's that rule written down? In my experience it's always
> better to use as restrictive a type as possible. It's really annoying
> when GCC suddenly starts complaining about comparison between signed and
> unsigned. So if a variable can never contain a signed value, why risk
> the ambiguity? The value goes from 0 to 4, the sign bit is useless.

Dunno if it's really written down anywhere. It's just something
experience has taught. IIRC there's also a rant from Linus about this
somewhere. Hm, can't find that one right now, but Andrew Morton also
seems to agree: https://lwn.net/Articles/309279/
Ah, here is one Linus rant about unsigned array indexes:
https://yarchive.net/comp/linux/gcc.html

My opinion: unsigned is an very *dangerous* keyword in C that demands
your respect. You should never use it without thinking first what the
ramifications are. You always have to have the promotion rules clear 
in you mind when you do any kind of arithmetic with >= unsigned int
type. And common idioms such as 'int i' should be respected so as to
not cause unexpected hair loss to other developers when they decide
to make the loop iterate backwards.

> 
> > > > +	/*
> > > > +	 * Restrict Adaptive Sync only for dp and edp
> > > > +	 */
> > > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > +		return;
> > > > +
> > > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > > +		return;
> > > > +
> > > > +	for (i = 0; i < 4; i++) {
> > > > +		timing  = &edid->detailed_timings[i];
> > > > +		data    = &timing->data.other_data;
> > > > +		range   = &data->data.range;
> > > > +		/*
> > > > +		 * Check if monitor has continuous frequency mode
> > > > +		 */
> > > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > > +			continue;
> > > > +		/*
> > > > +		 * Check for flag range limits only. If flag == 1 then
> > > > +		 * no additional timing information provided.
> > > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > > +		 * supported
> > > > +		 */
> > > > +		if (range->flags != 1)
> > > > +			continue;
> > > > +
> > > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > > +		info->adaptive_sync.pixel_clock_mhz =
> > > > +			range->pixel_clock_mhz * 10;
> > > > +		break;
> > > > +	}
> > > > +}
> > > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > > +
> > > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > > >   * all of the values which would have been set from EDID
> > > >   */
> > > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > > >  
> > > >  	info->non_desktop = 0;
> > > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > >  }
> > > >  
> > > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > > >  
> > > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > > >  
> > > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > > +
> > > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > > >  
> > > >  	if (edid->revision < 3)
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index 5f8c3389d46f..a27a84270d8d 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > > >  };
> > > >  
> > > > +/**
> > > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > > + * &drm_display_info
> > > > + *
> > > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > > + *
> > > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > > + *             EDID's detailed monitor range.
> > > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > > + *             EDID's detailed monitor range
> > > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > > + *                   EDID's detailed monitor range
> > > > + */
> > > > +struct drm_adaptive_sync_info {
> > > > +	int min_vfreq;
> > > > +	int max_vfreq;
> > > > +	int pixel_clock_mhz;
> > > 
> > > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > > to store the pixel clock as kHz like we do everywhere else?
> > 
> > Aye, all typical clock frequencies should be in khz.
> > 
> > Also the vfreqs are only u8 in the EDID, so can be u8 here as well.
> 
> Not if you store them in kHz, they can't.

IMO those can stay in Hz. That's what drm_mode_vrefresh() gives you as well.

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

Re: [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 8233 bytes --]

On Thu, Oct 24, 2019 at 05:20:32PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> > On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > > > Store this info as part fo drm_display_info so it can be used
> > > > > across all drivers.
> > > > > This part of the code is stripped out of amdgpu's function
> > > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > > across all DRM drivers
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > > > >  include/drm/drm_edid.h      |  2 ++
> > > > >  3 files changed, 76 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > index 474ac04d5600..97dd1200773e 100644
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > > +				  const struct edid *edid)
> > > > > +{
> > > > > +	struct drm_display_info *info = &connector->display_info;
> > > > > +	const struct detailed_timing *timing;
> > > > > +	const struct detailed_non_pixel *data;
> > > > > +	const struct detailed_data_monitor_range *range;
> > > > > +	int i;
> > > > 
> > > > This can be unsigned int.
> > > 
> > > Please no. A loop iterator called 'i' should always be a normal signed int.
> > 
> > What? Where's that rule written down? In my experience it's always
> > better to use as restrictive a type as possible. It's really annoying
> > when GCC suddenly starts complaining about comparison between signed and
> > unsigned. So if a variable can never contain a signed value, why risk
> > the ambiguity? The value goes from 0 to 4, the sign bit is useless.
> 
> Dunno if it's really written down anywhere. It's just something
> experience has taught. IIRC there's also a rant from Linus about this
> somewhere. Hm, can't find that one right now, but Andrew Morton also
> seems to agree: https://lwn.net/Articles/309279/
> Ah, here is one Linus rant about unsigned array indexes:
> https://yarchive.net/comp/linux/gcc.html

It's interesting that none of those actually give a real reason why
unsigned int shouldn't be used for variables called i.

> My opinion: unsigned is an very *dangerous* keyword in C that demands
> your respect. You should never use it without thinking first what the
> ramifications are. You always have to have the promotion rules clear 
> in you mind when you do any kind of arithmetic with >= unsigned int
> type. And common idioms such as 'int i' should be respected so as to
> not cause unexpected hair loss to other developers when they decide
> to make the loop iterate backwards.

I would argue that when you do things like make a loop iterate backwards
you better know what variable types you're dealing with.

Anyway, this is clearly very subjective, so feel free to let this be int
if you prefer.

> > > > > +	/*
> > > > > +	 * Restrict Adaptive Sync only for dp and edp
> > > > > +	 */
> > > > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > > +		return;
> > > > > +
> > > > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > > > +		return;
> > > > > +
> > > > > +	for (i = 0; i < 4; i++) {
> > > > > +		timing  = &edid->detailed_timings[i];
> > > > > +		data    = &timing->data.other_data;
> > > > > +		range   = &data->data.range;
> > > > > +		/*
> > > > > +		 * Check if monitor has continuous frequency mode
> > > > > +		 */
> > > > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > > > +			continue;
> > > > > +		/*
> > > > > +		 * Check for flag range limits only. If flag == 1 then
> > > > > +		 * no additional timing information provided.
> > > > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > > > +		 * supported
> > > > > +		 */
> > > > > +		if (range->flags != 1)
> > > > > +			continue;
> > > > > +
> > > > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > > > +		info->adaptive_sync.pixel_clock_mhz =
> > > > > +			range->pixel_clock_mhz * 10;
> > > > > +		break;
> > > > > +	}
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > > > +
> > > > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > > > >   * all of the values which would have been set from EDID
> > > > >   */
> > > > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > > > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > > > >  
> > > > >  	info->non_desktop = 0;
> > > > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > > >  }
> > > > >  
> > > > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > > > >  
> > > > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > > > >  
> > > > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > > > +
> > > > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > > > >  
> > > > >  	if (edid->revision < 3)
> > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > index 5f8c3389d46f..a27a84270d8d 100644
> > > > > --- a/include/drm/drm_connector.h
> > > > > +++ b/include/drm/drm_connector.h
> > > > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > > > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > > > >  };
> > > > >  
> > > > > +/**
> > > > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > > > + * &drm_display_info
> > > > > + *
> > > > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > > > + *
> > > > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range.
> > > > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range
> > > > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > > > + *                   EDID's detailed monitor range
> > > > > + */
> > > > > +struct drm_adaptive_sync_info {
> > > > > +	int min_vfreq;
> > > > > +	int max_vfreq;
> > > > > +	int pixel_clock_mhz;
> > > > 
> > > > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > > > to store the pixel clock as kHz like we do everywhere else?
> > > 
> > > Aye, all typical clock frequencies should be in khz.
> > > 
> > > Also the vfreqs are only u8 in the EDID, so can be u8 here as well.
> > 
> > Not if you store them in kHz, they can't.
> 
> IMO those can stay in Hz. That's what drm_mode_vrefresh() gives you as well.

Oh, you were talking about only the vfreqs. Yes, you're right in that
case.

Unless if at some point in the future we do end up with monitors where
the vrefresh rate no longer fits into u8... I guess manufacturers might
be discouraged from building those just based on the fact that you can't
store the values in EDID.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 8233 bytes --]

On Thu, Oct 24, 2019 at 05:20:32PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> > On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > > > Store this info as part fo drm_display_info so it can be used
> > > > > across all drivers.
> > > > > This part of the code is stripped out of amdgpu's function
> > > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > > across all DRM drivers
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > > > >  include/drm/drm_edid.h      |  2 ++
> > > > >  3 files changed, 76 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > index 474ac04d5600..97dd1200773e 100644
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > > +				  const struct edid *edid)
> > > > > +{
> > > > > +	struct drm_display_info *info = &connector->display_info;
> > > > > +	const struct detailed_timing *timing;
> > > > > +	const struct detailed_non_pixel *data;
> > > > > +	const struct detailed_data_monitor_range *range;
> > > > > +	int i;
> > > > 
> > > > This can be unsigned int.
> > > 
> > > Please no. A loop iterator called 'i' should always be a normal signed int.
> > 
> > What? Where's that rule written down? In my experience it's always
> > better to use as restrictive a type as possible. It's really annoying
> > when GCC suddenly starts complaining about comparison between signed and
> > unsigned. So if a variable can never contain a signed value, why risk
> > the ambiguity? The value goes from 0 to 4, the sign bit is useless.
> 
> Dunno if it's really written down anywhere. It's just something
> experience has taught. IIRC there's also a rant from Linus about this
> somewhere. Hm, can't find that one right now, but Andrew Morton also
> seems to agree: https://lwn.net/Articles/309279/
> Ah, here is one Linus rant about unsigned array indexes:
> https://yarchive.net/comp/linux/gcc.html

It's interesting that none of those actually give a real reason why
unsigned int shouldn't be used for variables called i.

> My opinion: unsigned is an very *dangerous* keyword in C that demands
> your respect. You should never use it without thinking first what the
> ramifications are. You always have to have the promotion rules clear 
> in you mind when you do any kind of arithmetic with >= unsigned int
> type. And common idioms such as 'int i' should be respected so as to
> not cause unexpected hair loss to other developers when they decide
> to make the loop iterate backwards.

I would argue that when you do things like make a loop iterate backwards
you better know what variable types you're dealing with.

Anyway, this is clearly very subjective, so feel free to let this be int
if you prefer.

> > > > > +	/*
> > > > > +	 * Restrict Adaptive Sync only for dp and edp
> > > > > +	 */
> > > > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > > +		return;
> > > > > +
> > > > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > > > +		return;
> > > > > +
> > > > > +	for (i = 0; i < 4; i++) {
> > > > > +		timing  = &edid->detailed_timings[i];
> > > > > +		data    = &timing->data.other_data;
> > > > > +		range   = &data->data.range;
> > > > > +		/*
> > > > > +		 * Check if monitor has continuous frequency mode
> > > > > +		 */
> > > > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > > > +			continue;
> > > > > +		/*
> > > > > +		 * Check for flag range limits only. If flag == 1 then
> > > > > +		 * no additional timing information provided.
> > > > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > > > +		 * supported
> > > > > +		 */
> > > > > +		if (range->flags != 1)
> > > > > +			continue;
> > > > > +
> > > > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > > > +		info->adaptive_sync.pixel_clock_mhz =
> > > > > +			range->pixel_clock_mhz * 10;
> > > > > +		break;
> > > > > +	}
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > > > +
> > > > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > > > >   * all of the values which would have been set from EDID
> > > > >   */
> > > > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > > > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > > > >  
> > > > >  	info->non_desktop = 0;
> > > > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > > >  }
> > > > >  
> > > > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > > > >  
> > > > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > > > >  
> > > > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > > > +
> > > > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > > > >  
> > > > >  	if (edid->revision < 3)
> > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > index 5f8c3389d46f..a27a84270d8d 100644
> > > > > --- a/include/drm/drm_connector.h
> > > > > +++ b/include/drm/drm_connector.h
> > > > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > > > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > > > >  };
> > > > >  
> > > > > +/**
> > > > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > > > + * &drm_display_info
> > > > > + *
> > > > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > > > + *
> > > > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range.
> > > > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range
> > > > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > > > + *                   EDID's detailed monitor range
> > > > > + */
> > > > > +struct drm_adaptive_sync_info {
> > > > > +	int min_vfreq;
> > > > > +	int max_vfreq;
> > > > > +	int pixel_clock_mhz;
> > > > 
> > > > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > > > to store the pixel clock as kHz like we do everywhere else?
> > > 
> > > Aye, all typical clock frequencies should be in khz.
> > > 
> > > Also the vfreqs are only u8 in the EDID, so can be u8 here as well.
> > 
> > Not if you store them in kHz, they can't.
> 
> IMO those can stay in Hz. That's what drm_mode_vrefresh() gives you as well.

Oh, you were talking about only the vfreqs. Yes, you're right in that
case.

Unless if at some point in the future we do end up with monitors where
the vrefresh rate no longer fits into u8... I guess manufacturers might
be discouraged from building those just based on the fact that you can't
store the values in EDID.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 8233 bytes --]

On Thu, Oct 24, 2019 at 05:20:32PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> > On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > > > Store this info as part fo drm_display_info so it can be used
> > > > > across all drivers.
> > > > > This part of the code is stripped out of amdgpu's function
> > > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > > across all DRM drivers
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > > > >  include/drm/drm_edid.h      |  2 ++
> > > > >  3 files changed, 76 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > index 474ac04d5600..97dd1200773e 100644
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > > +				  const struct edid *edid)
> > > > > +{
> > > > > +	struct drm_display_info *info = &connector->display_info;
> > > > > +	const struct detailed_timing *timing;
> > > > > +	const struct detailed_non_pixel *data;
> > > > > +	const struct detailed_data_monitor_range *range;
> > > > > +	int i;
> > > > 
> > > > This can be unsigned int.
> > > 
> > > Please no. A loop iterator called 'i' should always be a normal signed int.
> > 
> > What? Where's that rule written down? In my experience it's always
> > better to use as restrictive a type as possible. It's really annoying
> > when GCC suddenly starts complaining about comparison between signed and
> > unsigned. So if a variable can never contain a signed value, why risk
> > the ambiguity? The value goes from 0 to 4, the sign bit is useless.
> 
> Dunno if it's really written down anywhere. It's just something
> experience has taught. IIRC there's also a rant from Linus about this
> somewhere. Hm, can't find that one right now, but Andrew Morton also
> seems to agree: https://lwn.net/Articles/309279/
> Ah, here is one Linus rant about unsigned array indexes:
> https://yarchive.net/comp/linux/gcc.html

It's interesting that none of those actually give a real reason why
unsigned int shouldn't be used for variables called i.

> My opinion: unsigned is an very *dangerous* keyword in C that demands
> your respect. You should never use it without thinking first what the
> ramifications are. You always have to have the promotion rules clear 
> in you mind when you do any kind of arithmetic with >= unsigned int
> type. And common idioms such as 'int i' should be respected so as to
> not cause unexpected hair loss to other developers when they decide
> to make the loop iterate backwards.

I would argue that when you do things like make a loop iterate backwards
you better know what variable types you're dealing with.

Anyway, this is clearly very subjective, so feel free to let this be int
if you prefer.

> > > > > +	/*
> > > > > +	 * Restrict Adaptive Sync only for dp and edp
> > > > > +	 */
> > > > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > > +		return;
> > > > > +
> > > > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > > > +		return;
> > > > > +
> > > > > +	for (i = 0; i < 4; i++) {
> > > > > +		timing  = &edid->detailed_timings[i];
> > > > > +		data    = &timing->data.other_data;
> > > > > +		range   = &data->data.range;
> > > > > +		/*
> > > > > +		 * Check if monitor has continuous frequency mode
> > > > > +		 */
> > > > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > > > +			continue;
> > > > > +		/*
> > > > > +		 * Check for flag range limits only. If flag == 1 then
> > > > > +		 * no additional timing information provided.
> > > > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > > > +		 * supported
> > > > > +		 */
> > > > > +		if (range->flags != 1)
> > > > > +			continue;
> > > > > +
> > > > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > > > +		info->adaptive_sync.pixel_clock_mhz =
> > > > > +			range->pixel_clock_mhz * 10;
> > > > > +		break;
> > > > > +	}
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > > > +
> > > > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > > > >   * all of the values which would have been set from EDID
> > > > >   */
> > > > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > > > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > > > >  
> > > > >  	info->non_desktop = 0;
> > > > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > > >  }
> > > > >  
> > > > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > > > >  
> > > > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > > > >  
> > > > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > > > +
> > > > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > > > >  
> > > > >  	if (edid->revision < 3)
> > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > index 5f8c3389d46f..a27a84270d8d 100644
> > > > > --- a/include/drm/drm_connector.h
> > > > > +++ b/include/drm/drm_connector.h
> > > > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > > > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > > > >  };
> > > > >  
> > > > > +/**
> > > > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > > > + * &drm_display_info
> > > > > + *
> > > > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > > > + *
> > > > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range.
> > > > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range
> > > > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > > > + *                   EDID's detailed monitor range
> > > > > + */
> > > > > +struct drm_adaptive_sync_info {
> > > > > +	int min_vfreq;
> > > > > +	int max_vfreq;
> > > > > +	int pixel_clock_mhz;
> > > > 
> > > > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > > > to store the pixel clock as kHz like we do everywhere else?
> > > 
> > > Aye, all typical clock frequencies should be in khz.
> > > 
> > > Also the vfreqs are only u8 in the EDID, so can be u8 here as well.
> > 
> > Not if you store them in kHz, they can't.
> 
> IMO those can stay in Hz. That's what drm_mode_vrefresh() gives you as well.

Oh, you were talking about only the vfreqs. Yes, you're right in that
case.

Unless if at some point in the future we do end up with monitors where
the vrefresh rate no longer fits into u8... I guess manufacturers might
be discouraged from building those just based on the fact that you can't
store the values in EDID.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

On Thu, Oct 24, 2019 at 05:20:32PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> > On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > > > Store this info as part fo drm_display_info so it can be used
> > > > > across all drivers.
> > > > > This part of the code is stripped out of amdgpu's function
> > > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > > across all DRM drivers
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > > > >  include/drm/drm_edid.h      |  2 ++
> > > > >  3 files changed, 76 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > index 474ac04d5600..97dd1200773e 100644
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > > +				  const struct edid *edid)
> > > > > +{
> > > > > +	struct drm_display_info *info = &connector->display_info;
> > > > > +	const struct detailed_timing *timing;
> > > > > +	const struct detailed_non_pixel *data;
> > > > > +	const struct detailed_data_monitor_range *range;
> > > > > +	int i;
> > > > 
> > > > This can be unsigned int.
> > > 
> > > Please no. A loop iterator called 'i' should always be a normal signed int.
> > 
> > What? Where's that rule written down? In my experience it's always
> > better to use as restrictive a type as possible. It's really annoying
> > when GCC suddenly starts complaining about comparison between signed and
> > unsigned. So if a variable can never contain a signed value, why risk
> > the ambiguity? The value goes from 0 to 4, the sign bit is useless.
> 
> Dunno if it's really written down anywhere. It's just something
> experience has taught. IIRC there's also a rant from Linus about this
> somewhere. Hm, can't find that one right now, but Andrew Morton also
> seems to agree: https://lwn.net/Articles/309279/
> Ah, here is one Linus rant about unsigned array indexes:
> https://yarchive.net/comp/linux/gcc.html
> 
> My opinion: unsigned is an very *dangerous* keyword in C that demands
> your respect. You should never use it without thinking first what the
> ramifications are. You always have to have the promotion rules clear 
> in you mind when you do any kind of arithmetic with >= unsigned int
> type. And common idioms such as 'int i' should be respected so as to
> not cause unexpected hair loss to other developers when they decide
> to make the loop iterate backwards.
> 
> > 
> > > > > +	/*
> > > > > +	 * Restrict Adaptive Sync only for dp and edp
> > > > > +	 */
> > > > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > > +		return;
> > > > > +
> > > > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > > > +		return;
> > > > > +
> > > > > +	for (i = 0; i < 4; i++) {
> > > > > +		timing  = &edid->detailed_timings[i];
> > > > > +		data    = &timing->data.other_data;
> > > > > +		range   = &data->data.range;
> > > > > +		/*
> > > > > +		 * Check if monitor has continuous frequency mode
> > > > > +		 */
> > > > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > > > +			continue;
> > > > > +		/*
> > > > > +		 * Check for flag range limits only. If flag == 1 then
> > > > > +		 * no additional timing information provided.
> > > > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > > > +		 * supported
> > > > > +		 */
> > > > > +		if (range->flags != 1)
> > > > > +			continue;
> > > > > +
> > > > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > > > +		info->adaptive_sync.pixel_clock_mhz =
> > > > > +			range->pixel_clock_mhz * 10;
> > > > > +		break;
> > > > > +	}
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > > > +
> > > > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > > > >   * all of the values which would have been set from EDID
> > > > >   */
> > > > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > > > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > > > >  
> > > > >  	info->non_desktop = 0;
> > > > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > > >  }
> > > > >  
> > > > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > > > >  
> > > > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > > > >  
> > > > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > > > +
> > > > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > > > >  
> > > > >  	if (edid->revision < 3)
> > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > index 5f8c3389d46f..a27a84270d8d 100644
> > > > > --- a/include/drm/drm_connector.h
> > > > > +++ b/include/drm/drm_connector.h
> > > > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > > > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > > > >  };
> > > > >  
> > > > > +/**
> > > > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > > > + * &drm_display_info
> > > > > + *
> > > > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > > > + *
> > > > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range.
> > > > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range
> > > > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > > > + *                   EDID's detailed monitor range
> > > > > + */
> > > > > +struct drm_adaptive_sync_info {
> > > > > +	int min_vfreq;
> > > > > +	int max_vfreq;
> > > > > +	int pixel_clock_mhz;
> > > > 
> > > > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > > > to store the pixel clock as kHz like we do everywhere else?
> > > 
> > > Aye, all typical clock frequencies should be in khz.
> > > 
> > > Also the vfreqs are only u8 in the EDID, so can be u8 here as well.
> > 
> > Not if you store them in kHz, they can't.
> 
> IMO those can stay in Hz. That's what drm_mode_vrefresh() gives you as well.

And i kept them in hz for min and max and Mhz for pixel clock from the detailed_data_monitor_range struct
So change these all to u8?

Manasi

> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

On Thu, Oct 24, 2019 at 05:20:32PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> > On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > > > Store this info as part fo drm_display_info so it can be used
> > > > > across all drivers.
> > > > > This part of the code is stripped out of amdgpu's function
> > > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > > across all DRM drivers
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> > > > >  include/drm/drm_edid.h      |  2 ++
> > > > >  3 files changed, 76 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > index 474ac04d5600..97dd1200773e 100644
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > > +				  const struct edid *edid)
> > > > > +{
> > > > > +	struct drm_display_info *info = &connector->display_info;
> > > > > +	const struct detailed_timing *timing;
> > > > > +	const struct detailed_non_pixel *data;
> > > > > +	const struct detailed_data_monitor_range *range;
> > > > > +	int i;
> > > > 
> > > > This can be unsigned int.
> > > 
> > > Please no. A loop iterator called 'i' should always be a normal signed int.
> > 
> > What? Where's that rule written down? In my experience it's always
> > better to use as restrictive a type as possible. It's really annoying
> > when GCC suddenly starts complaining about comparison between signed and
> > unsigned. So if a variable can never contain a signed value, why risk
> > the ambiguity? The value goes from 0 to 4, the sign bit is useless.
> 
> Dunno if it's really written down anywhere. It's just something
> experience has taught. IIRC there's also a rant from Linus about this
> somewhere. Hm, can't find that one right now, but Andrew Morton also
> seems to agree: https://lwn.net/Articles/309279/
> Ah, here is one Linus rant about unsigned array indexes:
> https://yarchive.net/comp/linux/gcc.html
> 
> My opinion: unsigned is an very *dangerous* keyword in C that demands
> your respect. You should never use it without thinking first what the
> ramifications are. You always have to have the promotion rules clear 
> in you mind when you do any kind of arithmetic with >= unsigned int
> type. And common idioms such as 'int i' should be respected so as to
> not cause unexpected hair loss to other developers when they decide
> to make the loop iterate backwards.
> 
> > 
> > > > > +	/*
> > > > > +	 * Restrict Adaptive Sync only for dp and edp
> > > > > +	 */
> > > > > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > > +		return;
> > > > > +
> > > > > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > > > +		return;
> > > > > +
> > > > > +	for (i = 0; i < 4; i++) {
> > > > > +		timing  = &edid->detailed_timings[i];
> > > > > +		data    = &timing->data.other_data;
> > > > > +		range   = &data->data.range;
> > > > > +		/*
> > > > > +		 * Check if monitor has continuous frequency mode
> > > > > +		 */
> > > > > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > > > +			continue;
> > > > > +		/*
> > > > > +		 * Check for flag range limits only. If flag == 1 then
> > > > > +		 * no additional timing information provided.
> > > > > +		 * Default GTF, GTF Secondary curve and CVT are not
> > > > > +		 * supported
> > > > > +		 */
> > > > > +		if (range->flags != 1)
> > > > > +			continue;
> > > > > +
> > > > > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > > > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > > > +		info->adaptive_sync.pixel_clock_mhz =
> > > > > +			range->pixel_clock_mhz * 10;
> > > > > +		break;
> > > > > +	}
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > > > +
> > > > >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> > > > >   * all of the values which would have been set from EDID
> > > > >   */
> > > > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> > > > >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> > > > >  
> > > > >  	info->non_desktop = 0;
> > > > > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > > >  }
> > > > >  
> > > > >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > > > > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> > > > >  
> > > > >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> > > > >  
> > > > > +	drm_get_adaptive_sync_limits(connector, edid);
> > > > > +
> > > > >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> > > > >  
> > > > >  	if (edid->revision < 3)
> > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > index 5f8c3389d46f..a27a84270d8d 100644
> > > > > --- a/include/drm/drm_connector.h
> > > > > +++ b/include/drm/drm_connector.h
> > > > > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> > > > >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> > > > >  };
> > > > >  
> > > > > +/**
> > > > > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > > > > + * &drm_display_info
> > > > > + *
> > > > > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > > > > + * as parsed from EDID's detailed monitor range descriptor block.
> > > > > + *
> > > > > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range.
> > > > > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > > > > + *             EDID's detailed monitor range
> > > > > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > > > > + *                   EDID's detailed monitor range
> > > > > + */
> > > > > +struct drm_adaptive_sync_info {
> > > > > +	int min_vfreq;
> > > > > +	int max_vfreq;
> > > > > +	int pixel_clock_mhz;
> > > > 
> > > > Any reason why these can't be unsigned? Also, does it perhaps make sense
> > > > to store the pixel clock as kHz like we do everywhere else?
> > > 
> > > Aye, all typical clock frequencies should be in khz.
> > > 
> > > Also the vfreqs are only u8 in the EDID, so can be u8 here as well.
> > 
> > Not if you store them in kHz, they can't.
> 
> IMO those can stay in Hz. That's what drm_mode_vrefresh() gives you as well.

And i kept them in hz for min and max and Mhz for pixel clock from the detailed_data_monitor_range struct
So change these all to u8?

Manasi

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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> 
> This can be unsigned int.

Yes this can be unsigned, will change this

> 
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> 
> Any reason why these can't be unsigned? Also, does it perhaps make sense
> to store the pixel clock as kHz like we do everywhere else?
>

Yes I could change these to unsigned as well, just kept is similar to the AMDGPU defs to begin with
Also the pixel clock in MHz is to align with the drm_edid.h in detailed_data_monitor_range

Regards
Manasi
 
> Thierry


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

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> 
> This can be unsigned int.

Yes this can be unsigned, will change this

> 
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> 
> Any reason why these can't be unsigned? Also, does it perhaps make sense
> to store the pixel clock as kHz like we do everywhere else?
>

Yes I could change these to unsigned as well, just kept is similar to the AMDGPU defs to begin with
Also the pixel clock in MHz is to align with the drm_edid.h in detailed_data_monitor_range

Regards
Manasi
 
> Thierry


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

Re: [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Harry Wentland]

On 2019-10-23 8:00 p.m., Manasi Navare wrote:
> Adaptive Sync is a VESA feature so add a DRM core helper to parse
> the EDID's detailed descritors to obtain the adaptive sync monitor range.
> Store this info as part fo drm_display_info so it can be used
> across all drivers.
> This part of the code is stripped out of amdgpu's function
> amdgpu_dm_update_freesync_caps() to make it generic and be used
> across all DRM drivers
> 

Please CC Nick on these as well. Added him now.

Would it be possible to add a patch to update amdgpu to call this function?

Harry

> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h | 25 +++++++++++++++++++
>  include/drm/drm_edid.h      |  2 ++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 474ac04d5600..97dd1200773e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  	}
>  }
>  
> +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> +				  const struct edid *edid)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	const struct detailed_timing *timing;
> +	const struct detailed_non_pixel *data;
> +	const struct detailed_data_monitor_range *range;
> +	int i;
> +
> +	/*
> +	 * Restrict Adaptive Sync only for dp and edp
> +	 */
> +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> +		return;
> +
> +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> +		return;
> +
> +	for (i = 0; i < 4; i++) {
> +		timing  = &edid->detailed_timings[i];
> +		data    = &timing->data.other_data;
> +		range   = &data->data.range;
> +		/*
> +		 * Check if monitor has continuous frequency mode
> +		 */
> +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> +			continue;
> +		/*
> +		 * Check for flag range limits only. If flag == 1 then
> +		 * no additional timing information provided.
> +		 * Default GTF, GTF Secondary curve and CVT are not
> +		 * supported
> +		 */
> +		if (range->flags != 1)
> +			continue;
> +
> +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> +		info->adaptive_sync.pixel_clock_mhz =
> +			range->pixel_clock_mhz * 10;
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> +
>  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
>   * all of the values which would have been set from EDID
>   */
> @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
>  	memset(&info->hdmi, 0, sizeof(info->hdmi));
>  
>  	info->non_desktop = 0;
> +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
>  }
>  
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>  
>  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
>  
> +	drm_get_adaptive_sync_limits(connector, edid);
> +
>  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
>  
>  	if (edid->revision < 3)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5f8c3389d46f..a27a84270d8d 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -254,6 +254,26 @@ enum drm_panel_orientation {
>  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>  };
>  
> +/**
> + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> + * &drm_display_info
> + *
> + * This struct is used to store a Panel's Adaptive Sync capabilities
> + * as parsed from EDID's detailed monitor range descriptor block.
> + *
> + * @min_vfreq: This is the min supported refresh rate in Hz from
> + *             EDID's detailed monitor range.
> + * @max_vfreq: This is the max supported refresh rate in Hz from
> + *             EDID's detailed monitor range
> + * @pixel_clock_mhz: This is the dotclock in MHz from
> + *                   EDID's detailed monitor range
> + */
> +struct drm_adaptive_sync_info {
> +	int min_vfreq;
> +	int max_vfreq;
> +	int pixel_clock_mhz;
> +};
> +
>  /*
>   * This is a consolidated colorimetry list supported by HDMI and
>   * DP protocol standard. The respective connectors will register
> @@ -465,6 +485,11 @@ struct drm_display_info {
>  	 * @non_desktop: Non desktop display (HMD).
>  	 */
>  	bool non_desktop;
> +
> +	/**
> +	 * @adaptive_sync: Adaptive Sync capabilities of the DP/eDP sink
> +	 */
> +	struct drm_adaptive_sync_info adaptive_sync;
>  };
>  
>  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index f0b03d401c27..b9a230aa3e69 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -503,4 +503,6 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  					   int hsize, int vsize, int fresh,
>  					   bool rb);
> +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> +				  const struct edid *edid);
>  #endif /* __DRM_EDID_H__ */
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Harry Wentland]

On 2019-10-23 8:00 p.m., Manasi Navare wrote:
> Adaptive Sync is a VESA feature so add a DRM core helper to parse
> the EDID's detailed descritors to obtain the adaptive sync monitor range.
> Store this info as part fo drm_display_info so it can be used
> across all drivers.
> This part of the code is stripped out of amdgpu's function
> amdgpu_dm_update_freesync_caps() to make it generic and be used
> across all DRM drivers
> 

Please CC Nick on these as well. Added him now.

Would it be possible to add a patch to update amdgpu to call this function?

Harry

> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h | 25 +++++++++++++++++++
>  include/drm/drm_edid.h      |  2 ++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 474ac04d5600..97dd1200773e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  	}
>  }
>  
> +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> +				  const struct edid *edid)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	const struct detailed_timing *timing;
> +	const struct detailed_non_pixel *data;
> +	const struct detailed_data_monitor_range *range;
> +	int i;
> +
> +	/*
> +	 * Restrict Adaptive Sync only for dp and edp
> +	 */
> +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> +		return;
> +
> +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> +		return;
> +
> +	for (i = 0; i < 4; i++) {
> +		timing  = &edid->detailed_timings[i];
> +		data    = &timing->data.other_data;
> +		range   = &data->data.range;
> +		/*
> +		 * Check if monitor has continuous frequency mode
> +		 */
> +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> +			continue;
> +		/*
> +		 * Check for flag range limits only. If flag == 1 then
> +		 * no additional timing information provided.
> +		 * Default GTF, GTF Secondary curve and CVT are not
> +		 * supported
> +		 */
> +		if (range->flags != 1)
> +			continue;
> +
> +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> +		info->adaptive_sync.pixel_clock_mhz =
> +			range->pixel_clock_mhz * 10;
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> +
>  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
>   * all of the values which would have been set from EDID
>   */
> @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
>  	memset(&info->hdmi, 0, sizeof(info->hdmi));
>  
>  	info->non_desktop = 0;
> +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
>  }
>  
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>  
>  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
>  
> +	drm_get_adaptive_sync_limits(connector, edid);
> +
>  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
>  
>  	if (edid->revision < 3)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5f8c3389d46f..a27a84270d8d 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -254,6 +254,26 @@ enum drm_panel_orientation {
>  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>  };
>  
> +/**
> + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> + * &drm_display_info
> + *
> + * This struct is used to store a Panel's Adaptive Sync capabilities
> + * as parsed from EDID's detailed monitor range descriptor block.
> + *
> + * @min_vfreq: This is the min supported refresh rate in Hz from
> + *             EDID's detailed monitor range.
> + * @max_vfreq: This is the max supported refresh rate in Hz from
> + *             EDID's detailed monitor range
> + * @pixel_clock_mhz: This is the dotclock in MHz from
> + *                   EDID's detailed monitor range
> + */
> +struct drm_adaptive_sync_info {
> +	int min_vfreq;
> +	int max_vfreq;
> +	int pixel_clock_mhz;
> +};
> +
>  /*
>   * This is a consolidated colorimetry list supported by HDMI and
>   * DP protocol standard. The respective connectors will register
> @@ -465,6 +485,11 @@ struct drm_display_info {
>  	 * @non_desktop: Non desktop display (HMD).
>  	 */
>  	bool non_desktop;
> +
> +	/**
> +	 * @adaptive_sync: Adaptive Sync capabilities of the DP/eDP sink
> +	 */
> +	struct drm_adaptive_sync_info adaptive_sync;
>  };
>  
>  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index f0b03d401c27..b9a230aa3e69 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -503,4 +503,6 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  					   int hsize, int vsize, int fresh,
>  					   bool rb);
> +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> +				  const struct edid *edid);
>  #endif /* __DRM_EDID_H__ */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

On Thu, Oct 24, 2019 at 03:06:37PM +0000, Harry Wentland wrote:
> On 2019-10-23 8:00 p.m., Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> 
> Please CC Nick on these as well. Added him now.

Thanks Harry, i will copy Nick for all next patches.

> 
> Would it be possible to add a patch to update amdgpu to call this function?

So based on the comments on this patch, the suggestion is to have only min and max vfreq
stored in drm_display_info struct as u8s in Hz since pixel_clock_mhz never gets used.

Could you or Nick comment on this if this looks good so I can also work on modifying the
amdgpu to call this function, but that would also mean deleting the pixel_clock_mhz from
amdgpu_connector and I want to make sure there are no regerssions because of this

Manasi

> 
> Harry
> 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> > +};
> > +
> >  /*
> >   * This is a consolidated colorimetry list supported by HDMI and
> >   * DP protocol standard. The respective connectors will register
> > @@ -465,6 +485,11 @@ struct drm_display_info {
> >  	 * @non_desktop: Non desktop display (HMD).
> >  	 */
> >  	bool non_desktop;
> > +
> > +	/**
> > +	 * @adaptive_sync: Adaptive Sync capabilities of the DP/eDP sink
> > +	 */
> > +	struct drm_adaptive_sync_info adaptive_sync;
> >  };
> >  
> >  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index f0b03d401c27..b9a230aa3e69 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -503,4 +503,6 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
> >  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
> >  					   int hsize, int vsize, int fresh,
> >  					   bool rb);
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid);
> >  #endif /* __DRM_EDID_H__ */
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

On Thu, Oct 24, 2019 at 03:06:37PM +0000, Harry Wentland wrote:
> On 2019-10-23 8:00 p.m., Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> 
> Please CC Nick on these as well. Added him now.

Thanks Harry, i will copy Nick for all next patches.

> 
> Would it be possible to add a patch to update amdgpu to call this function?

So based on the comments on this patch, the suggestion is to have only min and max vfreq
stored in drm_display_info struct as u8s in Hz since pixel_clock_mhz never gets used.

Could you or Nick comment on this if this looks good so I can also work on modifying the
amdgpu to call this function, but that would also mean deleting the pixel_clock_mhz from
amdgpu_connector and I want to make sure there are no regerssions because of this

Manasi

> 
> Harry
> 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> > +};
> > +
> >  /*
> >   * This is a consolidated colorimetry list supported by HDMI and
> >   * DP protocol standard. The respective connectors will register
> > @@ -465,6 +485,11 @@ struct drm_display_info {
> >  	 * @non_desktop: Non desktop display (HMD).
> >  	 */
> >  	bool non_desktop;
> > +
> > +	/**
> > +	 * @adaptive_sync: Adaptive Sync capabilities of the DP/eDP sink
> > +	 */
> > +	struct drm_adaptive_sync_info adaptive_sync;
> >  };
> >  
> >  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index f0b03d401c27..b9a230aa3e69 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -503,4 +503,6 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
> >  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
> >  					   int hsize, int vsize, int fresh,
> >  					   bool rb);
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid);
> >  #endif /* __DRM_EDID_H__ */
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits

[Manasi Navare]

On Thu, Oct 24, 2019 at 03:06:37PM +0000, Harry Wentland wrote:
> On 2019-10-23 8:00 p.m., Manasi Navare wrote:
> > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > Store this info as part fo drm_display_info so it can be used
> > across all drivers.
> > This part of the code is stripped out of amdgpu's function
> > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > across all DRM drivers
> > 
> 
> Please CC Nick on these as well. Added him now.

Thanks Harry, i will copy Nick for all next patches.

> 
> Would it be possible to add a patch to update amdgpu to call this function?

So based on the comments on this patch, the suggestion is to have only min and max vfreq
stored in drm_display_info struct as u8s in Hz since pixel_clock_mhz never gets used.

Could you or Nick comment on this if this looks good so I can also work on modifying the
amdgpu to call this function, but that would also mean deleting the pixel_clock_mhz from
amdgpu_connector and I want to make sure there are no regerssions because of this

Manasi

> 
> Harry
> 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c  | 49 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h | 25 +++++++++++++++++++
> >  include/drm/drm_edid.h      |  2 ++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 474ac04d5600..97dd1200773e 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >  	}
> >  }
> >  
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid)
> > +{
> > +	struct drm_display_info *info = &connector->display_info;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> > +	int i;
> > +
> > +	/*
> > +	 * Restrict Adaptive Sync only for dp and edp
> > +	 */
> > +	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		return;
> > +
> > +	if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > +		return;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		timing  = &edid->detailed_timings[i];
> > +		data    = &timing->data.other_data;
> > +		range   = &data->data.range;
> > +		/*
> > +		 * Check if monitor has continuous frequency mode
> > +		 */
> > +		if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +			continue;
> > +		/*
> > +		 * Check for flag range limits only. If flag == 1 then
> > +		 * no additional timing information provided.
> > +		 * Default GTF, GTF Secondary curve and CVT are not
> > +		 * supported
> > +		 */
> > +		if (range->flags != 1)
> > +			continue;
> > +
> > +		info->adaptive_sync.min_vfreq = range->min_vfreq;
> > +		info->adaptive_sync.max_vfreq = range->max_vfreq;
> > +		info->adaptive_sync.pixel_clock_mhz =
> > +			range->pixel_clock_mhz * 10;
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > +
> >  /* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset
> >   * all of the values which would have been set from EDID
> >   */
> > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> >  	memset(&info->hdmi, 0, sizeof(info->hdmi));
> >  
> >  	info->non_desktop = 0;
> > +	memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> >  }
> >  
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
> > @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> >  
> >  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
> >  
> > +	drm_get_adaptive_sync_limits(connector, edid);
> > +
> >  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> >  
> >  	if (edid->revision < 3)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 5f8c3389d46f..a27a84270d8d 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> >  	DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> >  };
> >  
> > +/**
> > + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> > + * &drm_display_info
> > + *
> > + * This struct is used to store a Panel's Adaptive Sync capabilities
> > + * as parsed from EDID's detailed monitor range descriptor block.
> > + *
> > + * @min_vfreq: This is the min supported refresh rate in Hz from
> > + *             EDID's detailed monitor range.
> > + * @max_vfreq: This is the max supported refresh rate in Hz from
> > + *             EDID's detailed monitor range
> > + * @pixel_clock_mhz: This is the dotclock in MHz from
> > + *                   EDID's detailed monitor range
> > + */
> > +struct drm_adaptive_sync_info {
> > +	int min_vfreq;
> > +	int max_vfreq;
> > +	int pixel_clock_mhz;
> > +};
> > +
> >  /*
> >   * This is a consolidated colorimetry list supported by HDMI and
> >   * DP protocol standard. The respective connectors will register
> > @@ -465,6 +485,11 @@ struct drm_display_info {
> >  	 * @non_desktop: Non desktop display (HMD).
> >  	 */
> >  	bool non_desktop;
> > +
> > +	/**
> > +	 * @adaptive_sync: Adaptive Sync capabilities of the DP/eDP sink
> > +	 */
> > +	struct drm_adaptive_sync_info adaptive_sync;
> >  };
> >  
> >  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index f0b03d401c27..b9a230aa3e69 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -503,4 +503,6 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
> >  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
> >  					   int hsize, int vsize, int fresh,
> >  					   bool rb);
> > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > +				  const struct edid *edid);
> >  #endif /* __DRM_EDID_H__ */
> > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel