[Intel-gfx] [PATCH 0/6] Enabling DRRS support in the kernel

[Daniel Vetter]

On Thu, Nov 28, 2013 at 10:22:59AM +0000, Kannan, Vandana wrote:
> Hi Daniel,
> 
> 	Thank you for your inputs. Please read our response below and let us know your opinion.

Can you please switch to a mail client that does proper quoting? It makes
reading your response a bit hard - I've fixed it up here quickly.


> 
>> - You add another copypaste of the code to deduce the reduced clock from
>>   the edid to intel_dp.c. Imo it's better to add that to intel_panel.c to
>>   struct intel_panel - we already track the panel fixed mode in there, so
>>   this would naturally fit.

>[Pradeep/Vandana] We will work to move the piece of code to deduce
>reduced clock from EDID to intel_panel.c. However, we will be
>making this change only for eDP at this point in time, and will
>work to rearrange the same for LVDS a little later. 

On my quick readthrough of your code it looked like it's identical code,
so two patches (first to extract the code from lvds, 2nd to use in in the
dp code) look like the sane choice. Just moving the place around where you
add the new copy doesn't help that much.

>> - Imo we should track the reduced clock in the pipe config and also
>>   cross-check it in the state checker. That will lead to a bit of fun on
>>   bdw, so we need to special case the checker there so that it only checks
>>   that the clock matches either of the possible clocks.
>> 
>>   For this we need a bool downclock_available in struct intel_crtc_config
>>   and a 2nd set of m_n values, both set in the compute_config function for
>>   DP outputs.
> 	[Vandana/Pradeep]	Computing the 2nd set of M_N values in
> 	compute_config() for the  lowest refresh rate available can done.
> 	But in case of the use case of media playback at 50/48 Hz, which
> 	we have kept in mind during design/implementation, would mean that
> 	M_N values have to be computed initially and kept for all
> 	different refresh rates possible. The current implementation gives
> 	the flexibility of computing M_N as and when required by user
> 	space. If it is ok to keep an array of pre computed M_N values for
> 	different Refresh rates computed at intel_dp_compute_config then
> 	we could do this. Kindly let us know your opinion.

Imo the pipe config should only contain the low/normal frequencies for
automatic switching. After all the switching should be transparent to
userspace since we're never in the lowfreq mode when there's activity.

>>   For consistency it'd be nice to at least move the downclock_available
>>   logic for lvds also over to that. But since we first need to clean up
>>   the dpll handling to make lvds downclocking sane that's imo not really a
>>   priority at all.
> 	[Vandana/Pradeep]As mentioned above we will consider this on lower
> 	priority and not as part of this patch series.

Yeah, like I've said this is out of scope. Switching the downclock_avail
bool though should simplify things a bit, so I'd include that.

>>   The entire point behind all this pipe state tracking is to make sure we
>>   don't miss anything when fastbooting.
>> 
>> - The connector attribute is imo the wrong interface - userspace already
>>   supplies a mode attribute with dotclock to SetCrtc. Imo we simply need
>>   to fix up our fixed_mode logic (preferrably as a neat helper in
>>   intel_panel.c) to select the right one of the availabel downclocks, even
>>   when upscaling.
>> 
>>   The downside for now is that this will result in flickering. But we need
>>   a real flicker-free fast-update-path in our modeset code anyway to make
>>   fastboot happen for real. And a few other cool things. I'll increase
>>   the pain a bit in the hope that this moves forward again, so no quick
>>   hack please (even if it's very simple to do in the case of drrs).
> 	[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.

>> - Finally a quick testcase which iterates through all the downclock modes
>>   in kms_flip - together with the cross-checking and all the vblank
>>   timing tests we already have that should give us solid test coverage. To
>>   keep test runtimes reasonable I think just a pageflipping test with time
>>   checking is more than enough.
>	[Vandana/Pradeep]We will look into this.

Good. Decent test case coverage with i-g-t tests is a requirement for
getting new features in. I've recently documented the reasons and
expectations for that:

http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch