Re: [PATCH v6 3/5] drm: Document variable refresh properties

["Kazlauskas, Nicholas" <Nicholas.Kazlauskas@amd.com>]

On 11/7/18 9:57 AM, Wentland, Harry wrote:
> 
> 
> On 2018-11-06 3:24 p.m., Nicholas Kazlauskas wrote:
>> These include the drm_connector 'vrr_capable' and the drm_crtc
>> 'vrr_enabled' properties.
>>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Cc: Pekka Paalanen <ppaalanen@gmail.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Michel Dänzer <michel@daenzer.net>
>> ---
>>   Documentation/gpu/drm-kms.rst   |  7 ++++
>>   drivers/gpu/drm/drm_connector.c | 61 +++++++++++++++++++++++++++++++++
>>   2 files changed, 68 insertions(+)
>>
>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
>> index 4b1501b4835b..8da2a178cf85 100644
>> --- a/Documentation/gpu/drm-kms.rst
>> +++ b/Documentation/gpu/drm-kms.rst
>> @@ -575,6 +575,13 @@ Explicit Fencing Properties
>>   .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c
>>      :doc: explicit fencing properties
>>   
>> +
>> +Variable Refresh Properties
>> +---------------------------
>> +
>> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
>> +   :doc: Variable refresh properties
>> +
>>   Existing KMS Properties
>>   -----------------------
>>   
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 49290060ab7b..a6adf5450db3 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1255,6 +1255,67 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>>   }
>>   EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>>   
>> +/**
>> + * DOC: Variable refresh properties
>> + *
>> + * Variable refresh rate capable displays can dynamically adjust their
>> + * refresh rate by extending the duration of their vertical porch until
> 
> vertical porch -> vertical front porch
> 
>> + * page flip or timeout occurs. This can reduce or remove stuttering
>> + * and latency in scenarios where the page flip does not align with the
>> + * vblank interval.
>> + *
>> + * An example scenario would be an application flipping at a constant rate
>> + * of 48Hz on a 60Hz display. The page flip will frequently miss the vblank
>> + * interval and the same contents will be displayed twice. This can be
>> + * observed as stuttering for content with motion.
>> + *
>> + * If variable refresh rate was active on a display that supported a
>> + * variable refresh range from 35Hz to 60Hz no stuttering would be observable
>> + * for the example scenario. The minimum supported variable refresh rate of
>> + * 35Hz is below the page flip frequency and the vertical front porch can
>> + * be extended until the page flip occurs. The vblank interval will be
>> + * directly aligned to the page flip rate.
>> + *
>> + * Userspace control for variable refresh rate is supported via properties
>> + * on the &drm_connector and &drm_crtc objects.
>> + *
>> + * "vrr_capable":
>> + *	Optional &drm_connector boolean property that drivers should attach
>> + *	with drm_connector_attach_vrr_capable_property() on connectors that
>> + *	could support variable refresh rates. Drivers should update the
>> + *	property value by calling drm_connector_set_vrr_capable_property().
>> + *
>> + *	Absence of the property should indicate absence of support.
>> + *
>> + * "vrr_enabled":
>> + *	Default &drm_crtc boolean property that notifies the driver that the
>> + *	content on the CRTC is suitable for variable refresh rate presentation.
>> + *	The driver will take this property as a hint to enable variable
>> + *	refresh rate support if the receiver supports it, ie. if the
>> + *	"vrr_capable" property is true on the &drm_connector object. The
>> + *	veritcal front porch duration will be extended until page-flip or
> 
> veritcal -> vertical
> 
>> + *	timeout when enabled.
>> + *
>> + *	The minimum vertical front porch duration is defined as the vertical
>> + *	front porch duration for the current mode.
>> + *
>> + *	The maximum vertical front porch duration is greater than or equal to
>> + *	the minimum vertical front porch duration. The duration is derived
>> + *	from the minimum supported variable refresh rate for the connector.
>> + *
>> + *	The driver may place further restrictions within these minimum
>> + *	and maximum bounds.
>> + *
>> + *	Some displays may exhibit noticeable differences in brightness when
>> + *	varying vertical front porch duration.
>> + *
> 
> Maybe something like this makes sense here:
> 
>   *	Some displays may exhibit noticeable differences in brightness when
>   *	varying vertical front porch duration drastically. It is therefore
>   *	advised userspace only provide the "vrr_enabled" hint for content
>   *	with a render rate that is expected to change gradually frame to frame,
>   *	such as games.

Good point about mentioning that userspace shouldn't expect this to work 
well with frequent (and large) changes in vertical front porch duration.

I should probably more clearly word that the panel brightness may differ 
based on the vertical front porch duration. So large changes will be 
more noticeable.

> 
>> + *	The semantics for the vertical blank timestamp differ when
>> + *	variable refresh rate is active. The vertical blank timestamp
>> + *	is defined to be an estimate using the current mode's fixed
>> + *	refresh rate timings. The semantics for the page-flip event
>> + *	timestamp remain the same.
>> + */
>> +
>>   /**
>>    * drm_connector_attach_vrr_capable_property - creates the
>>    * vrr_capable property
>>

Thanks for the review, I'll fix up these for the next revision.

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