| Message ID | 1387785153-5329-2-git-send-email-vandana.kannan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> 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. > > v2: Incorporated review comments from Chris Wilson > Removed "intel_" as a prefix for DRRS specific declarations. > > Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com> > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > --- > 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 *. > + * 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. > + > /* 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. > +} 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. > + > /* 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 > + (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 > } > > 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. > + /* 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. > + /* 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. > } __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
On Jan-22-2014 6:39 PM, Jani Nikula wrote: > On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> 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. >> >> v2: Incorporated review comments from Chris Wilson >> Removed "intel_" as a prefix for DRRS specific declarations. >> >> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com> >> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >> --- >> 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. >> + >> /* 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. >> +} > > 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. >> + /* 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 >
On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan@intel.com> wrote: > On Jan-22-2014 6:39 PM, Jani Nikula wrote: >> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> 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. >>> >>> v2: Incorporated review comments from Chris Wilson >>> Removed "intel_" as a prefix for DRRS specific declarations. >>> >>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com> >>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>> --- >>> 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. >>> +} >> >> 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. > >>> + /* 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 >> >
On Jan-30-2014 11:50 AM, Jani Nikula wrote: > On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan@intel.com> wrote: >> On Jan-22-2014 6:39 PM, Jani Nikula wrote: >>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> 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. >>>> >>>> v2: Incorporated review comments from Chris Wilson >>>> Removed "intel_" as a prefix for DRRS specific declarations. >>>> >>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com> >>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>>> --- >>>> 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 >>> >> >
On Mon, Feb 03, 2014 at 09:13:17AM +0530, Vandana Kannan wrote: > > 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. Can we drag the author of the patch itself into this discussion, here on intel-gfx? Generally round-trip is much faster if we cut out as many middlemen as possible ... This is just a general upstreaming bkm. -Daniel
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 */ + /** + * 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; + /* 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; +} + /* 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.drrs_enabled = driver->drrs_state; + DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->drrs_state); } 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; + /* 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; } __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; + u16 reserved3:4; + u16 pc_feature_validity:1; } __packed; #define EDP_18BPP 0