[PATCHv4 0/3] Add USB typeC based DP support for BXT platform

[PATCHv4 0/3] Add USB typeC based DP support for BXT platform

[Durgadoss R]

This patch series adds upfront link training support to enable
USB type C based DP on BXT platform.

To support USB type C alternate DP mode, the display driver needs to
know the number of lanes required by the DP panel as well as number
of lanes that can be supported by the type-C cable. Sometimes, the
type-C cable may limit the bandwidth even if Panel can support
more lanes.

The goal is to find out the number of lanes which can be supported
using a particular cable so that we can cap 'max_available_lanes'
to that number during modeset.

Patch   1/3 : Moves finding unused crtc to a common function
Patch   2/3 : Split ddi_pll_select to be able to calculate
	      pll config for DP during upfront link train
Patch   3/3 : Upfront implementation for BXT platform

Changes from v3:
* Used an earlier patch from Ander that splits ddi_pll_select
  to be able to use it from upfront link train logic.
Changes from v2:
* Rebased on top of intel_dpll_mgr.c code.
* Use drm_atomic_commit instead of atomic_helper_dpms, since the
  later uses crtc->acquire_ctx used also in legacy paths.
* Using a drm_atomic_state helps in using the shared_dpll APIs
  as is, thus keeping the upfront code away from changing pll
  internals directly.
* Fixed locking/retry/backoff logic in upfront according to
  atomic implementation.
Changes from v1:
* Using atomic_helper_dpms() to do DPMS off for upfront link
  training, instead of using load detect functions.
* Made intel_get_shared_dpll handle non-atomic paths by
  duplicating the required shared_dpll_config and then working
  on top of it. This helps in upfront link train code not
  directly touch the 'pll/pll->config' internals.
* Fixed the link_bw update logic in intel_dp_get_link_retry_params()
  for non-HBR2 panels.
* As per comments on earlier version, merged few patches
  (that added new functions) with the upfront link train patch,
  to facilitate easy review.

Link for v3:    https://patchwork.freedesktop.org/patch/79792/
Link for v2:    https://patchwork.freedesktop.org/patch/72207/
Link for v1:    https://patchwork.freedesktop.org/patch/67784/
Link for RFCv2: https://patchwork.freedesktop.org/patch/61776/
Link for RFCv1: https://patchwork.freedesktop.org/patch/59589/

Durgadoss R (3):
  drm/i915: Make finding unused crtc as a generic function
  drm/i915: Split bxt_ddi_pll_select()
  drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

 drivers/gpu/drm/i915/intel_ddi.c      |  73 ++++++++++++++
 drivers/gpu/drm/i915/intel_display.c  |  74 ++++++++------
 drivers/gpu/drm/i915/intel_dp.c       | 180 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 166 +++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  17 ++++
 drivers/gpu/drm/i915/intel_drv.h      |   7 ++
 6 files changed, 409 insertions(+), 108 deletions(-)

-- 
1.9.1

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

[PATCHv4 1/3] drm/i915: Make finding unused crtc as a generic function

[Durgadoss R]

Looping over the crtc list and finding an unused crtc
has other users like upfront link training. Hence move it to
a common function to re-use the logic.

v4:
* Added ERR_PTR as a return value in kernel Doc comment (Ander)
v3:
* Added kernel Doc and removed an invalid comment (Ander)
* Rebased on top of latest code which includes locking
  for state->enable usage.
v2:
* Made this as a separate function instead of having it
  inside upfront link train code.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +
 2 files changed, 45 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4cca155..a6df48b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10404,6 +10404,43 @@ static int intel_modeset_setup_plane_state(struct drm_atomic_state *state,
 	return 0;
 }
 
+/**
+ * intel_get_unused_crtc - Find an unused crtc for the given encoder
+ * @encoder: drm_encoder structure
+ * @ctx: ctx pointer to use with locking
+ *
+ * This function tries to find an unused crtc that can drive
+ * the given encoder. Returns NULL or ERR_PTR on failure.
+ */
+struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder,
+				struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_crtc *possible_crtc;
+	struct drm_crtc *crtc = NULL;
+	struct drm_device *dev = encoder->dev;
+	int ret, i = -1;
+
+	for_each_crtc(dev, possible_crtc) {
+		i++;
+		if (!(encoder->possible_crtcs & (1 << i)))
+			continue;
+
+		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
+		if (ret)
+			return ERR_PTR(ret);
+
+		if (possible_crtc->state->enable) {
+			drm_modeset_unlock(&possible_crtc->mutex);
+			continue;
+		}
+
+		crtc = possible_crtc;
+		break;
+	}
+
+	return crtc;
+}
+
 bool intel_get_load_detect_pipe(struct drm_connector *connector,
 				struct drm_display_mode *mode,
 				struct intel_load_detect_pipe *old,
@@ -10412,7 +10449,6 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct intel_crtc *intel_crtc;
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
-	struct drm_crtc *possible_crtc;
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
@@ -10421,7 +10457,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_atomic_state *state = NULL, *restore_state = NULL;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *crtc_state;
-	int ret, i = -1;
+	int ret;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 		      connector->base.id, connector->name,
@@ -10451,39 +10487,15 @@ retry:
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
 			goto fail;
-
-		/* Make sure the crtc and connector are running */
-		goto found;
-	}
-
-	/* Find an unused one (if possible) */
-	for_each_crtc(dev, possible_crtc) {
-		i++;
-		if (!(encoder->possible_crtcs & (1 << i)))
-			continue;
-
-		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
-		if (ret)
+	} else {
+		crtc = intel_get_unused_crtc(encoder, ctx);
+		if (IS_ERR_OR_NULL(crtc)) {
+			ret = PTR_ERR_OR_ZERO(crtc);
+			DRM_DEBUG_KMS("no pipe available for load-detect\n");
 			goto fail;
-
-		if (possible_crtc->state->enable) {
-			drm_modeset_unlock(&possible_crtc->mutex);
-			continue;
 		}
-
-		crtc = possible_crtc;
-		break;
-	}
-
-	/*
-	 * If we didn't find an unused CRTC, don't use any.
-	 */
-	if (!crtc) {
-		DRM_DEBUG_KMS("no pipe available for load-detect\n");
-		goto fail;
 	}
 
-found:
 	intel_crtc = to_intel_crtc(crtc);
 
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e13ce22..27b5dcd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1159,6 +1159,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old,
 				    struct drm_modeset_acquire_ctx *ctx);
+struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder,
+				struct drm_modeset_acquire_ctx *ctx);
 int intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
 			       unsigned int rotation);
 struct drm_framebuffer *
-- 
1.9.1

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

Re: [PATCHv4 1/3] drm/i915: Make finding unused crtc as a generic function

[Ander Conselvan De Oliveira]

On Mon, 2016-04-18 at 19:01 +0530, Durgadoss R wrote:
> Looping over the crtc list and finding an unused crtc
> has other users like upfront link training. Hence move it to

"will have", since this patch precedes the upfront link training one.

> a common function to re-use the logic.
> 
> v4:
> * Added ERR_PTR as a return value in kernel Doc comment (Ander)
> v3:
> * Added kernel Doc and removed an invalid comment (Ander)
> * Rebased on top of latest code which includes locking
>   for state->enable usage.
> v2:
> * Made this as a separate function instead of having it
>   inside upfront link train code.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++------------
> ---
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  2 files changed, 45 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 4cca155..a6df48b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10404,6 +10404,43 @@ static int intel_modeset_setup_plane_state(struct
> drm_atomic_state *state,
>  	return 0;
>  }
>  
> +/**
> + * intel_get_unused_crtc - Find an unused crtc for the given encoder
> + * @encoder: drm_encoder structure
> + * @ctx: ctx pointer to use with locking
> + *
> + * This function tries to find an unused crtc that can drive
> + * the given encoder. Returns NULL or ERR_PTR on failure.

This NULL or ERR_PTR makes for an awkward interface. I wonder if it should just
return ERR_PTR(-EBUSY) or even ERR_PTR(-EINVAL) if there is no free crtc. It's
no big deal though.

Also, in the success case crtc->mutex is kept locked. That should be added to
the documentation above.

> + */
> +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder,
> +				struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_crtc *possible_crtc;
> +	struct drm_crtc *crtc = NULL;
> +	struct drm_device *dev = encoder->dev;
> +	int ret, i = -1;
> +
> +	for_each_crtc(dev, possible_crtc) {
> +		i++;
> +		if (!(encoder->possible_crtcs & (1 << i)))
> +			continue;
> +
> +		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		if (possible_crtc->state->enable) {
> +			drm_modeset_unlock(&possible_crtc->mutex);
> +			continue;
> +		}
> +
> +		crtc = possible_crtc;
> +		break;

This could be replaced with "return possible_crtc;". No need for the extra
assignment and break.

I see that in patch 3 you have a debug message for when this returns NULL. It
would make sense to move that debug here.

Ander

> +	}
> +
> +	return crtc;
> +}
> +
>  bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  				struct drm_display_mode *mode,
>  				struct intel_load_detect_pipe *old,
> @@ -10412,7 +10449,6 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
>  	struct intel_crtc *intel_crtc;
>  	struct intel_encoder *intel_encoder =
>  		intel_attached_encoder(connector);
> -	struct drm_crtc *possible_crtc;
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_device *dev = encoder->dev;
> @@ -10421,7 +10457,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
>  	struct drm_atomic_state *state = NULL, *restore_state = NULL;
>  	struct drm_connector_state *connector_state;
>  	struct intel_crtc_state *crtc_state;
> -	int ret, i = -1;
> +	int ret;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>  		      connector->base.id, connector->name,
> @@ -10451,39 +10487,15 @@ retry:
>  		ret = drm_modeset_lock(&crtc->mutex, ctx);
>  		if (ret)
>  			goto fail;
> -
> -		/* Make sure the crtc and connector are running */
> -		goto found;
> -	}
> -
> -	/* Find an unused one (if possible) */
> -	for_each_crtc(dev, possible_crtc) {
> -		i++;
> -		if (!(encoder->possible_crtcs & (1 << i)))
> -			continue;
> -
> -		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
> -		if (ret)
> +	} else {
> +		crtc = intel_get_unused_crtc(encoder, ctx);
> +		if (IS_ERR_OR_NULL(crtc)) {
> +			ret = PTR_ERR_OR_ZERO(crtc);
> +			DRM_DEBUG_KMS("no pipe available for load-detect\n");
>  			goto fail;
> -
> -		if (possible_crtc->state->enable) {
> -			drm_modeset_unlock(&possible_crtc->mutex);
> -			continue;
>  		}
> -
> -		crtc = possible_crtc;
> -		break;
> -	}
> -
> -	/*
> -	 * If we didn't find an unused CRTC, don't use any.
> -	 */
> -	if (!crtc) {
> -		DRM_DEBUG_KMS("no pipe available for load-detect\n");
> -		goto fail;
>  	}
>  
> -found:
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index e13ce22..27b5dcd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1159,6 +1159,8 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
>  void intel_release_load_detect_pipe(struct drm_connector *connector,
>  				    struct intel_load_detect_pipe *old,
>  				    struct drm_modeset_acquire_ctx *ctx);
> +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder,
> +				struct drm_modeset_acquire_ctx *ctx);
>  int intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
>  			       unsigned int rotation);
>  struct drm_framebuffer *
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCHv4 1/3] drm/i915: Make finding unused crtc as a generic function

[R, Durgadoss]

