[PATCH 1/4] drm/atomic-helper: Add an atomice best_encoder callback

[PATCH 1/4] drm/atomic-helper: Add an atomice best_encoder callback

[Daniel Vetter]

With legacy helpers all the routing was already set up when calling
best_encoder and so could be inspected. But with atomic it's staged,
hence we need a new atomic compliant callback for drivers which need
to inspect the requested state and can't just decided the best encoder
statically.

This is needed to fix up i915 dp mst where we need to pick the right
encoder depending upon the requested CRTC for the connector.

v2: Don't forget to amend the kerneldoc

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++-
 include/drm/drm_crtc_helper.h       | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index aac212297b49..8694ca9beee3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -196,7 +196,12 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 	}
 
 	funcs = connector->helper_private;
-	new_encoder = funcs->best_encoder(connector);
+
+	if (funcs->atomic_best_encoder)
+		new_encoder = funcs->atomic_best_encoder(connector,
+							 connector_state);
+	else
+		new_encoder = funcs->best_encoder(connector);
 
 	if (!new_encoder) {
 		DRM_DEBUG_ATOMIC("No suitable encoder found for [CONNECTOR:%d:%s]\n",
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index c8fc187061de..918aa68b5199 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -168,6 +168,7 @@ struct drm_encoder_helper_funcs {
  * @get_modes: get mode list for this connector
  * @mode_valid: is this mode valid on the given connector? (optional)
  * @best_encoder: return the preferred encoder for this connector
+ * @atomic_best_encoder: atomic version of @best_encoder
  *
  * The helper operations are called by the mid-layer CRTC helper.
  */
@@ -176,6 +177,8 @@ struct drm_connector_helper_funcs {
 	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
 					   struct drm_display_mode *mode);
 	struct drm_encoder *(*best_encoder)(struct drm_connector *connector);
+	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
+						   struct drm_connector_state *connector_state);
 };
 
 extern void drm_helper_disable_unused_functions(struct drm_device *dev);
-- 
2.5.0

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

[PATCH 2/4] drm/i915: Fixup dp mst encoder selection

[Daniel Vetter]

In

commit 8c7b5ccb729870e606321b3703e2c2e698c49a95
Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Date:   Tue Apr 21 17:13:19 2015 +0300

    drm/i915: Use atomic helpers for computing changed flags

we've switched over to the atomic version to compute the
crtc->encoder->connector routing from the i915 variant. That one
relies upon the ->best_encoder callback, but the i915-private version
relied upon intel_find_encoder. Which didn't matter except for dp mst,
where the encoder depends upon the selected crtc.

Fix this functional bug by implemented a correct atomic-state based
encoder selector for dp mst.

Note that we can't get rid of the legacy best_encoder callback since
the fbdev emulation uses that still. That means it's incorrect there
still, but that's been the case ever since i915 dp mst support was
merged so not a regression. Best to fix that by converting fbdev over
to atomic too.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 6e4cc5334f47..600afdbef8c9 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -357,6 +357,16 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
+static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *connector,
+							 struct drm_connector_state *state)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_dp *intel_dp = intel_connector->mst_port;
+	struct intel_crtc *crtc = to_intel_crtc(state->crtc);
+
+	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
+}
+
 static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
@@ -367,6 +377,7 @@ static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connecto
 static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_funcs = {
 	.get_modes = intel_dp_mst_get_modes,
 	.mode_valid = intel_dp_mst_mode_valid,
+	.atomic_best_encoder = intel_mst_atomic_best_encoder,
 	.best_encoder = intel_mst_best_encoder,
 };
 
-- 
2.5.0

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

[PATCH 3/4] drm/dp-mst: Remove debug WARN_ON

[Daniel Vetter]

Apparently been in there since forever and fairly easy to hit when
hotplugging really fast. I can do that since my mst hub has a manual
button to flick the hpd line for reprobing. The resulting WARNING spam
isn't pretty.

Cc: Dave Airlie <airlied@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 778bbb6425b8..b0487c9f018c 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1294,7 +1294,6 @@ retry:
 				goto retry;
 			}
 			DRM_DEBUG_KMS("failed to dpcd write %d %d\n", tosend, ret);
-			WARN(1, "fail\n");
 
 			return -EIO;
 		}
-- 
2.5.0

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

