[PATCH 0/8] drm: Fix/remove a few static checker error

[PATCH 0/8] drm: Fix/remove a few static checker error

[Imre Deak]

The patchset fixes static checker errors all over the place. All of them
are minor, mostly missing error return checks, with the exception of the
first which could fix a real use case.

Imre Deak (8):
  drm/i915/vlv: Fix port B PLL opamp initialization
  drm/i915/dp: Check error return during DPCD capability queries
  drm/i915/sdvo: Check error return from intel_sdvo_get_value()
  drm/i915: Check error return when setting DMA mask
  drm/i915: Check error return when converting pipe to connector
  drm/i915: Sanitize stolen memory size calculation
  drm/i915/lvds: Remove magic from PLL programming
  drm: Remove redundant NULL check during atomic plane commit

 drivers/gpu/drm/drm_plane_helper.c   | 10 ++++------
 drivers/gpu/drm/i915/dvo_ch7017.c    |  7 +++++--
 drivers/gpu/drm/i915/i915_gem_gtt.c  | 28 +++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_display.c |  4 ++--
 drivers/gpu/drm/i915/intel_dp.c      | 20 ++++++++++++--------
 drivers/gpu/drm/i915/intel_panel.c   | 14 ++++++++++++--
 drivers/gpu/drm/i915/intel_sdvo.c    |  8 ++++----
 7 files changed, 56 insertions(+), 35 deletions(-)

-- 
2.5.0

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

[PATCH 1/8] drm/i915/vlv: Fix port B PLL opamp initialization

[Imre Deak]

The current code looks like a typo, the specification calls for setting
bits 31:24 to 0x8C, while preserving bits 23:0. Fix things accordingly.

I'm not aware of the typo causing a real problem, so the fix is only for
consistency.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 85b9e2f5..19a7a1e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6369,8 +6369,8 @@ static void vlv_pllb_recal_opamp(struct drm_i915_private *dev_priv, enum pipe
 	vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW9(1), reg_val);
 
 	reg_val = vlv_dpio_read(dev_priv, pipe, VLV_REF_DW13);
-	reg_val &= 0x8cffffff;
-	reg_val = 0x8c000000;
+	reg_val &= 0x00ffffff;
+	reg_val |= 0x8c000000;
 	vlv_dpio_write(dev_priv, pipe, VLV_REF_DW13, reg_val);
 
 	reg_val = vlv_dpio_read(dev_priv, pipe, VLV_PLL_DW9(1));
-- 
2.5.0

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

[PATCH 2/8] drm/i915/dp: Check error return during DPCD capability queries

[Imre Deak]

The assumptions of these users of drm_dp_dpcd_readb() is that the passed
in output buffer won't change in case of error, but this isn't
guaranteed. Fix this by treating any error as the lack of the given
capability.

In case of DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP an error would leave the
buffer uninitialized even with the above assumption.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 08834f7..4a6feb6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3088,7 +3088,8 @@ static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
 {
 	uint8_t psr_caps = 0;
 
-	drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps);
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps) != 1)
+		return false;
 	return psr_caps & DP_PSR2_SU_Y_COORDINATE_REQUIRED;
 }
 
@@ -3096,9 +3097,9 @@ static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
 {
 	uint8_t dprx = 0;
 
-	drm_dp_dpcd_readb(&intel_dp->aux,
-			DP_DPRX_FEATURE_ENUMERATION_LIST,
-			&dprx);
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
+			      &dprx) != 1)
+		return false;
 	return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
 }
 
@@ -3106,7 +3107,9 @@ static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
 {
 	uint8_t alpm_caps = 0;
 
-	drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, &alpm_caps);
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP,
+			      &alpm_caps) != 1)
+		return false;
 	return alpm_caps & DP_ALPM_CAP;
 }
 
@@ -3679,9 +3682,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 		uint8_t frame_sync_cap;
 
 		dev_priv->psr.sink_support = true;
-		drm_dp_dpcd_readb(&intel_dp->aux,
-				  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
-				  &frame_sync_cap);
+		if (drm_dp_dpcd_readb(&intel_dp->aux,
+				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
+				      &frame_sync_cap) != 1)
+			frame_sync_cap = 0;
 		dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
 		/* PSR2 needs frame sync as well */
 		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
-- 
2.5.0

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

[PATCH 3/8] drm/i915/sdvo: Check error return from intel_sdvo_get_value()

[Imre Deak]

The current code assumes that 'enhancements' won't change in case of an
error, but this isn't guaranteed. Fix things by treating any error as a
lack of the given capability.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 816a6f5..0d1c511 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2893,10 +2893,10 @@ static bool intel_sdvo_create_enhance_property(struct intel_sdvo *intel_sdvo,
 	BUILD_BUG_ON(sizeof(enhancements) != 2);
 
 	enhancements.response = 0;
-	intel_sdvo_get_value(intel_sdvo,
-			     SDVO_CMD_GET_SUPPORTED_ENHANCEMENTS,
-			     &enhancements, sizeof(enhancements));
-	if (enhancements.response == 0) {
+	if (!intel_sdvo_get_value(intel_sdvo,
+				  SDVO_CMD_GET_SUPPORTED_ENHANCEMENTS,
+				  &enhancements, sizeof(enhancements)) ||
+	    enhancements.response == 0) {
 		DRM_DEBUG_KMS("No enhancement is supported\n");
 		return true;
 	}
-- 
2.5.0

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

[PATCH 4/8] drm/i915: Check error return when setting DMA mask

[Imre Deak]

Even though an error from these functions isn't fatal we still want to
have a diagnostic message about it.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8bab4ae..13bf099 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2741,13 +2741,16 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	unsigned int size;
 	u16 snb_gmch_ctl;
+	int err;
 
 	/* TODO: We're not aware of mappable constraints on gen8 yet */
 	ggtt->mappable_base = pci_resource_start(pdev, 2);
 	ggtt->mappable_end = pci_resource_len(pdev, 2);
 
-	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(39)))
-		pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
+	if (!(err = pci_set_dma_mask(pdev, DMA_BIT_MASK(39))))
+		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
+	if (err)
+		DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", err);
 
 	pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
 
@@ -2790,6 +2793,7 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	unsigned int size;
 	u16 snb_gmch_ctl;
+	int err;
 
 	ggtt->mappable_base = pci_resource_start(pdev, 2);
 	ggtt->mappable_end = pci_resource_len(pdev, 2);
@@ -2802,8 +2806,10 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
 		return -ENXIO;
 	}
 
-	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(40)))
-		pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
+	if (!(err = pci_set_dma_mask(pdev, DMA_BIT_MASK(40))))
+		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
+	if (err)
+		DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", err);
 	pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
 
 	ggtt->stolen_size = gen6_get_stolen_size(snb_gmch_ctl);
-- 
2.5.0

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

[PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

An error from intel_get_pipe_from_connector() would mean a bug somewhere
else, but we still should check for it to prevent some other more
obscure bug later.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index cb50c52..dbe05ec 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
-	enum transcoder cpu_transcoder =
-		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+	enum transcoder cpu_transcoder;
 	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
 
+	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
+		return;
+
+	cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+
 	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
 	if (cpu_ctl2 & BLM_PWM_ENABLE) {
 		DRM_DEBUG_KMS("cpu backlight already enabled\n");
@@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 ctl, ctl2, freq;
 
+	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
+		return;
+
 	ctl2 = I915_READ(BLC_PWM_CTL2);
 	if (ctl2 & BLM_PWM_ENABLE) {
 		DRM_DEBUG_KMS("backlight already enabled\n");
@@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
 	if (!panel->backlight.present)
 		return;
 
+	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
+		return;
+
 	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
 
 	mutex_lock(&dev_priv->backlight_lock);
-- 
2.5.0

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

[PATCH 6/8] drm/i915: Sanitize stolen memory size calculation

[Imre Deak]

On GEN8+ (not counting CHV) the calculation can in theory result in an
incorrect sign extension with all upper bits set. In practice this is
unlikely to happen since it would require 4GB of stolen memory set
aside. For consistency still prevent the sign extension explicitly
everywhere.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 13bf099..4b764b0 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2577,14 +2577,14 @@ static size_t gen6_get_stolen_size(u16 snb_gmch_ctl)
 {
 	snb_gmch_ctl >>= SNB_GMCH_GMS_SHIFT;
 	snb_gmch_ctl &= SNB_GMCH_GMS_MASK;
-	return snb_gmch_ctl << 25; /* 32 MB units */
+	return (size_t)snb_gmch_ctl << 25; /* 32 MB units */
 }
 
 static size_t gen8_get_stolen_size(u16 bdw_gmch_ctl)
 {
 	bdw_gmch_ctl >>= BDW_GMCH_GMS_SHIFT;
 	bdw_gmch_ctl &= BDW_GMCH_GMS_MASK;
-	return bdw_gmch_ctl << 25; /* 32 MB units */
+	return (size_t)bdw_gmch_ctl << 25; /* 32 MB units */
 }
 
 static size_t chv_get_stolen_size(u16 gmch_ctrl)
@@ -2598,11 +2598,11 @@ static size_t chv_get_stolen_size(u16 gmch_ctrl)
 	 * 0x17 to 0x1d: 4MB increments start at 36MB
 	 */
 	if (gmch_ctrl < 0x11)
-		return gmch_ctrl << 25;
+		return (size_t)gmch_ctrl << 25;
 	else if (gmch_ctrl < 0x17)
-		return (gmch_ctrl - 0x11 + 2) << 22;
+		return (size_t)(gmch_ctrl - 0x11 + 2) << 22;
 	else
-		return (gmch_ctrl - 0x17 + 9) << 22;
+		return (size_t)(gmch_ctrl - 0x17 + 9) << 22;
 }
 
 static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl)
@@ -2611,10 +2611,10 @@ static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl)
 	gen9_gmch_ctl &= BDW_GMCH_GMS_MASK;
 
 	if (gen9_gmch_ctl < 0xf0)
