On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@xxxxxxxxx> wrote:
> From: Pradeep Bhat <pradeep.bhat@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
