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

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

[Aurabindo Pillai]

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 | 330 +++++++++++++++++-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 4 files changed, 329 insertions(+), 15 deletions(-)

-- 
2.25.1

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

[PATCH v2 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>
---
 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 83ac06a3ec05..efbfee93c359 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.25.1

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

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

[Shashank Sharma]

LGTM,

Please feel free to use

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


Regards

Shashank

On 11/12/20 12:18 am, Aurabindo Pillai 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>
> ---
>  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 83ac06a3ec05..efbfee93c359 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)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2 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>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167 ++++++++++++++++++
 1 file changed, 167 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 fbff8d693e03..d15453b400d2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5178,6 +5178,54 @@ 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_high = NULL;
+	u16 current_refresh, highest_refresh;
+	struct list_head *list_head = use_probed_modes ?
+						    &aconnector->base.probed_modes :
+						    &aconnector->base.modes;
+	/* Find the preferred mode */
+	list_for_each_entry (m, list_head, head) {
+		if (!(m->type & DRM_MODE_TYPE_PREFERRED))
+			continue;
+
+		m_high = m;
+		highest_refresh = drm_mode_vrefresh(m_high);
+		break;
+	}
+
+	if (!m_high) {
+		/* Probably an EDID with no preferred mode. Fallback to first entry */
+		m_high = list_first_entry_or_null(&aconnector->base.modes,
+						  struct drm_display_mode, head);
+		if (!m_high)
+			return NULL;
+		else
+			highest_refresh = drm_mode_vrefresh(m_high);
+	}
+
+	/*
+	 * 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_high->hdisplay &&
+		    m->vdisplay == m_high->vdisplay &&
+		    highest_refresh < current_refresh) {
+			highest_refresh = current_refresh;
+			m_high = m;
+		}
+	}
+
+	return m_high;
+}
+
 static struct dc_stream_state *
 create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 		       const struct drm_display_mode *drm_mode,
@@ -7006,6 +7054,124 @@ 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, *m_save;
+	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 neededrates[] = { 23976, 24000, 25000, 29970,
+					 30000, 48000, 50000, 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_save = get_highest_refresh_rate_mode(aconnector, true);
+	if (!m_save)
+		goto out;
+
+	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
+		if (m != m_save)
+			continue;
+
+		for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
+			if (drm_mode_vrefresh(m) * 1000 < neededrates[i])
+				continue;
+
+			if (neededrates[i] < range->min_vfreq * 1000)
+				continue;
+
+			num = (unsigned long long)m->clock * 1000 * 1000;
+			den = neededrates[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 =
@@ -7021,6 +7187,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);
 
-- 
2.25.1

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

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

[Shashank Sharma]

On 11/12/20 12:18 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>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167 ++++++++++++++++++
>  1 file changed, 167 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 fbff8d693e03..d15453b400d2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5178,6 +5178,54 @@ 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_high = NULL;
I would prefer m_high to be renamed as m_pref, indicating it's the preferred mode
> +	u16 current_refresh, highest_refresh;
> +	struct list_head *list_head = use_probed_modes ?
> +						    &aconnector->base.probed_modes :
> +						    &aconnector->base.modes;
> +	/* Find the preferred mode */
> +	list_for_each_entry (m, list_head, head) {
> +		if (!(m->type & DRM_MODE_TYPE_PREFERRED))
> +			continue;
> +
> +		m_high = m;
> +		highest_refresh = drm_mode_vrefresh(m_high);
> +		break;
> +	}
> +
> +	if (!m_high) {
> +		/* Probably an EDID with no preferred mode. Fallback to first entry */
> +		m_high = list_first_entry_or_null(&aconnector->base.modes,
> +						  struct drm_display_mode, head);
> +		if (!m_high)
> +			return NULL;
> +		else
> +			highest_refresh = drm_mode_vrefresh(m_high);
> +	}
> +

Optional cleanup suggested below makes code more readable:


/* 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_high->hdisplay &&
> +		    m->vdisplay == m_high->vdisplay &&
> +		    highest_refresh < current_refresh) {
> +			highest_refresh = current_refresh;
> +			m_high = m;
> +		}
> +	}
> +
> +	return m_high;
> +}
> +
>  static struct dc_stream_state *
>  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>  		       const struct drm_display_mode *drm_mode,
> @@ -7006,6 +7054,124 @@ 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, *m_save;
> +	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;
num, den, target_vtotal, target_vtotal_diff can go inside the list_for_each_entry() loop; 
> +
> +	/* 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
I missed this last time, but why don't we have 60 fps here ? Most preferred modes are 60fps in general. Or was it missed in comment only ?
> +	 */
> +	const uint32_t neededrates[] = { 23976, 24000, 25000, 29970,
> +					 30000, 48000, 50000, 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_save = get_highest_refresh_rate_mode(aconnector, true);
> +	if (!m_save)
> +		goto out;
> +
> +	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
> +		if (m != m_save)
> +			continue;

Now when I think about it again,

- we already went through the list (aconnector->base.probed_modes) in function get_highest_refresh_rate_mode(), and got the m_save mode.

- then we are again going through the same list, to find m = m_save, why ? Am I missing something or we can use m_save directly here

some thing like:

m_save = get_highest_refresh_rate_mode(aconnector, true);

if (m_save)  {

   for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
       // do the same thing here 
   }
}

This would save another iteration through the probed_modes. 

> +
> +		for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
ARRAY_SIZE() here; also instead of calculating it for every iteration, we can use a local variable u8 len = ARRAY_SIZE(neededrates); Not sure if compiler will do that for us though ;-)
> +			if (drm_mode_vrefresh(m) * 1000 < neededrates[i])
> +				continue;
> +
> +			if (neededrates[i] < range->min_vfreq * 1000)
> +				continue;
> +
> +			num = (unsigned long long)m->clock * 1000 * 1000;
> +			den = neededrates[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;

Just FYI, All the DMT modes and CEA_MODES from EDID also use this flag, so even though it's the right flag to set, it's not unique enough to identify it as FS mode.

- Shashank

> +
> +			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 =
> @@ -7021,6 +7187,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);
>  
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Kazlauskas, Nicholas]

On 2020-12-11 12:54 a.m., Shashank Sharma wrote:
> 
> On 11/12/20 12:18 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>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167 ++++++++++++++++++
>>   1 file changed, 167 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 fbff8d693e03..d15453b400d2 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -5178,6 +5178,54 @@ 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_high = NULL;
> I would prefer m_high to be renamed as m_pref, indicating it's the preferred mode
>> +	u16 current_refresh, highest_refresh;
>> +	struct list_head *list_head = use_probed_modes ?
>> +						    &aconnector->base.probed_modes :
>> +						    &aconnector->base.modes;
>> +	/* Find the preferred mode */
>> +	list_for_each_entry (m, list_head, head) {
>> +		if (!(m->type & DRM_MODE_TYPE_PREFERRED))
>> +			continue;
>> +
>> +		m_high = m;
>> +		highest_refresh = drm_mode_vrefresh(m_high);
>> +		break;
>> +	}
>> +
>> +	if (!m_high) {
>> +		/* Probably an EDID with no preferred mode. Fallback to first entry */
>> +		m_high = list_first_entry_or_null(&aconnector->base.modes,
>> +						  struct drm_display_mode, head);
>> +		if (!m_high)
>> +			return NULL;
>> +		else
>> +			highest_refresh = drm_mode_vrefresh(m_high);
>> +	}
>> +
> 
> Optional cleanup suggested below makes code more readable:
> 
> 
> /* 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);

Agreed with this cleanup - naming is confusing as is.

>> +	/*
>> +	 * 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_high->hdisplay &&
>> +		    m->vdisplay == m_high->vdisplay &&
>> +		    highest_refresh < current_refresh) {
>> +			highest_refresh = current_refresh;
>> +			m_high = m;
>> +		}
>> +	}
>> +
>> +	return m_high;
>> +}
>> +
>>   static struct dc_stream_state *
>>   create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>   		       const struct drm_display_mode *drm_mode,
>> @@ -7006,6 +7054,124 @@ 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, *m_save;
>> +	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;
> num, den, target_vtotal, target_vtotal_diff can go inside the list_for_each_entry() loop;
>> +
>> +	/* 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
> I missed this last time, but why don't we have 60 fps here ? Most preferred modes are 60fps in general. Or was it missed in comment only ?

It should be in this list, but that brings up another point that needs 
to be addressed - if the mode already exists in the modelist then we 
should skip adding a duplicate mode.

>> +	 */
>> +	const uint32_t neededrates[] = { 23976, 24000, 25000, 29970,
>> +					 30000, 48000, 50000, 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_save = get_highest_refresh_rate_mode(aconnector, true);
>> +	if (!m_save)
>> +		goto out;
>> +
>> +	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
>> +		if (m != m_save)
>> +			continue;
> 
> Now when I think about it again,
> 
> - we already went through the list (aconnector->base.probed_modes) in function get_highest_refresh_rate_mode(), and got the m_save mode.
> 
> - then we are again going through the same list, to find m = m_save, why ? Am I missing something or we can use m_save directly here
> 
> some thing like:
> 
> m_save = get_highest_refresh_rate_mode(aconnector, true);
> 
> if (m_save)  {
> 
>     for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
>         // do the same thing here
>     }
> }
> 
> This would save another iteration through the probed_modes.
> 
>> +
>> +		for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
> ARRAY_SIZE() here; also instead of calculating it for every iteration, we can use a local variable u8 len = ARRAY_SIZE(neededrates); Not sure if compiler will do that for us though ;-)
>> +			if (drm_mode_vrefresh(m) * 1000 < neededrates[i])
>> +				continue;
>> +
>> +			if (neededrates[i] < range->min_vfreq * 1000)
>> +				continue;
>> +
>> +			num = (unsigned long long)m->clock * 1000 * 1000;
>> +			den = neededrates[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;
> 
> Just FYI, All the DMT modes and CEA_MODES from EDID also use this flag, so even though it's the right flag to set, it's not unique enough to identify it as FS mode.
> 
> - Shashank

I don't think we should even be bothering trying to identify whether the 
mode was a FS mode or not since the front porch modeset skip 
optimization can apply generically (at least as an experimental opt-in 
feature for now).

Regards,
Nicholas Kazlauskas

> 
>> +
>> +			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 =
>> @@ -7021,6 +7187,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);
>>   

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

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

[Aurabindo Pillai]

On Mon, 2021-01-04 at 11:06 -0500, Kazlauskas, Nicholas wrote:
> On 2020-12-11 12:54 a.m., Shashank Sharma wrote:
> > 
> > On 11/12/20 12:18 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>
> > > ---
> > >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167
> > > ++++++++++++++++++
> > >   1 file changed, 167 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 fbff8d693e03..d15453b400d2 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > @@ -5178,6 +5178,54 @@ 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_high = NULL;
> > I would prefer m_high to be renamed as m_pref, indicating it's the
> > preferred mode
> > > +       u16 current_refresh, highest_refresh;
> > > +       struct list_head *list_head = use_probed_modes ?
> > > +                                                   &aconnector-
> > > >base.probed_modes :
> > > +                                                   &aconnector-
> > > >base.modes;
> > > +       /* Find the preferred mode */
> > > +       list_for_each_entry (m, list_head, head) {
> > > +               if (!(m->type & DRM_MODE_TYPE_PREFERRED))
> > > +                       continue;
> > > +
> > > +               m_high = m;
> > > +               highest_refresh = drm_mode_vrefresh(m_high);
> > > +               break;
> > > +       }
> > > +
> > > +       if (!m_high) {
> > > +               /* Probably an EDID with no preferred mode.
> > > Fallback to first entry */
> > > +               m_high = list_first_entry_or_null(&aconnector-
> > > >base.modes,
> > > +                                                 struct
> > > drm_display_mode, head);
> > > +               if (!m_high)
> > > +                       return NULL;
> > > +               else
> > > +                       highest_refresh =
> > > drm_mode_vrefresh(m_high);
> > > +       }
> > > +
> > 
> > Optional cleanup suggested below makes code more readable:
> > 
> > 
> > /* 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);
> 
> Agreed with this cleanup - naming is confusing as is.

This was fixed in latest revision.

> 
> > > +       /*
> > > +        * 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_high->hdisplay &&
> > > +                   m->vdisplay == m_high->vdisplay &&
> > > +                   highest_refresh < current_refresh) {
> > > +                       highest_refresh = current_refresh;
> > > +                       m_high = m;
> > > +               }
> > > +       }
> > > +
> > > +       return m_high;
> > > +}
> > > +
> > >   static struct dc_stream_state *
> > >   create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> > >                        const struct drm_display_mode *drm_mode,
> > > @@ -7006,6 +7054,124 @@ 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, *m_save;
> > > +       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;
> > num, den, target_vtotal, target_vtotal_diff can go inside the
> > list_for_each_entry() loop;
> > > +
> > > +       /* 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
> > I missed this last time, but why don't we have 60 fps here ? Most
> > preferred modes are 60fps in general. Or was it missed in comment
> > only ?
> 
> It should be in this list, but that brings up another point that
> needs 
> to be addressed - if the mode already exi28310fd6dabcsts in the
> modelist then we 
> should skip adding a duplicate mode.
> 

This was fixed in latest revision.

> > > +        */
> > > +       const uint32_t neededrates[] = { 23976, 24000, 25000,
> > > 29970,
> > > +                                        30000, 48000, 50000,
> > > 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_save = get_highest_refresh_rate_mode(aconnector, true);
> > > +       if (!m_save)
> > > +               goto out;
> > > +
> > > +       list_for_each_entry (m, &aconnector->base.probed_modes,
> > > head) {
> > > +               if (m != m_save)
> > > +                       continue;
> > 
> > Now when I think about it again,
> > 
> > - we already went through the list (aconnector->base.probed_modes)
> > in function get_highest_refresh_rate_mode(), and got the m_save
> > mode.
> > 
> > - then we are again going through the same list, to find m =
> > m_save, why ? Am I missing something or we can use m_save directly
> > here
> > 
> > some thing like:
> > 
> > m_save = get_highest_refresh_rate_mode(aconnector, true);
> > 
> > if (m_save)  {
> > 
> >     for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
> >         // do the same thing here
> >     }
> > }
> > 
> > This would save another iteration through the probed_modes.
> > 
> > > +
> > > +               for (i = 0; i < sizeof(neededrates) /
> > > sizeof(uint32_t); i++) {
> > ARRAY_SIZE() here; also instead of calculating it for every
> > iteration, we can use a local variable u8 len =
> > ARRAY_SIZE(neededrates); Not sure if compiler will do that for us
> > though ;-)
> > > +                       if (drm_mode_vrefresh(m) * 1000 <
> > > neededrates[i])
> > > +                               continue;
> > > +
> > > +                       if (neededrates[i] < range->min_vfreq *
> > > 1000)
> > > +                               continue;
> > > +
> > > +                       num = (unsigned long long)m->clock * 1000
> > > * 1000;
> > > +                       den = neededrates[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;
> > 
> > Just FYI, All the DMT modes and CEA_MODES from EDID also use this
> > flag, so even though it's the right flag to set, it's not unique
> > enough to identify it as FS mode.
> > 
> > - Shashank
> 
> I don't think we should even be bothering trying to identify whether
> the 
> mode was a FS mode or not since the front porch modeset skip 
> optimization can apply generically (at least as an experimental opt-
> in 
> feature for now).
> 
> Regards,
> Nicholas Kazlauskas
> 
> > 
> > > +
> > > +                       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 =
> > > @@ -7021,6 +7187,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);
> > >   
> 


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

[PATCH v2 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>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 163 ++++++++++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 2 files changed, 149 insertions(+), 15 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 d15453b400d2..9e6aba961edd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -217,6 +217,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
  *
@@ -5226,6 +5229,24 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
 	return m_high;
 }
 
+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,
@@ -5239,16 +5260,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;
 #endif
 	uint32_t link_bandwidth_kbps;
-
 	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,20 +5327,32 @@ 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)
+	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);
 	else
@@ -7851,13 +7889,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);
@@ -8175,7 +8229,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,
@@ -8866,6 +8922,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 &&
@@ -8873,17 +8930,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;
 }
 
@@ -8896,6 +8962,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,
@@ -8986,6 +9097,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)) {
@@ -9017,6 +9133,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 251af783f6b1..28f2d8c9b260 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -453,6 +453,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.25.1

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Simon Ser]

Hi,

(CC dri-devel, Pekka and Martin who might be interested in this as well.)

On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:

> 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.

Thanks for working on this, it's an interesting feature! However I'd like to
take some time to think about the user-space API for this.

As I understand it, some new synthetic modes are added, and user-space can
perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
switch to a mode without blanking the screen.

However the exact modes amdgpu adds are just some guesses. I think it would be
great if user-space could control the min/max refresh rate values directly.
Not only this would remove the need for the kernel to hard-code "well-known
video refresh rates", but this would also enable more use-cases. For instance
some users might want to mitigate flickering on their screen by reducing the
VRR range. Some users might want to lower their screen refresh rate for power
savings.

What do you think? Would you be fine with adding min/max VRR range properties?

If you're scared about the user-space code requirement, I can provide that.

Thanks,

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Simon Ser]

Hi,

(CC dri-devel, Pekka and Martin who might be interested in this as well.)

On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:

> 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.

Thanks for working on this, it's an interesting feature! However I'd like to
take some time to think about the user-space API for this.

As I understand it, some new synthetic modes are added, and user-space can
perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
switch to a mode without blanking the screen.

However the exact modes amdgpu adds are just some guesses. I think it would be
great if user-space could control the min/max refresh rate values directly.
Not only this would remove the need for the kernel to hard-code "well-known
video refresh rates", but this would also enable more use-cases. For instance
some users might want to mitigate flickering on their screen by reducing the
VRR range. Some users might want to lower their screen refresh rate for power
savings.

What do you think? Would you be fine with adding min/max VRR range properties?

If you're scared about the user-space code requirement, I can provide that.

Thanks,

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Shashank Sharma]

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

Hello Simon,

Hope you are doing well,

I was helping out Aurabindo and the team with the design, so I have taken the liberty of adding some comments on behalf of the team, Inline.

On 11/12/20 3:31 am, Simon Ser wrote:
> Hi,
>
> (CC dri-devel, Pekka and Martin who might be interested in this as well.)
>
> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>
>> 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.
> Thanks for working on this, it's an interesting feature! However I'd like to
> take some time to think about the user-space API for this.
>
> As I understand it, some new synthetic modes are added, and user-space can
> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
> switch to a mode without blanking the screen.

The implementation is in those lines, but a bit different. The idea is to:

- check if the monitor supports VRR,

- If it does, add some new modes which are in the VRR tolerance range, as new video modes in the list (with driver flag).

- when you get modeset on any of these modes, skip the full modeset, and just adjust the front_porch timing

so they are not test-only as such, for any user-space these modes will be as real as any other probed modes of the list.

>
> However the exact modes amdgpu adds are just some guesses. I think it would be
> great if user-space could control the min/max refresh rate values directly.
> Not only this would remove the need for the kernel to hard-code "well-known
> video refresh rates", but this would also enable more use-cases. For instance
> some users might want to mitigate flickering on their screen by reducing the
> VRR range. Some users might want to lower their screen refresh rate for power
> savings.
>
> What do you think? Would you be fine with adding min/max VRR range properties?
>
> If you're scared about the user-space code requirement, I can provide that.

This sounds like a reasonable approach, and there is no reason why we can't do this if we have the proper userspace support as you mentioned.

But what we thought would be a sensitive approach towards this feature would be:

- *Phase 1: *Add this feature experimentally as kernel-only change, to:

   test out its functionality on all all supported platforms first, without going for the UAPI complexity.

   gain attention from UAPI stakeholders and get them involved for the UAPI design (so far so good :)).