-		return gen9_gmch_ctl << 25; /* 32 MB units */
+		return (size_t)gen9_gmch_ctl << 25; /* 32 MB units */
 	else
 		/* 4MB increments starting at 0xf0 for 4MB */
-		return (gen9_gmch_ctl - 0xf0 + 1) << 22;
+		return (size_t)(gen9_gmch_ctl - 0xf0 + 1) << 22;
 }
 
 static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
-- 
2.5.0

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

[PATCH 7/8] drm/i915/lvds: Remove magic from PLL programming

[Imre Deak]

This looks like a left-over from enabling work. I don't have the
specification to check whether we have to set
CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED, for now just keep things
as-is, removing the magic so that static checkers don't complain.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/dvo_ch7017.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c
index b3c7c19..c0712a5 100644
--- a/drivers/gpu/drm/i915/dvo_ch7017.c
+++ b/drivers/gpu/drm/i915/dvo_ch7017.c
@@ -280,10 +280,13 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo,
 			(0 << CH7017_PHASE_DETECTOR_SHIFT);
 	} else {
 		outputs_enable = CH7017_LVDS_CHANNEL_A | CH7017_CHARGE_PUMP_HIGH;
-		lvds_pll_feedback_div = CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
+		/*
+		 * FIXME: Check if CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED
+		 * needs to be also set for the following.
+		 */
+		lvds_pll_feedback_div =
 			(2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) |
 			(3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT);
-		lvds_pll_feedback_div = 35;
 		lvds_control_2 = (3 << CH7017_LOOP_FILTER_SHIFT) |
 			(0 << CH7017_PHASE_DETECTOR_SHIFT);
 		if (1) { /* XXX: dual channel panel detection.  Assume yes for now. */
-- 
2.5.0

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

[PATCH 8/8] drm: Remove redundant NULL check during atomic plane commit

[Imre Deak]

plane_state can't be NULL anywhere in this function, so the NULL check
at the end is redundant, remove it.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 9854a50..2c27f6f 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -511,12 +511,10 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 	if (plane_funcs->cleanup_fb)
 		plane_funcs->cleanup_fb(plane, plane_state);
 out:
-	if (plane_state) {
-		if (plane->funcs->atomic_destroy_state)
-			plane->funcs->atomic_destroy_state(plane, plane_state);
-		else
-			drm_atomic_helper_plane_destroy_state(plane, plane_state);
-	}
+	if (plane->funcs->atomic_destroy_state)
+		plane->funcs->atomic_destroy_state(plane, plane_state);
+	else
+		drm_atomic_helper_plane_destroy_state(plane, plane_state);
 
 	return ret;
 }
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] drm/i915: Check error return when setting DMA mask

[Jani Nikula]

On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> Even though an error from these functions isn't fatal we still want to
> have a diagnostic message about it.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8bab4ae..13bf099 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2741,13 +2741,16 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	unsigned int size;
>  	u16 snb_gmch_ctl;
> +	int err;
>  
>  	/* TODO: We're not aware of mappable constraints on gen8 yet */
>  	ggtt->mappable_base = pci_resource_start(pdev, 2);
>  	ggtt->mappable_end = pci_resource_len(pdev, 2);
>  
> -	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(39)))
> -		pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
> +	if (!(err = pci_set_dma_mask(pdev, DMA_BIT_MASK(39))))

Please split out the assignment to a line of its own, in both places.

BR,
Jani.

> +		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
> +	if (err)
> +		DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", err);
>  
>  	pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
>  
> @@ -2790,6 +2793,7 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	unsigned int size;
>  	u16 snb_gmch_ctl;
> +	int err;
>  
>  	ggtt->mappable_base = pci_resource_start(pdev, 2);
>  	ggtt->mappable_end = pci_resource_len(pdev, 2);
> @@ -2802,8 +2806,10 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
>  		return -ENXIO;
>  	}
>  
> -	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(40)))
> -		pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
> +	if (!(err = pci_set_dma_mask(pdev, DMA_BIT_MASK(40))))
> +		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
> +	if (err)
> +		DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", err);
>  	pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
>  
>  	ggtt->stolen_size = gen6_get_stolen_size(snb_gmch_ctl);

-- 
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 5/8] drm/i915: Check error return when converting pipe to connector

[Jani Nikula]

On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> An error from intel_get_pipe_from_connector() would mean a bug somewhere
> else, but we still should check for it to prevent some other more
> obscure bug later.

Do check for invalid pipe, but please just limp on instead of bailing
out of the functions. See notes inline.

BR,
Jani.

>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb50c52..dbe05ec 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> -	enum transcoder cpu_transcoder =
> -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +	enum transcoder cpu_transcoder;
>  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>  
> +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		return;

Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case,
go on, and hope for the best. Do leave the warn in place though.

> +
> +	cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +
>  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
> @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 ctl, ctl2, freq;
>  
> +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		return;

Same here, please just assign e.g. PIPE_A here with the warning, and
limp on.

