[PATCH v4 3/4] drm: Document variable refresh properties

[Manasi Navare]

On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> > On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:
> >> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> >>> On Thu, 11 Oct 2018 12:39:41 -0400
> >>> Nicholas Kazlauskas <nicholas.kazlauskas at amd.com> wrote:
> >>>
> >>>> These include the drm_connector 'vrr_capable' and the drm_crtc
> >>>> 'vrr_enabled' properties.
> >>>>
> >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> >>>> ---
> >>>>    Documentation/gpu/drm-kms.rst   |  7 +++++++
> >>>>    drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++
> >>>>    2 files changed, 29 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 f0deeb7298d0..2a12853ca917 100644
> >>>> --- a/drivers/gpu/drm/drm_connector.c
> >>>> +++ b/drivers/gpu/drm/drm_connector.c
> >>>> @@ -1254,6 +1254,28 @@ 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 control 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
> >>>> + *	variable refresh rate adjustment should be enabled for the CRTC.
> >>>
> >>> Hi,
> >>>
> >>> where is the documentation that explains how drivers must implement
> >>> "variable refresh rate adjustment"?
> >>>
> >>> What should and could userspace expect to get if it flicks this switch?
> >>>
> >>> I also think the kernel documentation should include a description of
> >>> what VRR actually is and how it conceptually works as far as userspace
> >>> is concerned.
> >>>
> >>> That is, the kernel documentation should describe what this thing does,
> >>> so that we avoid every driver implementing a different thing. For
> >>> example, one driver could prevent the luminance flickering itself by
> >>> tuning the timings while another driver might not do that, which means
> >>> that an application tested on the former driver will look just fine
> >>> while it is unbearable to watch on the latter.
> >>>
> >>>
> >>> Thanks,
> >>> pq
> >>
> >> I felt it was best to leave this more on the vague side to not impose
> >> restrictions yet on what a driver must do.
> >>
> >> If you think it's worth defining what the "baseline" expectation is for
> >> userspace, I would probably describe it as "utilizing the monitor's
> >> variable refresh rate to reduce stuttering or tearing that can occur
> >> during flipping". This is currently what the amdgpu driver has enabled

I would also mention that without VRR, the display engine processes the flips 
independently of the rendering speed which might result into stuttering or tearing.

Might also be worth giving a quick example with numbers in the documentation.
Something like: For Eg if the rendering speed is 40Hz, without VRR, display engine
will flip at constant 60Hz, which means that same frame will be displayed twice which
will be observed as stutter/repetition.
With VRR enabled, the vertical front porch will be extended and the flip will
be processed when its ready or at max blanking time resulting a smooth transition without repetition.

> >> for support. The implementation also isn't much more complex than just
> >> passing the variable refresh range to the hardware.
> >>
> >> I wouldn't want every driver to be forced to implement some sort of
> >> luminance flickering by default. It's not noticeable on many panels and
> >> any tuning would inherently add latency to the output. It would probably
> >> be better left as a new property or a driver specific feature.
> >>
> >> In general I would imagine that most future VRR features would end up as
> >> new properties. Anything that's purely software could be implemented as
> >> a drm helper that every driver can use. I think the target presentation
> >> timestamp feature is a good example for that.
> > 
> > Speaking of timestamps. What is the expected behaviour of vblank
> > timestamps when vrr is enabled?
> >
> 
> When vrr is enabled the duration of the vertical front porch will be
> extended until flip or timeout occurs. The vblank timestamp will vary
> based on duration of the vertical front porch. The min/max duration for
> the front porch can be specified by the driver via the min/max range.

This min max range that is read from monitor descriptor is only used to program
the HW registers right? Its not exposed to the userspace, correct?

Manasi

> 
> No changes to vblank timestamping handling should be necessary to
> accommodate variable refresh rate.
> 
> I think it's probably best to update the documentation for vrr_enable 
> with some of the specifics I described above. That should help clarify 
> userspace expectations as well.
> 
> Nicholas Kazlauskas