[PATCH v3 0/3] Experimental freesync video mode optimization

[PATCH v3 0/3] Experimental freesync video mode optimization

[Aurabindo Pillai]

Changes in V3
=============

1) Add freesync video modes based on preferred modes:

* Cache base freesync video mode during the first iteration to avoid
  iterating over modelist again later.
* Add mode for 60 fps videos

2) Skip modeset for front porch change

* Fixes for bug exposed by caching of modes.

Changes in V2
=============

1) Add freesync video modes based on preferred modes:

* Remove check for connector type before adding freesync compatible
  modes as VRR support is being checked, and there is no reason to block
  freesync video support on eDP.
* use drm_mode_equal() instead of creating same functionality.
* Additional null pointer deference check
* Removed unnecessary variables.
* Cosmetic fixes.

2) Skip modeset for front porch change

* Remove _FSV string being appended to freesync video modes so as to not
  define new policies or break existing application that might use the
  mode name to figure out mode resolution.
* Remove unnecessary variables
* Cosmetic fixes.

--

This patchset enables freesync video mode usecase where the userspace
can request a freesync compatible video mode such that switching to this
mode does not trigger blanking.

This feature is guarded by a module parameter which is disabled by
default. Enabling this paramters adds additional modes to the driver
modelist, and also enables the optimization to skip modeset when using
one of these modes.

--

Aurabindo Pillai (3):
  drm/amd/display: Add module parameter for freesync video mode
  drm/amd/display: Add freesync video modes based on preferred modes
  drm/amd/display: Skip modeset for front porch change

 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  12 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 377 ++++++++++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +
 4 files changed, 361 insertions(+), 32 deletions(-)

-- 
2.29.2

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

[PATCH v3 0/3] Experimental freesync video mode optimization

[Aurabindo Pillai]

Changes in V3
=============

1) Add freesync video modes based on preferred modes:

* Cache base freesync video mode during the first iteration to avoid
  iterating over modelist again later.
* Add mode for 60 fps videos

2) Skip modeset for front porch change

* Fixes for bug exposed by caching of modes.

Changes in V2
=============

1) Add freesync video modes based on preferred modes:

* Remove check for connector type before adding freesync compatible
  modes as VRR support is being checked, and there is no reason to block
  freesync video support on eDP.
* use drm_mode_equal() instead of creating same functionality.
* Additional null pointer deference check
* Removed unnecessary variables.
* Cosmetic fixes.

2) Skip modeset for front porch change

* Remove _FSV string being appended to freesync video modes so as to not
  define new policies or break existing application that might use the
  mode name to figure out mode resolution.
* Remove unnecessary variables
* Cosmetic fixes.

--

This patchset enables freesync video mode usecase where the userspace
can request a freesync compatible video mode such that switching to this
mode does not trigger blanking.

This feature is guarded by a module parameter which is disabled by
default. Enabling this paramters adds additional modes to the driver
modelist, and also enables the optimization to skip modeset when using
one of these modes.

--

Aurabindo Pillai (3):
  drm/amd/display: Add module parameter for freesync video mode
  drm/amd/display: Add freesync video modes based on preferred modes
  drm/amd/display: Skip modeset for front porch change

 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  12 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 377 ++++++++++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +
 4 files changed, 361 insertions(+), 32 deletions(-)

-- 
2.29.2

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

[PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode

[Aurabindo Pillai]

[Why&How]
Adds a module parameter to enable experimental freesync video mode modeset
optimization. Enabling this mode allows the driver to skip a full modeset when
freesync compatible modes are requested by the userspace. This paramters also
adds some standard modes based on the connected monitor's VRR range.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig at amd.com>
Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index eed5410947e9..e0942184efdd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
 extern int amdgpu_emu_mode;
 extern uint amdgpu_smu_memory_pool_size;
 extern uint amdgpu_dc_feature_mask;
+extern uint amdgpu_exp_freesync_vid_mode;
 extern uint amdgpu_dc_debug_mask;
 extern uint amdgpu_dm_abm_level;
 extern struct amdgpu_mgpu_info mgpu_info;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b2a1dd7581bf..ece51ecd53d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -158,6 +158,7 @@ int amdgpu_mes;
 int amdgpu_noretry = -1;
 int amdgpu_force_asic_type = -1;
 int amdgpu_tmz;
+uint amdgpu_exp_freesync_vid_mode;
 int amdgpu_reset_method = -1; /* auto */
 int amdgpu_num_kcq = -1;
 
@@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444);
 MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)");
 module_param_named(tmz, amdgpu_tmz, int, 0444);
 
+/**
+ * DOC: experimental_freesync_video (uint)
+ * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience
+ * when setting a freesync supported mode for which full modeset is not needed.
+ * The default value: 0 (off).
+ */
+MODULE_PARM_DESC(
+	experimental_freesync_video,
+	"Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");
+module_param_named(experimental_freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444);
+
 /**
  * DOC: reset_method (int)
  * GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)
-- 
2.29.2

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

[PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode

[Aurabindo Pillai]

[Why&How]
Adds a module parameter to enable experimental freesync video mode modeset
optimization. Enabling this mode allows the driver to skip a full modeset when
freesync compatible modes are requested by the userspace. This paramters also
adds some standard modes based on the connected monitor's VRR range.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig at amd.com>
Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index eed5410947e9..e0942184efdd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
 extern int amdgpu_emu_mode;
 extern uint amdgpu_smu_memory_pool_size;
 extern uint amdgpu_dc_feature_mask;
+extern uint amdgpu_exp_freesync_vid_mode;
 extern uint amdgpu_dc_debug_mask;
 extern uint amdgpu_dm_abm_level;
 extern struct amdgpu_mgpu_info mgpu_info;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b2a1dd7581bf..ece51ecd53d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -158,6 +158,7 @@ int amdgpu_mes;
 int amdgpu_noretry = -1;
 int amdgpu_force_asic_type = -1;
 int amdgpu_tmz;
+uint amdgpu_exp_freesync_vid_mode;
 int amdgpu_reset_method = -1; /* auto */
 int amdgpu_num_kcq = -1;
 
@@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444);
 MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)");
 module_param_named(tmz, amdgpu_tmz, int, 0444);
 
+/**
+ * DOC: experimental_freesync_video (uint)
+ * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience
+ * when setting a freesync supported mode for which full modeset is not needed.
+ * The default value: 0 (off).
+ */
+MODULE_PARM_DESC(
+	experimental_freesync_video,
+	"Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");
+module_param_named(experimental_freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444);
+
 /**
  * DOC: reset_method (int)
  * GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)
-- 
2.29.2

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

[PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes

[Aurabindo Pillai]

[Why&How]
If experimental freesync video mode module parameter is enabled, add
few extra display modes into the driver's mode list corresponding to common
video frame rates. When userspace sets these modes, no modeset will be
performed (if current mode was one of freesync modes or the base freesync mode
based off which timings have been generated for the rest of the freesync modes)
since these modes only differ from the base mode with front porch timing.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig at amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167 ++++++++++++++++++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   2 +
 2 files changed, 169 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e7ee2467eadb..c1ffd33e9d83 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5174,6 +5174,59 @@ static void dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
 	set_master_stream(context->streams, context->stream_count);
 }
 
+static struct drm_display_mode *
+get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
+			  bool use_probed_modes)
+{
+	struct drm_display_mode *m, *m_pref = NULL;
+	u16 current_refresh, highest_refresh;
+	struct list_head *list_head = use_probed_modes ?
+						    &aconnector->base.probed_modes :
+						    &aconnector->base.modes;
+
+	if (aconnector->freesync_vid_base.clock != 0)
+		return &aconnector->freesync_vid_base;
+
+	/* Find the preferred mode */
+	list_for_each_entry (m, list_head, head) {
+		if (m->type & DRM_MODE_TYPE_PREFERRED) {
+			m_pref = m;
+			break;
+		}
+	}
+
+	if (!m_pref) {
+		/* Probably an EDID with no preferred mode. Fallback to first entry */
+		m_pref = list_first_entry_or_null(
+			&aconnector->base.modes, struct drm_display_mode, head);
+		if (!m_pref) {
+			DRM_DEBUG_DRIVER("No preferred mode found in EDID\n");
+			return NULL;
+		}
+	}
+
+	highest_refresh = drm_mode_vrefresh(m_pref);
+
+	/*
+	 * Find the mode with highest refresh rate with same resolution.
+	 * For some monitors, preferred mode is not the mode with highest
+	 * supported refresh rate.
+	 */
+	list_for_each_entry (m, list_head, head) {
+		current_refresh  = drm_mode_vrefresh(m);
+
+		if (m->hdisplay == m_pref->hdisplay &&
+		    m->vdisplay == m_pref->vdisplay &&
+		    highest_refresh < current_refresh) {
+			highest_refresh = current_refresh;
+			m_pref = m;
+		}
+	}
+
+	aconnector->freesync_vid_base = *m_pref;
+	return m_pref;
+}
+
 static struct dc_stream_state *
 create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		       const struct drm_display_mode *drm_mode,
@@ -6999,6 +7052,119 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
 	}
 }
 
+static bool is_duplicate_mode(struct amdgpu_dm_connector *aconnector,
+			      struct drm_display_mode *mode)
+{
+	struct drm_display_mode *m;
+
+	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
+		if (drm_mode_equal(m, mode))
+			return true;
+	}
+
+	return false;
+}
+
+static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
+			 struct detailed_data_monitor_range *range)
+{
+	const struct drm_display_mode *m;
+	struct drm_display_mode *new_mode;
+	uint i;
+	uint64_t target_vtotal, target_vtotal_diff;
+	uint32_t new_modes_count = 0;
+	uint64_t num, den;
+
+	/* Standard FPS values
+	 *
+	 * 23.976 - TV/NTSC
+	 * 24	  - Cinema
+	 * 25     - TV/PAL
+	 * 29.97  - TV/NTSC
+	 * 30     - TV/NTSC
+	 * 48	  - Cinema HFR
+	 * 50	  - TV/PAL
+	 */
+	const uint32_t common_rates[] = { 23976, 24000, 25000, 29970, 30000,
+					 48000, 50000, 60000, 72000, 96000 };
+
+	/*
+	 * Find mode with highest refresh rate with the same resolution
+	 * as the preferred mode. Some monitors report a preferred mode
+	 * with lower resolution than the highest refresh rate supported.
+	 */
+
+	m = get_highest_refresh_rate_mode(aconnector, true);
+	if (!m)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(common_rates); i++) {
+		if (drm_mode_vrefresh(m) * 1000 < common_rates[i])
+			continue;
+
+		if (common_rates[i] < range->min_vfreq * 1000)
+			continue;
+
+		num = (unsigned long long)m->clock * 1000 * 1000;
+		den = common_rates[i] * (unsigned long long)m->htotal;
+		target_vtotal = div_u64(num, den);
+		target_vtotal_diff = target_vtotal - m->vtotal;
+
+		/* Check for illegal modes */
+		if (m->vsync_start + target_vtotal_diff < m->vdisplay ||
+		    m->vsync_end + target_vtotal_diff < m->vsync_start ||
+		    m->vtotal + target_vtotal_diff < m->vsync_end)
+			continue;
+
+		new_mode = drm_mode_duplicate(aconnector->base.dev, m);
+		if (!new_mode)
+			goto out;
+
+		new_mode->vtotal += (u16)target_vtotal_diff;
+		new_mode->vsync_start += (u16)target_vtotal_diff;
+		new_mode->vsync_end += (u16)target_vtotal_diff;
+		new_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
+		new_mode->type |= DRM_MODE_TYPE_DRIVER;
+
+		if (!is_duplicate_mode(aconnector, new_mode)) {
+			drm_mode_probed_add(&aconnector->base, new_mode);
+			new_modes_count += 1;
+		} else
+			drm_mode_destroy(aconnector->base.dev, new_mode);
+	}
+ out:
+	return new_modes_count;
+}
+
+static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
+						   struct edid *edid)
+{
+	uint8_t i;
+	struct detailed_timing *timing;
+	struct detailed_non_pixel *data;
+	struct detailed_data_monitor_range *range;
+	struct amdgpu_dm_connector *amdgpu_dm_connector =
+		to_amdgpu_dm_connector(connector);
+
+	if (!(amdgpu_exp_freesync_vid_mode && edid))
+		return;
+
+	if (edid->version == 1 && edid->revision > 1) {
+		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 &&
+			    range->max_vfreq - range->min_vfreq > 10) {
+				amdgpu_dm_connector->num_modes += add_fs_modes(amdgpu_dm_connector, range);
+				break;
+			}
+		}
+	}
+}
+
 static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
 {
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
@@ -7014,6 +7180,7 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
 	} else {
 		amdgpu_dm_connector_ddc_get_modes(connector, edid);
 		amdgpu_dm_connector_add_common_modes(encoder, connector);
+		amdgpu_dm_connector_add_freesync_modes(connector, edid);
 	}
 	amdgpu_dm_fbc_init(connector);
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 251af783f6b1..f5a5f1ad7dba 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -413,6 +413,8 @@ struct amdgpu_dm_connector {
 #endif
 	bool force_yuv420_output;
 	struct dsc_preferred_settings dsc_settings;
+	/* Cached display modes */
+	struct drm_display_mode freesync_vid_base;
 };
 
 #define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
-- 
2.29.2

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

[PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes

[Aurabindo Pillai]

[Why&How]
If experimental freesync video mode module parameter is enabled, add
few extra display modes into the driver's mode list corresponding to common
video frame rates. When userspace sets these modes, no modeset will be
performed (if current mode was one of freesync modes or the base freesync mode
based off which timings have been generated for the rest of the freesync modes)
since these modes only differ from the base mode with front porch timing.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig at amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167 ++++++++++++++++++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   2 +
 2 files changed, 169 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e7ee2467eadb..c1ffd33e9d83 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5174,6 +5174,59 @@ static void dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
 	set_master_stream(context->streams, context->stream_count);
 }
 
+static struct drm_display_mode *
+get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
+			  bool use_probed_modes)
+{
+	struct drm_display_mode *m, *m_pref = NULL;
+	u16 current_refresh, highest_refresh;
+	struct list_head *list_head = use_probed_modes ?
+						    &aconnector->base.probed_modes :
+						    &aconnector->base.modes;
+
+	if (aconnector->freesync_vid_base.clock != 0)
+		return &aconnector->freesync_vid_base;
+
+	/* Find the preferred mode */
+	list_for_each_entry (m, list_head, head) {
+		if (m->type & DRM_MODE_TYPE_PREFERRED) {
+			m_pref = m;
+			break;
+		}
+	}
+
+	if (!m_pref) {
+		/* Probably an EDID with no preferred mode. Fallback to first entry */
+		m_pref = list_first_entry_or_null(
+			&aconnector->base.modes, struct drm_display_mode, head);
+		if (!m_pref) {
+			DRM_DEBUG_DRIVER("No preferred mode found in EDID\n");
+			return NULL;
+		}
+	}
+
+	highest_refresh = drm_mode_vrefresh(m_pref);
+
+	/*
+	 * Find the mode with highest refresh rate with same resolution.
+	 * For some monitors, preferred mode is not the mode with highest
+	 * supported refresh rate.
+	 */
+	list_for_each_entry (m, list_head, head) {
+		current_refresh  = drm_mode_vrefresh(m);
+
+		if (m->hdisplay == m_pref->hdisplay &&
+		    m->vdisplay == m_pref->vdisplay &&
+		    highest_refresh < current_refresh) {
+			highest_refresh = current_refresh;
+			m_pref = m;
+		}
+	}
+
+	aconnector->freesync_vid_base = *m_pref;
+	return m_pref;
+}
+
 static struct dc_stream_state *
 create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		       const struct drm_display_mode *drm_mode,
@@ -6999,6 +7052,119 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
 	}
 }
 
+static bool is_duplicate_mode(struct amdgpu_dm_connector *aconnector,
+			      struct drm_display_mode *mode)
+{
+	struct drm_display_mode *m;
+
+	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
+		if (drm_mode_equal(m, mode))
+			return true;
+	}
+
+	return false;
+}
+
+static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
+			 struct detailed_data_monitor_range *range)
+{
+	const struct drm_display_mode *m;
+	struct drm_display_mode *new_mode;
+	uint i;
+	uint64_t target_vtotal, target_vtotal_diff;
+	uint32_t new_modes_count = 0;
+	uint64_t num, den;
+
+	/* Standard FPS values
+	 *
+	 * 23.976 - TV/NTSC
+	 * 24	  - Cinema
+	 * 25     - TV/PAL
+	 * 29.97  - TV/NTSC
+	 * 30     - TV/NTSC
+	 * 48	  - Cinema HFR
+	 * 50	  - TV/PAL
+	 */
+	const uint32_t common_rates[] = { 23976, 24000, 25000, 29970, 30000,
+					 48000, 50000, 60000, 72000, 96000 };
+
+	/*
+	 * Find mode with highest refresh rate with the same resolution
+	 * as the preferred mode. Some monitors report a preferred mode
+	 * with lower resolution than the highest refresh rate supported.
+	 */
+
+	m = get_highest_refresh_rate_mode(aconnector, true);
+	if (!m)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(common_rates); i++) {
+		if (drm_mode_vrefresh(m) * 1000 < common_rates[i])
+			continue;
+
+		if (common_rates[i] < range->min_vfreq * 1000)
+			continue;
+
+		num = (unsigned long long)m->clock * 1000 * 1000;
+		den = common_rates[i] * (unsigned long long)m->htotal;
+		target_vtotal = div_u64(num, den);
+		target_vtotal_diff = target_vtotal - m->vtotal;
+
+		/* Check for illegal modes */
+		if (m->vsync_start + target_vtotal_diff < m->vdisplay ||
+		    m->vsync_end + target_vtotal_diff < m->vsync_start ||
+		    m->vtotal + target_vtotal_diff < m->vsync_end)
+			continue;
+
+		new_mode = drm_mode_duplicate(aconnector->base.dev, m);
+		if (!new_mode)
+			goto out;
+
+		new_mode->vtotal += (u16)target_vtotal_diff;
+		new_mode->vsync_start += (u16)target_vtotal_diff;
+		new_mode->vsync_end += (u16)target_vtotal_diff;
+		new_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
+		new_mode->type |= DRM_MODE_TYPE_DRIVER;
+
+		if (!is_duplicate_mode(aconnector, new_mode)) {
+			drm_mode_probed_add(&aconnector->base, new_mode);
+			new_modes_count += 1;
+		} else
+			drm_mode_destroy(aconnector->base.dev, new_mode);
+	}
+ out:
+	return new_modes_count;
+}
+
+static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
+						   struct edid *edid)
+{
+	uint8_t i;
+	struct detailed_timing *timing;
+	struct detailed_non_pixel *data;
+	struct detailed_data_monitor_range *range;
+	struct amdgpu_dm_connector *amdgpu_dm_connector =
+		to_amdgpu_dm_connector(connector);
+
+	if (!(amdgpu_exp_freesync_vid_mode && edid))
+		return;
+
+	if (edid->version == 1 && edid->revision > 1) {
+		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 &&
+			    range->max_vfreq - range->min_vfreq > 10) {
+				amdgpu_dm_connector->num_modes += add_fs_modes(amdgpu_dm_connector, range);
+				break;
+			}
+		}
+	}
+}
+
 static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
 {
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
@@ -7014,6 +7180,7 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
 	} else {
 		amdgpu_dm_connector_ddc_get_modes(connector, edid);
 		amdgpu_dm_connector_add_common_modes(encoder, connector);
+		amdgpu_dm_connector_add_freesync_modes(connector, edid);
 	}
 	amdgpu_dm_fbc_init(connector);
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 251af783f6b1..f5a5f1ad7dba 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -413,6 +413,8 @@ struct amdgpu_dm_connector {
 #endif
 	bool force_yuv420_output;
 	struct dsc_preferred_settings dsc_settings;
+	/* Cached display modes */
+	struct drm_display_mode freesync_vid_base;
 };
 
 #define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
-- 
2.29.2

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

[PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change

[Aurabindo Pillai]

[Why&How]
Inorder to enable freesync video mode, driver adds extra
modes based on preferred modes for common freesync frame rates.
When commiting these mode changes, a full modeset is not needed.
If the change in only in the front porch timing value, skip full
modeset and continue using the same stream.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig at amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 210 +++++++++++++++---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 2 files changed, 179 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index c1ffd33e9d83..4ba6be59dcd5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
 static const struct drm_format_info *
 amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
 
+static bool
+is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
+				 struct drm_crtc_state *new_crtc_state);
 /*
  * dm_vblank_get_counter
  *
@@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode(
 	const struct drm_connector *connector,
 	const struct drm_connector_state *connector_state,
 	const struct dc_stream_state *old_stream,
-	int requested_bpc)
+	int requested_bpc,
+	bool is_in_modeset)
 {
 	struct dc_crtc_timing *timing_out = &stream->timing;
 	const struct drm_display_info *info = &connector->display_info;
@@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode(
 		timing_out->hdmi_vic = hv_frame.vic;
 	}
 
-	timing_out->h_addressable = mode_in->crtc_hdisplay;
-	timing_out->h_total = mode_in->crtc_htotal;
-	timing_out->h_sync_width =
-		mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
-	timing_out->h_front_porch =
-		mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
-	timing_out->v_total = mode_in->crtc_vtotal;
-	timing_out->v_addressable = mode_in->crtc_vdisplay;
-	timing_out->v_front_porch =
-		mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
-	timing_out->v_sync_width =
-		mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
-	timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
+	if (is_in_modeset) {
+		timing_out->h_addressable = mode_in->hdisplay;
+		timing_out->h_total = mode_in->htotal;
+		timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start;
+		timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay;
+		timing_out->v_total = mode_in->vtotal;
+		timing_out->v_addressable = mode_in->vdisplay;
+		timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay;
+		timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start;
+		timing_out->pix_clk_100hz = mode_in->clock * 10;
+	} else {
+		timing_out->h_addressable = mode_in->crtc_hdisplay;
+		timing_out->h_total = mode_in->crtc_htotal;
+		timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
+		timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
+		timing_out->v_total = mode_in->crtc_vtotal;
+		timing_out->v_addressable = mode_in->crtc_vdisplay;
+		timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
+		timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
+		timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
+	}
+
 	timing_out->aspect_ratio = get_aspect_ratio(mode_in);
 
 	stream->output_color_space = get_output_color_space(timing_out);
@@ -5227,6 +5240,24 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
 	return m_pref;
 }
 
+static bool is_freesync_video_mode(struct drm_display_mode *mode,
+				   struct amdgpu_dm_connector *aconnector)
+{
+	struct drm_display_mode *high_mode;
+
+	high_mode = get_highest_refresh_rate_mode(aconnector, false);
+	if (!high_mode || !mode)
+		return false;
+
+	if (high_mode->clock == 0 ||
+	    high_mode->hdisplay != mode->hdisplay ||
+	    high_mode->vdisplay != mode->vdisplay ||
+	    high_mode->clock != mode->clock)
+		return false;
+	else
+		return true;
+}
+
 static struct dc_stream_state *
 create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		       const struct drm_display_mode *drm_mode,
@@ -5240,15 +5271,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		dm_state ? &dm_state->base : NULL;
 	struct dc_stream_state *stream = NULL;
 	struct drm_display_mode mode = *drm_mode;
+	struct drm_display_mode saved_mode;
+	struct drm_display_mode *freesync_mode = NULL;
 	bool native_mode_found = false;
 	bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
 	int mode_refresh;
 	int preferred_refresh = 0;
+	bool is_fs_vid_mode = 0;
 #if defined(CONFIG_DRM_AMD_DC_DCN)
 	struct dsc_dec_dpcd_caps dsc_caps;
 	uint32_t link_bandwidth_kbps;
 #endif
 	struct dc_sink *sink = NULL;
+
+	memset(&saved_mode, 0, sizeof(struct drm_display_mode));
+
 	if (aconnector == NULL) {
 		DRM_ERROR("aconnector is NULL!\n");
 		return stream;
@@ -5301,25 +5338,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		 */
 		DRM_DEBUG_DRIVER("No preferred mode found\n");
 	} else {
-		decide_crtc_timing_for_drm_display_mode(
+		is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
+		if (is_fs_vid_mode) {
+			freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
+			saved_mode = mode;
+			mode = *freesync_mode;
+		}
+
+		if (!is_fs_vid_mode)
+			decide_crtc_timing_for_drm_display_mode(
 				&mode, preferred_mode,
 				dm_state ? (dm_state->scaling != RMX_OFF) : false);
+
 		preferred_refresh = drm_mode_vrefresh(preferred_mode);
 	}
 
 	if (!dm_state)
 		drm_mode_set_crtcinfo(&mode, 0);
 
-	/*
+	if (dm_state && is_fs_vid_mode)
+		drm_mode_set_crtcinfo(&saved_mode, 0);
+
+       /*
 	* If scaling is enabled and refresh rate didn't change
 	* we copy the vic and polarities of the old timings
 	*/
-	if (!scale || mode_refresh != preferred_refresh)
-		fill_stream_properties_from_drm_display_mode(stream,
-			&mode, &aconnector->base, con_state, NULL, requested_bpc);
+	if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh)
+		fill_stream_properties_from_drm_display_mode(
+			stream, &mode, &aconnector->base, con_state, NULL,
+			requested_bpc, dm_state ? 1 : 0);
 	else