> +
>  	ctl2 = I915_READ(BLC_PWM_CTL2);
>  	if (ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("backlight already enabled\n");
> @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  	if (!panel->backlight.present)
>  		return;
>  
> +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		return;

Here, pipe is only used for the debug logging, just skip that instead.

> +
>  	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>  
>  	mutex_lock(&dev_priv->backlight_lock);

-- 
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 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote:
> On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
> > else, but we still should check for it to prevent some other more
> > obscure bug later.
> 
> Do check for invalid pipe, but please just limp on instead of bailing
> out of the functions. See notes inline.
> 
> BR,
> Jani.
> 
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index cb50c52..dbe05ec 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> >  	struct intel_panel *panel = &connector->panel;
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> > -	enum transcoder cpu_transcoder =
> > -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > +	enum transcoder cpu_transcoder;
> >  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >  
> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		return;
> 
> Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case,
> go on, and hope for the best. Do leave the warn in place though.
> 
> > +
> > +	cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > +
> >  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
> >  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  	u32 ctl, ctl2, freq;
> >  
> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		return;
> 
> Same here, please just assign e.g. PIPE_A here with the warning, and
> limp on.
> 
> > +
> >  	ctl2 = I915_READ(BLC_PWM_CTL2);
> >  	if (ctl2 & BLM_PWM_ENABLE) {
> >  		DRM_DEBUG_KMS("backlight already enabled\n");
> > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
> >  	if (!panel->backlight.present)
> >  		return;
> >  
> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		return;
> 
> Here, pipe is only used for the debug logging, just skip that instead.

Ok, can do. This would make things inconsistent wrt.
vlv_enable/disable_backlight() though, so are you ok if I change those
too accordingly?

--Imre

> 
> > +
> >  	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> >  
> >  	mutex_lock(&dev_priv->backlight_lock);
> 
> -- 
> 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: success for drm: Fix/remove a few static checker error

[Patchwork]

== Series Details ==

Series: drm: Fix/remove a few static checker error
URL   : https://patchwork.freedesktop.org/series/23571/
State : success

== Summary ==

Series 23571v1 drm: Fix/remove a few static checker error
https://patchwork.freedesktop.org/api/1.0/series/23571/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:433s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:432s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:579s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:514s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:549s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:484s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:488s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:413s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:406s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:416s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:488s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:464s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:456s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:574s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:449s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:568s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:463s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:490s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:429s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:531s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:405s

9f955adc7ed1bbf9ada7dab63f39336a56ff1985 drm-tip: 2017y-04m-26d-13h-38m-52s UTC integration manifest
7ae111c drm: Remove redundant NULL check during atomic plane commit
c741171 drm/i915/lvds: Remove magic from PLL programming
a093053 drm/i915: Sanitize stolen memory size calculation
50f6722 drm/i915: Check error return when converting pipe to connector
96ec98c drm/i915: Check error return when setting DMA mask
0ff3bb1 drm/i915/sdvo: Check error return from intel_sdvo_get_value()
9a1b175 drm/i915/dp: Check error return during DPCD capability queries
cfccd23 drm/i915/vlv: Fix port B PLL opamp initialization

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4557/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/i915/lvds: Remove magic from PLL programming

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 04:40:12PM +0300, Imre Deak wrote:
> This looks like a left-over from enabling work. I don't have the
> specification to check whether we have to set
> CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED, for now just keep things
> as-is, removing the magic so that static checkers don't complain.

The spec does list the top two bits as reserved with the default value
of 10b. I don't see any mention of how reserved bits should be handled
though. But I think I'd just change it to set them to the default value.

The whole thing just looks like an oversight in the original ddx commit,
whose commit message isn't all that helpful:

commit 04e936935f0b0045600241424f1d04a6721a2432
Author: Eric Anholt <eric@anholt.net>
Date:   Mon Oct 1 17:29:35 2007 -0700

    Bring the CH7017 driver closer to spec.
    
    This is also closer to what my hardware is programmed with, except for some
    very confusing off-by-one bugs in an unexpected direction.

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/dvo_ch7017.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c
> index b3c7c19..c0712a5 100644
> --- a/drivers/gpu/drm/i915/dvo_ch7017.c
> +++ b/drivers/gpu/drm/i915/dvo_ch7017.c
> @@ -280,10 +280,13 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo,
>  			(0 << CH7017_PHASE_DETECTOR_SHIFT);
>  	} else {
>  		outputs_enable = CH7017_LVDS_CHANNEL_A | CH7017_CHARGE_PUMP_HIGH;
> -		lvds_pll_feedback_div = CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
> +		/*
> +		 * FIXME: Check if CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED
> +		 * needs to be also set for the following.
> +		 */
> +		lvds_pll_feedback_div =
>  			(2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) |
>  			(3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT);
> -		lvds_pll_feedback_div = 35;
>  		lvds_control_2 = (3 << CH7017_LOOP_FILTER_SHIFT) |
>  			(0 << CH7017_PHASE_DETECTOR_SHIFT);
>  		if (1) { /* XXX: dual channel panel detection.  Assume yes for now. */
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Jani Nikula]

On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote:
>> On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
>> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
>> > else, but we still should check for it to prevent some other more
>> > obscure bug later.
>> 
>> Do check for invalid pipe, but please just limp on instead of bailing
>> out of the functions. See notes inline.
>> 
>> BR,
>> Jani.
>> 
>> >
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++--
>> >  1 file changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> > index cb50c52..dbe05ec 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
>> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> >  	struct intel_panel *panel = &connector->panel;
>> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> > -	enum transcoder cpu_transcoder =
>> > -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>> > +	enum transcoder cpu_transcoder;
>> >  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>> >  
>> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +		return;
>> 
>> Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case,
>> go on, and hope for the best. Do leave the warn in place though.
>> 
>> > +
>> > +	cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>> > +
>> >  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>> >  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
>> >  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
>> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
>> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> >  	u32 ctl, ctl2, freq;
>> >  
>> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +		return;
>> 
>> Same here, please just assign e.g. PIPE_A here with the warning, and
>> limp on.
>> 
>> > +
>> >  	ctl2 = I915_READ(BLC_PWM_CTL2);
>> >  	if (ctl2 & BLM_PWM_ENABLE) {
>> >  		DRM_DEBUG_KMS("backlight already enabled\n");
>> > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>> >  	if (!panel->backlight.present)
>> >  		return;
>> >  
>> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +		return;
>> 
>> Here, pipe is only used for the debug logging, just skip that instead.
>
> Ok, can do. This would make things inconsistent wrt.
> vlv_enable/disable_backlight() though, so are you ok if I change those
> too accordingly?

Those are different in the sense that the *registers* are chosen based
on the pipe, so it *must* be one of A or B. But in the paths changed
here, pipe is only used for some bit fields of the registers being
updated. IIRC they are not even crucial for fundamental operation of the
backlight. So I don't want to bail out unless we really can't proceed.

BR,
Jani.


>
> --Imre
>
>> 
>> > +
>> >  	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>> >  
>> >  	mutex_lock(&dev_priv->backlight_lock);
>> 
>> -- 
>> 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 1/8] drm/i915/vlv: Fix port B PLL opamp initialization

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 04:40:06PM +0300, Imre Deak wrote:
> The current code looks like a typo, the specification calls for setting
> bits 31:24 to 0x8C, while preserving bits 23:0. Fix things accordingly.

Yeah, as we checked there were a couple of things in the low 24 bits
with non-zero default values.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> I'm not aware of the typo causing a real problem, so the fix is only for
> consistency.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 85b9e2f5..19a7a1e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6369,8 +6369,8 @@ static void vlv_pllb_recal_opamp(struct drm_i915_private *dev_priv, enum pipe
>  	vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW9(1), reg_val);
>  
>  	reg_val = vlv_dpio_read(dev_priv, pipe, VLV_REF_DW13);
> -	reg_val &= 0x8cffffff;
> -	reg_val = 0x8c000000;
> +	reg_val &= 0x00ffffff;
> +	reg_val |= 0x8c000000;
>  	vlv_dpio_write(dev_priv, pipe, VLV_REF_DW13, reg_val);
>  
>  	reg_val = vlv_dpio_read(dev_priv, pipe, VLV_PLL_DW9(1));
> -- 
> 2.5.0

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

Re: [PATCH 7/8] drm/i915/lvds: Remove magic from PLL programming

[Imre Deak]

On Wed, Apr 26, 2017 at 05:50:06PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2017 at 04:40:12PM +0300, Imre Deak wrote:
> > This looks like a left-over from enabling work. I don't have the
> > specification to check whether we have to set
> > CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED, for now just keep things
> > as-is, removing the magic so that static checkers don't complain.
> 
> The spec does list the top two bits as reserved with the default value
> of 10b. I don't see any mention of how reserved bits should be handled
> though. But I think I'd just change it to set them to the default value.

Ok, can change that based on the above.

--Imre

> 
> The whole thing just looks like an oversight in the original ddx commit,
> whose commit message isn't all that helpful:
> 
> commit 04e936935f0b0045600241424f1d04a6721a2432
> Author: Eric Anholt <eric@anholt.net>
> Date:   Mon Oct 1 17:29:35 2007 -0700
> 
>     Bring the CH7017 driver closer to spec.
>     
>     This is also closer to what my hardware is programmed with, except for some
>     very confusing off-by-one bugs in an unexpected direction.
> 
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/dvo_ch7017.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c
> > index b3c7c19..c0712a5 100644
> > --- a/drivers/gpu/drm/i915/dvo_ch7017.c
> > +++ b/drivers/gpu/drm/i915/dvo_ch7017.c
> > @@ -280,10 +280,13 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo,
> >  			(0 << CH7017_PHASE_DETECTOR_SHIFT);
> >  	} else {
> >  		outputs_enable = CH7017_LVDS_CHANNEL_A | CH7017_CHARGE_PUMP_HIGH;
> > -		lvds_pll_feedback_div = CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
> > +		/*
> > +		 * FIXME: Check if CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED
> > +		 * needs to be also set for the following.
> > +		 */
> > +		lvds_pll_feedback_div =
> >  			(2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) |
> >  			(3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT);
> > -		lvds_pll_feedback_div = 35;
> >  		lvds_control_2 = (3 << CH7017_LOOP_FILTER_SHIFT) |
> >  			(0 << CH7017_PHASE_DETECTOR_SHIFT);
> >  		if (1) { /* XXX: dual channel panel detection.  Assume yes for now. */
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915/dp: Check error return during DPCD capability queries

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 04:40:07PM +0300, Imre Deak wrote:
> The assumptions of these users of drm_dp_dpcd_readb() is that the passed
> in output buffer won't change in case of error, but this isn't
> guaranteed.

Hmm. We blindly copy as many bytes from the rxbuf into the user
provided buffer as the hardware told us to. And whether the transfer
actually is considered a success or not depends on what the first
received byte says. I don't recall what the DP spec really says about
replying with more than one byte on failure, but I guess we shouldn't
depend on it anyway.

We could actually make that guarantee if we moved txbuf+rxbuf up
into drm_dp_dpcd_access() and did the copy to the user buffer
only on succes. But that would require changing all the DP
capable drivers at the same time, so would require someone
motivated enough.

In the meantime
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> Fix this by treating any error as the lack of the given
> capability.
> 
> In case of DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP an error would leave the
> buffer uninitialized even with the above assumption.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 08834f7..4a6feb6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3088,7 +3088,8 @@ static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
>  {
>  	uint8_t psr_caps = 0;
>  
> -	drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps);
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps) != 1)
> +		return false;
>  	return psr_caps & DP_PSR2_SU_Y_COORDINATE_REQUIRED;
>  }
>  
> @@ -3096,9 +3097,9 @@ static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
>  {
>  	uint8_t dprx = 0;
>  
> -	drm_dp_dpcd_readb(&intel_dp->aux,
> -			DP_DPRX_FEATURE_ENUMERATION_LIST,
> -			&dprx);
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
> +			      &dprx) != 1)
> +		return false;
>  	return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
>  }
>  
> @@ -3106,7 +3107,9 @@ static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
>  {
>  	uint8_t alpm_caps = 0;
>  
> -	drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, &alpm_caps);
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP,
> +			      &alpm_caps) != 1)
> +		return false;
>  	return alpm_caps & DP_ALPM_CAP;
>  }
>  
> @@ -3679,9 +3682,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>  		uint8_t frame_sync_cap;
>  
>  		dev_priv->psr.sink_support = true;
> -		drm_dp_dpcd_readb(&intel_dp->aux,
> -				  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> -				  &frame_sync_cap);
> +		if (drm_dp_dpcd_readb(&intel_dp->aux,
> +				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> +				      &frame_sync_cap) != 1)
> +			frame_sync_cap = 0;
>  		dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
>  		/* PSR2 needs frame sync as well */
>  		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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/8] drm/i915/sdvo: Check error return from intel_sdvo_get_value()

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 04:40:08PM +0300, Imre Deak wrote:
> The current code assumes that 'enhancements' won't change in case of an
> error, but this isn't guaranteed. Fix things by treating any error as a
> lack of the given capability.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 816a6f5..0d1c511 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2893,10 +2893,10 @@ static bool intel_sdvo_create_enhance_property(struct intel_sdvo *intel_sdvo,
>  	BUILD_BUG_ON(sizeof(enhancements) != 2);
>  
>  	enhancements.response = 0;

This initialization could be removed if we don't use the thing
when the read fails.

Either way
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> -	intel_sdvo_get_value(intel_sdvo,
> -			     SDVO_CMD_GET_SUPPORTED_ENHANCEMENTS,
> -			     &enhancements, sizeof(enhancements));
> -	if (enhancements.response == 0) {
> +	if (!intel_sdvo_get_value(intel_sdvo,
> +				  SDVO_CMD_GET_SUPPORTED_ENHANCEMENTS,
> +				  &enhancements, sizeof(enhancements)) ||
> +	    enhancements.response == 0) {
>  		DRM_DEBUG_KMS("No enhancement is supported\n");
>  		return true;
>  	}
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/8] drm/i915/dp: Check error return during DPCD capability queries

[Imre Deak]

On Wed, Apr 26, 2017 at 06:08:24PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2017 at 04:40:07PM +0300, Imre Deak wrote:
> > The assumptions of these users of drm_dp_dpcd_readb() is that the passed
> > in output buffer won't change in case of error, but this isn't
> > guaranteed.
> 
> Hmm. We blindly copy as many bytes from the rxbuf into the user
> provided buffer as the hardware told us to. 

Haven't checked this, but now looking at it doesn't the 
'bytes > recv_size' check in intel_dp_aux_ch() bound that?

> And whether the transfer
> actually is considered a success or not depends on what the first
> received byte says. I don't recall what the DP spec really says about
> replying with more than one byte on failure, but I guess we shouldn't
> depend on it anyway.
> 
> We could actually make that guarantee if we moved txbuf+rxbuf up
> into drm_dp_dpcd_access() and did the copy to the user buffer
> only on succes. But that would require changing all the DP
> capable drivers at the same time, so would require someone
> motivated enough.

Ok. I stopped at the ZR24w workaround in drm_dp_dpcd_read().. But that
could also be used with a separate buffer. In any case I thought that
even with the guarantee that the buffer doesn't change - which is in
general a reasonable assumption - checking for the error return would
be more robust.

> 
> In the meantime
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks.

> 
> > Fix this by treating any error as the lack of the given
> > capability.
> > 
> > In case of DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP an error would leave the
> > buffer uninitialized even with the above assumption.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 08834f7..4a6feb6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3088,7 +3088,8 @@ static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t psr_caps = 0;
> >  
> > -	drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps);
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps) != 1)
> > +		return false;
> >  	return psr_caps & DP_PSR2_SU_Y_COORDINATE_REQUIRED;
> >  }
> >  
> > @@ -3096,9 +3097,9 @@ static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t dprx = 0;
> >  
> > -	drm_dp_dpcd_readb(&intel_dp->aux,
> > -			DP_DPRX_FEATURE_ENUMERATION_LIST,
> > -			&dprx);
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
> > +			      &dprx) != 1)
> > +		return false;
> >  	return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
> >  }
> >  
> > @@ -3106,7 +3107,9 @@ static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t alpm_caps = 0;
> >  
> > -	drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, &alpm_caps);
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP,
> > +			      &alpm_caps) != 1)
> > +		return false;
> >  	return alpm_caps & DP_ALPM_CAP;
> >  }
> >  
> > @@ -3679,9 +3682,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> >  		uint8_t frame_sync_cap;
> >  
> >  		dev_priv->psr.sink_support = true;
> > -		drm_dp_dpcd_readb(&intel_dp->aux,
> > -				  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > -				  &frame_sync_cap);
> > +		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > +				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > +				      &frame_sync_cap) != 1)
> > +			frame_sync_cap = 0;
> >  		dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
> >  		/* PSR2 needs frame sync as well */
> >  		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> 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/8] drm/i915/sdvo: Check error return from intel_sdvo_get_value()

