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

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Oct 26, 2018 at 08:06:11PM +0000, Kazlauskas, Nicholas wrote:
> On 10/26/18 3:13 PM, Manasi Navare wrote:
> > 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@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
> > 
> > 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.
> 
> Good points about mentioning the problems it solves in the documentation.
> 
> > 
> >>>> 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
> 
> Currently the range isn't exposed to userspace.
> 
> I think I wouldn't mind having them for testing purposes but that can 
> just be done via debugfs. They might make more sense as properties but I 
> don't have any integration patches to utilize them in userspace.
> 
> > 
> >>
> >> 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

Yes debugfs for min, max Vrefresh range will be good.

Manasi

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