- *Phase 2:* Have a design discussions with user-space stakeholders, examine the use-cases possible, and then create a reasonable UAPI, and make the other solution a fallback method.

So I guess we can fork out a parallel discussion for the UAPI thread too. How does this sound to you ?


- Shashank

>
> Thanks,
>
> Simon Ser

[-- Attachment #1.2: Type: text/html, Size: 4565 bytes --]

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Shashank Sharma]

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

Hello Simon,

Hope you are doing well,

I was helping out Aurabindo and the team with the design, so I have taken the liberty of adding some comments on behalf of the team, Inline.

On 11/12/20 3:31 am, Simon Ser wrote:
> Hi,
>
> (CC dri-devel, Pekka and Martin who might be interested in this as well.)
>
> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>
>> 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.
> Thanks for working on this, it's an interesting feature! However I'd like to
> take some time to think about the user-space API for this.
>
> As I understand it, some new synthetic modes are added, and user-space can
> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
> switch to a mode without blanking the screen.

The implementation is in those lines, but a bit different. The idea is to:

- check if the monitor supports VRR,

- If it does, add some new modes which are in the VRR tolerance range, as new video modes in the list (with driver flag).

- when you get modeset on any of these modes, skip the full modeset, and just adjust the front_porch timing

so they are not test-only as such, for any user-space these modes will be as real as any other probed modes of the list.

>
> However the exact modes amdgpu adds are just some guesses. I think it would be
> great if user-space could control the min/max refresh rate values directly.
> Not only this would remove the need for the kernel to hard-code "well-known
> video refresh rates", but this would also enable more use-cases. For instance
> some users might want to mitigate flickering on their screen by reducing the
> VRR range. Some users might want to lower their screen refresh rate for power
> savings.
>
> What do you think? Would you be fine with adding min/max VRR range properties?
>
> If you're scared about the user-space code requirement, I can provide that.

This sounds like a reasonable approach, and there is no reason why we can't do this if we have the proper userspace support as you mentioned.

But what we thought would be a sensitive approach towards this feature would be:

- *Phase 1: *Add this feature experimentally as kernel-only change, to:

   test out its functionality on all all supported platforms first, without going for the UAPI complexity.

   gain attention from UAPI stakeholders and get them involved for the UAPI design (so far so good :)).

- *Phase 2:* Have a design discussions with user-space stakeholders, examine the use-cases possible, and then create a reasonable UAPI, and make the other solution a fallback method.

So I guess we can fork out a parallel discussion for the UAPI thread too. How does this sound to you ?


- Shashank

>
> Thanks,
>
> Simon Ser

[-- Attachment #1.2: Type: text/html, Size: 4565 bytes --]

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Pekka Paalanen]

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

On Fri, 11 Dec 2020 09:56:07 +0530
Shashank Sharma <shashank.sharma@amd.com> wrote:

> Hello Simon,
> 
> Hope you are doing well,
> 
> I was helping out Aurabindo and the team with the design, so I have
> taken the liberty of adding some comments on behalf of the team,
> Inline.
> 
> On 11/12/20 3:31 am, Simon Ser wrote:
> > Hi,
> >
> > (CC dri-devel, Pekka and Martin who might be interested in this as
> > well.)

Thanks for the Cc! This is very interesting to me, and because it was
not Cc'd to dri-devel@ originally, I would have missed this otherwise.

> >
> > On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> >  
> >> 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.  
> > Thanks for working on this, it's an interesting feature! However I'd like to
> > take some time to think about the user-space API for this.
> >
> > As I understand it, some new synthetic modes are added, and user-space can
> > perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
> > switch to a mode without blanking the screen.  
> 
> The implementation is in those lines, but a bit different. The idea
> is to:
> 
> - check if the monitor supports VRR,
> 
> - If it does, add some new modes which are in the VRR tolerance
> range, as new video modes in the list (with driver flag).
> 
> - when you get modeset on any of these modes, skip the full modeset,
> and just adjust the front_porch timing
> 
> so they are not test-only as such, for any user-space these modes
> will be as real as any other probed modes of the list.

But is it worth to allow a modeset to be glitch-free if the userspace
does not know they are glitch-free? I think if this is going in, it
would be really useful to give the guarantees to userspace explicitly,
and not leave this feature at an "accidentally no glitch sometimes"
level.


I have been expecting and hoping for the ability to change video mode
timings without a modeset ever since I learnt that VRR is about
front-porch adjustment, quite a while ago.

This is how I envision userspace making use of it:

Let us have a Wayland compositor, which uses fixed-frequency video
modes, because it wants predictable vblank cycles. IOW, it will not
enable VRR as such.

When the Wayland compositor starts, it will choose *some* video mode
for an output. It may or may not be what a KMS driver calls "preferred
mode", because it depends on things like user preferences. The
compositor makes the initial modeset to this mode.

Use case 1:

A Wayland client comes up and determines that its window would really
like a refresh rate of, say, 47.5 Hz. Yes, it's not a traditional video
player rate, but let's assume the application has its reasons. The
client tells the compositor this (Wayland protocol still to be designed
to be able to do that). (Hey, this could be how future games should
implement refresh rate controls in cooperation with the window system.)

The compositor sees the wish, and according to its complex policy
rules, determines that yes, it shall try to honor that wish by changing
the whole output temporarily to 47.5 Hz if possible.

The compositor takes the original video mode it modeset on the output,
and adjusts the front-porch to create a new custom 47.5 Hz mode. Using
this mode, the compositor does a TEST_ONLY atomic commit *without*
ALLOW_MODESET.

If the test commit succeeds, the compositor knows that changing timings
will not cause any kind of glitch, flicker, blanking, or freeze, and
proceeds to commit this video mode without ALLOW_MODESET. The whole
output becomes 47.5 Hz until the compositor policy again determines
that it is time to change, e.g. to go back to the original mode. Going
back to the original mode also needs to work without ALLOW_MODESET -
but a compositor cannot test for this with atomic TEST_ONLY commits.

If the test commit fails, the compositor knows that it cannot change
the timings like this without risking a visible glitch. Therefore the
compositor does not change the video mode timings, and the client's
wish is not granted.

The client adapts to whatever the refresh rate is in any case.

Use case 2:

A client comes up, and starts presenting frames with a target timestamp
(Wayland protocol for this still to be designed). The compositor
analyzes the target timestamp, and according to the complex compositor
policy, determines that it should try to adjust video mode timings to
better meet the target timestamps.

Like in use case 1, the compositor creates a new custom video mode and
tests if it can be applied without any glitch. If yes, it is used. If
not, it is not used.

This use case is more complicated, because the video mode timing
changes may happen refresh by refresh, which means they need to
apply for the very next front-porch in the scanout cycle in
hardware. Hence, I'm not sure this use case is realistic. It can also
be argued that this is better implemented by just enabling VRR and
handling the flip timing in userspace, in the compositor: issue an
atomic flip at the exact time it needs to be executed instead of
issuing it well in advance and letting the driver wait for vblank.


Worth to note: neither case needs the kernel to expose new manufactured
video modes. Whether the feature is available or not is detected by an
atomic TEST_ONLY commit without ALLOW_MODESET.

> > However the exact modes amdgpu adds are just some guesses. I think it would be
> > great if user-space could control the min/max refresh rate values directly.

Setting min==max could be used to achieve the fixed refresh rate
proposed here, but it would also allow setting custom min < max limits.
This would be more flexible, but I'm not sure what the use case for it
could look like... oh, there are the use cases mentioned below: user
preferences. :-)

Maybe the min/max setting is better than fiddling with custom video
modes. If we have min/max to control, then there is no problem with
going back to the "original" video mode like in my example use case 1.

> > Not only this would remove the need for the kernel to hard-code "well-known
> > video refresh rates", but this would also enable more use-cases. For instance
> > some users might want to mitigate flickering on their screen by reducing the
> > VRR range. Some users might want to lower their screen refresh rate for power
> > savings.
> >
> > What do you think? Would you be fine with adding min/max VRR range properties?
> >
> > If you're scared about the user-space code requirement, I can
> > provide that.  
> 
> This sounds like a reasonable approach, and there is no reason why we
> can't do this if we have the proper userspace support as you
> mentioned.

Maybe the min/max controls are the way to go, considering that
the seamless refresh rate change feature in general cannot be
implemented without VRR. Or can it?

But if it can be implemented while not supporting VRR on some hardware,
then the video mode fiddling without ALLOW_MODESET is still a usable
approach. Or maybe such a driver could special-case VRR=enabled &&
min==max.

Yeah, min/max controls seems like the best idea to me so far.


Thanks,
pq

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

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Pekka Paalanen]

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

On Fri, 11 Dec 2020 09:56:07 +0530
Shashank Sharma <shashank.sharma@amd.com> wrote:

> Hello Simon,
> 
> Hope you are doing well,
> 
> I was helping out Aurabindo and the team with the design, so I have
> taken the liberty of adding some comments on behalf of the team,
> Inline.
> 
> On 11/12/20 3:31 am, Simon Ser wrote:
> > Hi,
> >
> > (CC dri-devel, Pekka and Martin who might be interested in this as
> > well.)

Thanks for the Cc! This is very interesting to me, and because it was
not Cc'd to dri-devel@ originally, I would have missed this otherwise.

> >
> > On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> >  
> >> 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.  
> > Thanks for working on this, it's an interesting feature! However I'd like to
> > take some time to think about the user-space API for this.
> >
> > As I understand it, some new synthetic modes are added, and user-space can
> > perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
> > switch to a mode without blanking the screen.  
> 
> The implementation is in those lines, but a bit different. The idea
> is to:
> 
> - check if the monitor supports VRR,
> 
> - If it does, add some new modes which are in the VRR tolerance
> range, as new video modes in the list (with driver flag).
> 
> - when you get modeset on any of these modes, skip the full modeset,
> and just adjust the front_porch timing
> 
> so they are not test-only as such, for any user-space these modes
> will be as real as any other probed modes of the list.