[Imre Deak]

On Wed, Apr 26, 2017 at 06:12:09PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2017 at 04:40:08PM +0300, Imre Deak wrote:
> > The current code assumes that 'enhancements' won't change in case of an
> > error, but this isn't guaranteed. Fix things by treating any error as a
> > lack of the given capability.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 816a6f5..0d1c511 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -2893,10 +2893,10 @@ static bool intel_sdvo_create_enhance_property(struct intel_sdvo *intel_sdvo,
> >  	BUILD_BUG_ON(sizeof(enhancements) != 2);
> >  
> >  	enhancements.response = 0;
> 
> This initialization could be removed if we don't use the thing
> when the read fails.

Yep, missed this, will remove it.

> 
> Either way
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > -	intel_sdvo_get_value(intel_sdvo,
> > -			     SDVO_CMD_GET_SUPPORTED_ENHANCEMENTS,
> > -			     &enhancements, sizeof(enhancements));
> > -	if (enhancements.response == 0) {
> > +	if (!intel_sdvo_get_value(intel_sdvo,
> > +				  SDVO_CMD_GET_SUPPORTED_ENHANCEMENTS,
> > +				  &enhancements, sizeof(enhancements)) ||
> > +	    enhancements.response == 0) {
> >  		DRM_DEBUG_KMS("No enhancement is supported\n");
> >  		return true;
> >  	}
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

On Wed, Apr 26, 2017 at 05:53:52PM +0300, Jani Nikula wrote:
> On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> > On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote:
> >> On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> >> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
> >> > else, but we still should check for it to prevent some other more
> >> > obscure bug later.
> >> 
> >> Do check for invalid pipe, but please just limp on instead of bailing
> >> out of the functions. See notes inline.
> >> 
> >> BR,
> >> Jani.
> >> 
> >> >
> >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_panel.c | 14 ++++++++++++--
> >> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> > index cb50c52..dbe05ec 100644
> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
> >> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> >> >  	struct intel_panel *panel = &connector->panel;
> >> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> > -	enum transcoder cpu_transcoder =
> >> > -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> >> > +	enum transcoder cpu_transcoder;
> >> >  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >> >  
> >> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> >> > +		return;
> >> 
> >> Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case,
> >> go on, and hope for the best. Do leave the warn in place though.
> >> 
> >> > +
> >> > +	cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> >> > +
> >> >  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >> >  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
> >> >  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
> >> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
> >> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> >  	u32 ctl, ctl2, freq;
> >> >  
> >> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> >> > +		return;
> >> 
> >> Same here, please just assign e.g. PIPE_A here with the warning, and
> >> limp on.
> >> 
> >> > +
> >> >  	ctl2 = I915_READ(BLC_PWM_CTL2);
> >> >  	if (ctl2 & BLM_PWM_ENABLE) {
> >> >  		DRM_DEBUG_KMS("backlight already enabled\n");
> >> > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
> >> >  	if (!panel->backlight.present)
> >> >  		return;
> >> >  
> >> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> >> > +		return;
> >> 
> >> Here, pipe is only used for the debug logging, just skip that instead.
> >
> > Ok, can do. This would make things inconsistent wrt.
> > vlv_enable/disable_backlight() though, so are you ok if I change those
> > too accordingly?
> 
> Those are different in the sense that the *registers* are chosen based
> on the pipe, so it *must* be one of A or B. But in the paths changed
> here, pipe is only used for some bit fields of the registers being
> updated. IIRC they are not even crucial for fundamental operation of the
> backlight. So I don't want to bail out unless we really can't proceed.

Ok, will keep those as-is.

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

Re: [PATCH 6/8] drm/i915: Sanitize stolen memory size calculation

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 04:40:11PM +0300, Imre Deak wrote:
> On GEN8+ (not counting CHV) the calculation can in theory result in an
> incorrect sign extension with all upper bits set. In practice this is
> unlikely to happen since it would require 4GB of stolen memory set
> aside. For consistency still prevent the sign extension explicitly
> everywhere.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 13bf099..4b764b0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2577,14 +2577,14 @@ static size_t gen6_get_stolen_size(u16 snb_gmch_ctl)
>  {
>  	snb_gmch_ctl >>= SNB_GMCH_GMS_SHIFT;
>  	snb_gmch_ctl &= SNB_GMCH_GMS_MASK;
> -	return snb_gmch_ctl << 25; /* 32 MB units */
> +	return (size_t)snb_gmch_ctl << 25; /* 32 MB units */

So the u16 gets promoted to int, which gets converted to size_t,
which may be larger than int, and thus things get sign extended.

Can't happen in the gen6 case actually due to SNB_GMCH_GMS_MASK being
small enough. But the gen8 case at least looks theoretically possible.
But having the case everywhere seems like the best way to avoid
someone copy-pasting the wrong thing when the next variant gets added.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  static size_t gen8_get_stolen_size(u16 bdw_gmch_ctl)
>  {
>  	bdw_gmch_ctl >>= BDW_GMCH_GMS_SHIFT;
>  	bdw_gmch_ctl &= BDW_GMCH_GMS_MASK;
> -	return bdw_gmch_ctl << 25; /* 32 MB units */
> +	return (size_t)bdw_gmch_ctl << 25; /* 32 MB units */
>  }
>  
>  static size_t chv_get_stolen_size(u16 gmch_ctrl)
> @@ -2598,11 +2598,11 @@ static size_t chv_get_stolen_size(u16 gmch_ctrl)
>  	 * 0x17 to 0x1d: 4MB increments start at 36MB
>  	 */
>  	if (gmch_ctrl < 0x11)
> -		return gmch_ctrl << 25;
> +		return (size_t)gmch_ctrl << 25;
>  	else if (gmch_ctrl < 0x17)
> -		return (gmch_ctrl - 0x11 + 2) << 22;
> +		return (size_t)(gmch_ctrl - 0x11 + 2) << 22;
>  	else
> -		return (gmch_ctrl - 0x17 + 9) << 22;
> +		return (size_t)(gmch_ctrl - 0x17 + 9) << 22;
>  }
>  
>  static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl)
> @@ -2611,10 +2611,10 @@ static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl)
>  	gen9_gmch_ctl &= BDW_GMCH_GMS_MASK;
>  
>  	if (gen9_gmch_ctl < 0xf0)
> -		return gen9_gmch_ctl << 25; /* 32 MB units */
> +		return (size_t)gen9_gmch_ctl << 25; /* 32 MB units */
>  	else
>  		/* 4MB increments starting at 0xf0 for 4MB */
> -		return (gen9_gmch_ctl - 0xf0 + 1) << 22;
> +		return (size_t)(gen9_gmch_ctl - 0xf0 + 1) << 22;
>  }
>  
>  static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/8] drm/i915/dp: Check error return during DPCD capability queries

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 06:23:42PM +0300, Imre Deak wrote:
> On Wed, Apr 26, 2017 at 06:08:24PM +0300, Ville Syrjälä wrote:
> > On Wed, Apr 26, 2017 at 04:40:07PM +0300, Imre Deak wrote:
> > > The assumptions of these users of drm_dp_dpcd_readb() is that the passed
> > > in output buffer won't change in case of error, but this isn't
> > > guaranteed.
> > 
> > Hmm. We blindly copy as many bytes from the rxbuf into the user
> > provided buffer as the hardware told us to. 
> 
> Haven't checked this, but now looking at it doesn't the 
> 'bytes > recv_size' check in intel_dp_aux_ch() bound that?