> -----Original Message-----
> From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
> Sent: Friday, May 6, 2016 6:38 PM
> To: R, Durgadoss <durgadoss.r@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCHv4 1/3] drm/i915: Make finding unused crtc as
> a generic function
> 
> On Mon, 2016-04-18 at 19:01 +0530, Durgadoss R wrote:
> > Looping over the crtc list and finding an unused crtc has other users
> > like upfront link training. Hence move it to
> 
> "will have", since this patch precedes the upfront link training one.
> 
> > a common function to re-use the logic.
> >
> > v4:
> > * Added ERR_PTR as a return value in kernel Doc comment (Ander)
> > v3:
> > * Added kernel Doc and removed an invalid comment (Ander)
> > * Rebased on top of latest code which includes locking
> >   for state->enable usage.
> > v2:
> > * Made this as a separate function instead of having it
> >   inside upfront link train code.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 74
> > +++++++++++++++++++++------------
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 +
> >  2 files changed, 45 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 4cca155..a6df48b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10404,6 +10404,43 @@ static int
> > intel_modeset_setup_plane_state(struct
> > drm_atomic_state *state,
> >  	return 0;
> >  }
> >
> > +/**
> > + * intel_get_unused_crtc - Find an unused crtc for the given encoder
> > + * @encoder: drm_encoder structure
> > + * @ctx: ctx pointer to use with locking
> > + *
> > + * This function tries to find an unused crtc that can drive
> > + * the given encoder. Returns NULL or ERR_PTR on failure.
> 
> This NULL or ERR_PTR makes for an awkward interface. I wonder if it should
> just return ERR_PTR(-EBUSY) or even ERR_PTR(-EINVAL) if there is no free
> crtc. It's no big deal though.
> 
> Also, in the success case crtc->mutex is kept locked. That should be added to
> the documentation above.
>

Ok, Will make the failure case as return (-EINVAL).
 
> > + */
> > +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder,
> > +				struct drm_modeset_acquire_ctx *ctx) {
> > +	struct drm_crtc *possible_crtc;
> > +	struct drm_crtc *crtc = NULL;
> > +	struct drm_device *dev = encoder->dev;
> > +	int ret, i = -1;
> > +
> > +	for_each_crtc(dev, possible_crtc) {
> > +		i++;
> > +		if (!(encoder->possible_crtcs & (1 << i)))
> > +			continue;
> > +
> > +		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +
> > +		if (possible_crtc->state->enable) {
> > +			drm_modeset_unlock(&possible_crtc->mutex);
> > +			continue;
> > +		}
> > +
> > +		crtc = possible_crtc;
> > +		break;
> 
> This could be replaced with "return possible_crtc;". No need for the extra
> assignment and break.
> 
> I see that in patch 3 you have a debug message for when this returns NULL. It
> would make sense to move that debug here.

Sure, will add it here..

Thanks,
Durga

> 
> Ander
> 
> > +	}
> > +
> > +	return crtc;
> > +}
> > +
> >  bool intel_get_load_detect_pipe(struct drm_connector *connector,
> >  				struct drm_display_mode *mode,
> >  				struct intel_load_detect_pipe *old, @@ -
> 10412,7 +10449,6 @@ bool
> > intel_get_load_detect_pipe(struct drm_connector *connector,
> >  	struct intel_crtc *intel_crtc;
> >  	struct intel_encoder *intel_encoder =
> >  		intel_attached_encoder(connector);
> > -	struct drm_crtc *possible_crtc;
> >  	struct drm_encoder *encoder = &intel_encoder->base;
> >  	struct drm_crtc *crtc = NULL;
> >  	struct drm_device *dev = encoder->dev; @@ -10421,7 +10457,7 @@
> bool
> > intel_get_load_detect_pipe(struct drm_connector *connector,
> >  	struct drm_atomic_state *state = NULL, *restore_state = NULL;
> >  	struct drm_connector_state *connector_state;
> >  	struct intel_crtc_state *crtc_state;
> > -	int ret, i = -1;
> > +	int ret;
> >
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> >  		      connector->base.id, connector->name, @@ -10451,39
> +10487,15
> > @@ retry:
> >  		ret = drm_modeset_lock(&crtc->mutex, ctx);
> >  		if (ret)
> >  			goto fail;
> > -
> > -		/* Make sure the crtc and connector are running */
> > -		goto found;
> > -	}
> > -
> > -	/* Find an unused one (if possible) */
> > -	for_each_crtc(dev, possible_crtc) {
> > -		i++;
> > -		if (!(encoder->possible_crtcs & (1 << i)))
> > -			continue;
> > -
> > -		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
> > -		if (ret)
> > +	} else {
> > +		crtc = intel_get_unused_crtc(encoder, ctx);
> > +		if (IS_ERR_OR_NULL(crtc)) {
> > +			ret = PTR_ERR_OR_ZERO(crtc);
> > +			DRM_DEBUG_KMS("no pipe available for load-
> detect\n");
> >  			goto fail;
> > -
> > -		if (possible_crtc->state->enable) {
> > -			drm_modeset_unlock(&possible_crtc->mutex);
> > -			continue;
> >  		}
> > -
> > -		crtc = possible_crtc;
> > -		break;
> > -	}
> > -
> > -	/*
> > -	 * If we didn't find an unused CRTC, don't use any.
> > -	 */
> > -	if (!crtc) {
> > -		DRM_DEBUG_KMS("no pipe available for load-detect\n");
> > -		goto fail;
> >  	}
> >
> > -found:
> >  	intel_crtc = to_intel_crtc(crtc);
> >
> >  	ret = drm_modeset_lock(&crtc->primary->mutex, ctx); diff --git
> > a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index e13ce22..27b5dcd 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1159,6 +1159,8 @@ bool intel_get_load_detect_pipe(struct
> > drm_connector *connector,
> >  void intel_release_load_detect_pipe(struct drm_connector *connector,
> >  				    struct intel_load_detect_pipe *old,
> >  				    struct drm_modeset_acquire_ctx *ctx);
> > +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder,
> > +				struct drm_modeset_acquire_ctx *ctx);
> >  int intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> >  			       unsigned int rotation);
> >  struct drm_framebuffer *
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Split bxt_ddi_pll_select()

[Durgadoss R]

Split out of bxt_ddi_pll_select() the logic that calculates the pll
dividers and dpll_hw_state into a new function that doesn't depend on
crtc state. This will be used for enabling the port pll when doing
upfront link training.

v1:
* Rebased on top of intel_dpll_mgr.c (Durga)
* Initial version from Ander on top of intel_ddi.c

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 166 +++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  17 ++++
 2 files changed, 108 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 639bf02..5df72f9 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -1471,17 +1471,6 @@ out:
 	return ret;
 }
 
-/* bxt clock parameters */
-struct bxt_clk_div {
-	int clock;
-	uint32_t p1;
-	uint32_t p2;
-	uint32_t m2_int;
-	uint32_t m2_frac;
-	bool m2_frac_en;
-	uint32_t n;
-};
-
 /* pre-calculated values for DP linkrates */
 static const struct bxt_clk_div bxt_dp_clk_val[] = {
 	{162000, 4, 2, 32, 1677722, 1, 1},
@@ -1493,57 +1482,60 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = {
 	{432000, 3, 1, 32, 1677722, 1, 1}
 };
 
-static struct intel_shared_dpll *
-bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
-	     struct intel_encoder *encoder)
+static bool
+bxt_ddi_hdmi_pll_dividers(struct intel_crtc *intel_crtc,
+			  struct intel_crtc_state *crtc_state, int clock,
+			  struct bxt_clk_div *clk_div)
 {
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	struct intel_shared_dpll *pll;
-	enum intel_dpll_id i;
-	struct intel_digital_port *intel_dig_port;
-	struct bxt_clk_div clk_div = {0};
-	int vco = 0;
-	uint32_t prop_coef, int_coef, gain_ctl, targ_cnt;
-	uint32_t lanestagger;
-	int clock = crtc_state->port_clock;
+	intel_clock_t best_clock;
 
-	if (encoder->type == INTEL_OUTPUT_HDMI) {
-		intel_clock_t best_clock;
+	/* Calculate HDMI div */
+	/*
+	 * FIXME: tie the following calculation into
+	 * i9xx_crtc_compute_clock
+	 */
+	if (!bxt_find_best_dpll(crtc_state, clock, &best_clock)) {
+		DRM_DEBUG_DRIVER("no PLL dividers found for clock %d pipe %c\n",
+				 clock, pipe_name(intel_crtc->pipe));
+		return false;
+	}
 
-		/* Calculate HDMI div */
-		/*
-		 * FIXME: tie the following calculation into
-		 * i9xx_crtc_compute_clock
-		 */
-		if (!bxt_find_best_dpll(crtc_state, clock, &best_clock)) {
-			DRM_DEBUG_DRIVER("no PLL dividers found for clock %d pipe %c\n",
-					 clock, pipe_name(crtc->pipe));
-			return NULL;
-		}
+	clk_div->p1 = best_clock.p1;
+	clk_div->p2 = best_clock.p2;
+	WARN_ON(best_clock.m1 != 2);
+	clk_div->n = best_clock.n;
+	clk_div->m2_int = best_clock.m2 >> 22;
+	clk_div->m2_frac = best_clock.m2 & ((1 << 22) - 1);
+	clk_div->m2_frac_en = clk_div->m2_frac != 0;
 
-		clk_div.p1 = best_clock.p1;
-		clk_div.p2 = best_clock.p2;
-		WARN_ON(best_clock.m1 != 2);
-		clk_div.n = best_clock.n;
-		clk_div.m2_int = best_clock.m2 >> 22;
-		clk_div.m2_frac = best_clock.m2 & ((1 << 22) - 1);
-		clk_div.m2_frac_en = clk_div.m2_frac != 0;
+	clk_div->vco = best_clock.vco;
 
-		vco = best_clock.vco;
-	} else if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
-		   encoder->type == INTEL_OUTPUT_EDP) {
-		int i;
+	return true;
+}
 
-		clk_div = bxt_dp_clk_val[0];
-		for (i = 0; i < ARRAY_SIZE(bxt_dp_clk_val); ++i) {
-			if (bxt_dp_clk_val[i].clock == clock) {
-				clk_div = bxt_dp_clk_val[i];
-				break;
-			}
+void bxt_ddi_dp_pll_dividers(int clock, struct bxt_clk_div *clk_div)
+{
+	int i;
+
+	*clk_div = bxt_dp_clk_val[0];
+	for (i = 0; i < ARRAY_SIZE(bxt_dp_clk_val); ++i) {
+		if (bxt_dp_clk_val[i].clock == clock) {
+			*clk_div = bxt_dp_clk_val[i];
+			break;
 		}
-		vco = clock * 10 / 2 * clk_div.p1 * clk_div.p2;
 	}
 
+	clk_div->vco = clock * 10 / 2 * clk_div->p1 * clk_div->p2;
+}
+
+bool bxt_ddi_set_dpll_hw_state(int clock,
+			  struct bxt_clk_div *clk_div,
+			  struct intel_dpll_hw_state *dpll_hw_state)
+{
+	int vco = clk_div->vco;
+	uint32_t prop_coef, int_coef, gain_ctl, targ_cnt;
+	uint32_t lanestagger;
+
 	if (vco >= 6200000 && vco <= 6700000) {
 		prop_coef = 4;
 		int_coef = 9;
@@ -1562,12 +1554,9 @@ bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 		targ_cnt = 9;
 	} else {
 		DRM_ERROR("Invalid VCO\n");
-		return NULL;
+		return false;
 	}
 
-	memset(&crtc_state->dpll_hw_state, 0,
-	       sizeof(crtc_state->dpll_hw_state));
-
 	if (clock > 270000)
 		lanestagger = 0x18;
 	else if (clock > 135000)
@@ -1579,33 +1568,60 @@ bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 	else
 		lanestagger = 0x02;
 
-	crtc_state->dpll_hw_state.ebb0 =
-		PORT_PLL_P1(clk_div.p1) | PORT_PLL_P2(clk_div.p2);
-	crtc_state->dpll_hw_state.pll0 = clk_div.m2_int;
-	crtc_state->dpll_hw_state.pll1 = PORT_PLL_N(clk_div.n);
-	crtc_state->dpll_hw_state.pll2 = clk_div.m2_frac;
+	dpll_hw_state->ebb0 = PORT_PLL_P1(clk_div->p1) | PORT_PLL_P2(clk_div->p2);
+	dpll_hw_state->pll0 = clk_div->m2_int;
+	dpll_hw_state->pll1 = PORT_PLL_N(clk_div->n);
+	dpll_hw_state->pll2 = clk_div->m2_frac;
 
-	if (clk_div.m2_frac_en)
-		crtc_state->dpll_hw_state.pll3 =
-			PORT_PLL_M2_FRAC_ENABLE;
+	if (clk_div->m2_frac_en)
+		dpll_hw_state->pll3 = PORT_PLL_M2_FRAC_ENABLE;
 
-	crtc_state->dpll_hw_state.pll6 =
-		prop_coef | PORT_PLL_INT_COEFF(int_coef);
-	crtc_state->dpll_hw_state.pll6 |=
-		PORT_PLL_GAIN_CTL(gain_ctl);
+	dpll_hw_state->pll6 = prop_coef | PORT_PLL_INT_COEFF(int_coef);
+	dpll_hw_state->pll6 |= PORT_PLL_GAIN_CTL(gain_ctl);
 
-	crtc_state->dpll_hw_state.pll8 = targ_cnt;
+	dpll_hw_state->pll8 = targ_cnt;
 
-	crtc_state->dpll_hw_state.pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
+	dpll_hw_state->pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
 
-	crtc_state->dpll_hw_state.pll10 =
+	dpll_hw_state->pll10 =
 		PORT_PLL_DCO_AMP(PORT_PLL_DCO_AMP_DEFAULT)
 		| PORT_PLL_DCO_AMP_OVR_EN_H;
 
-	crtc_state->dpll_hw_state.ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
+	dpll_hw_state->ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
+
+	dpll_hw_state->pcsdw12 = LANESTAGGER_STRAP_OVRD | lanestagger;
+
+	return true;
+}
+
+static struct intel_shared_dpll *
+bxt_get_dpll(struct intel_crtc *crtc,
+		struct intel_crtc_state *crtc_state,
+		struct intel_encoder *encoder)
+{
+	struct bxt_clk_div clk_div = {0};
+	struct intel_dpll_hw_state dpll_hw_state = {0};
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct intel_digital_port *intel_dig_port;
+	struct intel_shared_dpll *pll;
+	int i, clock = crtc_state->port_clock;
+
+	if (encoder->type == INTEL_OUTPUT_HDMI
+	    && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
+					  clock, &clk_div))
+			return false;
+
+	if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+	    encoder->type == INTEL_OUTPUT_EDP)
+		bxt_ddi_dp_pll_dividers(clock, &clk_div);
+
+	if (!bxt_ddi_set_dpll_hw_state(clock, &clk_div, &dpll_hw_state))
+		return false;
+
+	memset(&crtc_state->dpll_hw_state, 0,
+	       sizeof(crtc_state->dpll_hw_state));
 
-	crtc_state->dpll_hw_state.pcsdw12 =
-		LANESTAGGER_STRAP_OVRD | lanestagger;
+	crtc_state->dpll_hw_state = dpll_hw_state;
 
 	intel_dig_port = enc_to_dig_port(&encoder->base);
 
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 89c5ada..ffcb21c 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -126,6 +126,19 @@ struct intel_shared_dpll {
 	uint32_t flags;
 };
 
+/* bxt clock parameters */
+struct bxt_clk_div {
+	int clock;
+	uint32_t p1;
+	uint32_t p2;
+	uint32_t m2_int;
+	uint32_t m2_frac;
+	bool m2_frac_en;
+	uint32_t n;
+
+	int vco;
+};
+
 #define SKL_DPLL0 0
 #define SKL_DPLL1 1
 #define SKL_DPLL2 2
@@ -160,5 +173,9 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc);
 void intel_shared_dpll_commit(struct drm_atomic_state *state);
 void intel_shared_dpll_init(struct drm_device *dev);
 
+/* BXT dpll related functions */
+void bxt_ddi_dp_pll_dividers(int clock, struct bxt_clk_div *clk_div);
+bool bxt_ddi_set_dpll_hw_state(int clock, struct bxt_clk_div *clk_div,
+			  struct intel_dpll_hw_state *dpll_hw_state);
 
 #endif /* _INTEL_DPLL_MGR_H_ */
-- 
1.9.1

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

Re: [PATCH 2/3] drm/i915: Split bxt_ddi_pll_select()

[Ander Conselvan De Oliveira]

On Mon, 2016-04-18 at 19:01 +0530, Durgadoss R wrote:
> Split out of bxt_ddi_pll_select() the logic that calculates the pll
> dividers and dpll_hw_state into a new function that doesn't depend on
> crtc state. This will be used for enabling the port pll when doing
> upfront link training.
> 
> v1:
> * Rebased on top of intel_dpll_mgr.c (Durga)
> * Initial version from Ander on top of intel_ddi.c
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.
> com>
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 166 +++++++++++++++++++------------
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  17 ++++
>  2 files changed, 108 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 639bf02..5df72f9 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1471,17 +1471,6 @@ out:
>  	return ret;
>  }
>  
> -/* bxt clock parameters */
> -struct bxt_clk_div {
> -	int clock;
> -	uint32_t p1;
> -	uint32_t p2;
> -	uint32_t m2_int;
> -	uint32_t m2_frac;
> -	bool m2_frac_en;
> -	uint32_t n;
> -};
> -
>  /* pre-calculated values for DP linkrates */
>  static const struct bxt_clk_div bxt_dp_clk_val[] = {
>  	{162000, 4, 2, 32, 1677722, 1, 1},
> @@ -1493,57 +1482,60 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = {
>  	{432000, 3, 1, 32, 1677722, 1, 1}
>  };
>  
> -static struct intel_shared_dpll *
> -bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
> -	     struct intel_encoder *encoder)
> +static bool
> +bxt_ddi_hdmi_pll_dividers(struct intel_crtc *intel_crtc,
> +			  struct intel_crtc_state *crtc_state, int clock,
> +			  struct bxt_clk_div *clk_div)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct intel_shared_dpll *pll;
> -	enum intel_dpll_id i;
> -	struct intel_digital_port *intel_dig_port;
> -	struct bxt_clk_div clk_div = {0};
> -	int vco = 0;
> -	uint32_t prop_coef, int_coef, gain_ctl, targ_cnt;
> -	uint32_t lanestagger;
> -	int clock = crtc_state->port_clock;
> +	intel_clock_t best_clock;
>  
> -	if (encoder->type == INTEL_OUTPUT_HDMI) {
> -		intel_clock_t best_clock;
> +	/* Calculate HDMI div */
> +	/*
> +	 * FIXME: tie the following calculation into
> +	 * i9xx_crtc_compute_clock
> +	 */
> +	if (!bxt_find_best_dpll(crtc_state, clock, &best_clock)) {
> +		DRM_DEBUG_DRIVER("no PLL dividers found for clock %d pipe
> %c\n",
> +				 clock, pipe_name(intel_crtc->pipe));
> +		return false;
> +	}
>  
> -		/* Calculate HDMI div */
> -		/*
> -		 * FIXME: tie the following calculation into
> -		 * i9xx_crtc_compute_clock
> -		 */
> -		if (!bxt_find_best_dpll(crtc_state, clock, &best_clock)) {
> -			DRM_DEBUG_DRIVER("no PLL dividers found for clock %d
> pipe %c\n",
> -					 clock, pipe_name(crtc->pipe));
> -			return NULL;
> -		}
> +	clk_div->p1 = best_clock.p1;
> +	clk_div->p2 = best_clock.p2;
> +	WARN_ON(best_clock.m1 != 2);
> +	clk_div->n = best_clock.n;
> +	clk_div->m2_int = best_clock.m2 >> 22;
> +	clk_div->m2_frac = best_clock.m2 & ((1 << 22) - 1);
> +	clk_div->m2_frac_en = clk_div->m2_frac != 0;
>  
> -		clk_div.p1 = best_clock.p1;
> -		clk_div.p2 = best_clock.p2;
> -		WARN_ON(best_clock.m1 != 2);
> -		clk_div.n = best_clock.n;
> -		clk_div.m2_int = best_clock.m2 >> 22;
> -		clk_div.m2_frac = best_clock.m2 & ((1 << 22) - 1);
> -		clk_div.m2_frac_en = clk_div.m2_frac != 0;
> +	clk_div->vco = best_clock.vco;
>  
> -		vco = best_clock.vco;
> -	} else if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> -		   encoder->type == INTEL_OUTPUT_EDP) {
> -		int i;
> +	return true;
> +}
>  
> -		clk_div = bxt_dp_clk_val[0];
> -		for (i = 0; i < ARRAY_SIZE(bxt_dp_clk_val); ++i) {
> -			if (bxt_dp_clk_val[i].clock == clock) {
> -				clk_div = bxt_dp_clk_val[i];
> -				break;
> -			}
> +void bxt_ddi_dp_pll_dividers(int clock, struct bxt_clk_div *clk_div)
> +{
> +	int i;
> +
> +	*clk_div = bxt_dp_clk_val[0];
> +	for (i = 0; i < ARRAY_SIZE(bxt_dp_clk_val); ++i) {
> +		if (bxt_dp_clk_val[i].clock == clock) {
> +			*clk_div = bxt_dp_clk_val[i];
> +			break;
>  		}
> -		vco = clock * 10 / 2 * clk_div.p1 * clk_div.p2;
>  	}
>  
> +	clk_div->vco = clock * 10 / 2 * clk_div->p1 * clk_div->p2;
> +}
> +
> +bool bxt_ddi_set_dpll_hw_state(int clock,
> +			  struct bxt_clk_div *clk_div,
> +			  struct intel_dpll_hw_state *dpll_hw_state)
> +{
> +	int vco = clk_div->vco;
> +	uint32_t prop_coef, int_coef, gain_ctl, targ_cnt;
> +	uint32_t lanestagger;
> +
>  	if (vco >= 6200000 && vco <= 6700000) {
>  		prop_coef = 4;
>  		int_coef = 9;
> @@ -1562,12 +1554,9 @@ bxt_get_dpll(struct intel_crtc *crtc, struct
> intel_crtc_state *crtc_state,
>  		targ_cnt = 9;
>  	} else {
>  		DRM_ERROR("Invalid VCO\n");
> -		return NULL;
> +		return false;
>  	}
>  
> -	memset(&crtc_state->dpll_hw_state, 0,
> -	       sizeof(crtc_state->dpll_hw_state));
> -
>  	if (clock > 270000)
>  		lanestagger = 0x18;
>  	else if (clock > 135000)
> @@ -1579,33 +1568,60 @@ bxt_get_dpll(struct intel_crtc *crtc, struct
> intel_crtc_state *crtc_state,
>  	else
>  		lanestagger = 0x02;
>  
> -	crtc_state->dpll_hw_state.ebb0 =
> -		PORT_PLL_P1(clk_div.p1) | PORT_PLL_P2(clk_div.p2);
> -	crtc_state->dpll_hw_state.pll0 = clk_div.m2_int;
> -	crtc_state->dpll_hw_state.pll1 = PORT_PLL_N(clk_div.n);
> -	crtc_state->dpll_hw_state.pll2 = clk_div.m2_frac;
> +	dpll_hw_state->ebb0 = PORT_PLL_P1(clk_div->p1) | PORT_PLL_P2(clk_div-
> >p2);
> +	dpll_hw_state->pll0 = clk_div->m2_int;
> +	dpll_hw_state->pll1 = PORT_PLL_N(clk_div->n);
> +	dpll_hw_state->pll2 = clk_div->m2_frac;
>  
> -	if (clk_div.m2_frac_en)
> -		crtc_state->dpll_hw_state.pll3 =
> -			PORT_PLL_M2_FRAC_ENABLE;
> +	if (clk_div->m2_frac_en)
> +		dpll_hw_state->pll3 = PORT_PLL_M2_FRAC_ENABLE;
>  
> -	crtc_state->dpll_hw_state.pll6 =
> -		prop_coef | PORT_PLL_INT_COEFF(int_coef);
> -	crtc_state->dpll_hw_state.pll6 |=
> -		PORT_PLL_GAIN_CTL(gain_ctl);
> +	dpll_hw_state->pll6 = prop_coef | PORT_PLL_INT_COEFF(int_coef);
> +	dpll_hw_state->pll6 |= PORT_PLL_GAIN_CTL(gain_ctl);
>  
> -	crtc_state->dpll_hw_state.pll8 = targ_cnt;
> +	dpll_hw_state->pll8 = targ_cnt;
>  
> -	crtc_state->dpll_hw_state.pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
> +	dpll_hw_state->pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
>  
> -	crtc_state->dpll_hw_state.pll10 =
> +	dpll_hw_state->pll10 =
>  		PORT_PLL_DCO_AMP(PORT_PLL_DCO_AMP_DEFAULT)
>  		| PORT_PLL_DCO_AMP_OVR_EN_H;
>  
> -	crtc_state->dpll_hw_state.ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
> +	dpll_hw_state->ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
> +
> +	dpll_hw_state->pcsdw12 = LANESTAGGER_STRAP_OVRD | lanestagger;
> +
> +	return true;
> +}
> +
> +static struct intel_shared_dpll *
> +bxt_get_dpll(struct intel_crtc *crtc,
> +		struct intel_crtc_state *crtc_state,
> +		struct intel_encoder *encoder)
> +{
> +	struct bxt_clk_div clk_div = {0};
> +	struct intel_dpll_hw_state dpll_hw_state = {0};
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_digital_port *intel_dig_port;
> +	struct intel_shared_dpll *pll;
> +	int i, clock = crtc_state->port_clock;
> +
> +	if (encoder->type == INTEL_OUTPUT_HDMI
> +	    && !bxt_ddi_hdmi_pll_dividers(crtc, crtc_state,
> +					  clock, &clk_div))
> +			return false;
> +
> +	if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> +	    encoder->type == INTEL_OUTPUT_EDP)
> +		bxt_ddi_dp_pll_dividers(clock, &clk_div);
> +
> +	if (!bxt_ddi_set_dpll_hw_state(clock, &clk_div, &dpll_hw_state))
> +		return false;
> +
> +	memset(&crtc_state->dpll_hw_state, 0,
> +	       sizeof(crtc_state->dpll_hw_state));
>  
> -	crtc_state->dpll_hw_state.pcsdw12 =
> -		LANESTAGGER_STRAP_OVRD | lanestagger;
> +	crtc_state->dpll_hw_state = dpll_hw_state;
>  
>  	intel_dig_port = enc_to_dig_port(&encoder->base);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 89c5ada..ffcb21c 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -126,6 +126,19 @@ struct intel_shared_dpll {
>  	uint32_t flags;
>  };
>  
> +/* bxt clock parameters */
> +struct bxt_clk_div {
> +	int clock;
> +	uint32_t p1;
> +	uint32_t p2;
> +	uint32_t m2_int;
> +	uint32_t m2_frac;
> +	bool m2_frac_en;
> +	uint32_t n;
> +
> +	int vco;
> +};
> +
>  #define SKL_DPLL0 0
>  #define SKL_DPLL1 1
>  #define SKL_DPLL2 2
> @@ -160,5 +173,9 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc);
>  void intel_shared_dpll_commit(struct drm_atomic_state *state);
>  void intel_shared_dpll_init(struct drm_device *dev);
>  
> +/* BXT dpll related functions */
> +void bxt_ddi_dp_pll_dividers(int clock, struct bxt_clk_div *clk_div);
> +bool bxt_ddi_set_dpll_hw_state(int clock, struct bxt_clk_div *clk_div,
> +			  struct intel_dpll_hw_state *dpll_hw_state);

I'd rather not export these functions and the struct above. IMO it would be
better to create a new function that takes the dp rate and fills up the hw
state. That function would just call those two functions in succession, but that
way those details are hidden from the caller.

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

[PATCHv4 3/3] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Durgadoss R]

To support USB type C alternate DP mode, the display driver needs to
know the number of lanes required by the DP panel as well as number
of lanes that can be supported by the type-C cable. Sometimes, the
type-C cable may limit the bandwidth even if Panel can support
more lanes. To address these scenarios, the display driver will
start link training with max lanes, and if that fails, the driver
falls back to x2 lanes; and repeats this procedure for all
bandwidth/lane configurations.

* Since link training is done before modeset only the port
  (and not pipe/planes) and its associated PLLs are enabled.
* On DP hotplug: Directly start link training on the crtc
  associated with the DP encoder.
* On Connected boot scenarios: When booted with an LFP and a DP,
  typically, BIOS brings up DP. In these cases, we disable the
  crtc and then do upfront link training; and let the subsequent
  modeset re-enable the crtc.
* All local changes made for upfront link training are reset
  to their previous values once it is done; so that the
  subsequent modeset is not aware of these changes.

Changes since v3:
* Fixed a return value on BXT check
* Reworked on top of bxt_ddi_pll_select split from Ander
* Renamed from ddi_upfront to bxt_upfront since the
  upfront logic includes BXT specific functions for now.
Changes since v2:
* Rebased on top of latest dpll_mgr.c code and
  latest HPD related clean ups.
* Corrected return values from upfront (Ander)
* Corrected atomic locking for upfront in intel_dp.c (Ville)
Changes since v1:
*  all pll related functions inside ddi.c

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |  73 ++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 180 ++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h |   5 ++
 3 files changed, 256 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 96234c5..f7fa3db 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2268,6 +2268,79 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
 	return connector;
 }
 
