| Message ID | 1387258107-19232-2-git-send-email-vandana.kannan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 17, 2013 at 10:58:23AM +0530, Vandana Kannan wrote: > From: Pradeep Bhat <pradeep.bhat@intel.com> > > 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. What's the reason for the inconsistent intel_ tautology? > @@ -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.intel_drrs_enabled = driver->intel_drrs_state; > + DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->intel_drrs_state); Now this oddity needs a big explanation. Writing that will hopefully reveal a better strategy. -Chris
On Dec-17-2013 5:56 PM, Chris Wilson wrote: > On Tue, Dec 17, 2013 at 10:58:23AM +0530, Vandana Kannan wrote: >> From: Pradeep Bhat <pradeep.bhat@intel.com> >> >> 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. > > What's the reason for the inconsistent intel_ tautology? > If you are referring to the names of members under bdb_driver_features, which start with intel_, then this is something which can be modified to remove the "intel_". Is it ok? >> @@ -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.intel_drrs_enabled = driver->intel_drrs_state; >> + DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->intel_drrs_state); > > Now this oddity needs a big explanation. Writing that will hopefully > reveal a better strategy. > -Chris > Panel vendors specify panel capabilities via the VBT. Following this, the panel's capability to support seamless DRRS has to be read from the VBT. The same is being done in the piece of code above. Without this we cannot assume that the panel supports seamless DRRS.
On Wed, Dec 18, 2013 at 01:38:44PM +0530, Vandana Kannan wrote: > On Dec-17-2013 5:56 PM, Chris Wilson wrote: > > On Tue, Dec 17, 2013 at 10:58:23AM +0530, Vandana Kannan wrote: > >> From: Pradeep Bhat <pradeep.bhat@intel.com> > >> > >> 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. > > > > What's the reason for the inconsistent intel_ tautology? > > > If you are referring to the names of members under bdb_driver_features, > which start with intel_, then this is something which can be modified to > remove the "intel_". Is it ok? Yes, we use intel_ as a function prefix for generic routines that apply to almost all display engines. We expect that all of our hardware specific structure are used for Intel and so don't need reminding, least of all, inconsistently. And s/drrs_state/drrs/. > >> @@ -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.intel_drrs_enabled = driver->intel_drrs_state; > >> + DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->intel_drrs_state); > > > > Now this oddity needs a big explanation. Writing that will hopefully > > reveal a better strategy. > > -Chris > > > Panel vendors specify panel capabilities via the VBT. Following this, > the panel's capability to support seamless DRRS has to be read from the > VBT. The same is being done in the piece of code above. Without this we > cannot assume that the panel supports seamless DRRS. Nevermind, I misread driver-> as dev_priv->. -Chris
On Dec-18-2013 2:41 PM, Chris Wilson wrote: > On Wed, Dec 18, 2013 at 01:38:44PM +0530, Vandana Kannan wrote: >> On Dec-17-2013 5:56 PM, Chris Wilson wrote: >>> On Tue, Dec 17, 2013 at 10:58:23AM +0530, Vandana Kannan wrote: >>>> From: Pradeep Bhat <pradeep.bhat@intel.com> >>>> >>>> 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. >>> >>> What's the reason for the inconsistent intel_ tautology? >>> >> If you are referring to the names of members under bdb_driver_features, >> which start with intel_, then this is something which can be modified to >> remove the "intel_". Is it ok? > > Yes, we use intel_ as a function prefix for generic routines that apply > to almost all display engines. We expect that all of our hardware specific > structure are used for Intel and so don't need reminding, least > of all, inconsistently. > > And s/drrs_state/drrs/. > I will make these changes. - Vandana >>>> @@ -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.intel_drrs_enabled = driver->intel_drrs_state; >>>> + DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->intel_drrs_state); >>> >>> Now this oddity needs a big explanation. Writing that will hopefully >>> reveal a better strategy. >>> -Chris >>> >> Panel vendors specify panel capabilities via the VBT. Following this, >> the panel's capability to support seamless DRRS has to be read from the >> VBT. The same is being done in the piece of code above. Without this we >> cannot assume that the panel supports seamless DRRS. > > Nevermind, I misread driver-> as dev_priv->. > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 64ed8f4..02e11dc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1175,6 +1175,15 @@ struct intel_vbt_data { int lvds_ssc_freq; unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */ + /** + * 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 intel_drrs_enabled; + /* 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 6dd622d..15ee0b8 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; +} + /* 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", + (dev_priv->vbt.drrs_mode == 0) ? "STATIC" : "SEAMLESS"); + 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.intel_drrs_enabled = driver->intel_drrs_state; + DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->intel_drrs_state); } static void diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h index f580a2b..da6685b 100644 --- a/drivers/gpu/drm/i915/intel_bios.h +++ b/drivers/gpu/drm/i915/intel_bios.h @@ -202,6 +202,9 @@ struct bdb_general_features { #define DEVICE_PORT_DVOB 0x01 #define DEVICE_PORT_DVOC 0x02 +/* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */ +#define MODE_MASK 0x3 + /* We used to keep this struct but without any version control. We should avoid * using it in the future, but it should be safe to keep using it in the old * code. */ @@ -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; + /* 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; + /* LVDS panel type bits stored here */ + u32 dps_panel_type_bits; + /* LVDS backlight control type bits stored here */ + u32 blt_control_type_bits; } __attribute__((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 intel_rmpm_state:1; + u16 intel_s2ddt_state:1; + u16 intel_dpst_state:1; + u16 intel_bltclt_state:1; + u16 intel_adb_state:1; + u16 intel_drrs_state:1; + u16 intel_grs_state:1; + u16 intel_gpmt_state:1; + u16 intel_tbt_state:1; + u16 psr_state:1; + u16 ips_state:1; + u16 reserved3:4; + u16 pc_feature_validity:1; } __attribute__((packed)); #define EDP_18BPP 0