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@xxxxxxx> wrote: > >>>>> > >>>>>> These include the drm_connector 'vrr_capable' and the drm_crtc > >>>>>> 'vrr_enabled' properties. > >>>>>> > >>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx> > >>>>>> --- > >>>>>> 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 _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
