[PATCH] drm/amd/display: Add Freesync video documentation

[Aurabindo Pillai]

Hi Siqueira,

Please see inline comments.

On 2021-06-10 8:48 a.m., Rodrigo Siqueira wrote:
> Recently, we added support for an experimental feature named Freesync
> video; for more details on that, refer to:
> 
> commit a372f4abecb1 ("drm/amd/display: Skip modeset for front porch change")
> commit 952bc47fb2ca ("drm/amd/display: Add freesync video modes based on preferred modes")
> commit d03ee581eee6 ("drm/amd/display: Add module parameter for freesync video mode")
> 
> Nevertheless, we did not document it in detail in our driver. This
> commit introduces a kernel-doc and expands the module parameter
> description.
> 
> Cc: Aurabindo Pillai <aurabindo.pillai at amd.com>
> Cc: Sean Paul <seanpaul at chromium.org>
> Cc: Harry Wentland <Harry.Wentland at amd.com>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
> ---
>   Documentation/gpu/amdgpu-dc.rst               |  6 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       | 17 +++++++++--
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 30 +++++++++++++++++++
>   3 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/gpu/amdgpu-dc.rst b/Documentation/gpu/amdgpu-dc.rst
> index cc89b0fc11df..f7ff7e1309de 100644
> --- a/Documentation/gpu/amdgpu-dc.rst
> +++ b/Documentation/gpu/amdgpu-dc.rst
> @@ -66,3 +66,9 @@ Display Core
>   ============
>   
>   **WIP**
> +
> +FreeSync Video
> +--------------
> +
> +.. kernel-doc:: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +   :doc: FreeSync Video
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 3de1accb060e..561c7ead4a5c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -836,8 +836,21 @@ module_param_named(tmz, amdgpu_tmz, int, 0444);
>   
>   /**
>    * DOC: 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.
> + * 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

Nitpick, s/core/Core

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

Maybe add one more line here to indicate that userspace applications can not only read the mode 
list, but also add new modes as necessary, like "They 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(
> 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 54dfa245b656..710ee3954062 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5718,6 +5718,36 @@ static void apply_dsc_policy_for_stream(struct amdgpu_dm_connector *aconnector,
>   }
>   #endif
>   
> +/**
> + * DOC: FreeSync Video
> + *
> + * When a userspace application wants to play a video, the content follows a
> + * standard format definition that usually specifies the FPS for that format.
> + * The below list illustrates some video format and the expected FPS,
> + * respectively:
> + *
> + * - TV/NTSC (23.976 FPS)
> + * - Cinema (24 FPS)
> + * - TV/PAL (25 FPS)
> + * - TV/NTSC (29.97 FPS)
> + * - TV/NTSC (30 FPS)
> + * - Cinema HFR (48 FPS)
> + * - TV/PAL (50 FPS)
> + * - Commonly used (60 FPS)
> + * - Multiples of 24 (48,72,96 FPS)
> + *
> + * The list of standards video format is not huge and can be added to the

s/standards/standard

> + * connector modeset list beforehand. With that, userspace can leverage
> + * FreeSync to extends the front porch in order to attain the target refresh
> + * rate. Such a switch will happen seamlessly, without screen blanking or
> + * reprogramming of the output in any other way. If the userspace requests a
> + * modesetting change compatible with FreeSync modes that only differ in the
> + * refresh rate, DC will skip the full update and avoid blink during the
> + * transition. For example, the video player can change the modesetting from
> + * 60Hz to 30Hz for playing TV/NTSC content when it goes full screen without
> + * causing any display blink. This same concept can be applied to a mode

Nitpick, is blink or blank more appropriate?

> + * setting change.
> + */
>   static struct drm_display_mode *
>   get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
>   			  bool use_probed_modes)
> 

With or without these comments addressed, the patch is:
Reviewed by: Aurabindo Pillai <aurabindo.pillai at amd.com>


--

Thanks & Regards,
Aurabindo