-		fill_stream_properties_from_drm_display_mode(stream,
-			&mode, &aconnector->base, con_state, old_stream, requested_bpc);
+		fill_stream_properties_from_drm_display_mode(
+			stream, &mode, &aconnector->base, con_state, old_stream,
+			requested_bpc, dm_state ? 1 : 0);
 
 	stream->timing.flags.DSC = 0;
 
@@ -7844,13 +7895,29 @@ static void update_stream_irq_parameters(
 	if (new_crtc_state->vrr_supported &&
 	    config.min_refresh_in_uhz &&
 	    config.max_refresh_in_uhz) {
+		/*
+		 * if freesync compatible mode was set, config.state will be set
+		 * in atomic check
+		 */
+		if (config.state == VRR_STATE_ACTIVE_FIXED &&
+		    config.fixed_refresh_in_uhz && config.max_refresh_in_uhz &&
+		    config.min_refresh_in_uhz &&
+		    (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
+		     new_crtc_state->freesync_video_mode)) {
+			vrr_params.max_refresh_in_uhz = config.max_refresh_in_uhz;
+			vrr_params.min_refresh_in_uhz = config.min_refresh_in_uhz;
+			vrr_params.fixed_refresh_in_uhz = config.fixed_refresh_in_uhz;
+			vrr_params.state = VRR_STATE_ACTIVE_FIXED;
+			goto out;
+		}
+
 		config.state = new_crtc_state->base.vrr_enabled ?
 			VRR_STATE_ACTIVE_VARIABLE :
 			VRR_STATE_INACTIVE;
-	} else {
+	} else
 		config.state = VRR_STATE_UNSUPPORTED;
-	}
 
+out:
 	mod_freesync_build_vrr_params(dm->freesync_module,
 				      new_stream,
 				      &config, &vrr_params);
@@ -8168,7 +8235,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		 * as part of commit.
 		 */
 		if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
-		    amdgpu_dm_vrr_active(acrtc_state)) {
+		    amdgpu_dm_vrr_active(acrtc_state) ||
+		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
+		    acrtc_state->freesync_video_mode) {
 			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
 			dc_stream_adjust_vmin_vmax(
 				dm->dc, acrtc_state->stream,
@@ -8864,6 +8933,7 @@ static void get_freesync_config_for_crtc(
 			to_amdgpu_dm_connector(new_con_state->base.connector);
 	struct drm_display_mode *mode = &new_crtc_state->base.mode;
 	int vrefresh = drm_mode_vrefresh(mode);
+	bool fs_vid_mode = false;
 
 	new_crtc_state->vrr_supported = new_con_state->freesync_capable &&
 					vrefresh >= aconnector->min_vfreq &&
@@ -8871,17 +8941,26 @@ static void get_freesync_config_for_crtc(
 
 	if (new_crtc_state->vrr_supported) {
 		new_crtc_state->stream->ignore_msa_timing_param = true;
-		config.state = new_crtc_state->base.vrr_enabled ?
-				VRR_STATE_ACTIVE_VARIABLE :
-				VRR_STATE_INACTIVE;
-		config.min_refresh_in_uhz =
-				aconnector->min_vfreq * 1000000;
-		config.max_refresh_in_uhz =
-				aconnector->max_vfreq * 1000000;
+		fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
+			new_crtc_state->freesync_video_mode;
+
+		config.min_refresh_in_uhz = aconnector->min_vfreq * 1000000;
+		config.max_refresh_in_uhz = aconnector->max_vfreq * 1000000;
 		config.vsif_supported = true;
 		config.btr = true;
-	}
 
+		if (fs_vid_mode) {
+			config.state = VRR_STATE_ACTIVE_FIXED;
+			config.fixed_refresh_in_uhz = new_crtc_state->freesync_config.fixed_refresh_in_uhz;
+			goto out;
+		}
+		else if (new_crtc_state->base.vrr_enabled && !fs_vid_mode)
+			config.state = VRR_STATE_ACTIVE_VARIABLE;
+		else
+			config.state = VRR_STATE_INACTIVE;
+
+	}
+out:
 	new_crtc_state->freesync_config = config;
 }
 
@@ -8894,6 +8973,51 @@ static void reset_freesync_config_for_crtc(
 	       sizeof(new_crtc_state->vrr_infopacket));
 }
 
+static bool
+is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
+				 struct drm_crtc_state *new_crtc_state)
+{
+	struct drm_display_mode old_mode, new_mode;
+
+	if (!old_crtc_state || !new_crtc_state)
+		return false;
+
+	old_mode = old_crtc_state->mode;
+	new_mode = new_crtc_state->mode;
+
+	if (old_mode.clock       == new_mode.clock &&
+	    old_mode.hdisplay    == new_mode.hdisplay &&
+	    old_mode.vdisplay    == new_mode.vdisplay &&
+	    old_mode.htotal      == new_mode.htotal &&
+	    old_mode.vtotal      != new_mode.vtotal &&
+	    old_mode.hsync_start == new_mode.hsync_start &&
+	    old_mode.vsync_start != new_mode.vsync_start &&
+	    old_mode.hsync_end   == new_mode.hsync_end &&
+	    old_mode.vsync_end   != new_mode.vsync_end &&
+	    old_mode.hskew       == new_mode.hskew &&
+	    old_mode.vscan       == new_mode.vscan &&
+	    (old_mode.vsync_end - old_mode.vsync_start) ==
+	    (new_mode.vsync_end - new_mode.vsync_start))
+		return true;
+
+	return false;
+}
+
+static void set_freesync_fixed_config(struct dm_crtc_state *dm_new_crtc_state) {
+	uint64_t num, den, res;
+	struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state->base;
+
+	dm_new_crtc_state->freesync_config.state = VRR_STATE_ACTIVE_FIXED;
+
+	num = (unsigned long long)new_crtc_state->mode.clock * 1000 * 1000000;
+	den = (unsigned long long)new_crtc_state->mode.htotal *
+	      (unsigned long long)new_crtc_state->mode.vtotal;
+
+	res = div_u64(num, den);
+	dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz = res;
+	dm_new_crtc_state->freesync_video_mode = true;
+}
+
 static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 				struct drm_atomic_state *state,
 				struct drm_crtc *crtc,
@@ -8984,6 +9108,11 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 		 * TODO: Refactor this function to allow this check to work
 		 * in all conditions.
 		 */