Hmm. Right, so in the case of one byte read we will still copy the
actual data byte as well, if the hardware returned one. So it could
still clobber the byte we would like to preserve. I think.

> 
> > And whether the transfer
> > actually is considered a success or not depends on what the first
> > received byte says. I don't recall what the DP spec really says about
> > replying with more than one byte on failure, but I guess we shouldn't
> > depend on it anyway.
> > 
> > We could actually make that guarantee if we moved txbuf+rxbuf up
> > into drm_dp_dpcd_access() and did the copy to the user buffer
> > only on succes. But that would require changing all the DP
> > capable drivers at the same time, so would require someone
> > motivated enough.
> 
> Ok. I stopped at the ZR24w workaround in drm_dp_dpcd_read().. But that
> could also be used with a separate buffer. In any case I thought that
> even with the guarantee that the buffer doesn't change - which is in
> general a reasonable assumption - checking for the error return would
> be more robust.
> 
> > 
> > In the meantime
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Thanks.
> 
> > 
> > > Fix this by treating any error as the lack of the given
> > > capability.
> > > 
> > > In case of DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP an error would leave the
> > > buffer uninitialized even with the above assumption.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++--------
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 08834f7..4a6feb6 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3088,7 +3088,8 @@ static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > >  {
> > >  	uint8_t psr_caps = 0;
> > >  
> > > -	drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps);
> > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps) != 1)
> > > +		return false;
> > >  	return psr_caps & DP_PSR2_SU_Y_COORDINATE_REQUIRED;
> > >  }
> > >  
> > > @@ -3096,9 +3097,9 @@ static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
> > >  {
> > >  	uint8_t dprx = 0;
> > >  
> > > -	drm_dp_dpcd_readb(&intel_dp->aux,
> > > -			DP_DPRX_FEATURE_ENUMERATION_LIST,
> > > -			&dprx);
> > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
> > > +			      &dprx) != 1)
> > > +		return false;
> > >  	return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
> > >  }
> > >  
> > > @@ -3106,7 +3107,9 @@ static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
> > >  {
> > >  	uint8_t alpm_caps = 0;
> > >  
> > > -	drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, &alpm_caps);
> > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP,
> > > +			      &alpm_caps) != 1)
> > > +		return false;
> > >  	return alpm_caps & DP_ALPM_CAP;
> > >  }
> > >  
> > > @@ -3679,9 +3682,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> > >  		uint8_t frame_sync_cap;
> > >  
> > >  		dev_priv->psr.sink_support = true;
> > > -		drm_dp_dpcd_readb(&intel_dp->aux,
> > > -				  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > > -				  &frame_sync_cap);
> > > +		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > +				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > > +				      &frame_sync_cap) != 1)
> > > +			frame_sync_cap = 0;
> > >  		dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
> > >  		/* PSR2 needs frame sync as well */
> > >  		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

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

