[PATCH v2 1/6] drm/i915: Set connector_state->connector using the helper.

[PATCH v2 1/6] drm/i915: Set connector_state->connector using the helper.

[Maarten Lankhorst]

The atomic helper sets connector_state->connector, which the i915
code didn't. This will become a problem when we start using it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7dd7200d3ba9..1e42309ec40a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6477,13 +6477,11 @@ static void intel_connector_check_state(struct intel_connector *connector)
 
 int intel_connector_init(struct intel_connector *connector)
 {
-	struct drm_connector_state *connector_state;
+	drm_atomic_helper_connector_reset(&connector->base);
 
-	connector_state = kzalloc(sizeof *connector_state, GFP_KERNEL);
-	if (!connector_state)
+	if (!connector->base.state)
 		return -ENOMEM;
 
-	connector->base.state = connector_state;
 	return 0;
 }
 
-- 
2.1.0

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

[PATCH v2 2/6] drm/atomic: Add __drm_atomic_helper_connector_reset, v2.

[Maarten Lankhorst]

This is useful for drivers that subclass connector_state, like tegra.

Changes since v1:
- Docbook updates.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++++++++++++----
 include/drm/drm_atomic_helper.h     |  2 ++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 63f925b75357..27dd68f946e6 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2607,6 +2607,28 @@ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
 
 /**
+ * __drm_atomic_helper_connector_reset - reset state on connector
+ * @connector: drm connector
+ * @conn_state: connector state to assign
+ *
+ * Initializes the newly allocated @conn_state and assigns it to
+ * #connector ->state, usually required when initializing the drivers
+ * or when called from the ->reset hook.
+ *
+ * This is useful for drivers that subclass the connector state.
+ */
+void
+__drm_atomic_helper_connector_reset(struct drm_connector *connector,
+				    struct drm_connector_state *conn_state)
+{
+	if (conn_state)
+		conn_state->connector = connector;
+
+	connector->state = conn_state;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_connector_reset);
+
+/**
  * drm_atomic_helper_connector_reset - default ->reset hook for connectors
  * @connector: drm connector
  *
@@ -2616,11 +2638,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
  */
 void drm_atomic_helper_connector_reset(struct drm_connector *connector)
 {
-	kfree(connector->state);
-	connector->state = kzalloc(sizeof(*connector->state), GFP_KERNEL);
+	struct drm_connector_state *conn_state =
+		kzalloc(sizeof(*conn_state), GFP_KERNEL);
 
-	if (connector->state)
-		connector->state->connector = connector;
+	kfree(connector->state);
+	__drm_atomic_helper_connector_reset(connector, conn_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index a286cce98720..89d008dc08e2 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -126,6 +126,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 					  struct drm_plane_state *state);
 
+void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
+					 struct drm_connector_state *conn_state);
 void drm_atomic_helper_connector_reset(struct drm_connector *connector);
 void
 __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
-- 
2.1.0

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

Re: [PATCH v2 2/6] drm/atomic: Add __drm_atomic_helper_connector_reset, v2.

[Daniel Vetter]

On Mon, Jan 04, 2016 at 12:53:16PM +0100, Maarten Lankhorst wrote:
> This is useful for drivers that subclass connector_state, like tegra.
> 
> Changes since v1:
> - Docbook updates.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++++++++++++----
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 63f925b75357..27dd68f946e6 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2607,6 +2607,28 @@ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
>  
>  /**
> + * __drm_atomic_helper_connector_reset - reset state on connector
> + * @connector: drm connector
> + * @conn_state: connector state to assign
> + *
> + * Initializes the newly allocated @conn_state and assigns it to
> + * #connector ->state, usually required when initializing the drivers
> + * or when called from the ->reset hook.
> + *
> + * This is useful for drivers that subclass the connector state.
> + */
> +void
> +__drm_atomic_helper_connector_reset(struct drm_connector *connector,
> +				    struct drm_connector_state *conn_state)
> +{
> +	if (conn_state)
> +		conn_state->connector = connector;
> +
> +	connector->state = conn_state;
> +}
> +EXPORT_SYMBOL(__drm_atomic_helper_connector_reset);

