[PATCH v2 2/3] drm: Add variable refresh property to DRM CRTC

[Ville Syrjälä]

On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote:
> On 2018-09-24 10:26 p.m., 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:
> >>>
> >>> 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.
> 
> Actually rather the application, see the corresponding Mesa and
> xf86-video-amdgpu patches.
> 
> 
> > 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.
> 
> Connector properties are exposed directly to X11 clients as RandR output
> properties. With only the connector property, the user running e.g.
> 
>  xrandr --output <name> --set variable_refresh_enabled 1
> 
> would result in variable refresh being enabled regardless of whether a
> variable refresh compatible client is currently using page flipping,
> which can result in flickering or getting stuck at the minimum refresh rate.

in ddx:

configure_vrr()
{
	kms_vrr = client_vrr && is_vrr_a_good_idea();
	kms_setprop(kms_vrr);
}

set_prop()
{
	if (is_vrr_prop)
		configure_vrr();
	else
		kms_setprop();
}

other_stuff()
{
	/* some stuff */
	...

	if (vrr_related_stuff_changed)
		configure_vrr();
}

or something along those lines?

-- 
Ville Syrjälä
Intel