Re: [PATCH 8/8] drm: Remove redundant NULL check during atomic plane commit

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 04:40:13PM +0300, Imre Deak wrote:
> plane_state can't be NULL anywhere in this function, so the NULL check
> at the end is redundant, remove it.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/drm_plane_helper.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 9854a50..2c27f6f 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -511,12 +511,10 @@ int drm_plane_helper_commit(struct drm_plane *plane,
>  	if (plane_funcs->cleanup_fb)
>  		plane_funcs->cleanup_fb(plane, plane_state);
>  out:
> -	if (plane_state) {
> -		if (plane->funcs->atomic_destroy_state)
> -			plane->funcs->atomic_destroy_state(plane, plane_state);
> -		else
> -			drm_atomic_helper_plane_destroy_state(plane, plane_state);
> -	}
> +	if (plane->funcs->atomic_destroy_state)
> +		plane->funcs->atomic_destroy_state(plane, plane_state);
> +	else
> +		drm_atomic_helper_plane_destroy_state(plane, plane_state);

Hmm. If plane->state was NULL, then we could swap that with plane_state,
and thus plane_state could become NULL. But that would actually oops in
drm_atomic_plane_disabling() already, so yeah, no way this could work.

My only concern is the buggy drm_atomic_helper_plane_reset() which can't
fail gracefully if the kmalloc fails. But failure that would probably
lead to explosions all over anyway.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


>  
>  	return ret;
>  }
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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/8] drm/i915/sdvo: Check error return from intel_sdvo_get_value()

[Imre Deak]

The current code assumes that 'enhancements' won't change in case of an
error, but this isn't guaranteed. Fix things by treating any error as a
lack of the given capability.

v2:
- Remove the now redundant init of enhancements. (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 816a6f5..496b24c 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2892,11 +2892,10 @@ static bool intel_sdvo_create_enhance_property(struct intel_sdvo *intel_sdvo,
 
 	BUILD_BUG_ON(sizeof(enhancements) != 2);
 
-	enhancements.response = 0;
-	intel_sdvo_get_value(intel_sdvo,
-			     SDVO_CMD_GET_SUPPORTED_ENHANCEMENTS,
-			     &enhancements, sizeof(enhancements));
-	if (enhancements.response == 0) {
+	if (!intel_sdvo_get_value(intel_sdvo,
+				  SDVO_CMD_GET_SUPPORTED_ENHANCEMENTS,
+				  &enhancements, sizeof(enhancements)) ||
+	    enhancements.response == 0) {
 		DRM_DEBUG_KMS("No enhancement is supported\n");
 		return true;
 	}
-- 
2.5.0

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

[PATCH v2 4/8] drm/i915: Check error return when setting DMA mask

[Imre Deak]

Even though an error from these functions isn't fatal we still want to
have a diagnostic message about it.

v2:
- Don't do assignments in if statements. (Jani)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8bab4ae..0178c9e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2741,13 +2741,17 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	unsigned int size;
 	u16 snb_gmch_ctl;
+	int err;
 
 	/* TODO: We're not aware of mappable constraints on gen8 yet */
 	ggtt->mappable_base = pci_resource_start(pdev, 2);
 	ggtt->mappable_end = pci_resource_len(pdev, 2);
 
-	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(39)))
-		pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
+	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(39));
+	if (!err)
+		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
+	if (err)
+		DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", err);
 
 	pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
 
@@ -2790,6 +2794,7 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	unsigned int size;
 	u16 snb_gmch_ctl;
+	int err;
 
 	ggtt->mappable_base = pci_resource_start(pdev, 2);
 	ggtt->mappable_end = pci_resource_len(pdev, 2);
@@ -2802,8 +2807,11 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
 		return -ENXIO;
 	}
 
-	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(40)))
-		pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
+	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(40));
+	if (!err)
+		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
+	if (err)
+		DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", err);
 	pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
 
 	ggtt->stolen_size = gen6_get_stolen_size(snb_gmch_ctl);
-- 
2.5.0

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