But is it worth to allow a modeset to be glitch-free if the userspace
does not know they are glitch-free? I think if this is going in, it
would be really useful to give the guarantees to userspace explicitly,
and not leave this feature at an "accidentally no glitch sometimes"
level.


I have been expecting and hoping for the ability to change video mode
timings without a modeset ever since I learnt that VRR is about
front-porch adjustment, quite a while ago.

This is how I envision userspace making use of it:

Let us have a Wayland compositor, which uses fixed-frequency video
modes, because it wants predictable vblank cycles. IOW, it will not
enable VRR as such.

When the Wayland compositor starts, it will choose *some* video mode
for an output. It may or may not be what a KMS driver calls "preferred
mode", because it depends on things like user preferences. The
compositor makes the initial modeset to this mode.

Use case 1:

A Wayland client comes up and determines that its window would really
like a refresh rate of, say, 47.5 Hz. Yes, it's not a traditional video
player rate, but let's assume the application has its reasons. The
client tells the compositor this (Wayland protocol still to be designed
to be able to do that). (Hey, this could be how future games should
implement refresh rate controls in cooperation with the window system.)

The compositor sees the wish, and according to its complex policy
rules, determines that yes, it shall try to honor that wish by changing
the whole output temporarily to 47.5 Hz if possible.

The compositor takes the original video mode it modeset on the output,
and adjusts the front-porch to create a new custom 47.5 Hz mode. Using
this mode, the compositor does a TEST_ONLY atomic commit *without*
ALLOW_MODESET.

If the test commit succeeds, the compositor knows that changing timings
will not cause any kind of glitch, flicker, blanking, or freeze, and
proceeds to commit this video mode without ALLOW_MODESET. The whole
output becomes 47.5 Hz until the compositor policy again determines
that it is time to change, e.g. to go back to the original mode. Going
back to the original mode also needs to work without ALLOW_MODESET -
but a compositor cannot test for this with atomic TEST_ONLY commits.

If the test commit fails, the compositor knows that it cannot change
the timings like this without risking a visible glitch. Therefore the
compositor does not change the video mode timings, and the client's
wish is not granted.

The client adapts to whatever the refresh rate is in any case.

Use case 2:

A client comes up, and starts presenting frames with a target timestamp
(Wayland protocol for this still to be designed). The compositor
analyzes the target timestamp, and according to the complex compositor
policy, determines that it should try to adjust video mode timings to
better meet the target timestamps.

Like in use case 1, the compositor creates a new custom video mode and
tests if it can be applied without any glitch. If yes, it is used. If
not, it is not used.

This use case is more complicated, because the video mode timing
changes may happen refresh by refresh, which means they need to
apply for the very next front-porch in the scanout cycle in
hardware. Hence, I'm not sure this use case is realistic. It can also
be argued that this is better implemented by just enabling VRR and
handling the flip timing in userspace, in the compositor: issue an
atomic flip at the exact time it needs to be executed instead of
issuing it well in advance and letting the driver wait for vblank.


Worth to note: neither case needs the kernel to expose new manufactured
video modes. Whether the feature is available or not is detected by an
atomic TEST_ONLY commit without ALLOW_MODESET.

> > However the exact modes amdgpu adds are just some guesses. I think it would be
> > great if user-space could control the min/max refresh rate values directly.

Setting min==max could be used to achieve the fixed refresh rate
proposed here, but it would also allow setting custom min < max limits.
This would be more flexible, but I'm not sure what the use case for it
could look like... oh, there are the use cases mentioned below: user
preferences. :-)

Maybe the min/max setting is better than fiddling with custom video
modes. If we have min/max to control, then there is no problem with
going back to the "original" video mode like in my example use case 1.

> > Not only this would remove the need for the kernel to hard-code "well-known
> > video refresh rates", but this would also enable more use-cases. For instance
> > some users might want to mitigate flickering on their screen by reducing the
> > VRR range. Some users might want to lower their screen refresh rate for power
> > savings.
> >
> > What do you think? Would you be fine with adding min/max VRR range properties?
> >
> > If you're scared about the user-space code requirement, I can
> > provide that.  
> 
> This sounds like a reasonable approach, and there is no reason why we
> can't do this if we have the proper userspace support as you
> mentioned.

Maybe the min/max controls are the way to go, considering that
the seamless refresh rate change feature in general cannot be
implemented without VRR. Or can it?

But if it can be implemented while not supporting VRR on some hardware,
then the video mode fiddling without ALLOW_MODESET is still a usable
approach. Or maybe such a driver could special-case VRR=enabled &&
min==max.

Yeah, min/max controls seems like the best idea to me so far.


Thanks,
pq

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

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Christian König]

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

Am 11.12.20 um 10:55 schrieb Pekka Paalanen:
> On Fri, 11 Dec 2020 09:56:07 +0530
> Shashank Sharma <shashank.sharma@amd.com> wrote:
>
>> Hello Simon,
>>
>> Hope you are doing well,
>>
>> I was helping out Aurabindo and the team with the design, so I have
>> taken the liberty of adding some comments on behalf of the team,
>> Inline.
>>
>> On 11/12/20 3:31 am, Simon Ser wrote:
>>> Hi,
>>>
>>> (CC dri-devel, Pekka and Martin who might be interested in this as
>>> well.)
> Thanks for the Cc! This is very interesting to me, and because it was
> not Cc'd to dri-devel@ originally, I would have missed this otherwise.
>
>>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>>>   
>>>> 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.
>>> Thanks for working on this, it's an interesting feature! However I'd like to
>>> take some time to think about the user-space API for this.
>>>
>>> As I understand it, some new synthetic modes are added, and user-space can
>>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
>>> switch to a mode without blanking the screen.
>> The implementation is in those lines, but a bit different. The idea
>> is to:
>>
>> - check if the monitor supports VRR,
>>
>> - If it does, add some new modes which are in the VRR tolerance
>> range, as new video modes in the list (with driver flag).
>>
>> - when you get modeset on any of these modes, skip the full modeset,
>> and just adjust the front_porch timing
>>
>> so they are not test-only as such, for any user-space these modes
>> will be as real as any other probed modes of the list.
> But is it worth to allow a modeset to be glitch-free if the userspace
> does not know they are glitch-free? I think if this is going in, it
> would be really useful to give the guarantees to userspace explicitly,
> and not leave this feature at an "accidentally no glitch sometimes"
> level.
>
>
> I have been expecting and hoping for the ability to change video mode
> timings without a modeset ever since I learnt that VRR is about
> front-porch adjustment, quite a while ago.
>
> This is how I envision userspace making use of it:
>
> Let us have a Wayland compositor, which uses fixed-frequency video
> modes, because it wants predictable vblank cycles. IOW, it will not
> enable VRR as such.

Well in general please keep in mind that this is just a short term 
solution for X11 applications.

For things like Wayland we probably want to approach this from a 
completely different vector.

> When the Wayland compositor starts, it will choose *some* video mode
> for an output. It may or may not be what a KMS driver calls "preferred
> mode", because it depends on things like user preferences. The
> compositor makes the initial modeset to this mode.

I think the general idea we settled on is that we specify an earliest 
display time for each frame and give feedback to the application when a 
frame was actually displayed.

This approach should also be able to handle multiple applications with 
different refresh rates. E.g. just think of a video playback with 25 and 
another one with 30 Hz in two windows when the max refresh rate is 
something like 120Hz.

Regards,
Christian.

>
> Use case 1:
>
> A Wayland client comes up and determines that its window would really
> like a refresh rate of, say, 47.5 Hz. Yes, it's not a traditional video
> player rate, but let's assume the application has its reasons. The
> client tells the compositor this (Wayland protocol still to be designed
> to be able to do that). (Hey, this could be how future games should
> implement refresh rate controls in cooperation with the window system.)
>
> The compositor sees the wish, and according to its complex policy
> rules, determines that yes, it shall try to honor that wish by changing
> the whole output temporarily to 47.5 Hz if possible.
>
> The compositor takes the original video mode it modeset on the output,
> and adjusts the front-porch to create a new custom 47.5 Hz mode. Using
> this mode, the compositor does a TEST_ONLY atomic commit *without*
> ALLOW_MODESET.
>
> If the test commit succeeds, the compositor knows that changing timings
> will not cause any kind of glitch, flicker, blanking, or freeze, and
> proceeds to commit this video mode without ALLOW_MODESET. The whole
> output becomes 47.5 Hz until the compositor policy again determines
> that it is time to change, e.g. to go back to the original mode. Going
> back to the original mode also needs to work without ALLOW_MODESET -
> but a compositor cannot test for this with atomic TEST_ONLY commits.
>
> If the test commit fails, the compositor knows that it cannot change
> the timings like this without risking a visible glitch. Therefore the
> compositor does not change the video mode timings, and the client's
> wish is not granted.
>
> The client adapts to whatever the refresh rate is in any case.
>
> Use case 2:
>
> A client comes up, and starts presenting frames with a target timestamp
> (Wayland protocol for this still to be designed). The compositor
> analyzes the target timestamp, and according to the complex compositor
> policy, determines that it should try to adjust video mode timings to
> better meet the target timestamps.
>
> Like in use case 1, the compositor creates a new custom video mode and
> tests if it can be applied without any glitch. If yes, it is used. If
> not, it is not used.
>
> This use case is more complicated, because the video mode timing
> changes may happen refresh by refresh, which means they need to
> apply for the very next front-porch in the scanout cycle in
> hardware. Hence, I'm not sure this use case is realistic. It can also
> be argued that this is better implemented by just enabling VRR and
> handling the flip timing in userspace, in the compositor: issue an
> atomic flip at the exact time it needs to be executed instead of
> issuing it well in advance and letting the driver wait for vblank.
>
>
> Worth to note: neither case needs the kernel to expose new manufactured
> video modes. Whether the feature is available or not is detected by an
> atomic TEST_ONLY commit without ALLOW_MODESET.
>
>>> However the exact modes amdgpu adds are just some guesses. I think it would be
>>> great if user-space could control the min/max refresh rate values directly.
> Setting min==max could be used to achieve the fixed refresh rate
> proposed here, but it would also allow setting custom min < max limits.
> This would be more flexible, but I'm not sure what the use case for it
> could look like... oh, there are the use cases mentioned below: user
> preferences. :-)
>
> Maybe the min/max setting is better than fiddling with custom video
> modes. If we have min/max to control, then there is no problem with
> going back to the "original" video mode like in my example use case 1.
>
>>> Not only this would remove the need for the kernel to hard-code "well-known
>>> video refresh rates", but this would also enable more use-cases. For instance
>>> some users might want to mitigate flickering on their screen by reducing the
>>> VRR range. Some users might want to lower their screen refresh rate for power
>>> savings.
>>>
>>> What do you think? Would you be fine with adding min/max VRR range properties?
>>>
>>> If you're scared about the user-space code requirement, I can
>>> provide that.
>> This sounds like a reasonable approach, and there is no reason why we
>> can't do this if we have the proper userspace support as you
>> mentioned.
> Maybe the min/max controls are the way to go, considering that
> the seamless refresh rate change feature in general cannot be
> implemented without VRR. Or can it?
>
> But if it can be implemented while not supporting VRR on some hardware,
> then the video mode fiddling without ALLOW_MODESET is still a usable
> approach. Or maybe such a driver could special-case VRR=enabled &&
> min==max.
>
> Yeah, min/max controls seems like the best idea to me so far.
>
>
> Thanks,
> pq
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 10523 bytes --]

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Christian König]

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

