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

[Ville Syrjälä]

On Tue, Sep 25, 2018 at 04:35:59PM +0200, Michel Dänzer wrote:
> On 2018-09-25 4:04 p.m., Ville Syrjälä wrote:
> > 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?
> > 
> 
> Sure, that's not the problem, which is that if the Xorg driver doesn't
> actively hide the property from clients (e.g. because it's a currently
> released version), we get the issue I described above.

That sounds more like what client caps were meant for. Or maybe
drop the connector vrr enable prop and just go with the crtc one?

> 
> Also, if the Xorg driver were to hide the connector property from
> clients, there's no way for the user to completely disable variable
> refresh for a display (unless the Xorg driver exposes another fake
> property for that, which seems a bit silly).
> 
> 
> -- 
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer

-- 
Ville Syrjälä
Intel