From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 2/5] drm/i915: Parse EDID probed modes for DRRS support Date: Wed, 18 Dec 2013 09:06:57 +0000 Message-ID: <20131218090657.GZ22448@nuc-i3427.alporthouse.com> References: <1387258107-19232-1-git-send-email-vandana.kannan@intel.com> <1387258107-19232-3-git-send-email-vandana.kannan@intel.com> <20131217122842.GH22448@nuc-i3427.alporthouse.com> <52B158A9.8060007@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118]) by gabe.freedesktop.org (Postfix) with ESMTP id CB33CFA663 for ; Wed, 18 Dec 2013 01:07:01 -0800 (PST) Content-Disposition: inline In-Reply-To: <52B158A9.8060007@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Vandana Kannan Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Wed, Dec 18, 2013 at 01:41:21PM +0530, Vandana Kannan wrote: > On Dec-17-2013 5:58 PM, Chris Wilson wrote: > > On Tue, Dec 17, 2013 at 10:58:24AM +0530, Vandana Kannan wrote: > >> From: Pradeep Bhat > >> > >> This patch and finds out the lowest refresh rate supported for the resolution > >> same as the fixed_mode, based on the implementaion find_panel_downclock. > >> It also checks the VBT fields to see if panel supports seamless DRRS or not. > >> Based on above data it marks whether eDP panel supports seamless DRRS or not. > >> This information is needed for supporting seamless DRRS switch for > >> certain power saving usecases. This patch is tested by enabling the DRM logs > >> and user should see whether Seamless DRRS is supported or not. > >> > >> Signed-off-by: Pradeep Bhat > >> Signed-off-by: Vandana Kannan > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ > >> drivers/gpu/drm/i915/intel_dp.c | 47 ++++++++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++++ > >> 3 files changed, 78 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index 02e11dc..c9bca16 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -1462,8 +1462,10 @@ typedef struct drm_i915_private { > >> /* Reclocking support */ > >> bool render_reclock_avail; > >> bool lvds_downclock_avail; > >> + bool edp_downclock_avail; > >> /* indicates the reduced downclock for LVDS*/ > >> int lvds_downclock; > >> + int edp_downclock; > >> u16 orig_clock; > > > > Do any machines have both edp and lvds? Shouldn't this be a part of the > > panel state? > > > If there is a machine having both edp and lvds, then edp takes higher > priority. edp_downclock_avail and edp_downclock were added here > following the existing code having lvds_downclock_avail and > lvds_downclock here. If required, edp_downclock_avail and edp_downclock > can be moved to intel_panel structure. Kindly let us know. And we can consolidate both into intel_panel. > >> > >> +/** > >> + * This enum is used to indicate the DRRS support type. > >> + * The values of the enum map 1-to-1 with the values from VBT. > >> + */ > >> +enum edp_panel_type { > >> + DRRS_NOT_SUPPORTED = -1, > >> + STATIC_DRRS_SUPPORT = 0, > >> + SEAMLESS_DRRS_SUPPORT = 2 > >> +}; > >> +/** > >> + * HIGH_RR is the highest eDP panel refresh rate read from EDID > >> + * LOW_RR is the lowest eDP panel refresh rate found from EDID > >> + * parsing for same resolution. > >> + */ > >> +enum edp_drrs_refresh_rate_type { > >> + DRRS_HIGH_RR, > >> + DRRS_LOW_RR, > >> + DRRS_MAX_RR, /* RR count */ > >> +}; > >> +/** > >> + * The drrs_info struct will represent the DRRS feature for eDP > >> + * panel. > >> + */ > >> +struct drrs_info { > >> + int is_drrs_supported; > >> + int drrs_refresh_rate_type; > > > > So what was the point of the enums again? Are you purposely trying to > > disable gcc and sparse's type-safety? > > > The enum edp_panel_type is required to check DRRS capability of the > panel before performing any enabling. We will look into an > implementation which can do without edp_drrs_refresh_rate_type. All I am saying is have enum, use enum. It improves type safety. -Chris -- Chris Wilson, Intel Open Source Technology Centre