[PATCH v2 2/3] drm: Add variable refresh property to DRM CRTC
Kazlauskas, Nicholas
nicholas.kazlauskas at amd.com
Tue Sep 25 13:51:37 UTC 2018
On 09/24/2018 04:26 PM, Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote:
>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote:
>>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote:
>>>> Variable refresh rate algorithms have typically been enabled only
>>>> when the display is covered by a single source of content.
>>>>
>>>> This patch introduces a new default CRTC property that helps
>>>> hint to the driver when the CRTC composition is suitable for variable
>>>> refresh rate algorithms. Userspace can set this property dynamically
>>>> as the composition changes.
>>>>
>>>> Whether the variable refresh rate algorithms are active will still
>>>> depend on the CRTC being suitable and the connector being capable
>>>> and enabled by the user for variable refresh rate support.
>>>>
>>>> It is worth noting that while the property is atomic it isn't filtered
>>>> from legacy userspace queries. This allows for Xorg userspace drivers
>>>> to implement support in non-atomic setups.
>>>>
>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>>>> ---
>>>> drivers/gpu/drm/drm_atomic_helper.c | 1 +
>>>> drivers/gpu/drm/drm_atomic_uapi.c | 6 ++++++
>>>> drivers/gpu/drm/drm_crtc.c | 2 ++
>>>> drivers/gpu/drm/drm_mode_config.c | 6 ++++++
>>>> include/drm/drm_crtc.h | 13 +++++++++++++
>>>> include/drm/drm_mode_config.h | 8 ++++++++
>>>> 6 files changed, 36 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 3cf1aa132778..b768d397a811 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>>>> state->planes_changed = false;
>>>> state->connectors_changed = false;
>>>> state->color_mgmt_changed = false;
>>>> + state->variable_refresh_changed = false;
>>>
>>> Another bool just to check if one bool changed seems a bit excessive.
>>> Is there a reason you can't directly check if the other bool changed?
>>
>> It's nice for an atomic driver to be able to check if the property has
>> changed to steer control flow.
>>
>> The driver could just check if the old crtc variable refresh value isn't
>> equal to the new one itself, but there's already precedent set to
>> provide flags like these instead.
>
> Generally the changed flags are for more complicated things. Not
> sure we want to start adding them for every little thing. Though
> I suppose active_changed blows a hole in that theory.
>
>>
>> It also lets the driver change it as needed during atomic commits.
>> You'll see many drivers making use of the other flags like
>> connectors_changed, mode_changed, etc.
>>
>>>
>>> Actually I don't understand why this per-crtc thing is being added at
>>> all. You already have the prop on the connector. Why do we want to
>>> make life more difficult by requiring the thing to be set on both the
>>> crtc and connector?
>>
>> It doesn't make much sense without both.
>>
>> The user can globally enable or disable the variable_refresh_enabled on
>> the connector. This is the user's preference.
>>
>> What they don't control is the variable_refresh - the content hint that
>> userspace specifies when the CRTC contents are suitable for enabling
>> variable refresh features (like adaptive sync). This is userspace's
>> preference.
>
> By userspace I guess you mean the compositor/display server. I don't
> really see why the kernel has to be involved like this in a userspace
> policy matter. If the compositor doesn't think vrr is a good idea then
> it could simply choose to disable the prop on the connector even when
> requested by its clients.
The properties are both a kernel and userspace API so I think it's
important to think about the usecase and API usage for both.
In a DRM driver the variable refresh capability will be determined by
connector type and the display that's connected. The driver needs a way
to track this if it's going to be sending any sort of packet over the
connector. The capability property provides a method for the driver to
do this while also exposing this information to the userspace for querying.
The variable_refresh_enabled property exists because not all users will
want this feature enabled. I think it makes sense to place this
alongside the capability property on the connector because of their
relation and because of the ease of access for the end user in existing
userspace stacks - xrandr makes this easy.
The variable_refresh CRTC property exists for a few reasons.
Userspace needs a method via the atomic API to let a DRM driver know
that it wants variable frame timing to be enabled. The connector enable
property certainly satisfies this condition - but I think there are
other to take into consideration here.
Whenever I mentioned variable refresh "features", what I really meant
was operating in one of two modes:
(1) Letting the driver and hardware adjust refresh automatically based
on the flip rate on a CRTC from a single application
(2) Setting a fixed frame duration based on the flip rate on a CRTC from
a single application
In both cases the important thing to note is that they're both dependent
on how often a CRTC is flipping. There's also the "requirement" for
single application in both cases - if there are multiple applications
issuing flips then the hardware can't determine the correct content
rate. The resulting user experience is going to be negative as the
monitor seemingly adjusts to a random rate.
With the existing kernelspace and userspace stacks a DRM driver can't
reasonably understand whether a single application is flipping or not
and what variable refresh "mode" to operate in - these both depend on
what's happening in userspace.
These factors are decided in userspace when interfacing with a DRM CRTC.
The userspace integration patches I've posted alongside this interface
demonstrate this usage - checks are done against a CRTC to see if the
application fully covers the surface and the automatic adjustment mode
is only enabled for when flips are issued for the CRTC originating from
that application.
Determining which connectors use the CRTC can certainly be done and the
property could be changed there, but I don't think this logically
follows from the API usage described above.
The reasoning behind this being a default property on the CRTC is that I
don't think userspace should need to keep track of what's actually
connected using the CRTC. A user can hotplug displays at will or
enable/disable variable refresh support via their display's OSD.
Capabilities can change and this is only something a driver really needs
to concern themselves with - the driver is what sends the control
packets to the hardware.
I probably could have gone into more detail about some of this in the
cover letter itself for these patchsets. These patches were actually a
connector only interface originally but evolved after developing the
actual implementation.
>
>>
>> When both preferences agree, only then will variable refresh features be
>> enabled.
>>
>> The reasoning for the split is because not all content is suitable for
>> variable refresh. Desktop environments, web browsers, etc only typically
>> flip when needed - which will result in display flickering.
>>
>> The userspace integration as part of these patches demonstrates enabling
>> variable_refresh on the CRTC selectively.
>>
>>>
>>>> state->zpos_changed = false;
>>>> state->commit = NULL;
>>>> state->event = NULL;
>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> index 0bb27a24a55c..32a4cf8a01c3 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>>> drm_property_blob_put(mode);
>>>> return ret;
>>>> + } else if (property == config->prop_variable_refresh) {
>>>> + if (state->variable_refresh != val)
>>>> + state->variable_refresh_changed = true;
>>>> + state->variable_refresh = val;
>>>> } else if (property == config->degamma_lut_property) {
>>>> ret = drm_atomic_replace_property_blob_from_id(dev,
>>>> &state->degamma_lut,
>>>> @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>>>> *val = state->active;
>>>> else if (property == config->prop_mode_id)
>>>> *val = (state->mode_blob) ? state->mode_blob->base.id : 0;
>>>> + else if (property == config->prop_variable_refresh)
>>>> + *val = state->variable_refresh;
>>>> else if (property == config->degamma_lut_property)
>>>> *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
>>>> else if (property == config->ctm_property)
>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>> index 5f488aa80bcd..ca33d6fb90ac 100644
>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>>>> drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>>>> drm_object_attach_property(&crtc->base,
>>>> config->prop_out_fence_ptr, 0);
>>>> + drm_object_attach_property(&crtc->base,
>>>> + config->prop_variable_refresh, 0);
>>>> }
>>>>
>>>> return 0;
>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>>>> index ee80788f2c40..1287c161d65d 100644
>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>> @@ -310,6 +310,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>>>> return -ENOMEM;
>>>> dev->mode_config.prop_mode_id = prop;
>>>>
>>>> + prop = drm_property_create_bool(dev, 0,
>>>> + "VARIABLE_REFRESH");
>>>> + if (!prop)
>>>> + return -ENOMEM;
>>>> + dev->mode_config.prop_variable_refresh = prop;
>>>> +
>>>> prop = drm_property_create(dev,
>>>> DRM_MODE_PROP_BLOB,
>>>> "DEGAMMA_LUT", 0);
>>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>>> index b21437bc95bf..32b77f18ce6d 100644
>>>> --- a/include/drm/drm_crtc.h
>>>> +++ b/include/drm/drm_crtc.h
>>>> @@ -168,6 +168,12 @@ struct drm_crtc_state {
>>>> * drivers to steer the atomic commit control flow.
>>>> */
>>>> bool color_mgmt_changed : 1;
>>>> + /**
>>>> + * @variable_refresh_changed: Variable refresh support has changed
>>>> + * on the CRTC. Used by the atomic helpers and drivers to steer the
>>>> + * atomic commit control flow.
>>>> + */
>>>> + bool variable_refresh_changed : 1;
>>>>
>>>> /**
>>>> * @no_vblank:
>>>> @@ -290,6 +296,13 @@ struct drm_crtc_state {
>>>> */
>>>> u32 pageflip_flags;
>>>>
>>>> + /**
>>>> + * @variable_refresh:
>>>> + *
>>>> + * Indicates whether the CRTC is suitable for variable refresh rate.
>>>> + */
>>>> + bool variable_refresh;
>>>> +
>>>> /**
>>>> * @event:
>>>> *
>>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>>>> index 928e4172a0bb..1290191cd96e 100644
>>>> --- a/include/drm/drm_mode_config.h
>>>> +++ b/include/drm/drm_mode_config.h
>>>> @@ -639,6 +639,14 @@ struct drm_mode_config {
>>>> * connectors must be of and active must be set to disabled, too.
>>>> */
>>>> struct drm_property *prop_mode_id;
>>>> + /**
>>>> + * @prop_variable_refresh: Default atomic CRTC property to indicate
>>>> + * whether the CRTC is suitable for variable refresh rate support.
>>>> + *
>>>> + * This is only an indication of support, not whether variable
>>>> + * refresh is active on the CRTC.
>>>> + */
>>>> + struct drm_property *prop_variable_refresh;
>>>>
>>>> /**
>>>> * @dvi_i_subconnector_property: Optional DVI-I property to
>>>> --
>>>> 2.19.0
>>>
>>
>> Nicholas Kazlauskas
>
Nicholas Kazlauskas
More information about the amd-gfx
mailing list