+		if (dm_new_crtc_state->stream &&
+		    is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
+		    amdgpu_exp_freesync_vid_mode)
+			goto skip_modeset;
+
 		if (dm_new_crtc_state->stream &&
 		    dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
 		    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
@@ -9015,6 +9144,23 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 		if (!dm_old_crtc_state->stream)
 			goto skip_modeset;
 
+		if (dm_new_crtc_state->stream &&
+		    is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
+		    amdgpu_exp_freesync_vid_mode) {
+			new_crtc_state->mode_changed = false;
+			DRM_DEBUG_DRIVER(
+				"Mode change not required for front porch change, "
+				"setting mode_changed to %d",
+				new_crtc_state->mode_changed);
+
+			set_freesync_fixed_config(dm_new_crtc_state);
+
+			goto skip_modeset;
+		} else if (aconnector &&
+			   is_freesync_video_mode(&new_crtc_state->mode, aconnector) &&
+			   amdgpu_exp_freesync_vid_mode)
+			set_freesync_fixed_config(dm_new_crtc_state);
+
 		ret = dm_atomic_get_state(state, &dm_state);
 		if (ret)
 			goto fail;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index f5a5f1ad7dba..fd5fb9dafded 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -455,6 +455,7 @@ struct dm_crtc_state {
 
 	bool freesync_timing_changed;
 	bool freesync_vrr_info_changed;
+	bool freesync_video_mode;
 
 	bool dsc_force_changed;
 	bool vrr_supported;
-- 
2.29.2

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

[PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change

[Aurabindo Pillai]

[Why&How]
Inorder to enable freesync video mode, driver adds extra
modes based on preferred modes for common freesync frame rates.
When commiting these mode changes, a full modeset is not needed.
If the change in only in the front porch timing value, skip full
modeset and continue using the same stream.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig at amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 210 +++++++++++++++---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 2 files changed, 179 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index c1ffd33e9d83..4ba6be59dcd5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
 static const struct drm_format_info *
 amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
 
+static bool
+is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
+				 struct drm_crtc_state *new_crtc_state);
 /*
  * dm_vblank_get_counter
  *
@@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode(
 	const struct drm_connector *connector,
 	const struct drm_connector_state *connector_state,
 	const struct dc_stream_state *old_stream,
-	int requested_bpc)
+	int requested_bpc,
+	bool is_in_modeset)
 {
 	struct dc_crtc_timing *timing_out = &stream->timing;
 	const struct drm_display_info *info = &connector->display_info;
@@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode(
 		timing_out->hdmi_vic = hv_frame.vic;
 	}
 
-	timing_out->h_addressable = mode_in->crtc_hdisplay;
-	timing_out->h_total = mode_in->crtc_htotal;
-	timing_out->h_sync_width =
-		mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
-	timing_out->h_front_porch =
-		mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
-	timing_out->v_total = mode_in->crtc_vtotal;
-	timing_out->v_addressable = mode_in->crtc_vdisplay;
-	timing_out->v_front_porch =
-		mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
-	timing_out->v_sync_width =
-		mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
-	timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
+	if (is_in_modeset) {
+		timing_out->h_addressable = mode_in->hdisplay;
+		timing_out->h_total = mode_in->htotal;
+		timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start;
+		timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay;
+		timing_out->v_total = mode_in->vtotal;
+		timing_out->v_addressable = mode_in->vdisplay;
+		timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay;
+		timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start;
+		timing_out->pix_clk_100hz = mode_in->clock * 10;
+	} else {
+		timing_out->h_addressable = mode_in->crtc_hdisplay;
+		timing_out->h_total = mode_in->crtc_htotal;
+		timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
+		timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
+		timing_out->v_total = mode_in->crtc_vtotal;
+		timing_out->v_addressable = mode_in->crtc_vdisplay;
+		timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
+		timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
+		timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
+	}
+
 	timing_out->aspect_ratio = get_aspect_ratio(mode_in);
 
 	stream->output_color_space = get_output_color_space(timing_out);
@@ -5227,6 +5240,24 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
 	return m_pref;
 }
 
+static bool is_freesync_video_mode(struct drm_display_mode *mode,
+				   struct amdgpu_dm_connector *aconnector)
+{
+	struct drm_display_mode *high_mode;
+
+	high_mode = get_highest_refresh_rate_mode(aconnector, false);
+	if (!high_mode || !mode)
+		return false;
+
+	if (high_mode->clock == 0 ||
+	    high_mode->hdisplay != mode->hdisplay ||
+	    high_mode->vdisplay != mode->vdisplay ||
+	    high_mode->clock != mode->clock)
+		return false;
+	else
+		return true;
+}
+
 static struct dc_stream_state *
 create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		       const struct drm_display_mode *drm_mode,
@@ -5240,15 +5271,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		dm_state ? &dm_state->base : NULL;
 	struct dc_stream_state *stream = NULL;
 	struct drm_display_mode mode = *drm_mode;
+	struct drm_display_mode saved_mode;
+	struct drm_display_mode *freesync_mode = NULL;
 	bool native_mode_found = false;
 	bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
 	int mode_refresh;
 	int preferred_refresh = 0;
+	bool is_fs_vid_mode = 0;
 #if defined(CONFIG_DRM_AMD_DC_DCN)
 	struct dsc_dec_dpcd_caps dsc_caps;
 	uint32_t link_bandwidth_kbps;
 #endif
 	struct dc_sink *sink = NULL;
+
+	memset(&saved_mode, 0, sizeof(struct drm_display_mode));
+
 	if (aconnector == NULL) {
 		DRM_ERROR("aconnector is NULL!\n");
 		return stream;
@@ -5301,25 +5338,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		 */
 		DRM_DEBUG_DRIVER("No preferred mode found\n");
 	} else {
-		decide_crtc_timing_for_drm_display_mode(
+		is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
+		if (is_fs_vid_mode) {
+			freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
+			saved_mode = mode;
+			mode = *freesync_mode;
+		}
+
+		if (!is_fs_vid_mode)
+			decide_crtc_timing_for_drm_display_mode(
 				&mode, preferred_mode,
 				dm_state ? (dm_state->scaling != RMX_OFF) : false);
+
 		preferred_refresh = drm_mode_vrefresh(preferred_mode);
 	}
 
 	if (!dm_state)
 		drm_mode_set_crtcinfo(&mode, 0);
 
-	/*
+	if (dm_state && is_fs_vid_mode)
+		drm_mode_set_crtcinfo(&saved_mode, 0);
+
+       /*
 	* If scaling is enabled and refresh rate didn't change
 	* we copy the vic and polarities of the old timings
 	*/
-	if (!scale || mode_refresh != preferred_refresh)
-		fill_stream_properties_from_drm_display_mode(stream,
-			&mode, &aconnector->base, con_state, NULL, requested_bpc);
+	if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh)
+		fill_stream_properties_from_drm_display_mode(
+			stream, &mode, &aconnector->base, con_state, NULL,
+			requested_bpc, dm_state ? 1 : 0);
 	else
-		fill_stream_properties_from_drm_display_mode(stream,
-			&mode, &aconnector->base, con_state, old_stream, requested_bpc);
+		fill_stream_properties_from_drm_display_mode(
+			stream, &mode, &aconnector->base, con_state, old_stream,
+			requested_bpc, dm_state ? 1 : 0);
 
 	stream->timing.flags.DSC = 0;
 
@@ -7844,13 +7895,29 @@ static void update_stream_irq_parameters(
 	if (new_crtc_state->vrr_supported &&
 	    config.min_refresh_in_uhz &&
 	    config.max_refresh_in_uhz) {
+		/*
+		 * if freesync compatible mode was set, config.state will be set
+		 * in atomic check
+		 */
+		if (config.state == VRR_STATE_ACTIVE_FIXED &&
+		    config.fixed_refresh_in_uhz && config.max_refresh_in_uhz &&
+		    config.min_refresh_in_uhz &&
+		    (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
+		     new_crtc_state->freesync_video_mode)) {
+			vrr_params.max_refresh_in_uhz = config.max_refresh_in_uhz;
+			vrr_params.min_refresh_in_uhz = config.min_refresh_in_uhz;
+			vrr_params.fixed_refresh_in_uhz = config.fixed_refresh_in_uhz;
+			vrr_params.state = VRR_STATE_ACTIVE_FIXED;
+			goto out;
+		}
+
 		config.state = new_crtc_state->base.vrr_enabled ?
 			VRR_STATE_ACTIVE_VARIABLE :
 			VRR_STATE_INACTIVE;
-	} else {
+	} else
 		config.state = VRR_STATE_UNSUPPORTED;
-	}
 
+out:
 	mod_freesync_build_vrr_params(dm->freesync_module,
 				      new_stream,
 				      &config, &vrr_params);
@@ -8168,7 +8235,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		 * as part of commit.
 		 */
 		if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
-		    amdgpu_dm_vrr_active(acrtc_state)) {
+		    amdgpu_dm_vrr_active(acrtc_state) ||
+		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
+		    acrtc_state->freesync_video_mode) {
 			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
 			dc_stream_adjust_vmin_vmax(
 				dm->dc, acrtc_state->stream,
@@ -8864,6 +8933,7 @@ static void get_freesync_config_for_crtc(
 			to_amdgpu_dm_connector(new_con_state->base.connector);
 	struct drm_display_mode *mode = &new_crtc_state->base.mode;
 	int vrefresh = drm_mode_vrefresh(mode);
+	bool fs_vid_mode = false;
 
 	new_crtc_state->vrr_supported = new_con_state->freesync_capable &&
 					vrefresh >= aconnector->min_vfreq &&
@@ -8871,17 +8941,26 @@ static void get_freesync_config_for_crtc(
 
 	if (new_crtc_state->vrr_supported) {
 		new_crtc_state->stream->ignore_msa_timing_param = true;
-		config.state = new_crtc_state->base.vrr_enabled ?
-				VRR_STATE_ACTIVE_VARIABLE :
-				VRR_STATE_INACTIVE;
-		config.min_refresh_in_uhz =
-				aconnector->min_vfreq * 1000000;
-		config.max_refresh_in_uhz =
-				aconnector->max_vfreq * 1000000;
+		fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
+			new_crtc_state->freesync_video_mode;
+
+		config.min_refresh_in_uhz = aconnector->min_vfreq * 1000000;
+		config.max_refresh_in_uhz = aconnector->max_vfreq * 1000000;
 		config.vsif_supported = true;
 		config.btr = true;
-	}
 
+		if (fs_vid_mode) {
+			config.state = VRR_STATE_ACTIVE_FIXED;
+			config.fixed_refresh_in_uhz = new_crtc_state->freesync_config.fixed_refresh_in_uhz;
+			goto out;
+		}
+		else if (new_crtc_state->base.vrr_enabled && !fs_vid_mode)
+			config.state = VRR_STATE_ACTIVE_VARIABLE;
+		else
+			config.state = VRR_STATE_INACTIVE;
+
+	}
+out:
 	new_crtc_state->freesync_config = config;
 }
 
@@ -8894,6 +8973,51 @@ static void reset_freesync_config_for_crtc(
 	       sizeof(new_crtc_state->vrr_infopacket));
 }
 
+static bool
+is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
+				 struct drm_crtc_state *new_crtc_state)
+{
+	struct drm_display_mode old_mode, new_mode;
+
+	if (!old_crtc_state || !new_crtc_state)
+		return false;
+
+	old_mode = old_crtc_state->mode;
+	new_mode = new_crtc_state->mode;
+
+	if (old_mode.clock       == new_mode.clock &&
+	    old_mode.hdisplay    == new_mode.hdisplay &&
+	    old_mode.vdisplay    == new_mode.vdisplay &&
+	    old_mode.htotal      == new_mode.htotal &&
+	    old_mode.vtotal      != new_mode.vtotal &&
+	    old_mode.hsync_start == new_mode.hsync_start &&
+	    old_mode.vsync_start != new_mode.vsync_start &&
+	    old_mode.hsync_end   == new_mode.hsync_end &&
+	    old_mode.vsync_end   != new_mode.vsync_end &&
+	    old_mode.hskew       == new_mode.hskew &&
+	    old_mode.vscan       == new_mode.vscan &&
+	    (old_mode.vsync_end - old_mode.vsync_start) ==
+	    (new_mode.vsync_end - new_mode.vsync_start))
+		return true;
+
+	return false;
+}
+
+static void set_freesync_fixed_config(struct dm_crtc_state *dm_new_crtc_state) {
+	uint64_t num, den, res;
+	struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state->base;
+
+	dm_new_crtc_state->freesync_config.state = VRR_STATE_ACTIVE_FIXED;
+
+	num = (unsigned long long)new_crtc_state->mode.clock * 1000 * 1000000;
+	den = (unsigned long long)new_crtc_state->mode.htotal *
+	      (unsigned long long)new_crtc_state->mode.vtotal;
+
+	res = div_u64(num, den);
+	dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz = res;
+	dm_new_crtc_state->freesync_video_mode = true;
+}
+
 static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 				struct drm_atomic_state *state,
 				struct drm_crtc *crtc,
@@ -8984,6 +9108,11 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 		 * TODO: Refactor this function to allow this check to work
 		 * in all conditions.
 		 */