It's a bit strange that we don't have __*reset functions for crtc/plane
now, especially since that would uncover that probably we should move
cleaning up the old state into this helper (to share more code). That can
be done by simply calling ->atomic_state_destroy. Callers would then only
allocate a suitable struct, call this func and init any driver-private
state.

But this here won't hurt, so merged it.
-Daniel

> +
> +/**
>   * drm_atomic_helper_connector_reset - default ->reset hook for connectors
>   * @connector: drm connector
>   *
> @@ -2616,11 +2638,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
>   */
>  void drm_atomic_helper_connector_reset(struct drm_connector *connector)
>  {
> -	kfree(connector->state);
> -	connector->state = kzalloc(sizeof(*connector->state), GFP_KERNEL);
> +	struct drm_connector_state *conn_state =
> +		kzalloc(sizeof(*conn_state), GFP_KERNEL);
>  
> -	if (connector->state)
> -		connector->state->connector = connector;
> +	kfree(connector->state);
> +	__drm_atomic_helper_connector_reset(connector, conn_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
>  
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index a286cce98720..89d008dc08e2 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -126,6 +126,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  					  struct drm_plane_state *state);
>  
> +void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
> +					 struct drm_connector_state *conn_state);
>  void drm_atomic_helper_connector_reset(struct drm_connector *connector);
>  void
>  __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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

[PATCH v2 3/6] drm/tegra: Use __drm_atomic_helper_reset_connector for subclassing connector state, v2.

[Maarten Lankhorst]

Changes since v1:
- Do not reset if state allocation fails.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Thierry Reding <treding@nvidia.com> #irc
---
 drivers/gpu/drm/tegra/dsi.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 50d46ae3786b..44e102799195 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -745,14 +745,13 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
 
 static void tegra_dsi_connector_reset(struct drm_connector *connector)
 {
-	struct tegra_dsi_state *state;
-
-	kfree(connector->state);
-	connector->state = NULL;
+	struct tegra_dsi_state *state =
+		kzalloc(sizeof(*state), GFP_KERNEL);
 
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state)
-		connector->state = &state->base;
+	if (state) {
+		kfree(connector->state);
+		__drm_atomic_helper_connector_reset(connector, &state->base);
+	}
 }
 
 static struct drm_connector_state *
-- 
2.1.0

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

[PATCH v2 4/6] drm/atomic: add connector mask to drm_crtc_state.

[Maarten Lankhorst]

It can be useful to iterate over connectors without grabbing
connection_mutex. It can also be used to see how many connectors
are on a crtc without iterating over the list.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 11 +++++++++++
 include/drm/drm_crtc.h       |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6a21e5c378c1..14b321580517 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1063,10 +1063,21 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 {
 	struct drm_crtc_state *crtc_state;
 
+	if (conn_state->crtc && conn_state->crtc != crtc) {
+		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
+								conn_state->crtc);
+
+		crtc_state->connector_mask &=
+			~(1 << drm_connector_index(conn_state->connector));
+	}
+
 	if (crtc) {
 		crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc);
 		if (IS_ERR(crtc_state))
 			return PTR_ERR(crtc_state);
+
+		crtc_state->connector_mask |=
+			1 << drm_connector_index(conn_state->connector);
 	}
 
 	conn_state->crtc = crtc;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c2f98ba2bb98..dd0db4ceab26 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -301,6 +301,7 @@ struct drm_plane_helper_funcs;
  * @active_changed: crtc_state->active has been toggled.
  * @connectors_changed: connectors to this crtc have been updated
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
+ * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
  * @last_vblank_count: for helpers and drivers to capture the vblank of the
  * 	update to ensure framebuffer cleanup isn't done too early
  * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