Am 11.12.20 um 10:55 schrieb Pekka Paalanen:
> On Fri, 11 Dec 2020 09:56:07 +0530
> Shashank Sharma <shashank.sharma@amd.com> wrote:
>
>> Hello Simon,
>>
>> Hope you are doing well,
>>
>> I was helping out Aurabindo and the team with the design, so I have
>> taken the liberty of adding some comments on behalf of the team,
>> Inline.
>>
>> On 11/12/20 3:31 am, Simon Ser wrote:
>>> Hi,
>>>
>>> (CC dri-devel, Pekka and Martin who might be interested in this as
>>> well.)
> Thanks for the Cc! This is very interesting to me, and because it was
> not Cc'd to dri-devel@ originally, I would have missed this otherwise.
>
>>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>>>   
>>>> 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.
>>> Thanks for working on this, it's an interesting feature! However I'd like to
>>> take some time to think about the user-space API for this.
>>>
>>> As I understand it, some new synthetic modes are added, and user-space can
>>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
>>> switch to a mode without blanking the screen.
>> The implementation is in those lines, but a bit different. The idea
>> is to:
>>
>> - check if the monitor supports VRR,
>>
>> - If it does, add some new modes which are in the VRR tolerance
>> range, as new video modes in the list (with driver flag).
>>
>> - when you get modeset on any of these modes, skip the full modeset,
>> and just adjust the front_porch timing
>>
>> so they are not test-only as such, for any user-space these modes
>> will be as real as any other probed modes of the list.
> But is it worth to allow a modeset to be glitch-free if the userspace
> does not know they are glitch-free? I think if this is going in, it
> would be really useful to give the guarantees to userspace explicitly,
> and not leave this feature at an "accidentally no glitch sometimes"
> level.
>
>
> I have been expecting and hoping for the ability to change video mode
> timings without a modeset ever since I learnt that VRR is about
> front-porch adjustment, quite a while ago.
>
> This is how I envision userspace making use of it:
>
> Let us have a Wayland compositor, which uses fixed-frequency video
> modes, because it wants predictable vblank cycles. IOW, it will not
> enable VRR as such.

Well in general please keep in mind that this is just a short term 
solution for X11 applications.

For things like Wayland we probably want to approach this from a 
completely different vector.

> When the Wayland compositor starts, it will choose *some* video mode
> for an output. It may or may not be what a KMS driver calls "preferred
> mode", because it depends on things like user preferences. The
> compositor makes the initial modeset to this mode.

I think the general idea we settled on is that we specify an earliest 
display time for each frame and give feedback to the application when a 
frame was actually displayed.

This approach should also be able to handle multiple applications with 
different refresh rates. E.g. just think of a video playback with 25 and 
another one with 30 Hz in two windows when the max refresh rate is 
something like 120Hz.

Regards,
Christian.

>
> Use case 1:
>
> A Wayland client comes up and determines that its window would really
> like a refresh rate of, say, 47.5 Hz. Yes, it's not a traditional video
> player rate, but let's assume the application has its reasons. The
> client tells the compositor this (Wayland protocol still to be designed
> to be able to do that). (Hey, this could be how future games should
> implement refresh rate controls in cooperation with the window system.)
>
> The compositor sees the wish, and according to its complex policy
> rules, determines that yes, it shall try to honor that wish by changing
> the whole output temporarily to 47.5 Hz if possible.
>
> The compositor takes the original video mode it modeset on the output,
> and adjusts the front-porch to create a new custom 47.5 Hz mode. Using
> this mode, the compositor does a TEST_ONLY atomic commit *without*
> ALLOW_MODESET.
>
> If the test commit succeeds, the compositor knows that changing timings
> will not cause any kind of glitch, flicker, blanking, or freeze, and
> proceeds to commit this video mode without ALLOW_MODESET. The whole
> output becomes 47.5 Hz until the compositor policy again determines
> that it is time to change, e.g. to go back to the original mode. Going
> back to the original mode also needs to work without ALLOW_MODESET -
> but a compositor cannot test for this with atomic TEST_ONLY commits.
>
> If the test commit fails, the compositor knows that it cannot change
> the timings like this without risking a visible glitch. Therefore the
> compositor does not change the video mode timings, and the client's
> wish is not granted.
>
> The client adapts to whatever the refresh rate is in any case.
>
> Use case 2:
>
> A client comes up, and starts presenting frames with a target timestamp
> (Wayland protocol for this still to be designed). The compositor
> analyzes the target timestamp, and according to the complex compositor
> policy, determines that it should try to adjust video mode timings to
> better meet the target timestamps.
>
> Like in use case 1, the compositor creates a new custom video mode and
> tests if it can be applied without any glitch. If yes, it is used. If
> not, it is not used.
>
> This use case is more complicated, because the video mode timing
> changes may happen refresh by refresh, which means they need to
> apply for the very next front-porch in the scanout cycle in
> hardware. Hence, I'm not sure this use case is realistic. It can also
> be argued that this is better implemented by just enabling VRR and
> handling the flip timing in userspace, in the compositor: issue an
> atomic flip at the exact time it needs to be executed instead of
> issuing it well in advance and letting the driver wait for vblank.
>
>
> Worth to note: neither case needs the kernel to expose new manufactured
> video modes. Whether the feature is available or not is detected by an
> atomic TEST_ONLY commit without ALLOW_MODESET.
>
>>> However the exact modes amdgpu adds are just some guesses. I think it would be
>>> great if user-space could control the min/max refresh rate values directly.
> Setting min==max could be used to achieve the fixed refresh rate
> proposed here, but it would also allow setting custom min < max limits.
> This would be more flexible, but I'm not sure what the use case for it
> could look like... oh, there are the use cases mentioned below: user
> preferences. :-)
>
> Maybe the min/max setting is better than fiddling with custom video
> modes. If we have min/max to control, then there is no problem with
> going back to the "original" video mode like in my example use case 1.
>
>>> Not only this would remove the need for the kernel to hard-code "well-known
>>> video refresh rates", but this would also enable more use-cases. For instance
>>> some users might want to mitigate flickering on their screen by reducing the
>>> VRR range. Some users might want to lower their screen refresh rate for power
>>> savings.
>>>
>>> What do you think? Would you be fine with adding min/max VRR range properties?
>>>
>>> If you're scared about the user-space code requirement, I can
>>> provide that.
>> This sounds like a reasonable approach, and there is no reason why we
>> can't do this if we have the proper userspace support as you
>> mentioned.
> Maybe the min/max controls are the way to go, considering that
> the seamless refresh rate change feature in general cannot be
> implemented without VRR. Or can it?
>
> But if it can be implemented while not supporting VRR on some hardware,
> then the video mode fiddling without ALLOW_MODESET is still a usable
> approach. Or maybe such a driver could special-case VRR=enabled &&
> min==max.
>
> Yeah, min/max controls seems like the best idea to me so far.
>
>
> Thanks,
> pq
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 10523 bytes --]

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Pekka Paalanen]

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

On Fri, 11 Dec 2020 11:28:36 +0100
Christian König <ckoenig.leichtzumerken@gmail.com> wrote:

> Am 11.12.20 um 10:55 schrieb Pekka Paalanen:
> > On Fri, 11 Dec 2020 09:56:07 +0530
> > Shashank Sharma <shashank.sharma@amd.com> wrote:
> >  
> >> Hello Simon,
> >>
> >> Hope you are doing well,
> >>
> >> I was helping out Aurabindo and the team with the design, so I have
> >> taken the liberty of adding some comments on behalf of the team,
> >> Inline.
> >>
> >> On 11/12/20 3:31 am, Simon Ser wrote:  
> >>> Hi,
> >>>
> >>> (CC dri-devel, Pekka and Martin who might be interested in this as
> >>> well.)  
> > Thanks for the Cc! This is very interesting to me, and because it was
> > not Cc'd to dri-devel@ originally, I would have missed this otherwise.
> >  
> >>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> >>>     
> >>>> 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.  
> >>> Thanks for working on this, it's an interesting feature! However I'd like to
> >>> take some time to think about the user-space API for this.
> >>>
> >>> As I understand it, some new synthetic modes are added, and user-space can
> >>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
> >>> switch to a mode without blanking the screen.  
> >> The implementation is in those lines, but a bit different. The idea
> >> is to:
> >>
> >> - check if the monitor supports VRR,
> >>
> >> - If it does, add some new modes which are in the VRR tolerance
> >> range, as new video modes in the list (with driver flag).
> >>
> >> - when you get modeset on any of these modes, skip the full modeset,
> >> and just adjust the front_porch timing
> >>
> >> so they are not test-only as such, for any user-space these modes
> >> will be as real as any other probed modes of the list.  
> > But is it worth to allow a modeset to be glitch-free if the userspace
> > does not know they are glitch-free? I think if this is going in, it
> > would be really useful to give the guarantees to userspace explicitly,
> > and not leave this feature at an "accidentally no glitch sometimes"
> > level.
> >
> >
> > I have been expecting and hoping for the ability to change video mode
> > timings without a modeset ever since I learnt that VRR is about
> > front-porch adjustment, quite a while ago.
> >
> > This is how I envision userspace making use of it:
> >
> > Let us have a Wayland compositor, which uses fixed-frequency video
> > modes, because it wants predictable vblank cycles. IOW, it will not
> > enable VRR as such.  
> 
> Well in general please keep in mind that this is just a short term 
> solution for X11 applications.

