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

["Ville Syrjälä" <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

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@amd.com> wrote:
> > 
> >> These include the drm_connector 'vrr_capable' and the drm_crtc
> >> 'vrr_enabled' properties.
> >>
> >> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@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 
> 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?

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx