From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v9 3/6] drm/i915: Add support for DRRS to switch RR Date: Fri, 11 Apr 2014 11:26:41 +0200 Message-ID: <20140411092641.GD9262@phenom.ffwll.local> References: <1396587501-7218-1-git-send-email-vandana.kannan@intel.com> <1396680208-19390-1-git-send-email-vandana.kannan@intel.com> <877g6xy3q4.fsf@intel.com> <20140410085854.GQ9262@phenom.ffwll.local> <5347B37D.4000703@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f43.google.com (mail-ee0-f43.google.com [74.125.83.43]) by gabe.freedesktop.org (Postfix) with ESMTP id 06F346ED3B for ; Fri, 11 Apr 2014 02:26:44 -0700 (PDT) Received: by mail-ee0-f43.google.com with SMTP id e53so3875017eek.16 for ; Fri, 11 Apr 2014 02:26:44 -0700 (PDT) Content-Disposition: inline In-Reply-To: <5347B37D.4000703@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Vandana Kannan Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org 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 > >> > >> > >> On Sat, 05 Apr 2014, Vandana Kannan wrote: > >>> From: Pradeep Bhat > >>> > >>> 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 > >>> Signed-off-by: Vandana Kannan > >>> Cc: Jani Nikula > > > > 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