@@ -334,6 +335,8 @@ struct drm_crtc_state {
 	 */
 	u32 plane_mask;
 
+	u32 connector_mask;
+
 	/* last_vblank_count: for vblank waits before cleanup */
 	u32 last_vblank_count;
 
-- 
2.1.0

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

Re: [PATCH v2 4/6] drm/atomic: add connector mask to drm_crtc_state.

[Daniel Vetter]

On Mon, Jan 04, 2016 at 12:53:18PM +0100, Maarten Lankhorst wrote:
> It can be useful to iterate over connectors without grabbing
> connection_mutex. It can also be used to see how many connectors
> are on a crtc without iterating over the list.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Merged up to this patch here, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c | 11 +++++++++++
>  include/drm/drm_crtc.h       |  3 +++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6a21e5c378c1..14b321580517 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1063,10 +1063,21 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  {
>  	struct drm_crtc_state *crtc_state;
>  
> +	if (conn_state->crtc && conn_state->crtc != crtc) {
> +		crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,
> +								conn_state->crtc);
> +
> +		crtc_state->connector_mask &=
> +			~(1 << drm_connector_index(conn_state->connector));
> +	}
> +
>  	if (crtc) {
>  		crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc);
>  		if (IS_ERR(crtc_state))
>  			return PTR_ERR(crtc_state);
> +
> +		crtc_state->connector_mask |=
> +			1 << drm_connector_index(conn_state->connector);
>  	}
>  
>  	conn_state->crtc = crtc;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c2f98ba2bb98..dd0db4ceab26 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -301,6 +301,7 @@ struct drm_plane_helper_funcs;
>   * @active_changed: crtc_state->active has been toggled.
>   * @connectors_changed: connectors to this crtc have been updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
> + * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   * 	update to ensure framebuffer cleanup isn't done too early
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
> @@ -334,6 +335,8 @@ struct drm_crtc_state {
>  	 */
>  	u32 plane_mask;
>  
> +	u32 connector_mask;
> +
>  	/* last_vblank_count: for vblank waits before cleanup */
>  	u32 last_vblank_count;
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 5/6] drm/i915: Update connector_mask during readout.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1e42309ec40a..b76778d76035 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
+		crtc->base.state->connector_mask = 0;
 
 		/* Because we only establish the connector -> encoder ->
 		 * crtc links if something is active, this means the
@@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 {
 	struct intel_connector *connector;
 	struct drm_device *dev = encoder->base.dev;
+	struct drm_crtc *crtc = encoder->base.crtc;
 	bool active = false;
 
 	/* We need to check both for a crtc link (meaning that the
 	 * encoder is active and trying to read from a pipe) and the
 	 * pipe itself being active. */
