[PATCH 1/2] Revert "drm/amd: Remove freesync video mode amdgpu parameter"

[PATCH 1/2] Revert "drm/amd: Remove freesync video mode amdgpu parameter"

[Alex Deucher]

This reverts commit e94e787e37b99645e7c02d20d0a1ba0f8a18a82a.

This conflicts with how compositors want to handle VRR.  Now
that compositors actually handle VRR, we probably don't need
freesync video.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2985
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 27 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 0e365cadcc3fc..925026c183f41 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -194,6 +194,7 @@ extern int amdgpu_emu_mode;
 extern uint amdgpu_smu_memory_pool_size;
 extern int amdgpu_smu_pptable_id;
 extern uint amdgpu_dc_feature_mask;
+extern uint amdgpu_freesync_vid_mode;
 extern uint amdgpu_dc_debug_mask;
 extern uint amdgpu_dc_visual_confirm;
 extern int amdgpu_dm_abm_level;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 15a8a64fc4e28..82b154b103f43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -199,6 +199,7 @@ int amdgpu_mes_kiq;
 int amdgpu_noretry = -1;
 int amdgpu_force_asic_type = -1;
 int amdgpu_tmz = -1; /* auto */
+uint amdgpu_freesync_vid_mode;
 int amdgpu_reset_method = -1; /* auto */
 int amdgpu_num_kcq = -1;
 int amdgpu_smartshift_bias;
@@ -883,6 +884,32 @@ module_param_named(damageclips, amdgpu_damage_clips, int, 0444);
 MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto (default), 0 = off, 1 = on)");
 module_param_named(tmz, amdgpu_tmz, int, 0444);
 
+/**
+ * DOC: freesync_video (uint)
+ * Enable 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 Display Core will add a set of modes derived from the base FreeSync
+ * video mode into the corresponding connector's mode list based on commonly
+ * used refresh rates and VRR range of the connected display, when users enable
+ * this feature. From the userspace perspective, they can see a seamless mode
+ * change experience when the change between different refresh rates under the
+ * same resolution. Additionally, userspace applications such as Video playback
+ * can read this modeset list and change the refresh rate based on the video
+ * frame rate. Finally, the userspace can also derive an appropriate mode for a
+ * particular refresh rate based on the FreeSync Mode and add it to the
+ * connector's mode list.
+ *
+ * Note: This is an experimental feature.
+ *
+ * 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_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.43.2

[PATCH 2/2] Reapply "Revert drm/amd/display: Enable Freesync Video Mode by default"

[Alex Deucher]

This reverts commit 11b92df8a2f7f4605ccc764ce6ae4a72760674df.

This conflicts with how compositors want to handle VRR.  Now
that compositors actually handle VRR, we probably don't need
freesync video.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2985
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 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 5765e492c4557..2c283c47bcf6a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6230,7 +6230,8 @@ create_stream_for_sink(struct drm_connector *connector,
 		 */
 		DRM_DEBUG_DRIVER("No preferred mode found\n");
 	} else if (aconnector) {
-		recalculate_timing = is_freesync_video_mode(&mode, aconnector);
+		recalculate_timing = amdgpu_freesync_vid_mode &&
+				 is_freesync_video_mode(&mode, aconnector);
 		if (recalculate_timing) {
 			freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
 			drm_mode_copy(&saved_mode, &mode);
@@ -7541,7 +7542,7 @@ static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connect
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 		to_amdgpu_dm_connector(connector);
 
-	if (!edid)
+	if (!(amdgpu_freesync_vid_mode && edid))
 		return;
 
 	if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
@@ -9835,7 +9836,8 @@ 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 &&
+		if (amdgpu_freesync_vid_mode &&
+		    dm_new_crtc_state->stream &&
 		    is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state))
 			goto skip_modeset;
 
@@ -9875,7 +9877,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 		}
 
 		/* Now check if we should set freesync video mode */