Re: [PATCH 3/4] drm/dp-mst: Remove debug WARN_ON

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 991 bytes --]

On Mon, Aug 03, 2015 at 05:24:10PM +0200, Daniel Vetter wrote:
> Apparently been in there since forever and fairly easy to hit when
> hotplugging really fast. I can do that since my mst hub has a manual
> button to flick the hpd line for reprobing. The resulting WARNING spam
> isn't pretty.
> 
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 778bbb6425b8..b0487c9f018c 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1294,7 +1294,6 @@ retry:
>  				goto retry;
>  			}
>  			DRM_DEBUG_KMS("failed to dpcd write %d %d\n", tosend, ret);
> -			WARN(1, "fail\n");
>  
>  			return -EIO;
>  		}

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 4/4] drm/atomic-helpers: Make encoder picking more robust

[Daniel Vetter]

We've had a few issues with atomic where subtle bugs in the encoder
picking logic lead to accidental self-stealing of the encoder,
resulting in a NULL connector_state->crtc in update_connector_routing
and subsequent.

Linus applied some duct-tape for an mst regression in

commit 27667f4744fc5a0f3e50910e78740bac5670d18b
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed Jul 29 22:18:16 2015 -0700

    i915: temporary fix for DP MST docking station NULL pointer dereference

But that was incomplete (the code will still oops when debuggin is
enabled) and mangled the state even further. So instead WARN and bail
out as the more future-proof option.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8694ca9beee3..9dcc7280e572 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 		}
 	}
 
+	if (WARN_ON(!connector_state->crtc))
+		return -EINVAL;
+
 	connector_state->best_encoder = new_encoder;
-	if (connector_state->crtc) {
-		idx = drm_crtc_index(connector_state->crtc);
+	idx = drm_crtc_index(connector_state->crtc);
 
-		crtc_state = state->crtc_states[idx];
-		crtc_state->mode_changed = true;
-	}
+	crtc_state = state->crtc_states[idx];
+	crtc_state->mode_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
 			 connector->base.id,
-- 
2.5.0

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

Re: [PATCH 4/4] drm/atomic-helpers: Make encoder picking more robust

[Ander Conselvan De Oliveira]

For the series:

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


On Mon, 2015-08-03 at 17:24 +0200, Daniel Vetter wrote:
> We've had a few issues with atomic where subtle bugs in the encoder
> picking logic lead to accidental self-stealing of the encoder,
> resulting in a NULL connector_state->crtc in update_connector_routing
> and subsequent.
> 
> Linus applied some duct-tape for an mst regression in
> 
> commit 27667f4744fc5a0f3e50910e78740bac5670d18b
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Wed Jul 29 22:18:16 2015 -0700
> 
>     i915: temporary fix for DP MST docking station NULL pointer dereference
> 
> But that was incomplete (the code will still oops when debuggin is
> enabled) and mangled the state even further. So instead WARN and bail
> out as the more future-proof option.
> 
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8694ca9beee3..9dcc7280e572 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>  		}
>  	}
>  
> +	if (WARN_ON(!connector_state->crtc))
> +		return -EINVAL;
> +
>  	connector_state->best_encoder = new_encoder;
> -	if (connector_state->crtc) {
> -		idx = drm_crtc_index(connector_state->crtc);
> +	idx = drm_crtc_index(connector_state->crtc);
>  
> -		crtc_state = state->crtc_states[idx];
> -		crtc_state->mode_changed = true;
> -	}
> +	crtc_state = state->crtc_states[idx];
> +	crtc_state->mode_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
>  			 connector->base.id,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/atomic-helpers: Make encoder picking more robust

[Daniel Vetter]