I guess someone should have mentioned that. :-)

Do we really want to add more Xorg-only features in the kernel?

It feels like "it's a short term solution for X11" could be almost used
as an excuse to avoid proper uAPI design process. However, with uAPI
there is no "short term". Once it's in, it's there for decades. So why
does it matter if you design it for Xorg foremost? Are others forbidden
to make use of it? Or do you deliberately want to design it so that
it's not generally useful and it will indeed end up being a short term
feature? Planned obsolescence from the start?

> For things like Wayland we probably want to approach this from a 
> completely different vector.
> 
> > When the Wayland compositor starts, it will choose *some* video mode
> > for an output. It may or may not be what a KMS driver calls "preferred
> > mode", because it depends on things like user preferences. The
> > compositor makes the initial modeset to this mode.  
> 
> I think the general idea we settled on is that we specify an earliest 
> display time for each frame and give feedback to the application when a 
> frame was actually displayed.

That is indeed something completely different, and feels like it would
be several years in the future, while the proposed scheme or the
min/max properties could be done fairly quickly. But I'm not in a hurry.

Setting the earliest display time for a flip does not fully cover what
video mode timing adjustment or min/max frame time or refresh rate
property would offer: prediction of when the next flip can happen.
Choosing display times requires knowing the possible display times, so
something more is needed. The min/max properties would fit in.

> This approach should also be able to handle multiple applications with 
> different refresh rates. E.g. just think of a video playback with 25 and 
> another one with 30 Hz in two windows when the max refresh rate is 
> something like 120Hz.

But that's just an extension to what I already described and completely
possible with the proposed and the min/max approaches.


Thanks,
pq

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

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Pekka Paalanen]

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

On Fri, 11 Dec 2020 11:28:36 +0100
Christian König <ckoenig.leichtzumerken@gmail.com> wrote:

> Am 11.12.20 um 10:55 schrieb Pekka Paalanen:
> > On Fri, 11 Dec 2020 09:56:07 +0530
> > Shashank Sharma <shashank.sharma@amd.com> wrote:
> >  
> >> Hello Simon,
> >>
> >> Hope you are doing well,
> >>
> >> I was helping out Aurabindo and the team with the design, so I have
> >> taken the liberty of adding some comments on behalf of the team,
> >> Inline.
> >>
> >> On 11/12/20 3:31 am, Simon Ser wrote:  
> >>> Hi,
> >>>
> >>> (CC dri-devel, Pekka and Martin who might be interested in this as
> >>> well.)  
> > Thanks for the Cc! This is very interesting to me, and because it was
> > not Cc'd to dri-devel@ originally, I would have missed this otherwise.
> >  
> >>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> >>>     
> >>>> 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.  
> >>> Thanks for working on this, it's an interesting feature! However I'd like to
> >>> take some time to think about the user-space API for this.
> >>>
> >>> As I understand it, some new synthetic modes are added, and user-space can
> >>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
> >>> switch to a mode without blanking the screen.  
> >> The implementation is in those lines, but a bit different. The idea
> >> is to:
> >>
> >> - check if the monitor supports VRR,
> >>
> >> - If it does, add some new modes which are in the VRR tolerance
> >> range, as new video modes in the list (with driver flag).
> >>
> >> - when you get modeset on any of these modes, skip the full modeset,
> >> and just adjust the front_porch timing
> >>
> >> so they are not test-only as such, for any user-space these modes
> >> will be as real as any other probed modes of the list.  
> > But is it worth to allow a modeset to be glitch-free if the userspace
> > does not know they are glitch-free? I think if this is going in, it
> > would be really useful to give the guarantees to userspace explicitly,
> > and not leave this feature at an "accidentally no glitch sometimes"
> > level.
> >
> >
> > I have been expecting and hoping for the ability to change video mode
> > timings without a modeset ever since I learnt that VRR is about
> > front-porch adjustment, quite a while ago.
> >
> > This is how I envision userspace making use of it:
> >
> > Let us have a Wayland compositor, which uses fixed-frequency video
> > modes, because it wants predictable vblank cycles. IOW, it will not
> > enable VRR as such.  
> 
> Well in general please keep in mind that this is just a short term 
> solution for X11 applications.

I guess someone should have mentioned that. :-)

Do we really want to add more Xorg-only features in the kernel?

It feels like "it's a short term solution for X11" could be almost used
as an excuse to avoid proper uAPI design process. However, with uAPI
there is no "short term". Once it's in, it's there for decades. So why
does it matter if you design it for Xorg foremost? Are others forbidden
to make use of it? Or do you deliberately want to design it so that
it's not generally useful and it will indeed end up being a short term
feature? Planned obsolescence from the start?

> For things like Wayland we probably want to approach this from a 
> completely different vector.
> 
> > When the Wayland compositor starts, it will choose *some* video mode
> > for an output. It may or may not be what a KMS driver calls "preferred
> > mode", because it depends on things like user preferences. The
> > compositor makes the initial modeset to this mode.  
> 
> I think the general idea we settled on is that we specify an earliest 
> display time for each frame and give feedback to the application when a 
> frame was actually displayed.

That is indeed something completely different, and feels like it would
be several years in the future, while the proposed scheme or the
min/max properties could be done fairly quickly. But I'm not in a hurry.

Setting the earliest display time for a flip does not fully cover what
video mode timing adjustment or min/max frame time or refresh rate
property would offer: prediction of when the next flip can happen.
Choosing display times requires knowing the possible display times, so
something more is needed. The min/max properties would fit in.

> This approach should also be able to handle multiple applications with 
> different refresh rates. E.g. just think of a video playback with 25 and 
> another one with 30 Hz in two windows when the max refresh rate is 
> something like 120Hz.

But that's just an extension to what I already described and completely
possible with the proposed and the min/max approaches.


Thanks,
pq

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

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Christian König]

Am 11.12.20 um 13:20 schrieb Pekka Paalanen:
> On Fri, 11 Dec 2020 11:28:36 +0100
> Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>
>> Am 11.12.20 um 10:55 schrieb Pekka Paalanen:
>>> On Fri, 11 Dec 2020 09:56:07 +0530
>>> Shashank Sharma <shashank.sharma@amd.com> wrote:
>>>   
>>>> Hello Simon,
>>>>
>>>> Hope you are doing well,
>>>>
>>>> I was helping out Aurabindo and the team with the design, so I have
>>>> taken the liberty of adding some comments on behalf of the team,
>>>> Inline.
>>>>
>>>> On 11/12/20 3:31 am, Simon Ser wrote:
>>>>> Hi,
>>>>>
>>>>> (CC dri-devel, Pekka and Martin who might be interested in this as
>>>>> well.)
>>> Thanks for the Cc! This is very interesting to me, and because it was
>>> not Cc'd to dri-devel@ originally, I would have missed this otherwise.
>>>   
>>>>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>>>>>      
>>>>>> 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.
>>>>> Thanks for working on this, it's an interesting feature! However I'd like to
>>>>> take some time to think about the user-space API for this.
>>>>>
>>>>> As I understand it, some new synthetic modes are added, and user-space can
>>>>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
>>>>> switch to a mode without blanking the screen.
>>>> The implementation is in those lines, but a bit different. The idea
>>>> is to:
>>>>
>>>> - check if the monitor supports VRR,
>>>>
>>>> - If it does, add some new modes which are in the VRR tolerance
>>>> range, as new video modes in the list (with driver flag).
>>>>
>>>> - when you get modeset on any of these modes, skip the full modeset,
>>>> and just adjust the front_porch timing
>>>>
>>>> so they are not test-only as such, for any user-space these modes
>>>> will be as real as any other probed modes of the list.
>>> But is it worth to allow a modeset to be glitch-free if the userspace
>>> does not know they are glitch-free? I think if this is going in, it
>>> would be really useful to give the guarantees to userspace explicitly,
>>> and not leave this feature at an "accidentally no glitch sometimes"
>>> level.
>>>
>>>
>>> I have been expecting and hoping for the ability to change video mode
>>> timings without a modeset ever since I learnt that VRR is about
>>> front-porch adjustment, quite a while ago.
>>>
>>> This is how I envision userspace making use of it:
>>>
>>> Let us have a Wayland compositor, which uses fixed-frequency video
>>> modes, because it wants predictable vblank cycles. IOW, it will not
>>> enable VRR as such.
>> Well in general please keep in mind that this is just a short term
>> solution for X11 applications.
> I guess someone should have mentioned that. :-)
>
> Do we really want to add more Xorg-only features in the kernel?

Well, my preferred solution was to add the mode in userspace instead :)

> It feels like "it's a short term solution for X11" could be almost used
> as an excuse to avoid proper uAPI design process. However, with uAPI
> there is no "short term". Once it's in, it's there for decades. So why
> does it matter if you design it for Xorg foremost? Are others forbidden
> to make use of it? Or do you deliberately want to design it so that
> it's not generally useful and it will indeed end up being a short term
> feature? Planned obsolescence from the start?