-	bool has_active_crtc = encoder->base.crtc &&
-		to_intel_crtc(encoder->base.crtc)->active;
+	bool has_active_crtc = crtc && crtc->state->active;
 
 	for_each_intel_connector(dev, connector) {
 		if (connector->base.encoder != &encoder->base)
 			continue;
 
 		active = true;
-		break;
+		if (!has_active_crtc)
+			break;
+
+		crtc->state->connector_mask |=
+			1 << drm_connector_index(&connector->base);
 	}
 
 	if (active && !has_active_crtc) {
-- 
2.1.0

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

Re: [PATCH v2 5/6] drm/i915: Update connector_mask during readout.

[Daniel Vetter]

On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1e42309ec40a..b76778d76035 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
>  		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
> +		crtc->base.state->connector_mask = 0;
>  
>  		/* Because we only establish the connector -> encoder ->
>  		 * crtc links if something is active, this means the
> @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  {
>  	struct intel_connector *connector;
>  	struct drm_device *dev = encoder->base.dev;
> +	struct drm_crtc *crtc = encoder->base.crtc;
>  	bool active = false;
>  
>  	/* We need to check both for a crtc link (meaning that the
>  	 * encoder is active and trying to read from a pipe) and the
>  	 * pipe itself being active. */
> -	bool has_active_crtc = encoder->base.crtc &&
> -		to_intel_crtc(encoder->base.crtc)->active;
> +	bool has_active_crtc = crtc && crtc->state->active;
>  
>  	for_each_intel_connector(dev, connector) {
>  		if (connector->base.encoder != &encoder->base)
>  			continue;
>  
>  		active = true;
> -		break;
> +		if (!has_active_crtc)
> +			break;
> +
> +		crtc->state->connector_mask |=
> +			1 << drm_connector_index(&connector->base);

I still think this is the wrong place. Imo this should be done in
intel_modeset_update_connector_atomic_state. It'd be great if we could
somehow share the logic with drm_atomic_set_crtc_for_connector even, but
that's probably over the top.
-Daniel

>  	}
>  
>  	if (active && !has_active_crtc) {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/6] drm/i915: Update connector_mask during readout.

[Maarten Lankhorst]

Op 05-01-16 om 09:35 schreef Daniel Vetter:
> On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1e42309ec40a..b76778d76035 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>>  		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
>>  		crtc->base.state->active = crtc->active;
>>  		crtc->base.enabled = crtc->active;
>> +		crtc->base.state->connector_mask = 0;
>>  
>>  		/* Because we only establish the connector -> encoder ->
>>  		 * crtc links if something is active, this means the
>> @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>>  {
>>  	struct intel_connector *connector;
>>  	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_crtc *crtc = encoder->base.crtc;
>>  	bool active = false;
>>  
>>  	/* We need to check both for a crtc link (meaning that the
>>  	 * encoder is active and trying to read from a pipe) and the
>>  	 * pipe itself being active. */
>> -	bool has_active_crtc = encoder->base.crtc &&
>> -		to_intel_crtc(encoder->base.crtc)->active;
>> +	bool has_active_crtc = crtc && crtc->state->active;
>>  
>>  	for_each_intel_connector(dev, connector) {
>>  		if (connector->base.encoder != &encoder->base)
>>  			continue;
>>  
>>  		active = true;
>> -		break;
>> +		if (!has_active_crtc)
>> +			break;
>> +
>> +		crtc->state->connector_mask |=
>> +			1 << drm_connector_index(&connector->base);
> I still think this is the wrong place. Imo this should be done in
> intel_modeset_update_connector_atomic_state. It'd be great if we could
> somehow share the logic with drm_atomic_set_crtc_for_connector even, but
> that's probably over the top.
>
No it should be done sooner. I want to be able use it anywhere in the .crtc_disable calls without worrying about it..

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

Re: [Intel-gfx] [PATCH v2 5/6] drm/i915: Update connector_mask during readout.

[Daniel Vetter]

On Tue, Jan 05, 2016 at 10:05:21AM +0100, Maarten Lankhorst wrote:
> Op 05-01-16 om 09:35 schreef Daniel Vetter:
> > On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 1e42309ec40a..b76778d76035 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >>  		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
> >>  		crtc->base.state->active = crtc->active;
> >>  		crtc->base.enabled = crtc->active;
> >> +		crtc->base.state->connector_mask = 0;
> >>  
> >>  		/* Because we only establish the connector -> encoder ->
> >>  		 * crtc links if something is active, this means the
> >> @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
> >>  {
> >>  	struct intel_connector *connector;
> >>  	struct drm_device *dev = encoder->base.dev;
> >> +	struct drm_crtc *crtc = encoder->base.crtc;
> >>  	bool active = false;
> >>  
> >>  	/* We need to check both for a crtc link (meaning that the
> >>  	 * encoder is active and trying to read from a pipe) and the
> >>  	 * pipe itself being active. */
> >> -	bool has_active_crtc = encoder->base.crtc &&
> >> -		to_intel_crtc(encoder->base.crtc)->active;
> >> +	bool has_active_crtc = crtc && crtc->state->active;
> >>  
> >>  	for_each_intel_connector(dev, connector) {
> >>  		if (connector->base.encoder != &encoder->base)
> >>  			continue;
> >>  
> >>  		active = true;
> >> -		break;
> >> +		if (!has_active_crtc)
> >> +			break;
> >> +
> >> +		crtc->state->connector_mask |=
> >> +			1 << drm_connector_index(&connector->base);
> > I still think this is the wrong place. Imo this should be done in
> > intel_modeset_update_connector_atomic_state. It'd be great if we could
> > somehow share the logic with drm_atomic_set_crtc_for_connector even, but
> > that's probably over the top.
> >
> No it should be done sooner. I want to be able use it anywhere in the
> .crtc_disable calls without worrying about it..

Well I don't want to split things up all over. Atm our state recover is a
complete mess, and we need to start recovering some order in it. Updating
related things at completely different places without even a comment
stating why that's required is imo a no-go.
-Daniel
-- 
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 v2 5/6] drm/i915: Update connector_mask during readout.

[Maarten Lankhorst]

Op 05-01-16 om 10:10 schreef Daniel Vetter:
> On Tue, Jan 05, 2016 at 10:05:21AM +0100, Maarten Lankhorst wrote:
>> Op 05-01-16 om 09:35 schreef Daniel Vetter:
>>> On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote:
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++---
>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 1e42309ec40a..b76778d76035 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>>>>  		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
>>>>  		crtc->base.state->active = crtc->active;
>>>>  		crtc->base.enabled = crtc->active;
>>>> +		crtc->base.state->connector_mask = 0;
>>>>  
>>>>  		/* Because we only establish the connector -> encoder ->
>>>>  		 * crtc links if something is active, this means the
>>>> @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>>>>  {
>>>>  	struct intel_connector *connector;
>>>>  	struct drm_device *dev = encoder->base.dev;
>>>> +	struct drm_crtc *crtc = encoder->base.crtc;
>>>>  	bool active = false;
>>>>  
>>>>  	/* We need to check both for a crtc link (meaning that the
>>>>  	 * encoder is active and trying to read from a pipe) and the
>>>>  	 * pipe itself being active. */
>>>> -	bool has_active_crtc = encoder->base.crtc &&
>>>> -		to_intel_crtc(encoder->base.crtc)->active;
>>>> +	bool has_active_crtc = crtc && crtc->state->active;
>>>>  
>>>>  	for_each_intel_connector(dev, connector) {
>>>>  		if (connector->base.encoder != &encoder->base)
>>>>  			continue;
>>>>  
>>>>  		active = true;
>>>> -		break;
>>>> +		if (!has_active_crtc)
>>>> +			break;
>>>> +
>>>> +		crtc->state->connector_mask |=
>>>> +			1 << drm_connector_index(&connector->base);
>>> I still think this is the wrong place. Imo this should be done in
>>> intel_modeset_update_connector_atomic_state. It'd be great if we could
>>> somehow share the logic with drm_atomic_set_crtc_for_connector even, but
>>> that's probably over the top.
>>>
>> No it should be done sooner. I want to be able use it anywhere in the
>> .crtc_disable calls without worrying about it..
> Well I don't want to split things up all over. Atm our state recover is a
> complete mess, and we need to start recovering some order in it. Updating
> related things at completely different places without even a comment
> stating why that's required is imo a no-go.
Ok so if I resubmit with a comment in this hunk it will be acceptable?

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

[PATCH v2.1 5/6] drm/i915: Update connector_mask during readout, v2.

[Maarten Lankhorst]

drm/i915: Update connector_mask during readout, v2.

The connector_mask may be used any time during the non-atomic
.crtc_disable which is called before the full atomic state is
set up and needs to be accurate for that reason.

Changes since v1:
- Update connector_mask in readout_hw_state and add a comment.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fde49d90c37f..547862f6951e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15602,6 +15602,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
+		crtc->base.state->connector_mask = 0;
 
 		/* Because we only establish the connector -> encoder ->
 		 * crtc links if something is active, this means the
@@ -15828,7 +15829,21 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	for_each_intel_connector(dev, connector) {
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
-			connector->base.encoder = &connector->encoder->base;
+
+			encoder = connector->encoder;
+			connector->base.encoder = &encoder->base;
+
+			if (encoder->base.crtc &&
+			    encoder->base.crtc->state->active) {
+				/*
+				 * This has to be done during hardware readout
+				 * because anything calling .crtc_disable may
+				 * rely on the connector_mask being accurate.
+				 */
+				encoder->base.crtc->state->connector_mask |=
+					1 << drm_connector_index(&connector->base);
+			}
+
 		} else {
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
 			connector->base.encoder = NULL;

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

[PATCH v2 6/6] drm/atomic: Remove drm_atomic_connectors_for_crtc.

[Maarten Lankhorst]

Now that connector_mask is reliable there's no need for this
function any more.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 30 ------------------------------
 drivers/gpu/drm/drm_atomic_helper.c | 10 ++++------
 drivers/gpu/drm/vc4/vc4_crtc.c      |  2 +-
 include/drm/drm_atomic.h            |  4 ----
 4 files changed, 5 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 14b321580517..d3ed12447fbb 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1183,36 +1183,6 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_add_affected_planes);
 
 /**
- * drm_atomic_connectors_for_crtc - count number of connected outputs
- * @state: atomic state
- * @crtc: DRM crtc
- *
- * This function counts all connectors which will be connected to @crtc
- * according to @state. Useful to recompute the enable state for @crtc.
- */
-int
-drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
-			       struct drm_crtc *crtc)
-{
-	struct drm_connector *connector;
-	struct drm_connector_state *conn_state;
-
-	int i, num_connected_connectors = 0;
-
-	for_each_connector_in_state(state, connector, conn_state, i) {
-		if (conn_state->crtc == crtc)
-			num_connected_connectors++;
-	}
-
-	DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d:%s]\n",
-			 state, num_connected_connectors,
-			 crtc->base.id, crtc->name);
-
-	return num_connected_connectors;
-}
-EXPORT_SYMBOL(drm_atomic_connectors_for_crtc);
-
-/**
  * drm_atomic_legacy_backoff - locking backoff for legacy ioctls
  * @state: atomic state
  *
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 27dd68f946e6..13b771cb0d35 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -464,7 +464,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	 * crtc only changed its mode but has the same set of connectors.
 	 */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		int num_connectors;
+		bool has_connectors =
+			!!crtc_state->connector_mask;
 
 		/*
 		 * We must set ->active_changed after walking connectors for
@@ -493,10 +494,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (ret != 0)
 			return ret;
 
-		num_connectors = drm_atomic_connectors_for_crtc(state,
-								crtc);
-
-		if (crtc_state->enable != !!num_connectors) {
+		if (crtc_state->enable != has_connectors) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n",
 					 crtc->base.id, crtc->name);
 
@@ -1755,7 +1753,7 @@ static int update_output_state(struct drm_atomic_state *state,
 		if (crtc == set->crtc)
 			continue;
 
-		if (!drm_atomic_connectors_for_crtc(state, crtc)) {
+		if (!crtc_state->connector_mask) {
 			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
 								NULL);
 			if (ret < 0)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 2168a99d59aa..aa7ed24a41c7 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -327,7 +327,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
 	/* The pixelvalve can only feed one encoder (and encoders are
 	 * 1:1 with connectors.)
 	 */
-	if (drm_atomic_connectors_for_crtc(state->state, crtc) > 1)
+	if (hweight32(state->connector_mask) > 1)
 		return -EINVAL;
 
 	drm_atomic_crtc_state_for_each_plane(plane, state) {
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d8576ac55693..d3eaa5df187a 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -130,10 +130,6 @@ int __must_check
 drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 			       struct drm_crtc *crtc);
 