+		if (dm_new_crtc_state->stream &&
+		    is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
+		    amdgpu_exp_freesync_vid_mode)
+			goto skip_modeset;
+
 		if (dm_new_crtc_state->stream &&
 		    dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
 		    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
@@ -9015,6 +9144,23 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 		if (!dm_old_crtc_state->stream)
 			goto skip_modeset;
 
+		if (dm_new_crtc_state->stream &&
+		    is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
+		    amdgpu_exp_freesync_vid_mode) {
+			new_crtc_state->mode_changed = false;
+			DRM_DEBUG_DRIVER(
+				"Mode change not required for front porch change, "
+				"setting mode_changed to %d",
+				new_crtc_state->mode_changed);
+
+			set_freesync_fixed_config(dm_new_crtc_state);
+
+			goto skip_modeset;
+		} else if (aconnector &&
+			   is_freesync_video_mode(&new_crtc_state->mode, aconnector) &&
+			   amdgpu_exp_freesync_vid_mode)
+			set_freesync_fixed_config(dm_new_crtc_state);
+
 		ret = dm_atomic_get_state(state, &dm_state);
 		if (ret)
 			goto fail;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index f5a5f1ad7dba..fd5fb9dafded 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -455,6 +455,7 @@ struct dm_crtc_state {
 
 	bool freesync_timing_changed;
 	bool freesync_vrr_info_changed;
+	bool freesync_video_mode;
 
 	bool dsc_force_changed;
 	bool vrr_supported;
-- 
2.29.2

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

Re: [PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes

[Shashank Sharma]

On 15/12/20 3:50 am, Aurabindo Pillai wrote:
> [Why&How]
> If experimental freesync video mode module parameter is enabled, add
> few extra display modes into the driver's mode list corresponding to common
> video frame rates. When userspace sets these modes, no modeset will be
> performed (if current mode was one of freesync modes or the base freesync mode
> based off which timings have been generated for the rest of the freesync modes)
> since these modes only differ from the base mode with front porch timing.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Acked-by: Christian König <christian.koenig at amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167 ++++++++++++++++++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   2 +
>  2 files changed, 169 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index e7ee2467eadb..c1ffd33e9d83 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5174,6 +5174,59 @@ static void dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
>  	set_master_stream(context->streams, context->stream_count);
>  }
>  
> +static struct drm_display_mode *
> +get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
> +			  bool use_probed_modes)
> +{
> +	struct drm_display_mode *m, *m_pref = NULL;
> +	u16 current_refresh, highest_refresh;
> +	struct list_head *list_head = use_probed_modes ?
> +						    &aconnector->base.probed_modes :
> +						    &aconnector->base.modes;
> +
> +	if (aconnector->freesync_vid_base.clock != 0)
> +		return &aconnector->freesync_vid_base;
> +
> +	/* Find the preferred mode */
> +	list_for_each_entry (m, list_head, head) {
> +		if (m->type & DRM_MODE_TYPE_PREFERRED) {
> +			m_pref = m;
> +			break;
> +		}
> +	}
> +
> +	if (!m_pref) {
> +		/* Probably an EDID with no preferred mode. Fallback to first entry */
> +		m_pref = list_first_entry_or_null(
> +			&aconnector->base.modes, struct drm_display_mode, head);
> +		if (!m_pref) {
> +			DRM_DEBUG_DRIVER("No preferred mode found in EDID\n");
> +			return NULL;
> +		}
> +	}
> +
> +	highest_refresh = drm_mode_vrefresh(m_pref);
> +
> +	/*
> +	 * Find the mode with highest refresh rate with same resolution.
> +	 * For some monitors, preferred mode is not the mode with highest
> +	 * supported refresh rate.
> +	 */
> +	list_for_each_entry (m, list_head, head) {
> +		current_refresh  = drm_mode_vrefresh(m);
> +
> +		if (m->hdisplay == m_pref->hdisplay &&
> +		    m->vdisplay == m_pref->vdisplay &&
> +		    highest_refresh < current_refresh) {
> +			highest_refresh = current_refresh;
> +			m_pref = m;
> +		}
> +	}
> +
> +	aconnector->freesync_vid_base = *m_pref;
> +	return m_pref;
> +}
> +
>  static struct dc_stream_state *
>  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>  		       const struct drm_display_mode *drm_mode,
> @@ -6999,6 +7052,119 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
>  	}
>  }
>  
> +static bool is_duplicate_mode(struct amdgpu_dm_connector *aconnector,
> +			      struct drm_display_mode *mode)
> +{
> +	struct drm_display_mode *m;
> +
> +	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
> +		if (drm_mode_equal(m, mode))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
> +			 struct detailed_data_monitor_range *range)
> +{
> +	const struct drm_display_mode *m;
> +	struct drm_display_mode *new_mode;
> +	uint i;
> +	uint64_t target_vtotal, target_vtotal_diff;
> +	uint32_t new_modes_count = 0;
> +	uint64_t num, den;
My previous review comment about num,den, target_vtotal and target_vtotal_diff here was neither addressed nor explained.
> +
> +	/* Standard FPS values
> +	 *
> +	 * 23.976 - TV/NTSC
> +	 * 24	  - Cinema
> +	 * 25     - TV/PAL
> +	 * 29.97  - TV/NTSC
> +	 * 30     - TV/NTSC
> +	 * 48	  - Cinema HFR
> +	 * 50	  - TV/PAL
> +	 */
My previous review comment about 60 fps here was neither addressed nor explained.
> +	const uint32_t common_rates[] = { 23976, 24000, 25000, 29970, 30000,
> +					 48000, 50000, 60000, 72000, 96000 };
> +
> +	/*
> +	 * Find mode with highest refresh rate with the same resolution
> +	 * as the preferred mode. Some monitors report a preferred mode
> +	 * with lower resolution than the highest refresh rate supported.
> +	 */
> +
> +	m = get_highest_refresh_rate_mode(aconnector, true);
> +	if (!m)
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(common_rates); i++) {
> +		if (drm_mode_vrefresh(m) * 1000 < common_rates[i])
> +			continue;
> +
> +		if (common_rates[i] < range->min_vfreq * 1000)
> +			continue;
> +
> +		num = (unsigned long long)m->clock * 1000 * 1000;
> +		den = common_rates[i] * (unsigned long long)m->htotal;
> +		target_vtotal = div_u64(num, den);
> +		target_vtotal_diff = target_vtotal - m->vtotal;
> +
> +		/* Check for illegal modes */
> +		if (m->vsync_start + target_vtotal_diff < m->vdisplay ||
> +		    m->vsync_end + target_vtotal_diff < m->vsync_start ||
> +		    m->vtotal + target_vtotal_diff < m->vsync_end)
> +			continue;
> +
> +		new_mode = drm_mode_duplicate(aconnector->base.dev, m);
> +		if (!new_mode)
> +			goto out;
> +
> +		new_mode->vtotal += (u16)target_vtotal_diff;
> +		new_mode->vsync_start += (u16)target_vtotal_diff;
> +		new_mode->vsync_end += (u16)target_vtotal_diff;
> +		new_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
> +		new_mode->type |= DRM_MODE_TYPE_DRIVER;
> +
> +		if (!is_duplicate_mode(aconnector, new_mode)) {
> +			drm_mode_probed_add(&aconnector->base, new_mode);
> +			new_modes_count += 1;
> +		} else
> +			drm_mode_destroy(aconnector->base.dev, new_mode);
> +	}
> + out:
> +	return new_modes_count;
> +}
> +
> +static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
> +						   struct edid *edid)
> +{
> +	uint8_t i;
> +	struct detailed_timing *timing;
> +	struct detailed_non_pixel *data;
> +	struct detailed_data_monitor_range *range;
> +	struct amdgpu_dm_connector *amdgpu_dm_connector =
> +		to_amdgpu_dm_connector(connector);
> +
> +	if (!(amdgpu_exp_freesync_vid_mode && edid))
> +		return;
> +
> +	if (edid->version == 1 && edid->revision > 1) {
> +		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 &&
> +			    range->max_vfreq - range->min_vfreq > 10) {
> +				amdgpu_dm_connector->num_modes += add_fs_modes(amdgpu_dm_connector, range);
> +				break;
> +			}
> +		}
> +	}
> +}
> +
>  static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
> @@ -7014,6 +7180,7 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>  	} else {
>  		amdgpu_dm_connector_ddc_get_modes(connector, edid);
>  		amdgpu_dm_connector_add_common_modes(encoder, connector);
> +		amdgpu_dm_connector_add_freesync_modes(connector, edid);
>  	}
>  	amdgpu_dm_fbc_init(connector);
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 251af783f6b1..f5a5f1ad7dba 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -413,6 +413,8 @@ struct amdgpu_dm_connector {
>  #endif
>  	bool force_yuv420_output;
>  	struct dsc_preferred_settings dsc_settings;
> +	/* Cached display modes */
> +	struct drm_display_mode freesync_vid_base;

With (or without) the missing comments addressed:

feel free to use: Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>

- Shashank

>  };
>  
>  #define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes

[Shashank Sharma]

On 15/12/20 3:50 am, Aurabindo Pillai wrote:
> [Why&How]
> If experimental freesync video mode module parameter is enabled, add
> few extra display modes into the driver's mode list corresponding to common
> video frame rates. When userspace sets these modes, no modeset will be
> performed (if current mode was one of freesync modes or the base freesync mode
> based off which timings have been generated for the rest of the freesync modes)
> since these modes only differ from the base mode with front porch timing.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Acked-by: Christian König <christian.koenig at amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167 ++++++++++++++++++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   2 +
>  2 files changed, 169 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index e7ee2467eadb..c1ffd33e9d83 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5174,6 +5174,59 @@ static void dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
>  	set_master_stream(context->streams, context->stream_count);
>  }
>  
> +static struct drm_display_mode *
> +get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
> +			  bool use_probed_modes)
> +{
> +	struct drm_display_mode *m, *m_pref = NULL;
> +	u16 current_refresh, highest_refresh;
> +	struct list_head *list_head = use_probed_modes ?
> +						    &aconnector->base.probed_modes :
> +						    &aconnector->base.modes;
> +
> +	if (aconnector->freesync_vid_base.clock != 0)
> +		return &aconnector->freesync_vid_base;
> +
> +	/* Find the preferred mode */
> +	list_for_each_entry (m, list_head, head) {
> +		if (m->type & DRM_MODE_TYPE_PREFERRED) {
> +			m_pref = m;
> +			break;
> +		}
> +	}
> +
> +	if (!m_pref) {
> +		/* Probably an EDID with no preferred mode. Fallback to first entry */
> +		m_pref = list_first_entry_or_null(
> +			&aconnector->base.modes, struct drm_display_mode, head);
> +		if (!m_pref) {
> +			DRM_DEBUG_DRIVER("No preferred mode found in EDID\n");
> +			return NULL;
> +		}
> +	}
> +
> +	highest_refresh = drm_mode_vrefresh(m_pref);
> +
> +	/*
> +	 * Find the mode with highest refresh rate with same resolution.
> +	 * For some monitors, preferred mode is not the mode with highest
> +	 * supported refresh rate.
> +	 */
> +	list_for_each_entry (m, list_head, head) {
> +		current_refresh  = drm_mode_vrefresh(m);
> +
> +		if (m->hdisplay == m_pref->hdisplay &&
> +		    m->vdisplay == m_pref->vdisplay &&
> +		    highest_refresh < current_refresh) {
> +			highest_refresh = current_refresh;
> +			m_pref = m;
> +		}
> +	}
> +
> +	aconnector->freesync_vid_base = *m_pref;
> +	return m_pref;
> +}
> +
>  static struct dc_stream_state *
>  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>  		       const struct drm_display_mode *drm_mode,
> @@ -6999,6 +7052,119 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
>  	}
>  }
>  
> +static bool is_duplicate_mode(struct amdgpu_dm_connector *aconnector,
> +			      struct drm_display_mode *mode)
> +{
> +	struct drm_display_mode *m;
> +
> +	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
> +		if (drm_mode_equal(m, mode))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
> +			 struct detailed_data_monitor_range *range)
> +{
> +	const struct drm_display_mode *m;
> +	struct drm_display_mode *new_mode;
> +	uint i;
> +	uint64_t target_vtotal, target_vtotal_diff;
> +	uint32_t new_modes_count = 0;
> +	uint64_t num, den;
My previous review comment about num,den, target_vtotal and target_vtotal_diff here was neither addressed nor explained.
> +
> +	/* Standard FPS values
> +	 *
> +	 * 23.976 - TV/NTSC
> +	 * 24	  - Cinema
> +	 * 25     - TV/PAL
> +	 * 29.97  - TV/NTSC
> +	 * 30     - TV/NTSC
> +	 * 48	  - Cinema HFR
> +	 * 50	  - TV/PAL
> +	 */
My previous review comment about 60 fps here was neither addressed nor explained.
> +	const uint32_t common_rates[] = { 23976, 24000, 25000, 29970, 30000,
> +					 48000, 50000, 60000, 72000, 96000 };
> +
> +	/*
> +	 * Find mode with highest refresh rate with the same resolution
> +	 * as the preferred mode. Some monitors report a preferred mode
> +	 * with lower resolution than the highest refresh rate supported.
> +	 */
> +
> +	m = get_highest_refresh_rate_mode(aconnector, true);
> +	if (!m)
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(common_rates); i++) {
> +		if (drm_mode_vrefresh(m) * 1000 < common_rates[i])
> +			continue;
> +
> +		if (common_rates[i] < range->min_vfreq * 1000)
> +			continue;
> +
> +		num = (unsigned long long)m->clock * 1000 * 1000;
> +		den = common_rates[i] * (unsigned long long)m->htotal;
> +		target_vtotal = div_u64(num, den);
> +		target_vtotal_diff = target_vtotal - m->vtotal;
> +
> +		/* Check for illegal modes */
> +		if (m->vsync_start + target_vtotal_diff < m->vdisplay ||
> +		    m->vsync_end + target_vtotal_diff < m->vsync_start ||
> +		    m->vtotal + target_vtotal_diff < m->vsync_end)
> +			continue;
> +
> +		new_mode = drm_mode_duplicate(aconnector->base.dev, m);
> +		if (!new_mode)
> +			goto out;
> +
> +		new_mode->vtotal += (u16)target_vtotal_diff;
> +		new_mode->vsync_start += (u16)target_vtotal_diff;
> +		new_mode->vsync_end += (u16)target_vtotal_diff;
> +		new_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
> +		new_mode->type |= DRM_MODE_TYPE_DRIVER;
> +
> +		if (!is_duplicate_mode(aconnector, new_mode)) {
> +			drm_mode_probed_add(&aconnector->base, new_mode);
> +			new_modes_count += 1;
> +		} else
> +			drm_mode_destroy(aconnector->base.dev, new_mode);
> +	}
> + out:
> +	return new_modes_count;
> +}
> +
> +static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
> +						   struct edid *edid)
> +{
> +	uint8_t i;
> +	struct detailed_timing *timing;
> +	struct detailed_non_pixel *data;
> +	struct detailed_data_monitor_range *range;
> +	struct amdgpu_dm_connector *amdgpu_dm_connector =
> +		to_amdgpu_dm_connector(connector);
> +
> +	if (!(amdgpu_exp_freesync_vid_mode && edid))
> +		return;
> +
> +	if (edid->version == 1 && edid->revision > 1) {
> +		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 &&
> +			    range->max_vfreq - range->min_vfreq > 10) {
> +				amdgpu_dm_connector->num_modes += add_fs_modes(amdgpu_dm_connector, range);
> +				break;
> +			}
> +		}
> +	}
> +}
> +
>  static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
> @@ -7014,6 +7180,7 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>  	} else {
>  		amdgpu_dm_connector_ddc_get_modes(connector, edid);
>  		amdgpu_dm_connector_add_common_modes(encoder, connector);
> +		amdgpu_dm_connector_add_freesync_modes(connector, edid);
>  	}
>  	amdgpu_dm_fbc_init(connector);
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 251af783f6b1..f5a5f1ad7dba 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -413,6 +413,8 @@ struct amdgpu_dm_connector {
>  #endif
>  	bool force_yuv420_output;
>  	struct dsc_preferred_settings dsc_settings;
> +	/* Cached display modes */
> +	struct drm_display_mode freesync_vid_base;

With (or without) the missing comments addressed:

feel free to use: Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>

- Shashank

>  };
>  
>  #define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change