+int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
+				struct intel_crtc *crtc)
+{
+	struct intel_connector *connector = intel_dp->attached_connector;
+	struct intel_encoder *encoder = connector->encoder;
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_shared_dpll *pll;
+	struct intel_shared_dpll_config tmp_pll_config;
+	struct bxt_clk_div clk_div = {0};
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	enum intel_dpll_id dpll_id = (enum intel_dpll_id)dig_port->port;
+	uint8_t link_bw, lane_count;
+
+	if (WARN_ON(!crtc))
+		return -EINVAL;
+
+	/* Initialize with Max Link rate & lane count supported by panel */
+	link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
+	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
+
+	mutex_lock(&dev_priv->dpll_lock);
+	/*
+	 * FIXME: Works only for BXT.
+	 * Select the required PLL. This works for platforms where
+	 * there is no shared DPLL.
+	 */
+	pll = &dev_priv->shared_dplls[dpll_id];
+	if (WARN_ON(pll->active_mask)) {
+		DRM_ERROR("Shared DPLL already in use. active_mask:%x\n", pll->active_mask);
+		mutex_unlock(&dev_priv->dpll_lock);
+		return -EINVAL;
+	}
+
+	tmp_pll_config = pll->config;
+
+	DRM_DEBUG_KMS("Using pipe %c with pll %s\n",
+			pipe_name(crtc->pipe), pll->name);
+	do {
+		crtc->config->port_clock = drm_dp_bw_code_to_link_rate(link_bw);
+		crtc->config->lane_count = lane_count;
+
+		bxt_ddi_dp_pll_dividers(crtc->config->port_clock, &clk_div);
+		if (!bxt_ddi_set_dpll_hw_state(crtc->config->port_clock,
+					&clk_div, &pll->config.hw_state)) {
+			DRM_ERROR("Could not setup DPLL\n");
+			goto exit_pll;
+		}
+
+		/* Enable PLL followed by port */
+		pll->funcs.enable(dev_priv, pll);
+		encoder->pre_enable(encoder);
+
+		/* Check if link training passed; if so update DPCD */
+		if (intel_dp->train_set_valid)
+			intel_dp_update_dpcd_params(intel_dp);
+
+		/* Disable port followed by PLL for next retry/clean up */
+		encoder->post_disable(encoder);
+		pll->funcs.disable(dev_priv, pll);
+
+	} while (!intel_dp->train_set_valid &&
+		intel_dp_get_link_retry_params(intel_dp, &lane_count, &link_bw));
+
+	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
+	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count, link_bw);
+
+exit_pll:
+	pll->config = tmp_pll_config;
+	mutex_unlock(&dev_priv->dpll_lock);
+
+	return intel_dp->train_set_valid ? 0 : -1;
+}
+
 void intel_ddi_init(struct drm_device *dev, enum port port)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 61ee226..57d7159 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1595,6 +1595,41 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
 	intel_dp->lane_count = pipe_config->lane_count;
 }
 
+void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
+{
+	intel_dp->dpcd[DP_MAX_LANE_COUNT] &= ~DP_MAX_LANE_COUNT_MASK;
+	intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
+			intel_dp->lane_count & DP_MAX_LANE_COUNT_MASK;
+
+	intel_dp->dpcd[DP_MAX_LINK_RATE] =
+			drm_dp_link_rate_to_bw_code(intel_dp->link_rate);
+}
+
+bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
+			uint8_t *lane_count, uint8_t *link_bw)
+{
+	/*
+	 * As per DP1.3 Spec, retry all link rates for a particular
+	 * lane count value, before reducing number of lanes.
+	 */
+	if (*link_bw == DP_LINK_BW_5_4) {
+		*link_bw = DP_LINK_BW_2_7;
+	} else if (*link_bw == DP_LINK_BW_2_7) {
+		*link_bw = DP_LINK_BW_1_62;
+	} else if (*lane_count == 4) {
+		*lane_count = 2;
+		*link_bw = intel_dp_max_link_bw(intel_dp);
+	} else if (*lane_count == 2) {
+		*lane_count = 1;
+		*link_bw = intel_dp_max_link_bw(intel_dp);
+	} else {
+		/* Tried all combinations, so exit */
+		return false;
+	}
+
+	return true;
+}
+
 static void intel_dp_prepare(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -4548,6 +4583,135 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_dp->has_audio = false;
 }
 
+static struct intel_crtc_state *intel_dp_upfront_get_crtc_state(
+				struct intel_crtc *crtc,
+				struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_atomic_state *state;
+	struct intel_crtc_state *crtc_state;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	state->acquire_ctx = ctx;
+
+	crtc_state = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state)) {
+		drm_atomic_state_free(state);
+		state = NULL;
+	}
+
+	return crtc_state;
+}
+
+static int intel_dp_upfront_commit(struct intel_crtc *crtc,
+				struct drm_modeset_acquire_ctx *ctx)
+{
+	int ret;
+	struct intel_crtc_state *crtc_state;
+	enum pipe pipe = crtc->pipe;
+
+	crtc_state = intel_dp_upfront_get_crtc_state(crtc, ctx);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	DRM_DEBUG_KMS("Disabling crtc %c for upfront link train\n", pipe_name(pipe));
+
+	crtc_state->base.active = false;
+	ret = drm_atomic_commit(crtc_state->base.state);
+	if (ret) {
+		drm_atomic_state_free(crtc_state->base.state);
+		crtc_state->base.state = NULL;
+	}
+	return ret;
+}
+
+static int intel_dp_upfront_link_train(struct drm_connector *connector)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx ctx;
+	struct intel_crtc_state *crtc_state, *tmp_crtc_config;
+	struct intel_crtc *intel_crtc;
+	struct drm_crtc *crtc = NULL;
+	bool crtc_enabled = false;
+	int ret, status = 0;
+
+	if (!IS_BROXTON(dev))
+		return 0;
+
+	drm_modeset_acquire_init(&ctx, 0);
+retry:
+	ret = drm_modeset_lock(&config->connection_mutex, &ctx);
+	if (ret)
+		goto exit_fail;
+
+	if (connector->state->crtc) {
+		crtc = connector->state->crtc;
+
+		ret = drm_modeset_lock(&crtc->mutex, &ctx);
+		if (ret)
+			goto exit_fail;
+
+		crtc_enabled = true;
+	} else {
+		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
+		if (IS_ERR_OR_NULL(crtc)) {
+			ret = PTR_ERR_OR_ZERO(crtc);
+			DRM_DEBUG_KMS("No crtc available for upfront link train:%d\n", ret);
+			goto exit_fail;
+		}
+		intel_encoder->base.crtc = crtc;
+	}
+
+	intel_crtc = to_intel_crtc(crtc);
+	DRM_DEBUG_KMS("Using pipe %c for upfront link training\n",
+					pipe_name(intel_crtc->pipe));
+
+	ret = drm_modeset_lock(&crtc->primary->mutex, &ctx);
+	if (ret)
+		goto exit_fail;
+
+	if (crtc_enabled) {
+		ret = intel_dp_upfront_commit(intel_crtc, &ctx);
+		if (ret)
+			goto exit_fail;
+	}
+
+	crtc_state = intel_dp_upfront_get_crtc_state(intel_crtc, &ctx);
+	if (IS_ERR(crtc_state)) {
+		ret = PTR_ERR(crtc_state);
+		goto exit_fail;
+	}
+
+	/* Save the existing config */
+	tmp_crtc_config = intel_crtc->config;
+	intel_crtc->config = crtc_state;
+
+	if (IS_BROXTON(dev))
+		status = intel_bxt_upfront_link_train(intel_dp, intel_crtc);
+		/* Other platforms upfront link train call goes here..*/
+
+	/* Restore the saved config */
+	intel_crtc->config = tmp_crtc_config;
+	intel_encoder->base.crtc = crtc_enabled ? crtc : NULL;
+	drm_atomic_state_free(crtc_state->base.state);
+
+exit_fail:
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+	return status;
+}
+
 static void
 intel_dp_long_pulse(struct intel_connector *intel_connector)
 {
@@ -4558,7 +4722,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	struct drm_device *dev = connector->dev;
 	enum drm_connector_status status;
 	enum intel_display_power_domain power_domain;
-	bool ret;
+	bool ret, do_upfront_link_train;
 	u8 sink_irq_vector;
 
 	power_domain = intel_display_port_aux_power_domain(intel_encoder);
@@ -4604,7 +4768,11 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 		intel_dp_check_link_status(intel_dp);
 		drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		goto out;
+		/*
+		 * If we are here, we have (re)read DPCD above and hence
+		 * need to do upfront link train again.
+		 */
+		goto upfront;
 	}
 
 	/*
@@ -4634,6 +4802,14 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
+upfront:
+	/* Do not do upfront link train, if it is a compliance request */
+	do_upfront_link_train =
+		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
+		intel_dp->compliance_test_type != DP_TEST_LINK_TRAINING;
+
+	if (do_upfront_link_train && intel_dp_upfront_link_train(connector))
+		status = connector_status_disconnected;
 out:
 	if (status != connector_status_connected) {
 		intel_dp_unset_edid(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 27b5dcd..bf98473 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1082,6 +1082,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
 			 struct intel_crtc_state *pipe_config);
 void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
 uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
+int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
+				struct intel_crtc *crtc);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
@@ -1284,6 +1286,9 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			     struct intel_connector *intel_connector);
 void intel_dp_set_link_params(struct intel_dp *intel_dp,
 			      const struct intel_crtc_state *pipe_config);
+void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
+bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
+				uint8_t *lane_count, uint8_t *link_bw);
 void intel_dp_start_link_train(struct intel_dp *intel_dp);
 void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
-- 
1.9.1

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

Re: [PATCHv4 3/3] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Ander Conselvan De Oliveira]

On Mon, 2016-04-18 at 19:01 +0530, Durgadoss R wrote:
> To support USB type C alternate DP mode, the display driver needs to
> know the number of lanes required by the DP panel as well as number
> of lanes that can be supported by the type-C cable. Sometimes, the
> type-C cable may limit the bandwidth even if Panel can support
> more lanes. To address these scenarios, the display driver will
> start link training with max lanes, and if that fails, the driver
> falls back to x2 lanes; and repeats this procedure for all
> bandwidth/lane configurations.
> 
> * Since link training is done before modeset only the port
>   (and not pipe/planes) and its associated PLLs are enabled.
> * On DP hotplug: Directly start link training on the crtc
>   associated with the DP encoder.
> * On Connected boot scenarios: When booted with an LFP and a DP,
>   typically, BIOS brings up DP. In these cases, we disable the
>   crtc and then do upfront link training; and let the subsequent
>   modeset re-enable the crtc.
> * All local changes made for upfront link training are reset
>   to their previous values once it is done; so that the
>   subsequent modeset is not aware of these changes.
> 
> Changes since v3:
> * Fixed a return value on BXT check
> * Reworked on top of bxt_ddi_pll_select split from Ander
> * Renamed from ddi_upfront to bxt_upfront since the
>   upfront logic includes BXT specific functions for now.
> Changes since v2:
> * Rebased on top of latest dpll_mgr.c code and
>   latest HPD related clean ups.
> * Corrected return values from upfront (Ander)
> * Corrected atomic locking for upfront in intel_dp.c (Ville)
> Changes since v1:
> *  all pll related functions inside ddi.c
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |  73 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 180
> ++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h |   5 ++
>  3 files changed, 256 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 96234c5..f7fa3db 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2268,6 +2268,79 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port
> *intel_dig_port)
>  	return connector;
>  }
>  
> +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
> +				struct intel_crtc *crtc)
> +{
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	struct intel_encoder *encoder = connector->encoder;
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_shared_dpll *pll;
> +	struct intel_shared_dpll_config tmp_pll_config;
> +	struct bxt_clk_div clk_div = {0};
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	enum intel_dpll_id dpll_id = (enum intel_dpll_id)dig_port->port;
> +	uint8_t link_bw, lane_count;
> +
> +	if (WARN_ON(!crtc))
> +		return -EINVAL;
> +
> +	/* Initialize with Max Link rate & lane count supported by panel */
> +	link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
> +	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);

What if the panel supports HBR3? We should use the maximum common rate between
source and sink. And it would be nice if those details where in intel_dp.c.
Ideally, the platform specific part would take link rate and lane count as
arguments and just do the pll/encoder enabling/disabling.

> +
> +	mutex_lock(&dev_priv->dpll_lock);
> +	/*
> +	 * FIXME: Works only for BXT.
> +	 * Select the required PLL. This works for platforms where
> +	 * there is no shared DPLL.
> +	 */
> +	pll = &dev_priv->shared_dplls[dpll_id];
> +	if (WARN_ON(pll->active_mask)) {
> +		DRM_ERROR("Shared DPLL already in use. active_mask:%x\n",
> pll->active_mask);
> +		mutex_unlock(&dev_priv->dpll_lock);
> +		return -EINVAL;
> +	}
> +
> +	tmp_pll_config = pll->config;
> +
> +	DRM_DEBUG_KMS("Using pipe %c with pll %s\n",
> +			pipe_name(crtc->pipe), pll->name);
> +	do {
> +		crtc->config->port_clock =
> drm_dp_bw_code_to_link_rate(link_bw);
> +		crtc->config->lane_count = lane_count;
> +
> +		bxt_ddi_dp_pll_dividers(crtc->config->port_clock, &clk_div);
> +		if (!bxt_ddi_set_dpll_hw_state(crtc->config->port_clock,
> +					&clk_div, &pll->config.hw_state)) {
> +			DRM_ERROR("Could not setup DPLL\n");
> +			goto exit_pll;
> +		}
> +
> +		/* Enable PLL followed by port */
> +		pll->funcs.enable(dev_priv, pll);
> +		encoder->pre_enable(encoder);
> +
> +		/* Check if link training passed; if so update DPCD */
> +		if (intel_dp->train_set_valid)
> +			intel_dp_update_dpcd_params(intel_dp);
> +
> +		/* Disable port followed by PLL for next retry/clean up */
> +		encoder->post_disable(encoder);
> +		pll->funcs.disable(dev_priv, pll);
> +
> +	} while (!intel_dp->train_set_valid &&
> +		intel_dp_get_link_retry_params(intel_dp, &lane_count,
> &link_bw));
> +
> +	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
> +	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
> link_bw);
> +
> +exit_pll:
> +	pll->config = tmp_pll_config;
> +	mutex_unlock(&dev_priv->dpll_lock);
> +
> +	return intel_dp->train_set_valid ? 0 : -1;
> +}
> +
>  void intel_ddi_init(struct drm_device *dev, enum port port)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 61ee226..57d7159 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1595,6 +1595,41 @@ void intel_dp_set_link_params(struct intel_dp
> *intel_dp,
>  	intel_dp->lane_count = pipe_config->lane_count;
>  }
>  
> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
> +{
> +	intel_dp->dpcd[DP_MAX_LANE_COUNT] &= ~DP_MAX_LANE_COUNT_MASK;
> +	intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
> +			intel_dp->lane_count & DP_MAX_LANE_COUNT_MASK;
> +
> +	intel_dp->dpcd[DP_MAX_LINK_RATE] =
> +			drm_dp_link_rate_to_bw_code(intel_dp->link_rate);

I think I mentioned this before. I don't think we should update the dpcd fields
directly. Instead, there should be a field for the max rate and lane count
achieved through upfront link training in struct intel_dp and
intel_dp_max_link_bw() and intel_dp_max_lane_count() should take that into
account.

> +}
> +
> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> +			uint8_t *lane_count, uint8_t *link_bw)
> +{
> +	/*
> +	 * As per DP1.3 Spec, retry all link rates for a particular
> +	 * lane count value, before reducing number of lanes.
> +	 */
> +	if (*link_bw == DP_LINK_BW_5_4) {
> +		*link_bw = DP_LINK_BW_2_7;
> +	} else if (*link_bw == DP_LINK_BW_2_7) {
> +		*link_bw = DP_LINK_BW_1_62;
> +	} else if (*lane_count == 4) {
> +		*lane_count = 2;
> +		*link_bw = intel_dp_max_link_bw(intel_dp);
> +	} else if (*lane_count == 2) {
> +		*lane_count = 1;
> +		*link_bw = intel_dp_max_link_bw(intel_dp);
> +	} else {
> +		/* Tried all combinations, so exit */
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static void intel_dp_prepare(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
> @@ -4548,6 +4583,135 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  	intel_dp->has_audio = false;
>  }
>  
> +static struct intel_crtc_state *intel_dp_upfront_get_crtc_state(
> +				struct intel_crtc *crtc,
> +				struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_atomic_state *state;
> +	struct intel_crtc_state *crtc_state;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	state->acquire_ctx = ctx;
> +
> +	crtc_state = intel_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state)) {
> +		drm_atomic_state_free(state);
> +		state = NULL;
> +	}

All this function does is call intel_atomic_get_crtc_state(). Since the caller
also have to deal with error handling, I don't see what it adds on top of that.
The code is actually simpler if merged with the function below.

> +
> +	return crtc_state;
> +}
> +
> +static int intel_dp_upfront_commit(struct intel_crtc *crtc,
> +				struct drm_modeset_acquire_ctx *ctx)