-int
-drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
-			       struct drm_crtc *crtc);
-
 void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
 
 void
-- 
2.1.0

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

Re: [PATCH v2 6/6] drm/atomic: Remove drm_atomic_connectors_for_crtc.

[Daniel Vetter]

On Mon, Jan 04, 2016 at 12:53:20PM +0100, Maarten Lankhorst wrote:
> Now that connector_mask is reliable there's no need for this
> function any more.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Since this doesn't touch i915 I figured I can merge this one too. So
except for the previous i915 patch it's now all in drm-misc.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c        | 30 ------------------------------
>  drivers/gpu/drm/drm_atomic_helper.c | 10 ++++------
>  drivers/gpu/drm/vc4/vc4_crtc.c      |  2 +-
>  include/drm/drm_atomic.h            |  4 ----
>  4 files changed, 5 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 14b321580517..d3ed12447fbb 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1183,36 +1183,6 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_add_affected_planes);
>  
>  /**
> - * drm_atomic_connectors_for_crtc - count number of connected outputs
> - * @state: atomic state
> - * @crtc: DRM crtc
> - *
> - * This function counts all connectors which will be connected to @crtc
> - * according to @state. Useful to recompute the enable state for @crtc.
> - */
> -int
> -drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
> -			       struct drm_crtc *crtc)
> -{
> -	struct drm_connector *connector;
> -	struct drm_connector_state *conn_state;
> -
> -	int i, num_connected_connectors = 0;
> -
> -	for_each_connector_in_state(state, connector, conn_state, i) {
> -		if (conn_state->crtc == crtc)
> -			num_connected_connectors++;
> -	}
> -
> -	DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d:%s]\n",
> -			 state, num_connected_connectors,
> -			 crtc->base.id, crtc->name);
> -
> -	return num_connected_connectors;
> -}
> -EXPORT_SYMBOL(drm_atomic_connectors_for_crtc);
> -
> -/**
>   * drm_atomic_legacy_backoff - locking backoff for legacy ioctls
>   * @state: atomic state
>   *
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 27dd68f946e6..13b771cb0d35 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -464,7 +464,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  	 * crtc only changed its mode but has the same set of connectors.
>  	 */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		int num_connectors;
> +		bool has_connectors =
> +			!!crtc_state->connector_mask;
>  
>  		/*
>  		 * We must set ->active_changed after walking connectors for
> @@ -493,10 +494,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		if (ret != 0)
>  			return ret;
>  
> -		num_connectors = drm_atomic_connectors_for_crtc(state,
> -								crtc);
> -
> -		if (crtc_state->enable != !!num_connectors) {
> +		if (crtc_state->enable != has_connectors) {
>  			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n",
>  					 crtc->base.id, crtc->name);
>  
> @@ -1755,7 +1753,7 @@ static int update_output_state(struct drm_atomic_state *state,
>  		if (crtc == set->crtc)
>  			continue;
>  
> -		if (!drm_atomic_connectors_for_crtc(state, crtc)) {
> +		if (!crtc_state->connector_mask) {
>  			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
>  								NULL);
>  			if (ret < 0)
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 2168a99d59aa..aa7ed24a41c7 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -327,7 +327,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
>  	/* The pixelvalve can only feed one encoder (and encoders are
>  	 * 1:1 with connectors.)
>  	 */
> -	if (drm_atomic_connectors_for_crtc(state->state, crtc) > 1)
> +	if (hweight32(state->connector_mask) > 1)
>  		return -EINVAL;
>  
>  	drm_atomic_crtc_state_for_each_plane(plane, state) {
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d8576ac55693..d3eaa5df187a 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -130,10 +130,6 @@ int __must_check
>  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>  			       struct drm_crtc *crtc);
>  
> -int
> -drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
> -			       struct drm_crtc *crtc);
> -
>  void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
>  
>  void
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on c1e9dc2dcb577438a6350c7f1cb36ba8ad0e1dfd drm-intel-nightly: 2016y-01m-04d-09h-35m-16s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (skl-i5k-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (bsw-nuc-2)
                dmesg-warn -> PASS       (hsw-brixbox)
        Subgroup basic-flip-vs-wf_vblank:
                dmesg-warn -> PASS       (snb-x220t)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (snb-x220t)
        Subgroup read-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (skl-i7k-2)

bdw-nuci7        total:132  pass:122  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:113  dwarn:2   dfail:0   fail:0   skip:20 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
hsw-xps12        total:132  pass:125  dwarn:3   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:100  dwarn:0   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:125  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:122  dwarn:1   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_1067/

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