| Message ID | 20181012164458.12864-3-nicholas.kazlauskas@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | A DRM API for adaptive sync and variable refresh rate support | expand |
Am 12.10.2018 um 18:44 schrieb Nicholas Kazlauskas: > This patch introduces the 'vrr_enabled' CRTC property to allow > dynamic control over variable refresh rate support for a CRTC. > > This property should be treated like a content hint to the driver - > if the hardware or driver is not capable of driving variable refresh > timings then this is not considered an error. > > Capability for variable refresh rate support should be determined > by querying the vrr_capable drm connector property. > > It is worth noting that while the property is intended for atomic use > it isn't filtered from legacy userspace queries. This allows for Xorg > userspace drivers to implement support. I'm honestly still not convinced that a per CRTC property is actually the right approach. What we should rather do instead is to implement a target timestamp for the page flip. Regards, Christian. > > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> > --- > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ > drivers/gpu/drm/drm_crtc.c | 2 ++ > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > include/drm/drm_crtc.h | 9 +++++++++ > include/drm/drm_mode_config.h | 5 +++++ > 5 files changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index d5b7f315098c..eec396a57b88 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -433,6 +433,8 @@ 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_vrr_enabled) { > + state->vrr_enabled = val; > } else if (property == config->degamma_lut_property) { > ret = drm_atomic_replace_property_blob_from_id(dev, > &state->degamma_lut, > @@ -491,6 +493,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_vrr_enabled) > + *val = state->vrr_enabled; > 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..e4eb2c897ff4 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_vrr_enabled, 0); > } > > return 0; > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index ee80788f2c40..5670c67f28d4 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, > + "VRR_ENABLED"); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.prop_vrr_enabled = 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..39c3900aab3c 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -290,6 +290,15 @@ struct drm_crtc_state { > */ > u32 pageflip_flags; > > + /** > + * @vrr_enabled: > + * > + * Indicates if variable refresh rate should be enabled for the CRTC. > + * Support for the requested vrr state will depend on driver and > + * hardware capabiltiy - lacking support is not treated as failure. > + */ > + bool vrr_enabled; > + > /** > * @event: > * > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 928e4172a0bb..49f2fcfdb5fc 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -639,6 +639,11 @@ struct drm_mode_config { > * connectors must be of and active must be set to disabled, too. > */ > struct drm_property *prop_mode_id; > + /** > + * @prop_vrr_enabled: Default atomic CRTC property to indicate > + * whether variable refresh rate should be enabled on the CRTC. > + */ > + struct drm_property *prop_vrr_enabled; > > /** > * @dvi_i_subconnector_property: Optional DVI-I property to
On 2018-10-13 7:38 p.m., Christian König wrote: > Am 12.10.2018 um 18:44 schrieb Nicholas Kazlauskas: >> This patch introduces the 'vrr_enabled' CRTC property to allow >> dynamic control over variable refresh rate support for a CRTC. >> >> This property should be treated like a content hint to the driver - >> if the hardware or driver is not capable of driving variable refresh >> timings then this is not considered an error. >> >> Capability for variable refresh rate support should be determined >> by querying the vrr_capable drm connector property. >> >> It is worth noting that while the property is intended for atomic use >> it isn't filtered from legacy userspace queries. This allows for Xorg >> userspace drivers to implement support. > > I'm honestly still not convinced that a per CRTC property is actually > the right approach. > > What we should rather do instead is to implement a target timestamp for > the page flip. You'd have to be more specific about how the latter could completely replace the former. I don't see how it could.
Am 15.10.2018 um 11:40 schrieb Michel Dänzer: > On 2018-10-13 7:38 p.m., Christian König wrote: >> Am 12.10.2018 um 18:44 schrieb Nicholas Kazlauskas: >>> This patch introduces the 'vrr_enabled' CRTC property to allow >>> dynamic control over variable refresh rate support for a CRTC. >>> >>> This property should be treated like a content hint to the driver - >>> if the hardware or driver is not capable of driving variable refresh >>> timings then this is not considered an error. >>> >>> Capability for variable refresh rate support should be determined >>> by querying the vrr_capable drm connector property. >>> >>> It is worth noting that while the property is intended for atomic use >>> it isn't filtered from legacy userspace queries. This allows for Xorg >>> userspace drivers to implement support. >> I'm honestly still not convinced that a per CRTC property is actually >> the right approach. >> >> What we should rather do instead is to implement a target timestamp for >> the page flip. > You'd have to be more specific about how the latter could completely > replace the former. I don't see how it could. Each flip request send by an application gets a timestamp when the flip should be displayed. If I'm not completely mistaken we already have something like that in both DRI2 as well as DRI3. So as far as I can see we only need to add an extra flag that those information are trust worth in the context of VRR as well. If we don't set this flag we always get the always working fallback behavior, e.g. VRR is disabled and we have a fixed refresh rate. If we set this flag and the timestamp is in the past we get the VRR behavior to display the next frame as soon as possible. If we set this flag and specific a specific timestamp then we get the VRR behavior to display the frame as close as possible to the specified timestamp. Regards, Christian.
On 2018-10-15 11:47 a.m., Christian König wrote: > Am 15.10.2018 um 11:40 schrieb Michel Dänzer: >> On 2018-10-13 7:38 p.m., Christian König wrote: >>> Am 12.10.2018 um 18:44 schrieb Nicholas Kazlauskas: >>>> This patch introduces the 'vrr_enabled' CRTC property to allow >>>> dynamic control over variable refresh rate support for a CRTC. >>>> >>>> This property should be treated like a content hint to the driver - >>>> if the hardware or driver is not capable of driving variable refresh >>>> timings then this is not considered an error. >>>> >>>> Capability for variable refresh rate support should be determined >>>> by querying the vrr_capable drm connector property. >>>> >>>> It is worth noting that while the property is intended for atomic use >>>> it isn't filtered from legacy userspace queries. This allows for Xorg >>>> userspace drivers to implement support. >>> I'm honestly still not convinced that a per CRTC property is actually >>> the right approach. >>> >>> What we should rather do instead is to implement a target timestamp for >>> the page flip. >> You'd have to be more specific about how the latter could completely >> replace the former. I don't see how it could. > > Each flip request send by an application gets a timestamp when the flip > should be displayed. > > If I'm not completely mistaken we already have something like that in > both DRI2 as well as DRI3. Certainly not DRI2, but for now we're not enabling VRR with that anyway. While the X11 Present extension specifies PresentOptionUST for this, support for it isn't implemented yet in xserver (as in setting the option has no effect, the X server interprets the target value as an MSC anyway). So this couldn't work before the next major release of xserver, which based on recent history could be at least about one year away. In contrast, the CRTC property based solution for the gaming use-case can work even with older xserver versions. > So as far as I can see we only need to add an extra flag that those > information are trust worth in the context of VRR as well. > > If we don't set this flag we always get the always working fallback > behavior, e.g. VRR is disabled and we have a fixed refresh rate. > > If we set this flag and the timestamp is in the past we get the VRR > behavior to display the next frame as soon as possible. > > If we set this flag and specific a specific timestamp then we get the > VRR behavior to display the frame as close as possible to the specified > timestamp. Apart from the above, another issue is that this would give direct control to the client on whether or not VRR should be used. But we want to allow the user to disable VRR even if a client wants to use it, via an RandR output property. This requires that the Xorg driver can control whether or not VRR can actually be used, via the CRTC property added by this patch.
Am 15.10.2018 um 12:06 schrieb Michel Dänzer: > [SNIP] > Apart from the above, another issue is that this would give direct > control to the client on whether or not VRR should be used. But we want > to allow the user to disable VRR even if a client wants to use it, via > an RandR output property. This requires that the Xorg driver can control > whether or not VRR can actually be used, via the CRTC property added by > this patch. Oh, that is a really good argument! No objection from my side to this then, Christian.
On 2018-10-12 12:44 p.m., Nicholas Kazlauskas wrote: > This patch introduces the 'vrr_enabled' CRTC property to allow > dynamic control over variable refresh rate support for a CRTC. > > This property should be treated like a content hint to the driver - > if the hardware or driver is not capable of driving variable refresh > timings then this is not considered an error. > > Capability for variable refresh rate support should be determined > by querying the vrr_capable drm connector property. > > It is worth noting that while the property is intended for atomic use > it isn't filtered from legacy userspace queries. This allows for Xorg > userspace drivers to implement support. > > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> Reviewed-by: Harry Wentland <harry.wentland@amd.com> Harry > --- > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ > drivers/gpu/drm/drm_crtc.c | 2 ++ > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > include/drm/drm_crtc.h | 9 +++++++++ > include/drm/drm_mode_config.h | 5 +++++ > 5 files changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index d5b7f315098c..eec396a57b88 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -433,6 +433,8 @@ 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_vrr_enabled) { > + state->vrr_enabled = val; > } else if (property == config->degamma_lut_property) { > ret = drm_atomic_replace_property_blob_from_id(dev, > &state->degamma_lut, > @@ -491,6 +493,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_vrr_enabled) > + *val = state->vrr_enabled; > 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..e4eb2c897ff4 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_vrr_enabled, 0); > } > > return 0; > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index ee80788f2c40..5670c67f28d4 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, > + "VRR_ENABLED"); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.prop_vrr_enabled = 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..39c3900aab3c 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -290,6 +290,15 @@ struct drm_crtc_state { > */ > u32 pageflip_flags; > > + /** > + * @vrr_enabled: > + * > + * Indicates if variable refresh rate should be enabled for the CRTC. > + * Support for the requested vrr state will depend on driver and > + * hardware capabiltiy - lacking support is not treated as failure. > + */ > + bool vrr_enabled; > + > /** > * @event: > * > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 928e4172a0bb..49f2fcfdb5fc 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -639,6 +639,11 @@ struct drm_mode_config { > * connectors must be of and active must be set to disabled, too. > */ > struct drm_property *prop_mode_id; > + /** > + * @prop_vrr_enabled: Default atomic CRTC property to indicate > + * whether variable refresh rate should be enabled on the CRTC. > + */ > + struct drm_property *prop_vrr_enabled; > > /** > * @dvi_i_subconnector_property: Optional DVI-I property to >
On Mon, 15 Oct 2018 12:06:52 +0200 Michel Dänzer <michel@daenzer.net> wrote: > On 2018-10-15 11:47 a.m., Christian König wrote: > > Am 15.10.2018 um 11:40 schrieb Michel Dänzer: > >> On 2018-10-13 7:38 p.m., Christian König wrote: > >>> Am 12.10.2018 um 18:44 schrieb Nicholas Kazlauskas: > >>>> This patch introduces the 'vrr_enabled' CRTC property to allow > >>>> dynamic control over variable refresh rate support for a CRTC. > >>>> > >>>> This property should be treated like a content hint to the driver - > >>>> if the hardware or driver is not capable of driving variable refresh > >>>> timings then this is not considered an error. > >>>> > >>>> Capability for variable refresh rate support should be determined > >>>> by querying the vrr_capable drm connector property. > >>>> > >>>> It is worth noting that while the property is intended for atomic use > >>>> it isn't filtered from legacy userspace queries. This allows for Xorg > >>>> userspace drivers to implement support. > >>> I'm honestly still not convinced that a per CRTC property is actually > >>> the right approach. > >>> > >>> What we should rather do instead is to implement a target timestamp for > >>> the page flip. > >> You'd have to be more specific about how the latter could completely > >> replace the former. I don't see how it could. > > > > Each flip request send by an application gets a timestamp when the flip > > should be displayed. > > > > If I'm not completely mistaken we already have something like that in > > both DRI2 as well as DRI3. > > Certainly not DRI2, but for now we're not enabling VRR with that anyway. > > While the X11 Present extension specifies PresentOptionUST for this, > support for it isn't implemented yet in xserver (as in setting the > option has no effect, the X server interprets the target value as an MSC > anyway). > > So this couldn't work before the next major release of xserver, which > based on recent history could be at least about one year away. Hi > In contrast, the CRTC property based solution for the gaming use-case > can work even with older xserver versions. This is probably the heaviest reason. Coming up with a KMS UABI for target timestamps could get complicated. Do you need a flip queue deeper than one? Do you need to be able to cancel flips? > > So as far as I can see we only need to add an extra flag that those > > information are trust worth in the context of VRR as well. > > > > If we don't set this flag we always get the always working fallback > > behavior, e.g. VRR is disabled and we have a fixed refresh rate. > > > > If we set this flag and the timestamp is in the past we get the VRR > > behavior to display the next frame as soon as possible. > > > > If we set this flag and specific a specific timestamp then we get the > > VRR behavior to display the frame as close as possible to the specified > > timestamp. > > Apart from the above, another issue is that this would give direct > control to the client on whether or not VRR should be used. But we want > to allow the user to disable VRR even if a client wants to use it, via > an RandR output property. This requires that the Xorg driver can control > whether or not VRR can actually be used, via the CRTC property added by > this patch. It would not imply direct control to clients. The target timestamps go through the X server, the X server can mangle them or remove them before calling KMS any way it wants. The X server can invent a RandR property to disable/enable VRR. One would need a video-DDX update the very minimum to start passing the timestamps to the kernel, so there is no way VRR would be enabled unwanted. Thanks, pq
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index d5b7f315098c..eec396a57b88 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -433,6 +433,8 @@ 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_vrr_enabled) { + state->vrr_enabled = val; } else if (property == config->degamma_lut_property) { ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, @@ -491,6 +493,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_vrr_enabled) + *val = state->vrr_enabled; 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..e4eb2c897ff4 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_vrr_enabled, 0); } return 0; diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index ee80788f2c40..5670c67f28d4 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, + "VRR_ENABLED"); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_vrr_enabled = 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..39c3900aab3c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -290,6 +290,15 @@ struct drm_crtc_state { */ u32 pageflip_flags; + /** + * @vrr_enabled: + * + * Indicates if variable refresh rate should be enabled for the CRTC. + * Support for the requested vrr state will depend on driver and + * hardware capabiltiy - lacking support is not treated as failure. + */ + bool vrr_enabled; + /** * @event: * diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 928e4172a0bb..49f2fcfdb5fc 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -639,6 +639,11 @@ struct drm_mode_config { * connectors must be of and active must be set to disabled, too. */ struct drm_property *prop_mode_id; + /** + * @prop_vrr_enabled: Default atomic CRTC property to indicate + * whether variable refresh rate should be enabled on the CRTC. + */ + struct drm_property *prop_vrr_enabled; /** * @dvi_i_subconnector_property: Optional DVI-I property to
This patch introduces the 'vrr_enabled' CRTC property to allow dynamic control over variable refresh rate support for a CRTC. This property should be treated like a content hint to the driver - if the hardware or driver is not capable of driving variable refresh timings then this is not considered an error. Capability for variable refresh rate support should be determined by querying the vrr_capable drm connector property. It is worth noting that while the property is intended for atomic use it isn't filtered from legacy userspace queries. This allows for Xorg userspace drivers to implement support. Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> --- drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ drivers/gpu/drm/drm_crtc.c | 2 ++ drivers/gpu/drm/drm_mode_config.c | 6 ++++++ include/drm/drm_crtc.h | 9 +++++++++ include/drm/drm_mode_config.h | 5 +++++ 5 files changed, 26 insertions(+)