From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vandana Kannan Subject: Re: [PATCH 2/5] drm/i915: Parse EDID probed modes for DRRS support Date: Wed, 18 Dec 2013 15:42:55 +0530 Message-ID: <52B17527.10504@intel.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> <20131218090657.GZ22448@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 6C723FBFB1 for ; Wed, 18 Dec 2013 02:13:17 -0800 (PST) In-Reply-To: <20131218090657.GZ22448@nuc-i3427.alporthouse.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: Chris Wilson Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Dec-18-2013 2:36 PM, Chris Wilson wrote: > 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. > For this patch series, I will move edp_downclock_avail and edp_downclock to intel_panel, and following that, work on a separate patch to move lvds_downclock_avail and lvds_downclock to 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 > Ok. I will not be making any change related to this.