On Tue, Aug 04, 2015 at 11:56:08AM +0300, Ander Conselvan De Oliveira wrote:
> For the series:
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Thanks for the review.
-Daniel
> 
> 
> On Mon, 2015-08-03 at 17:24 +0200, Daniel Vetter wrote:
> > We've had a few issues with atomic where subtle bugs in the encoder
> > picking logic lead to accidental self-stealing of the encoder,
> > resulting in a NULL connector_state->crtc in update_connector_routing
> > and subsequent.
> > 
> > Linus applied some duct-tape for an mst regression in
> > 
> > commit 27667f4744fc5a0f3e50910e78740bac5670d18b
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Wed Jul 29 22:18:16 2015 -0700
> > 
> >     i915: temporary fix for DP MST docking station NULL pointer dereference
> > 
> > But that was incomplete (the code will still oops when debuggin is
> > enabled) and mangled the state even further. So instead WARN and bail
> > out as the more future-proof option.
> > 
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 8694ca9beee3..9dcc7280e572 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
> >  		}
> >  	}
> >  
> > +	if (WARN_ON(!connector_state->crtc))
> > +		return -EINVAL;
> > +
> >  	connector_state->best_encoder = new_encoder;
> > -	if (connector_state->crtc) {
> > -		idx = drm_crtc_index(connector_state->crtc);
> > +	idx = drm_crtc_index(connector_state->crtc);
> >  
> > -		crtc_state = state->crtc_states[idx];
> > -		crtc_state->mode_changed = true;
> > -	}
> > +	crtc_state = state->crtc_states[idx];
> > +	crtc_state->mode_changed = true;
> >  
> >  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
> >  			 connector->base.id,

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

Re: [PATCH 4/4] drm/atomic-helpers: Make encoder picking more robust

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1986 bytes --]

On Mon, Aug 03, 2015 at 05:24:11PM +0200, Daniel Vetter wrote:
> We've had a few issues with atomic where subtle bugs in the encoder
> picking logic lead to accidental self-stealing of the encoder,
> resulting in a NULL connector_state->crtc in update_connector_routing
> and subsequent.
> 
> Linus applied some duct-tape for an mst regression in
> 
> commit 27667f4744fc5a0f3e50910e78740bac5670d18b
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Wed Jul 29 22:18:16 2015 -0700
> 
>     i915: temporary fix for DP MST docking station NULL pointer dereference
> 
> But that was incomplete (the code will still oops when debuggin is
> enabled) and mangled the state even further. So instead WARN and bail
> out as the more future-proof option.
> 
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8694ca9beee3..9dcc7280e572 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>  		}
>  	}
>  
> +	if (WARN_ON(!connector_state->crtc))
> +		return -EINVAL;
> +
>  	connector_state->best_encoder = new_encoder;
> -	if (connector_state->crtc) {
> -		idx = drm_crtc_index(connector_state->crtc);
> +	idx = drm_crtc_index(connector_state->crtc);
>  
> -		crtc_state = state->crtc_states[idx];
> -		crtc_state->mode_changed = true;
> -	}
> +	crtc_state = state->crtc_states[idx];
> +	crtc_state->mode_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
>  			 connector->base.id,

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/4] drm/atomic-helper: Add an atomice best_encoder callback

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2478 bytes --]

On Mon, Aug 03, 2015 at 05:24:08PM +0200, Daniel Vetter wrote:
> With legacy helpers all the routing was already set up when calling
> best_encoder and so could be inspected. But with atomic it's staged,
> hence we need a new atomic compliant callback for drivers which need
> to inspect the requested state and can't just decided the best encoder
> statically.
> 
> This is needed to fix up i915 dp mst where we need to pick the right
> encoder depending upon the requested CRTC for the connector.
> 
> v2: Don't forget to amend the kerneldoc
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++-
>  include/drm/drm_crtc_helper.h       | 3 +++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index aac212297b49..8694ca9beee3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -196,7 +196,12 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>  	}
>  
>  	funcs = connector->helper_private;
> -	new_encoder = funcs->best_encoder(connector);
> +
> +	if (funcs->atomic_best_encoder)
> +		new_encoder = funcs->atomic_best_encoder(connector,
> +							 connector_state);
> +	else
> +		new_encoder = funcs->best_encoder(connector);
>  
>  	if (!new_encoder) {
>  		DRM_DEBUG_ATOMIC("No suitable encoder found for [CONNECTOR:%d:%s]\n",
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index c8fc187061de..918aa68b5199 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -168,6 +168,7 @@ struct drm_encoder_helper_funcs {
>   * @get_modes: get mode list for this connector
>   * @mode_valid: is this mode valid on the given connector? (optional)
>   * @best_encoder: return the preferred encoder for this connector
> + * @atomic_best_encoder: atomic version of @best_encoder

This could arguably be more verbose, but the limits of kerneldoc don't
make that easy. Perhaps a more verbose description could be provided in
done in the DRM DocBook, along with other documentation about atomic
mode-setting.

Irrespective of that, this makes sense, so:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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