-		if (dm_new_crtc_state->stream &&
+		if (amdgpu_freesync_vid_mode && 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) &&
 		    is_timing_unchanged_for_freesync(new_crtc_state,
@@ -9888,7 +9890,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 			set_freesync_fixed_config(dm_new_crtc_state);
 
 			goto skip_modeset;
-		} else if (aconnector &&
+		} else if (amdgpu_freesync_vid_mode && aconnector &&
 			   is_freesync_video_mode(&new_crtc_state->mode,
 						  aconnector)) {
 			struct drm_display_mode *high_mode;
-- 
2.43.2

Re: [PATCH 1/2] Revert "drm/amd: Remove freesync video mode amdgpu parameter"

[Hamza Mahfooz]

On 2/27/24 13:48, Alex Deucher wrote:
> This reverts commit e94e787e37b99645e7c02d20d0a1ba0f8a18a82a.
> 
> This conflicts with how compositors want to handle VRR.  Now
> that compositors actually handle VRR, we probably don't need
> freesync video.
> 
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2985
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

Series is:
Acked-by: Hamza Mahfooz <hamza.mahfooz@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 27 +++++++++++++++++++++++++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 0e365cadcc3fc..925026c183f41 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -194,6 +194,7 @@ extern int amdgpu_emu_mode;
>   extern uint amdgpu_smu_memory_pool_size;
>   extern int amdgpu_smu_pptable_id;
>   extern uint amdgpu_dc_feature_mask;
> +extern uint amdgpu_freesync_vid_mode;
>   extern uint amdgpu_dc_debug_mask;
>   extern uint amdgpu_dc_visual_confirm;
>   extern int amdgpu_dm_abm_level;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 15a8a64fc4e28..82b154b103f43 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -199,6 +199,7 @@ int amdgpu_mes_kiq;
>   int amdgpu_noretry = -1;
>   int amdgpu_force_asic_type = -1;
>   int amdgpu_tmz = -1; /* auto */
> +uint amdgpu_freesync_vid_mode;
>   int amdgpu_reset_method = -1; /* auto */
>   int amdgpu_num_kcq = -1;
>   int amdgpu_smartshift_bias;
> @@ -883,6 +884,32 @@ module_param_named(damageclips, amdgpu_damage_clips, int, 0444);
>   MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto (default), 0 = off, 1 = on)");
>   module_param_named(tmz, amdgpu_tmz, int, 0444);
>   
> +/**
> + * DOC: freesync_video (uint)
> + * Enable 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 Display Core will add a set of modes derived from the base FreeSync
> + * video mode into the corresponding connector's mode list based on commonly
> + * used refresh rates and VRR range of the connected display, when users enable
> + * this feature. From the userspace perspective, they can see a seamless mode
> + * change experience when the change between different refresh rates under the
> + * same resolution. Additionally, userspace applications such as Video playback
> + * can read this modeset list and change the refresh rate based on the video
> + * frame rate. Finally, the userspace can also derive an appropriate mode for a
> + * particular refresh rate based on the FreeSync Mode and add it to the
> + * connector's mode list.
> + *
> + * Note: This is an experimental feature.
> + *
> + * 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_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)
-- 
Hamza

Re: [PATCH 1/2] Revert "drm/amd: Remove freesync video mode amdgpu parameter"

[Christian König]

Am 27.02.24 um 19:48 schrieb Alex Deucher:
> This reverts commit e94e787e37b99645e7c02d20d0a1ba0f8a18a82a.
>
> This conflicts with how compositors want to handle VRR.  Now
> that compositors actually handle VRR, we probably don't need
> freesync video.
>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2985

Scratching my head what actually happens here? Doesn't the problem then 
just depend on a module parameter?

Regards,
Christian.

> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 27 +++++++++++++++++++++++++
>   2 files changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 0e365cadcc3fc..925026c183f41 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -194,6 +194,7 @@ extern int amdgpu_emu_mode;
>   extern uint amdgpu_smu_memory_pool_size;
>   extern int amdgpu_smu_pptable_id;
>   extern uint amdgpu_dc_feature_mask;
> +extern uint amdgpu_freesync_vid_mode;
>   extern uint amdgpu_dc_debug_mask;
>   extern uint amdgpu_dc_visual_confirm;
>   extern int amdgpu_dm_abm_level;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 15a8a64fc4e28..82b154b103f43 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -199,6 +199,7 @@ int amdgpu_mes_kiq;
>   int amdgpu_noretry = -1;
>   int amdgpu_force_asic_type = -1;
>   int amdgpu_tmz = -1; /* auto */
> +uint amdgpu_freesync_vid_mode;
>   int amdgpu_reset_method = -1; /* auto */
>   int amdgpu_num_kcq = -1;
>   int amdgpu_smartshift_bias;
> @@ -883,6 +884,32 @@ module_param_named(damageclips, amdgpu_damage_clips, int, 0444);
>   MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto (default), 0 = off, 1 = on)");
>   module_param_named(tmz, amdgpu_tmz, int, 0444);
>   
> +/**
> + * DOC: freesync_video (uint)
> + * Enable 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 Display Core will add a set of modes derived from the base FreeSync
> + * video mode into the corresponding connector's mode list based on commonly
> + * used refresh rates and VRR range of the connected display, when users enable
> + * this feature. From the userspace perspective, they can see a seamless mode
> + * change experience when the change between different refresh rates under the
> + * same resolution. Additionally, userspace applications such as Video playback
> + * can read this modeset list and change the refresh rate based on the video
> + * frame rate. Finally, the userspace can also derive an appropriate mode for a
> + * particular refresh rate based on the FreeSync Mode and add it to the
> + * connector's mode list.
> + *
> + * Note: This is an experimental feature.
> + *
> + * 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_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)

Re: [PATCH 1/2] Revert "drm/amd: Remove freesync video mode amdgpu parameter"

[Alex Deucher]

On Wed, Feb 28, 2024 at 2:03 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 27.02.24 um 19:48 schrieb Alex Deucher:
> > This reverts commit e94e787e37b99645e7c02d20d0a1ba0f8a18a82a.
> >
> > This conflicts with how compositors want to handle VRR.  Now
> > that compositors actually handle VRR, we probably don't need
> > freesync video.
> >
> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2985
>
> Scratching my head what actually happens here? Doesn't the problem then
> just depend on a module parameter?

Yes.  The problem is that when freesync video is enabled, compositors
don't know which modes are actual modes and which are a VRR video
mode.  There are still customers that want the vrr video mode smooth
video playback, but compositors don't want this by default.  I guess
the alternative is to just drop this feature altogether now that
compositors and media players are starting to support this properly.

Alex

>
> Regards,
> Christian.
>
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 27 +++++++++++++++++++++++++
> >   2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 0e365cadcc3fc..925026c183f41 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -194,6 +194,7 @@ extern int amdgpu_emu_mode;
> >   extern uint amdgpu_smu_memory_pool_size;
> >   extern int amdgpu_smu_pptable_id;
> >   extern uint amdgpu_dc_feature_mask;
> > +extern uint amdgpu_freesync_vid_mode;
> >   extern uint amdgpu_dc_debug_mask;
> >   extern uint amdgpu_dc_visual_confirm;
> >   extern int amdgpu_dm_abm_level;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index 15a8a64fc4e28..82b154b103f43 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -199,6 +199,7 @@ int amdgpu_mes_kiq;
> >   int amdgpu_noretry = -1;
> >   int amdgpu_force_asic_type = -1;
> >   int amdgpu_tmz = -1; /* auto */
> > +uint amdgpu_freesync_vid_mode;
> >   int amdgpu_reset_method = -1; /* auto */
> >   int amdgpu_num_kcq = -1;
> >   int amdgpu_smartshift_bias;
> > @@ -883,6 +884,32 @@ module_param_named(damageclips, amdgpu_damage_clips, int, 0444);
> >   MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto (default), 0 = off, 1 = on)");
> >   module_param_named(tmz, amdgpu_tmz, int, 0444);
> >
> > +/**
> > + * DOC: freesync_video (uint)
> > + * Enable 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 Display Core will add a set of modes derived from the base FreeSync
> > + * video mode into the corresponding connector's mode list based on commonly
> > + * used refresh rates and VRR range of the connected display, when users enable
> > + * this feature. From the userspace perspective, they can see a seamless mode
> > + * change experience when the change between different refresh rates under the
> > + * same resolution. Additionally, userspace applications such as Video playback
> > + * can read this modeset list and change the refresh rate based on the video
> > + * frame rate. Finally, the userspace can also derive an appropriate mode for a
> > + * particular refresh rate based on the FreeSync Mode and add it to the
> > + * connector's mode list.
> > + *
> > + * Note: This is an experimental feature.
> > + *
> > + * 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_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)
>

Re: [PATCH 1/2] Revert "drm/amd: Remove freesync video mode amdgpu parameter"

[Christian König]

Am 28.02.24 um 15:23 schrieb Alex Deucher:
> On Wed, Feb 28, 2024 at 2:03 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Am 27.02.24 um 19:48 schrieb Alex Deucher:
>>> This reverts commit e94e787e37b99645e7c02d20d0a1ba0f8a18a82a.
>>>
>>> This conflicts with how compositors want to handle VRR.  Now
>>> that compositors actually handle VRR, we probably don't need
>>> freesync video.
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2985
>> Scratching my head what actually happens here? Doesn't the problem then
>> just depend on a module parameter?
> Yes.  The problem is that when freesync video is enabled, compositors
> don't know which modes are actual modes and which are a VRR video
> mode.  There are still customers that want the vrr video mode smooth
> video playback, but compositors don't want this by default.  I guess
> the alternative is to just drop this feature altogether now that
> compositors and media players are starting to support this properly.

That's what I would suggest as well.

As far as I can see adding those modes is actually buggy behavior and we 
need to avoid it.

Christian.

>
> Alex
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 27 +++++++++++++++++++++++++
>>>    2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 0e365cadcc3fc..925026c183f41 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -194,6 +194,7 @@ extern int amdgpu_emu_mode;
>>>    extern uint amdgpu_smu_memory_pool_size;
>>>    extern int amdgpu_smu_pptable_id;
>>>    extern uint amdgpu_dc_feature_mask;
>>> +extern uint amdgpu_freesync_vid_mode;
>>>    extern uint amdgpu_dc_debug_mask;
>>>    extern uint amdgpu_dc_visual_confirm;
>>>    extern int amdgpu_dm_abm_level;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index 15a8a64fc4e28..82b154b103f43 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -199,6 +199,7 @@ int amdgpu_mes_kiq;
>>>    int amdgpu_noretry = -1;
>>>    int amdgpu_force_asic_type = -1;
>>>    int amdgpu_tmz = -1; /* auto */
>>> +uint amdgpu_freesync_vid_mode;
>>>    int amdgpu_reset_method = -1; /* auto */
>>>    int amdgpu_num_kcq = -1;
>>>    int amdgpu_smartshift_bias;
>>> @@ -883,6 +884,32 @@ module_param_named(damageclips, amdgpu_damage_clips, int, 0444);
>>>    MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto (default), 0 = off, 1 = on)");
>>>    module_param_named(tmz, amdgpu_tmz, int, 0444);
>>>
>>> +/**
>>> + * DOC: freesync_video (uint)
>>> + * Enable 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 Display Core will add a set of modes derived from the base FreeSync
>>> + * video mode into the corresponding connector's mode list based on commonly
>>> + * used refresh rates and VRR range of the connected display, when users enable
>>> + * this feature. From the userspace perspective, they can see a seamless mode
>>> + * change experience when the change between different refresh rates under the
>>> + * same resolution. Additionally, userspace applications such as Video playback
>>> + * can read this modeset list and change the refresh rate based on the video
>>> + * frame rate. Finally, the userspace can also derive an appropriate mode for a
>>> + * particular refresh rate based on the FreeSync Mode and add it to the
>>> + * connector's mode list.
>>> + *
>>> + * Note: This is an experimental feature.
>>> + *
>>> + * 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_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)

Re: [PATCH 1/2] Revert "drm/amd: Remove freesync video mode amdgpu parameter"

[Alex Deucher]

On Wed, Feb 28, 2024 at 9:45 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 28.02.24 um 15:23 schrieb Alex Deucher:
> > On Wed, Feb 28, 2024 at 2:03 AM Christian König
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> >> Am 27.02.24 um 19:48 schrieb Alex Deucher:
> >>> This reverts commit e94e787e37b99645e7c02d20d0a1ba0f8a18a82a.
> >>>
> >>> This conflicts with how compositors want to handle VRR.  Now
> >>> that compositors actually handle VRR, we probably don't need
> >>> freesync video.
> >>>
> >>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2985
> >> Scratching my head what actually happens here? Doesn't the problem then
> >> just depend on a module parameter?
> > Yes.  The problem is that when freesync video is enabled, compositors
> > don't know which modes are actual modes and which are a VRR video
> > mode.  There are still customers that want the vrr video mode smooth
> > video playback, but compositors don't want this by default.  I guess
> > the alternative is to just drop this feature altogether now that
> > compositors and media players are starting to support this properly.
>
> That's what I would suggest as well.
>
> As far as I can see adding those modes is actually buggy behavior and we
> need to avoid it.

Well, they work as expected for the use case they were added for,
video playback with different refresh rates without a modeset.  I'll
apply then as is for now and then work on a set of patches to remove
the functionality.

Alex


>
> Christian.
>
> >
> > Alex
> >
> >> Regards,
> >> Christian.
> >>
> >>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >>> ---
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 27 +++++++++++++++++++++++++
> >>>    2 files changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index 0e365cadcc3fc..925026c183f41 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -194,6 +194,7 @@ extern int amdgpu_emu_mode;
> >>>    extern uint amdgpu_smu_memory_pool_size;
> >>>    extern int amdgpu_smu_pptable_id;
> >>>    extern uint amdgpu_dc_feature_mask;
> >>> +extern uint amdgpu_freesync_vid_mode;
> >>>    extern uint amdgpu_dc_debug_mask;
> >>>    extern uint amdgpu_dc_visual_confirm;
> >>>    extern int amdgpu_dm_abm_level;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> index 15a8a64fc4e28..82b154b103f43 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>> @@ -199,6 +199,7 @@ int amdgpu_mes_kiq;
> >>>    int amdgpu_noretry = -1;
> >>>    int amdgpu_force_asic_type = -1;
> >>>    int amdgpu_tmz = -1; /* auto */
> >>> +uint amdgpu_freesync_vid_mode;
> >>>    int amdgpu_reset_method = -1; /* auto */
> >>>    int amdgpu_num_kcq = -1;
> >>>    int amdgpu_smartshift_bias;
> >>> @@ -883,6 +884,32 @@ module_param_named(damageclips, amdgpu_damage_clips, int, 0444);
> >>>    MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto (default), 0 = off, 1 = on)");
> >>>    module_param_named(tmz, amdgpu_tmz, int, 0444);
> >>>
> >>> +/**
> >>> + * DOC: freesync_video (uint)
> >>> + * Enable 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 Display Core will add a set of modes derived from the base FreeSync
> >>> + * video mode into the corresponding connector's mode list based on commonly
> >>> + * used refresh rates and VRR range of the connected display, when users enable
> >>> + * this feature. From the userspace perspective, they can see a seamless mode
> >>> + * change experience when the change between different refresh rates under the
> >>> + * same resolution. Additionally, userspace applications such as Video playback
> >>> + * can read this modeset list and change the refresh rate based on the video
> >>> + * frame rate. Finally, the userspace can also derive an appropriate mode for a
> >>> + * particular refresh rate based on the FreeSync Mode and add it to the
> >>> + * connector's mode list.
> >>> + *
> >>> + * Note: This is an experimental feature.
> >>> + *
> >>> + * 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_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)
>