[Shashank Sharma]

On 15/12/20 3:50 am, Aurabindo Pillai wrote:
> [Why&How]
> Inorder to enable freesync video mode, driver adds extra
> modes based on preferred modes for common freesync frame rates.
> When commiting these mode changes, a full modeset is not needed.
> If the change in only in the front porch timing value, skip full
> modeset and continue using the same stream.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Acked-by: Christian König <christian.koenig at amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 210 +++++++++++++++---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
>  2 files changed, 179 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index c1ffd33e9d83..4ba6be59dcd5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
>  static const struct drm_format_info *
>  amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>  
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> +				 struct drm_crtc_state *new_crtc_state);
Do we need this forward declaration for a static function ?
>  /*
>   * dm_vblank_get_counter
>   *
> @@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode(
>  	const struct drm_connector *connector,
>  	const struct drm_connector_state *connector_state,
>  	const struct dc_stream_state *old_stream,
> -	int requested_bpc)
> +	int requested_bpc,
> +	bool is_in_modeset)
>  {
>  	struct dc_crtc_timing *timing_out = &stream->timing;
>  	const struct drm_display_info *info = &connector->display_info;
> @@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode(
>  		timing_out->hdmi_vic = hv_frame.vic;
>  	}
>  
> -	timing_out->h_addressable = mode_in->crtc_hdisplay;
> -	timing_out->h_total = mode_in->crtc_htotal;
> -	timing_out->h_sync_width =
> -		mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
> -	timing_out->h_front_porch =
> -		mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
> -	timing_out->v_total = mode_in->crtc_vtotal;
> -	timing_out->v_addressable = mode_in->crtc_vdisplay;
> -	timing_out->v_front_porch =
> -		mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
> -	timing_out->v_sync_width =
> -		mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
> -	timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
> +	if (is_in_modeset) {
> +		timing_out->h_addressable = mode_in->hdisplay;
> +		timing_out->h_total = mode_in->htotal;
> +		timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start;
> +		timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay;
> +		timing_out->v_total = mode_in->vtotal;
> +		timing_out->v_addressable = mode_in->vdisplay;
> +		timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay;
> +		timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start;
> +		timing_out->pix_clk_100hz = mode_in->clock * 10;
> +	} else {
> +		timing_out->h_addressable = mode_in->crtc_hdisplay;
> +		timing_out->h_total = mode_in->crtc_htotal;
> +		timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
> +		timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
> +		timing_out->v_total = mode_in->crtc_vtotal;
> +		timing_out->v_addressable = mode_in->crtc_vdisplay;
> +		timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
> +		timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
> +		timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
> +	}
> +
>  	timing_out->aspect_ratio = get_aspect_ratio(mode_in);
>  
>  	stream->output_color_space = get_output_color_space(timing_out);
> @@ -5227,6 +5240,24 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
>  	return m_pref;
>  }
>  
> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> +				   struct amdgpu_dm_connector *aconnector)
> +{
> +	struct drm_display_mode *high_mode;
> +
> +	high_mode = get_highest_refresh_rate_mode(aconnector, false);
> +	if (!high_mode || !mode)
> +		return false;
> +
> +	if (high_mode->clock == 0 ||
> +	    high_mode->hdisplay != mode->hdisplay ||
> +	    high_mode->vdisplay != mode->vdisplay ||
> +	    high_mode->clock != mode->clock)
> +		return false;
> +	else
> +		return true;
> +}
> +
>  static struct dc_stream_state *
>  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>  		       const struct drm_display_mode *drm_mode,
> @@ -5240,15 +5271,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>  		dm_state ? &dm_state->base : NULL;
>  	struct dc_stream_state *stream = NULL;
>  	struct drm_display_mode mode = *drm_mode;
> +	struct drm_display_mode saved_mode;
> +	struct drm_display_mode *freesync_mode = NULL;
>  	bool native_mode_found = false;
>  	bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
>  	int mode_refresh;
>  	int preferred_refresh = 0;
> +	bool is_fs_vid_mode = 0;
>  #if defined(CONFIG_DRM_AMD_DC_DCN)
>  	struct dsc_dec_dpcd_caps dsc_caps;
>  	uint32_t link_bandwidth_kbps;
>  #endif
>  	struct dc_sink *sink = NULL;
> +
> +	memset(&saved_mode, 0, sizeof(struct drm_display_mode));
> +
>  	if (aconnector == NULL) {
>  		DRM_ERROR("aconnector is NULL!\n");
>  		return stream;
> @@ -5301,25 +5338,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>  		 */
>  		DRM_DEBUG_DRIVER("No preferred mode found\n");
>  	} else {
> -		decide_crtc_timing_for_drm_display_mode(
> +		is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
> +		if (is_fs_vid_mode) {
> +			freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
> +			saved_mode = mode;
> +			mode = *freesync_mode;
> +		}
> +
> +		if (!is_fs_vid_mode)
This should go as else {} case for above if() instead of a new check
> +			decide_crtc_timing_for_drm_display_mode(
>  				&mode, preferred_mode,
>  				dm_state ? (dm_state->scaling != RMX_OFF) : false);
> +
>  		preferred_refresh = drm_mode_vrefresh(preferred_mode);
>  	}
>  
>  	if (!dm_state)
>  		drm_mode_set_crtcinfo(&mode, 0);
>  
> -	/*
> +	if (dm_state && is_fs_vid_mode)
> +		drm_mode_set_crtcinfo(&saved_mode, 0);
Why is this step required ?
> +
> +       /*
>  	* If scaling is enabled and refresh rate didn't change
>  	* we copy the vic and polarities of the old timings
>  	*/
> -	if (!scale || mode_refresh != preferred_refresh)
> -		fill_stream_properties_from_drm_display_mode(stream,
> -			&mode, &aconnector->base, con_state, NULL, requested_bpc);
> +	if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh)
> +		fill_stream_properties_from_drm_display_mode(
> +			stream, &mode, &aconnector->base, con_state, NULL,
> +			requested_bpc, dm_state ? 1 : 0);
>  	else
> -		fill_stream_properties_from_drm_display_mode(stream,
> -			&mode, &aconnector->base, con_state, old_stream, requested_bpc);
> +		fill_stream_properties_from_drm_display_mode(
> +			stream, &mode, &aconnector->base, con_state, old_stream,
> +			requested_bpc, dm_state ? 1 : 0);
>  
>  	stream->timing.flags.DSC = 0;
>  
> @@ -7844,13 +7895,29 @@ static void update_stream_irq_parameters(
>  	if (new_crtc_state->vrr_supported &&
>  	    config.min_refresh_in_uhz &&
>  	    config.max_refresh_in_uhz) {
> +		/*
> +		 * if freesync compatible mode was set, config.state will be set
> +		 * in atomic check
> +		 */
> +		if (config.state == VRR_STATE_ACTIVE_FIXED &&
> +		    config.fixed_refresh_in_uhz && config.max_refresh_in_uhz &&
> +		    config.min_refresh_in_uhz &&
> +		    (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
> +		     new_crtc_state->freesync_video_mode)) {
> +			vrr_params.max_refresh_in_uhz = config.max_refresh_in_uhz;
> +			vrr_params.min_refresh_in_uhz = config.min_refresh_in_uhz;
> +			vrr_params.fixed_refresh_in_uhz = config.fixed_refresh_in_uhz;
> +			vrr_params.state = VRR_STATE_ACTIVE_FIXED;
> +			goto out;
> +		}
> +
>  		config.state = new_crtc_state->base.vrr_enabled ?
>  			VRR_STATE_ACTIVE_VARIABLE :
>  			VRR_STATE_INACTIVE;
> -	} else {
> +	} else
conventionally, if a if() condition has {}, its else condition should also have {}, even if it's a single statement. So you can skip this change.
>  		config.state = VRR_STATE_UNSUPPORTED;
> -	}
>  
> +out:
>  	mod_freesync_build_vrr_params(dm->freesync_module,
>  				      new_stream,
>  				      &config, &vrr_params);
> @@ -8168,7 +8235,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  		 * as part of commit.
>  		 */
>  		if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
> -		    amdgpu_dm_vrr_active(acrtc_state)) {
> +		    amdgpu_dm_vrr_active(acrtc_state) ||
> +		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> +		    acrtc_state->freesync_video_mode) {
>  			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
>  			dc_stream_adjust_vmin_vmax(
>  				dm->dc, acrtc_state->stream,
> @@ -8864,6 +8933,7 @@ static void get_freesync_config_for_crtc(
>  			to_amdgpu_dm_connector(new_con_state->base.connector);
>  	struct drm_display_mode *mode = &new_crtc_state->base.mode;
>  	int vrefresh = drm_mode_vrefresh(mode);
> +	bool fs_vid_mode = false;
>  
>  	new_crtc_state->vrr_supported = new_con_state->freesync_capable &&
>  					vrefresh >= aconnector->min_vfreq &&
> @@ -8871,17 +8941,26 @@ static void get_freesync_config_for_crtc(
>  
>  	if (new_crtc_state->vrr_supported) {
>  		new_crtc_state->stream->ignore_msa_timing_param = true;
> -		config.state = new_crtc_state->base.vrr_enabled ?
> -				VRR_STATE_ACTIVE_VARIABLE :
> -				VRR_STATE_INACTIVE;
> -		config.min_refresh_in_uhz =
> -				aconnector->min_vfreq * 1000000;
> -		config.max_refresh_in_uhz =
> -				aconnector->max_vfreq * 1000000;
> +		fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> +			new_crtc_state->freesync_video_mode;
> +
> +		config.min_refresh_in_uhz = aconnector->min_vfreq * 1000000;
> +		config.max_refresh_in_uhz = aconnector->max_vfreq * 1000000;
>  		config.vsif_supported = true;
>  		config.btr = true;
> -	}
>  
> +		if (fs_vid_mode) {
> +			config.state = VRR_STATE_ACTIVE_FIXED;
> +			config.fixed_refresh_in_uhz = new_crtc_state->freesync_config.fixed_refresh_in_uhz;
> +			goto out;
> +		}
> +		else if (new_crtc_state->base.vrr_enabled && !fs_vid_mode)
> +			config.state = VRR_STATE_ACTIVE_VARIABLE;
> +		else
> +			config.state = VRR_STATE_INACTIVE;
> +
> +	}
> +out:
>  	new_crtc_state->freesync_config = config;
>  }
>  
> @@ -8894,6 +8973,51 @@ static void reset_freesync_config_for_crtc(
>  	       sizeof(new_crtc_state->vrr_infopacket));
>  }
>  
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> +				 struct drm_crtc_state *new_crtc_state)
> +{
> +	struct drm_display_mode old_mode, new_mode;
> +
> +	if (!old_crtc_state || !new_crtc_state)
> +		return false;
> +
> +	old_mode = old_crtc_state->mode;
> +	new_mode = new_crtc_state->mode;
> +
> +	if (old_mode.clock       == new_mode.clock &&
> +	    old_mode.hdisplay    == new_mode.hdisplay &&
> +	    old_mode.vdisplay    == new_mode.vdisplay &&
> +	    old_mode.htotal      == new_mode.htotal &&
> +	    old_mode.vtotal      != new_mode.vtotal &&
> +	    old_mode.hsync_start == new_mode.hsync_start &&
> +	    old_mode.vsync_start != new_mode.vsync_start &&
> +	    old_mode.hsync_end   == new_mode.hsync_end &&
> +	    old_mode.vsync_end   != new_mode.vsync_end &&
> +	    old_mode.hskew       == new_mode.hskew &&
> +	    old_mode.vscan       == new_mode.vscan &&
> +	    (old_mode.vsync_end - old_mode.vsync_start) ==
> +	    (new_mode.vsync_end - new_mode.vsync_start))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void set_freesync_fixed_config(struct dm_crtc_state *dm_new_crtc_state) {
> +	uint64_t num, den, res;
> +	struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state->base;
> +
> +	dm_new_crtc_state->freesync_config.state = VRR_STATE_ACTIVE_FIXED;
> +
> +	num = (unsigned long long)new_crtc_state->mode.clock * 1000 * 1000000;
> +	den = (unsigned long long)new_crtc_state->mode.htotal *
> +	      (unsigned long long)new_crtc_state->mode.vtotal;
> +
> +	res = div_u64(num, den);
> +	dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz = res;
> +	dm_new_crtc_state->freesync_video_mode = true;
> +}
> +
>  static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>  				struct drm_atomic_state *state,
>  				struct drm_crtc *crtc,
> @@ -8984,6 +9108,11 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>  		 * TODO: Refactor this function to allow this check to work
>  		 * in all conditions.
>  		 */
> +		if (dm_new_crtc_state->stream &&
> +		    is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> +		    amdgpu_exp_freesync_vid_mode)
keeping this flag check before the function call might allow the compiler to save the function call, when this flag is disabled. 
> +			goto skip_modeset;
> +
>  		if (dm_new_crtc_state->stream &&
>  		    dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
>  		    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
> @@ -9015,6 +9144,23 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>  		if (!dm_old_crtc_state->stream)
>  			goto skip_modeset;
>  
> +		if (dm_new_crtc_state->stream &&
> +		    is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> +		    amdgpu_exp_freesync_vid_mode) {
same here
> +			new_crtc_state->mode_changed = false;
> +			DRM_DEBUG_DRIVER(
> +				"Mode change not required for front porch change, "
> +				"setting mode_changed to %d",
> +				new_crtc_state->mode_changed);
> +
> +			set_freesync_fixed_config(dm_new_crtc_state);
> +
> +			goto skip_modeset;
> +		} else if (aconnector &&
> +			   is_freesync_video_mode(&new_crtc_state->mode, aconnector) &&
> +			   amdgpu_exp_freesync_vid_mode)
> +			set_freesync_fixed_config(dm_new_crtc_state);
> +
>  		ret = dm_atomic_get_state(state, &dm_state);
>  		if (ret)
>  			goto fail;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index f5a5f1ad7dba..fd5fb9dafded 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -455,6 +455,7 @@ struct dm_crtc_state {
>  
>  	bool freesync_timing_changed;
>  	bool freesync_vrr_info_changed;
> +	bool freesync_video_mode;
>  
>  	bool dsc_force_changed;
>  	bool vrr_supported;

With the minor comments fixed/explained above,

this patch is Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>

- Shashank

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

Re: [PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes

[Aurabindo Pillai]

On Tue, 2020-12-15 at 08:32 +0530, Shashank Sharma wrote:
> 
> On 15/12/20 3:50 am, Aurabindo Pillai wrote:
> > [Why&How]
> > If experimental freesync video mode module parameter is enabled,
> > add
> > few extra display modes into the driver's mode list corresponding
> > to common
> > video frame rates. When userspace sets these modes, no modeset will
> > be
> > performed (if current mode was one of freesync modes or the base
> > freesync mode
> > based off which timings have been generated for the rest of the
> > freesync modes)
> > since these modes only differ from the base mode with front porch
> > timing.
> > 
> > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> > Acked-by: Christian König <christian.koenig at amd.com>
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167
> > ++++++++++++++++++
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   2 +
> >  2 files changed, 169 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index e7ee2467eadb..c1ffd33e9d83 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -5174,6 +5174,59 @@ static void
> > dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
> >         set_master_stream(context->streams, context->stream_count);
> >  }
> >  
> > +static struct drm_display_mode *
> > +get_highest_refresh_rate_mode(struct amdgpu_dm_connector
> > *aconnector,
> > +                         bool use_probed_modes)
> > +{
> > +       struct drm_display_mode *m, *m_pref = NULL;
> > +       u16 current_refresh, highest_refresh;
> > +       struct list_head *list_head = use_probed_modes ?
> > +                                                   &aconnector-
> > >base.probed_modes :
> > +                                                   &aconnector-
> > >base.modes;
> > +
> > +       if (aconnector->freesync_vid_base.clock != 0)
> > +               return &aconnector->freesync_vid_base;
> > +
> > +       /* Find the preferred mode */
> > +       list_for_each_entry (m, list_head, head) {
> > +               if (m->type & DRM_MODE_TYPE_PREFERRED) {
> > +                       m_pref = m;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!m_pref) {
> > +               /* Probably an EDID with no preferred mode.
> > Fallback to first entry */
> > +               m_pref = list_first_entry_or_null(
> > +                       &aconnector->base.modes, struct
> > drm_display_mode, head);
> > +               if (!m_pref) {
> > +                       DRM_DEBUG_DRIVER("No preferred mode found
> > in EDID\n");
> > +                       return NULL;
> > +               }
> > +       }
> > +
> > +       highest_refresh = drm_mode_vrefresh(m_pref);
> > +
> > +       /*
> > +        * Find the mode with highest refresh rate with same
> > resolution.
> > +        * For some monitors, preferred mode is not the mode with
> > highest
> > +        * supported refresh rate.
> > +        */
> > +       list_for_each_entry (m, list_head, head) {
> > +               current_refresh  = drm_mode_vrefresh(m);
> > +
> > +               if (m->hdisplay == m_pref->hdisplay &&
> > +                   m->vdisplay == m_pref->vdisplay &&
> > +                   highest_refresh < current_refresh) {
> > +                       highest_refresh = current_refresh;
> > +                       m_pref = m;
> > +               }
> > +       }
> > +
> > +       aconnector->freesync_vid_base = *m_pref;
> > +       return m_pref;
> > +}
> > +
> >  static struct dc_stream_state *
> >  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> >                        const struct drm_display_mode *drm_mode,
> > @@ -6999,6 +7052,119 @@ static void
> > amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
> >         }
> >  }
> >  
> > +static bool is_duplicate_mode(struct amdgpu_dm_connector
> > *aconnector,
> > +                             struct drm_display_mode *mode)
> > +{
> > +       struct drm_display_mode *m;
> > +
> > +       list_for_each_entry (m, &aconnector->base.probed_modes,
> > head) {
> > +               if (drm_mode_equal(m, mode))
> > +                       return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> > +static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
> > +                        struct detailed_data_monitor_range *range)
> > +{
> > +       const struct drm_display_mode *m;
> > +       struct drm_display_mode *new_mode;
> > +       uint i;
> > +       uint64_t target_vtotal, target_vtotal_diff;
> > +       uint32_t new_modes_count = 0;
> > +       uint64_t num, den;
> My previous review comment about num,den, target_vtotal and
> target_vtotal_diff here was neither addressed nor explained.
> > +
> > +       /* Standard FPS values
> > +        *
> > +        * 23.976 - TV/NTSC
> > +        * 24     - Cinema
> > +        * 25     - TV/PAL
> > +        * 29.97  - TV/NTSC
> > +        * 30     - TV/NTSC
> > +        * 48     - Cinema HFR
> > +        * 50     - TV/PAL
> > +        */
> My previous review comment about 60 fps here was neither addressed
> nor explained.
> 

Sorry about that, the list_for_each is gone, so I'll move it into the
for loop. I'll add the comment as well.

Thanks for the review!

--

Thanks & Regards,
Aurabindo Pillai

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

Re: [PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes

[Aurabindo Pillai]

On Tue, 2020-12-15 at 08:32 +0530, Shashank Sharma wrote:
> 
> On 15/12/20 3:50 am, Aurabindo Pillai wrote:
> > [Why&How]
> > If experimental freesync video mode module parameter is enabled,
> > add
> > few extra display modes into the driver's mode list corresponding
> > to common
> > video frame rates. When userspace sets these modes, no modeset will
> > be
> > performed (if current mode was one of freesync modes or the base
> > freesync mode
> > based off which timings have been generated for the rest of the
> > freesync modes)
> > since these modes only differ from the base mode with front porch
> > timing.
> > 
> > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> > Acked-by: Christian König <christian.koenig at amd.com>
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167
> > ++++++++++++++++++
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   2 +
> >  2 files changed, 169 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index e7ee2467eadb..c1ffd33e9d83 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -5174,6 +5174,59 @@ static void
> > dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
> >         set_master_stream(context->streams, context->stream_count);
> >  }
> >  
> > +static struct drm_display_mode *
> > +get_highest_refresh_rate_mode(struct amdgpu_dm_connector
> > *aconnector,
> > +                         bool use_probed_modes)
> > +{
> > +       struct drm_display_mode *m, *m_pref = NULL;
> > +       u16 current_refresh, highest_refresh;
> > +       struct list_head *list_head = use_probed_modes ?
> > +                                                   &aconnector-
> > >base.probed_modes :
> > +                                                   &aconnector-
> > >base.modes;
> > +
> > +       if (aconnector->freesync_vid_base.clock != 0)
> > +               return &aconnector->freesync_vid_base;
> > +
> > +       /* Find the preferred mode */
> > +       list_for_each_entry (m, list_head, head) {
> > +               if (m->type & DRM_MODE_TYPE_PREFERRED) {
> > +                       m_pref = m;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!m_pref) {
> > +               /* Probably an EDID with no preferred mode.
> > Fallback to first entry */
> > +               m_pref = list_first_entry_or_null(
> > +                       &aconnector->base.modes, struct
> > drm_display_mode, head);
> > +               if (!m_pref) {
> > +                       DRM_DEBUG_DRIVER("No preferred mode found
> > in EDID\n");
> > +                       return NULL;
> > +               }
> > +       }
> > +
> > +       highest_refresh = drm_mode_vrefresh(m_pref);
> > +
> > +       /*
> > +        * Find the mode with highest refresh rate with same
> > resolution.
> > +        * For some monitors, preferred mode is not the mode with
> > highest
> > +        * supported refresh rate.
> > +        */
> > +       list_for_each_entry (m, list_head, head) {
> > +               current_refresh  = drm_mode_vrefresh(m);
> > +
> > +               if (m->hdisplay == m_pref->hdisplay &&
> > +                   m->vdisplay == m_pref->vdisplay &&
> > +                   highest_refresh < current_refresh) {
> > +                       highest_refresh = current_refresh;
> > +                       m_pref = m;
> > +               }
> > +       }
> > +
> > +       aconnector->freesync_vid_base = *m_pref;
> > +       return m_pref;
> > +}
> > +
> >  static struct dc_stream_state *
> >  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> >                        const struct drm_display_mode *drm_mode,
> > @@ -6999,6 +7052,119 @@ static void
> > amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
> >         }
> >  }
> >  
> > +static bool is_duplicate_mode(struct amdgpu_dm_connector
> > *aconnector,
> > +                             struct drm_display_mode *mode)
> > +{
> > +       struct drm_display_mode *m;
> > +
> > +       list_for_each_entry (m, &aconnector->base.probed_modes,
> > head) {
> > +               if (drm_mode_equal(m, mode))
> > +                       return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> > +static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
> > +                        struct detailed_data_monitor_range *range)
> > +{
> > +       const struct drm_display_mode *m;
> > +       struct drm_display_mode *new_mode;
> > +       uint i;
> > +       uint64_t target_vtotal, target_vtotal_diff;
> > +       uint32_t new_modes_count = 0;
> > +       uint64_t num, den;
> My previous review comment about num,den, target_vtotal and
> target_vtotal_diff here was neither addressed nor explained.
> > +
> > +       /* Standard FPS values
> > +        *
> > +        * 23.976 - TV/NTSC
> > +        * 24     - Cinema
> > +        * 25     - TV/PAL
> > +        * 29.97  - TV/NTSC
> > +        * 30     - TV/NTSC
> > +        * 48     - Cinema HFR
> > +        * 50     - TV/PAL
> > +        */
> My previous review comment about 60 fps here was neither addressed
> nor explained.
> 

Sorry about that, the list_for_each is gone, so I'll move it into the
for loop. I'll add the comment as well.

Thanks for the review!

--

Thanks & Regards,
Aurabindo Pillai

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

Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode

[Alex Deucher]

On Mon, Dec 14, 2020 at 5:21 PM Aurabindo Pillai
<aurabindo.pillai@amd.com> wrote:
>
> [Why&How]
> Adds a module parameter to enable experimental freesync video mode modeset
> optimization. Enabling this mode allows the driver to skip a full modeset when
> freesync compatible modes are requested by the userspace. This paramters also
> adds some standard modes based on the connected monitor's VRR range.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Acked-by: Christian König <christian.koenig at amd.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index eed5410947e9..e0942184efdd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
>  extern int amdgpu_emu_mode;
>  extern uint amdgpu_smu_memory_pool_size;
>  extern uint amdgpu_dc_feature_mask;
> +extern uint amdgpu_exp_freesync_vid_mode;
>  extern uint amdgpu_dc_debug_mask;
>  extern uint amdgpu_dm_abm_level;
>  extern struct amdgpu_mgpu_info mgpu_info;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b2a1dd7581bf..ece51ecd53d1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -158,6 +158,7 @@ int amdgpu_mes;
>  int amdgpu_noretry = -1;
>  int amdgpu_force_asic_type = -1;
>  int amdgpu_tmz;
> +uint amdgpu_exp_freesync_vid_mode;
>  int amdgpu_reset_method = -1; /* auto */
>  int amdgpu_num_kcq = -1;
>
> @@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444);
>  MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)");
>  module_param_named(tmz, amdgpu_tmz, int, 0444);
>
> +/**
> + * DOC: experimental_freesync_video (uint)
> + * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience
> + * when setting a freesync supported mode for which full modeset is not needed.
> + * The default value: 0 (off).
> + */
> +MODULE_PARM_DESC(
> +       experimental_freesync_video,
> +       "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");

Maybe just call this freesync_video so that we can change the default
if we decide to at some point.

Alex

> +module_param_named(experimental_freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444);
> +
>  /**
>   * DOC: reset_method (int)
>   * GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)
> --
> 2.29.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode

[Alex Deucher]

On Mon, Dec 14, 2020 at 5:21 PM Aurabindo Pillai
<aurabindo.pillai@amd.com> wrote:
>
> [Why&How]
> Adds a module parameter to enable experimental freesync video mode modeset
> optimization. Enabling this mode allows the driver to skip a full modeset when
> freesync compatible modes are requested by the userspace. This paramters also
> adds some standard modes based on the connected monitor's VRR range.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Acked-by: Christian König <christian.koenig at amd.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index eed5410947e9..e0942184efdd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
>  extern int amdgpu_emu_mode;
>  extern uint amdgpu_smu_memory_pool_size;
>  extern uint amdgpu_dc_feature_mask;
> +extern uint amdgpu_exp_freesync_vid_mode;
>  extern uint amdgpu_dc_debug_mask;
>  extern uint amdgpu_dm_abm_level;
>  extern struct amdgpu_mgpu_info mgpu_info;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b2a1dd7581bf..ece51ecd53d1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -158,6 +158,7 @@ int amdgpu_mes;
>  int amdgpu_noretry = -1;
>  int amdgpu_force_asic_type = -1;
>  int amdgpu_tmz;
> +uint amdgpu_exp_freesync_vid_mode;
>  int amdgpu_reset_method = -1; /* auto */
>  int amdgpu_num_kcq = -1;
>
> @@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444);
>  MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)");
>  module_param_named(tmz, amdgpu_tmz, int, 0444);
>
> +/**
> + * DOC: experimental_freesync_video (uint)
> + * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience
> + * when setting a freesync supported mode for which full modeset is not needed.
> + * The default value: 0 (off).
> + */
> +MODULE_PARM_DESC(
> +       experimental_freesync_video,
> +       "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");

Maybe just call this freesync_video so that we can change the default
if we decide to at some point.

Alex

> +module_param_named(experimental_freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444);
> +
>  /**
>   * DOC: reset_method (int)
>   * GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)
> --
> 2.29.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode

[Aurabindo Pillai]

On Thu, Dec 17, 2020 at 14:11, Alex Deucher <alexdeucher@gmail.com> 
wrote:
> On Mon, Dec 14, 2020 at 5:21 PM Aurabindo Pillai
> <aurabindo.pillai@amd.com> wrote:
>> 
>>  [Why&How]
>>  Adds a module parameter to enable experimental freesync video mode 
>> modeset
>>  optimization. Enabling this mode allows the driver to skip a full 
>> modeset when
>>  freesync compatible modes are requested by the userspace. This 
>> paramters also
>>  adds some standard modes based on the connected monitor's VRR range.
>> 
>>  Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>>  Acked-by: Christian König <christian.koenig at amd.com>
>>  Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
>>  ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
>>   2 files changed, 13 insertions(+)
>> 
>>  diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>  index eed5410947e9..e0942184efdd 100644
>>  --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>  +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>  @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
>>   extern int amdgpu_emu_mode;
>>   extern uint amdgpu_smu_memory_pool_size;
>>   extern uint amdgpu_dc_feature_mask;
>>  +extern uint amdgpu_exp_freesync_vid_mode;
>>   extern uint amdgpu_dc_debug_mask;
>>   extern uint amdgpu_dm_abm_level;
>>   extern struct amdgpu_mgpu_info mgpu_info;
>>  diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>  index b2a1dd7581bf..ece51ecd53d1 100644
>>  --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>  +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>  @@ -158,6 +158,7 @@ int amdgpu_mes;
>>   int amdgpu_noretry = -1;
>>   int amdgpu_force_asic_type = -1;
>>   int amdgpu_tmz;
>>  +uint amdgpu_exp_freesync_vid_mode;
>>   int amdgpu_reset_method = -1; /* auto */
>>   int amdgpu_num_kcq = -1;
>> 
>>  @@ -786,6 +787,17 @@ module_param_named(abmlevel, 
>> amdgpu_dm_abm_level, uint, 0444);
>>   MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off 
>> (default), 1 = on)");
>>   module_param_named(tmz, amdgpu_tmz, int, 0444);
>> 
>>  +/**
>>  + * DOC: experimental_freesync_video (uint)
>>  + * Enabled the optimization to adjust front porch timing to 
>> achieve seamless mode change experience
>>  + * when setting a freesync supported mode for which full modeset 
>> is not needed.
>>  + * The default value: 0 (off).
>>  + */
>>  +MODULE_PARM_DESC(
>>  +       experimental_freesync_video,
>>  +       "Enable freesync modesetting optimization feature (0 = off 
>> (default), 1 = on)");
> 
> Maybe just call this freesync_video so that we can change the default
> if we decide to at some point.

Sure, will do.

> 
> Alex


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

Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode

[Aurabindo Pillai]

On Thu, Dec 17, 2020 at 14:11, Alex Deucher <alexdeucher@gmail.com> 
wrote:
> On Mon, Dec 14, 2020 at 5:21 PM Aurabindo Pillai
> <aurabindo.pillai@amd.com> wrote:
>> 
>>  [Why&How]
>>  Adds a module parameter to enable experimental freesync video mode 
>> modeset
>>  optimization. Enabling this mode allows the driver to skip a full 
>> modeset when
>>  freesync compatible modes are requested by the userspace. This 
>> paramters also
>>  adds some standard modes based on the connected monitor's VRR range.
>> 
>>  Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>>  Acked-by: Christian König <christian.koenig at amd.com>
>>  Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
>>  ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
>>   2 files changed, 13 insertions(+)
>> 
>>  diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>  index eed5410947e9..e0942184efdd 100644
>>  --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>  +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>  @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
>>   extern int amdgpu_emu_mode;
>>   extern uint amdgpu_smu_memory_pool_size;
>>   extern uint amdgpu_dc_feature_mask;
>>  +extern uint amdgpu_exp_freesync_vid_mode;
>>   extern uint amdgpu_dc_debug_mask;
>>   extern uint amdgpu_dm_abm_level;
>>   extern struct amdgpu_mgpu_info mgpu_info;
>>  diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>  index b2a1dd7581bf..ece51ecd53d1 100644
>>  --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>  +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>  @@ -158,6 +158,7 @@ int amdgpu_mes;
>>   int amdgpu_noretry = -1;
>>   int amdgpu_force_asic_type = -1;
>>   int amdgpu_tmz;
>>  +uint amdgpu_exp_freesync_vid_mode;
>>   int amdgpu_reset_method = -1; /* auto */
>>   int amdgpu_num_kcq = -1;
>> 
>>  @@ -786,6 +787,17 @@ module_param_named(abmlevel, 
>> amdgpu_dm_abm_level, uint, 0444);
>>   MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off 
>> (default), 1 = on)");
>>   module_param_named(tmz, amdgpu_tmz, int, 0444);
>> 
>>  +/**
>>  + * DOC: experimental_freesync_video (uint)
>>  + * Enabled the optimization to adjust front porch timing to 
>> achieve seamless mode change experience
>>  + * when setting a freesync supported mode for which full modeset 
>> is not needed.
>>  + * The default value: 0 (off).
>>  + */
>>  +MODULE_PARM_DESC(
>>  +       experimental_freesync_video,
>>  +       "Enable freesync modesetting optimization feature (0 = off 
>> (default), 1 = on)");
> 
> Maybe just call this freesync_video so that we can change the default
> if we decide to at some point.

Sure, will do.

> 
> Alex


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

[PATCH v3 0/3] Experimental freesync video mode optimization

[Aurabindo Pillai]

Changes in V4
=============

1) Add module parameter for freesync video mode

