Re: [PATCH 0/6] Enabling DRRS support in the kernel

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 28 Nov 2013 14:14:09 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> > 	[Vandana/Pradeep]	This patch series is for seamless DRRS
> > 	support and has to be a separate path from mode set as we just
> > 	toggle the PIPE_CONF_RR_SWTICH for seamless RR transition without
> > 	flicker. Complete modeset using setCrtc will result in partial
> > 	blanking or flicker. We are not implementing fast boot as part of
> > 	this series.  Most eDP panels today which support multiple refresh
> > 	rates, support seamless DRRS. This means that hardware capability
> > 	is made use of by toggling a bit in pipe config to switch between
> > 	refresh rates on the fly. The patch series is aimed to take
> > 	advantage of this capability to switch refresh rates based on
> > 	idleness or video playback requirement. Hence, we chose the
> > 	connector attribute/ set property path.  Also in order to support
> > 	the Media refresh rates of 50/48 we thought it incorrect to
> > 	populate those modes in probed_mode list as those values are not
> > 	from the EDID. So in order for User space to know the media
> > 	refresh rates supported we have exposed a set_property interface
> > 	which makes User space aware of these media refresh rates. Also we
> > 	do not intend to do a complete mode set on media use cases in
> > 	which case we think using setCrtc is not needed. Using setCrtc
> > 	will invariably result in intermittant blanking or flicker which
> > 	cannot be avoided, and gives undesirable effect during playback.
> > 	Seamless DRRS doesn't need a complete modeset and hence no flicker
> > 	or temporary blanking. Changing the toggle RR bit in PIPE_CONF is
> > 	faster. Also since BDW has only single M/N/TU register the current
> > 	code takes care of it easily. Kindly let us know your opinion.
> > 	Since Seamless DRRS for eDP is separate path from modeset we can
> > 	keep the connector property attribute as it is.
> 
> First the technical stuff:
> 
> - Adding the additional modes to the mode list is imo totally ok and as
>   the design intents. We are already adding other modes from the vbt here
>   if those are available. The only restriction is that the kernel may not
>   add modes which don't work. But as long as the vbt tells us that 48Hz
>   will work we can (and imo should) use it.
> 
> - Second changing the frequency changes the refresh rate. Atm both our
>   userspace and the kernel support code assume that this can only be done
>   through a SetCrtc call. So adjusting the dotclock is actually the right
>   interface and we don't need any additional property.
> 
> Now the more political stuff:
> 
> - We have the long-term goal of solid fastboot support. We've the designed
>   we've picked that actually means 95% effort to get flicker-free SetCrtc
>   going and 5% actual code for fastboot. We're not there yet, there's only
>   a preview available which is disabled by default and uses a few hacks.
>   But generally all features that are relevant need to use the new
>   infrastructure and help move things forward.
> 
> - Upstream has a time horizon of 5-10 years for the kernel/userspace
>   interface. Which means quick hacks are not allowed. And in my opinion
>   your property is a quick hack, which nicely simplified the
>   implementation.
> 
> So in short I know that my request extends the scope of your patches. But
> upstream also imposes differing constraints than a product tree.

It should be possible to still merge the automatic, in-kernel only DRRS
bits though, right?  Then work on making dotclock/refresh changes from
mode sets flicker free (shouldn't be a massive amount of work).

-- 
Jesse Barnes, Intel Open Source Technology Center