Yes exactly. We need something which works for now and can be removed 
again later on when we have a better solution. Adding some extra modes 
is not considered UAPI since both displays and drivers are doing this 
for a couple of different purposes already.

Another main reason is that this approach works with existing media 
players like mpv and kodi without changing them because we do pretty 
much the same thing in the kernel which TVs do in their EDID.

E.g. when starting to play a video kodi will automatically try to switch 
to a mode which has the same fps as the video.

>
>> For things like Wayland we probably want to approach this from a
>> completely different vector.
>>
>>> When the Wayland compositor starts, it will choose *some* video mode
>>> for an output. It may or may not be what a KMS driver calls "preferred
>>> mode", because it depends on things like user preferences. The
>>> compositor makes the initial modeset to this mode.
>> I think the general idea we settled on is that we specify an earliest
>> display time for each frame and give feedback to the application when a
>> frame was actually displayed.
> That is indeed something completely different, and feels like it would
> be several years in the future, while the proposed scheme or the
> min/max properties could be done fairly quickly. But I'm not in a hurry.

X11 already supports that with the DRI3 extension. Also see VDPAU 
present and the Vulkan extension for reference application API 
implementations, so that stuff is already present.

It's just not wired up correctly with KMS and we don't have anything 
similar in Wayland as far as I know.

> Setting the earliest display time for a flip does not fully cover what
> video mode timing adjustment or min/max frame time or refresh rate
> property would offer: prediction of when the next flip can happen.
> Choosing display times requires knowing the possible display times, so
> something more is needed. The min/max properties would fit in.

Well you need to communicate to the application what the minimum and 
maximum refresh rates are by some extension of the mode description.

Regards,
Christian.


>
>> This approach should also be able to handle multiple applications with
>> different refresh rates. E.g. just think of a video playback with 25 and
>> another one with 30 Hz in two windows when the max refresh rate is
>> something like 120Hz.
> But that's just an extension to what I already described and completely
> possible with the proposed and the min/max approaches.
>
>
> Thanks,
> pq

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Christian König]

Am 11.12.20 um 13:20 schrieb Pekka Paalanen:
> On Fri, 11 Dec 2020 11:28:36 +0100
> Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>
>> Am 11.12.20 um 10:55 schrieb Pekka Paalanen:
>>> On Fri, 11 Dec 2020 09:56:07 +0530
>>> Shashank Sharma <shashank.sharma@amd.com> wrote:
>>>   
>>>> Hello Simon,
>>>>
>>>> Hope you are doing well,
>>>>
>>>> I was helping out Aurabindo and the team with the design, so I have
>>>> taken the liberty of adding some comments on behalf of the team,
>>>> Inline.
>>>>
>>>> On 11/12/20 3:31 am, Simon Ser wrote:
>>>>> Hi,
>>>>>
>>>>> (CC dri-devel, Pekka and Martin who might be interested in this as
>>>>> well.)
>>> Thanks for the Cc! This is very interesting to me, and because it was
>>> not Cc'd to dri-devel@ originally, I would have missed this otherwise.
>>>   
>>>>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>>>>>      
>>>>>> 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.
>>>>> Thanks for working on this, it's an interesting feature! However I'd like to
>>>>> take some time to think about the user-space API for this.
>>>>>
>>>>> As I understand it, some new synthetic modes are added, and user-space can
>>>>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
>>>>> switch to a mode without blanking the screen.
>>>> The implementation is in those lines, but a bit different. The idea
>>>> is to:
>>>>
>>>> - check if the monitor supports VRR,
>>>>
>>>> - If it does, add some new modes which are in the VRR tolerance
>>>> range, as new video modes in the list (with driver flag).
>>>>
>>>> - when you get modeset on any of these modes, skip the full modeset,
>>>> and just adjust the front_porch timing
>>>>
>>>> so they are not test-only as such, for any user-space these modes
>>>> will be as real as any other probed modes of the list.
>>> But is it worth to allow a modeset to be glitch-free if the userspace
>>> does not know they are glitch-free? I think if this is going in, it
>>> would be really useful to give the guarantees to userspace explicitly,
>>> and not leave this feature at an "accidentally no glitch sometimes"
>>> level.
>>>
>>>
>>> I have been expecting and hoping for the ability to change video mode
>>> timings without a modeset ever since I learnt that VRR is about
>>> front-porch adjustment, quite a while ago.
>>>
>>> This is how I envision userspace making use of it:
>>>
>>> Let us have a Wayland compositor, which uses fixed-frequency video
>>> modes, because it wants predictable vblank cycles. IOW, it will not
>>> enable VRR as such.
>> Well in general please keep in mind that this is just a short term
>> solution for X11 applications.
> I guess someone should have mentioned that. :-)
>
> Do we really want to add more Xorg-only features in the kernel?

Well, my preferred solution was to add the mode in userspace instead :)

> It feels like "it's a short term solution for X11" could be almost used
> as an excuse to avoid proper uAPI design process. However, with uAPI
> there is no "short term". Once it's in, it's there for decades. So why
> does it matter if you design it for Xorg foremost? Are others forbidden
> to make use of it? Or do you deliberately want to design it so that
> it's not generally useful and it will indeed end up being a short term
> feature? Planned obsolescence from the start?

Yes exactly. We need something which works for now and can be removed 
again later on when we have a better solution. Adding some extra modes 
is not considered UAPI since both displays and drivers are doing this 
for a couple of different purposes already.

Another main reason is that this approach works with existing media 
players like mpv and kodi without changing them because we do pretty 
much the same thing in the kernel which TVs do in their EDID.

E.g. when starting to play a video kodi will automatically try to switch 
to a mode which has the same fps as the video.

>
>> For things like Wayland we probably want to approach this from a
>> completely different vector.
>>
>>> When the Wayland compositor starts, it will choose *some* video mode
>>> for an output. It may or may not be what a KMS driver calls "preferred
>>> mode", because it depends on things like user preferences. The
>>> compositor makes the initial modeset to this mode.
>> I think the general idea we settled on is that we specify an earliest
>> display time for each frame and give feedback to the application when a
>> frame was actually displayed.
> That is indeed something completely different, and feels like it would
> be several years in the future, while the proposed scheme or the
> min/max properties could be done fairly quickly. But I'm not in a hurry.

X11 already supports that with the DRI3 extension. Also see VDPAU 
present and the Vulkan extension for reference application API 
implementations, so that stuff is already present.

It's just not wired up correctly with KMS and we don't have anything 
similar in Wayland as far as I know.

> Setting the earliest display time for a flip does not fully cover what
> video mode timing adjustment or min/max frame time or refresh rate
> property would offer: prediction of when the next flip can happen.
> Choosing display times requires knowing the possible display times, so
> something more is needed. The min/max properties would fit in.

Well you need to communicate to the application what the minimum and 
maximum refresh rates are by some extension of the mode description.

Regards,
Christian.


>
>> This approach should also be able to handle multiple applications with
>> different refresh rates. E.g. just think of a video playback with 25 and
>> another one with 30 Hz in two windows when the max refresh rate is
>> something like 120Hz.
> But that's just an extension to what I already described and completely
> possible with the proposed and the min/max approaches.
>
>
> Thanks,
> pq

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Pekka Paalanen]

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

On Mon, 14 Dec 2020 21:57:25 +0100
Christian König <christian.koenig@amd.com> wrote:

> Am 11.12.20 um 13:20 schrieb Pekka Paalanen:
> > On Fri, 11 Dec 2020 11:28:36 +0100
> > Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
> >  
> >> Am 11.12.20 um 10:55 schrieb Pekka Paalanen:  
> >>> On Fri, 11 Dec 2020 09:56:07 +0530
> >>> Shashank Sharma <shashank.sharma@amd.com> wrote:
> >>>     
> >>>> Hello Simon,
> >>>>
> >>>> Hope you are doing well,
> >>>>
> >>>> I was helping out Aurabindo and the team with the design, so I have
> >>>> taken the liberty of adding some comments on behalf of the team,
> >>>> Inline.
> >>>>
> >>>> On 11/12/20 3:31 am, Simon Ser wrote:  
> >>>>> Hi,
> >>>>>
> >>>>> (CC dri-devel, Pekka and Martin who might be interested in this as
> >>>>> well.)  
> >>> Thanks for the Cc! This is very interesting to me, and because it was
> >>> not Cc'd to dri-devel@ originally, I would have missed this otherwise.
> >>>     
> >>>>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> >>>>>        
> >>>>>> 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.  
> >>>>> Thanks for working on this, it's an interesting feature! However I'd like to
> >>>>> take some time to think about the user-space API for this.
> >>>>>
> >>>>> As I understand it, some new synthetic modes are added, and user-space can
> >>>>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
> >>>>> switch to a mode without blanking the screen.  
> >>>> The implementation is in those lines, but a bit different. The idea
> >>>> is to:
> >>>>
> >>>> - check if the monitor supports VRR,
> >>>>
> >>>> - If it does, add some new modes which are in the VRR tolerance
> >>>> range, as new video modes in the list (with driver flag).
> >>>>
> >>>> - when you get modeset on any of these modes, skip the full modeset,
> >>>> and just adjust the front_porch timing
> >>>>
> >>>> so they are not test-only as such, for any user-space these modes
> >>>> will be as real as any other probed modes of the list.  
> >>> But is it worth to allow a modeset to be glitch-free if the userspace
> >>> does not know they are glitch-free? I think if this is going in, it
> >>> would be really useful to give the guarantees to userspace explicitly,
> >>> and not leave this feature at an "accidentally no glitch sometimes"
> >>> level.
> >>>
> >>>
> >>> I have been expecting and hoping for the ability to change video mode
> >>> timings without a modeset ever since I learnt that VRR is about
> >>> front-porch adjustment, quite a while ago.
> >>>
> >>> This is how I envision userspace making use of it:
> >>>
> >>> Let us have a Wayland compositor, which uses fixed-frequency video
> >>> modes, because it wants predictable vblank cycles. IOW, it will not
> >>> enable VRR as such.  
> >> Well in general please keep in mind that this is just a short term
> >> solution for X11 applications.  
> > I guess someone should have mentioned that. :-)
> >
> > Do we really want to add more Xorg-only features in the kernel?  
> 
> Well, my preferred solution was to add the mode in userspace instead :)
> 
> > It feels like "it's a short term solution for X11" could be almost used
> > as an excuse to avoid proper uAPI design process. However, with uAPI
> > there is no "short term". Once it's in, it's there for decades. So why
> > does it matter if you design it for Xorg foremost? Are others forbidden
> > to make use of it? Or do you deliberately want to design it so that
> > it's not generally useful and it will indeed end up being a short term
> > feature? Planned obsolescence from the start?  
> 
> Yes exactly. We need something which works for now and can be removed 
> again later on when we have a better solution. Adding some extra modes 
> is not considered UAPI since both displays and drivers are doing this 
> for a couple of different purposes already.
> 
> Another main reason is that this approach works with existing media 
> players like mpv and kodi without changing them because we do pretty 
> much the same thing in the kernel which TVs do in their EDID.
> 
> E.g. when starting to play a video kodi will automatically try to switch 
> to a mode which has the same fps as the video.

