* [PATCH v3 0/3] Experimental freesync video mode optimization
@ 2021-01-04 21:07 Aurabindo Pillai
2021-01-04 21:07 ` [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode Aurabindo Pillai
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Aurabindo Pillai @ 2021-01-04 21:07 UTC (permalink / raw)
To: amd-gfx, dri-devel, nicholas.kazlauskas
Cc: stylon.wang, thong.thai, shashank.sharma, aurabindo.pillai,
wayne.lin, alexander.deucher
Changes in V4
=============
1) Add module parameter for freesync video mode
* Change module parameter name to freesync_video
2) Add freesync video modes based on preferred modes:
* Cosmetic fixes
* Added comments about all modes being added by the driver.
3) Skip modeset for front porch change
* Added more conditions for checking freesync video mode
Changes in V3
=============
1) Add freesync video modes based on preferred modes:
* Cache base freesync video mode during the first iteration to avoid
iterating over modelist again later.
* Add mode for 60 fps videos
2) Skip modeset for front porch change
* Fixes for bug exposed by caching of modes.
Changes in V2
=============
1) Add freesync video modes based on preferred modes:
* Remove check for connector type before adding freesync compatible
modes as VRR support is being checked, and there is no reason to block
freesync video support on eDP.
* use drm_mode_equal() instead of creating same functionality.
* Additional null pointer deference check
* Removed unnecessary variables.
* Cosmetic fixes.
2) Skip modeset for front porch change
* Remove _FSV string being appended to freesync video modes so as to not
define new policies or break existing application that might use the
mode name to figure out mode resolution.
* Remove unnecessary variables
* Cosmetic fixes.
--
This patchset enables freesync video mode usecase where the userspace
can request a freesync compatible video mode such that switching to this
mode does not trigger blanking.
This feature is guarded by a module parameter which is disabled by
default. Enabling this paramters adds additional modes to the driver
modelist, and also enables the optimization to skip modeset when using
one of these modes.
--
Aurabindo Pillai (3):
drm/amd/display: Add module parameter for freesync video mode
drm/amd/display: Add freesync video modes based on preferred modes
drm/amd/display: Skip modeset for front porch change
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 +
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 389 ++++++++++++++++--
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 +
4 files changed, 373 insertions(+), 32 deletions(-)
--
2.30.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2021-01-04 21:07 [PATCH v3 0/3] Experimental freesync video mode optimization Aurabindo Pillai
@ 2021-01-04 21:07 ` Aurabindo Pillai
2021-01-14 9:14 ` Pekka Paalanen
2021-01-04 21:07 ` [PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes Aurabindo Pillai
2021-01-04 21:08 ` [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai
2 siblings, 1 reply; 15+ messages in thread
From: Aurabindo Pillai @ 2021-01-04 21:07 UTC (permalink / raw)
To: amd-gfx, dri-devel, nicholas.kazlauskas
Cc: stylon.wang, thong.thai, shashank.sharma, aurabindo.pillai,
wayne.lin, alexander.deucher
[Why&How]
Adds a module parameter to enable experimental freesync video mode modeset
optimization. Enabling this mode allows the driver to skip a full modeset when
freesync compatible modes are requested by the userspace. This paramters also
adds some standard modes based on the connected monitor's VRR range.
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig at amd.com>
Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 100a431f0792..12b13a90eddf 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 b48d7a3c2a11..2badbc8b2294 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(
+ freesync_video,
+ "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");
+module_param_named(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.30.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes
2021-01-04 21:07 [PATCH v3 0/3] Experimental freesync video mode optimization Aurabindo Pillai
2021-01-04 21:07 ` [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode Aurabindo Pillai
@ 2021-01-04 21:07 ` Aurabindo Pillai
2021-01-04 21:08 ` [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai
2 siblings, 0 replies; 15+ messages in thread
From: Aurabindo Pillai @ 2021-01-04 21:07 UTC (permalink / raw)
To: amd-gfx, dri-devel, nicholas.kazlauskas
Cc: stylon.wang, thong.thai, shashank.sharma, aurabindo.pillai,
wayne.lin, alexander.deucher, Christian König
[Why&How]
If experimental freesync video mode module parameter is enabled, add
few extra display modes into the driver's mode list corresponding to common
video frame rates. When userspace sets these modes, no modeset will be
performed (if current mode was one of freesync modes or the base freesync mode
based off which timings have been generated for the rest of the freesync modes)
since these modes only differ from the base mode with front porch timing.
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 170 ++++++++++++++++++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +
2 files changed, 172 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 245bd1284e5f..aaef2fb528fd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5174,6 +5174,59 @@ static void dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
set_master_stream(context->streams, context->stream_count);
}
+static struct drm_display_mode *
+get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
+ bool use_probed_modes)
+{
+ struct drm_display_mode *m, *m_pref = NULL;
+ u16 current_refresh, highest_refresh;
+ struct list_head *list_head = use_probed_modes ?
+ &aconnector->base.probed_modes :
+ &aconnector->base.modes;
+
+ if (aconnector->freesync_vid_base.clock != 0)
+ return &aconnector->freesync_vid_base;
+
+ /* Find the preferred mode */
+ list_for_each_entry (m, list_head, head) {
+ if (m->type & DRM_MODE_TYPE_PREFERRED) {
+ m_pref = m;
+ break;
+ }
+ }
+
+ if (!m_pref) {
+ /* Probably an EDID with no preferred mode. Fallback to first entry */
+ m_pref = list_first_entry_or_null(
+ &aconnector->base.modes, struct drm_display_mode, head);
+ if (!m_pref) {
+ DRM_DEBUG_DRIVER("No preferred mode found in EDID\n");
+ return NULL;
+ }
+ }
+
+ highest_refresh = drm_mode_vrefresh(m_pref);
+
+ /*
+ * Find the mode with highest refresh rate with same resolution.
+ * For some monitors, preferred mode is not the mode with highest
+ * supported refresh rate.
+ */
+ list_for_each_entry (m, list_head, head) {
+ current_refresh = drm_mode_vrefresh(m);
+
+ if (m->hdisplay == m_pref->hdisplay &&
+ m->vdisplay == m_pref->vdisplay &&
+ highest_refresh < current_refresh) {
+ highest_refresh = current_refresh;
+ m_pref = m;
+ }
+ }
+
+ aconnector->freesync_vid_base = *m_pref;
+ return m_pref;
+}
+
static struct dc_stream_state *
create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
const struct drm_display_mode *drm_mode,
@@ -6999,6 +7052,122 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
}
}
+static bool is_duplicate_mode(struct amdgpu_dm_connector *aconnector,
+ struct drm_display_mode *mode)
+{
+ struct drm_display_mode *m;
+
+ list_for_each_entry (m, &aconnector->base.probed_modes, head) {
+ if (drm_mode_equal(m, mode))
+ return true;
+ }
+
+ return false;
+}
+
+static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
+ struct detailed_data_monitor_range *range)
+{
+ const struct drm_display_mode *m;
+ struct drm_display_mode *new_mode;
+ uint i;
+ uint32_t new_modes_count = 0;
+
+ /* 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
+ * 60 - Commonly used
+ * 48,72,96 - Multiples of 24
+ */
+ const uint32_t common_rates[] = { 23976, 24000, 25000, 29970, 30000,
+ 48000, 50000, 60000, 72000, 96000 };
+
+ /*
+ * Find mode with highest refresh rate with the same resolution
+ * as the preferred mode. Some monitors report a preferred mode
+ * with lower resolution than the highest refresh rate supported.
+ */
+
+ m = get_highest_refresh_rate_mode(aconnector, true);
+ if (!m)
+ return 0;
+
+ for (i = 0; i < ARRAY_SIZE(common_rates); i++) {
+ uint64_t target_vtotal, target_vtotal_diff;
+ uint64_t num, den;
+
+ if (drm_mode_vrefresh(m) * 1000 < common_rates[i])
+ continue;
+
+ if (common_rates[i] < range->min_vfreq * 1000)
+ continue;
+
+ num = (unsigned long long)m->clock * 1000 * 1000;
+ den = common_rates[i] * (unsigned long long)m->htotal;
+ target_vtotal = div_u64(num, den);
+ target_vtotal_diff = target_vtotal - m->vtotal;
+
+ /* Check for illegal modes */
+ if (m->vsync_start + target_vtotal_diff < m->vdisplay ||
+ m->vsync_end + target_vtotal_diff < m->vsync_start ||
+ m->vtotal + target_vtotal_diff < m->vsync_end)
+ continue;
+
+ new_mode = drm_mode_duplicate(aconnector->base.dev, m);
+ if (!new_mode)
+ goto out;
+
+ new_mode->vtotal += (u16)target_vtotal_diff;
+ new_mode->vsync_start += (u16)target_vtotal_diff;
+ new_mode->vsync_end += (u16)target_vtotal_diff;
+ new_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
+ new_mode->type |= DRM_MODE_TYPE_DRIVER;
+
+ if (!is_duplicate_mode(aconnector, new_mode)) {
+ drm_mode_probed_add(&aconnector->base, new_mode);
+ new_modes_count += 1;
+ } else
+ drm_mode_destroy(aconnector->base.dev, new_mode);
+ }
+ out:
+ return new_modes_count;
+}
+
+static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
+ struct edid *edid)
+{
+ uint8_t i;
+ struct detailed_timing *timing;
+ struct detailed_non_pixel *data;
+ struct detailed_data_monitor_range *range;
+ struct amdgpu_dm_connector *amdgpu_dm_connector =
+ to_amdgpu_dm_connector(connector);
+
+ if (!(amdgpu_exp_freesync_vid_mode && edid))
+ return;
+
+ if (edid->version == 1 && edid->revision > 1) {
+ for (i = 0; i < 4; i++) {
+ timing = &edid->detailed_timings[i];
+ data = &timing->data.other_data;
+ range = &data->data.range;
+
+ /* Check if monitor has continuous frequency mode */
+ if (data->type == EDID_DETAIL_MONITOR_RANGE &&
+ range->max_vfreq - range->min_vfreq > 10) {
+ amdgpu_dm_connector->num_modes += add_fs_modes(amdgpu_dm_connector, range);
+ break;
+ }
+ }
+ }
+}
+
static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
{
struct amdgpu_dm_connector *amdgpu_dm_connector =
@@ -7014,6 +7183,7 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
} else {
amdgpu_dm_connector_ddc_get_modes(connector, edid);
amdgpu_dm_connector_add_common_modes(encoder, connector);
+ amdgpu_dm_connector_add_freesync_modes(connector, edid);
}
amdgpu_dm_fbc_init(connector);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index c9d82b9e4d7e..3ea85be9c546 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -432,6 +432,8 @@ struct amdgpu_dm_connector {
#endif
bool force_yuv420_output;
struct dsc_preferred_settings dsc_settings;
+ /* Cached display modes */
+ struct drm_display_mode freesync_vid_base;
};
#define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
--
2.30.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change
2021-01-04 21:07 [PATCH v3 0/3] Experimental freesync video mode optimization Aurabindo Pillai
2021-01-04 21:07 ` [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode Aurabindo Pillai
2021-01-04 21:07 ` [PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes Aurabindo Pillai
@ 2021-01-04 21:08 ` Aurabindo Pillai
2021-01-06 20:02 ` Kazlauskas, Nicholas
2 siblings, 1 reply; 15+ messages in thread
From: Aurabindo Pillai @ 2021-01-04 21:08 UTC (permalink / raw)
To: amd-gfx, dri-devel, nicholas.kazlauskas
Cc: stylon.wang, thong.thai, shashank.sharma, aurabindo.pillai,
wayne.lin, alexander.deucher, Christian König
[Why&How]
Inorder to enable freesync video mode, driver adds extra
modes based on preferred modes for common freesync frame rates.
When commiting these mode changes, a full modeset is not needed.
If the change in only in the front porch timing value, skip full
modeset and continue using the same stream.
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig@amd.com>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 219 +++++++++++++++---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
2 files changed, 188 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index aaef2fb528fd..315756207f0f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
static const struct drm_format_info *
amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
+static bool
+is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
+ struct drm_crtc_state *new_crtc_state);
/*
* dm_vblank_get_counter
*
@@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode(
const struct drm_connector *connector,
const struct drm_connector_state *connector_state,
const struct dc_stream_state *old_stream,
- int requested_bpc)
+ int requested_bpc,
+ bool is_in_modeset)
{
struct dc_crtc_timing *timing_out = &stream->timing;
const struct drm_display_info *info = &connector->display_info;
@@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode(
timing_out->hdmi_vic = hv_frame.vic;
}
- timing_out->h_addressable = mode_in->crtc_hdisplay;
- timing_out->h_total = mode_in->crtc_htotal;
- timing_out->h_sync_width =
- mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
- timing_out->h_front_porch =
- mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
- timing_out->v_total = mode_in->crtc_vtotal;
- timing_out->v_addressable = mode_in->crtc_vdisplay;
- timing_out->v_front_porch =
- mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
- timing_out->v_sync_width =
- mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
- timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
+ if (is_in_modeset) {
+ timing_out->h_addressable = mode_in->hdisplay;
+ timing_out->h_total = mode_in->htotal;
+ timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start;
+ timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay;
+ timing_out->v_total = mode_in->vtotal;
+ timing_out->v_addressable = mode_in->vdisplay;
+ timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay;
+ timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start;
+ timing_out->pix_clk_100hz = mode_in->clock * 10;
+ } else {
+ timing_out->h_addressable = mode_in->crtc_hdisplay;
+ timing_out->h_total = mode_in->crtc_htotal;
+ timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
+ timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
+ timing_out->v_total = mode_in->crtc_vtotal;
+ timing_out->v_addressable = mode_in->crtc_vdisplay;
+ timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
+ timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
+ timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
+ }
+
timing_out->aspect_ratio = get_aspect_ratio(mode_in);
stream->output_color_space = get_output_color_space(timing_out);
@@ -5227,6 +5240,33 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
return m_pref;
}
+static bool is_freesync_video_mode(struct drm_display_mode *mode,
+ struct amdgpu_dm_connector *aconnector)
+{
+ struct drm_display_mode *high_mode;
+ int timing_diff;
+
+ high_mode = get_highest_refresh_rate_mode(aconnector, false);
+ if (!high_mode || !mode)
+ return false;
+
+ timing_diff = high_mode->vtotal - mode->vtotal;
+
+ if (high_mode->clock == 0 || high_mode->clock != mode->clock ||
+ high_mode->hdisplay != mode->hdisplay ||
+ high_mode->vdisplay != mode->vdisplay ||
+ high_mode->hsync_start != mode->hsync_start ||
+ high_mode->hsync_end != mode->hsync_end ||
+ high_mode->htotal != mode->htotal ||
+ high_mode->hskew != mode->hskew ||
+ high_mode->vscan != mode->vscan ||
+ high_mode->vsync_start - mode->vsync_start != timing_diff ||
+ high_mode->vsync_end - mode->vsync_end != timing_diff)
+ return false;
+ else
+ return true;
+}
+
static struct dc_stream_state *
create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
const struct drm_display_mode *drm_mode,
@@ -5240,15 +5280,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
dm_state ? &dm_state->base : NULL;
struct dc_stream_state *stream = NULL;
struct drm_display_mode mode = *drm_mode;
+ struct drm_display_mode saved_mode;
+ struct drm_display_mode *freesync_mode = NULL;
bool native_mode_found = false;
bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
int mode_refresh;
int preferred_refresh = 0;
+ bool is_fs_vid_mode = 0;
#if defined(CONFIG_DRM_AMD_DC_DCN)
struct dsc_dec_dpcd_caps dsc_caps;
uint32_t link_bandwidth_kbps;
#endif
struct dc_sink *sink = NULL;
+
+ memset(&saved_mode, 0, sizeof(struct drm_display_mode));
+
if (aconnector == NULL) {
DRM_ERROR("aconnector is NULL!\n");
return stream;
@@ -5301,25 +5347,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
*/
DRM_DEBUG_DRIVER("No preferred mode found\n");
} else {
- decide_crtc_timing_for_drm_display_mode(
+ is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
+ if (is_fs_vid_mode) {
+ freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
+ saved_mode = mode;
+ mode = *freesync_mode;
+ }
+
+ if (!is_fs_vid_mode)
+ decide_crtc_timing_for_drm_display_mode(
&mode, preferred_mode,
dm_state ? (dm_state->scaling != RMX_OFF) : false);
+
preferred_refresh = drm_mode_vrefresh(preferred_mode);
}
if (!dm_state)
drm_mode_set_crtcinfo(&mode, 0);
- /*
+ if (dm_state && is_fs_vid_mode)
+ drm_mode_set_crtcinfo(&saved_mode, 0);
+
+ /*
* If scaling is enabled and refresh rate didn't change
* we copy the vic and polarities of the old timings
*/
- if (!scale || mode_refresh != preferred_refresh)
- fill_stream_properties_from_drm_display_mode(stream,
- &mode, &aconnector->base, con_state, NULL, requested_bpc);
+ if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh)
+ fill_stream_properties_from_drm_display_mode(
+ stream, &mode, &aconnector->base, con_state, NULL,
+ requested_bpc, dm_state ? 1 : 0);
else
- fill_stream_properties_from_drm_display_mode(stream,
- &mode, &aconnector->base, con_state, old_stream, requested_bpc);
+ fill_stream_properties_from_drm_display_mode(
+ stream, &mode, &aconnector->base, con_state, old_stream,
+ requested_bpc, dm_state ? 1 : 0);
stream->timing.flags.DSC = 0;
@@ -7847,13 +7907,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);
@@ -8171,7 +8247,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,
@@ -8867,6 +8945,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 &&
@@ -8874,17 +8953,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;
}
@@ -8897,6 +8985,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,
@@ -8987,6 +9120,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)) {
@@ -9018,6 +9156,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 3ea85be9c546..ff4675572125 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -474,6 +474,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.30.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change
2021-01-04 21:08 ` [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai
@ 2021-01-06 20:02 ` Kazlauskas, Nicholas
2021-01-17 19:52 ` Aurabindo Pillai
0 siblings, 1 reply; 15+ messages in thread
From: Kazlauskas, Nicholas @ 2021-01-06 20:02 UTC (permalink / raw)
To: Aurabindo Pillai, amd-gfx, dri-devel
Cc: stylon.wang, shashank.sharma, thong.thai, wayne.lin,
alexander.deucher, Christian König
On 2021-01-04 4:08 p.m., Aurabindo Pillai wrote:
> [Why&How]
> Inorder to enable freesync video mode, driver adds extra
> modes based on preferred modes for common freesync frame rates.
> When commiting these mode changes, a full modeset is not needed.
> If the change in only in the front porch timing value, skip full
> modeset and continue using the same stream.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Acked-by: Christian König <christian.koenig@amd.com>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 219 +++++++++++++++---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> 2 files changed, 188 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index aaef2fb528fd..315756207f0f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
> static const struct drm_format_info *
> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state);
> /*
> * dm_vblank_get_counter
> *
> @@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode(
> const struct drm_connector *connector,
> const struct drm_connector_state *connector_state,
> const struct dc_stream_state *old_stream,
> - int requested_bpc)
> + int requested_bpc,
> + bool is_in_modeset)
> {
> struct dc_crtc_timing *timing_out = &stream->timing;
> const struct drm_display_info *info = &connector->display_info;
> @@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode(
> timing_out->hdmi_vic = hv_frame.vic;
> }
>
> - timing_out->h_addressable = mode_in->crtc_hdisplay;
> - timing_out->h_total = mode_in->crtc_htotal;
> - timing_out->h_sync_width =
> - mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
> - timing_out->h_front_porch =
> - mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
> - timing_out->v_total = mode_in->crtc_vtotal;
> - timing_out->v_addressable = mode_in->crtc_vdisplay;
> - timing_out->v_front_porch =
> - mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
> - timing_out->v_sync_width =
> - mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
> - timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
> + if (is_in_modeset) {
> + timing_out->h_addressable = mode_in->hdisplay;
> + timing_out->h_total = mode_in->htotal;
> + timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start;
> + timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay;
> + timing_out->v_total = mode_in->vtotal;
> + timing_out->v_addressable = mode_in->vdisplay;
> + timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay;
> + timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start;
> + timing_out->pix_clk_100hz = mode_in->clock * 10;
> + } else {
> + timing_out->h_addressable = mode_in->crtc_hdisplay;
> + timing_out->h_total = mode_in->crtc_htotal;
> + timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
> + timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
> + timing_out->v_total = mode_in->crtc_vtotal;
> + timing_out->v_addressable = mode_in->crtc_vdisplay;
> + timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
> + timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
> + timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
> + }
> +
> timing_out->aspect_ratio = get_aspect_ratio(mode_in);
>
> stream->output_color_space = get_output_color_space(timing_out);
> @@ -5227,6 +5240,33 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
> return m_pref;
> }
>
> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> + struct amdgpu_dm_connector *aconnector)
> +{
> + struct drm_display_mode *high_mode;
> + int timing_diff;
> +
> + high_mode = get_highest_refresh_rate_mode(aconnector, false);
> + if (!high_mode || !mode)
> + return false;
> +
> + timing_diff = high_mode->vtotal - mode->vtotal;
> +
> + if (high_mode->clock == 0 || high_mode->clock != mode->clock ||
> + high_mode->hdisplay != mode->hdisplay ||
> + high_mode->vdisplay != mode->vdisplay ||
> + high_mode->hsync_start != mode->hsync_start ||
> + high_mode->hsync_end != mode->hsync_end ||
> + high_mode->htotal != mode->htotal ||
> + high_mode->hskew != mode->hskew ||
> + high_mode->vscan != mode->vscan ||
> + high_mode->vsync_start - mode->vsync_start != timing_diff ||
> + high_mode->vsync_end - mode->vsync_end != timing_diff)
> + return false;
> + else
> + return true;
> +}
> +
> static struct dc_stream_state *
> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> const struct drm_display_mode *drm_mode,
> @@ -5240,15 +5280,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;
nitpick: change 0 to false
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> struct dsc_dec_dpcd_caps dsc_caps;
> uint32_t link_bandwidth_kbps;
> #endif
> struct dc_sink *sink = NULL;
> +
> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
nitpick: sizeof(saved_mode)
> +
> if (aconnector == NULL) {
> DRM_ERROR("aconnector is NULL!\n");
> return stream;
> @@ -5301,25 +5347,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> */
> DRM_DEBUG_DRIVER("No preferred mode found\n");
> } else {
> - decide_crtc_timing_for_drm_display_mode(
> + is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
> + if (is_fs_vid_mode) {
> + freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
> + saved_mode = mode;
> + mode = *freesync_mode;
> + }
> +
> + if (!is_fs_vid_mode)
nitpick: don't need to check this twice, just use an } else {
> + decide_crtc_timing_for_drm_display_mode(
> &mode, preferred_mode,
> dm_state ? (dm_state->scaling != RMX_OFF) : false);
> +
> preferred_refresh = drm_mode_vrefresh(preferred_mode);
> }
>
> if (!dm_state)
> drm_mode_set_crtcinfo(&mode, 0);
>
> - /*
> + if (dm_state && is_fs_vid_mode)
> + drm_mode_set_crtcinfo(&saved_mode, 0);
> +
> + /*
> * If scaling is enabled and refresh rate didn't change
> * we copy the vic and polarities of the old timings
> */
> - if (!scale || mode_refresh != preferred_refresh)
> - fill_stream_properties_from_drm_display_mode(stream,
> - &mode, &aconnector->base, con_state, NULL, requested_bpc);
> + if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh)
> + fill_stream_properties_from_drm_display_mode(
> + stream, &mode, &aconnector->base, con_state, NULL,
> + requested_bpc, dm_state ? 1 : 0);
Some notes on this dm_state 1 : 0 stuff below...
> else
> - fill_stream_properties_from_drm_display_mode(stream,
> - &mode, &aconnector->base, con_state, old_stream, requested_bpc);
> + fill_stream_properties_from_drm_display_mode(
> + stream, &mode, &aconnector->base, con_state, old_stream,
> + requested_bpc, dm_state ? 1 : 0);
>
> stream->timing.flags.DSC = 0;
>
> @@ -7847,13 +7907,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;
style nitpick: can just drop this goto if you do an } else { here.
> + }
> +
> config.state = new_crtc_state->base.vrr_enabled ?
> VRR_STATE_ACTIVE_VARIABLE :
> VRR_STATE_INACTIVE;
> - } else {
> + } else
style nitpick: prefer braces on all conditions if they're on any, ie.
use } else {
> config.state = VRR_STATE_UNSUPPORTED;
> - }
>
> +out:
> mod_freesync_build_vrr_params(dm->freesync_module,
> new_stream,
> &config, &vrr_params);
> @@ -8171,7 +8247,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 ||
You should be modifying the amdgpu_dm_vrr_active check instead. It's
important that we're sending back the correct vblank events to userspace
as long as we're in DRR.
The logic that does this is in the IRQ handlers, but you should only
need to modify this check function.
> + acrtc_state->freesync_video_mode) {
> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> dc_stream_adjust_vmin_vmax(
> dm->dc, acrtc_state->stream,
> @@ -8867,6 +8945,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 &&
> @@ -8874,17 +8953,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;
style nitpick: braces on all if conditions
You also don't need to check !fs_vid_mode, it's always false in this branch.
> +
> + }
> +out:
> new_crtc_state->freesync_config = config;
> }
>
> @@ -8897,6 +8985,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;
The 'freesync_video_mode' doesn't automatically carry over when CRTC
state is duplicated. You'll need to add that to dm_crtc_duplicate_state
as well, but keep in mind that I don't see you resetting this to false
anywhere.
You might want to add that in on removing the stream, but I haven't
looked too deep into this.
> +}
> +
> static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> struct drm_atomic_state *state,
> struct drm_crtc *crtc,
> @@ -8987,6 +9120,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)
Reorder this so that we have amdgpu_exp_freesync_vid_mode as the first
check. Small optimization.
> + 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)) {
> @@ -9018,6 +9156,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) {
Reorder this so that we have amdgpu_exp_freesync_vid_mode as the first
check. Small optimization.
Regards,
Nicholas Kazlauskas
> + 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 3ea85be9c546..ff4675572125 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -474,6 +474,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;
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2021-01-04 21:07 ` [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode Aurabindo Pillai
@ 2021-01-14 9:14 ` Pekka Paalanen
2021-01-18 14:36 ` Aurabindo Pillai
0 siblings, 1 reply; 15+ messages in thread
From: Pekka Paalanen @ 2021-01-14 9:14 UTC (permalink / raw)
To: Aurabindo Pillai
Cc: stylon.wang, shashank.sharma, thong.thai, dri-devel,
Christian König, amd-gfx, wayne.lin, alexander.deucher,
nicholas.kazlauskas
[-- Attachment #1.1: Type: text/plain, Size: 3482 bytes --]
On Mon, 4 Jan 2021 16:07:58 -0500
Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> [Why&How]
> Adds a module parameter to enable experimental freesync video mode modeset
> optimization. Enabling this mode allows the driver to skip a full modeset when
> freesync compatible modes are requested by the userspace. This paramters also
> adds some standard modes based on the connected monitor's VRR range.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Acked-by: Christian König <christian.koenig at amd.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 100a431f0792..12b13a90eddf 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 b48d7a3c2a11..2badbc8b2294 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(
> + freesync_video,
> + "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");
> +module_param_named(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)
Hi,
please document somewhere that ends up in git history (commit message,
code comments, description of the parameter would be the best but maybe
there isn't enough space?) what Christian König explained in
https://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html
that this is a stop-gap feature intended to be removed as soon as
possible (when a better solution comes up, which could be years).
So far I have not seen a single mention of this intention in your patch
submissions, and I think it is very important to make known.
I also did not see an explanation of why this instead of manufacturing
these video modes in userspace (an idea mentioned by Christian in the
referenced email). I think that too should be part of a commit message.
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change
2021-01-06 20:02 ` Kazlauskas, Nicholas
@ 2021-01-17 19:52 ` Aurabindo Pillai
0 siblings, 0 replies; 15+ messages in thread
From: Aurabindo Pillai @ 2021-01-17 19:52 UTC (permalink / raw)
To: Kazlauskas, Nicholas
Cc: stylon.wang, shashank.sharma, thong.thai, dri-devel, amd-gfx,
wayne.lin, alexander.deucher, Christian König
--
Thanks & Regards,
Aurabindo Pillai
On Wed, Jan 6, 2021 at 15:02, "Kazlauskas, Nicholas"
<nicholas.kazlauskas@amd.com> wrote:
> On 2021-01-04 4:08 p.m., Aurabindo Pillai wrote:
>> [Why&How]
>> Inorder to enable freesync video mode, driver adds extra
>> modes based on preferred modes for common freesync frame rates.
>> When commiting these mode changes, a full modeset is not needed.
>> If the change in only in the front porch timing value, skip full
>> modeset and continue using the same stream.
>>
>> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>> Acked-by: Christian König <christian.koenig@amd.com>
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 219
>> +++++++++++++++---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
>> 2 files changed, 188 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index aaef2fb528fd..315756207f0f 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct
>> amdgpu_display_manager *dm);
>> static const struct drm_format_info *
>> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>> \x7f+static bool
>> +is_timing_unchanged_for_freesync(struct drm_crtc_state
>> *old_crtc_state,
>> + struct drm_crtc_state *new_crtc_state);
>> /*
>> * dm_vblank_get_counter
>> *
>> @@ -4940,7 +4943,8 @@ static void
>> fill_stream_properties_from_drm_display_mode(
>> const struct drm_connector *connector,
>> const struct drm_connector_state *connector_state,
>> const struct dc_stream_state *old_stream,
>> - int requested_bpc)
>> + int requested_bpc,
>> + bool is_in_modeset)
>> {
>> struct dc_crtc_timing *timing_out = &stream->timing;
>> const struct drm_display_info *info = &connector->display_info;
>> @@ -4995,19 +4999,28 @@ static void
>> fill_stream_properties_from_drm_display_mode(
>> timing_out->hdmi_vic = hv_frame.vic;
>> }
>> \x7f- timing_out->h_addressable = mode_in->crtc_hdisplay;
>> - timing_out->h_total = mode_in->crtc_htotal;
>> - timing_out->h_sync_width =
>> - mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
>> - timing_out->h_front_porch =
>> - mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
>> - timing_out->v_total = mode_in->crtc_vtotal;
>> - timing_out->v_addressable = mode_in->crtc_vdisplay;
>> - timing_out->v_front_porch =
>> - mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
>> - timing_out->v_sync_width =
>> - mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
>> - timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
>> + if (is_in_modeset) {
>> + timing_out->h_addressable = mode_in->hdisplay;
>> + timing_out->h_total = mode_in->htotal;
>> + timing_out->h_sync_width = mode_in->hsync_end -
>> mode_in->hsync_start;
>> + timing_out->h_front_porch = mode_in->hsync_start -
>> mode_in->hdisplay;
>> + timing_out->v_total = mode_in->vtotal;
>> + timing_out->v_addressable = mode_in->vdisplay;
>> + timing_out->v_front_porch = mode_in->vsync_start -
>> mode_in->vdisplay;
>> + timing_out->v_sync_width = mode_in->vsync_end -
>> mode_in->vsync_start;
>> + timing_out->pix_clk_100hz = mode_in->clock * 10;
>> + } else {
>> + timing_out->h_addressable = mode_in->crtc_hdisplay;
>> + timing_out->h_total = mode_in->crtc_htotal;
>> + timing_out->h_sync_width = mode_in->crtc_hsync_end -
>> mode_in->crtc_hsync_start;
>> + timing_out->h_front_porch = mode_in->crtc_hsync_start -
>> mode_in->crtc_hdisplay;
>> + timing_out->v_total = mode_in->crtc_vtotal;
>> + timing_out->v_addressable = mode_in->crtc_vdisplay;
>> + timing_out->v_front_porch = mode_in->crtc_vsync_start -
>> mode_in->crtc_vdisplay;
>> + timing_out->v_sync_width = mode_in->crtc_vsync_end -
>> mode_in->crtc_vsync_start;
>> + timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
>> + }
>> +
>> timing_out->aspect_ratio = get_aspect_ratio(mode_in);
>> \x7f stream->output_color_space =
>> get_output_color_space(timing_out);
>> @@ -5227,6 +5240,33 @@ get_highest_refresh_rate_mode(struct
>> amdgpu_dm_connector *aconnector,
>> return m_pref;
>> }
>> \x7f+static bool is_freesync_video_mode(struct drm_display_mode *mode,
>> + struct amdgpu_dm_connector *aconnector)
>> +{
>> + struct drm_display_mode *high_mode;
>> + int timing_diff;
>> +
>> + high_mode = get_highest_refresh_rate_mode(aconnector, false);
>> + if (!high_mode || !mode)
>> + return false;
>> +
>> + timing_diff = high_mode->vtotal - mode->vtotal;
>> +
>> + if (high_mode->clock == 0 || high_mode->clock != mode->clock ||
>> + high_mode->hdisplay != mode->hdisplay ||
>> + high_mode->vdisplay != mode->vdisplay ||
>> + high_mode->hsync_start != mode->hsync_start ||
>> + high_mode->hsync_end != mode->hsync_end ||
>> + high_mode->htotal != mode->htotal ||
>> + high_mode->hskew != mode->hskew ||
>> + high_mode->vscan != mode->vscan ||
>> + high_mode->vsync_start - mode->vsync_start != timing_diff ||
>> + high_mode->vsync_end - mode->vsync_end != timing_diff)
>> + return false;
>> + else
>> + return true;
>> +}
>> +
>> static struct dc_stream_state *
>> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>> const struct drm_display_mode *drm_mode,
>> @@ -5240,15 +5280,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;
>
> nitpick: change 0 to false
>
>> #if defined(CONFIG_DRM_AMD_DC_DCN)
>> struct dsc_dec_dpcd_caps dsc_caps;
>> uint32_t link_bandwidth_kbps;
>> #endif
>> struct dc_sink *sink = NULL;
>> +
>> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
>
> nitpick: sizeof(saved_mode)
>
>> +
>> if (aconnector == NULL) {
>> DRM_ERROR("aconnector is NULL!\n");
>> return stream;
>> @@ -5301,25 +5347,39 @@ create_stream_for_sink(struct
>> amdgpu_dm_connector *aconnector,
>> */
>> DRM_DEBUG_DRIVER("No preferred mode found\n");
>> } else {
>> - decide_crtc_timing_for_drm_display_mode(
>> + is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
>> + if (is_fs_vid_mode) {
>> + freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
>> + saved_mode = mode;
>> + mode = *freesync_mode;
>> + }
>> +
>> + if (!is_fs_vid_mode)
>
> nitpick: don't need to check this twice, just use an } else {
>
>> + 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);
>> }
>> \x7f if (!dm_state)
>> drm_mode_set_crtcinfo(&mode, 0);
>> \x7f- /*
>> + if (dm_state && is_fs_vid_mode)
>> + drm_mode_set_crtcinfo(&saved_mode, 0);
>> +
>> + /*
>> * If scaling is enabled and refresh rate didn't change
>> * we copy the vic and polarities of the old timings
>> */
>> - if (!scale || mode_refresh != preferred_refresh)
>> - fill_stream_properties_from_drm_display_mode(stream,
>> - &mode, &aconnector->base, con_state, NULL, requested_bpc);
>> + if (!(scale && is_fs_vid_mode) || mode_refresh !=
>> preferred_refresh)
>> + fill_stream_properties_from_drm_display_mode(
>> + stream, &mode, &aconnector->base, con_state, NULL,
>> + requested_bpc, dm_state ? 1 : 0);
>
> Some notes on this dm_state 1 : 0 stuff below...
>
>> else
>> - fill_stream_properties_from_drm_display_mode(stream,
>> - &mode, &aconnector->base, con_state, old_stream, requested_bpc);
>> + fill_stream_properties_from_drm_display_mode(
>> + stream, &mode, &aconnector->base, con_state, old_stream,
>> + requested_bpc, dm_state ? 1 : 0);
>> \x7f stream->timing.flags.DSC = 0;
>> \x7f@@ -7847,13 +7907,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;
>
> style nitpick: can just drop this goto if you do an } else { here.
>
>> + }
>> +
>> config.state = new_crtc_state->base.vrr_enabled ?
>> VRR_STATE_ACTIVE_VARIABLE :
>> VRR_STATE_INACTIVE;
>> - } else {
>> + } else
>
> style nitpick: prefer braces on all conditions if they're on any, ie.
> use } else {
>
>> config.state = VRR_STATE_UNSUPPORTED;
>> - }
>> \x7f+out:
>> mod_freesync_build_vrr_params(dm->freesync_module,
>> new_stream,
>> &config, &vrr_params);
>> @@ -8171,7 +8247,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
>> ||
>
> You should be modifying the amdgpu_dm_vrr_active check instead. It's
> important that we're sending back the correct vblank events to
> userspace as long as we're in DRR.
>
> The logic that does this is in the IRQ handlers, but you should only
> need to modify this check function.
This is a duplicate check indeed. Will have it removed. However the
check for freesync_video_mode is still needed. Will send a v5 along
with other changes suggested above.
>
>
>> + acrtc_state->freesync_video_mode) {
>> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
>> dc_stream_adjust_vmin_vmax(
>> dm->dc, acrtc_state->stream,
>> @@ -8867,6 +8945,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;
>> \x7f new_crtc_state->vrr_supported =
>> new_con_state->freesync_capable &&
>> vrefresh >= aconnector->min_vfreq &&
>> @@ -8874,17 +8953,26 @@ static void get_freesync_config_for_crtc(
>> \x7f 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;
>> - }
>> \x7f+ 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;
>
> style nitpick: braces on all if conditions
>
> You also don't need to check !fs_vid_mode, it's always false in this
> branch.
Will fix.
>
>> +
>> + }
>> +out:
>> new_crtc_state->freesync_config = config;
>> }
>> \x7f@@ -8897,6 +8985,51 @@ static void reset_freesync_config_for_crtc(
>> sizeof(new_crtc_state->vrr_infopacket));
>> }
>> \x7f+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;
>
> The 'freesync_video_mode' doesn't automatically carry over when CRTC
> state is duplicated. You'll need to add that to
> dm_crtc_duplicate_state as well, but keep in mind that I don't see
> you resetting this to false anywhere.
>
> You might want to add that in on removing the stream, but I haven't
> looked too deep into this.
Freesync config is already copied in dm_crtc_duplicate_state, so we
only need to add freesync_video_mode duplication there. Will add
resetting freesync_video_mode as well. I'm not sure if setting this to
false during stream removal is necessary though, since other freesync
parameters appear to be untouched in dm's commit_tail. What I'm
thinking is like:
---
static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
[snip]
continue;
}
- if (dm_old_crtc_state->stream)
+ if (dm_old_crtc_state->stream) {
remove_stream(adev, acrtc,
dm_old_crtc_state->stream);
+ dm_old_crtc_state->freesync_video_mode
= 0;
+ }
pm_runtime_get_noresume(dev->dev);
---
>
>> +}
>> +
>> static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>> struct drm_atomic_state *state,
>> struct drm_crtc *crtc,
>> @@ -8987,6 +9120,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)
>
> Reorder this so that we have amdgpu_exp_freesync_vid_mode as the
> first check. Small optimization.
>
>> + 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)) {
>> @@ -9018,6 +9156,23 @@ static int dm_update_crtc_state(struct
>> amdgpu_display_manager *dm,
>> if (!dm_old_crtc_state->stream)
>> goto skip_modeset;
>> \x7f+ if (dm_new_crtc_state->stream &&
>> + is_timing_unchanged_for_freesync(new_crtc_state,
>> old_crtc_state) &&
>> + amdgpu_exp_freesync_vid_mode) {
>
> Reorder this so that we have amdgpu_exp_freesync_vid_mode as the
> first check. Small optimization.
>
Will do these two reorders.
> Regards,
> Nicholas Kazlauskas
>
>> + 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 3ea85be9c546..ff4675572125 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -474,6 +474,7 @@ struct dm_crtc_state {
>> \x7f bool freesync_timing_changed;
>> bool freesync_vrr_info_changed;
>> + bool freesync_video_mode;
>> \x7f bool dsc_force_changed;
>> bool vrr_supported;
>>
>
--
Thanks & Regards,
Aurabindo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2021-01-14 9:14 ` Pekka Paalanen
@ 2021-01-18 14:36 ` Aurabindo Pillai
2021-01-19 8:35 ` Pekka Paalanen
0 siblings, 1 reply; 15+ messages in thread
From: Aurabindo Pillai @ 2021-01-18 14:36 UTC (permalink / raw)
To: Pekka Paalanen
Cc: stylon.wang, shashank.sharma, thong.thai, dri-devel,
Christian König, amd-gfx, wayne.lin, alexander.deucher,
nicholas.kazlauskas
On Thu, 2021-01-14 at 11:14 +0200, Pekka Paalanen wrote:
>
> Hi,
>
> please document somewhere that ends up in git history (commit
> message,
> code comments, description of the parameter would be the best but
> maybe
> there isn't enough space?) what Christian König explained in
>
>
> https://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html
>
> that this is a stop-gap feature intended to be removed as soon as
> possible (when a better solution comes up, which could be years).
>
> So far I have not seen a single mention of this intention in your
> patch
> submissions, and I think it is very important to make known.
Hi,
Thanks for the headsup, I shall add the relevant info in the next
verison.
>
> I also did not see an explanation of why this instead of
> manufacturing
> these video modes in userspace (an idea mentioned by Christian in the
> referenced email). I think that too should be part of a commit
> message.
This is an opt-in feature, which shall be superseded by a better
solution. We also add a set of common modes for scaling similarly.
Userspace can still add whatever mode they want. So I dont see a reason
why this cant be in the kernel.
--
Regards,
Aurabindo Pillai
>
>
> Thanks,
> pq
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2021-01-18 14:36 ` Aurabindo Pillai
@ 2021-01-19 8:35 ` Pekka Paalanen
2021-01-19 13:11 ` Daniel Vetter
0 siblings, 1 reply; 15+ messages in thread
From: Pekka Paalanen @ 2021-01-19 8:35 UTC (permalink / raw)
To: Aurabindo Pillai
Cc: stylon.wang, shashank.sharma, thong.thai, dri-devel,
Christian König, amd-gfx, wayne.lin, alexander.deucher,
nicholas.kazlauskas
[-- Attachment #1.1: Type: text/plain, Size: 2745 bytes --]
On Mon, 18 Jan 2021 09:36:47 -0500
Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> On Thu, 2021-01-14 at 11:14 +0200, Pekka Paalanen wrote:
> >
> > Hi,
> >
> > please document somewhere that ends up in git history (commit
> > message,
> > code comments, description of the parameter would be the best but
> > maybe
> > there isn't enough space?) what Christian König explained in
> >
> >
> > https://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html
> >
> > that this is a stop-gap feature intended to be removed as soon as
> > possible (when a better solution comes up, which could be years).
> >
> > So far I have not seen a single mention of this intention in your
> > patch
> > submissions, and I think it is very important to make known.
>
> Hi,
>
> Thanks for the headsup, I shall add the relevant info in the next
> verison.
>
> >
> > I also did not see an explanation of why this instead of
> > manufacturing
> > these video modes in userspace (an idea mentioned by Christian in the
> > referenced email). I think that too should be part of a commit
> > message.
>
> This is an opt-in feature, which shall be superseded by a better
> solution. We also add a set of common modes for scaling similarly.
> Userspace can still add whatever mode they want. So I dont see a reason
> why this cant be in the kernel.
Hi,
sorry, I think that kind of thinking is backwards. There needs to be a
reason to put something in the kernel, and if there is no reason, then
it remains in userspace. So what's the reason to put this in the kernel?
One example reason why this should not be in the kernel is that the set
of video modes to manufacture is a kind of policy, which modes to add
and which not. Userspace knows what modes it needs, and establishing
the modes in the kernel instead is second-guessing what the userspace
would want. So if userspace needs to manufacture modes in userspace
anyway as some modes might be missed by the kernel, then why bother in
the kernel to begin with? Why should the kernel play catch-up with what
modes userspace wants when we already have everything userspace needs
to make its own modes, even to add them to the kernel mode list?
Does manufacturing these extra video modes to achieve fast timing
changes require AMD hardware-specific knowledge, as opposed to the
general VRR approach of simply adjusting the front porch?
Something like this should also be documented in a commit message. Or
if you insist that "no reason to not put this in the kernel" is reason
enough, then write that down, because it does not seem obvious to me or
others that this feature needs to be in the kernel.
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2021-01-19 8:35 ` Pekka Paalanen
@ 2021-01-19 13:11 ` Daniel Vetter
2021-01-19 16:08 ` Pillai, Aurabindo
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2021-01-19 13:11 UTC (permalink / raw)
To: Pekka Paalanen
Cc: Stylon Wang, thong.thai, shashank.sharma, dri-devel,
Christian König, Aurabindo Pillai, amd-gfx list, Wayne Lin,
Alex Deucher, Kazlauskas, Nicholas
On Tue, Jan 19, 2021 at 9:35 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Mon, 18 Jan 2021 09:36:47 -0500
> Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>
> > On Thu, 2021-01-14 at 11:14 +0200, Pekka Paalanen wrote:
> > >
> > > Hi,
> > >
> > > please document somewhere that ends up in git history (commit
> > > message,
> > > code comments, description of the parameter would be the best but
> > > maybe
> > > there isn't enough space?) what Christian König explained in
> > >
> > >
> > > https://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html
> > >
> > > that this is a stop-gap feature intended to be removed as soon as
> > > possible (when a better solution comes up, which could be years).
> > >
> > > So far I have not seen a single mention of this intention in your
> > > patch
> > > submissions, and I think it is very important to make known.
> >
> > Hi,
> >
> > Thanks for the headsup, I shall add the relevant info in the next
> > verison.
> >
> > >
> > > I also did not see an explanation of why this instead of
> > > manufacturing
> > > these video modes in userspace (an idea mentioned by Christian in the
> > > referenced email). I think that too should be part of a commit
> > > message.
> >
> > This is an opt-in feature, which shall be superseded by a better
> > solution. We also add a set of common modes for scaling similarly.
> > Userspace can still add whatever mode they want. So I dont see a reason
> > why this cant be in the kernel.
>
> Hi,
>
> sorry, I think that kind of thinking is backwards. There needs to be a
> reason to put something in the kernel, and if there is no reason, then
> it remains in userspace. So what's the reason to put this in the kernel?
>
> One example reason why this should not be in the kernel is that the set
> of video modes to manufacture is a kind of policy, which modes to add
> and which not. Userspace knows what modes it needs, and establishing
> the modes in the kernel instead is second-guessing what the userspace
> would want. So if userspace needs to manufacture modes in userspace
> anyway as some modes might be missed by the kernel, then why bother in
> the kernel to begin with? Why should the kernel play catch-up with what
> modes userspace wants when we already have everything userspace needs
> to make its own modes, even to add them to the kernel mode list?
>
> Does manufacturing these extra video modes to achieve fast timing
> changes require AMD hardware-specific knowledge, as opposed to the
> general VRR approach of simply adjusting the front porch?
>
> Something like this should also be documented in a commit message. Or
> if you insist that "no reason to not put this in the kernel" is reason
> enough, then write that down, because it does not seem obvious to me or
> others that this feature needs to be in the kernel.
One reason might be debugging, if a feature is known to cause issues.
But imo in that case the knob should be using the _unsafe variants so
it taints the kernel, since otherwise we get stuck in this very cozy
place where kernel maintainers don't have to care much for bugs
"because it's off by default", but also not really care about
polishing the feature "since users can just enable it if they want
it". Just a slightly different flavour of what you're explaining above
already.
-Daniel
> Thanks,
> pq
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2021-01-19 13:11 ` Daniel Vetter
@ 2021-01-19 16:08 ` Pillai, Aurabindo
2021-01-19 18:58 ` Daniel Vetter
0 siblings, 1 reply; 15+ messages in thread
From: Pillai, Aurabindo @ 2021-01-19 16:08 UTC (permalink / raw)
To: Daniel Vetter
Cc: Wang, Chao-kai (Stylon),
Thai, Thong, Sharma, Shashank, dri-devel, Koenig, Christian,
amd-gfx list, Lin, Wayne, Deucher, Alexander, Kazlauskas,
Nicholas
[-- Attachment #1.1: Type: text/plain, Size: 5166 bytes --]
[AMD Official Use Only - Internal Distribution Only]
Hi Daniel,
Could you please be more specific about the _unsafe API options you mentioned ?
--
Thanks & Regards,
Aurabindo Pillai
________________________________
From: Daniel Vetter <daniel@ffwll.ch>
Sent: Tuesday, January 19, 2021 8:11 AM
To: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; dri-devel <dri-devel@lists.freedesktop.org>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wang, Chao-kai (Stylon) <Stylon.Wang@amd.com>; Thai, Thong <Thong.Thai@amd.com>; Sharma, Shashank <Shashank.Sharma@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Subject: Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
On Tue, Jan 19, 2021 at 9:35 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Mon, 18 Jan 2021 09:36:47 -0500
> Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
>
> > On Thu, 2021-01-14 at 11:14 +0200, Pekka Paalanen wrote:
> > >
> > > Hi,
> > >
> > > please document somewhere that ends up in git history (commit
> > > message,
> > > code comments, description of the parameter would be the best but
> > > maybe
> > > there isn't enough space?) what Christian König explained in
> > >
> > >
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-December%2F291254.html&data=04%7C01%7Caurabindo.pillai%40amd.com%7C56ba07934c5c48e7ad7b08d8bc7bb4a9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466586800649481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GM0ZEM9JeFM5os13E1zlVy8Bn3D8Kxmo%2FajSG02WsGI%3D&reserved=0
> > >
> > > that this is a stop-gap feature intended to be removed as soon as
> > > possible (when a better solution comes up, which could be years).
> > >
> > > So far I have not seen a single mention of this intention in your
> > > patch
> > > submissions, and I think it is very important to make known.
> >
> > Hi,
> >
> > Thanks for the headsup, I shall add the relevant info in the next
> > verison.
> >
> > >
> > > I also did not see an explanation of why this instead of
> > > manufacturing
> > > these video modes in userspace (an idea mentioned by Christian in the
> > > referenced email). I think that too should be part of a commit
> > > message.
> >
> > This is an opt-in feature, which shall be superseded by a better
> > solution. We also add a set of common modes for scaling similarly.
> > Userspace can still add whatever mode they want. So I dont see a reason
> > why this cant be in the kernel.
>
> Hi,
>
> sorry, I think that kind of thinking is backwards. There needs to be a
> reason to put something in the kernel, and if there is no reason, then
> it remains in userspace. So what's the reason to put this in the kernel?
>
> One example reason why this should not be in the kernel is that the set
> of video modes to manufacture is a kind of policy, which modes to add
> and which not. Userspace knows what modes it needs, and establishing
> the modes in the kernel instead is second-guessing what the userspace
> would want. So if userspace needs to manufacture modes in userspace
> anyway as some modes might be missed by the kernel, then why bother in
> the kernel to begin with? Why should the kernel play catch-up with what
> modes userspace wants when we already have everything userspace needs
> to make its own modes, even to add them to the kernel mode list?
>
> Does manufacturing these extra video modes to achieve fast timing
> changes require AMD hardware-specific knowledge, as opposed to the
> general VRR approach of simply adjusting the front porch?
>
> Something like this should also be documented in a commit message. Or
> if you insist that "no reason to not put this in the kernel" is reason
> enough, then write that down, because it does not seem obvious to me or
> others that this feature needs to be in the kernel.
One reason might be debugging, if a feature is known to cause issues.
But imo in that case the knob should be using the _unsafe variants so
it taints the kernel, since otherwise we get stuck in this very cozy
place where kernel maintainers don't have to care much for bugs
"because it's off by default", but also not really care about
polishing the feature "since users can just enable it if they want
it". Just a slightly different flavour of what you're explaining above
already.
-Daniel
> Thanks,
> pq
--
Daniel Vetter
Software Engineer, Intel Corporation
https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=04%7C01%7Caurabindo.pillai%40amd.com%7C56ba07934c5c48e7ad7b08d8bc7bb4a9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466586800649481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=2isCpwa3V92TnO4njhe9cQjdWVdsV1GQMo7WP7buVZI%3D&reserved=0
[-- Attachment #1.2: Type: text/html, Size: 8747 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2021-01-19 16:08 ` Pillai, Aurabindo
@ 2021-01-19 18:58 ` Daniel Vetter
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2021-01-19 18:58 UTC (permalink / raw)
To: Pillai, Aurabindo
Cc: Wang, Chao-kai (Stylon),
Thai, Thong, Sharma, Shashank, dri-devel, Koenig, Christian,
amd-gfx list, Lin, Wayne, Deucher, Alexander, Kazlauskas,
Nicholas
On Tue, Jan 19, 2021 at 5:08 PM Pillai, Aurabindo
<Aurabindo.Pillai@amd.com> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
> Hi Daniel,
>
> Could you please be more specific about the _unsafe API options you mentioned ?
module_param_named_unsafe()
Cheers, Daniel
>
> --
>
> Thanks & Regards,
> Aurabindo Pillai
> ________________________________
> From: Daniel Vetter <daniel@ffwll.ch>
> Sent: Tuesday, January 19, 2021 8:11 AM
> To: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; dri-devel <dri-devel@lists.freedesktop.org>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wang, Chao-kai (Stylon) <Stylon.Wang@amd.com>; Thai, Thong <Thong.Thai@amd.com>; Sharma, Shashank <Shashank.Sharma@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> Subject: Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
>
> On Tue, Jan 19, 2021 at 9:35 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Mon, 18 Jan 2021 09:36:47 -0500
> > Aurabindo Pillai <aurabindo.pillai@amd.com> wrote:
> >
> > > On Thu, 2021-01-14 at 11:14 +0200, Pekka Paalanen wrote:
> > > >
> > > > Hi,
> > > >
> > > > please document somewhere that ends up in git history (commit
> > > > message,
> > > > code comments, description of the parameter would be the best but
> > > > maybe
> > > > there isn't enough space?) what Christian König explained in
> > > >
> > > >
> > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-December%2F291254.html&data=04%7C01%7Caurabindo.pillai%40amd.com%7C56ba07934c5c48e7ad7b08d8bc7bb4a9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466586800649481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GM0ZEM9JeFM5os13E1zlVy8Bn3D8Kxmo%2FajSG02WsGI%3D&reserved=0
> > > >
> > > > that this is a stop-gap feature intended to be removed as soon as
> > > > possible (when a better solution comes up, which could be years).
> > > >
> > > > So far I have not seen a single mention of this intention in your
> > > > patch
> > > > submissions, and I think it is very important to make known.
> > >
> > > Hi,
> > >
> > > Thanks for the headsup, I shall add the relevant info in the next
> > > verison.
> > >
> > > >
> > > > I also did not see an explanation of why this instead of
> > > > manufacturing
> > > > these video modes in userspace (an idea mentioned by Christian in the
> > > > referenced email). I think that too should be part of a commit
> > > > message.
> > >
> > > This is an opt-in feature, which shall be superseded by a better
> > > solution. We also add a set of common modes for scaling similarly.
> > > Userspace can still add whatever mode they want. So I dont see a reason
> > > why this cant be in the kernel.
> >
> > Hi,
> >
> > sorry, I think that kind of thinking is backwards. There needs to be a
> > reason to put something in the kernel, and if there is no reason, then
> > it remains in userspace. So what's the reason to put this in the kernel?
> >
> > One example reason why this should not be in the kernel is that the set
> > of video modes to manufacture is a kind of policy, which modes to add
> > and which not. Userspace knows what modes it needs, and establishing
> > the modes in the kernel instead is second-guessing what the userspace
> > would want. So if userspace needs to manufacture modes in userspace
> > anyway as some modes might be missed by the kernel, then why bother in
> > the kernel to begin with? Why should the kernel play catch-up with what
> > modes userspace wants when we already have everything userspace needs
> > to make its own modes, even to add them to the kernel mode list?
> >
> > Does manufacturing these extra video modes to achieve fast timing
> > changes require AMD hardware-specific knowledge, as opposed to the
> > general VRR approach of simply adjusting the front porch?
> >
> > Something like this should also be documented in a commit message. Or
> > if you insist that "no reason to not put this in the kernel" is reason
> > enough, then write that down, because it does not seem obvious to me or
> > others that this feature needs to be in the kernel.
>
> One reason might be debugging, if a feature is known to cause issues.
> But imo in that case the knob should be using the _unsafe variants so
> it taints the kernel, since otherwise we get stuck in this very cozy
> place where kernel maintainers don't have to care much for bugs
> "because it's off by default", but also not really care about
> polishing the feature "since users can just enable it if they want
> it". Just a slightly different flavour of what you're explaining above
> already.
> -Daniel
>
> > Thanks,
> > pq
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=04%7C01%7Caurabindo.pillai%40amd.com%7C56ba07934c5c48e7ad7b08d8bc7bb4a9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466586800649481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=2isCpwa3V92TnO4njhe9cQjdWVdsV1GQMo7WP7buVZI%3D&reserved=0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2020-12-17 19:11 ` Alex Deucher
@ 2020-12-17 22:37 ` Aurabindo Pillai
0 siblings, 0 replies; 15+ messages in thread
From: Aurabindo Pillai @ 2020-12-17 22:37 UTC (permalink / raw)
To: Alex Deucher
Cc: Deucher, Alexander, Shashank Sharma,
Maling list - DRI developers, amd-gfx list, Kazlauskas, Nicholas
On Thu, Dec 17, 2020 at 14:11, Alex Deucher <alexdeucher@gmail.com>
wrote:
> On Mon, Dec 14, 2020 at 5:21 PM Aurabindo Pillai
> <aurabindo.pillai@amd.com> wrote:
>>
>> [Why&How]
>> Adds a module parameter to enable experimental freesync video mode
>> modeset
>> optimization. Enabling this mode allows the driver to skip a full
>> modeset when
>> freesync compatible modes are requested by the userspace. This
>> paramters also
>> adds some standard modes based on the connected monitor's VRR range.
>>
>> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>> Acked-by: Christian König <christian.koenig at amd.com>
>> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
>> 2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index eed5410947e9..e0942184efdd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
>> extern int amdgpu_emu_mode;
>> extern uint amdgpu_smu_memory_pool_size;
>> extern uint amdgpu_dc_feature_mask;
>> +extern uint amdgpu_exp_freesync_vid_mode;
>> extern uint amdgpu_dc_debug_mask;
>> extern uint amdgpu_dm_abm_level;
>> extern struct amdgpu_mgpu_info mgpu_info;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index b2a1dd7581bf..ece51ecd53d1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -158,6 +158,7 @@ int amdgpu_mes;
>> int amdgpu_noretry = -1;
>> int amdgpu_force_asic_type = -1;
>> int amdgpu_tmz;
>> +uint amdgpu_exp_freesync_vid_mode;
>> int amdgpu_reset_method = -1; /* auto */
>> int amdgpu_num_kcq = -1;
>>
>> @@ -786,6 +787,17 @@ module_param_named(abmlevel,
>> amdgpu_dm_abm_level, uint, 0444);
>> MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off
>> (default), 1 = on)");
>> module_param_named(tmz, amdgpu_tmz, int, 0444);
>>
>> +/**
>> + * DOC: experimental_freesync_video (uint)
>> + * Enabled the optimization to adjust front porch timing to
>> achieve seamless mode change experience
>> + * when setting a freesync supported mode for which full modeset
>> is not needed.
>> + * The default value: 0 (off).
>> + */
>> +MODULE_PARM_DESC(
>> + experimental_freesync_video,
>> + "Enable freesync modesetting optimization feature (0 = off
>> (default), 1 = on)");
>
> Maybe just call this freesync_video so that we can change the default
> if we decide to at some point.
Sure, will do.
>
> Alex
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2020-12-14 22:20 ` [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode Aurabindo Pillai
@ 2020-12-17 19:11 ` Alex Deucher
2020-12-17 22:37 ` Aurabindo Pillai
0 siblings, 1 reply; 15+ messages in thread
From: Alex Deucher @ 2020-12-17 19:11 UTC (permalink / raw)
To: Aurabindo Pillai
Cc: Deucher, Alexander, Shashank Sharma,
Maling list - DRI developers, amd-gfx list, Kazlauskas, Nicholas
On Mon, Dec 14, 2020 at 5:21 PM Aurabindo Pillai
<aurabindo.pillai@amd.com> wrote:
>
> [Why&How]
> Adds a module parameter to enable experimental freesync video mode modeset
> optimization. Enabling this mode allows the driver to skip a full modeset when
> freesync compatible modes are requested by the userspace. This paramters also
> adds some standard modes based on the connected monitor's VRR range.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Acked-by: Christian König <christian.koenig at amd.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index eed5410947e9..e0942184efdd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
> extern int amdgpu_emu_mode;
> extern uint amdgpu_smu_memory_pool_size;
> extern uint amdgpu_dc_feature_mask;
> +extern uint amdgpu_exp_freesync_vid_mode;
> extern uint amdgpu_dc_debug_mask;
> extern uint amdgpu_dm_abm_level;
> extern struct amdgpu_mgpu_info mgpu_info;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b2a1dd7581bf..ece51ecd53d1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -158,6 +158,7 @@ int amdgpu_mes;
> int amdgpu_noretry = -1;
> int amdgpu_force_asic_type = -1;
> int amdgpu_tmz;
> +uint amdgpu_exp_freesync_vid_mode;
> int amdgpu_reset_method = -1; /* auto */
> int amdgpu_num_kcq = -1;
>
> @@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444);
> MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)");
> module_param_named(tmz, amdgpu_tmz, int, 0444);
>
> +/**
> + * DOC: experimental_freesync_video (uint)
> + * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience
> + * when setting a freesync supported mode for which full modeset is not needed.
> + * The default value: 0 (off).
> + */
> +MODULE_PARM_DESC(
> + experimental_freesync_video,
> + "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");
Maybe just call this freesync_video so that we can change the default
if we decide to at some point.
Alex
> +module_param_named(experimental_freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444);
> +
> /**
> * DOC: reset_method (int)
> * GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)
> --
> 2.29.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode
2020-12-14 22:20 [PATCH v3 0/3] Experimental freesync video mode optimization Aurabindo Pillai
@ 2020-12-14 22:20 ` Aurabindo Pillai
2020-12-17 19:11 ` Alex Deucher
0 siblings, 1 reply; 15+ messages in thread
From: Aurabindo Pillai @ 2020-12-14 22:20 UTC (permalink / raw)
To: amd-gfx, dri-devel
Cc: alexander.deucher, aurabindo.pillai, nicholas.kazlauskas,
shashank.sharma
[Why&How]
Adds a module parameter to enable experimental freesync video mode modeset
optimization. Enabling this mode allows the driver to skip a full modeset when
freesync compatible modes are requested by the userspace. This paramters also
adds some standard modes based on the connected monitor's VRR range.
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Acked-by: Christian König <christian.koenig at amd.com>
Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index eed5410947e9..e0942184efdd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery;
extern int amdgpu_emu_mode;
extern uint amdgpu_smu_memory_pool_size;
extern uint amdgpu_dc_feature_mask;
+extern uint amdgpu_exp_freesync_vid_mode;
extern uint amdgpu_dc_debug_mask;
extern uint amdgpu_dm_abm_level;
extern struct amdgpu_mgpu_info mgpu_info;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b2a1dd7581bf..ece51ecd53d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -158,6 +158,7 @@ int amdgpu_mes;
int amdgpu_noretry = -1;
int amdgpu_force_asic_type = -1;
int amdgpu_tmz;
+uint amdgpu_exp_freesync_vid_mode;
int amdgpu_reset_method = -1; /* auto */
int amdgpu_num_kcq = -1;
@@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444);
MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)");
module_param_named(tmz, amdgpu_tmz, int, 0444);
+/**
+ * DOC: experimental_freesync_video (uint)
+ * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience
+ * when setting a freesync supported mode for which full modeset is not needed.
+ * The default value: 0 (off).
+ */
+MODULE_PARM_DESC(
+ experimental_freesync_video,
+ "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");
+module_param_named(experimental_freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444);
+
/**
* DOC: reset_method (int)
* GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)
--
2.29.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-01-19 18:59 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-04 21:07 [PATCH v3 0/3] Experimental freesync video mode optimization Aurabindo Pillai
2021-01-04 21:07 ` [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode Aurabindo Pillai
2021-01-14 9:14 ` Pekka Paalanen
2021-01-18 14:36 ` Aurabindo Pillai
2021-01-19 8:35 ` Pekka Paalanen
2021-01-19 13:11 ` Daniel Vetter
2021-01-19 16:08 ` Pillai, Aurabindo
2021-01-19 18:58 ` Daniel Vetter
2021-01-04 21:07 ` [PATCH v3 2/3] drm/amd/display: Add freesync video modes based on preferred modes Aurabindo Pillai
2021-01-04 21:08 ` [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai
2021-01-06 20:02 ` Kazlauskas, Nicholas
2021-01-17 19:52 ` Aurabindo Pillai
-- strict thread matches above, loose matches on Subject: below --
2020-12-14 22:20 [PATCH v3 0/3] Experimental freesync video mode optimization Aurabindo Pillai
2020-12-14 22:20 ` [PATCH v3 1/3] drm/amd/display: Add module parameter for freesync video mode Aurabindo Pillai
2020-12-17 19:11 ` Alex Deucher
2020-12-17 22:37 ` Aurabindo Pillai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).