Re: [PATCH v9 3/6] drm/i915: Add support for DRRS to switch RR

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 11, 2014 at 02:48:53PM +0530, Vandana Kannan wrote:
> On Apr-10-2014 2:28 PM, Daniel Vetter wrote:
> > On Thu, Apr 10, 2014 at 11:43:15AM +0300, Jani Nikula wrote:
> >>
> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >>
> >>
> >> On Sat, 05 Apr 2014, Vandana Kannan <vandana.kannan@intel.com> wrote:
> >>> From: Pradeep Bhat <pradeep.bhat@intel.com>
> >>>
> >>> This patch computes and stored 2nd M/N/TU for switching to different
> >>> refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
> >>> between alternate refresh rates programmed in 2nd M/N/TU registers.
> >>>
> >>> v2: Daniel's review comments
> >>> Computing M2/N2 in compute_config and storing it in crtc_config
> >>>
> >>> v3: Modified reference to edp_downclock and edp_downclock_avail based on the
> >>> changes made to move them from dev_private to intel_panel.
> >>>
> >>> v4: Modified references to is_drrs_supported based on the changes made to
> >>> rename it to drrs_support.
> >>>
> >>> v5: Jani's review comments
> >>> Removed superfluous return statements. Changed support for Gen 7 and above.
> >>> Corrected indentation. Re-structured the code which finds crtc and connector
> >>> from encoder. Changed some logs to be less verbose.
> >>>
> >>> v6: Modifying i915_drrs to include only intel connector as intel_dp can be
> >>> derived from intel connector when required.
> >>>
> >>> v7: As per internal review comments, acquiring mutex just before accessing
> >>> drrs RR. As per Chris's review comments, added documentation about the use
> >>> of locking in the function.
> >>>
> >>> v8: Incorporated Jani's review comments.
> >>> Removed reference to edp_downclock.
> >>>
> >>> v9: Jani's review comments. Modified comment in set_drrs. Changed index to
> >>> type edp_drrs_refresh_rate_type. Check if PSR is enabled before setting
> >>> registers fo DRRS.
> >>>
> >>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> >>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> >>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > 
> > Queued for -next, thanks for the patch. One thing that's missing though is
> > the state readout and cross-check support for this new bit of crtc->config
> > data. We need to add that before putting this to real use.
> > -Daniel
> > 
> Hi Daniel,
> 
> Could you please elaborate on your input above - on the missing code and
> cross-checking for support part?

If you add new state to crtc->config then you need to add the relevan
readout code for that state (see the code in check_crtc_state) and ofc
also add it to intel_pipe_config_compare.

This is a debug feature of our driver to make sure we never lose track of
things and thus far has been extremely helpful in catching issues early.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch