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

[Daniel Vetter]

On Tue, Sep 25, 2018 at 06:28:17PM +0300, Ville Syrjälä wrote:
> 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).

Even easier solution: Only add the enable knob on the CRTC. The CRTC is
the place where we set the timing information (through the mode), not the
connector. Splitting timing information across crtc and connector doesn't
make sense to me, especially since you can have (in theory at least)
multiple connectors.

And once it's at the CRTC, you don't have the forward-compat issue
anymore, it's up to the DDX to decide how/where exactly to expose this
knob. Simplest (because no one loves to extend xproto) seems indeed to be
to add it as an xrandr connector prop. But just because that's the
reasonable solution at the xrandr level doesn't mean it's a great at idea
at the kms api level.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch