From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vandana Kannan Subject: Re: [PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature Date: Mon, 03 Feb 2014 09:13:17 +0530 Message-ID: <52EF1055.1090706@intel.com> References: <1387785153-5329-1-git-send-email-vandana.kannan@intel.com> <1387785153-5329-2-git-send-email-vandana.kannan@intel.com> <87ppnk5fk5.fsf@intel.com> <52E9C734.4040107@intel.com> <87a9ee2dpn.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id 9B20E1111C9 for ; Sun, 2 Feb 2014 19:43:22 -0800 (PST) In-Reply-To: <87a9ee2dpn.fsf@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: Jani Nikula Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Jan-30-2014 11:50 AM, Jani Nikula wrote: > On Thu, 30 Jan 2014, Vandana Kannan wrote: >> On Jan-22-2014 6:39 PM, Jani Nikula wrote: >>> On Mon, 23 Dec 2013, Vandana Kannan wrote: >>>> From: Pradeep Bhat >>>> >>>> This patch reads the DRRS support and Mode type from VBT fields. >>>> The read information will be stored in VBT struct during BIOS >>>> parsing. The above functionality is needed for decision making >>>> whether DRRS feature is supported in i915 driver for eDP panels. >>>> This information helps us decide if seamless DRRS can be done >>>> at runtime to support certain power saving features. This patch >>>> was tested by setting necessary bit in VBT struct and merging >>>> the new VBT with system BIOS so that we can read the value. >>>> >>>> v2: Incorporated review comments from Chris Wilson >>>> Removed "intel_" as a prefix for DRRS specific declarations. >>>> >>>> Signed-off-by: Pradeep Bhat >>>> Signed-off-by: Vandana Kannan >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ >>>> drivers/gpu/drm/i915/intel_bios.c | 23 +++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_bios.h | 29 +++++++++++++++++++++++++++++ >>>> 3 files changed, 61 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index ae2c80c..f8fd045 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -1174,6 +1174,15 @@ struct intel_vbt_data { >>>> int lvds_ssc_freq; >>>> unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */ >>>> >>>> + /** >>> >>> This is not a kerneldoc comment, so drop the extra *. >>> >> Ok. >>>> + * DRRS mode type (Seamless OR Static DRRS) >>>> + * drrs_mode Val 0x2 is Seamless DRRS and 0 is Static DRRS. >>>> + * These values correspond to the VBT values for drrs mode. >>>> + */ >>>> + int drrs_mode; >>>> + /* DRRS enabled or disabled in VBT */ >>>> + bool drrs_enabled; >>> >>> Both the drrs_mode and drrs_enabled should be replaced by one enum >>> drrs_support_type which you introduce later. It's all self-explanatory >>> that way, and you don't need to explain so much. >>> >> There are 2 options in the VBT. drrs_enabled to check if DRRS is >> supported, drrs_mode to show which type. It has been added here as it is >> for readability. > > I can understand the argument for anything defined in intel_bios.[ch], > but for the rest, including struct intel_vbt_data, readability comes > from other reasons than being equivalent with the VBT specs. > >>>> + >>>> /* eDP */ >>>> int edp_rate; >>>> int edp_lanes; >>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >>>> index f88e507..5b04a69 100644 >>>> --- a/drivers/gpu/drm/i915/intel_bios.c >>>> +++ b/drivers/gpu/drm/i915/intel_bios.c >>>> @@ -195,6 +195,21 @@ get_lvds_fp_timing(const struct bdb_header *bdb, >>>> return (const struct lvds_fp_timing *)((const u8 *)bdb + ofs); >>>> } >>>> >>>> +/** >>>> + * This function returns the 2 bit information pertaining to a panel type >>>> + * present in a 32 bit field in VBT blocks. There are 16 panel types in VBT >>>> + * each occupying 2 bits of information in some 32 bit fields of VBT blocks. >>>> + */ >>>> +static int >>>> +get_mode_by_paneltype(unsigned int word) >>>> +{ >>>> + /** >>>> + * The caller of this API should interpret the 2 bits >>>> + * based on VBT description for that field. >>>> + */ >>>> + return (word >> ((panel_type - 1) * 2)) & MODE_MASK; >>> >>> Everywhere else in intel_bios.c panel_type is used 0-based. >>> >> VBT indexes panel type as 1,2,3. Therefore, we have to make the change >> to match kernel's 0-based usage. > > Again, everywhere else in intel_bios.c we use panel_type, directly as it > is in VBT, 0-based. Are you saying it's all wrong? panel_type can't be > 1-based in this one instance, and 0-based in all other instances, right? > > This is actually the first priority to check. > I have discussed with the author of the patch and i will modify this to make panel_type 0-based. >>>> +} >>> >>> You use the above function only once. IMHO with all the explaining above >>> it's just too much of a burden to the reader. Just do this in the code. >>> >> Ok. >>>> + >>>> /* Try to find integrated panel data */ >>>> static void >>>> parse_lfp_panel_data(struct drm_i915_private *dev_priv, >>>> @@ -218,6 +233,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, >>>> >>>> panel_type = lvds_options->panel_type; >>>> >>>> + dev_priv->vbt.drrs_mode = >>>> + get_mode_by_paneltype(lvds_options->dps_panel_type_bits); >>>> + DRM_DEBUG_KMS("DRRS supported mode is : %s\n", >>> ^ drop the space here >>> >> Ok >>>> + (dev_priv->vbt.drrs_mode == 0) ? "STATIC" : "SEAMLESS"); >>> >>> Why shouting? >>> >>>> + >>>> lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA); >>>> if (!lvds_lfp_data) >>>> return; >>>> @@ -488,6 +508,9 @@ parse_driver_features(struct drm_i915_private *dev_priv, >>>> >>>> if (driver->dual_frequency) >>>> dev_priv->render_reclock_avail = true; >>>> + >>>> + dev_priv->vbt.drrs_enabled = driver->drrs_state; >>>> + DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->drrs_state); >>> ^ and here and everywhere >>> >> Ok >>>> } >>>> >>>> static void >>>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h >>>> index 81ed58c..0a3a751 100644 >>>> --- a/drivers/gpu/drm/i915/intel_bios.h >>>> +++ b/drivers/gpu/drm/i915/intel_bios.h >>>> @@ -281,6 +281,9 @@ struct bdb_general_definitions { >>>> union child_device_config devices[0]; >>>> } __packed; >>>> >>>> +/* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */ >>>> +#define MODE_MASK 0x3 >>>> + >>>> struct bdb_lvds_options { >>>> u8 panel_type; >>>> u8 rsvd1; >>>> @@ -293,6 +296,18 @@ struct bdb_lvds_options { >>>> u8 lvds_edid:1; >>>> u8 rsvd2:1; >>>> u8 rsvd4; >>>> + /* LVDS Panel channel bits stored here */ >>>> + u32 lvds_panel_channel_bits; >>> >>> Why does this have "lvds" but the rest not? Why do some fields end in >>> "bits" but some others not? Some consistency please. >>> >> This is how the vbt block definition doc mentions each of these fields. >> This is the reason why it has been added in this manner. > > I don't have my vbt spec handy right now, but when I was last checking > these it didn't look consistent with the spec. > > Same for the ones below. > VBT-driver interface document has been referred for this. >> >>>> + /* LVDS SSC (Spread Spectrum Clock) bits stored here. */ >>>> + u16 ssc_bits; >>>> + u16 ssc_freq; >>>> + u16 ssc_ddt; >>>> + /* Panel color depth defined here */ >>>> + u16 panel_color_depth; >>> >>> I really *really* wish I knew why some stuff is specified in the lvds >>> bios blob and some other in edp blob and some stuff specified here is >>> used in the edp side. Ugh. I wonder if we should check this >>> panel_color_depth for edp too. >>> >> This is how the vbt block definition doc mentions each of these fields. >> This is the reason why it has been added in this manner. >> >>>> + /* LVDS panel type bits stored here */ >>>> + u32 dps_panel_type_bits; >>>> + /* LVDS backlight control type bits stored here */ >>>> + u32 blt_control_type_bits; >>>> } __packed; >>>> >>>> /* LFP pointer table contains entries to the struct below */ >>>> @@ -462,6 +477,20 @@ struct bdb_driver_features { >>>> >>>> u8 hdmi_termination; >>>> u8 custom_vbt_version; >>>> + /* Driver features data block */ >>>> + u16 rmpm_state:1; >>>> + u16 s2ddt_state:1; >>>> + u16 dpst_state:1; >>>> + u16 bltclt_state:1; >>>> + u16 adb_state:1; >>>> + u16 drrs_state:1; >>>> + u16 grs_state:1; >>>> + u16 gpmt_state:1; >>>> + u16 tbt_state:1; >>>> + u16 psr_state:1; >>>> + u16 ips_state:1; >>> >>> All of the above should be foo_enabled to make them self-explanatory. >>> >>>> + u16 reserved3:4; >>>> + u16 pc_feature_validity:1; >>> >>> Similarly for this one, should be pc_feature_valid. >>> >> This is how the vbt block definition doc mentions this field. >> This is the reason why it has been added in this manner. >>>> } __packed; >>>> >>>> #define EDP_18BPP 0 >>>> -- >>>> 1.7.9.5 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> >> >