[PATCH v2 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

An error from intel_get_pipe_from_connector() would mean a bug somewhere
else, but we still should check for it to prevent some other more
obscure bug later.

v2:
- Fall back to a reasonable default instead of bailing out in case of
  error. (Jani)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index cb50c52..3508f42 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
-	enum transcoder cpu_transcoder =
-		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+	enum transcoder cpu_transcoder;
 	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
 
+	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
+		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+	else
+		cpu_transcoder = TRANSCODER_EDP;
+
 	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
 	if (cpu_ctl2 & BLM_PWM_ENABLE) {
 		DRM_DEBUG_KMS("cpu backlight already enabled\n");
@@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 ctl, ctl2, freq;
 
+	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
+		pipe = PIPE_A;
+
 	ctl2 = I915_READ(BLC_PWM_CTL2);
 	if (ctl2 & BLM_PWM_ENABLE) {
 		DRM_DEBUG_KMS("backlight already enabled\n");
@@ -1037,6 +1044,9 @@ static void bxt_enable_backlight(struct intel_connector *connector)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 pwm_ctl, val;
 
+	if (WARN_ON_ONCE(pipe) == PIPE_INVALID)
+		pipe = PIPE_A;
+
 	/* Controller 1 uses the utility pin. */
 	if (panel->backlight.controller == 1) {
 		val = I915_READ(UTIL_PIN_CTL);
@@ -1093,7 +1103,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
 	if (!panel->backlight.present)
 		return;
 
-	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
+	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
+		DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
 
 	mutex_lock(&dev_priv->backlight_lock);
 
-- 
2.5.0

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

[PATCH v2 7/8] drm/i915/lvds: Remove magic from PLL programming

[Imre Deak]

This looks like a left-over from enabling work. The spec defines
CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED as reserved set, so follow
this in the programming.

v2:
- Follow the spec to set CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED.
  (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/dvo_ch7017.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c
index b3c7c19..80b3e16 100644
--- a/drivers/gpu/drm/i915/dvo_ch7017.c
+++ b/drivers/gpu/drm/i915/dvo_ch7017.c
@@ -280,10 +280,10 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo,
 			(0 << CH7017_PHASE_DETECTOR_SHIFT);
 	} else {
 		outputs_enable = CH7017_LVDS_CHANNEL_A | CH7017_CHARGE_PUMP_HIGH;
-		lvds_pll_feedback_div = CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
+		lvds_pll_feedback_div =
+			CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
 			(2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) |
 			(3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT);
-		lvds_pll_feedback_div = 35;
 		lvds_control_2 = (3 << CH7017_LOOP_FILTER_SHIFT) |
 			(0 << CH7017_PHASE_DETECTOR_SHIFT);
 		if (1) { /* XXX: dual channel panel detection.  Assume yes for now. */
-- 
2.5.0

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

Re: [PATCH v2 7/8] drm/i915/lvds: Remove magic from PLL programming

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 08:18:06PM +0300, Imre Deak wrote:
> This looks like a left-over from enabling work. The spec defines
> CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED as reserved set, so follow
> this in the programming.
> 
> v2:
> - Follow the spec to set CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED.
>   (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/dvo_ch7017.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c
> index b3c7c19..80b3e16 100644
> --- a/drivers/gpu/drm/i915/dvo_ch7017.c
> +++ b/drivers/gpu/drm/i915/dvo_ch7017.c
> @@ -280,10 +280,10 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo,
>  			(0 << CH7017_PHASE_DETECTOR_SHIFT);
>  	} else {
>  		outputs_enable = CH7017_LVDS_CHANNEL_A | CH7017_CHARGE_PUMP_HIGH;
> -		lvds_pll_feedback_div = CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
> +		lvds_pll_feedback_div =
> +			CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
>  			(2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) |
>  			(3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT);
> -		lvds_pll_feedback_div = 35;
>  		lvds_control_2 = (3 << CH7017_LOOP_FILTER_SHIFT) |
>  			(0 << CH7017_PHASE_DETECTOR_SHIFT);
>  		if (1) { /* XXX: dual channel panel detection.  Assume yes for now. */
> -- 
> 2.5.0

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

Re: [PATCH v2 5/8] drm/i915: Check error return when converting pipe to connector

[Jani Nikula]

On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> An error from intel_get_pipe_from_connector() would mean a bug somewhere
> else, but we still should check for it to prevent some other more
> obscure bug later.
>
> v2:
> - Fall back to a reasonable default instead of bailing out in case of
>   error. (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb50c52..3508f42 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> -	enum transcoder cpu_transcoder =
> -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +	enum transcoder cpu_transcoder;
>  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>  
> +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +	else
> +		cpu_transcoder = TRANSCODER_EDP;
> +
>  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
> @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 ctl, ctl2, freq;
>  
> +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		pipe = PIPE_A;
> +
>  	ctl2 = I915_READ(BLC_PWM_CTL2);
>  	if (ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("backlight already enabled\n");
> @@ -1037,6 +1044,9 @@ static void bxt_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 pwm_ctl, val;
>  
> +	if (WARN_ON_ONCE(pipe) == PIPE_INVALID)

Le pipe invalid? I think you mean INVALID_PIPE here.

BR,
Jani.

> +		pipe = PIPE_A;
> +
>  	/* Controller 1 uses the utility pin. */
>  	if (panel->backlight.controller == 1) {
>  		val = I915_READ(UTIL_PIN_CTL);
> @@ -1093,7 +1103,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  	if (!panel->backlight.present)
>  		return;
>  
> -	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>  
>  	mutex_lock(&dev_priv->backlight_lock);

-- 
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 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

On Thu, Apr 27, 2017 at 10:09:35AM +0300, Jani Nikula wrote:
> On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
> > else, but we still should check for it to prevent some other more
> > obscure bug later.
> >
> > v2:
> > - Fall back to a reasonable default instead of bailing out in case of
> >   error. (Jani)
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index cb50c52..3508f42 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> >  	struct intel_panel *panel = &connector->panel;
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> > -	enum transcoder cpu_transcoder =
> > -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > +	enum transcoder cpu_transcoder;
> >  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >  
> > +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > +	else
> > +		cpu_transcoder = TRANSCODER_EDP;
> > +
> >  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
> >  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  	u32 ctl, ctl2, freq;
> >  
> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		pipe = PIPE_A;
> > +
> >  	ctl2 = I915_READ(BLC_PWM_CTL2);
> >  	if (ctl2 & BLM_PWM_ENABLE) {
> >  		DRM_DEBUG_KMS("backlight already enabled\n");
> > @@ -1037,6 +1044,9 @@ static void bxt_enable_backlight(struct intel_connector *connector)
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  	u32 pwm_ctl, val;
> >  
> > +	if (WARN_ON_ONCE(pipe) == PIPE_INVALID)
> 
> Le pipe invalid? I think you mean INVALID_PIPE here.

Arg, forgot git add at some point..

--Imre

> 
> BR,
> Jani.
> 
> > +		pipe = PIPE_A;
> > +
> >  	/* Controller 1 uses the utility pin. */
> >  	if (panel->backlight.controller == 1) {
> >  		val = I915_READ(UTIL_PIN_CTL);
> > @@ -1093,7 +1103,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
> >  	if (!panel->backlight.present)
> >  		return;
> >  
> > -	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> > +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> >  
> >  	mutex_lock(&dev_priv->backlight_lock);
> 
> -- 
> 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 v3 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

An error from intel_get_pipe_from_connector() would mean a bug somewhere
else, but we still should check for it to prevent some other more
obscure bug later.

v2:
- Fall back to a reasonable default instead of bailing out in case of
  error. (Jani)
v3:
- Fix s/PIPE_INVALID/INVALID_PIPE/ typo. (Jani)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index cb50c52..d1abbf1 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_panel *panel = &connector->panel;
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
-	enum transcoder cpu_transcoder =
-		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+	enum transcoder cpu_transcoder;
 	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
 
+	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
+		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+	else
+		cpu_transcoder = TRANSCODER_EDP;
+
 	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
 	if (cpu_ctl2 & BLM_PWM_ENABLE) {
 		DRM_DEBUG_KMS("cpu backlight already enabled\n");
@@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 ctl, ctl2, freq;
 
+	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
+		pipe = PIPE_A;
+
 	ctl2 = I915_READ(BLC_PWM_CTL2);
 	if (ctl2 & BLM_PWM_ENABLE) {
 		DRM_DEBUG_KMS("backlight already enabled\n");
@@ -1037,6 +1044,9 @@ static void bxt_enable_backlight(struct intel_connector *connector)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 pwm_ctl, val;
 
+	if (WARN_ON_ONCE(pipe) == INVALID_PIPE)
+		pipe = PIPE_A;
+
 	/* Controller 1 uses the utility pin. */
 	if (panel->backlight.controller == 1) {
 		val = I915_READ(UTIL_PIN_CTL);
@@ -1093,7 +1103,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
 	if (!panel->backlight.present)
 		return;
 
-	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
+	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
+		DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
 
 	mutex_lock(&dev_priv->backlight_lock);
 
-- 
2.5.0

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

Re: [PATCH v3 5/8] drm/i915: Check error return when converting pipe to connector

[Jani Nikula]

On Thu, 27 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> An error from intel_get_pipe_from_connector() would mean a bug somewhere
> else, but we still should check for it to prevent some other more
> obscure bug later.
>
> v2:
> - Fall back to a reasonable default instead of bailing out in case of
>   error. (Jani)
> v3:
> - Fix s/PIPE_INVALID/INVALID_PIPE/ typo. (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

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


> ---
>  drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb50c52..d1abbf1 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> -	enum transcoder cpu_transcoder =
> -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +	enum transcoder cpu_transcoder;
>  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>  
> +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +	else
> +		cpu_transcoder = TRANSCODER_EDP;
> +
>  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
> @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 ctl, ctl2, freq;
>  
> +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		pipe = PIPE_A;
> +
>  	ctl2 = I915_READ(BLC_PWM_CTL2);
>  	if (ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("backlight already enabled\n");
> @@ -1037,6 +1044,9 @@ static void bxt_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 pwm_ctl, val;
>  
> +	if (WARN_ON_ONCE(pipe) == INVALID_PIPE)
> +		pipe = PIPE_A;
> +
>  	/* Controller 1 uses the utility pin. */
>  	if (panel->backlight.controller == 1) {
>  		val = I915_READ(UTIL_PIN_CTL);
> @@ -1093,7 +1103,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  	if (!panel->backlight.present)
>  		return;
>  
> -	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>  
>  	mutex_lock(&dev_priv->backlight_lock);

-- 
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 6/8] drm/i915: Sanitize stolen memory size calculation

[Joonas Lahtinen]

On ke, 2017-04-26 at 18:27 +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2017 at 04:40:11PM +0300, Imre Deak wrote:
> > 
> > On GEN8+ (not counting CHV) the calculation can in theory result in an
> > incorrect sign extension with all upper bits set. In practice this is
> > unlikely to happen since it would require 4GB of stolen memory set
> > aside. For consistency still prevent the sign extension explicitly
> > everywhere.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>

<SNIP>

> > @@ -2577,14 +2577,14 @@ static size_t gen6_get_stolen_size(u16 snb_gmch_ctl)
> >  {
> > > >  	snb_gmch_ctl >>= SNB_GMCH_GMS_SHIFT;
> > > >  	snb_gmch_ctl &= SNB_GMCH_GMS_MASK;
> > > > -	return snb_gmch_ctl << 25; /* 32 MB units */
> > > > +	return (size_t)snb_gmch_ctl << 25; /* 32 MB units */
> 
> So the u16 gets promoted to int, which gets converted to size_t,
> which may be larger than int, and thus things get sign extended.
> 
> Can't happen in the gen6 case actually due to SNB_GMCH_GMS_MASK being
> small enough. But the gen8 case at least looks theoretically possible.
> But having the case everywhere seems like the best way to avoid
> someone copy-pasting the wrong thing when the next variant gets added.

I was about to comment that early-quirks needs to be fixed too, but it
was already fixed when I synchronized the code last time.

That reminded me that I still have the GIT branch to eliminate the code
duplication, which is probably why I didn't revamp the i915 variants.

The trouble is that in early-quirks, pci subsystem functions are
unavailable and the functions are __init, so we'll have to choose
between:

a) code duplication, as we have now
b) move the functions to i915_drm.h as inline with hooks, code size is not
   shrunk, but code duplication is eliminated
c) always have the functions as non __init, even if i915 is disabled
   reduces duplication and kernel size, (well, increases x86 tiny kernel
   size)
c) <insert your suggestion here>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/8] drm/i915: Check error return when setting DMA mask

[Jani Nikula]

On Wed, 26 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> Even though an error from these functions isn't fatal we still want to
> have a diagnostic message about it.
>
> v2:
> - Don't do assignments in if statements. (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

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

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8bab4ae..0178c9e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2741,13 +2741,17 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	unsigned int size;
>  	u16 snb_gmch_ctl;
> +	int err;
>  
>  	/* TODO: We're not aware of mappable constraints on gen8 yet */
>  	ggtt->mappable_base = pci_resource_start(pdev, 2);
>  	ggtt->mappable_end = pci_resource_len(pdev, 2);
>  
> -	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(39)))
> -		pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
> +	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(39));
> +	if (!err)
> +		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
> +	if (err)
> +		DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", err);
>  
>  	pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
>  
> @@ -2790,6 +2794,7 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	unsigned int size;
>  	u16 snb_gmch_ctl;
> +	int err;
>  
>  	ggtt->mappable_base = pci_resource_start(pdev, 2);
>  	ggtt->mappable_end = pci_resource_len(pdev, 2);
> @@ -2802,8 +2807,11 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
>  		return -ENXIO;
>  	}
>  
> -	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(40)))
> -		pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
> +	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(40));
> +	if (!err)
> +		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
> +	if (err)
> +		DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", err);
>  	pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
>  
>  	ggtt->stolen_size = gen6_get_stolen_size(snb_gmch_ctl);

-- 
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 v3 5/8] drm/i915: Check error return when converting pipe to connector

[Ville Syrjälä]

On Thu, Apr 27, 2017 at 11:36:54AM +0300, Imre Deak wrote:
> An error from intel_get_pipe_from_connector() would mean a bug somewhere
> else, but we still should check for it to prevent some other more
> obscure bug later.
> 
> v2:
> - Fall back to a reasonable default instead of bailing out in case of
>   error. (Jani)
> v3:
> - Fix s/PIPE_INVALID/INVALID_PIPE/ typo. (Jani)
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb50c52..d1abbf1 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> -	enum transcoder cpu_transcoder =
> -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +	enum transcoder cpu_transcoder;
>  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>  
> +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +	else
> +		cpu_transcoder = TRANSCODER_EDP;
> +
>  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
> @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 ctl, ctl2, freq;
>  
> +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		pipe = PIPE_A;
> +
>  	ctl2 = I915_READ(BLC_PWM_CTL2);
>  	if (ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("backlight already enabled\n");
> @@ -1037,6 +1044,9 @@ static void bxt_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 pwm_ctl, val;
>  
> +	if (WARN_ON_ONCE(pipe) == INVALID_PIPE)
                             ^

Isn't that thing in the wrong place?

> +		pipe = PIPE_A;
> +
>  	/* Controller 1 uses the utility pin. */
>  	if (panel->backlight.controller == 1) {
>  		val = I915_READ(UTIL_PIN_CTL);
> @@ -1093,7 +1103,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  	if (!panel->backlight.present)
>  		return;
>  
> -	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> +		DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>  
>  	mutex_lock(&dev_priv->backlight_lock);
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

On Thu, Apr 27, 2017 at 02:49:55PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 27, 2017 at 11:36:54AM +0300, Imre Deak wrote:
> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
> > else, but we still should check for it to prevent some other more
> > obscure bug later.
> > 
> > v2:
> > - Fall back to a reasonable default instead of bailing out in case of
> >   error. (Jani)
> > v3:
> > - Fix s/PIPE_INVALID/INVALID_PIPE/ typo. (Jani)
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index cb50c52..d1abbf1 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> >  	struct intel_panel *panel = &connector->panel;
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> > -	enum transcoder cpu_transcoder =
> > -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > +	enum transcoder cpu_transcoder;
> >  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >  
> > +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > +	else
> > +		cpu_transcoder = TRANSCODER_EDP;
> > +
> >  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
> >  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  	u32 ctl, ctl2, freq;
> >  
> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		pipe = PIPE_A;
> > +
> >  	ctl2 = I915_READ(BLC_PWM_CTL2);
> >  	if (ctl2 & BLM_PWM_ENABLE) {
> >  		DRM_DEBUG_KMS("backlight already enabled\n");
> > @@ -1037,6 +1044,9 @@ static void bxt_enable_backlight(struct intel_connector *connector)
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  	u32 pwm_ctl, val;
> >  
> > +	if (WARN_ON_ONCE(pipe) == INVALID_PIPE)
>                              ^
> 
> Isn't that thing in the wrong place?

Yes, thanks for catching it.

> 
> > +		pipe = PIPE_A;
> > +
> >  	/* Controller 1 uses the utility pin. */
> >  	if (panel->backlight.controller == 1) {
> >  		val = I915_READ(UTIL_PIN_CTL);
> > @@ -1093,7 +1103,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
> >  	if (!panel->backlight.present)
> >  		return;
> >  
> > -	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> > +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +		DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> >  
> >  	mutex_lock(&dev_priv->backlight_lock);
> >  
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/8] drm/i915: Check error return when converting pipe to connector

[Jani Nikula]

On Thu, 27 Apr 2017, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, Apr 27, 2017 at 02:49:55PM +0300, Ville Syrjälä wrote:
>> On Thu, Apr 27, 2017 at 11:36:54AM +0300, Imre Deak wrote:
>> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
>> > else, but we still should check for it to prevent some other more
>> > obscure bug later.
>> > 
>> > v2:
>> > - Fall back to a reasonable default instead of bailing out in case of
>> >   error. (Jani)
>> > v3:
>> > - Fix s/PIPE_INVALID/INVALID_PIPE/ typo. (Jani)
>> > 
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_panel.c | 17 ++++++++++++++---
>> >  1 file changed, 14 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> > index cb50c52..d1abbf1 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector *connector)
>> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> >  	struct intel_panel *panel = &connector->panel;
>> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> > -	enum transcoder cpu_transcoder =
>> > -		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>> > +	enum transcoder cpu_transcoder;
>> >  	u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>> >  
>> > +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>> > +	else
>> > +		cpu_transcoder = TRANSCODER_EDP;
>> > +
>> >  	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>> >  	if (cpu_ctl2 & BLM_PWM_ENABLE) {
>> >  		DRM_DEBUG_KMS("cpu backlight already enabled\n");
>> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector *connector)
>> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> >  	u32 ctl, ctl2, freq;
>> >  
>> > +	if (WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +		pipe = PIPE_A;
>> > +
>> >  	ctl2 = I915_READ(BLC_PWM_CTL2);
>> >  	if (ctl2 & BLM_PWM_ENABLE) {
>> >  		DRM_DEBUG_KMS("backlight already enabled\n");
>> > @@ -1037,6 +1044,9 @@ static void bxt_enable_backlight(struct intel_connector *connector)
>> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>> >  	u32 pwm_ctl, val;
>> >  
>> > +	if (WARN_ON_ONCE(pipe) == INVALID_PIPE)
>>                              ^
>> 
>> Isn't that thing in the wrong place?
>
> Yes, thanks for catching it.

*facepalm* I guess my review is not to be trusted. :/

BR,
Jani.


>
>> 
>> > +		pipe = PIPE_A;
>> > +
>> >  	/* Controller 1 uses the utility pin. */
>> >  	if (panel->backlight.controller == 1) {
>> >  		val = I915_READ(UTIL_PIN_CTL);
>> > @@ -1093,7 +1103,8 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>> >  	if (!panel->backlight.present)
>> >  		return;
>> >  
>> > -	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>> > +	if (!WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +		DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>> >  
>> >  	mutex_lock(&dev_priv->backlight_lock);
>> >  
>> > -- 
>> > 2.5.0
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Ville Syrjälä
>> Intel OTC

-- 
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: [Intel-gfx] [PATCH 8/8] drm: Remove redundant NULL check during atomic plane commit

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 06:44:53PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2017 at 04:40:13PM +0300, Imre Deak wrote:
> > plane_state can't be NULL anywhere in this function, so the NULL check
> > at the end is redundant, remove it.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane_helper.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> > index 9854a50..2c27f6f 100644
> > --- a/drivers/gpu/drm/drm_plane_helper.c
> > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > @@ -511,12 +511,10 @@ int drm_plane_helper_commit(struct drm_plane *plane,
> >  	if (plane_funcs->cleanup_fb)
> >  		plane_funcs->cleanup_fb(plane, plane_state);
> >  out:
> > -	if (plane_state) {
> > -		if (plane->funcs->atomic_destroy_state)
> > -			plane->funcs->atomic_destroy_state(plane, plane_state);
> > -		else
> > -			drm_atomic_helper_plane_destroy_state(plane, plane_state);
> > -	}
> > +	if (plane->funcs->atomic_destroy_state)
> > +		plane->funcs->atomic_destroy_state(plane, plane_state);
> > +	else
> > +		drm_atomic_helper_plane_destroy_state(plane, plane_state);
> 
> Hmm. If plane->state was NULL, then we could swap that with plane_state,
> and thus plane_state could become NULL. But that would actually oops in
> drm_atomic_plane_disabling() already, so yeah, no way this could work.
> 
> My only concern is the buggy drm_atomic_helper_plane_reset() which can't
> fail gracefully if the kmalloc fails. But failure that would probably
> lead to explosions all over anyway.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I pushed this one to drm-misc-next. Thanks for the patch.

> 
> 
> >  
> >  	return ret;
> >  }
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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