* Change module parameter name to freesync_video

2) Add freesync video modes based on preferred modes:

* Cosmetic fixes
* Added comments about all modes being added by the driver.

3) Skip modeset for front porch change

* Added more conditions for checking freesync video mode

Changes in V3
=============

1) Add freesync video modes based on preferred modes:

* Cache base freesync video mode during the first iteration to avoid
  iterating over modelist again later.
* Add mode for 60 fps videos

2) Skip modeset for front porch change

* Fixes for bug exposed by caching of modes.

Changes in V2
=============

1) Add freesync video modes based on preferred modes:

* Remove check for connector type before adding freesync compatible
  modes as VRR support is being checked, and there is no reason to block
  freesync video support on eDP.
* use drm_mode_equal() instead of creating same functionality.
* Additional null pointer deference check
* Removed unnecessary variables.
* Cosmetic fixes.

2) Skip modeset for front porch change

* Remove _FSV string being appended to freesync video modes so as to not
  define new policies or break existing application that might use the
  mode name to figure out mode resolution.
* Remove unnecessary variables
* Cosmetic fixes.

--

This patchset enables freesync video mode usecase where the userspace
can request a freesync compatible video mode such that switching to this
mode does not trigger blanking.

This feature is guarded by a module parameter which is disabled by
default. Enabling this paramters adds additional modes to the driver
modelist, and also enables the optimization to skip modeset when using
one of these modes.