Aha! That is very valuable information to put this proposal into
perspective. I'll leave you to it, then. :-)


Thanks,
pq

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

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Pekka Paalanen]

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

On Mon, 14 Dec 2020 21:57:25 +0100
Christian König <christian.koenig@amd.com> wrote:

> Am 11.12.20 um 13:20 schrieb Pekka Paalanen:
> > On Fri, 11 Dec 2020 11:28:36 +0100
> > Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
> >  
> >> Am 11.12.20 um 10:55 schrieb Pekka Paalanen:  
> >>> On Fri, 11 Dec 2020 09:56:07 +0530
> >>> Shashank Sharma <shashank.sharma@amd.com> wrote:
> >>>     
> >>>> Hello Simon,
> >>>>
> >>>> Hope you are doing well,
> >>>>
> >>>> I was helping out Aurabindo and the team with the design, so I have
> >>>> taken the liberty of adding some comments on behalf of the team,
> >>>> Inline.
> >>>>
> >>>> On 11/12/20 3:31 am, Simon Ser wrote:  
> >>>>> Hi,
> >>>>>
> >>>>> (CC dri-devel, Pekka and Martin who might be interested in this as
> >>>>> well.)  
> >>> Thanks for the Cc! This is very interesting to me, and because it was
> >>> not Cc'd to dri-devel@ originally, I would have missed this otherwise.
> >>>     
> >>>>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> >>>>>        
> >>>>>> 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.  
> >>>>> Thanks for working on this, it's an interesting feature! However I'd like to
> >>>>> take some time to think about the user-space API for this.
> >>>>>
> >>>>> As I understand it, some new synthetic modes are added, and user-space can
> >>>>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can
> >>>>> switch to a mode without blanking the screen.  
> >>>> The implementation is in those lines, but a bit different. The idea
> >>>> is to:
> >>>>
> >>>> - check if the monitor supports VRR,
> >>>>
> >>>> - If it does, add some new modes which are in the VRR tolerance
> >>>> range, as new video modes in the list (with driver flag).
> >>>>
> >>>> - when you get modeset on any of these modes, skip the full modeset,
> >>>> and just adjust the front_porch timing
> >>>>
> >>>> so they are not test-only as such, for any user-space these modes
> >>>> will be as real as any other probed modes of the list.  
> >>> But is it worth to allow a modeset to be glitch-free if the userspace
> >>> does not know they are glitch-free? I think if this is going in, it
> >>> would be really useful to give the guarantees to userspace explicitly,
> >>> and not leave this feature at an "accidentally no glitch sometimes"
> >>> level.
> >>>
> >>>
> >>> I have been expecting and hoping for the ability to change video mode
> >>> timings without a modeset ever since I learnt that VRR is about
> >>> front-porch adjustment, quite a while ago.
> >>>
> >>> This is how I envision userspace making use of it:
> >>>
> >>> Let us have a Wayland compositor, which uses fixed-frequency video
> >>> modes, because it wants predictable vblank cycles. IOW, it will not
> >>> enable VRR as such.  
> >> Well in general please keep in mind that this is just a short term
> >> solution for X11 applications.  
> > I guess someone should have mentioned that. :-)
> >
> > Do we really want to add more Xorg-only features in the kernel?  
> 
> Well, my preferred solution was to add the mode in userspace instead :)
> 
> > It feels like "it's a short term solution for X11" could be almost used
> > as an excuse to avoid proper uAPI design process. However, with uAPI
> > there is no "short term". Once it's in, it's there for decades. So why
> > does it matter if you design it for Xorg foremost? Are others forbidden
> > to make use of it? Or do you deliberately want to design it so that
> > it's not generally useful and it will indeed end up being a short term
> > feature? Planned obsolescence from the start?  
> 
> Yes exactly. We need something which works for now and can be removed 
> again later on when we have a better solution. Adding some extra modes 
> is not considered UAPI since both displays and drivers are doing this 
> for a couple of different purposes already.
> 
> Another main reason is that this approach works with existing media 
> players like mpv and kodi without changing them because we do pretty 
> much the same thing in the kernel which TVs do in their EDID.
> 
> E.g. when starting to play a video kodi will automatically try to switch 
> to a mode which has the same fps as the video.

Aha! That is very valuable information to put this proposal into
perspective. I'll leave you to it, then. :-)


Thanks,
pq

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

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

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Michel Dänzer]

On 2020-12-14 9:57 p.m., Christian König wrote:
> Am 11.12.20 um 13:20 schrieb Pekka Paalanen:
>> On Fri, 11 Dec 2020 11:28:36 +0100
>> Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>>
>>> I think the general idea we settled on is that we specify an earliest
>>> display time for each frame and give feedback to the application when a
>>> frame was actually displayed.
>> That is indeed something completely different, and feels like it would
>> be several years in the future, while the proposed scheme or the
>> min/max properties could be done fairly quickly. But I'm not in a hurry.
> 
> X11 already supports that with the DRI3 extension. Also see VDPAU 
> present and the Vulkan extension for reference application API 
> implementations, so that stuff is already present.

I suspect you mean the Present extension, specifically PresentOptionUST. 
There is no working implementation of this yet (the xserver tree has 
never had any code which would even advertise PresentCapabilityUST, let 
alone do anything with a timestamp passed in by the client).


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Michel Dänzer]

On 2020-12-14 9:57 p.m., Christian König wrote:
> Am 11.12.20 um 13:20 schrieb Pekka Paalanen:
>> On Fri, 11 Dec 2020 11:28:36 +0100
>> Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>>
>>> I think the general idea we settled on is that we specify an earliest
>>> display time for each frame and give feedback to the application when a
>>> frame was actually displayed.
>> That is indeed something completely different, and feels like it would
>> be several years in the future, while the proposed scheme or the
>> min/max properties could be done fairly quickly. But I'm not in a hurry.
> 
> X11 already supports that with the DRI3 extension. Also see VDPAU 
> present and the Vulkan extension for reference application API 
> implementations, so that stuff is already present.

I suspect you mean the Present extension, specifically PresentOptionUST. 
There is no working implementation of this yet (the xserver tree has 
never had any code which would even advertise PresentCapabilityUST, let 
alone do anything with a timestamp passed in by the client).


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Christian König]

Am 11.12.20 um 14:58 schrieb Michel Dänzer:
> On 2020-12-14 9:57 p.m., Christian König wrote:
>> Am 11.12.20 um 13:20 schrieb Pekka Paalanen:
>>> On Fri, 11 Dec 2020 11:28:36 +0100
>>> Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>>>
>>>> I think the general idea we settled on is that we specify an earliest
>>>> display time for each frame and give feedback to the application 
>>>> when a
>>>> frame was actually displayed.
>>> That is indeed something completely different, and feels like it would
>>> be several years in the future, while the proposed scheme or the
>>> min/max properties could be done fairly quickly. But I'm not in a 
>>> hurry.
>>
>> X11 already supports that with the DRI3 extension. Also see VDPAU 
>> present and the Vulkan extension for reference application API 
>> implementations, so that stuff is already present.
>
> I suspect you mean the Present extension, specifically 
> PresentOptionUST. There is no working implementation of this yet (the 
> xserver tree has never had any code which would even advertise 
> PresentCapabilityUST, let alone do anything with a timestamp passed in 
> by the client).

Good point, what I wanted to say is that this is already specified in 
those extensions.

What we are missing is somehow wiring it all together and see how it works.

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

Re: [PATCH v2 0/3] Experimental freesync video mode optimization

[Christian König]

Am 11.12.20 um 14:58 schrieb Michel Dänzer:
> On 2020-12-14 9:57 p.m., Christian König wrote:
>> Am 11.12.20 um 13:20 schrieb Pekka Paalanen:
>>> On Fri, 11 Dec 2020 11:28:36 +0100
>>> Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>>>
>>>> I think the general idea we settled on is that we specify an earliest
>>>> display time for each frame and give feedback to the application 
>>>> when a
>>>> frame was actually displayed.
>>> That is indeed something completely different, and feels like it would
>>> be several years in the future, while the proposed scheme or the
>>> min/max properties could be done fairly quickly. But I'm not in a 
>>> hurry.
>>
>> X11 already supports that with the DRI3 extension. Also see VDPAU 
>> present and the Vulkan extension for reference application API 
>> implementations, so that stuff is already present.
>
> I suspect you mean the Present extension, specifically 
> PresentOptionUST. There is no working implementation of this yet (the 
> xserver tree has never had any code which would even advertise 
> PresentCapabilityUST, let alone do anything with a timestamp passed in 
> by the client).

Good point, what I wanted to say is that this is already specified in 
those extensions.

What we are missing is somehow wiring it all together and see how it works.

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