This function could use a better name. Something that suggests the crtc is being
disabled. Maybe disable_crtc_for_upfront() ? It doens't really depends on
anything DP specific, so maybe it would make sense to move it to intel_display.c
or intel_atomic.c. Then we could call it intel_crtc_atomic_disable_locked(). The
"locked" part since this assumes the ctx already have crtc->mutex locked so that
there's no risk of -EDEADLK. Although it can just pass the -EDEADLK error to the
caller, so just intel_crtc_atomic_disable() ? 



> +{
> +	int ret;
> +	struct intel_crtc_state *crtc_state;
> +	enum pipe pipe = crtc->pipe;
> +
> +	crtc_state = intel_dp_upfront_get_crtc_state(crtc, ctx);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	DRM_DEBUG_KMS("Disabling crtc %c for upfront link train\n",
> pipe_name(pipe));
> +
> +	crtc_state->base.active = false;
> +	ret = drm_atomic_commit(crtc_state->base.state);
> +	if (ret) {
> +		drm_atomic_state_free(crtc_state->base.state);
> +		crtc_state->base.state = NULL;

The function drm_atomic_state_free() frees the crtc_state, so this is a use
after free.

> +	}
> +	return ret;
> +}
> +
> +static int intel_dp_upfront_link_train(struct drm_connector *connector)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);

You could just pass intel_dp directly. The only use of connector is when
connector->state->crtc is inspected. But that should have the same value as
intel_dp->base.base.crtc

> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct drm_device *dev = intel_encoder->base.dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct intel_crtc_state *crtc_state, *tmp_crtc_config;
> +	struct intel_crtc *intel_crtc;
> +	struct drm_crtc *crtc = NULL;
> +	bool crtc_enabled = false;
> +	int ret, status = 0;
> +
> +	if (!IS_BROXTON(dev))
> +		return 0;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +retry:
> +	ret = drm_modeset_lock(&config->connection_mutex, &ctx);
> +	if (ret)
> +		goto exit_fail;
> +
> +	if (connector->state->crtc) {
> +		crtc = connector->state->crtc;
> +
> +		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> +		if (ret)
> +			goto exit_fail;
> +
> +		crtc_enabled = true;
> +	} else {
> +		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
> +		if (IS_ERR_OR_NULL(crtc)) {
> +			ret = PTR_ERR_OR_ZERO(crtc);
> +			DRM_DEBUG_KMS("No crtc available for upfront link
> train:%d\n", ret);
> +			goto exit_fail;
> +		}
> +		intel_encoder->base.crtc = crtc;
> +	}

IMO, the following would be a bit more readable.

	if (intel_encoder->base.crtc)
		crtc = intel_encoder->base.crtc;
	else
		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);

	if (IS_ERR(crtc)) {
		...
	}

	ret = drm_modeset_lock(&crtc->mutex, &ctx);
	if (ret)
		goto exit_fail;

	crtc_enabled = crtc->config->active;

> +
> +	intel_crtc = to_intel_crtc(crtc);
> +	DRM_DEBUG_KMS("Using pipe %c for upfront link training\n",
> +					pipe_name(intel_crtc->pipe));
> +
> +	ret = drm_modeset_lock(&crtc->primary->mutex, &ctx);
> +	if (ret)
> +		goto exit_fail;
> +
> +	if (crtc_enabled) {
> +		ret = intel_dp_upfront_commit(intel_crtc, &ctx);
> +		if (ret)
> +			goto exit_fail;
> +	}
> +
> +	crtc_state = intel_dp_upfront_get_crtc_state(intel_crtc, &ctx);
> +	if (IS_ERR(crtc_state)) {
> +		ret = PTR_ERR(crtc_state);
> +		goto exit_fail;
> +	}
> +
> +	/* Save the existing config */
> +	tmp_crtc_config = intel_crtc->config;

You need to save this earlier. Otherwise if intel_dp_upfront_commit() is called,
the state will be restored with active == false when it should actually be true.
But that would cause intel_crtc->config to be freed, so you need to
use intel_crtc_duplicate_state().

> +	intel_crtc->config = crtc_state;
> +
> +	if (IS_BROXTON(dev))
> +		status = intel_bxt_upfront_link_train(intel_dp, intel_crtc);
> +		/* Other platforms upfront link train call goes here..*/
> +
> +	/* Restore the saved config */
> +	intel_crtc->config = tmp_crtc_config;
> +	intel_encoder->base.crtc = crtc_enabled ? crtc : NULL;
> +	drm_atomic_state_free(crtc_state->base.state);

This doesn't actually re-enables the crtc if if was disabled in
intel_dp_upfront_commit(). This will require another atomic operation.
Maybe that intel_crtc_atomic_disable() should just take a bool specifying if it
should on of off.


> +
> +exit_fail:
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	return status;
> +}
> +
>  static void
>  intel_dp_long_pulse(struct intel_connector *intel_connector)
>  {
> @@ -4558,7 +4722,7 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  	struct drm_device *dev = connector->dev;
>  	enum drm_connector_status status;
>  	enum intel_display_power_domain power_domain;
> -	bool ret;
> +	bool ret, do_upfront_link_train;
>  	u8 sink_irq_vector;
>  
>  	power_domain = intel_display_port_aux_power_domain(intel_encoder);
> @@ -4604,7 +4768,11 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  		intel_dp_check_link_status(intel_dp);
>  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -		goto out;
> +		/*
> +		 * If we are here, we have (re)read DPCD above and hence
> +		 * need to do upfront link train again.
> +		 */
> +		goto upfront;

We can avoid this by not overwriting intel_dp->dpcd as I mentioned above.


Ander

>  	}
>  
>  	/*
> @@ -4634,6 +4802,14 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq
> unhandled\n");
>  	}
>  
> +upfront:
> +	/* Do not do upfront link train, if it is a compliance request */
> +	do_upfront_link_train =
> +		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
> +		intel_dp->compliance_test_type != DP_TEST_LINK_TRAINING;
> +
> +	if (do_upfront_link_train && intel_dp_upfront_link_train(connector))
> +		status = connector_status_disconnected;
>  out:
>  	if (status != connector_status_connected) {
>  		intel_dp_unset_edid(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 27b5dcd..bf98473 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1082,6 +1082,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>  			 struct intel_crtc_state *pipe_config);
>  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
>  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
> +				struct intel_crtc *crtc);
>  
>  /* intel_frontbuffer.c */
>  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> @@ -1284,6 +1286,9 @@ bool intel_dp_init_connector(struct intel_digital_port
> *intel_dig_port,
>  			     struct intel_connector *intel_connector);
>  void intel_dp_set_link_params(struct intel_dp *intel_dp,
>  			      const struct intel_crtc_state *pipe_config);
> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> +				uint8_t *lane_count, uint8_t *link_bw);
>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCHv4 3/3] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[R, Durgadoss]

Hi Ander,

Thanks for looking at it.
Few queries below..