--

Aurabindo Pillai (3):
  drm/amd/display: Add module parameter for freesync video mode
  drm/amd/display: Add freesync video modes based on preferred modes
  drm/amd/display: Skip modeset for front porch change

 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  12 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 389 ++++++++++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +
 4 files changed, 373 insertions(+), 32 deletions(-)

-- 
2.30.0

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

[PATCH v3 0/3] Experimental freesync video mode optimization

[Aurabindo Pillai]

Changes in V4
=============

1) Add module parameter for freesync video mode

* Change module parameter name to freesync_video

2) Add freesync video modes based on preferred modes:

* Cosmetic fixes
* Added comments about all modes being added by the driver.

3) Skip modeset for front porch change

* Added more conditions for checking freesync video mode

Changes in V3
=============

1) Add freesync video modes based on preferred modes:

* Cache base freesync video mode during the first iteration to avoid
  iterating over modelist again later.
* Add mode for 60 fps videos

2) Skip modeset for front porch change

* Fixes for bug exposed by caching of modes.

Changes in V2
=============

1) Add freesync video modes based on preferred modes:

* Remove check for connector type before adding freesync compatible
  modes as VRR support is being checked, and there is no reason to block
  freesync video support on eDP.
* use drm_mode_equal() instead of creating same functionality.
* Additional null pointer deference check
* Removed unnecessary variables.
* Cosmetic fixes.

2) Skip modeset for front porch change

* Remove _FSV string being appended to freesync video modes so as to not
  define new policies or break existing application that might use the
  mode name to figure out mode resolution.
* Remove unnecessary variables
* Cosmetic fixes.

--

This patchset enables freesync video mode usecase where the userspace
can request a freesync compatible video mode such that switching to this
mode does not trigger blanking.

This feature is guarded by a module parameter which is disabled by
default. Enabling this paramters adds additional modes to the driver
modelist, and also enables the optimization to skip modeset when using
one of these modes.

--

Aurabindo Pillai (3):
  drm/amd/display: Add module parameter for freesync video mode
  drm/amd/display: Add freesync video modes based on preferred modes
  drm/amd/display: Skip modeset for front porch change

 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  12 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 389 ++++++++++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +
 4 files changed, 373 insertions(+), 32 deletions(-)

-- 
2.30.0

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