diff mbox

[03/13] drm/i915: VBT Parsing for the PSR Feature Block for HSW

Message ID 1371070554-1947-3-git-send-email-rodrigo.vivi@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi June 12, 2013, 8:55 p.m. UTC 
From: Shobhit Kumar <shobhit.kumar@intel.com>

Parse and store useful information in i915_dev_private

v2: Add to new vbt struct and call them psr_*
v3: Fix comment and variable name

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  7 +++++++
 drivers/gpu/drm/i915/intel_bios.c | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_bios.h | 20 +++++++++++++++++++-
 3 files changed, 56 insertions(+), 1 deletion(-)

Comments

Chris Wilson June 13, 2013, 1:41 p.m. UTC | #1 
On Wed, Jun 12, 2013 at 05:55:44PM -0300, Rodrigo Vivi wrote:
> From: Shobhit Kumar <shobhit.kumar@intel.com>
> 
> Parse and store useful information in i915_dev_private
> 
> v2: Add to new vbt struct and call them psr_*
> v3: Fix comment and variable name
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  7 +++++++
>  drivers/gpu/drm/i915/intel_bios.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_bios.h | 20 +++++++++++++++++++-
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 87f7f88..dd459a5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -951,6 +951,13 @@ struct intel_vbt_data {
>  	int edp_bpp;
>  	struct edp_power_seq edp_pps;
>  
> +	/* eDP PSR*/
> +	u8 psr_full_link_state;
> +	u8 psr_wait_lines;
> +	u8 psr_idle_frames;
> +	u16 psr_wakeup_tp1;
> +	u16 psr_wakeup_tp2_tp3;

Let's be neat and tidy and move these into their own psr struct. Do we
want a valid flag here?

>+
>  	int crt_ddc_pin;
>  
>  	int child_dev_num;
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 53f2bed..99c6788 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -383,6 +383,35 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +
> +static void
> +parse_edp_psr(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
> +{
> +	struct bdb_psr_features *psr;
> +	struct bdb_lvds_options *lvds_opts;
> +	int index = 0;
> +	lvds_opts = find_section(bdb, BDB_LVDS_OPTIONS);
> +	if (!lvds_opts) {
> +		DRM_DEBUG_KMS("No LVDS Options block found.\n");
> +		return;
> +	}
> +
> +	index = lvds_opts->panel_type;
> +
> +	psr = find_section(bdb, BDB_PSR_FEATURES);
> +	if (!psr) {
> +		DRM_DEBUG_KMS("No PSR feature block found.\n");
> +		return;
> +	}

You trust the bios that much to read a random index into an array?
Everything from the BIOS is tainted and must be validated.
-Chris
Paulo Zanoni June 14, 2013, 5:02 p.m. UTC | #2 
2013/6/12 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> From: Shobhit Kumar <shobhit.kumar@intel.com>
>
> Parse and store useful information in i915_dev_private
>
> v2: Add to new vbt struct and call them psr_*
> v3: Fix comment and variable name
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  7 +++++++
>  drivers/gpu/drm/i915/intel_bios.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_bios.h | 20 +++++++++++++++++++-
>  3 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 87f7f88..dd459a5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -951,6 +951,13 @@ struct intel_vbt_data {
>         int edp_bpp;
>         struct edp_power_seq edp_pps;
>
> +       /* eDP PSR*/
> +       u8 psr_full_link_state;
> +       u8 psr_wait_lines;
> +       u8 psr_idle_frames;
> +       u16 psr_wakeup_tp1;
> +       u16 psr_wakeup_tp2_tp3;
> +
>         int crt_ddc_pin;
>
>         int child_dev_num;
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 53f2bed..99c6788 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -383,6 +383,35 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
>         }
>  }
>
> +
> +static void
> +parse_edp_psr(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
> +{
> +       struct bdb_psr_features *psr;
> +       struct bdb_lvds_options *lvds_opts;
> +       int index = 0;
> +       lvds_opts = find_section(bdb, BDB_LVDS_OPTIONS);
> +       if (!lvds_opts) {
> +               DRM_DEBUG_KMS("No LVDS Options block found.\n");
> +               return;

We already parse BDB_LVDS_OPTIONS inside parse_lfp_panel_data(). I
wonder if we can reuse that parse somehow.


> +       }
> +
> +       index = lvds_opts->panel_type;
> +
> +       psr = find_section(bdb, BDB_PSR_FEATURES);

Don't we need to check for VBT version >= 155 here before proceeding?
Actually the docs are confusing, they say this block comes from
version 155, but all the bits inside it come from version 165... Which
one is the correct one? It seems 165 is the "safer" option at least...


> +       if (!psr) {
> +               DRM_DEBUG_KMS("No PSR feature block found.\n");
> +               return;
> +       }
> +
> +       dev_priv->vbt.psr_full_link_state = psr[index].link_disable;
> +       dev_priv->vbt.psr_wait_lines = psr[index].wait_lines;
> +       dev_priv->vbt.psr_idle_frames = psr[index].idle_frames;
> +       dev_priv->vbt.psr_wakeup_tp1 = psr[index].wakeup_tp1;
> +       dev_priv->vbt.psr_wakeup_tp2_tp3 = psr[index].wakeup_tp2_tp3;
> +}
> +
> +
>  static void
>  parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
>                           struct bdb_header *bdb)
> @@ -745,6 +774,7 @@ intel_parse_bios(struct drm_device *dev)
>         parse_device_mapping(dev_priv, bdb);
>         parse_driver_features(dev_priv, bdb);
>         parse_edp(dev_priv, bdb);
> +       parse_edp_psr(dev_priv, bdb);
>
>         if (bios)
>                 pci_unmap_rom(pdev, bios);
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index e088d6f..c883b87 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -80,7 +80,7 @@ struct vbios_data {
>  #define BDB_EXT_MMIO_REGS        6
>  #define BDB_SWF_IO               7
>  #define BDB_SWF_MMIO             8
> -#define BDB_DOT_CLOCK_TABLE      9

This is probably from some older VBT version. That's why I suggested
checking the version above. I guess we shouldn't remove the old one,
but we should add some comments explaining this.


> +#define BDB_PSR_FEATURES         9
>  #define BDB_MODE_REMOVAL_TABLE  10
>  #define BDB_CHILD_DEVICE_TABLE  11
>  #define BDB_DRIVER_FEATURES     12
> @@ -265,6 +265,24 @@ struct bdb_lvds_options {
>         u8 rsvd4;
>  } __attribute__((packed));
>
> +struct bdb_psr_features {
> +       /* Feature bits */
> +       u8 link_disable:1;

Considering that 1 means "Enable", I'd rename this to "full_link_enable".


> +       u8 require_aux:1;
> +       u8 rsvd1:6;
> +
> +       /* Wait times */
> +       u8 idle_frames:4;
> +       u8 wait_lines:3;
> +       u8 rsvd2:1;
> +
> +       /* TP1 wakeup time */
> +       u16 wakeup_tp1;
> +
> +       /* TP2 TP3 wakeup time */
> +       u16 wakeup_tp2_tp3;
> +} __attribute__((packed));
> +
>  /* LFP pointer table contains entries to the struct below */
>  struct bdb_lvds_lfp_data_ptr {
>         u16 fp_timing_offset; /* offsets are from start of bdb */
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 87f7f88..dd459a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -951,6 +951,13 @@  struct intel_vbt_data {
 	int edp_bpp;
 	struct edp_power_seq edp_pps;
 
+	/* eDP PSR*/
+	u8 psr_full_link_state;
+	u8 psr_wait_lines;
+	u8 psr_idle_frames;
+	u16 psr_wakeup_tp1;
+	u16 psr_wakeup_tp2_tp3;
+
 	int crt_ddc_pin;
 
 	int child_dev_num;
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 53f2bed..99c6788 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -383,6 +383,35 @@  parse_general_definitions(struct drm_i915_private *dev_priv,
 	}
 }
 
+
+static void
+parse_edp_psr(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
+{
+	struct bdb_psr_features *psr;
+	struct bdb_lvds_options *lvds_opts;
+	int index = 0;
+	lvds_opts = find_section(bdb, BDB_LVDS_OPTIONS);
+	if (!lvds_opts) {
+		DRM_DEBUG_KMS("No LVDS Options block found.\n");
+		return;
+	}
+
+	index = lvds_opts->panel_type;
+
+	psr = find_section(bdb, BDB_PSR_FEATURES);
+	if (!psr) {
+		DRM_DEBUG_KMS("No PSR feature block found.\n");
+		return;
+	}
+
+	dev_priv->vbt.psr_full_link_state = psr[index].link_disable;
+	dev_priv->vbt.psr_wait_lines = psr[index].wait_lines;
+	dev_priv->vbt.psr_idle_frames = psr[index].idle_frames;
+	dev_priv->vbt.psr_wakeup_tp1 = psr[index].wakeup_tp1;
+	dev_priv->vbt.psr_wakeup_tp2_tp3 = psr[index].wakeup_tp2_tp3;
+}
+
+
 static void
 parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
 			  struct bdb_header *bdb)
@@ -745,6 +774,7 @@  intel_parse_bios(struct drm_device *dev)
 	parse_device_mapping(dev_priv, bdb);
 	parse_driver_features(dev_priv, bdb);
 	parse_edp(dev_priv, bdb);
+	parse_edp_psr(dev_priv, bdb);
 
 	if (bios)
 		pci_unmap_rom(pdev, bios);
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index e088d6f..c883b87 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -80,7 +80,7 @@  struct vbios_data {
 #define BDB_EXT_MMIO_REGS	  6
 #define BDB_SWF_IO		  7
 #define BDB_SWF_MMIO		  8
-#define BDB_DOT_CLOCK_TABLE	  9
+#define BDB_PSR_FEATURES	  9
 #define BDB_MODE_REMOVAL_TABLE	 10
 #define BDB_CHILD_DEVICE_TABLE	 11
 #define BDB_DRIVER_FEATURES	 12
@@ -265,6 +265,24 @@  struct bdb_lvds_options {
 	u8 rsvd4;
 } __attribute__((packed));
 
+struct bdb_psr_features {
+	/* Feature bits */
+	u8 link_disable:1;
+	u8 require_aux:1;
+	u8 rsvd1:6;
+
+	/* Wait times */
+	u8 idle_frames:4;
+	u8 wait_lines:3;
+	u8 rsvd2:1;
+
+	/* TP1 wakeup time */
+	u16 wakeup_tp1;
+
+	/* TP2 TP3 wakeup time */
+	u16 wakeup_tp2_tp3;
+} __attribute__((packed));
+
 /* LFP pointer table contains entries to the struct below */
 struct bdb_lvds_lfp_data_ptr {
 	u16 fp_timing_offset; /* offsets are from start of bdb */