> -----Original Message-----
> From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
> Sent: Friday, May 6, 2016 6:39 PM
> To: R, Durgadoss <durgadoss.r@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCHv4 3/3] drm/i915/dp: Enable Upfront link
> training for typeC DP support on BXT
> 
> On Mon, 2016-04-18 at 19:01 +0530, Durgadoss R wrote:
> > To support USB type C alternate DP mode, the display driver needs to
> > know the number of lanes required by the DP panel as well as number
> > of lanes that can be supported by the type-C cable. Sometimes, the
> > type-C cable may limit the bandwidth even if Panel can support
> > more lanes. To address these scenarios, the display driver will
> > start link training with max lanes, and if that fails, the driver
> > falls back to x2 lanes; and repeats this procedure for all
> > bandwidth/lane configurations.
> >
> > * Since link training is done before modeset only the port
> >   (and not pipe/planes) and its associated PLLs are enabled.
> > * On DP hotplug: Directly start link training on the crtc
> >   associated with the DP encoder.
> > * On Connected boot scenarios: When booted with an LFP and a DP,
> >   typically, BIOS brings up DP. In these cases, we disable the
> >   crtc and then do upfront link training; and let the subsequent
> >   modeset re-enable the crtc.
> > * All local changes made for upfront link training are reset
> >   to their previous values once it is done; so that the
> >   subsequent modeset is not aware of these changes.
> >
> > Changes since v3:
> > * Fixed a return value on BXT check
> > * Reworked on top of bxt_ddi_pll_select split from Ander
> > * Renamed from ddi_upfront to bxt_upfront since the
> >   upfront logic includes BXT specific functions for now.
> > Changes since v2:
> > * Rebased on top of latest dpll_mgr.c code and
> >   latest HPD related clean ups.
> > * Corrected return values from upfront (Ander)
> > * Corrected atomic locking for upfront in intel_dp.c (Ville)
> > Changes since v1:
> > *  all pll related functions inside ddi.c
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c |  73 ++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c  | 180
> > ++++++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h |   5 ++
> >  3 files changed, 256 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 96234c5..f7fa3db 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2268,6 +2268,79 @@ intel_ddi_init_hdmi_connector(struct
> intel_digital_port
> > *intel_dig_port)
> >  	return connector;
> >  }
> >
> > +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
> > +				struct intel_crtc *crtc)
> > +{
> > +	struct intel_connector *connector = intel_dp->attached_connector;
> > +	struct intel_encoder *encoder = connector->encoder;
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_shared_dpll *pll;
> > +	struct intel_shared_dpll_config tmp_pll_config;
> > +	struct bxt_clk_div clk_div = {0};
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	enum intel_dpll_id dpll_id = (enum intel_dpll_id)dig_port->port;
> > +	uint8_t link_bw, lane_count;
> > +
> > +	if (WARN_ON(!crtc))
> > +		return -EINVAL;
> > +
> > +	/* Initialize with Max Link rate & lane count supported by panel */
> > +	link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
> > +	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> 
> What if the panel supports HBR3? We should use the maximum common rate

Ok, today that will add one more iteration of Link training before going
down to a supported rate. 

> between source and sink. And it would be nice if those details where in intel_dp.c.
> Ideally, the platform specific part would take link rate and lane count as
> arguments and just do the pll/encoder enabling/disabling.

May be with the new {link_bw/lane_count}_upfront variables in intel_dp structure,
we could do this easily. Will try this implementation and see how it comes up.

> 
> > +
> > +	mutex_lock(&dev_priv->dpll_lock);
> > +	/*
> > +	 * FIXME: Works only for BXT.
> > +	 * Select the required PLL. This works for platforms where
> > +	 * there is no shared DPLL.
> > +	 */
> > +	pll = &dev_priv->shared_dplls[dpll_id];
> > +	if (WARN_ON(pll->active_mask)) {
> > +		DRM_ERROR("Shared DPLL already in use.
> active_mask:%x\n",
> > pll->active_mask);
> > +		mutex_unlock(&dev_priv->dpll_lock);
> > +		return -EINVAL;
> > +	}
> > +
> > +	tmp_pll_config = pll->config;
> > +
> > +	DRM_DEBUG_KMS("Using pipe %c with pll %s\n",
> > +			pipe_name(crtc->pipe), pll->name);
> > +	do {
> > +		crtc->config->port_clock =
> > drm_dp_bw_code_to_link_rate(link_bw);
> > +		crtc->config->lane_count = lane_count;
> > +
> > +		bxt_ddi_dp_pll_dividers(crtc->config->port_clock, &clk_div);
> > +		if (!bxt_ddi_set_dpll_hw_state(crtc->config->port_clock,
> > +					&clk_div, &pll->config.hw_state)) {
> > +			DRM_ERROR("Could not setup DPLL\n");
> > +			goto exit_pll;
> > +		}
> > +
> > +		/* Enable PLL followed by port */
> > +		pll->funcs.enable(dev_priv, pll);
> > +		encoder->pre_enable(encoder);
> > +
> > +		/* Check if link training passed; if so update DPCD */
> > +		if (intel_dp->train_set_valid)
> > +			intel_dp_update_dpcd_params(intel_dp);
> > +
> > +		/* Disable port followed by PLL for next retry/clean up */
> > +		encoder->post_disable(encoder);
> > +		pll->funcs.disable(dev_priv, pll);
> > +
> > +	} while (!intel_dp->train_set_valid &&
> > +		intel_dp_get_link_retry_params(intel_dp, &lane_count,
> > &link_bw));
> > +
> > +	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
> > +	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
> > link_bw);
> > +
> > +exit_pll:
> > +	pll->config = tmp_pll_config;
> > +	mutex_unlock(&dev_priv->dpll_lock);
> > +
> > +	return intel_dp->train_set_valid ? 0 : -1;
> > +}
> > +
> >  void intel_ddi_init(struct drm_device *dev, enum port port)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> > index 61ee226..57d7159 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1595,6 +1595,41 @@ void intel_dp_set_link_params(struct intel_dp
> > *intel_dp,
> >  	intel_dp->lane_count = pipe_config->lane_count;
> >  }
> >
> > +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
> > +{
> > +	intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
> ~DP_MAX_LANE_COUNT_MASK;
> > +	intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
> > +			intel_dp->lane_count &
> DP_MAX_LANE_COUNT_MASK;
> > +
> > +	intel_dp->dpcd[DP_MAX_LINK_RATE] =
> > +			drm_dp_link_rate_to_bw_code(intel_dp-
> >link_rate);
> 
> I think I mentioned this before. I don't think we should update the dpcd
> fields
> directly. Instead, there should be a field for the max rate and lane count
> achieved through upfront link training in struct intel_dp and
> intel_dp_max_link_bw() and intel_dp_max_lane_count() should take that
> into
> account.

I thought we agreed for having that change as a later implementation.
Anyway, it's not a big change; So I will do as part of next revision,
probably as a separate patch.

> 
> > +}
> > +
> > +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> > +			uint8_t *lane_count, uint8_t *link_bw)
> > +{
> > +	/*
> > +	 * As per DP1.3 Spec, retry all link rates for a particular
> > +	 * lane count value, before reducing number of lanes.
> > +	 */
> > +	if (*link_bw == DP_LINK_BW_5_4) {
> > +		*link_bw = DP_LINK_BW_2_7;
> > +	} else if (*link_bw == DP_LINK_BW_2_7) {
> > +		*link_bw = DP_LINK_BW_1_62;
> > +	} else if (*lane_count == 4) {
> > +		*lane_count = 2;
> > +		*link_bw = intel_dp_max_link_bw(intel_dp);
> > +	} else if (*lane_count == 2) {
> > +		*lane_count = 1;
> > +		*link_bw = intel_dp_max_link_bw(intel_dp);
> > +	} else {
> > +		/* Tried all combinations, so exit */
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  static void intel_dp_prepare(struct intel_encoder *encoder)
> >  {
> >  	struct drm_device *dev = encoder->base.dev;
> > @@ -4548,6 +4583,135 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> >  	intel_dp->has_audio = false;
> >  }
> >
> > +static struct intel_crtc_state *intel_dp_upfront_get_crtc_state(
> > +				struct intel_crtc *crtc,
> > +				struct drm_modeset_acquire_ctx *ctx)
> > +{
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_atomic_state *state;
> > +	struct intel_crtc_state *crtc_state;
> > +
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	state->acquire_ctx = ctx;
> > +
> > +	crtc_state = intel_atomic_get_crtc_state(state, crtc);
> > +	if (IS_ERR(crtc_state)) {
> > +		drm_atomic_state_free(state);
> > +		state = NULL;
> > +	}
> 
> All this function does is call intel_atomic_get_crtc_state(). Since the caller
> also have to deal with error handling, I don't see what it adds on top of that.
> The code is actually simpler if merged with the function below.

This function needs to be called from two places. One in the commit() below,
and the other in the upfront_link_train() itself, when the crtc is not
enabled already.

> 
> > +
> > +	return crtc_state;
> > +}
> > +
> > +static int intel_dp_upfront_commit(struct intel_crtc *crtc,
> > +				struct drm_modeset_acquire_ctx *ctx)
> 
> This function could use a better name. Something that suggests the crtc is
> being disabled. Maybe disable_crtc_for_upfront() ? It doens't really depends on

Sure, better name makes sense..

> anything DP specific, so maybe it would make sense to move it to
> intel_display.c  or intel_atomic.c. Then we could call it intel_crtc_atomic_disable_locked().
> The "locked" part since this assumes the ctx already have crtc->mutex locked so
> that there's no risk of -EDEADLK. Although it can just pass the -EDEADLK error to
> the caller, so just intel_crtc_atomic_disable() ?

Still this would need to call the above get_crtc_state(..) function.
So, I will re-name and if it all looks Ok, will move it to atomic.c
Otherwise, will keep it here..

> 
> 
> 
> > +{
> > +	int ret;
> > +	struct intel_crtc_state *crtc_state;
> > +	enum pipe pipe = crtc->pipe;
> > +
> > +	crtc_state = intel_dp_upfront_get_crtc_state(crtc, ctx);
> > +	if (IS_ERR(crtc_state))
> > +		return PTR_ERR(crtc_state);
> > +
> > +	DRM_DEBUG_KMS("Disabling crtc %c for upfront link train\n",
> > pipe_name(pipe));
> > +
> > +	crtc_state->base.active = false;
> > +	ret = drm_atomic_commit(crtc_state->base.state);
> > +	if (ret) {
> > +		drm_atomic_state_free(crtc_state->base.state);
> > +		crtc_state->base.state = NULL;
> 
> The function drm_atomic_state_free() frees the crtc_state, so this is a use
> after free.

Will fix this..

> 
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int intel_dp_upfront_link_train(struct drm_connector *connector)
> > +{
> > +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> 
> You could just pass intel_dp directly. The only use of connector is when
> connector->state->crtc is inspected. But that should have the same value as
> intel_dp->base.base.crtc

Ok, will change.

> 
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > +	struct drm_device *dev = intel_encoder->base.dev;
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct intel_crtc_state *crtc_state, *tmp_crtc_config;
> > +	struct intel_crtc *intel_crtc;
> > +	struct drm_crtc *crtc = NULL;
> > +	bool crtc_enabled = false;
> > +	int ret, status = 0;
> > +
> > +	if (!IS_BROXTON(dev))
> > +		return 0;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +retry:
> > +	ret = drm_modeset_lock(&config->connection_mutex, &ctx);
> > +	if (ret)
> > +		goto exit_fail;
> > +
> > +	if (connector->state->crtc) {
> > +		crtc = connector->state->crtc;
> > +
> > +		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> > +		if (ret)
> > +			goto exit_fail;
> > +
> > +		crtc_enabled = true;
> > +	} else {
> > +		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
> > +		if (IS_ERR_OR_NULL(crtc)) {
> > +			ret = PTR_ERR_OR_ZERO(crtc);
> > +			DRM_DEBUG_KMS("No crtc available for upfront link
> > train:%d\n", ret);
> > +			goto exit_fail;
> > +		}
> > +		intel_encoder->base.crtc = crtc;
> > +	}
> 
> IMO, the following would be a bit more readable.
> 
> 	if (intel_encoder->base.crtc)
> 		crtc = intel_encoder->base.crtc;
> 	else
> 		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
> 
> 	if (IS_ERR(crtc)) {
> 		...
> 	}
> 
> 	ret = drm_modeset_lock(&crtc->mutex, &ctx);
> 	if (ret)
> 		goto exit_fail;
> 
> 	crtc_enabled = crtc->config->active;
> 

Ok, makes sense. Will re-order this way.

> > +
> > +	intel_crtc = to_intel_crtc(crtc);
> > +	DRM_DEBUG_KMS("Using pipe %c for upfront link training\n",
> > +					pipe_name(intel_crtc->pipe));
> > +
> > +	ret = drm_modeset_lock(&crtc->primary->mutex, &ctx);
> > +	if (ret)
> > +		goto exit_fail;
> > +
> > +	if (crtc_enabled) {
> > +		ret = intel_dp_upfront_commit(intel_crtc, &ctx);
> > +		if (ret)
> > +			goto exit_fail;
> > +	}
> > +
> > +	crtc_state = intel_dp_upfront_get_crtc_state(intel_crtc, &ctx);
> > +	if (IS_ERR(crtc_state)) {
> > +		ret = PTR_ERR(crtc_state);
> > +		goto exit_fail;
> > +	}
> > +
> > +	/* Save the existing config */
> > +	tmp_crtc_config = intel_crtc->config;
> 
> You need to save this earlier. Otherwise if intel_dp_upfront_commit() is
> called, the state will be restored with active == false when it should actually be true.

Ok, will save this earlier. But since we don’t re-enable crtc  here after upfront,
(we let it get re-enabled as part of subsequent modeset) the restoring
with false does not happen today..

> But that would cause intel_crtc->config to be freed, so you need to
> use intel_crtc_duplicate_state().

Since we alloc a new state and then only call get_crtc_state(), I thought that
automatically boils down to duplicate_state() inside drm_atomic_get_crtc_state().
Please correct if my understanding is wrong.

> 
> > +	intel_crtc->config = crtc_state;
> > +
> > +	if (IS_BROXTON(dev))
> > +		status = intel_bxt_upfront_link_train(intel_dp, intel_crtc);
> > +		/* Other platforms upfront link train call goes here..*/
> > +
> > +	/* Restore the saved config */
> > +	intel_crtc->config = tmp_crtc_config;
> > +	intel_encoder->base.crtc = crtc_enabled ? crtc : NULL;
> > +	drm_atomic_state_free(crtc_state->base.state);
> 
> This doesn't actually re-enables the crtc if if was disabled in
> intel_dp_upfront_commit(). This will require another atomic operation.
> Maybe that intel_crtc_atomic_disable() should just take a bool specifying if it
> should on of off.

We let it get re-enabled as part of mode set that happens subsequently.
Please let me know your opinion on this.

Thanks,
Durga

> 
> 
> > +
> > +exit_fail:
> > +	if (ret == -EDEADLK) {
> > +		drm_modeset_backoff(&ctx);
> > +		goto retry;
> > +	}
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +	return status;
> > +}
> > +
> >  static void
> >  intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  {
> > @@ -4558,7 +4722,7 @@ intel_dp_long_pulse(struct intel_connector
> > *intel_connector)
> >  	struct drm_device *dev = connector->dev;
> >  	enum drm_connector_status status;
> >  	enum intel_display_power_domain power_domain;
> > -	bool ret;
> > +	bool ret, do_upfront_link_train;
> >  	u8 sink_irq_vector;
> >
> >  	power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> > @@ -4604,7 +4768,11 @@ intel_dp_long_pulse(struct intel_connector
> > *intel_connector)
> >  		drm_modeset_lock(&dev-
> >mode_config.connection_mutex, NULL);
> >  		intel_dp_check_link_status(intel_dp);
> >  		drm_modeset_unlock(&dev-
> >mode_config.connection_mutex);
> > -		goto out;
> > +		/*
> > +		 * If we are here, we have (re)read DPCD above and hence
> > +		 * need to do upfront link train again.
> > +		 */
> > +		goto upfront;
> 
> We can avoid this by not overwriting intel_dp->dpcd as I mentioned above.
> 
> 
> Ander
> 
> >  	}
> >
> >  	/*
> > @@ -4634,6 +4802,14 @@ intel_dp_long_pulse(struct intel_connector
> > *intel_connector)
> >  			DRM_DEBUG_DRIVER("CP or sink specific irq
> > unhandled\n");
> >  	}
> >
> > +upfront:
> > +	/* Do not do upfront link train, if it is a compliance request */
> > +	do_upfront_link_train =
> > +		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
> > +		intel_dp->compliance_test_type !=
> DP_TEST_LINK_TRAINING;
> > +
> > +	if (do_upfront_link_train &&
> intel_dp_upfront_link_train(connector))
> > +		status = connector_status_disconnected;
> >  out:
> >  	if (status != connector_status_connected) {
> >  		intel_dp_unset_edid(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 27b5dcd..bf98473 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1082,6 +1082,8 @@ void intel_ddi_clock_get(struct intel_encoder
> *encoder,
> >  			 struct intel_crtc_state *pipe_config);
> >  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
> >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> > +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
> > +				struct intel_crtc *crtc);
> >
> >  /* intel_frontbuffer.c */
> >  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > @@ -1284,6 +1286,9 @@ bool intel_dp_init_connector(struct
> intel_digital_port
> > *intel_dig_port,
> >  			     struct intel_connector *intel_connector);
> >  void intel_dp_set_link_params(struct intel_dp *intel_dp,
> >  			      const struct intel_crtc_state *pipe_config);
> > +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
> > +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> > +				uint8_t *lane_count, uint8_t *link_bw);
> >  void intel_dp_start_link_train(struct intel_dp *intel_dp);
> >  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCHv4 3/3] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Ander Conselvan De Oliveira]

On Mon, 2016-05-09 at 10:43 +0000, R, Durgadoss wrote:
> Hi Ander,
> 
> Thanks for looking at it.
> Few queries below..
> 
> > 
> > -----Original Message-----
> > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
> > Sent: Friday, May 6, 2016 6:39 PM
> > To: R, Durgadoss <durgadoss.r@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCHv4 3/3] drm/i915/dp: Enable Upfront link
> > training for typeC DP support on BXT
> > 
> > On Mon, 2016-04-18 at 19:01 +0530, Durgadoss R wrote:
> > > 
> > > To support USB type C alternate DP mode, the display driver needs to
> > > know the number of lanes required by the DP panel as well as number
> > > of lanes that can be supported by the type-C cable. Sometimes, the
> > > type-C cable may limit the bandwidth even if Panel can support
> > > more lanes. To address these scenarios, the display driver will
> > > start link training with max lanes, and if that fails, the driver
> > > falls back to x2 lanes; and repeats this procedure for all
> > > bandwidth/lane configurations.
> > > 
> > > * Since link training is done before modeset only the port
> > >   (and not pipe/planes) and its associated PLLs are enabled.
> > > * On DP hotplug: Directly start link training on the crtc
> > >   associated with the DP encoder.
> > > * On Connected boot scenarios: When booted with an LFP and a DP,
> > >   typically, BIOS brings up DP. In these cases, we disable the
> > >   crtc and then do upfront link training; and let the subsequent
> > >   modeset re-enable the crtc.
> > > * All local changes made for upfront link training are reset
> > >   to their previous values once it is done; so that the
> > >   subsequent modeset is not aware of these changes.
> > > 
> > > Changes since v3:
> > > * Fixed a return value on BXT check
> > > * Reworked on top of bxt_ddi_pll_select split from Ander
> > > * Renamed from ddi_upfront to bxt_upfront since the
> > >   upfront logic includes BXT specific functions for now.
> > > Changes since v2:
> > > * Rebased on top of latest dpll_mgr.c code and
> > >   latest HPD related clean ups.
> > > * Corrected return values from upfront (Ander)
> > > * Corrected atomic locking for upfront in intel_dp.c (Ville)
> > > Changes since v1:
> > > *  all pll related functions inside ddi.c
> > > 
> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c |  73 ++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_dp.c  | 180
> > > ++++++++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_drv.h |   5 ++
> > >  3 files changed, 256 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 96234c5..f7fa3db 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2268,6 +2268,79 @@ intel_ddi_init_hdmi_connector(struct
> > intel_digital_port
> > > 
> > > *intel_dig_port)
> > >  	return connector;
> > >  }
> > > 
> > > +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
> > > +				struct intel_crtc *crtc)
> > > +{
> > > +	struct intel_connector *connector = intel_dp->attached_connector;
> > > +	struct intel_encoder *encoder = connector->encoder;
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > +	struct intel_shared_dpll *pll;
> > > +	struct intel_shared_dpll_config tmp_pll_config;
> > > +	struct bxt_clk_div clk_div = {0};
> > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > +	enum intel_dpll_id dpll_id = (enum intel_dpll_id)dig_port->port;
> > > +	uint8_t link_bw, lane_count;
> > > +
> > > +	if (WARN_ON(!crtc))
> > > +		return -EINVAL;
> > > +
> > > +	/* Initialize with Max Link rate & lane count supported by panel
> > > */
> > > +	link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
> > > +	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> > What if the panel supports HBR3? We should use the maximum common rate
> Ok, today that will add one more iteration of Link training before going
> down to a supported rate. 
> 
> > 
> > between source and sink. And it would be nice if those details where in
> > intel_dp.c.
> > Ideally, the platform specific part would take link rate and lane count as
> > arguments and just do the pll/encoder enabling/disabling.
> May be with the new {link_bw/lane_count}_upfront variables in intel_dp
> structure,
> we could do this easily. Will try this implementation and see how it comes up.
> 
> > 
> > 
> > > 
> > > +
> > > +	mutex_lock(&dev_priv->dpll_lock);
> > > +	/*
> > > +	 * FIXME: Works only for BXT.
> > > +	 * Select the required PLL. This works for platforms where
> > > +	 * there is no shared DPLL.
> > > +	 */
> > > +	pll = &dev_priv->shared_dplls[dpll_id];
> > > +	if (WARN_ON(pll->active_mask)) {
> > > +		DRM_ERROR("Shared DPLL already in use.
> > active_mask:%x\n",
> > > 
> > > pll->active_mask);
> > > +		mutex_unlock(&dev_priv->dpll_lock);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	tmp_pll_config = pll->config;
> > > +
> > > +	DRM_DEBUG_KMS("Using pipe %c with pll %s\n",
> > > +			pipe_name(crtc->pipe), pll->name);
> > > +	do {
> > > +		crtc->config->port_clock =
> > > drm_dp_bw_code_to_link_rate(link_bw);
> > > +		crtc->config->lane_count = lane_count;
> > > +
> > > +		bxt_ddi_dp_pll_dividers(crtc->config->port_clock,
> > > &clk_div);
> > > +		if (!bxt_ddi_set_dpll_hw_state(crtc->config->port_clock,
> > > +					&clk_div, &pll->config.hw_state)) 
> > > {
> > > +			DRM_ERROR("Could not setup DPLL\n");
> > > +			goto exit_pll;
> > > +		}
> > > +
> > > +		/* Enable PLL followed by port */
> > > +		pll->funcs.enable(dev_priv, pll);
> > > +		encoder->pre_enable(encoder);
> > > +
> > > +		/* Check if link training passed; if so update DPCD */
> > > +		if (intel_dp->train_set_valid)
> > > +			intel_dp_update_dpcd_params(intel_dp);
> > > +
> > > +		/* Disable port followed by PLL for next retry/clean up
> > > */
> > > +		encoder->post_disable(encoder);
> > > +		pll->funcs.disable(dev_priv, pll);
> > > +
> > > +	} while (!intel_dp->train_set_valid &&
> > > +		intel_dp_get_link_retry_params(intel_dp, &lane_count,
> > > &link_bw));
> > > +
> > > +	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
> > > +	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
> > > link_bw);
> > > +
> > > +exit_pll:
> > > +	pll->config = tmp_pll_config;
> > > +	mutex_unlock(&dev_priv->dpll_lock);
> > > +
> > > +	return intel_dp->train_set_valid ? 0 : -1;
> > > +}
> > > +
> > >  void intel_ddi_init(struct drm_device *dev, enum port port)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > > 
> > > index 61ee226..57d7159 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1595,6 +1595,41 @@ void intel_dp_set_link_params(struct intel_dp
> > > *intel_dp,
> > >  	intel_dp->lane_count = pipe_config->lane_count;
> > >  }
> > > 
> > > +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
> > > +{
> > > +	intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
> > ~DP_MAX_LANE_COUNT_MASK;
> > > 
> > > +	intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
> > > +			intel_dp->lane_count &
> > DP_MAX_LANE_COUNT_MASK;
> > > 
> > > +
> > > +	intel_dp->dpcd[DP_MAX_LINK_RATE] =
> > > +			drm_dp_link_rate_to_bw_code(intel_dp-
> > > link_rate);
> > I think I mentioned this before. I don't think we should update the dpcd
> > fields
> > directly. Instead, there should be a field for the max rate and lane count
> > achieved through upfront link training in struct intel_dp and
> > intel_dp_max_link_bw() and intel_dp_max_lane_count() should take that
> > into
> > account.
> I thought we agreed for having that change as a later implementation.
> Anyway, it's not a big change; So I will do as part of next revision,
> probably as a separate patch.
> 
> > 
> > 
> > > 
> > > +}
> > > +
> > > +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> > > +			uint8_t *lane_count, uint8_t *link_bw)
> > > +{
> > > +	/*
> > > +	 * As per DP1.3 Spec, retry all link rates for a particular
> > > +	 * lane count value, before reducing number of lanes.
> > > +	 */
> > > +	if (*link_bw == DP_LINK_BW_5_4) {
> > > +		*link_bw = DP_LINK_BW_2_7;
> > > +	} else if (*link_bw == DP_LINK_BW_2_7) {
> > > +		*link_bw = DP_LINK_BW_1_62;
> > > +	} else if (*lane_count == 4) {
> > > +		*lane_count = 2;
> > > +		*link_bw = intel_dp_max_link_bw(intel_dp);
> > > +	} else if (*lane_count == 2) {
> > > +		*lane_count = 1;
> > > +		*link_bw = intel_dp_max_link_bw(intel_dp);
> > > +	} else {
> > > +		/* Tried all combinations, so exit */
> > > +		return false;
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  static void intel_dp_prepare(struct intel_encoder *encoder)
> > >  {
> > >  	struct drm_device *dev = encoder->base.dev;
> > > @@ -4548,6 +4583,135 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> > >  	intel_dp->has_audio = false;
> > >  }
> > > 
> > > +static struct intel_crtc_state *intel_dp_upfront_get_crtc_state(
> > > +				struct intel_crtc *crtc,
> > > +				struct drm_modeset_acquire_ctx *ctx)
> > > +{
> > > +	struct drm_device *dev = crtc->base.dev;
> > > +	struct drm_atomic_state *state;
> > > +	struct intel_crtc_state *crtc_state;
> > > +
> > > +	state = drm_atomic_state_alloc(dev);
> > > +	if (!state)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	state->acquire_ctx = ctx;
> > > +
> > > +	crtc_state = intel_atomic_get_crtc_state(state, crtc);
> > > +	if (IS_ERR(crtc_state)) {
> > > +		drm_atomic_state_free(state);
> > > +		state = NULL;
> > > +	}
> > All this function does is call intel_atomic_get_crtc_state(). Since the
> > caller
> > also have to deal with error handling, I don't see what it adds on top of
> > that.
> > The code is actually simpler if merged with the function below.
> This function needs to be called from two places. One in the commit() below,
> and the other in the upfront_link_train() itself, when the crtc is not
> enabled already.
> 
> > 
> > 
> > > 
> > > +
> > > +	return crtc_state;
> > > +}
> > > +
> > > +static int intel_dp_upfront_commit(struct intel_crtc *crtc,
> > > +				struct drm_modeset_acquire_ctx *ctx)
> > This function could use a better name. Something that suggests the crtc is
> > being disabled. Maybe disable_crtc_for_upfront() ? It doens't really depends
> > on
> Sure, better name makes sense..
> 
> > 
> > anything DP specific, so maybe it would make sense to move it to
> > intel_display.c  or intel_atomic.c. Then we could call it
> > intel_crtc_atomic_disable_locked().
> > The "locked" part since this assumes the ctx already have crtc->mutex locked
> > so
> > that there's no risk of -EDEADLK. Although it can just pass the -EDEADLK
> > error to
> > the caller, so just intel_crtc_atomic_disable() ?
> Still this would need to call the above get_crtc_state(..) function.
> So, I will re-name and if it all looks Ok, will move it to atomic.c
> Otherwise, will keep it here..
> 
> > 
> > 
> > 
> > 
> > > 
> > > +{
> > > +	int ret;
> > > +	struct intel_crtc_state *crtc_state;
> > > +	enum pipe pipe = crtc->pipe;
> > > +
> > > +	crtc_state = intel_dp_upfront_get_crtc_state(crtc, ctx);
> > > +	if (IS_ERR(crtc_state))
> > > +		return PTR_ERR(crtc_state);
> > > +
> > > +	DRM_DEBUG_KMS("Disabling crtc %c for upfront link train\n",
> > > pipe_name(pipe));
> > > +
> > > +	crtc_state->base.active = false;
> > > +	ret = drm_atomic_commit(crtc_state->base.state);
> > > +	if (ret) {
> > > +		drm_atomic_state_free(crtc_state->base.state);
> > > +		crtc_state->base.state = NULL;
> > The function drm_atomic_state_free() frees the crtc_state, so this is a use
> > after free.
> Will fix this..
> 
> > 
> > 
> > > 
> > > +	}
> > > +	return ret;
> > > +}
> > > +
> > > +static int intel_dp_upfront_link_train(struct drm_connector *connector)
> > > +{
> > > +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > You could just pass intel_dp directly. The only use of connector is when
> > connector->state->crtc is inspected. But that should have the same value as
> > intel_dp->base.base.crtc
> Ok, will change.
> 
> > 
> > 
> > > 
> > > +	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > +	struct drm_device *dev = intel_encoder->base.dev;
> > > +	struct drm_mode_config *config = &dev->mode_config;
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	struct intel_crtc_state *crtc_state, *tmp_crtc_config;
> > > +	struct intel_crtc *intel_crtc;
> > > +	struct drm_crtc *crtc = NULL;
> > > +	bool crtc_enabled = false;
> > > +	int ret, status = 0;
> > > +
> > > +	if (!IS_BROXTON(dev))
> > > +		return 0;
> > > +
> > > +	drm_modeset_acquire_init(&ctx, 0);
> > > +retry:
> > > +	ret = drm_modeset_lock(&config->connection_mutex, &ctx);
> > > +	if (ret)
> > > +		goto exit_fail;
> > > +
> > > +	if (connector->state->crtc) {
> > > +		crtc = connector->state->crtc;
> > > +
> > > +		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> > > +		if (ret)
> > > +			goto exit_fail;
> > > +
> > > +		crtc_enabled = true;
> > > +	} else {
> > > +		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
> > > +		if (IS_ERR_OR_NULL(crtc)) {
> > > +			ret = PTR_ERR_OR_ZERO(crtc);
> > > +			DRM_DEBUG_KMS("No crtc available for upfront link
> > > train:%d\n", ret);
> > > +			goto exit_fail;
> > > +		}
> > > +		intel_encoder->base.crtc = crtc;
> > > +	}
> > IMO, the following would be a bit more readable.
> > 
> > 	if (intel_encoder->base.crtc)
> > 		crtc = intel_encoder->base.crtc;
> > 	else
> > 		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
> > 
> > 	if (IS_ERR(crtc)) {
> > 		...
> > 	}
> > 
> > 	ret = drm_modeset_lock(&crtc->mutex, &ctx);
> > 	if (ret)
> > 		goto exit_fail;
> > 
> > 	crtc_enabled = crtc->config->active;
> > 
> Ok, makes sense. Will re-order this way.
> 
> > 
> > > 
> > > +
> > > +	intel_crtc = to_intel_crtc(crtc);
> > > +	DRM_DEBUG_KMS("Using pipe %c for upfront link training\n",
> > > +					pipe_name(intel_crtc->pipe));
> > > +
> > > +	ret = drm_modeset_lock(&crtc->primary->mutex, &ctx);
> > > +	if (ret)
> > > +		goto exit_fail;
> > > +
> > > +	if (crtc_enabled) {
> > > +		ret = intel_dp_upfront_commit(intel_crtc, &ctx);
> > > +		if (ret)
> > > +			goto exit_fail;
> > > +	}
> > > +
> > > +	crtc_state = intel_dp_upfront_get_crtc_state(intel_crtc, &ctx);
> > > +	if (IS_ERR(crtc_state)) {
> > > +		ret = PTR_ERR(crtc_state);
> > > +		goto exit_fail;
> > > +	}
> > > +
> > > +	/* Save the existing config */
> > > +	tmp_crtc_config = intel_crtc->config;
> > You need to save this earlier. Otherwise if intel_dp_upfront_commit() is
> > called, the state will be restored with active == false when it should
> > actually be true.
> Ok, will save this earlier. But since we don’t re-enable crtc  here after
> upfront,
> (we let it get re-enabled as part of subsequent modeset) the restoring
> with false does not happen today..
> 
> > 
> > But that would cause intel_crtc->config to be freed, so you need to
> > use intel_crtc_duplicate_state().
> Since we alloc a new state and then only call get_crtc_state(), I thought that
> automatically boils down to duplicate_state() inside
> drm_atomic_get_crtc_state().
> Please correct if my understanding is wrong.

It does, but it is a somewhat messy way of doing it in this case, since you need
to allocate an extra atomic_state.

In any case, after looking at why we need to fiddle with the crtc_state, my
conclusion was that it is easier to remove those dependencies than to do this
save/restore dance. All it would take is to pass link rate and lane count
directly to intel_dp_set_link_params(). The bulk of intel_ddi_pre_enable() can
be split to a function that takes link rate, lane count and dpll as arguments,
and then intel_bxt_upfront_link_train() can call it directly.



> 
> > 
> > 
> > > 
> > > +	intel_crtc->config = crtc_state;
> > > +
> > > +	if (IS_BROXTON(dev))
> > > +		status = intel_bxt_upfront_link_train(intel_dp,
> > > intel_crtc);
> > > +		/* Other platforms upfront link train call goes here..*/
> > > +
> > > +	/* Restore the saved config */
> > > +	intel_crtc->config = tmp_crtc_config;
> > > +	intel_encoder->base.crtc = crtc_enabled ? crtc : NULL;
> > > +	drm_atomic_state_free(crtc_state->base.state);
> > This doesn't actually re-enables the crtc if if was disabled in
> > intel_dp_upfront_commit(). This will require another atomic operation.
> > Maybe that intel_crtc_atomic_disable() should just take a bool specifying if
> > it
> > should on of off.
> We let it get re-enabled as part of mode set that happens subsequently.
> Please let me know your opinion on this.

There's no guarantee there will be a mode set afterwards, since that's a user
space decision. One thing to check is if this works across suspend/resume. IIUC,
it is possible a hotplug event won't be sent to user space if the connector
status didn't change. In that case, I think this might lead to blank screen.

Ander

> 
> Thanks,
> Durga
> 
> > 
> > 
> > 
> > > 
> > > +
> > > +exit_fail:
> > > +	if (ret == -EDEADLK) {
> > > +		drm_modeset_backoff(&ctx);
> > > +		goto retry;
> > > +	}
> > > +	drm_modeset_drop_locks(&ctx);
> > > +	drm_modeset_acquire_fini(&ctx);
> > > +	return status;
> > > +}
> > > +
> > >  static void
> > >  intel_dp_long_pulse(struct intel_connector *intel_connector)
> > >  {
> > > @@ -4558,7 +4722,7 @@ intel_dp_long_pulse(struct intel_connector
> > > *intel_connector)
> > >  	struct drm_device *dev = connector->dev;
> > >  	enum drm_connector_status status;
> > >  	enum intel_display_power_domain power_domain;
> > > -	bool ret;
> > > +	bool ret, do_upfront_link_train;
> > >  	u8 sink_irq_vector;
> > > 
> > >  	power_domain =
> > intel_display_port_aux_power_domain(intel_encoder);
> > > 
> > > @@ -4604,7 +4768,11 @@ intel_dp_long_pulse(struct intel_connector
> > > *intel_connector)
> > >  		drm_modeset_lock(&dev-
> > > mode_config.connection_mutex, NULL);
> > >  		intel_dp_check_link_status(intel_dp);
> > >  		drm_modeset_unlock(&dev-
> > > mode_config.connection_mutex);
> > > -		goto out;
> > > +		/*
> > > +		 * If we are here, we have (re)read DPCD above and hence
> > > +		 * need to do upfront link train again.
> > > +		 */
> > > +		goto upfront;
> > We can avoid this by not overwriting intel_dp->dpcd as I mentioned above.
> > 
> > 
> > Ander
> > 
> > > 
> > >  	}
> > > 
> > >  	/*
> > > @@ -4634,6 +4802,14 @@ intel_dp_long_pulse(struct intel_connector
> > > *intel_connector)
> > >  			DRM_DEBUG_DRIVER("CP or sink specific irq
> > > unhandled\n");
> > >  	}
> > > 
> > > +upfront:
> > > +	/* Do not do upfront link train, if it is a compliance request */
> > > +	do_upfront_link_train =
> > > +		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
> > > +		intel_dp->compliance_test_type !=
> > DP_TEST_LINK_TRAINING;
> > > 
> > > +
> > > +	if (do_upfront_link_train &&
> > intel_dp_upfront_link_train(connector))
> > > 
> > > +		status = connector_status_disconnected;
> > >  out:
> > >  	if (status != connector_status_connected) {
> > >  		intel_dp_unset_edid(intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 27b5dcd..bf98473 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1082,6 +1082,8 @@ void intel_ddi_clock_get(struct intel_encoder
> > *encoder,
> > > 
> > >  			 struct intel_crtc_state *pipe_config);
> > >  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
> > >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> > > +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
> > > +				struct intel_crtc *crtc);
> > > 
> > >  /* intel_frontbuffer.c */
> > >  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > > @@ -1284,6 +1286,9 @@ bool intel_dp_init_connector(struct
> > intel_digital_port
> > > 
> > > *intel_dig_port,
> > >  			     struct intel_connector *intel_connector);
> > >  void intel_dp_set_link_params(struct intel_dp *intel_dp,
> > >  			      const struct intel_crtc_state
> > > *pipe_config);
> > > +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
> > > +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> > > +				uint8_t *lane_count, uint8_t *link_bw);
> > >  void intel_dp_start_link_train(struct intel_dp *intel_dp);
> > >  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> > >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCHv4 3/3] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Rodrigo Vivi]

Also, how does this integrate with fastboot?

If you unconditionally do the upfront training counting on a modeset
we will never be able to enable fastboot.



On Wed, May 11, 2016 at 2:02 AM, Ander Conselvan De Oliveira
<conselvan2@gmail.com> wrote:
> On Mon, 2016-05-09 at 10:43 +0000, R, Durgadoss wrote:
>> Hi Ander,
>>
>> Thanks for looking at it.
>> Few queries below..
>>
>> >
>> > -----Original Message-----
>> > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
>> > Sent: Friday, May 6, 2016 6:39 PM
>> > To: R, Durgadoss <durgadoss.r@intel.com>; intel-gfx@lists.freedesktop.org
>> > Subject: Re: [Intel-gfx] [PATCHv4 3/3] drm/i915/dp: Enable Upfront link
>> > training for typeC DP support on BXT
>> >
>> > On Mon, 2016-04-18 at 19:01 +0530, Durgadoss R wrote:
>> > >
>> > > To support USB type C alternate DP mode, the display driver needs to
>> > > know the number of lanes required by the DP panel as well as number
>> > > of lanes that can be supported by the type-C cable. Sometimes, the
>> > > type-C cable may limit the bandwidth even if Panel can support
>> > > more lanes. To address these scenarios, the display driver will
>> > > start link training with max lanes, and if that fails, the driver
>> > > falls back to x2 lanes; and repeats this procedure for all
>> > > bandwidth/lane configurations.
>> > >
>> > > * Since link training is done before modeset only the port
>> > >   (and not pipe/planes) and its associated PLLs are enabled.
>> > > * On DP hotplug: Directly start link training on the crtc
>> > >   associated with the DP encoder.
>> > > * On Connected boot scenarios: When booted with an LFP and a DP,
>> > >   typically, BIOS brings up DP. In these cases, we disable the
>> > >   crtc and then do upfront link training; and let the subsequent
>> > >   modeset re-enable the crtc.
>> > > * All local changes made for upfront link training are reset
>> > >   to their previous values once it is done; so that the
>> > >   subsequent modeset is not aware of these changes.
>> > >
>> > > Changes since v3:
>> > > * Fixed a return value on BXT check
>> > > * Reworked on top of bxt_ddi_pll_select split from Ander
>> > > * Renamed from ddi_upfront to bxt_upfront since the
>> > >   upfront logic includes BXT specific functions for now.
>> > > Changes since v2:
>> > > * Rebased on top of latest dpll_mgr.c code and
>> > >   latest HPD related clean ups.
>> > > * Corrected return values from upfront (Ander)
>> > > * Corrected atomic locking for upfront in intel_dp.c (Ville)
>> > > Changes since v1:
>> > > *  all pll related functions inside ddi.c
>> > >
>> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/intel_ddi.c |  73 ++++++++++++++++
>> > >  drivers/gpu/drm/i915/intel_dp.c  | 180
>> > > ++++++++++++++++++++++++++++++++++++++-
>> > >  drivers/gpu/drm/i915/intel_drv.h |   5 ++
>> > >  3 files changed, 256 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> > > b/drivers/gpu/drm/i915/intel_ddi.c
>> > > index 96234c5..f7fa3db 100644
>> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > > @@ -2268,6 +2268,79 @@ intel_ddi_init_hdmi_connector(struct
>> > intel_digital_port
>> > >
>> > > *intel_dig_port)
>> > >   return connector;
>> > >  }
>> > >
>> > > +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
>> > > +                         struct intel_crtc *crtc)
>> > > +{
>> > > + struct intel_connector *connector = intel_dp->attached_connector;
>> > > + struct intel_encoder *encoder = connector->encoder;
>> > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> > > + struct intel_shared_dpll *pll;
>> > > + struct intel_shared_dpll_config tmp_pll_config;
>> > > + struct bxt_clk_div clk_div = {0};
>> > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> > > + enum intel_dpll_id dpll_id = (enum intel_dpll_id)dig_port->port;
>> > > + uint8_t link_bw, lane_count;
>> > > +
>> > > + if (WARN_ON(!crtc))
>> > > +         return -EINVAL;
>> > > +
>> > > + /* Initialize with Max Link rate & lane count supported by panel
>> > > */
>> > > + link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
>> > > + lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
>> > What if the panel supports HBR3? We should use the maximum common rate
>> Ok, today that will add one more iteration of Link training before going
>> down to a supported rate.
>>
>> >
>> > between source and sink. And it would be nice if those details where in
>> > intel_dp.c.
>> > Ideally, the platform specific part would take link rate and lane count as
>> > arguments and just do the pll/encoder enabling/disabling.
>> May be with the new {link_bw/lane_count}_upfront variables in intel_dp
>> structure,
>> we could do this easily. Will try this implementation and see how it comes up.
>>
>> >
>> >
>> > >
>> > > +
>> > > + mutex_lock(&dev_priv->dpll_lock);
>> > > + /*
>> > > +  * FIXME: Works only for BXT.
>> > > +  * Select the required PLL. This works for platforms where
>> > > +  * there is no shared DPLL.
>> > > +  */
>> > > + pll = &dev_priv->shared_dplls[dpll_id];
>> > > + if (WARN_ON(pll->active_mask)) {
>> > > +         DRM_ERROR("Shared DPLL already in use.
>> > active_mask:%x\n",
>> > >
>> > > pll->active_mask);
>> > > +         mutex_unlock(&dev_priv->dpll_lock);
>> > > +         return -EINVAL;
>> > > + }
>> > > +
>> > > + tmp_pll_config = pll->config;
>> > > +
>> > > + DRM_DEBUG_KMS("Using pipe %c with pll %s\n",
>> > > +                 pipe_name(crtc->pipe), pll->name);
>> > > + do {
>> > > +         crtc->config->port_clock =
>> > > drm_dp_bw_code_to_link_rate(link_bw);
>> > > +         crtc->config->lane_count = lane_count;
>> > > +
>> > > +         bxt_ddi_dp_pll_dividers(crtc->config->port_clock,
>> > > &clk_div);
>> > > +         if (!bxt_ddi_set_dpll_hw_state(crtc->config->port_clock,
>> > > +                                 &clk_div, &pll->config.hw_state))
>> > > {
>> > > +                 DRM_ERROR("Could not setup DPLL\n");
>> > > +                 goto exit_pll;
>> > > +         }
>> > > +
>> > > +         /* Enable PLL followed by port */
>> > > +         pll->funcs.enable(dev_priv, pll);
>> > > +         encoder->pre_enable(encoder);
>> > > +
>> > > +         /* Check if link training passed; if so update DPCD */
>> > > +         if (intel_dp->train_set_valid)
>> > > +                 intel_dp_update_dpcd_params(intel_dp);
>> > > +
>> > > +         /* Disable port followed by PLL for next retry/clean up
>> > > */
>> > > +         encoder->post_disable(encoder);
>> > > +         pll->funcs.disable(dev_priv, pll);
>> > > +
>> > > + } while (!intel_dp->train_set_valid &&
>> > > +         intel_dp_get_link_retry_params(intel_dp, &lane_count,
>> > > &link_bw));
>> > > +
>> > > + DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
>> > > + intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
>> > > link_bw);
>> > > +
>> > > +exit_pll:
>> > > + pll->config = tmp_pll_config;
>> > > + mutex_unlock(&dev_priv->dpll_lock);
>> > > +
>> > > + return intel_dp->train_set_valid ? 0 : -1;
>> > > +}
>> > > +
>> > >  void intel_ddi_init(struct drm_device *dev, enum port port)
>> > >  {
>> > >   struct drm_i915_private *dev_priv = dev->dev_private;
>> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > b/drivers/gpu/drm/i915/intel_dp.c
>> > >
>> > > index 61ee226..57d7159 100644
>> > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > @@ -1595,6 +1595,41 @@ void intel_dp_set_link_params(struct intel_dp
>> > > *intel_dp,
>> > >   intel_dp->lane_count = pipe_config->lane_count;
>> > >  }
>> > >
>> > > +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
>> > > +{
>> > > + intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
>> > ~DP_MAX_LANE_COUNT_MASK;
>> > >
>> > > + intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
>> > > +                 intel_dp->lane_count &
>> > DP_MAX_LANE_COUNT_MASK;
>> > >
>> > > +
>> > > + intel_dp->dpcd[DP_MAX_LINK_RATE] =
>> > > +                 drm_dp_link_rate_to_bw_code(intel_dp-
>> > > link_rate);
>> > I think I mentioned this before. I don't think we should update the dpcd
>> > fields
>> > directly. Instead, there should be a field for the max rate and lane count
>> > achieved through upfront link training in struct intel_dp and
>> > intel_dp_max_link_bw() and intel_dp_max_lane_count() should take that
>> > into
>> > account.
>> I thought we agreed for having that change as a later implementation.
>> Anyway, it's not a big change; So I will do as part of next revision,
>> probably as a separate patch.
>>
>> >
>> >
>> > >
>> > > +}
>> > > +
>> > > +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
>> > > +                 uint8_t *lane_count, uint8_t *link_bw)
>> > > +{
>> > > + /*
>> > > +  * As per DP1.3 Spec, retry all link rates for a particular
>> > > +  * lane count value, before reducing number of lanes.
>> > > +  */
>> > > + if (*link_bw == DP_LINK_BW_5_4) {
>> > > +         *link_bw = DP_LINK_BW_2_7;
>> > > + } else if (*link_bw == DP_LINK_BW_2_7) {
>> > > +         *link_bw = DP_LINK_BW_1_62;
>> > > + } else if (*lane_count == 4) {
>> > > +         *lane_count = 2;
>> > > +         *link_bw = intel_dp_max_link_bw(intel_dp);
>> > > + } else if (*lane_count == 2) {
>> > > +         *lane_count = 1;
>> > > +         *link_bw = intel_dp_max_link_bw(intel_dp);
>> > > + } else {
>> > > +         /* Tried all combinations, so exit */
>> > > +         return false;
>> > > + }
>> > > +
>> > > + return true;
>> > > +}
>> > > +
>> > >  static void intel_dp_prepare(struct intel_encoder *encoder)
>> > >  {
>> > >   struct drm_device *dev = encoder->base.dev;
>> > > @@ -4548,6 +4583,135 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>> > >   intel_dp->has_audio = false;
>> > >  }
>> > >
>> > > +static struct intel_crtc_state *intel_dp_upfront_get_crtc_state(
>> > > +                         struct intel_crtc *crtc,
>> > > +                         struct drm_modeset_acquire_ctx *ctx)
>> > > +{
>> > > + struct drm_device *dev = crtc->base.dev;
>> > > + struct drm_atomic_state *state;
>> > > + struct intel_crtc_state *crtc_state;
>> > > +
>> > > + state = drm_atomic_state_alloc(dev);
>> > > + if (!state)
>> > > +         return ERR_PTR(-ENOMEM);
>> > > +
>> > > + state->acquire_ctx = ctx;
>> > > +
>> > > + crtc_state = intel_atomic_get_crtc_state(state, crtc);
>> > > + if (IS_ERR(crtc_state)) {
>> > > +         drm_atomic_state_free(state);
>> > > +         state = NULL;
>> > > + }
>> > All this function does is call intel_atomic_get_crtc_state(). Since the
>> > caller
>> > also have to deal with error handling, I don't see what it adds on top of
>> > that.
>> > The code is actually simpler if merged with the function below.
>> This function needs to be called from two places. One in the commit() below,
>> and the other in the upfront_link_train() itself, when the crtc is not
>> enabled already.
>>
>> >
>> >
>> > >
>> > > +
>> > > + return crtc_state;
>> > > +}
>> > > +
>> > > +static int intel_dp_upfront_commit(struct intel_crtc *crtc,
>> > > +                         struct drm_modeset_acquire_ctx *ctx)
>> > This function could use a better name. Something that suggests the crtc is
>> > being disabled. Maybe disable_crtc_for_upfront() ? It doens't really depends
>> > on
>> Sure, better name makes sense..
>>
>> >
>> > anything DP specific, so maybe it would make sense to move it to
>> > intel_display.c  or intel_atomic.c. Then we could call it
>> > intel_crtc_atomic_disable_locked().
>> > The "locked" part since this assumes the ctx already have crtc->mutex locked
>> > so
>> > that there's no risk of -EDEADLK. Although it can just pass the -EDEADLK
>> > error to
>> > the caller, so just intel_crtc_atomic_disable() ?
>> Still this would need to call the above get_crtc_state(..) function.
>> So, I will re-name and if it all looks Ok, will move it to atomic.c
>> Otherwise, will keep it here..
>>
>> >
>> >
>> >
>> >
>> > >
>> > > +{
>> > > + int ret;
>> > > + struct intel_crtc_state *crtc_state;
>> > > + enum pipe pipe = crtc->pipe;
>> > > +
>> > > + crtc_state = intel_dp_upfront_get_crtc_state(crtc, ctx);
>> > > + if (IS_ERR(crtc_state))
>> > > +         return PTR_ERR(crtc_state);
>> > > +
>> > > + DRM_DEBUG_KMS("Disabling crtc %c for upfront link train\n",
>> > > pipe_name(pipe));
>> > > +
>> > > + crtc_state->base.active = false;
>> > > + ret = drm_atomic_commit(crtc_state->base.state);
>> > > + if (ret) {
>> > > +         drm_atomic_state_free(crtc_state->base.state);
>> > > +         crtc_state->base.state = NULL;
>> > The function drm_atomic_state_free() frees the crtc_state, so this is a use
>> > after free.
>> Will fix this..
>>
>> >
>> >
>> > >
>> > > + }
>> > > + return ret;
>> > > +}
>> > > +
>> > > +static int intel_dp_upfront_link_train(struct drm_connector *connector)
>> > > +{
>> > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
>> > You could just pass intel_dp directly. The only use of connector is when
>> > connector->state->crtc is inspected. But that should have the same value as
>> > intel_dp->base.base.crtc
>> Ok, will change.
>>
>> >
>> >
>> > >
>> > > + struct intel_digital_port *intel_dig_port =
>> > > dp_to_dig_port(intel_dp);
>> > > + struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> > > + struct drm_device *dev = intel_encoder->base.dev;
>> > > + struct drm_mode_config *config = &dev->mode_config;
>> > > + struct drm_modeset_acquire_ctx ctx;
>> > > + struct intel_crtc_state *crtc_state, *tmp_crtc_config;
>> > > + struct intel_crtc *intel_crtc;
>> > > + struct drm_crtc *crtc = NULL;
>> > > + bool crtc_enabled = false;
>> > > + int ret, status = 0;
>> > > +
>> > > + if (!IS_BROXTON(dev))
>> > > +         return 0;
>> > > +
>> > > + drm_modeset_acquire_init(&ctx, 0);
>> > > +retry:
>> > > + ret = drm_modeset_lock(&config->connection_mutex, &ctx);
>> > > + if (ret)
>> > > +         goto exit_fail;
>> > > +
>> > > + if (connector->state->crtc) {
>> > > +         crtc = connector->state->crtc;
>> > > +
>> > > +         ret = drm_modeset_lock(&crtc->mutex, &ctx);
>> > > +         if (ret)
>> > > +                 goto exit_fail;
>> > > +
>> > > +         crtc_enabled = true;
>> > > + } else {
>> > > +         crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
>> > > +         if (IS_ERR_OR_NULL(crtc)) {
>> > > +                 ret = PTR_ERR_OR_ZERO(crtc);
>> > > +                 DRM_DEBUG_KMS("No crtc available for upfront link
>> > > train:%d\n", ret);
>> > > +                 goto exit_fail;
>> > > +         }
>> > > +         intel_encoder->base.crtc = crtc;
>> > > + }
>> > IMO, the following would be a bit more readable.
>> >
>> >     if (intel_encoder->base.crtc)
>> >             crtc = intel_encoder->base.crtc;
>> >     else
>> >             crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
>> >
>> >     if (IS_ERR(crtc)) {
>> >             ...
>> >     }
>> >
>> >     ret = drm_modeset_lock(&crtc->mutex, &ctx);
>> >     if (ret)
>> >             goto exit_fail;
>> >
>> >     crtc_enabled = crtc->config->active;
>> >
>> Ok, makes sense. Will re-order this way.
>>
>> >
>> > >
>> > > +
>> > > + intel_crtc = to_intel_crtc(crtc);
>> > > + DRM_DEBUG_KMS("Using pipe %c for upfront link training\n",
>> > > +                                 pipe_name(intel_crtc->pipe));
>> > > +
>> > > + ret = drm_modeset_lock(&crtc->primary->mutex, &ctx);
>> > > + if (ret)
>> > > +         goto exit_fail;
>> > > +
>> > > + if (crtc_enabled) {
>> > > +         ret = intel_dp_upfront_commit(intel_crtc, &ctx);
>> > > +         if (ret)
>> > > +                 goto exit_fail;
>> > > + }
>> > > +
>> > > + crtc_state = intel_dp_upfront_get_crtc_state(intel_crtc, &ctx);
>> > > + if (IS_ERR(crtc_state)) {
>> > > +         ret = PTR_ERR(crtc_state);
>> > > +         goto exit_fail;
>> > > + }
>> > > +
>> > > + /* Save the existing config */
>> > > + tmp_crtc_config = intel_crtc->config;
>> > You need to save this earlier. Otherwise if intel_dp_upfront_commit() is
>> > called, the state will be restored with active == false when it should
>> > actually be true.
>> Ok, will save this earlier. But since we don’t re-enable crtc  here after
>> upfront,
>> (we let it get re-enabled as part of subsequent modeset) the restoring
>> with false does not happen today..
>>
>> >
>> > But that would cause intel_crtc->config to be freed, so you need to
>> > use intel_crtc_duplicate_state().
>> Since we alloc a new state and then only call get_crtc_state(), I thought that
>> automatically boils down to duplicate_state() inside
>> drm_atomic_get_crtc_state().
>> Please correct if my understanding is wrong.
>
> It does, but it is a somewhat messy way of doing it in this case, since you need
> to allocate an extra atomic_state.
>
> In any case, after looking at why we need to fiddle with the crtc_state, my
> conclusion was that it is easier to remove those dependencies than to do this
> save/restore dance. All it would take is to pass link rate and lane count
> directly to intel_dp_set_link_params(). The bulk of intel_ddi_pre_enable() can
> be split to a function that takes link rate, lane count and dpll as arguments,
> and then intel_bxt_upfront_link_train() can call it directly.
>
>
>
>>
>> >
>> >
>> > >
>> > > + intel_crtc->config = crtc_state;
>> > > +
>> > > + if (IS_BROXTON(dev))
>> > > +         status = intel_bxt_upfront_link_train(intel_dp,
>> > > intel_crtc);
>> > > +         /* Other platforms upfront link train call goes here..*/
>> > > +
>> > > + /* Restore the saved config */
>> > > + intel_crtc->config = tmp_crtc_config;
>> > > + intel_encoder->base.crtc = crtc_enabled ? crtc : NULL;
>> > > + drm_atomic_state_free(crtc_state->base.state);
>> > This doesn't actually re-enables the crtc if if was disabled in
>> > intel_dp_upfront_commit(). This will require another atomic operation.
>> > Maybe that intel_crtc_atomic_disable() should just take a bool specifying if
>> > it
>> > should on of off.
>> We let it get re-enabled as part of mode set that happens subsequently.
>> Please let me know your opinion on this.
>
> There's no guarantee there will be a mode set afterwards, since that's a user
> space decision. One thing to check is if this works across suspend/resume. IIUC,
> it is possible a hotplug event won't be sent to user space if the connector
> status didn't change. In that case, I think this might lead to blank screen.
>
> Ander
>
>>
>> Thanks,
>> Durga
>>
>> >
>> >
>> >
>> > >
>> > > +
>> > > +exit_fail:
>> > > + if (ret == -EDEADLK) {
>> > > +         drm_modeset_backoff(&ctx);
>> > > +         goto retry;
>> > > + }
>> > > + drm_modeset_drop_locks(&ctx);
>> > > + drm_modeset_acquire_fini(&ctx);
>> > > + return status;
>> > > +}
>> > > +
>> > >  static void
>> > >  intel_dp_long_pulse(struct intel_connector *intel_connector)
>> > >  {
>> > > @@ -4558,7 +4722,7 @@ intel_dp_long_pulse(struct intel_connector
>> > > *intel_connector)
>> > >   struct drm_device *dev = connector->dev;
>> > >   enum drm_connector_status status;
>> > >   enum intel_display_power_domain power_domain;
>> > > - bool ret;
>> > > + bool ret, do_upfront_link_train;
>> > >   u8 sink_irq_vector;
>> > >
>> > >   power_domain =
>> > intel_display_port_aux_power_domain(intel_encoder);
>> > >
>> > > @@ -4604,7 +4768,11 @@ intel_dp_long_pulse(struct intel_connector
>> > > *intel_connector)
>> > >           drm_modeset_lock(&dev-
>> > > mode_config.connection_mutex, NULL);
>> > >           intel_dp_check_link_status(intel_dp);
>> > >           drm_modeset_unlock(&dev-
>> > > mode_config.connection_mutex);
>> > > -         goto out;
>> > > +         /*
>> > > +          * If we are here, we have (re)read DPCD above and hence
>> > > +          * need to do upfront link train again.
>> > > +          */
>> > > +         goto upfront;
>> > We can avoid this by not overwriting intel_dp->dpcd as I mentioned above.
>> >
>> >
>> > Ander
>> >
>> > >
>> > >   }
>> > >
>> > >   /*
>> > > @@ -4634,6 +4802,14 @@ intel_dp_long_pulse(struct intel_connector
>> > > *intel_connector)
>> > >                   DRM_DEBUG_DRIVER("CP or sink specific irq
>> > > unhandled\n");
>> > >   }
>> > >
>> > > +upfront:
>> > > + /* Do not do upfront link train, if it is a compliance request */
>> > > + do_upfront_link_train =
>> > > +         intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
>> > > +         intel_dp->compliance_test_type !=
>> > DP_TEST_LINK_TRAINING;
>> > >
>> > > +
>> > > + if (do_upfront_link_train &&
>> > intel_dp_upfront_link_train(connector))
>> > >
>> > > +         status = connector_status_disconnected;
>> > >  out:
>> > >   if (status != connector_status_connected) {
>> > >           intel_dp_unset_edid(intel_dp);
>> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> > > b/drivers/gpu/drm/i915/intel_drv.h
>> > > index 27b5dcd..bf98473 100644
>> > > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > > @@ -1082,6 +1082,8 @@ void intel_ddi_clock_get(struct intel_encoder
>> > *encoder,
>> > >
>> > >                    struct intel_crtc_state *pipe_config);
>> > >  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
>> > >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
>> > > +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
>> > > +                         struct intel_crtc *crtc);
>> > >
>> > >  /* intel_frontbuffer.c */
>> > >  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>> > > @@ -1284,6 +1286,9 @@ bool intel_dp_init_connector(struct
>> > intel_digital_port
>> > >
>> > > *intel_dig_port,
>> > >                        struct intel_connector *intel_connector);
>> > >  void intel_dp_set_link_params(struct intel_dp *intel_dp,
>> > >                         const struct intel_crtc_state
>> > > *pipe_config);
>> > > +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
>> > > +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
>> > > +                         uint8_t *lane_count, uint8_t *link_bw);
>> > >  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>> > >  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>> > >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCHv4 3/3] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Daniel Vetter]

