[PATCH 1/3] drm/i915: Reject panel_type > 0xf from VBT

[PATCH 1/3] drm/i915: Reject panel_type > 0xf from VBT

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VBT can only contain 16 panel entries, indexed with the panel_type.
To play it safe we should reject panel_type > 0xf, so that we don't
read past the valid data.

Cc: Rob Kramer <rob@solution-space.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index eb756c41d9e1..13bdd4316092 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -212,7 +212,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 		return;
 
 	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
-	if (lvds_options->panel_type == 0xff)
+	if (lvds_options->panel_type > 0xf)
 		return;
 
 	panel_type = lvds_options->panel_type;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Replace the static panel_type variable with dev_priv->vbt.panel_type

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Store the extracted panel_type under dev_priv.vbt instead of keeping
around a static variable for it.

Cc: Rob Kramer <rob@solution-space.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_bios.c | 13 +++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ebd3ff02803..53ace572b292 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1443,6 +1443,7 @@ struct intel_vbt_data {
 	unsigned int lvds_use_ssc:1;
 	unsigned int display_clock_mode:1;
 	unsigned int fdi_rx_polarity_inverted:1;
+	unsigned int panel_type:4;
 	int lvds_ssc_freq;
 	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
 
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 13bdd4316092..d9568be4b33b 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -58,8 +58,6 @@
 #define	SLAVE_ADDR1	0x70
 #define	SLAVE_ADDR2	0x72
 
-static int panel_type;
-
 /* Get BDB block size given a pointer to Block ID. */
 static u32 _get_blocksize(const u8 *block_base)
 {
@@ -205,6 +203,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 	const struct lvds_dvo_timing *panel_dvo_timing;
 	const struct lvds_fp_timing *fp_timing;
 	struct drm_display_mode *panel_fixed_mode;
+	int panel_type;
 	int drrs_mode;
 
 	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
@@ -216,6 +215,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 		return;
 
 	panel_type = lvds_options->panel_type;
+	dev_priv->vbt.panel_type = panel_type;
 
 	drrs_mode = (lvds_options->dps_panel_type_bits
 				>> (panel_type * 2)) & MODE_MASK;
@@ -251,7 +251,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 
 	panel_dvo_timing = get_lvds_dvo_timing(lvds_lfp_data,
 					       lvds_lfp_data_ptrs,
-					       lvds_options->panel_type);
+					       panel_type);
 
 	panel_fixed_mode = kzalloc(sizeof(*panel_fixed_mode), GFP_KERNEL);
 	if (!panel_fixed_mode)
@@ -266,7 +266,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 
 	fp_timing = get_lvds_fp_timing(bdb, lvds_lfp_data,
 				       lvds_lfp_data_ptrs,
-				       lvds_options->panel_type);
+				       panel_type);
 	if (fp_timing) {
 		/* check the resolution, just to be sure */
 		if (fp_timing->x_res == panel_fixed_mode->hdisplay &&
@@ -284,6 +284,7 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv,
 {
 	const struct bdb_lfp_backlight_data *backlight_data;
 	const struct bdb_lfp_backlight_data_entry *entry;
+	int panel_type = dev_priv->vbt.panel_type;
 
 	backlight_data = find_section(bdb, BDB_LVDS_BACKLIGHT);
 	if (!backlight_data)
@@ -546,6 +547,7 @@ parse_edp(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 	const struct bdb_edp *edp;
 	const struct edp_power_seq *edp_pps;
 	const struct edp_link_params *edp_link_params;
+	int panel_type = dev_priv->vbt.panel_type;
 
 	edp = find_section(bdb, BDB_EDP);
 	if (!edp) {
@@ -657,6 +659,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 {
 	const struct bdb_psr *psr;
 	const struct psr_table *psr_table;
+	int panel_type = dev_priv->vbt.panel_type;
 
 	psr = find_section(bdb, BDB_PSR);
 	if (!psr) {
@@ -703,6 +706,7 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
 	const struct bdb_mipi_config *start;
 	const struct mipi_config *config;
 	const struct mipi_pps_data *pps;
+	int panel_type = dev_priv->vbt.panel_type;
 
 	/* parse MIPI blocks only if LFP type is MIPI */
 	if (!intel_bios_is_dsi_present(dev_priv, NULL))
@@ -910,6 +914,7 @@ static void
 parse_mipi_sequence(struct drm_i915_private *dev_priv,
 		    const struct bdb_header *bdb)
 {
+	int panel_type = dev_priv->vbt.panel_type;
 	const struct bdb_mipi_sequence *sequence;
 	const u8 *seq_data;
 	u32 seq_size;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Replace the static panel_type variable with dev_priv->vbt.panel_type

[Jani Nikula]

On Fri, 08 Apr 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Store the extracted panel_type under dev_priv.vbt instead of keeping
> around a static variable for it.
>
> Cc: Rob Kramer <rob@solution-space.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yes please.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  1 +
>  drivers/gpu/drm/i915/intel_bios.c | 13 +++++++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4ebd3ff02803..53ace572b292 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1443,6 +1443,7 @@ struct intel_vbt_data {
>  	unsigned int lvds_use_ssc:1;
>  	unsigned int display_clock_mode:1;
>  	unsigned int fdi_rx_polarity_inverted:1;
> +	unsigned int panel_type:4;
>  	int lvds_ssc_freq;
>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>  
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 13bdd4316092..d9568be4b33b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -58,8 +58,6 @@
>  #define	SLAVE_ADDR1	0x70
>  #define	SLAVE_ADDR2	0x72
>  
> -static int panel_type;
> -
>  /* Get BDB block size given a pointer to Block ID. */
>  static u32 _get_blocksize(const u8 *block_base)
>  {
> @@ -205,6 +203,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  	const struct lvds_dvo_timing *panel_dvo_timing;
>  	const struct lvds_fp_timing *fp_timing;
>  	struct drm_display_mode *panel_fixed_mode;
> +	int panel_type;
>  	int drrs_mode;
>  
>  	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
> @@ -216,6 +215,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  		return;
>  
>  	panel_type = lvds_options->panel_type;
> +	dev_priv->vbt.panel_type = panel_type;
>  
>  	drrs_mode = (lvds_options->dps_panel_type_bits
>  				>> (panel_type * 2)) & MODE_MASK;
> @@ -251,7 +251,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  
>  	panel_dvo_timing = get_lvds_dvo_timing(lvds_lfp_data,
>  					       lvds_lfp_data_ptrs,
> -					       lvds_options->panel_type);
> +					       panel_type);
>  
>  	panel_fixed_mode = kzalloc(sizeof(*panel_fixed_mode), GFP_KERNEL);
>  	if (!panel_fixed_mode)
> @@ -266,7 +266,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  
>  	fp_timing = get_lvds_fp_timing(bdb, lvds_lfp_data,
>  				       lvds_lfp_data_ptrs,
> -				       lvds_options->panel_type);
> +				       panel_type);
>  	if (fp_timing) {
>  		/* check the resolution, just to be sure */
>  		if (fp_timing->x_res == panel_fixed_mode->hdisplay &&
> @@ -284,6 +284,7 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv,
>  {
>  	const struct bdb_lfp_backlight_data *backlight_data;
>  	const struct bdb_lfp_backlight_data_entry *entry;
> +	int panel_type = dev_priv->vbt.panel_type;
>  
>  	backlight_data = find_section(bdb, BDB_LVDS_BACKLIGHT);
>  	if (!backlight_data)
> @@ -546,6 +547,7 @@ parse_edp(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  	const struct bdb_edp *edp;
>  	const struct edp_power_seq *edp_pps;
>  	const struct edp_link_params *edp_link_params;
> +	int panel_type = dev_priv->vbt.panel_type;
>  
>  	edp = find_section(bdb, BDB_EDP);
>  	if (!edp) {
> @@ -657,6 +659,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  {
>  	const struct bdb_psr *psr;
>  	const struct psr_table *psr_table;
> +	int panel_type = dev_priv->vbt.panel_type;
>  
>  	psr = find_section(bdb, BDB_PSR);
>  	if (!psr) {
> @@ -703,6 +706,7 @@ parse_mipi_config(struct drm_i915_private *dev_priv,
>  	const struct bdb_mipi_config *start;
>  	const struct mipi_config *config;
>  	const struct mipi_pps_data *pps;
> +	int panel_type = dev_priv->vbt.panel_type;
>  
>  	/* parse MIPI blocks only if LFP type is MIPI */
>  	if (!intel_bios_is_dsi_present(dev_priv, NULL))
> @@ -910,6 +914,7 @@ static void
>  parse_mipi_sequence(struct drm_i915_private *dev_priv,
>  		    const struct bdb_header *bdb)
>  {
> +	int panel_type = dev_priv->vbt.panel_type;
>  	const struct bdb_mipi_sequence *sequence;
>  	const u8 *seq_data;
>  	u32 seq_size;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Get panel_type from OpRegion panel details

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We've had problems on several occasions with using the panel type
from the VBT block 40. Usually it seems to be 2, which often
doesn't give us the correct timings for the panel. After some
more digging I found a way to get a panel type via the OpRegion
SWSCI GBDA "Get Panel Details" method. Let's try to use it.

The spec has this to say about the output:
"Bits [15:8] - Panel Type
 Bits contain the panel type user setting from CMOS
 00h = Not Valid, use default Panel Type & Timings from VBT
 01h - 0Fh = Panel Number"

The other bits in the output don't look relevant for the problem at
hand.

The input is specified as:
"Bits [31:4] - Reserved
 Reserved (must be zero)
 Bits [3:0] - Panel Number
 These bits contain the sequential index of Panel, starting at 0 and
 counting upwards from the first integrated Internal Flat-Panel Display
 Encoder present, and then from the first external Display Encoder
 (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
 0h - 0Fh = Panel number"

For now I've just hardcoded the input panel number as 0. That would seem
like a decent choise for LVDS. Not so sure about eDP when port != A.

Cc: Rob Kramer <rob@solution-space.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
 drivers/gpu/drm/i915/intel_bios.c     | 13 ++++++++++---
 drivers/gpu/drm/i915/intel_opregion.c | 13 +++++++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 53ace572b292..533263a7a12d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
 					 bool enable);
 extern int intel_opregion_notify_adapter(struct drm_device *dev,
 					 pci_power_t state);
+extern int intel_opregion_get_panel_type(struct drm_device *dev);
 #else
 static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
 static inline void intel_opregion_init(struct drm_device *dev) { return; }
@@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
 {
 	return 0;
 }
+static inline int intel_opregion_get_panel_type(struct drm_device *dev)
+{
+	return 0;
+}
 #endif
 
 /* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index d9568be4b33b..718e49931b5f 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -205,16 +205,23 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 	struct drm_display_mode *panel_fixed_mode;
 	int panel_type;
 	int drrs_mode;
+	int ret;
 
 	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
 	if (!lvds_options)
 		return;
 
 	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
-	if (lvds_options->panel_type > 0xf)
-		return;
 
-	panel_type = lvds_options->panel_type;
+	ret = intel_opregion_get_panel_type(dev_priv->dev);
+	if (ret >= 0x1 && ret <= 0xf) {
+		panel_type = ret;
+	} else {
+		if (lvds_options->panel_type > 0xf)
+			return;
+		panel_type = lvds_options->panel_type;
+	}
+
 	dev_priv->vbt.panel_type = panel_type;
 
 	drrs_mode = (lvds_options->dps_panel_type_bits
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index c15718b4862a..5e17fa5dc869 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -1024,3 +1024,16 @@ err_out:
 	memunmap(base);
 	return err;
 }
+
+int
+intel_opregion_get_panel_type(struct drm_device *dev)
+{
+	u32 panel_details;
+	int ret;
+
+	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
+	if (ret)
+		return ret;
+
+	return (panel_details >> 8) & 0xff;
+}
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Get panel_type from OpRegion panel details

[Jani Nikula]

On Fri, 08 Apr 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We've had problems on several occasions with using the panel type
> from the VBT block 40. Usually it seems to be 2, which often
> doesn't give us the correct timings for the panel. After some
> more digging I found a way to get a panel type via the OpRegion
> SWSCI GBDA "Get Panel Details" method. Let's try to use it.
>
> The spec has this to say about the output:
> "Bits [15:8] - Panel Type
>  Bits contain the panel type user setting from CMOS
>  00h = Not Valid, use default Panel Type & Timings from VBT
>  01h - 0Fh = Panel Number"
>
> The other bits in the output don't look relevant for the problem at
> hand.
>
> The input is specified as:
> "Bits [31:4] - Reserved
>  Reserved (must be zero)
>  Bits [3:0] - Panel Number
>  These bits contain the sequential index of Panel, starting at 0 and
>  counting upwards from the first integrated Internal Flat-Panel Display
>  Encoder present, and then from the first external Display Encoder
>  (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
>  0h - 0Fh = Panel number"
>
> For now I've just hardcoded the input panel number as 0. That would seem
> like a decent choise for LVDS. Not so sure about eDP when port != A.

Frankly, I didn't understand the point of the input parameter for
figuring out the output. The spec is written for someone who already
knows what they're doing...

>
> Cc: Rob Kramer <rob@solution-space.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
>  drivers/gpu/drm/i915/intel_bios.c     | 13 ++++++++++---
>  drivers/gpu/drm/i915/intel_opregion.c | 13 +++++++++++++
>  3 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 53ace572b292..533263a7a12d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  					 bool enable);
>  extern int intel_opregion_notify_adapter(struct drm_device *dev,
>  					 pci_power_t state);
> +extern int intel_opregion_get_panel_type(struct drm_device *dev);
>  #else
>  static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
> @@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
>  {
>  	return 0;
>  }
> +static inline int intel_opregion_get_panel_type(struct drm_device *dev)
> +{
> +	return 0;
> +}
>  #endif
>  
>  /* intel_acpi.c */
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index d9568be4b33b..718e49931b5f 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -205,16 +205,23 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  	struct drm_display_mode *panel_fixed_mode;
>  	int panel_type;
>  	int drrs_mode;
> +	int ret;
>  
>  	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
>  	if (!lvds_options)
>  		return;
>  
>  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
> -	if (lvds_options->panel_type > 0xf)
> -		return;
>  
> -	panel_type = lvds_options->panel_type;
> +	ret = intel_opregion_get_panel_type(dev_priv->dev);
> +	if (ret >= 0x1 && ret <= 0xf) {

Undecided whether I'd like to have this check within
intel_opregion_get_panel_type(). Just make that return 0 for "use vbt"
(and CONFIG_ACPI=n), and valid stuff otherwise? *shrug*

> +		panel_type = ret;
> +	} else {
> +		if (lvds_options->panel_type > 0xf)
> +			return;
> +		panel_type = lvds_options->panel_type;
> +	}
> +

If this actually works, I'd like to have some debug logging for pretty
much all the code paths here.

Other than that, seems fine.

BR,
Jani.

>  	dev_priv->vbt.panel_type = panel_type;
>  
>  	drrs_mode = (lvds_options->dps_panel_type_bits
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index c15718b4862a..5e17fa5dc869 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -1024,3 +1024,16 @@ err_out:
>  	memunmap(base);
>  	return err;
>  }
> +
> +int
> +intel_opregion_get_panel_type(struct drm_device *dev)
> +{
> +	u32 panel_details;
> +	int ret;
> +
> +	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
> +	if (ret)
> +		return ret;
> +
> +	return (panel_details >> 8) & 0xff;
> +}

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Get panel_type from OpRegion panel details

[Ville Syrjälä]

On Fri, Apr 08, 2016 at 05:50:06PM +0300, Jani Nikula wrote:
> On Fri, 08 Apr 2016, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We've had problems on several occasions with using the panel type
> > from the VBT block 40. Usually it seems to be 2, which often
> > doesn't give us the correct timings for the panel. After some
> > more digging I found a way to get a panel type via the OpRegion
> > SWSCI GBDA "Get Panel Details" method. Let's try to use it.
> >
> > The spec has this to say about the output:
> > "Bits [15:8] - Panel Type
> >  Bits contain the panel type user setting from CMOS
> >  00h = Not Valid, use default Panel Type & Timings from VBT
> >  01h - 0Fh = Panel Number"
> >
> > The other bits in the output don't look relevant for the problem at
> > hand.
> >
> > The input is specified as:
> > "Bits [31:4] - Reserved
> >  Reserved (must be zero)
> >  Bits [3:0] - Panel Number
> >  These bits contain the sequential index of Panel, starting at 0 and
> >  counting upwards from the first integrated Internal Flat-Panel Display
> >  Encoder present, and then from the first external Display Encoder
> >  (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
> >  0h - 0Fh = Panel number"
> >
> > For now I've just hardcoded the input panel number as 0. That would seem
> > like a decent choise for LVDS. Not so sure about eDP when port != A.
> 
> Frankly, I didn't understand the point of the input parameter for
> figuring out the output. The spec is written for someone who already
> knows what they're doing...
> 
> >
> > Cc: Rob Kramer <rob@solution-space.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
> >  drivers/gpu/drm/i915/intel_bios.c     | 13 ++++++++++---
> >  drivers/gpu/drm/i915/intel_opregion.c | 13 +++++++++++++
> >  3 files changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 53ace572b292..533263a7a12d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> >  					 bool enable);
> >  extern int intel_opregion_notify_adapter(struct drm_device *dev,
> >  					 pci_power_t state);
> > +extern int intel_opregion_get_panel_type(struct drm_device *dev);
> >  #else
> >  static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
> >  static inline void intel_opregion_init(struct drm_device *dev) { return; }
> > @@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
> >  {
> >  	return 0;
> >  }
> > +static inline int intel_opregion_get_panel_type(struct drm_device *dev)
> > +{
> > +	return 0;
> > +}
> >  #endif
> >  
> >  /* intel_acpi.c */
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index d9568be4b33b..718e49931b5f 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -205,16 +205,23 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
> >  	struct drm_display_mode *panel_fixed_mode;
> >  	int panel_type;
> >  	int drrs_mode;
> > +	int ret;
> >  
> >  	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
> >  	if (!lvds_options)
> >  		return;
> >  
> >  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
> > -	if (lvds_options->panel_type > 0xf)
> > -		return;
> >  
> > -	panel_type = lvds_options->panel_type;
> > +	ret = intel_opregion_get_panel_type(dev_priv->dev);
> > +	if (ret >= 0x1 && ret <= 0xf) {
> 
> Undecided whether I'd like to have this check within
> intel_opregion_get_panel_type(). Just make that return 0 for "use vbt"
> (and CONFIG_ACPI=n), and valid stuff otherwise? *shrug*

We don't compile intel_opregion.c when ACPI==n.

> 
> > +		panel_type = ret;
> > +	} else {
> > +		if (lvds_options->panel_type > 0xf)
> > +			return;
> > +		panel_type = lvds_options->panel_type;
> > +	}
> > +
> 
> If this actually works, I'd like to have some debug logging for pretty
> much all the code paths here.

Yeah that would be nice for figuring out where the information came 
from or why it was rejected.

> 
> Other than that, seems fine.
> 
> BR,
> Jani.
> 
> >  	dev_priv->vbt.panel_type = panel_type;
> >  
> >  	drrs_mode = (lvds_options->dps_panel_type_bits
> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > index c15718b4862a..5e17fa5dc869 100644
> > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > @@ -1024,3 +1024,16 @@ err_out:
> >  	memunmap(base);
> >  	return err;
> >  }
> > +
> > +int
> > +intel_opregion_get_panel_type(struct drm_device *dev)
> > +{
> > +	u32 panel_details;
> > +	int ret;
> > +
> > +	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return (panel_details >> 8) & 0xff;
> > +}
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Get panel_type from OpRegion panel details

[Jani Nikula]

On Fri, 08 Apr 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Apr 08, 2016 at 05:50:06PM +0300, Jani Nikula wrote:
>> On Fri, 08 Apr 2016, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > We've had problems on several occasions with using the panel type
>> > from the VBT block 40. Usually it seems to be 2, which often
>> > doesn't give us the correct timings for the panel. After some
>> > more digging I found a way to get a panel type via the OpRegion
>> > SWSCI GBDA "Get Panel Details" method. Let's try to use it.
>> >
>> > The spec has this to say about the output:
>> > "Bits [15:8] - Panel Type
>> >  Bits contain the panel type user setting from CMOS
>> >  00h = Not Valid, use default Panel Type & Timings from VBT
>> >  01h - 0Fh = Panel Number"
>> >
>> > The other bits in the output don't look relevant for the problem at
>> > hand.
>> >
>> > The input is specified as:
>> > "Bits [31:4] - Reserved
>> >  Reserved (must be zero)
>> >  Bits [3:0] - Panel Number
>> >  These bits contain the sequential index of Panel, starting at 0 and
>> >  counting upwards from the first integrated Internal Flat-Panel Display
>> >  Encoder present, and then from the first external Display Encoder
>> >  (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
>> >  0h - 0Fh = Panel number"
>> >
>> > For now I've just hardcoded the input panel number as 0. That would seem
>> > like a decent choise for LVDS. Not so sure about eDP when port != A.
>> 
>> Frankly, I didn't understand the point of the input parameter for
>> figuring out the output. The spec is written for someone who already
>> knows what they're doing...
>> 
>> >
>> > Cc: Rob Kramer <rob@solution-space.com>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
>> >  drivers/gpu/drm/i915/intel_bios.c     | 13 ++++++++++---
>> >  drivers/gpu/drm/i915/intel_opregion.c | 13 +++++++++++++
>> >  3 files changed, 28 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > index 53ace572b292..533263a7a12d 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>> >  					 bool enable);
>> >  extern int intel_opregion_notify_adapter(struct drm_device *dev,
>> >  					 pci_power_t state);
>> > +extern int intel_opregion_get_panel_type(struct drm_device *dev);
>> >  #else
>> >  static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
>> >  static inline void intel_opregion_init(struct drm_device *dev) { return; }
>> > @@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
>> >  {
>> >  	return 0;
>> >  }
>> > +static inline int intel_opregion_get_panel_type(struct drm_device *dev)
>> > +{
>> > +	return 0;
>> > +}
>> >  #endif
>> >  
>> >  /* intel_acpi.c */
>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> > index d9568be4b33b..718e49931b5f 100644
>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>> > @@ -205,16 +205,23 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>> >  	struct drm_display_mode *panel_fixed_mode;
>> >  	int panel_type;
>> >  	int drrs_mode;
>> > +	int ret;
>> >  
>> >  	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
>> >  	if (!lvds_options)
>> >  		return;
>> >  
>> >  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
>> > -	if (lvds_options->panel_type > 0xf)
>> > -		return;
>> >  
>> > -	panel_type = lvds_options->panel_type;
>> > +	ret = intel_opregion_get_panel_type(dev_priv->dev);
>> > +	if (ret >= 0x1 && ret <= 0xf) {
>> 
>> Undecided whether I'd like to have this check within
>> intel_opregion_get_panel_type(). Just make that return 0 for "use vbt"
>> (and CONFIG_ACPI=n), and valid stuff otherwise? *shrug*
>
> We don't compile intel_opregion.c when ACPI==n.

Yeah I meant the static inline stub in i915_drv.h for ACPI=n. Which does
return 0.

BR,
Jani.


>
>> 
>> > +		panel_type = ret;
>> > +	} else {
>> > +		if (lvds_options->panel_type > 0xf)
>> > +			return;
>> > +		panel_type = lvds_options->panel_type;
>> > +	}
>> > +
>> 
>> If this actually works, I'd like to have some debug logging for pretty
>> much all the code paths here.
>
> Yeah that would be nice for figuring out where the information came 
> from or why it was rejected.
>
>> 
>> Other than that, seems fine.
>> 
>> BR,
>> Jani.
>> 
>> >  	dev_priv->vbt.panel_type = panel_type;
>> >  
>> >  	drrs_mode = (lvds_options->dps_panel_type_bits
>> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> > index c15718b4862a..5e17fa5dc869 100644
>> > --- a/drivers/gpu/drm/i915/intel_opregion.c
>> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> > @@ -1024,3 +1024,16 @@ err_out:
>> >  	memunmap(base);
>> >  	return err;
>> >  }
>> > +
>> > +int
>> > +intel_opregion_get_panel_type(struct drm_device *dev)
>> > +{
>> > +	u32 panel_details;
>> > +	int ret;
>> > +
>> > +	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	return (panel_details >> 8) & 0xff;
>> > +}
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Get panel_type from OpRegion panel details

[Ville Syrjälä]

On Fri, Apr 08, 2016 at 06:01:41PM +0300, Jani Nikula wrote:
> On Fri, 08 Apr 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Apr 08, 2016 at 05:50:06PM +0300, Jani Nikula wrote:
> >> On Fri, 08 Apr 2016, ville.syrjala@linux.intel.com wrote:
> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> > We've had problems on several occasions with using the panel type
> >> > from the VBT block 40. Usually it seems to be 2, which often
> >> > doesn't give us the correct timings for the panel. After some
> >> > more digging I found a way to get a panel type via the OpRegion
> >> > SWSCI GBDA "Get Panel Details" method. Let's try to use it.
> >> >
> >> > The spec has this to say about the output:
> >> > "Bits [15:8] - Panel Type
> >> >  Bits contain the panel type user setting from CMOS
> >> >  00h = Not Valid, use default Panel Type & Timings from VBT
> >> >  01h - 0Fh = Panel Number"
> >> >
> >> > The other bits in the output don't look relevant for the problem at
> >> > hand.
> >> >
> >> > The input is specified as:
> >> > "Bits [31:4] - Reserved
> >> >  Reserved (must be zero)
> >> >  Bits [3:0] - Panel Number
> >> >  These bits contain the sequential index of Panel, starting at 0 and
> >> >  counting upwards from the first integrated Internal Flat-Panel Display
> >> >  Encoder present, and then from the first external Display Encoder
> >> >  (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
> >> >  0h - 0Fh = Panel number"
> >> >
> >> > For now I've just hardcoded the input panel number as 0. That would seem
> >> > like a decent choise for LVDS. Not so sure about eDP when port != A.
> >> 
> >> Frankly, I didn't understand the point of the input parameter for
> >> figuring out the output. The spec is written for someone who already
> >> knows what they're doing...
> >> 
> >> >
> >> > Cc: Rob Kramer <rob@solution-space.com>
> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
> >> >  drivers/gpu/drm/i915/intel_bios.c     | 13 ++++++++++---
> >> >  drivers/gpu/drm/i915/intel_opregion.c | 13 +++++++++++++
> >> >  3 files changed, 28 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> > index 53ace572b292..533263a7a12d 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> >> >  					 bool enable);
> >> >  extern int intel_opregion_notify_adapter(struct drm_device *dev,
> >> >  					 pci_power_t state);
> >> > +extern int intel_opregion_get_panel_type(struct drm_device *dev);
> >> >  #else
> >> >  static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
> >> >  static inline void intel_opregion_init(struct drm_device *dev) { return; }
> >> > @@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
> >> >  {
> >> >  	return 0;
> >> >  }
> >> > +static inline int intel_opregion_get_panel_type(struct drm_device *dev)
> >> > +{
> >> > +	return 0;
> >> > +}
> >> >  #endif
> >> >  
> >> >  /* intel_acpi.c */
> >> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> > index d9568be4b33b..718e49931b5f 100644
> >> > --- a/drivers/gpu/drm/i915/intel_bios.c
> >> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> > @@ -205,16 +205,23 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
> >> >  	struct drm_display_mode *panel_fixed_mode;
> >> >  	int panel_type;
> >> >  	int drrs_mode;
> >> > +	int ret;
> >> >  
> >> >  	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
> >> >  	if (!lvds_options)
> >> >  		return;
> >> >  
> >> >  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
> >> > -	if (lvds_options->panel_type > 0xf)
> >> > -		return;
> >> >  
> >> > -	panel_type = lvds_options->panel_type;
> >> > +	ret = intel_opregion_get_panel_type(dev_priv->dev);
> >> > +	if (ret >= 0x1 && ret <= 0xf) {
> >> 
> >> Undecided whether I'd like to have this check within
> >> intel_opregion_get_panel_type(). Just make that return 0 for "use vbt"
> >> (and CONFIG_ACPI=n), and valid stuff otherwise? *shrug*
> >
> > We don't compile intel_opregion.c when ACPI==n.
> 
> Yeah I meant the static inline stub in i915_drv.h for ACPI=n. Which does
> return 0.

I'm a bit lost here. Oh, maybe the ACPI=n note just confused me.
Did you just mean that I should do the 0x1-0xf check in
intel_opregion_get_panel_type() already?

> >> > +		panel_type = ret;
> >> > +	} else {
> >> > +		if (lvds_options->panel_type > 0xf)
> >> > +			return;
> >> > +		panel_type = lvds_options->panel_type;
> >> > +	}
> >> > +
> >> 
> >> If this actually works, I'd like to have some debug logging for pretty
> >> much all the code paths here.
> >
> > Yeah that would be nice for figuring out where the information came 
> > from or why it was rejected.
> >
> >> 
> >> Other than that, seems fine.
> >> 
> >> BR,
> >> Jani.
> >> 
> >> >  	dev_priv->vbt.panel_type = panel_type;
> >> >  
> >> >  	drrs_mode = (lvds_options->dps_panel_type_bits
> >> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> >> > index c15718b4862a..5e17fa5dc869 100644
> >> > --- a/drivers/gpu/drm/i915/intel_opregion.c
> >> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> >> > @@ -1024,3 +1024,16 @@ err_out:
> >> >  	memunmap(base);
> >> >  	return err;
> >> >  }
> >> > +
> >> > +int
> >> > +intel_opregion_get_panel_type(struct drm_device *dev)
> >> > +{
> >> > +	u32 panel_details;
> >> > +	int ret;
> >> > +
> >> > +	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
> >> > +	if (ret)
> >> > +		return ret;
> >> > +
> >> > +	return (panel_details >> 8) & 0xff;
> >> > +}
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/3] drm/i915: Get panel_type from OpRegion panel details

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We've had problems on several occasions with using the panel type
from the VBT block 40. Usually it seems to be 2, which often
doesn't give us the correct timings for the panel. After some
more digging I found a way to get a panel type via the OpRegion
SWSCI GBDA "Get Panel Details" method. Let's try to use it.

The spec has this to say about the output:
"Bits [15:8] - Panel Type
 Bits contain the panel type user setting from CMOS
 00h = Not Valid, use default Panel Type & Timings from VBT
 01h - 0Fh = Panel Number"

Another version of the spec lists the valid range as 1-16, which makes
more sense since VBT supports 16 panels. Based on actual results
from Rob's G45, 1-16 is what we need to accept.

The other bits in the output don't look relevant for the problem at
hand.

The input is specified as:
"Bits [31:4] - Reserved
 Reserved (must be zero)
 Bits [3:0] - Panel Number
 These bits contain the sequential index of Panel, starting at 0 and
 counting upwards from the first integrated Internal Flat-Panel Display
 Encoder present, and then from the first external Display Encoder
 (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
 0h - 0Fh = Panel number"

For now I've just hardcoded the input panel number as 0. That would seem
like a decent choise for LVDS. Not so sure about eDP when port != A.

v2: Accept values 1-16
    Filter out bogus results in opregion code (Jani)
    Add debug logging for all the different branches (Jani)

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Rob Kramer <rob@solution-space.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
 drivers/gpu/drm/i915/intel_bios.c     | 20 +++++++++++++++-----
 drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 53ace572b292..42234e17b789 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
 					 bool enable);
 extern int intel_opregion_notify_adapter(struct drm_device *dev,
 					 pci_power_t state);
+extern int intel_opregion_get_panel_type(struct drm_device *dev);
 #else
 static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
 static inline void intel_opregion_init(struct drm_device *dev) { return; }
@@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
 {
 	return 0;
 }
+static inline int intel_opregion_get_panel_type(struct drm_device *dev)
+{
+	return -ENODEV;
+}
 #endif
 
 /* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index d595ca30a7e1..e72dd9a8d6bf 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -205,19 +205,29 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 	struct drm_display_mode *panel_fixed_mode;
 	int panel_type;
 	int drrs_mode;
+	int ret;
 
 	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
 	if (!lvds_options)
 		return;
 
 	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
-	if (lvds_options->panel_type > 0xf) {
-		DRM_DEBUG_KMS("Invalid VBT panel type 0x%x\n",
-			      lvds_options->panel_type);
-		return;
+
+	ret = intel_opregion_get_panel_type(dev_priv->dev);
+	if (ret >= 0) {
+		WARN_ON(ret > 0xf);
+		panel_type = ret;
+		DRM_DEBUG_KMS("Panel type: %d (OpRegion)\n", panel_type);
+	} else {
+		if (lvds_options->panel_type > 0xf) {
+			DRM_DEBUG_KMS("Invalid VBT panel type 0x%x\n",
+				      lvds_options->panel_type);
+			return;
+		}
+		panel_type = lvds_options->panel_type;
+		DRM_DEBUG_KMS("Panel type: %d (VBT)\n", panel_type);
 	}
 
-	panel_type = lvds_options->panel_type;
 	dev_priv->vbt.panel_type = panel_type;
 
 	drrs_mode = (lvds_options->dps_panel_type_bits
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index c15718b4862a..d3c4945a0e36 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -1024,3 +1024,31 @@ err_out:
 	memunmap(base);
 	return err;
 }
+
+int
+intel_opregion_get_panel_type(struct drm_device *dev)
+{
+	u32 panel_details;
+	int ret;
+
+	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
+	if (ret) {
+		DRM_DEBUG_KMS("Failed to get panel details from OpRegion (%d)\n",
+			      ret);
+		return ret;
+	}
+
+	ret = (panel_details >> 8) & 0xff;
+	if (ret > 0x10) {
+		DRM_DEBUG_KMS("Invalid OpRegion panel type 0x%x\n", ret);
+		return -EINVAL;
+	}
+
+	/* fall back to VBT panel type? */
+	if (ret == 0x0) {
+		DRM_DEBUG_KMS("No panel type in OpRegion\n");
+		return -ENODEV;
+	}
+
+	return ret - 1;
+}
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Get panel_type from OpRegion panel details

[Jani Nikula]

On Mon, 11 Apr 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We've had problems on several occasions with using the panel type
> from the VBT block 40. Usually it seems to be 2, which often
> doesn't give us the correct timings for the panel. After some
> more digging I found a way to get a panel type via the OpRegion
> SWSCI GBDA "Get Panel Details" method. Let's try to use it.
>
> The spec has this to say about the output:
> "Bits [15:8] - Panel Type
>  Bits contain the panel type user setting from CMOS
>  00h = Not Valid, use default Panel Type & Timings from VBT
>  01h - 0Fh = Panel Number"
>
> Another version of the spec lists the valid range as 1-16, which makes
> more sense since VBT supports 16 panels. Based on actual results
> from Rob's G45, 1-16 is what we need to accept.
>
> The other bits in the output don't look relevant for the problem at
> hand.
>
> The input is specified as:
> "Bits [31:4] - Reserved
>  Reserved (must be zero)
>  Bits [3:0] - Panel Number
>  These bits contain the sequential index of Panel, starting at 0 and
>  counting upwards from the first integrated Internal Flat-Panel Display
>  Encoder present, and then from the first external Display Encoder
>  (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
>  0h - 0Fh = Panel number"
>
> For now I've just hardcoded the input panel number as 0. That would seem
> like a decent choise for LVDS. Not so sure about eDP when port != A.
>
> v2: Accept values 1-16
>     Filter out bogus results in opregion code (Jani)
>     Add debug logging for all the different branches (Jani)
>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Rob Kramer <rob@solution-space.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

So I don't know if this is the right thing to do or not, and we'll only
find out with testing, but the code looks good to me.

With that caveat,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
>  drivers/gpu/drm/i915/intel_bios.c     | 20 +++++++++++++++-----
>  drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 53ace572b292..42234e17b789 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  					 bool enable);
>  extern int intel_opregion_notify_adapter(struct drm_device *dev,
>  					 pci_power_t state);
> +extern int intel_opregion_get_panel_type(struct drm_device *dev);
>  #else
>  static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
>  static inline void intel_opregion_init(struct drm_device *dev) { return; }
> @@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
>  {
>  	return 0;
>  }
> +static inline int intel_opregion_get_panel_type(struct drm_device *dev)
> +{
> +	return -ENODEV;
> +}
>  #endif
>  
>  /* intel_acpi.c */
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index d595ca30a7e1..e72dd9a8d6bf 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -205,19 +205,29 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  	struct drm_display_mode *panel_fixed_mode;
>  	int panel_type;
>  	int drrs_mode;
> +	int ret;
>  
>  	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
>  	if (!lvds_options)
>  		return;
>  
>  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
> -	if (lvds_options->panel_type > 0xf) {
> -		DRM_DEBUG_KMS("Invalid VBT panel type 0x%x\n",
> -			      lvds_options->panel_type);
> -		return;
> +
> +	ret = intel_opregion_get_panel_type(dev_priv->dev);
> +	if (ret >= 0) {
> +		WARN_ON(ret > 0xf);
> +		panel_type = ret;
> +		DRM_DEBUG_KMS("Panel type: %d (OpRegion)\n", panel_type);
> +	} else {
> +		if (lvds_options->panel_type > 0xf) {
> +			DRM_DEBUG_KMS("Invalid VBT panel type 0x%x\n",
> +				      lvds_options->panel_type);
> +			return;
> +		}
> +		panel_type = lvds_options->panel_type;
> +		DRM_DEBUG_KMS("Panel type: %d (VBT)\n", panel_type);
>  	}
>  
> -	panel_type = lvds_options->panel_type;
>  	dev_priv->vbt.panel_type = panel_type;
>  
>  	drrs_mode = (lvds_options->dps_panel_type_bits
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index c15718b4862a..d3c4945a0e36 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -1024,3 +1024,31 @@ err_out:
>  	memunmap(base);
>  	return err;
>  }
> +
> +int
> +intel_opregion_get_panel_type(struct drm_device *dev)
> +{
> +	u32 panel_details;
> +	int ret;
> +
> +	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
> +	if (ret) {
> +		DRM_DEBUG_KMS("Failed to get panel details from OpRegion (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	ret = (panel_details >> 8) & 0xff;
> +	if (ret > 0x10) {
> +		DRM_DEBUG_KMS("Invalid OpRegion panel type 0x%x\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	/* fall back to VBT panel type? */
> +	if (ret == 0x0) {
> +		DRM_DEBUG_KMS("No panel type in OpRegion\n");
> +		return -ENODEV;
> +	}
> +
> +	return ret - 1;
> +}

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915: Get panel_type from OpRegion panel details

[Ville Syrjälä]

On Mon, Apr 11, 2016 at 11:09:57AM +0300, Jani Nikula wrote:
> On Mon, 11 Apr 2016, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We've had problems on several occasions with using the panel type
> > from the VBT block 40. Usually it seems to be 2, which often
> > doesn't give us the correct timings for the panel. After some
> > more digging I found a way to get a panel type via the OpRegion
> > SWSCI GBDA "Get Panel Details" method. Let's try to use it.
> >
> > The spec has this to say about the output:
> > "Bits [15:8] - Panel Type
> >  Bits contain the panel type user setting from CMOS
> >  00h = Not Valid, use default Panel Type & Timings from VBT
> >  01h - 0Fh = Panel Number"
> >
> > Another version of the spec lists the valid range as 1-16, which makes
> > more sense since VBT supports 16 panels. Based on actual results
> > from Rob's G45, 1-16 is what we need to accept.
> >
> > The other bits in the output don't look relevant for the problem at
> > hand.
> >
> > The input is specified as:
> > "Bits [31:4] - Reserved
> >  Reserved (must be zero)
> >  Bits [3:0] - Panel Number
> >  These bits contain the sequential index of Panel, starting at 0 and
> >  counting upwards from the first integrated Internal Flat-Panel Display
> >  Encoder present, and then from the first external Display Encoder
> >  (e.g., S/DVO-B then S/DVO-C) which supports Internal Flat-Panels.
> >  0h - 0Fh = Panel number"
> >
> > For now I've just hardcoded the input panel number as 0. That would seem
> > like a decent choise for LVDS. Not so sure about eDP when port != A.
> >
> > v2: Accept values 1-16
> >     Filter out bogus results in opregion code (Jani)
> >     Add debug logging for all the different branches (Jani)
> >
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Rob Kramer <rob@solution-space.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94825
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> So I don't know if this is the right thing to do or not, and we'll only
> find out with testing, but the code looks good to me.
> 
> With that caveat,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Entire series pushed to dinq. Thanks for the reviews and testing.

> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h       |  5 +++++
> >  drivers/gpu/drm/i915/intel_bios.c     | 20 +++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++++++++++++++++
> >  3 files changed, 48 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 53ace572b292..42234e17b789 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3421,6 +3421,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> >  					 bool enable);
> >  extern int intel_opregion_notify_adapter(struct drm_device *dev,
> >  					 pci_power_t state);
> > +extern int intel_opregion_get_panel_type(struct drm_device *dev);
> >  #else
> >  static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
> >  static inline void intel_opregion_init(struct drm_device *dev) { return; }
> > @@ -3436,6 +3437,10 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
> >  {
> >  	return 0;
> >  }
> > +static inline int intel_opregion_get_panel_type(struct drm_device *dev)
> > +{
> > +	return -ENODEV;
> > +}
> >  #endif
> >  
> >  /* intel_acpi.c */
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index d595ca30a7e1..e72dd9a8d6bf 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -205,19 +205,29 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
> >  	struct drm_display_mode *panel_fixed_mode;
> >  	int panel_type;
> >  	int drrs_mode;
> > +	int ret;
> >  
> >  	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
> >  	if (!lvds_options)
> >  		return;
> >  
> >  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
> > -	if (lvds_options->panel_type > 0xf) {
> > -		DRM_DEBUG_KMS("Invalid VBT panel type 0x%x\n",
> > -			      lvds_options->panel_type);
> > -		return;
> > +
> > +	ret = intel_opregion_get_panel_type(dev_priv->dev);
> > +	if (ret >= 0) {
> > +		WARN_ON(ret > 0xf);
> > +		panel_type = ret;
> > +		DRM_DEBUG_KMS("Panel type: %d (OpRegion)\n", panel_type);
> > +	} else {
> > +		if (lvds_options->panel_type > 0xf) {
> > +			DRM_DEBUG_KMS("Invalid VBT panel type 0x%x\n",
> > +				      lvds_options->panel_type);
> > +			return;
> > +		}
> > +		panel_type = lvds_options->panel_type;
> > +		DRM_DEBUG_KMS("Panel type: %d (VBT)\n", panel_type);
> >  	}
> >  
> > -	panel_type = lvds_options->panel_type;
> >  	dev_priv->vbt.panel_type = panel_type;
> >  
> >  	drrs_mode = (lvds_options->dps_panel_type_bits
> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > index c15718b4862a..d3c4945a0e36 100644
> > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > @@ -1024,3 +1024,31 @@ err_out:
> >  	memunmap(base);
> >  	return err;
> >  }
> > +
> > +int
> > +intel_opregion_get_panel_type(struct drm_device *dev)
> > +{
> > +	u32 panel_details;
> > +	int ret;
> > +
> > +	ret = swsci(dev, SWSCI_GBDA_PANEL_DETAILS, 0x0, &panel_details);
> > +	if (ret) {
> > +		DRM_DEBUG_KMS("Failed to get panel details from OpRegion (%d)\n",
> > +			      ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = (panel_details >> 8) & 0xff;
> > +	if (ret > 0x10) {
> > +		DRM_DEBUG_KMS("Invalid OpRegion panel type 0x%x\n", ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* fall back to VBT panel type? */
> > +	if (ret == 0x0) {
> > +		DRM_DEBUG_KMS("No panel type in OpRegion\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	return ret - 1;
> > +}
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Reject panel_type > 0xf from VBT

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Reject panel_type > 0xf from VBT
URL   : https://patchwork.freedesktop.org/series/5458/
State : failure

== Summary ==

Series 5458v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5458/revisions/1/mbox/

Test gem_exec_basic:
        Subgroup basic-bsd:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_sync:
        Subgroup basic-bsd:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-c:
                pass       -> INCOMPLETE (bdw-nuci7)
Test pm_rpm:
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:183  pass:170  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:159  dwarn:0   dfail:0   fail:0   skip:37 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:132  dwarn:0   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1844/

949884a57b51aa158e3ae9afe1f08130cdb7a3ef drm-intel-nightly: 2016y-04m-08d-10h-45m-28s UTC integration manifest
b960eee70e97b57ff7a03f51161dfd6aea81b9f8 drm/i915: Get panel_type from OpRegion panel details
734e95750f44ce78ccd687ff7c308bfcb92d075d drm/i915: Replace the static panel_type variable with dev_priv->vbt.panel_type
8d6d67a9694026f62862b6f98296c5c334248ae1 drm/i915: Reject panel_type > 0xf from VBT

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Reject panel_type > 0xf from VBT

[Ville Syrjälä]

On Fri, Apr 08, 2016 at 01:56:15PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/3] drm/i915: Reject panel_type > 0xf from VBT
> URL   : https://patchwork.freedesktop.org/series/5458/
> State : failure
> 
> == Summary ==
> 
> Series 5458v1 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/5458/revisions/1/mbox/
> 
> Test gem_exec_basic:
>         Subgroup basic-bsd:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> Test gem_sync:
>         Subgroup basic-bsd:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> Test kms_pipe_crc_basic:
>         Subgroup hang-read-crc-pipe-c:
>                 pass       -> INCOMPLETE (bdw-nuci7)

Machine died or something? Last thing in the log

[  518.517277] kms_pipe_crc_basic: starting subtest hang-read-crc-pipe-C
[  518.517713] [drm:i915_ring_stop_set] Stopping rings 0x00000001
[  519.708140] [drm:intel_print_rc6_info] Enabling RC6 states: RC6 on

> Test pm_rpm:
>         Subgroup basic-rte:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> 
> bdw-nuci7        total:183  pass:170  dwarn:0   dfail:0   fail:0   skip:12 
> bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
> bsw-nuc-2        total:196  pass:159  dwarn:0   dfail:0   fail:0   skip:37 
> byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
> hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
> hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
> ilk-hp8440p      total:196  pass:132  dwarn:0   dfail:0   fail:0   skip:64 
> ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
> skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
> skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
> snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
> snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1844/
> 
> 949884a57b51aa158e3ae9afe1f08130cdb7a3ef drm-intel-nightly: 2016y-04m-08d-10h-45m-28s UTC integration manifest
> b960eee70e97b57ff7a03f51161dfd6aea81b9f8 drm/i915: Get panel_type from OpRegion panel details
> 734e95750f44ce78ccd687ff7c308bfcb92d075d drm/i915: Replace the static panel_type variable with dev_priv->vbt.panel_type
> 8d6d67a9694026f62862b6f98296c5c334248ae1 drm/i915: Reject panel_type > 0xf from VBT

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Reject panel_type > 0xf from VBT

[Jani Nikula]

On Fri, 08 Apr 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> VBT can only contain 16 panel entries, indexed with the panel_type.
> To play it safe we should reject panel_type > 0xf, so that we don't
> read past the valid data.
>
> Cc: Rob Kramer <rob@solution-space.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index eb756c41d9e1..13bdd4316092 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -212,7 +212,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  		return;
>  
>  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
> -	if (lvds_options->panel_type == 0xff)
> +	if (lvds_options->panel_type > 0xf)

Debug logging on this?

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



>  		return;
>  
>  	panel_type = lvds_options->panel_type;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/3] drm/i915: Reject panel_type > 0xf from VBT

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VBT can only contain 16 panel entries, indexed with the panel_type.
To play it safe we should reject panel_type > 0xf, so that we don't
read past the valid data.

v2: Add debug logging (Jani)

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Rob Kramer <rob@solution-space.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com> (v1)
---
 drivers/gpu/drm/i915/intel_bios.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index eb756c41d9e1..c8857b5dbfec 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -212,8 +212,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 		return;
 
 	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
-	if (lvds_options->panel_type == 0xff)
+	if (lvds_options->panel_type > 0xf) {
+		DRM_DEBUG_KMS("Invalid VBT panel type 0x%x\n",
+			      lvds_options->panel_type);
 		return;
+	}
 
 	panel_type = lvds_options->panel_type;
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Reject panel_type > 0xf from VBT

[Jani Nikula]

On Mon, 11 Apr 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> VBT can only contain 16 panel entries, indexed with the panel_type.
> To play it safe we should reject panel_type > 0xf, so that we don't
> read past the valid data.
>
> v2: Add debug logging (Jani)
>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Rob Kramer <rob@solution-space.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com> (v1)

Yup.

> ---
>  drivers/gpu/drm/i915/intel_bios.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index eb756c41d9e1..c8857b5dbfec 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -212,8 +212,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  		return;
>  
>  	dev_priv->vbt.lvds_dither = lvds_options->pixel_dither;
> -	if (lvds_options->panel_type == 0xff)
> +	if (lvds_options->panel_type > 0xf) {
> +		DRM_DEBUG_KMS("Invalid VBT panel type 0x%x\n",
> +			      lvds_options->panel_type);
>  		return;
> +	}
>  
>  	panel_type = lvds_options->panel_type;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [v2,1/3] drm/i915: Reject panel_type > 0xf from VBT (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Reject panel_type > 0xf from VBT (rev3)
URL   : https://patchwork.freedesktop.org/series/5458/
State : failure

== Summary ==

Series 5458v3 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5458/revisions/3/mbox/

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (bsw-nuc-2)
                dmesg-warn -> PASS       (ilk-hp8440p)
Test gem_storedw_loop:
        Subgroup basic-default:
                pass       -> DMESG-FAIL (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:157  dwarn:1   dfail:1   fail:0   skip:37 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:132  dwarn:0   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1857/

56aa709c9614bf7f39ee255fd0ddf4f1b2743387 drm-intel-nightly: 2016y-04m-09d-11h-10m-49s UTC integration manifest
bf70c98572c65ec3f1f5251266761bb760dd8dd5 drm/i915: Get panel_type from OpRegion panel details
51669c5721760b80ec9cbe6c29a3ff24ef29c34b drm/i915: Replace the static panel_type variable with dev_priv->vbt.panel_type
122e188b6ff062ec5fb4908db63f246c8df1cef9 drm/i915: Reject panel_type > 0xf from VBT

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx