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

[Daniel Vetter <daniel@ffwll.ch>]

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