On Tue, Jul 26, 2016 at 01:51:20PM -0700, Rodrigo Vivi wrote:
> Also, how does this integrate with fastboot?
> 
> If you unconditionally do the upfront training counting on a modeset
> we will never be able to enable fastboot.

We need to do upfront link training on hotplug, anything else won't really
work. And we need to make sure we (re)probe all that on driver load and on
resume.

I didn't look at the code to check whether that's the case already ;-)
-Daniel

> 
> 
> 
> On Wed, May 11, 2016 at 2:02 AM, Ander Conselvan De Oliveira
> <conselvan2@gmail.com> wrote:
> > On Mon, 2016-05-09 at 10:43 +0000, R, Durgadoss wrote:
> >> Hi Ander,
> >>
> >> Thanks for looking at it.
> >> Few queries below..
> >>
> >> >
> >> > -----Original Message-----
> >> > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
> >> > Sent: Friday, May 6, 2016 6:39 PM
> >> > To: R, Durgadoss <durgadoss.r@intel.com>; intel-gfx@lists.freedesktop.org
> >> > Subject: Re: [Intel-gfx] [PATCHv4 3/3] drm/i915/dp: Enable Upfront link
> >> > training for typeC DP support on BXT
> >> >
> >> > On Mon, 2016-04-18 at 19:01 +0530, Durgadoss R wrote:
> >> > >
> >> > > To support USB type C alternate DP mode, the display driver needs to
> >> > > know the number of lanes required by the DP panel as well as number
> >> > > of lanes that can be supported by the type-C cable. Sometimes, the
> >> > > type-C cable may limit the bandwidth even if Panel can support
> >> > > more lanes. To address these scenarios, the display driver will
> >> > > start link training with max lanes, and if that fails, the driver
> >> > > falls back to x2 lanes; and repeats this procedure for all
> >> > > bandwidth/lane configurations.
> >> > >
> >> > > * Since link training is done before modeset only the port
> >> > >   (and not pipe/planes) and its associated PLLs are enabled.
> >> > > * On DP hotplug: Directly start link training on the crtc
> >> > >   associated with the DP encoder.
> >> > > * On Connected boot scenarios: When booted with an LFP and a DP,
> >> > >   typically, BIOS brings up DP. In these cases, we disable the
> >> > >   crtc and then do upfront link training; and let the subsequent
> >> > >   modeset re-enable the crtc.
> >> > > * All local changes made for upfront link training are reset
> >> > >   to their previous values once it is done; so that the
> >> > >   subsequent modeset is not aware of these changes.
> >> > >
> >> > > Changes since v3:
> >> > > * Fixed a return value on BXT check
> >> > > * Reworked on top of bxt_ddi_pll_select split from Ander
> >> > > * Renamed from ddi_upfront to bxt_upfront since the
> >> > >   upfront logic includes BXT specific functions for now.
> >> > > Changes since v2:
> >> > > * Rebased on top of latest dpll_mgr.c code and
> >> > >   latest HPD related clean ups.
> >> > > * Corrected return values from upfront (Ander)
> >> > > * Corrected atomic locking for upfront in intel_dp.c (Ville)
> >> > > Changes since v1:
> >> > > *  all pll related functions inside ddi.c
> >> > >
> >> > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >> > > ---
> >> > >  drivers/gpu/drm/i915/intel_ddi.c |  73 ++++++++++++++++
> >> > >  drivers/gpu/drm/i915/intel_dp.c  | 180
> >> > > ++++++++++++++++++++++++++++++++++++++-
> >> > >  drivers/gpu/drm/i915/intel_drv.h |   5 ++
> >> > >  3 files changed, 256 insertions(+), 2 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> >> > > b/drivers/gpu/drm/i915/intel_ddi.c
> >> > > index 96234c5..f7fa3db 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> > > @@ -2268,6 +2268,79 @@ intel_ddi_init_hdmi_connector(struct
> >> > intel_digital_port
> >> > >
> >> > > *intel_dig_port)
> >> > >   return connector;
> >> > >  }
> >> > >
> >> > > +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
> >> > > +                         struct intel_crtc *crtc)
> >> > > +{
> >> > > + struct intel_connector *connector = intel_dp->attached_connector;
> >> > > + struct intel_encoder *encoder = connector->encoder;
> >> > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >> > > + struct intel_shared_dpll *pll;
> >> > > + struct intel_shared_dpll_config tmp_pll_config;
> >> > > + struct bxt_clk_div clk_div = {0};
> >> > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >> > > + enum intel_dpll_id dpll_id = (enum intel_dpll_id)dig_port->port;
> >> > > + uint8_t link_bw, lane_count;
> >> > > +
> >> > > + if (WARN_ON(!crtc))
> >> > > +         return -EINVAL;
> >> > > +
> >> > > + /* Initialize with Max Link rate & lane count supported by panel
> >> > > */
> >> > > + link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
> >> > > + lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> >> > What if the panel supports HBR3? We should use the maximum common rate
> >> Ok, today that will add one more iteration of Link training before going
> >> down to a supported rate.
> >>
> >> >
> >> > between source and sink. And it would be nice if those details where in
> >> > intel_dp.c.
> >> > Ideally, the platform specific part would take link rate and lane count as
> >> > arguments and just do the pll/encoder enabling/disabling.
> >> May be with the new {link_bw/lane_count}_upfront variables in intel_dp
> >> structure,
> >> we could do this easily. Will try this implementation and see how it comes up.
> >>
> >> >
> >> >
> >> > >
> >> > > +
> >> > > + mutex_lock(&dev_priv->dpll_lock);
> >> > > + /*
> >> > > +  * FIXME: Works only for BXT.
> >> > > +  * Select the required PLL. This works for platforms where
> >> > > +  * there is no shared DPLL.
> >> > > +  */
> >> > > + pll = &dev_priv->shared_dplls[dpll_id];
> >> > > + if (WARN_ON(pll->active_mask)) {
> >> > > +         DRM_ERROR("Shared DPLL already in use.
> >> > active_mask:%x\n",
> >> > >
> >> > > pll->active_mask);
> >> > > +         mutex_unlock(&dev_priv->dpll_lock);
> >> > > +         return -EINVAL;
> >> > > + }
> >> > > +
> >> > > + tmp_pll_config = pll->config;
> >> > > +
> >> > > + DRM_DEBUG_KMS("Using pipe %c with pll %s\n",
> >> > > +                 pipe_name(crtc->pipe), pll->name);
> >> > > + do {
> >> > > +         crtc->config->port_clock =
> >> > > drm_dp_bw_code_to_link_rate(link_bw);
> >> > > +         crtc->config->lane_count = lane_count;
> >> > > +
> >> > > +         bxt_ddi_dp_pll_dividers(crtc->config->port_clock,
> >> > > &clk_div);
> >> > > +         if (!bxt_ddi_set_dpll_hw_state(crtc->config->port_clock,
> >> > > +                                 &clk_div, &pll->config.hw_state))
> >> > > {
> >> > > +                 DRM_ERROR("Could not setup DPLL\n");
> >> > > +                 goto exit_pll;
> >> > > +         }
> >> > > +
> >> > > +         /* Enable PLL followed by port */
> >> > > +         pll->funcs.enable(dev_priv, pll);
> >> > > +         encoder->pre_enable(encoder);
> >> > > +
> >> > > +         /* Check if link training passed; if so update DPCD */
> >> > > +         if (intel_dp->train_set_valid)
> >> > > +                 intel_dp_update_dpcd_params(intel_dp);
> >> > > +
> >> > > +         /* Disable port followed by PLL for next retry/clean up
> >> > > */
> >> > > +         encoder->post_disable(encoder);
> >> > > +         pll->funcs.disable(dev_priv, pll);
> >> > > +
> >> > > + } while (!intel_dp->train_set_valid &&
> >> > > +         intel_dp_get_link_retry_params(intel_dp, &lane_count,
> >> > > &link_bw));
> >> > > +
> >> > > + DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
> >> > > + intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
> >> > > link_bw);
> >> > > +
> >> > > +exit_pll:
> >> > > + pll->config = tmp_pll_config;
> >> > > + mutex_unlock(&dev_priv->dpll_lock);
> >> > > +
> >> > > + return intel_dp->train_set_valid ? 0 : -1;
> >> > > +}
> >> > > +
> >> > >  void intel_ddi_init(struct drm_device *dev, enum port port)
> >> > >  {
> >> > >   struct drm_i915_private *dev_priv = dev->dev_private;
> >> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >> > b/drivers/gpu/drm/i915/intel_dp.c
> >> > >
> >> > > index 61ee226..57d7159 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > > @@ -1595,6 +1595,41 @@ void intel_dp_set_link_params(struct intel_dp
> >> > > *intel_dp,
> >> > >   intel_dp->lane_count = pipe_config->lane_count;
> >> > >  }
> >> > >
> >> > > +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
> >> > > +{
> >> > > + intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
> >> > ~DP_MAX_LANE_COUNT_MASK;
> >> > >
> >> > > + intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
> >> > > +                 intel_dp->lane_count &
> >> > DP_MAX_LANE_COUNT_MASK;
> >> > >
> >> > > +
> >> > > + intel_dp->dpcd[DP_MAX_LINK_RATE] =
> >> > > +                 drm_dp_link_rate_to_bw_code(intel_dp-
> >> > > link_rate);
> >> > I think I mentioned this before. I don't think we should update the dpcd
> >> > fields
> >> > directly. Instead, there should be a field for the max rate and lane count
> >> > achieved through upfront link training in struct intel_dp and
> >> > intel_dp_max_link_bw() and intel_dp_max_lane_count() should take that
> >> > into
> >> > account.
> >> I thought we agreed for having that change as a later implementation.
> >> Anyway, it's not a big change; So I will do as part of next revision,
> >> probably as a separate patch.
> >>
> >> >
> >> >
> >> > >
> >> > > +}
> >> > > +
> >> > > +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> >> > > +                 uint8_t *lane_count, uint8_t *link_bw)
> >> > > +{
> >> > > + /*
> >> > > +  * As per DP1.3 Spec, retry all link rates for a particular
> >> > > +  * lane count value, before reducing number of lanes.
> >> > > +  */
> >> > > + if (*link_bw == DP_LINK_BW_5_4) {
> >> > > +         *link_bw = DP_LINK_BW_2_7;
> >> > > + } else if (*link_bw == DP_LINK_BW_2_7) {
> >> > > +         *link_bw = DP_LINK_BW_1_62;
> >> > > + } else if (*lane_count == 4) {
> >> > > +         *lane_count = 2;
> >> > > +         *link_bw = intel_dp_max_link_bw(intel_dp);
> >> > > + } else if (*lane_count == 2) {
> >> > > +         *lane_count = 1;
> >> > > +         *link_bw = intel_dp_max_link_bw(intel_dp);
> >> > > + } else {
> >> > > +         /* Tried all combinations, so exit */
> >> > > +         return false;
> >> > > + }
> >> > > +
> >> > > + return true;
> >> > > +}
> >> > > +
> >> > >  static void intel_dp_prepare(struct intel_encoder *encoder)
> >> > >  {
> >> > >   struct drm_device *dev = encoder->base.dev;
> >> > > @@ -4548,6 +4583,135 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> >> > >   intel_dp->has_audio = false;
> >> > >  }
> >> > >
> >> > > +static struct intel_crtc_state *intel_dp_upfront_get_crtc_state(
> >> > > +                         struct intel_crtc *crtc,
> >> > > +                         struct drm_modeset_acquire_ctx *ctx)
> >> > > +{
> >> > > + struct drm_device *dev = crtc->base.dev;
> >> > > + struct drm_atomic_state *state;
> >> > > + struct intel_crtc_state *crtc_state;
> >> > > +
> >> > > + state = drm_atomic_state_alloc(dev);
> >> > > + if (!state)
> >> > > +         return ERR_PTR(-ENOMEM);
> >> > > +
> >> > > + state->acquire_ctx = ctx;
> >> > > +
> >> > > + crtc_state = intel_atomic_get_crtc_state(state, crtc);
> >> > > + if (IS_ERR(crtc_state)) {
> >> > > +         drm_atomic_state_free(state);
> >> > > +         state = NULL;
> >> > > + }
> >> > All this function does is call intel_atomic_get_crtc_state(). Since the
> >> > caller
> >> > also have to deal with error handling, I don't see what it adds on top of
> >> > that.
> >> > The code is actually simpler if merged with the function below.
> >> This function needs to be called from two places. One in the commit() below,
> >> and the other in the upfront_link_train() itself, when the crtc is not
> >> enabled already.
> >>
> >> >
> >> >
> >> > >
> >> > > +
> >> > > + return crtc_state;
> >> > > +}
> >> > > +
> >> > > +static int intel_dp_upfront_commit(struct intel_crtc *crtc,
> >> > > +                         struct drm_modeset_acquire_ctx *ctx)
> >> > This function could use a better name. Something that suggests the crtc is
> >> > being disabled. Maybe disable_crtc_for_upfront() ? It doens't really depends
> >> > on
> >> Sure, better name makes sense..
> >>
> >> >
> >> > anything DP specific, so maybe it would make sense to move it to
> >> > intel_display.c  or intel_atomic.c. Then we could call it
> >> > intel_crtc_atomic_disable_locked().
> >> > The "locked" part since this assumes the ctx already have crtc->mutex locked
> >> > so
> >> > that there's no risk of -EDEADLK. Although it can just pass the -EDEADLK
> >> > error to
> >> > the caller, so just intel_crtc_atomic_disable() ?
> >> Still this would need to call the above get_crtc_state(..) function.
> >> So, I will re-name and if it all looks Ok, will move it to atomic.c
> >> Otherwise, will keep it here..
> >>
> >> >
> >> >
> >> >
> >> >
> >> > >
> >> > > +{
> >> > > + int ret;
> >> > > + struct intel_crtc_state *crtc_state;
> >> > > + enum pipe pipe = crtc->pipe;
> >> > > +
> >> > > + crtc_state = intel_dp_upfront_get_crtc_state(crtc, ctx);
> >> > > + if (IS_ERR(crtc_state))
> >> > > +         return PTR_ERR(crtc_state);
> >> > > +
> >> > > + DRM_DEBUG_KMS("Disabling crtc %c for upfront link train\n",
> >> > > pipe_name(pipe));
> >> > > +
> >> > > + crtc_state->base.active = false;
> >> > > + ret = drm_atomic_commit(crtc_state->base.state);
> >> > > + if (ret) {
> >> > > +         drm_atomic_state_free(crtc_state->base.state);
> >> > > +         crtc_state->base.state = NULL;
> >> > The function drm_atomic_state_free() frees the crtc_state, so this is a use
> >> > after free.
> >> Will fix this..
> >>
> >> >
> >> >
> >> > >
> >> > > + }
> >> > > + return ret;
> >> > > +}
> >> > > +
> >> > > +static int intel_dp_upfront_link_train(struct drm_connector *connector)
> >> > > +{
> >> > > + struct intel_dp *intel_dp = intel_attached_dp(connector);
> >> > You could just pass intel_dp directly. The only use of connector is when
> >> > connector->state->crtc is inspected. But that should have the same value as
> >> > intel_dp->base.base.crtc
> >> Ok, will change.
> >>
> >> >
> >> >
> >> > >
> >> > > + struct intel_digital_port *intel_dig_port =
> >> > > dp_to_dig_port(intel_dp);
> >> > > + struct intel_encoder *intel_encoder = &intel_dig_port->base;
> >> > > + struct drm_device *dev = intel_encoder->base.dev;
> >> > > + struct drm_mode_config *config = &dev->mode_config;
> >> > > + struct drm_modeset_acquire_ctx ctx;
> >> > > + struct intel_crtc_state *crtc_state, *tmp_crtc_config;
> >> > > + struct intel_crtc *intel_crtc;
> >> > > + struct drm_crtc *crtc = NULL;
> >> > > + bool crtc_enabled = false;
> >> > > + int ret, status = 0;
> >> > > +
> >> > > + if (!IS_BROXTON(dev))
> >> > > +         return 0;
> >> > > +
> >> > > + drm_modeset_acquire_init(&ctx, 0);
> >> > > +retry:
> >> > > + ret = drm_modeset_lock(&config->connection_mutex, &ctx);
> >> > > + if (ret)
> >> > > +         goto exit_fail;
> >> > > +
> >> > > + if (connector->state->crtc) {
> >> > > +         crtc = connector->state->crtc;
> >> > > +
> >> > > +         ret = drm_modeset_lock(&crtc->mutex, &ctx);
> >> > > +         if (ret)
> >> > > +                 goto exit_fail;
> >> > > +
> >> > > +         crtc_enabled = true;
> >> > > + } else {
> >> > > +         crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
> >> > > +         if (IS_ERR_OR_NULL(crtc)) {
> >> > > +                 ret = PTR_ERR_OR_ZERO(crtc);
> >> > > +                 DRM_DEBUG_KMS("No crtc available for upfront link
> >> > > train:%d\n", ret);
> >> > > +                 goto exit_fail;
> >> > > +         }
> >> > > +         intel_encoder->base.crtc = crtc;
> >> > > + }
> >> > IMO, the following would be a bit more readable.
> >> >
> >> >     if (intel_encoder->base.crtc)
> >> >             crtc = intel_encoder->base.crtc;
> >> >     else
> >> >             crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
> >> >
> >> >     if (IS_ERR(crtc)) {
> >> >             ...
> >> >     }
> >> >
> >> >     ret = drm_modeset_lock(&crtc->mutex, &ctx);
> >> >     if (ret)
> >> >             goto exit_fail;
> >> >
> >> >     crtc_enabled = crtc->config->active;
> >> >
> >> Ok, makes sense. Will re-order this way.
> >>
> >> >
> >> > >
> >> > > +
> >> > > + intel_crtc = to_intel_crtc(crtc);
> >> > > + DRM_DEBUG_KMS("Using pipe %c for upfront link training\n",
> >> > > +                                 pipe_name(intel_crtc->pipe));
> >> > > +
> >> > > + ret = drm_modeset_lock(&crtc->primary->mutex, &ctx);
> >> > > + if (ret)
> >> > > +         goto exit_fail;
> >> > > +
> >> > > + if (crtc_enabled) {
> >> > > +         ret = intel_dp_upfront_commit(intel_crtc, &ctx);
> >> > > +         if (ret)
> >> > > +                 goto exit_fail;
> >> > > + }
> >> > > +
> >> > > + crtc_state = intel_dp_upfront_get_crtc_state(intel_crtc, &ctx);
> >> > > + if (IS_ERR(crtc_state)) {
> >> > > +         ret = PTR_ERR(crtc_state);
> >> > > +         goto exit_fail;
> >> > > + }
> >> > > +
> >> > > + /* Save the existing config */
> >> > > + tmp_crtc_config = intel_crtc->config;
> >> > You need to save this earlier. Otherwise if intel_dp_upfront_commit() is
> >> > called, the state will be restored with active == false when it should
> >> > actually be true.
> >> Ok, will save this earlier. But since we don’t re-enable crtc  here after
> >> upfront,
> >> (we let it get re-enabled as part of subsequent modeset) the restoring
> >> with false does not happen today..
> >>
> >> >
> >> > But that would cause intel_crtc->config to be freed, so you need to
> >> > use intel_crtc_duplicate_state().
> >> Since we alloc a new state and then only call get_crtc_state(), I thought that
> >> automatically boils down to duplicate_state() inside
> >> drm_atomic_get_crtc_state().
> >> Please correct if my understanding is wrong.
> >
> > It does, but it is a somewhat messy way of doing it in this case, since you need
> > to allocate an extra atomic_state.
> >
> > In any case, after looking at why we need to fiddle with the crtc_state, my
> > conclusion was that it is easier to remove those dependencies than to do this
> > save/restore dance. All it would take is to pass link rate and lane count
> > directly to intel_dp_set_link_params(). The bulk of intel_ddi_pre_enable() can
> > be split to a function that takes link rate, lane count and dpll as arguments,
> > and then intel_bxt_upfront_link_train() can call it directly.
> >
> >
> >
> >>
> >> >
> >> >
> >> > >
> >> > > + intel_crtc->config = crtc_state;
> >> > > +
> >> > > + if (IS_BROXTON(dev))
> >> > > +         status = intel_bxt_upfront_link_train(intel_dp,
> >> > > intel_crtc);
> >> > > +         /* Other platforms upfront link train call goes here..*/
> >> > > +
> >> > > + /* Restore the saved config */
> >> > > + intel_crtc->config = tmp_crtc_config;
> >> > > + intel_encoder->base.crtc = crtc_enabled ? crtc : NULL;
> >> > > + drm_atomic_state_free(crtc_state->base.state);
> >> > This doesn't actually re-enables the crtc if if was disabled in
> >> > intel_dp_upfront_commit(). This will require another atomic operation.
> >> > Maybe that intel_crtc_atomic_disable() should just take a bool specifying if
> >> > it
> >> > should on of off.
> >> We let it get re-enabled as part of mode set that happens subsequently.
> >> Please let me know your opinion on this.
> >
> > There's no guarantee there will be a mode set afterwards, since that's a user
> > space decision. One thing to check is if this works across suspend/resume. IIUC,
> > it is possible a hotplug event won't be sent to user space if the connector
> > status didn't change. In that case, I think this might lead to blank screen.
> >
> > Ander
> >
> >>
> >> Thanks,
> >> Durga
> >>
> >> >
> >> >
> >> >
> >> > >
> >> > > +
> >> > > +exit_fail:
> >> > > + if (ret == -EDEADLK) {
> >> > > +         drm_modeset_backoff(&ctx);
> >> > > +         goto retry;
> >> > > + }
> >> > > + drm_modeset_drop_locks(&ctx);
> >> > > + drm_modeset_acquire_fini(&ctx);
> >> > > + return status;
> >> > > +}
> >> > > +
> >> > >  static void
> >> > >  intel_dp_long_pulse(struct intel_connector *intel_connector)
> >> > >  {
> >> > > @@ -4558,7 +4722,7 @@ intel_dp_long_pulse(struct intel_connector
> >> > > *intel_connector)
> >> > >   struct drm_device *dev = connector->dev;
> >> > >   enum drm_connector_status status;
> >> > >   enum intel_display_power_domain power_domain;
> >> > > - bool ret;
> >> > > + bool ret, do_upfront_link_train;
> >> > >   u8 sink_irq_vector;
> >> > >
> >> > >   power_domain =
> >> > intel_display_port_aux_power_domain(intel_encoder);
> >> > >
> >> > > @@ -4604,7 +4768,11 @@ intel_dp_long_pulse(struct intel_connector
> >> > > *intel_connector)
> >> > >           drm_modeset_lock(&dev-
> >> > > mode_config.connection_mutex, NULL);
> >> > >           intel_dp_check_link_status(intel_dp);
> >> > >           drm_modeset_unlock(&dev-
> >> > > mode_config.connection_mutex);
> >> > > -         goto out;
> >> > > +         /*
> >> > > +          * If we are here, we have (re)read DPCD above and hence
> >> > > +          * need to do upfront link train again.
> >> > > +          */
> >> > > +         goto upfront;
> >> > We can avoid this by not overwriting intel_dp->dpcd as I mentioned above.
> >> >
> >> >
> >> > Ander
> >> >
> >> > >
> >> > >   }
> >> > >
> >> > >   /*
> >> > > @@ -4634,6 +4802,14 @@ intel_dp_long_pulse(struct intel_connector
> >> > > *intel_connector)
> >> > >                   DRM_DEBUG_DRIVER("CP or sink specific irq
> >> > > unhandled\n");
> >> > >   }
> >> > >
> >> > > +upfront:
> >> > > + /* Do not do upfront link train, if it is a compliance request */
> >> > > + do_upfront_link_train =
> >> > > +         intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
> >> > > +         intel_dp->compliance_test_type !=
> >> > DP_TEST_LINK_TRAINING;
> >> > >
> >> > > +
> >> > > + if (do_upfront_link_train &&
> >> > intel_dp_upfront_link_train(connector))
> >> > >
> >> > > +         status = connector_status_disconnected;
> >> > >  out:
> >> > >   if (status != connector_status_connected) {
> >> > >           intel_dp_unset_edid(intel_dp);
> >> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> > > b/drivers/gpu/drm/i915/intel_drv.h
> >> > > index 27b5dcd..bf98473 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> >> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> > > @@ -1082,6 +1082,8 @@ void intel_ddi_clock_get(struct intel_encoder
> >> > *encoder,
> >> > >
> >> > >                    struct intel_crtc_state *pipe_config);
> >> > >  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
> >> > >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> >> > > +int intel_bxt_upfront_link_train(struct intel_dp *intel_dp,
> >> > > +                         struct intel_crtc *crtc);
> >> > >
> >> > >  /* intel_frontbuffer.c */
> >> > >  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> >> > > @@ -1284,6 +1286,9 @@ bool intel_dp_init_connector(struct
> >> > intel_digital_port
> >> > >
> >> > > *intel_dig_port,
> >> > >                        struct intel_connector *intel_connector);
> >> > >  void intel_dp_set_link_params(struct intel_dp *intel_dp,
> >> > >                         const struct intel_crtc_state
> >> > > *pipe_config);
> >> > > +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
> >> > > +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> >> > > +                         uint8_t *lane_count, uint8_t *link_bw);
> >> > >  void intel_dp_start_link_train(struct intel_dp *intel_dp);
> >> > >  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> >> > >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Add USB typeC based DP support for BXT platform (rev5)

[Patchwork]

== Series Details ==

Series: Add USB typeC based DP support for BXT platform (rev5)
URL   : https://patchwork.freedesktop.org/series/1731/
State : failure

== Summary ==

Series 1731v5 Add USB typeC based DP support for BXT platform
http://patchwork.freedesktop.org/api/1.0/series/1731/revisions/5/mbox/

Test gem_busy:
        Subgroup basic-blt:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_exec_whisper:
        Subgroup basic:
                incomplete -> PASS       (bdw-nuci7)
Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                pass       -> SKIP       (snb-x220t)

bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:202  pass:162  dwarn:0   dfail:0   fail:0   skip:40 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:203  pass:184  dwarn:0   dfail:0   fail:0   skip:19 
ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:203  pass:165  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:203  pass:164  dwarn:0   dfail:0   fail:1   skip:38 
BOOT FAILED for ilk-hp8440p

Results at /archive/results/CI_IGT_test/Patchwork_1928/

78673b24b60c1a884c947ee5a45ad860ca618418 drm-intel-nightly: 2016y-04m-18d-12h-32m-33s UTC integration manifest
09cbee3 drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
8d1b436 drm/i915: Split bxt_ddi_pll_select()
8240bff drm/i915: Make finding unused crtc as a generic function

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