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

[Kannan, Vandana]

Hi Daniel,

	Thank you for your inputs. Please read our response below and let us know your opinion.

 - 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. 

 - 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.
 
   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.
 
   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.
 
 - 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.

Thanks,
Vandana

 -----Original Message-----
 From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
 Sent: Tuesday, November 26, 2013 11:57 PM
 To: Kannan, Vandana
 Cc: intel-gfx at lists.freedesktop.org
 Subject: Re: [Intel-gfx] [PATCH 0/6] Enabling DRRS support in the kernel
 
 On Tue, Nov 19, 2013 at 11:36:58AM +0530, Vandana Kannan wrote:
 > Dynamic Refresh Rate Switching (DRRS) is a power conservation feature which
 > enables switching between low and high refresh rates based on the usage
 > scenario. This feature is applicable for internal eDP panel. Indication that
 > the panel can support DRRS is given by the panel EDID, which would list
 > multiple refresh rates for one resolution.
 > DRRS is of 2 types -
 > Static DRRS involves execution of the entire mode set sequence to switch
 > between refresh rate.
 > seamless DRRS involves refresh rate switching during runtime without any
 > blanking effect/mode set.
 > The vendor fills in a VBT field indicating static/seamless DRRS based on the
 > panel spec. This information is needed to enable seamless DRRS in kernel.
 > The patch series supports idleness detection in display i915 driver and switch
 > to low refresh rate. It also provides set_property API for user space to
 > request for different refresh rates for active use cases like video playback
 > at 48/50 Hz.
 >
 >
 > Pradeep Bhat (5):
 >   drm/i915: Adding VBT fields to support eDP DRRS feature
 >   drm/i915: Parse EDID probed modes for DRRS support
 >   drm/i915: Add support for DRRS set property to switch RR
 >   drm/i915: Support to read DMRRS field from VBT structure
 >   drm/i915: Adding support for DMRRS for media playback
 >
 > Vandana Kannan (1):
 >   drm/i915: Idleness detection for DRRS
 
 My apologies for delaying my high-level maintainer review for so long - there's been a bit a
 firedrill around here so it took a while to write it all down.
 
 Overall a nice patch series, but I think we need to shuffle a few things around to better
 align with some of the longer-term driver architecture reworks and goals:
 
 - 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.
 
 - 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.
 
   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.
 
   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).
 
 - 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.
 
 Cheers, Daniel
 --
 Daniel Vetter
 Software Engineer, Intel Corporation
 +41 (0) 79 365 57 48 - http://blog.ffwll.ch