[PATCH v2 01/20] drm/atomic: add connectors_changed to separate it from mode_changed

[PATCH v2 01/20] drm/atomic: add connectors_changed to separate it from mode_changed

[Maarten Lankhorst]

This can be a separate case from mode_changed, when connectors stay the
same but only the mode is different. Drivers may choose to implement specific
optimizations to prevent a full modeset for this case.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c  | 25 +++++++++++++++++++------
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 include/drm/drm_atomic.h             |  3 ++-
 include/drm/drm_crtc.h               |  8 +++++---
 4 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 66e76f4f43be..b818e3111380 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -124,7 +124,7 @@ steal_encoder(struct drm_atomic_state *state,
 	if (IS_ERR(crtc_state))
 		return PTR_ERR(crtc_state);
 
-	crtc_state->mode_changed = true;
+	crtc_state->connectors_changed = true;
 
 	list_for_each_entry(connector, &config->connector_list, head) {
 		if (connector->state->best_encoder != encoder)
@@ -174,14 +174,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 			idx = drm_crtc_index(connector->state->crtc);
 
 			crtc_state = state->crtc_states[idx];
-			crtc_state->mode_changed = true;
+			crtc_state->connectors_changed = true;
 		}
 
 		if (connector_state->crtc) {
 			idx = drm_crtc_index(connector_state->crtc);
 
 			crtc_state = state->crtc_states[idx];
-			crtc_state->mode_changed = true;
+			crtc_state->connectors_changed = true;
 		}
 	}
 
@@ -233,7 +233,7 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 	idx = drm_crtc_index(connector_state->crtc);
 
 	crtc_state = state->crtc_states[idx];
-	crtc_state->mode_changed = true;
+	crtc_state->connectors_changed = true;
 
 	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
 			 connector->base.id,
@@ -256,7 +256,8 @@ mode_fixup(struct drm_atomic_state *state)
 	bool ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc_state->mode_changed)
+		if (!crtc_state->mode_changed &&
+		    !crtc_state->connectors_changed)
 			continue;
 
 		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
@@ -312,7 +313,8 @@ mode_fixup(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
-		if (!crtc_state->mode_changed)
+		if (!crtc_state->mode_changed &&
+		    !crtc_state->connectors_changed)
 			continue;
 
 		funcs = crtc->helper_private;
@@ -373,7 +375,17 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		if (crtc->state->enable != crtc_state->enable) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
 					 crtc->base.id);
+
+			/*
+			 * For clarity this assignment is done here, but
+			 * enable == 0 is only true when there are no
+			 * connectors and a NULL mode.
+			 *
+			 * The other way around is true as well. enable != 0
+			 * iff connectors are attached and a mode is set.
+			 */
 			crtc_state->mode_changed = true;
+			crtc_state->connectors_changed = true;
 		}
 	}
 
@@ -2072,6 +2084,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	state->mode_changed = false;
 	state->active_changed = false;
 	state->planes_changed = false;
+	state->connectors_changed = false;
 	state->event = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e91613b1c871..6ddb462b4124 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -421,7 +421,7 @@ static const intel_limit_t intel_limits_bxt = {
 static bool
 needs_modeset(struct drm_crtc_state *state)
 {
-	return state->mode_changed || state->active_changed;
+	return drm_atomic_crtc_needs_modeset(state);
 }
 
 /**
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 8a3a913320eb..e67aeac2aee0 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -166,7 +166,8 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
 static inline bool
 drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
 {
-	return state->mode_changed || state->active_changed;
+	return state->mode_changed || state->active_changed ||
+	       state->connectors_changed;
 }
 
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 57ca8cc383a6..e39d6f1de5f7 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -255,12 +255,13 @@ struct drm_atomic_state;
  * @crtc: backpointer to the CRTC
  * @enable: whether the CRTC should be enabled, gates all other state
  * @active: whether the CRTC is actively displaying (used for DPMS)
- * @mode_changed: for use by helpers and drivers when computing state updates
- * @active_changed: for use by helpers and drivers when computing state updates
+ * @planes_changed: planes on this crtc are updated
+ * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
+ * @active_changed: crtc_state->active
+ * @connectors_changed: connectors to this crtc have been updated
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @last_vblank_count: for helpers and drivers to capture the vblank of the
  * 	update to ensure framebuffer cleanup isn't done too early
- * @planes_changed: for use by helpers and drivers when computing state updates
  * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
  * @mode: current mode timings
  * @event: optional pointer to a DRM event to signal upon completion of the
@@ -283,6 +284,7 @@ struct drm_crtc_state {
 	bool planes_changed : 1;
 	bool mode_changed : 1;
 	bool active_changed : 1;
+	bool connectors_changed : 1;
 
 	/* attached planes bitmask:
 	 * WARNING: transitional helpers do not maintain plane_mask so
-- 
2.1.0

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

Re: [PATCH v2 01/20] drm/atomic: add connectors_changed to separate it from mode_changed

[Daniel Vetter]

On Tue, Jul 07, 2015 at 09:08:12AM +0200, Maarten Lankhorst wrote:
> This can be a separate case from mode_changed, when connectors stay the
> same but only the mode is different. Drivers may choose to implement specific
> optimizations to prevent a full modeset for this case.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c  | 25 +++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  include/drm/drm_atomic.h             |  3 ++-
>  include/drm/drm_crtc.h               |  8 +++++---
>  4 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 66e76f4f43be..b818e3111380 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -124,7 +124,7 @@ steal_encoder(struct drm_atomic_state *state,
>  	if (IS_ERR(crtc_state))
>  		return PTR_ERR(crtc_state);
>  
> -	crtc_state->mode_changed = true;
> +	crtc_state->connectors_changed = true;
>  
>  	list_for_each_entry(connector, &config->connector_list, head) {
>  		if (connector->state->best_encoder != encoder)
> @@ -174,14 +174,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>  			idx = drm_crtc_index(connector->state->crtc);
>  
>  			crtc_state = state->crtc_states[idx];
> -			crtc_state->mode_changed = true;
> +			crtc_state->connectors_changed = true;
>  		}
>  
>  		if (connector_state->crtc) {
>  			idx = drm_crtc_index(connector_state->crtc);
>  
>  			crtc_state = state->crtc_states[idx];
> -			crtc_state->mode_changed = true;
> +			crtc_state->connectors_changed = true;
>  		}
>  	}
>  
> @@ -233,7 +233,7 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>  	idx = drm_crtc_index(connector_state->crtc);
>  
>  	crtc_state = state->crtc_states[idx];
> -	crtc_state->mode_changed = true;
> +	crtc_state->connectors_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
>  			 connector->base.id,
> @@ -256,7 +256,8 @@ mode_fixup(struct drm_atomic_state *state)
>  	bool ret;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!crtc_state->mode_changed)
> +		if (!crtc_state->mode_changed &&
> +		    !crtc_state->connectors_changed)
>  			continue;
>  
>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
> @@ -312,7 +313,8 @@ mode_fixup(struct drm_atomic_state *state)
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		const struct drm_crtc_helper_funcs *funcs;
>  
> -		if (!crtc_state->mode_changed)
> +		if (!crtc_state->mode_changed &&
> +		    !crtc_state->connectors_changed)
>  			continue;
>  
>  		funcs = crtc->helper_private;
> @@ -373,7 +375,17 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		if (crtc->state->enable != crtc_state->enable) {
>  			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
>  					 crtc->base.id);
> +
> +			/*
> +			 * For clarity this assignment is done here, but
> +			 * enable == 0 is only true when there are no
> +			 * connectors and a NULL mode.
> +			 *
> +			 * The other way around is true as well. enable != 0
> +			 * iff connectors are attached and a mode is set.
> +			 */
>  			crtc_state->mode_changed = true;

I'd drop this one so that connectors_changed and mode_changed are truly
orthogonal. Also ->enable implies connectors changed since we do check
that there's only connected connectors if the crtc is on. Needs kerneldoc
update too ofc.

> +			crtc_state->connectors_changed = true;
>  		}
>  	}
>  
> @@ -2072,6 +2084,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  	state->mode_changed = false;
>  	state->active_changed = false;
>  	state->planes_changed = false;
> +	state->connectors_changed = false;
>  	state->event = NULL;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e91613b1c871..6ddb462b4124 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -421,7 +421,7 @@ static const intel_limit_t intel_limits_bxt = {
>  static bool
>  needs_modeset(struct drm_crtc_state *state)
>  {
> -	return state->mode_changed || state->active_changed;
> +	return drm_atomic_crtc_needs_modeset(state);
>  }
>  
>  /**
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 8a3a913320eb..e67aeac2aee0 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -166,7 +166,8 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>  static inline bool
>  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
>  {
> -	return state->mode_changed || state->active_changed;
> +	return state->mode_changed || state->active_changed ||
> +	       state->connectors_changed;
>  }
>  
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 57ca8cc383a6..e39d6f1de5f7 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -255,12 +255,13 @@ struct drm_atomic_state;
>   * @crtc: backpointer to the CRTC
>   * @enable: whether the CRTC should be enabled, gates all other state
>   * @active: whether the CRTC is actively displaying (used for DPMS)
> - * @mode_changed: for use by helpers and drivers when computing state updates
> - * @active_changed: for use by helpers and drivers when computing state updates
> + * @planes_changed: planes on this crtc are updated
> + * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
> + * @active_changed: crtc_state->active

missing a "has changed".

> + * @connectors_changed: connectors to this crtc have been updated
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   * 	update to ensure framebuffer cleanup isn't done too early
> - * @planes_changed: for use by helpers and drivers when computing state updates
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
>   * @event: optional pointer to a DRM event to signal upon completion of the
> @@ -283,6 +284,7 @@ struct drm_crtc_state {
>  	bool planes_changed : 1;
>  	bool mode_changed : 1;
>  	bool active_changed : 1;
> +	bool connectors_changed : 1;
>  
>  	/* attached planes bitmask:
>  	 * WARNING: transitional helpers do not maintain plane_mask so
> -- 
> 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

Re: [PATCH v2 01/20] drm/atomic: add connectors_changed to separate it from mode_changed

[Maarten Lankhorst]

Op 07-07-15 om 10:59 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 09:08:12AM +0200, Maarten Lankhorst wrote:
>> This can be a separate case from mode_changed, when connectors stay the
>> same but only the mode is different. Drivers may choose to implement specific
>> optimizations to prevent a full modeset for this case.
>>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c  | 25 +++++++++++++++++++------
>>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>>  include/drm/drm_atomic.h             |  3 ++-
>>  include/drm/drm_crtc.h               |  8 +++++---
>>  4 files changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 66e76f4f43be..b818e3111380 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -124,7 +124,7 @@ steal_encoder(struct drm_atomic_state *state,
>>  	if (IS_ERR(crtc_state))
>>  		return PTR_ERR(crtc_state);
>>  
>> -	crtc_state->mode_changed = true;
>> +	crtc_state->connectors_changed = true;
>>  
>>  	list_for_each_entry(connector, &config->connector_list, head) {
>>  		if (connector->state->best_encoder != encoder)
>> @@ -174,14 +174,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>>  			idx = drm_crtc_index(connector->state->crtc);
>>  
>>  			crtc_state = state->crtc_states[idx];
>> -			crtc_state->mode_changed = true;
>> +			crtc_state->connectors_changed = true;
>>  		}
>>  
>>  		if (connector_state->crtc) {
>>  			idx = drm_crtc_index(connector_state->crtc);
>>  
>>  			crtc_state = state->crtc_states[idx];
>> -			crtc_state->mode_changed = true;
>> +			crtc_state->connectors_changed = true;
>>  		}
>>  	}
>>  
>> @@ -233,7 +233,7 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
>>  	idx = drm_crtc_index(connector_state->crtc);
>>  
>>  	crtc_state = state->crtc_states[idx];
>> -	crtc_state->mode_changed = true;
>> +	crtc_state->connectors_changed = true;
>>  
>>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
>>  			 connector->base.id,
>> @@ -256,7 +256,8 @@ mode_fixup(struct drm_atomic_state *state)
>>  	bool ret;
>>  
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (!crtc_state->mode_changed)
>> +		if (!crtc_state->mode_changed &&
>> +		    !crtc_state->connectors_changed)
>>  			continue;
>>  
>>  		drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
>> @@ -312,7 +313,8 @@ mode_fixup(struct drm_atomic_state *state)
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>  		const struct drm_crtc_helper_funcs *funcs;
>>  
>> -		if (!crtc_state->mode_changed)
>> +		if (!crtc_state->mode_changed &&
>> +		    !crtc_state->connectors_changed)
>>  			continue;
>>  
>>  		funcs = crtc->helper_private;
>> @@ -373,7 +375,17 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>>  		if (crtc->state->enable != crtc_state->enable) {
>>  			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
>>  					 crtc->base.id);
>> +
>> +			/*
>> +			 * For clarity this assignment is done here, but
>> +			 * enable == 0 is only true when there are no
>> +			 * connectors and a NULL mode.
>> +			 *
>> +			 * The other way around is true as well. enable != 0
>> +			 * iff connectors are attached and a mode is set.
>> +			 */
>>  			crtc_state->mode_changed = true;
> I'd drop this one so that connectors_changed and mode_changed are truly
> orthogonal. Also ->enable implies connectors changed since we do check
> that there's only connected connectors if the crtc is on. Needs kerneldoc
> update too ofc.

They are orthogonal, think of this case:

1. crtc previously enabled, connector removed, mode stays same ->
	connector_changed = true, mode_changed = false

2. crtc previously enabled, connectors stay the same, different mode -> 
	connectors_changed = false, mode_changed = true

The following is enforced by the checks:
crtc disabled, implies 0 connectors, no mode.
crtc enabled implies > 0 connectors, and a mode.

Hence the connectors_changed and mode_changed here are for documentation purposes only. :)

So if someone wonders what happens when enable is changed they don't have to dig through
the entire drm_atomic_helper_check_modeset function and still not be sure if it's coincidence
or not. 

You're right about the kerneldoc, I'll fix it.

>> +			crtc_state->connectors_changed = true;
>>  		}
>>  	}
>>  
>> @@ -2072,6 +2084,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>>  	state->mode_changed = false;
>>  	state->active_changed = false;
>>  	state->planes_changed = false;
>> +	state->connectors_changed = false;
>>  	state->event = NULL;
>>  }
>>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e91613b1c871..6ddb462b4124 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -421,7 +421,7 @@ static const intel_limit_t intel_limits_bxt = {
>>  static bool
>>  needs_modeset(struct drm_crtc_state *state)
>>  {
>> -	return state->mode_changed || state->active_changed;
>> +	return drm_atomic_crtc_needs_modeset(state);
>>  }
>>  
>>  /**
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 8a3a913320eb..e67aeac2aee0 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -166,7 +166,8 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>>  static inline bool
>>  drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
>>  {
>> -	return state->mode_changed || state->active_changed;
>> +	return state->mode_changed || state->active_changed ||
>> +	       state->connectors_changed;
>>  }
>>  
>>  
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 57ca8cc383a6..e39d6f1de5f7 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -255,12 +255,13 @@ struct drm_atomic_state;
>>   * @crtc: backpointer to the CRTC
>>   * @enable: whether the CRTC should be enabled, gates all other state
>>   * @active: whether the CRTC is actively displaying (used for DPMS)
>> - * @mode_changed: for use by helpers and drivers when computing state updates
>> - * @active_changed: for use by helpers and drivers when computing state updates
>> + * @planes_changed: planes on this crtc are updated
>> + * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
>> + * @active_changed: crtc_state->active
> missing a "has changed".
Oops, indeed!

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

Re: [PATCH v2 01/20] drm/atomic: add connectors_changed to separate it from mode_changed

[Daniel Vetter]

On Tue, Jul 07, 2015 at 12:05:40PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 10:59 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 09:08:12AM +0200, Maarten Lankhorst wrote:
> >> @@ -373,7 +375,17 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >>  		if (crtc->state->enable != crtc_state->enable) {
> >>  			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
> >>  					 crtc->base.id);
> >> +
> >> +			/*
> >> +			 * For clarity this assignment is done here, but
> >> +			 * enable == 0 is only true when there are no
> >> +			 * connectors and a NULL mode.
> >> +			 *
> >> +			 * The other way around is true as well. enable != 0
> >> +			 * iff connectors are attached and a mode is set.
> >> +			 */
> >>  			crtc_state->mode_changed = true;
> > I'd drop this one so that connectors_changed and mode_changed are truly
> > orthogonal. Also ->enable implies connectors changed since we do check
> > that there's only connected connectors if the crtc is on. Needs kerneldoc
> > update too ofc.
> 
> They are orthogonal, think of this case:
> 
> 1. crtc previously enabled, connector removed, mode stays same ->
> 	connector_changed = true, mode_changed = false
> 
> 2. crtc previously enabled, connectors stay the same, different mode -> 
> 	connectors_changed = false, mode_changed = true
> 
> The following is enforced by the checks:
> crtc disabled, implies 0 connectors, no mode.
> crtc enabled implies > 0 connectors, and a mode.
> 
> Hence the connectors_changed and mode_changed here are for documentation purposes only. :)
> 
> So if someone wonders what happens when enable is changed they don't have to dig through
> the entire drm_atomic_helper_check_modeset function and still not be sure if it's coincidence
> or not. 
> 
> You're right about the kerneldoc, I'll fix it.

Right, I guess I should have read your comment properly and disregarded
the kerneldoc ;-)
-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

[PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

This allows the first atomic call during hw init to be a real modeset,
which is useful for forcing a recalculation.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index cac422916c7a..33b5e4ecaf46 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -322,10 +322,12 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 	drm_warn_on_modeset_not_all_locked(dev);
 
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-		if (plane->type != DRM_PLANE_TYPE_PRIMARY)
+		if (plane->type != DRM_PLANE_TYPE_PRIMARY &&
+		    (!plane->state || plane->state->fb))
 			drm_plane_force_disable(plane);
 
-		if (dev->mode_config.rotation_property) {
+		if (dev->mode_config.rotation_property &&
+		    (!plane->state || plane->state->rotation != BIT(DRM_ROTATE_0))) {
 			drm_mode_plane_set_obj_prop(plane,
 						    dev->mode_config.rotation_property,
 						    BIT(DRM_ROTATE_0));
-- 
2.1.0

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

Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> This allows the first atomic call during hw init to be a real modeset,
> which is useful for forcing a recalculation.

fbcon is optional, you can't rely on anything being done in any specific
way. What exactly do you need this for, what's the implications?
-Daniel

> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index cac422916c7a..33b5e4ecaf46 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -322,10 +322,12 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  	drm_warn_on_modeset_not_all_locked(dev);
>  
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> -		if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> +		if (plane->type != DRM_PLANE_TYPE_PRIMARY &&
> +		    (!plane->state || plane->state->fb))
>  			drm_plane_force_disable(plane);
>  
> -		if (dev->mode_config.rotation_property) {
> +		if (dev->mode_config.rotation_property &&
> +		    (!plane->state || plane->state->rotation != BIT(DRM_ROTATE_0))) {
>  			drm_mode_plane_set_obj_prop(plane,
>  						    dev->mode_config.rotation_property,
>  						    BIT(DRM_ROTATE_0));
> -- 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 07-07-15 om 11:18 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>> This allows the first atomic call during hw init to be a real modeset,
>> which is useful for forcing a recalculation.
> fbcon is optional, you can't rely on anything being done in any specific
> way. What exactly do you need this for, what's the implications?
In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
I want the first function to be the modeset, so we have a sane base to commit changes on.
Ideally this whole function would have a atomic counterpart which does it in one go. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >> This allows the first atomic call during hw init to be a real modeset,
> >> which is useful for forcing a recalculation.
> > fbcon is optional, you can't rely on anything being done in any specific
> > way. What exactly do you need this for, what's the implications?
> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> I want the first function to be the modeset, so we have a sane base to commit changes on.
> Ideally this whole function would have a atomic counterpart which does it in one go. :)

Yeah. Otoh as soon as we have atomic modeset working we can replace all
the legacy entry points with atomic helpers, and then even plane_disable
will be a full atomic modeset.

What did fall apart with just touching properties/planes now?
-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: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 07-07-15 om 14:10 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>> This allows the first atomic call during hw init to be a real modeset,
>>>> which is useful for forcing a recalculation.
>>> fbcon is optional, you can't rely on anything being done in any specific
>>> way. What exactly do you need this for, what's the implications?
>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> Yeah. Otoh as soon as we have atomic modeset working we can replace all
> the legacy entry points with atomic helpers, and then even plane_disable
> will be a full atomic modeset.
>
> What did fall apart with just touching properties/planes now?
Setting rotation on the primary plane caused it to be disabled because
the src and dst rectangle are not set up yet until the modeset. So the
check function saw that the plane should be invisible and performs the
update.

It's also an extra vblank wait when the primary plane is visible.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Tue, Jul 07, 2015 at 04:32:44PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 14:10 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> >> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> >>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >>>> This allows the first atomic call during hw init to be a real modeset,
> >>>> which is useful for forcing a recalculation.
> >>> fbcon is optional, you can't rely on anything being done in any specific
> >>> way. What exactly do you need this for, what's the implications?
> >> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> >> I want the first function to be the modeset, so we have a sane base to commit changes on.
> >> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> > Yeah. Otoh as soon as we have atomic modeset working we can replace all
> > the legacy entry points with atomic helpers, and then even plane_disable
> > will be a full atomic modeset.
> >
> > What did fall apart with just touching properties/planes now?
> Setting rotation on the primary plane caused it to be disabled because
> the src and dst rectangle are not set up yet until the modeset. So the
> check function saw that the plane should be invisible and performs the
> update.

Sounds like a bug - we need to recreate more of the primary plane state.
Or maybe we need to call the atomic_check function for the primary plane
to compute all that derived state. But we really can't rely upon userspace
to do a modeset first, e.g. X at start (if there's no fbcon) loves to read
and then write back all the properties (or at least did).

We really need to handle this in the backend properly.

> It's also an extra vblank wait when the primary plane is visible.

Another bug, no-op changes with the same fb should not result in a vblank
wait. At least not when using the helpers (or if there is a case I need to
copypaste the fix again).
-Daniel
-- 
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 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 07-07-15 om 14:10 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>> This allows the first atomic call during hw init to be a real modeset,
>>>> which is useful for forcing a recalculation.
>>> fbcon is optional, you can't rely on anything being done in any specific
>>> way. What exactly do you need this for, what's the implications?
>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> Yeah. Otoh as soon as we have atomic modeset working we can replace all
> the legacy entry points with atomic helpers, and then even plane_disable
> will be a full atomic modeset.
>
> What did fall apart with just touching properties/planes now?
Also when i915 is fully atomic it calculates in intel_modeset_compute_config
if a modeset is needed after the first atomic call. Right now because
intel_modeset_compute_config is only called in set_config so this works as expected.
Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
and if the final mode is different this will introduce a double modeset.

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

Re: [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 14:10 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> >> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> >>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >>>> This allows the first atomic call during hw init to be a real modeset,
> >>>> which is useful for forcing a recalculation.
> >>> fbcon is optional, you can't rely on anything being done in any specific
> >>> way. What exactly do you need this for, what's the implications?
> >> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> >> I want the first function to be the modeset, so we have a sane base to commit changes on.
> >> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> > Yeah. Otoh as soon as we have atomic modeset working we can replace all
> > the legacy entry points with atomic helpers, and then even plane_disable
> > will be a full atomic modeset.
> >
> > What did fall apart with just touching properties/planes now?
> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
> if a modeset is needed after the first atomic call. Right now because
> intel_modeset_compute_config is only called in set_config so this works as expected.
> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
> and if the final mode is different this will introduce a double modeset.

For expensive properties (i.e. a no-op changes causes something that takes
time like modeset or vblank wait) we need to make sure we filter them out
in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
the existing legacy set_prop functions should all filter out no-op changes
themselves. If we don't do that for rotation then that's a bug.

Same for disabling planes harder, that shouldn't take time. Especially
since fbcon only force-disable non-primary plane, and for driver load
that's the exact thing we already do in the driver anyway.
-Daniel
-- 
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: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 07-07-15 om 18:43 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>>>> This allows the first atomic call during hw init to be a real modeset,
>>>>>> which is useful for forcing a recalculation.
>>>>> fbcon is optional, you can't rely on anything being done in any specific
>>>>> way. What exactly do you need this for, what's the implications?
>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
>>> the legacy entry points with atomic helpers, and then even plane_disable
>>> will be a full atomic modeset.
>>>
>>> What did fall apart with just touching properties/planes now?
>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
>> if a modeset is needed after the first atomic call. Right now because
>> intel_modeset_compute_config is only called in set_config so this works as expected.
>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
>> and if the final mode is different this will introduce a double modeset.
> For expensive properties (i.e. a no-op changes causes something that takes
> time like modeset or vblank wait) we need to make sure we filter them out
> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
> the existing legacy set_prop functions should all filter out no-op changes
> themselves. If we don't do that for rotation then that's a bug.
>
> Same for disabling planes harder, that shouldn't take time. Especially
> since fbcon only force-disable non-primary plane, and for driver load
> that's the exact thing we already do in the driver anyway.

Something like this?
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a1d4e13f3908..2989232f4996 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include "drm_crtc_internal.h"
 #include <linux/fence.h>
 
 /**
@@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
 {
 	struct drm_atomic_state *state;
 	struct drm_crtc_state *crtc_state;
-	int ret = 0;
+	uint64_t retval;
+	int ret;
+
+	ret = drm_atomic_get_property(&crtc->base, property, &retval);
+	if (!ret && val == retval)
+		return 0;
 
 	state = drm_atomic_state_alloc(crtc->dev);
 	if (!state)
@@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
 {
 	struct drm_atomic_state *state;
 	struct drm_plane_state *plane_state;
-	int ret = 0;
+	uint64_t retval;
+	int ret;
+
+	ret = drm_atomic_get_property(&plane->base, property, &retval);
+	if (!ret && val == retval)
+		return 0;
 
 	state = drm_atomic_state_alloc(plane->dev);
 	if (!state)
@@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
 {
 	struct drm_atomic_state *state;
 	struct drm_connector_state *connector_state;
-	int ret = 0;
+	uint64_t retval;
+	int ret;
+
+	ret = drm_atomic_get_property(&connector->base, property, &retval);
+	if (!ret && val == retval)
+		return 0;
 
 	state = drm_atomic_state_alloc(connector->dev);
 	if (!state)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 424c83323aaa..5bab7bff8a15 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1327,7 +1327,8 @@ void drm_plane_force_disable(struct drm_plane *plane)
 {
 	int ret;
 
-	if (!plane->fb)
+	if ((plane->state && !plane->state->fb) ||
+	    (!plane->state && !plane->fb))
 		return;
 
 	plane->old_fb = plane->fb;

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

Re: [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 18:43 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
> >> Op 07-07-15 om 14:10 schreef Daniel Vetter:
> >>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> >>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> >>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >>>>>> This allows the first atomic call during hw init to be a real modeset,
> >>>>>> which is useful for forcing a recalculation.
> >>>>> fbcon is optional, you can't rely on anything being done in any specific
> >>>>> way. What exactly do you need this for, what's the implications?
> >>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> >>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
> >>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> >>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
> >>> the legacy entry points with atomic helpers, and then even plane_disable
> >>> will be a full atomic modeset.
> >>>
> >>> What did fall apart with just touching properties/planes now?
> >> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
> >> if a modeset is needed after the first atomic call. Right now because
> >> intel_modeset_compute_config is only called in set_config so this works as expected.
> >> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
> >> and if the final mode is different this will introduce a double modeset.
> > For expensive properties (i.e. a no-op changes causes something that takes
> > time like modeset or vblank wait) we need to make sure we filter them out
> > in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
> > the existing legacy set_prop functions should all filter out no-op changes
> > themselves. If we don't do that for rotation then that's a bug.
> >
> > Same for disabling planes harder, that shouldn't take time. Especially
> > since fbcon only force-disable non-primary plane, and for driver load
> > that's the exact thing we already do in the driver anyway.
> 
> Something like this?
> ---
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a1d4e13f3908..2989232f4996 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic_helper.h>
> +#include "drm_crtc_internal.h"
>  #include <linux/fence.h>
>  
>  /**
> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>  {
>  	struct drm_atomic_state *state;
>  	struct drm_crtc_state *crtc_state;
> -	int ret = 0;
> +	uint64_t retval;
> +	int ret;
> +
> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
> +	if (!ret && val == retval)
> +		return 0;
>  
>  	state = drm_atomic_state_alloc(crtc->dev);
>  	if (!state)
> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>  {
>  	struct drm_atomic_state *state;
>  	struct drm_plane_state *plane_state;
> -	int ret = 0;
> +	uint64_t retval;
> +	int ret;
> +
> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
> +	if (!ret && val == retval)
> +		return 0;
>  
>  	state = drm_atomic_state_alloc(plane->dev);
>  	if (!state)
> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>  {
>  	struct drm_atomic_state *state;
>  	struct drm_connector_state *connector_state;
> -	int ret = 0;
> +	uint64_t retval;
> +	int ret;
> +
> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
> +	if (!ret && val == retval)
> +		return 0;
>  
>  	state = drm_atomic_state_alloc(connector->dev);
>  	if (!state)

The reason I didn't do this is that a prop change might still result in no
hw state change (e.g. if you go automitic->explicit setting matching
automatic one). Hence I think we need to solve this in lower levels
anyway, i.e. in when computing the config. But it shouldn't cause trouble
yet.

> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 424c83323aaa..5bab7bff8a15 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1327,7 +1327,8 @@ void drm_plane_force_disable(struct drm_plane *plane)
>  {
>  	int ret;
>  
> -	if (!plane->fb)
> +	if ((plane->state && !plane->state->fb) ||
> +	    (!plane->state && !plane->fb))
>  		return;

Nah, atomic helpers should figure this out imo. Since if userspace does
the same (loop over all planes) then it won't go through force_disable.
-Daniel

>  
>  	plane->old_fb = plane->fb;
> 

-- 
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 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 08-07-15 om 10:55 schreef Daniel Vetter:
> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
>>>>>>>> which is useful for forcing a recalculation.
>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
>>>>>>> way. What exactly do you need this for, what's the implications?
>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
>>>>> the legacy entry points with atomic helpers, and then even plane_disable
>>>>> will be a full atomic modeset.
>>>>>
>>>>> What did fall apart with just touching properties/planes now?
>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
>>>> if a modeset is needed after the first atomic call. Right now because
>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
>>>> and if the final mode is different this will introduce a double modeset.
>>> For expensive properties (i.e. a no-op changes causes something that takes
>>> time like modeset or vblank wait) we need to make sure we filter them out
>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
>>> the existing legacy set_prop functions should all filter out no-op changes
>>> themselves. If we don't do that for rotation then that's a bug.
>>>
>>> Same for disabling planes harder, that shouldn't take time. Especially
>>> since fbcon only force-disable non-primary plane, and for driver load
>>> that's the exact thing we already do in the driver anyway.
>> Something like this?
>> ---
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index a1d4e13f3908..2989232f4996 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -30,6 +30,7 @@
>>  #include <drm/drm_plane_helper.h>
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_atomic_helper.h>
>> +#include "drm_crtc_internal.h"
>>  #include <linux/fence.h>
>>  
>>  /**
>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>>  {
>>  	struct drm_atomic_state *state;
>>  	struct drm_crtc_state *crtc_state;
>> -	int ret = 0;
>> +	uint64_t retval;
>> +	int ret;
>> +
>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
>> +	if (!ret && val == retval)
>> +		return 0;
>>  
>>  	state = drm_atomic_state_alloc(crtc->dev);
>>  	if (!state)
>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>>  {
>>  	struct drm_atomic_state *state;
>>  	struct drm_plane_state *plane_state;
>> -	int ret = 0;
>> +	uint64_t retval;
>> +	int ret;
>> +
>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
>> +	if (!ret && val == retval)
>> +		return 0;
>>  
>>  	state = drm_atomic_state_alloc(plane->dev);
>>  	if (!state)
>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>>  {
>>  	struct drm_atomic_state *state;
>>  	struct drm_connector_state *connector_state;
>> -	int ret = 0;
>> +	uint64_t retval;
>> +	int ret;
>> +
>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
>> +	if (!ret && val == retval)
>> +		return 0;
>>  
>>  	state = drm_atomic_state_alloc(connector->dev);
>>  	if (!state)
> The reason I didn't do this is that a prop change might still result in no
> hw state change (e.g. if you go automitic->explicit setting matching
> automatic one). Hence I think we need to solve this in lower levels
> anyway, i.e. in when computing the config. But it shouldn't cause trouble
> yet.
Is that a ack or nack?
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 424c83323aaa..5bab7bff8a15 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -1327,7 +1327,8 @@ void drm_plane_force_disable(struct drm_plane *plane)
>>  {
>>  	int ret;
>>  
>> -	if (!plane->fb)
>> +	if ((plane->state && !plane->state->fb) ||
>> +	    (!plane->state && !plane->fb))
>>  		return;
> Nah, atomic helpers should figure this out imo. Since if userspace does
> the same (loop over all planes) then it won't go through force_disable.
> -Daniel
>
>>  
>>  	plane->old_fb = plane->fb;
>>

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

Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
> Op 08-07-15 om 10:55 schreef Daniel Vetter:
> > On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
> >> Op 07-07-15 om 18:43 schreef Daniel Vetter:
> >>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
> >>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
> >>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> >>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >>>>>>>> This allows the first atomic call during hw init to be a real modeset,
> >>>>>>>> which is useful for forcing a recalculation.
> >>>>>>> fbcon is optional, you can't rely on anything being done in any specific
> >>>>>>> way. What exactly do you need this for, what's the implications?
> >>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> >>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
> >>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> >>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
> >>>>> the legacy entry points with atomic helpers, and then even plane_disable
> >>>>> will be a full atomic modeset.
> >>>>>
> >>>>> What did fall apart with just touching properties/planes now?
> >>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
> >>>> if a modeset is needed after the first atomic call. Right now because
> >>>> intel_modeset_compute_config is only called in set_config so this works as expected.
> >>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
> >>>> and if the final mode is different this will introduce a double modeset.
> >>> For expensive properties (i.e. a no-op changes causes something that takes
> >>> time like modeset or vblank wait) we need to make sure we filter them out
> >>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
> >>> the existing legacy set_prop functions should all filter out no-op changes
> >>> themselves. If we don't do that for rotation then that's a bug.
> >>>
> >>> Same for disabling planes harder, that shouldn't take time. Especially
> >>> since fbcon only force-disable non-primary plane, and for driver load
> >>> that's the exact thing we already do in the driver anyway.
> >> Something like this?
> >> ---
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index a1d4e13f3908..2989232f4996 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -30,6 +30,7 @@
> >>  #include <drm/drm_plane_helper.h>
> >>  #include <drm/drm_crtc_helper.h>
> >>  #include <drm/drm_atomic_helper.h>
> >> +#include "drm_crtc_internal.h"
> >>  #include <linux/fence.h>
> >>  
> >>  /**
> >> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
> >>  {
> >>  	struct drm_atomic_state *state;
> >>  	struct drm_crtc_state *crtc_state;
> >> -	int ret = 0;
> >> +	uint64_t retval;
> >> +	int ret;
> >> +
> >> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
> >> +	if (!ret && val == retval)
> >> +		return 0;
> >>  
> >>  	state = drm_atomic_state_alloc(crtc->dev);
> >>  	if (!state)
> >> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
> >>  {
> >>  	struct drm_atomic_state *state;
> >>  	struct drm_plane_state *plane_state;
> >> -	int ret = 0;
> >> +	uint64_t retval;
> >> +	int ret;
> >> +
> >> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
> >> +	if (!ret && val == retval)
> >> +		return 0;
> >>  
> >>  	state = drm_atomic_state_alloc(plane->dev);
> >>  	if (!state)
> >> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
> >>  {
> >>  	struct drm_atomic_state *state;
> >>  	struct drm_connector_state *connector_state;
> >> -	int ret = 0;
> >> +	uint64_t retval;
> >> +	int ret;
> >> +
> >> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
> >> +	if (!ret && val == retval)
> >> +		return 0;
> >>  
> >>  	state = drm_atomic_state_alloc(connector->dev);
> >>  	if (!state)
> > The reason I didn't do this is that a prop change might still result in no
> > hw state change (e.g. if you go automitic->explicit setting matching
> > automatic one). Hence I think we need to solve this in lower levels
> > anyway, i.e. in when computing the config. But it shouldn't cause trouble
> > yet.
> Is that a ack or nack?

I think we shouldn't need this really for i915, and it might cover up
bugs. I prefer we just do the evade modeset logic you've implemented once
we switch over to atomic props. Since atm we only have atomic props which
get updated in pageflips we shouldn't have serious problems here yet (for
setting the rotation prop to 0° again when fbdev starts up).

Or do I miss something still here?
-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: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 08-07-15 om 19:52 schreef Daniel Vetter:
> On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
>> Op 08-07-15 om 10:55 schreef Daniel Vetter:
>>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
>>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
>>>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
>>>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
>>>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>>>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
>>>>>>>>>> which is useful for forcing a recalculation.
>>>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
>>>>>>>>> way. What exactly do you need this for, what's the implications?
>>>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>>>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>>>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
>>>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
>>>>>>> the legacy entry points with atomic helpers, and then even plane_disable
>>>>>>> will be a full atomic modeset.
>>>>>>>
>>>>>>> What did fall apart with just touching properties/planes now?
>>>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
>>>>>> if a modeset is needed after the first atomic call. Right now because
>>>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
>>>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
>>>>>> and if the final mode is different this will introduce a double modeset.
>>>>> For expensive properties (i.e. a no-op changes causes something that takes
>>>>> time like modeset or vblank wait) we need to make sure we filter them out
>>>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
>>>>> the existing legacy set_prop functions should all filter out no-op changes
>>>>> themselves. If we don't do that for rotation then that's a bug.
>>>>>
>>>>> Same for disabling planes harder, that shouldn't take time. Especially
>>>>> since fbcon only force-disable non-primary plane, and for driver load
>>>>> that's the exact thing we already do in the driver anyway.
>>>> Something like this?
>>>> ---
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index a1d4e13f3908..2989232f4996 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -30,6 +30,7 @@
>>>>  #include <drm/drm_plane_helper.h>
>>>>  #include <drm/drm_crtc_helper.h>
>>>>  #include <drm/drm_atomic_helper.h>
>>>> +#include "drm_crtc_internal.h"
>>>>  #include <linux/fence.h>
>>>>  
>>>>  /**
>>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>>>>  {
>>>>  	struct drm_atomic_state *state;
>>>>  	struct drm_crtc_state *crtc_state;
>>>> -	int ret = 0;
>>>> +	uint64_t retval;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
>>>> +	if (!ret && val == retval)
>>>> +		return 0;
>>>>  
>>>>  	state = drm_atomic_state_alloc(crtc->dev);
>>>>  	if (!state)
>>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>>>>  {
>>>>  	struct drm_atomic_state *state;
>>>>  	struct drm_plane_state *plane_state;
>>>> -	int ret = 0;
>>>> +	uint64_t retval;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
>>>> +	if (!ret && val == retval)
>>>> +		return 0;
>>>>  
>>>>  	state = drm_atomic_state_alloc(plane->dev);
>>>>  	if (!state)
>>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>>>>  {
>>>>  	struct drm_atomic_state *state;
>>>>  	struct drm_connector_state *connector_state;
>>>> -	int ret = 0;
>>>> +	uint64_t retval;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
>>>> +	if (!ret && val == retval)
>>>> +		return 0;
>>>>  
>>>>  	state = drm_atomic_state_alloc(connector->dev);
>>>>  	if (!state)
>>> The reason I didn't do this is that a prop change might still result in no
>>> hw state change (e.g. if you go automitic->explicit setting matching
>>> automatic one). Hence I think we need to solve this in lower levels
>>> anyway, i.e. in when computing the config. But it shouldn't cause trouble
>>> yet.
>> Is that a ack or nack?
> I think we shouldn't need this really for i915, and it might cover up
> bugs. I prefer we just do the evade modeset logic you've implemented once
> we switch over to atomic props. Since atm we only have atomic props which
> get updated in pageflips we shouldn't have serious problems here yet (for
> setting the rotation prop to 0° again when fbdev starts up).
>
> Or do I miss something still here?
Yes, if the hardware mode is incompatible with its calculated sw mode,
and we set a different mode from fbdev you get 2 modesets instead of 1.

First to make the mode compatible because of the rotate_0, second to set the new mode.

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

Re: [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
> Op 08-07-15 om 19:52 schreef Daniel Vetter:
> > On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
> >> Op 08-07-15 om 10:55 schreef Daniel Vetter:
> >>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
> >>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
> >>>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
> >>>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> >>>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> >>>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
> >>>>>>>>>> which is useful for forcing a recalculation.
> >>>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
> >>>>>>>>> way. What exactly do you need this for, what's the implications?
> >>>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> >>>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
> >>>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> >>>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
> >>>>>>> the legacy entry points with atomic helpers, and then even plane_disable
> >>>>>>> will be a full atomic modeset.
> >>>>>>>
> >>>>>>> What did fall apart with just touching properties/planes now?
> >>>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
> >>>>>> if a modeset is needed after the first atomic call. Right now because
> >>>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
> >>>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
> >>>>>> and if the final mode is different this will introduce a double modeset.
> >>>>> For expensive properties (i.e. a no-op changes causes something that takes
> >>>>> time like modeset or vblank wait) we need to make sure we filter them out
> >>>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
> >>>>> the existing legacy set_prop functions should all filter out no-op changes
> >>>>> themselves. If we don't do that for rotation then that's a bug.
> >>>>>
> >>>>> Same for disabling planes harder, that shouldn't take time. Especially
> >>>>> since fbcon only force-disable non-primary plane, and for driver load
> >>>>> that's the exact thing we already do in the driver anyway.
> >>>> Something like this?
> >>>> ---
> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> index a1d4e13f3908..2989232f4996 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> @@ -30,6 +30,7 @@
> >>>>  #include <drm/drm_plane_helper.h>
> >>>>  #include <drm/drm_crtc_helper.h>
> >>>>  #include <drm/drm_atomic_helper.h>
> >>>> +#include "drm_crtc_internal.h"
> >>>>  #include <linux/fence.h>
> >>>>  
> >>>>  /**
> >>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
> >>>>  {
> >>>>  	struct drm_atomic_state *state;
> >>>>  	struct drm_crtc_state *crtc_state;
> >>>> -	int ret = 0;
> >>>> +	uint64_t retval;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
> >>>> +	if (!ret && val == retval)
> >>>> +		return 0;
> >>>>  
> >>>>  	state = drm_atomic_state_alloc(crtc->dev);
> >>>>  	if (!state)
> >>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
> >>>>  {
> >>>>  	struct drm_atomic_state *state;
> >>>>  	struct drm_plane_state *plane_state;
> >>>> -	int ret = 0;
> >>>> +	uint64_t retval;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
> >>>> +	if (!ret && val == retval)
> >>>> +		return 0;
> >>>>  
> >>>>  	state = drm_atomic_state_alloc(plane->dev);
> >>>>  	if (!state)
> >>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
> >>>>  {
> >>>>  	struct drm_atomic_state *state;
> >>>>  	struct drm_connector_state *connector_state;
> >>>> -	int ret = 0;
> >>>> +	uint64_t retval;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
> >>>> +	if (!ret && val == retval)
> >>>> +		return 0;
> >>>>  
> >>>>  	state = drm_atomic_state_alloc(connector->dev);
> >>>>  	if (!state)
> >>> The reason I didn't do this is that a prop change might still result in no
> >>> hw state change (e.g. if you go automitic->explicit setting matching
> >>> automatic one). Hence I think we need to solve this in lower levels
> >>> anyway, i.e. in when computing the config. But it shouldn't cause trouble
> >>> yet.
> >> Is that a ack or nack?
> > I think we shouldn't need this really for i915, and it might cover up
> > bugs. I prefer we just do the evade modeset logic you've implemented once
> > we switch over to atomic props. Since atm we only have atomic props which
> > get updated in pageflips we shouldn't have serious problems here yet (for
> > setting the rotation prop to 0° again when fbdev starts up).
> >
> > Or do I miss something still here?
> Yes, if the hardware mode is incompatible with its calculated sw mode,
> and we set a different mode from fbdev you get 2 modesets instead of 1.

How does that happen? For setting the rotation property we should just
duplicate the current crtc state. Since there's no mode changing (they
should match perfectly no matter how botched the reconstruction is) there
shouldn't be any need to recompute the config completely and discover that
there's a mismatch. Which means we'll just do the plane update (which
might do a few silly mmios but shouldn't block) and that's it.

At least that's what I'd expect - where does this fall apart?
-Daniel
-- 
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 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 08-07-15 om 22:12 schreef Daniel Vetter:
> On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
>> Op 08-07-15 om 19:52 schreef Daniel Vetter:
>>> On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
>>>> Op 08-07-15 om 10:55 schreef Daniel Vetter:
>>>>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
>>>>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
>>>>>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
>>>>>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
>>>>>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>>>>>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>>>>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>>>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
>>>>>>>>>>>> which is useful for forcing a recalculation.
>>>>>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
>>>>>>>>>>> way. What exactly do you need this for, what's the implications?
>>>>>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>>>>>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>>>>>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
>>>>>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
>>>>>>>>> the legacy entry points with atomic helpers, and then even plane_disable
>>>>>>>>> will be a full atomic modeset.
>>>>>>>>>
>>>>>>>>> What did fall apart with just touching properties/planes now?
>>>>>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
>>>>>>>> if a modeset is needed after the first atomic call. Right now because
>>>>>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
>>>>>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
>>>>>>>> and if the final mode is different this will introduce a double modeset.
>>>>>>> For expensive properties (i.e. a no-op changes causes something that takes
>>>>>>> time like modeset or vblank wait) we need to make sure we filter them out
>>>>>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
>>>>>>> the existing legacy set_prop functions should all filter out no-op changes
>>>>>>> themselves. If we don't do that for rotation then that's a bug.
>>>>>>>
>>>>>>> Same for disabling planes harder, that shouldn't take time. Especially
>>>>>>> since fbcon only force-disable non-primary plane, and for driver load
>>>>>>> that's the exact thing we already do in the driver anyway.
>>>>>> Something like this?
>>>>>> ---
>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> index a1d4e13f3908..2989232f4996 100644
>>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> @@ -30,6 +30,7 @@
>>>>>>  #include <drm/drm_plane_helper.h>
>>>>>>  #include <drm/drm_crtc_helper.h>
>>>>>>  #include <drm/drm_atomic_helper.h>
>>>>>> +#include "drm_crtc_internal.h"
>>>>>>  #include <linux/fence.h>
>>>>>>  
>>>>>>  /**
>>>>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>>>>>>  {
>>>>>>  	struct drm_atomic_state *state;
>>>>>>  	struct drm_crtc_state *crtc_state;
>>>>>> -	int ret = 0;
>>>>>> +	uint64_t retval;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
>>>>>> +	if (!ret && val == retval)
>>>>>> +		return 0;
>>>>>>  
>>>>>>  	state = drm_atomic_state_alloc(crtc->dev);
>>>>>>  	if (!state)
>>>>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>>>>>>  {
>>>>>>  	struct drm_atomic_state *state;
>>>>>>  	struct drm_plane_state *plane_state;
>>>>>> -	int ret = 0;
>>>>>> +	uint64_t retval;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
>>>>>> +	if (!ret && val == retval)
>>>>>> +		return 0;
>>>>>>  
>>>>>>  	state = drm_atomic_state_alloc(plane->dev);
>>>>>>  	if (!state)
>>>>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>>>>>>  {
>>>>>>  	struct drm_atomic_state *state;
>>>>>>  	struct drm_connector_state *connector_state;
>>>>>> -	int ret = 0;
>>>>>> +	uint64_t retval;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
>>>>>> +	if (!ret && val == retval)
>>>>>> +		return 0;
>>>>>>  
>>>>>>  	state = drm_atomic_state_alloc(connector->dev);
>>>>>>  	if (!state)
>>>>> The reason I didn't do this is that a prop change might still result in no
>>>>> hw state change (e.g. if you go automitic->explicit setting matching
>>>>> automatic one). Hence I think we need to solve this in lower levels
>>>>> anyway, i.e. in when computing the config. But it shouldn't cause trouble
>>>>> yet.
>>>> Is that a ack or nack?
>>> I think we shouldn't need this really for i915, and it might cover up
>>> bugs. I prefer we just do the evade modeset logic you've implemented once
>>> we switch over to atomic props. Since atm we only have atomic props which
>>> get updated in pageflips we shouldn't have serious problems here yet (for
>>> setting the rotation prop to 0° again when fbdev starts up).
>>>
>>> Or do I miss something still here?
>> Yes, if the hardware mode is incompatible with its calculated sw mode,
>> and we set a different mode from fbdev you get 2 modesets instead of 1.
> How does that happen? For setting the rotation property we should just
> duplicate the current crtc state. Since there's no mode changing (they
> should match perfectly no matter how botched the reconstruction is) there
> shouldn't be any need to recompute the config completely and discover that
> there's a mismatch. Which means we'll just do the plane update (which
> might do a few silly mmios but shouldn't block) and that's it.
>
> At least that's what I'd expect - where does this fall apart?
If crtc is active and primary fb visible, and converted to atomic:

restore_fbdev_mode() ->
	drm_mode_plane_set_obj_prop() ->
		drm_atomic_helper_plane_set_property() ->
			drm_atomic_get_plane_state() ->
				drm_atomic_get_crtc_state()
crtc state is part of the state, intel_modeset_pipe_config performs
the initial check if modeset's needed. Lets assume yes:

			modeset()

		drm_mode_set_config_internal() ->
			modeset()

Boom double modeset. :(

The alternative solution is making a atomic version of restore_fbdev_mode,
but that would break drivers that are only partially converted to atomic,
like i915 with i915.nuclear_pageflip=true before the convert to atomic commit.

~Maarten

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

Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
> Op 08-07-15 om 22:12 schreef Daniel Vetter:
> > On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
> >> Op 08-07-15 om 19:52 schreef Daniel Vetter:
> >>> On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
> >>>> Op 08-07-15 om 10:55 schreef Daniel Vetter:
> >>>>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
> >>>>>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
> >>>>>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
> >>>>>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> >>>>>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
> >>>>>>>>>>>> which is useful for forcing a recalculation.
> >>>>>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
> >>>>>>>>>>> way. What exactly do you need this for, what's the implications?
> >>>>>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> >>>>>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
> >>>>>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> >>>>>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
> >>>>>>>>> the legacy entry points with atomic helpers, and then even plane_disable
> >>>>>>>>> will be a full atomic modeset.
> >>>>>>>>>
> >>>>>>>>> What did fall apart with just touching properties/planes now?
> >>>>>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
> >>>>>>>> if a modeset is needed after the first atomic call. Right now because
> >>>>>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
> >>>>>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
> >>>>>>>> and if the final mode is different this will introduce a double modeset.
> >>>>>>> For expensive properties (i.e. a no-op changes causes something that takes
> >>>>>>> time like modeset or vblank wait) we need to make sure we filter them out
> >>>>>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
> >>>>>>> the existing legacy set_prop functions should all filter out no-op changes
> >>>>>>> themselves. If we don't do that for rotation then that's a bug.
> >>>>>>>
> >>>>>>> Same for disabling planes harder, that shouldn't take time. Especially
> >>>>>>> since fbcon only force-disable non-primary plane, and for driver load
> >>>>>>> that's the exact thing we already do in the driver anyway.
> >>>>>> Something like this?
> >>>>>> ---
> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> index a1d4e13f3908..2989232f4996 100644
> >>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> @@ -30,6 +30,7 @@
> >>>>>>  #include <drm/drm_plane_helper.h>
> >>>>>>  #include <drm/drm_crtc_helper.h>
> >>>>>>  #include <drm/drm_atomic_helper.h>
> >>>>>> +#include "drm_crtc_internal.h"
> >>>>>>  #include <linux/fence.h>
> >>>>>>  
> >>>>>>  /**
> >>>>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
> >>>>>>  {
> >>>>>>  	struct drm_atomic_state *state;
> >>>>>>  	struct drm_crtc_state *crtc_state;
> >>>>>> -	int ret = 0;
> >>>>>> +	uint64_t retval;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
> >>>>>> +	if (!ret && val == retval)
> >>>>>> +		return 0;
> >>>>>>  
> >>>>>>  	state = drm_atomic_state_alloc(crtc->dev);
> >>>>>>  	if (!state)
> >>>>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
> >>>>>>  {
> >>>>>>  	struct drm_atomic_state *state;
> >>>>>>  	struct drm_plane_state *plane_state;
> >>>>>> -	int ret = 0;
> >>>>>> +	uint64_t retval;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
> >>>>>> +	if (!ret && val == retval)
> >>>>>> +		return 0;
> >>>>>>  
> >>>>>>  	state = drm_atomic_state_alloc(plane->dev);
> >>>>>>  	if (!state)
> >>>>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
> >>>>>>  {
> >>>>>>  	struct drm_atomic_state *state;
> >>>>>>  	struct drm_connector_state *connector_state;
> >>>>>> -	int ret = 0;
> >>>>>> +	uint64_t retval;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
> >>>>>> +	if (!ret && val == retval)
> >>>>>> +		return 0;
> >>>>>>  
> >>>>>>  	state = drm_atomic_state_alloc(connector->dev);
> >>>>>>  	if (!state)
> >>>>> The reason I didn't do this is that a prop change might still result in no
> >>>>> hw state change (e.g. if you go automitic->explicit setting matching
> >>>>> automatic one). Hence I think we need to solve this in lower levels
> >>>>> anyway, i.e. in when computing the config. But it shouldn't cause trouble
> >>>>> yet.
> >>>> Is that a ack or nack?
> >>> I think we shouldn't need this really for i915, and it might cover up
> >>> bugs. I prefer we just do the evade modeset logic you've implemented once
> >>> we switch over to atomic props. Since atm we only have atomic props which
> >>> get updated in pageflips we shouldn't have serious problems here yet (for
> >>> setting the rotation prop to 0° again when fbdev starts up).
> >>>
> >>> Or do I miss something still here?
> >> Yes, if the hardware mode is incompatible with its calculated sw mode,
> >> and we set a different mode from fbdev you get 2 modesets instead of 1.
> > How does that happen? For setting the rotation property we should just
> > duplicate the current crtc state. Since there's no mode changing (they
> > should match perfectly no matter how botched the reconstruction is) there
> > shouldn't be any need to recompute the config completely and discover that
> > there's a mismatch. Which means we'll just do the plane update (which
> > might do a few silly mmios but shouldn't block) and that's it.
> >
> > At least that's what I'd expect - where does this fall apart?
> If crtc is active and primary fb visible, and converted to atomic:
> 
> restore_fbdev_mode() ->
> 	drm_mode_plane_set_obj_prop() ->
> 		drm_atomic_helper_plane_set_property() ->
> 			drm_atomic_get_plane_state() ->
> 				drm_atomic_get_crtc_state()
> crtc state is part of the state, intel_modeset_pipe_config performs
> the initial check if modeset's needed. Lets assume yes:

"Let's assume yes" -> that's imo a bug, so where does this happen so that
we can fix it? Disabling a plane or setting a plane prop really shouldn't
result in a modeset. Well at least if it's not a plane prop that does
required a modeset (but I don't think we have any of those).
-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: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 13-07-15 om 11:13 schreef Daniel Vetter:
> On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
>> Op 08-07-15 om 22:12 schreef Daniel Vetter:
>>> On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
>>>> Op 08-07-15 om 19:52 schreef Daniel Vetter:
>>>>> On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
>>>>>> Op 08-07-15 om 10:55 schreef Daniel Vetter:
>>>>>>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
>>>>>>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
>>>>>>>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
>>>>>>>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
>>>>>>>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>>>>>>>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>>>>>>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>>>>>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
>>>>>>>>>>>>>> which is useful for forcing a recalculation.
>>>>>>>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
>>>>>>>>>>>>> way. What exactly do you need this for, what's the implications?
>>>>>>>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>>>>>>>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>>>>>>>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
>>>>>>>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
>>>>>>>>>>> the legacy entry points with atomic helpers, and then even plane_disable
>>>>>>>>>>> will be a full atomic modeset.
>>>>>>>>>>>
>>>>>>>>>>> What did fall apart with just touching properties/planes now?
>>>>>>>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
>>>>>>>>>> if a modeset is needed after the first atomic call. Right now because
>>>>>>>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
>>>>>>>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
>>>>>>>>>> and if the final mode is different this will introduce a double modeset.
>>>>>>>>> For expensive properties (i.e. a no-op changes causes something that takes
>>>>>>>>> time like modeset or vblank wait) we need to make sure we filter them out
>>>>>>>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
>>>>>>>>> the existing legacy set_prop functions should all filter out no-op changes
>>>>>>>>> themselves. If we don't do that for rotation then that's a bug.
>>>>>>>>>
>>>>>>>>> Same for disabling planes harder, that shouldn't take time. Especially
>>>>>>>>> since fbcon only force-disable non-primary plane, and for driver load
>>>>>>>>> that's the exact thing we already do in the driver anyway.
>>>>>>>> Something like this?
>>>>>>>> ---
>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>>>> index a1d4e13f3908..2989232f4996 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>>>> @@ -30,6 +30,7 @@
>>>>>>>>  #include <drm/drm_plane_helper.h>
>>>>>>>>  #include <drm/drm_crtc_helper.h>
>>>>>>>>  #include <drm/drm_atomic_helper.h>
>>>>>>>> +#include "drm_crtc_internal.h"
>>>>>>>>  #include <linux/fence.h>
>>>>>>>>  
>>>>>>>>  /**
>>>>>>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>>>>>>>>  {
>>>>>>>>  	struct drm_atomic_state *state;
>>>>>>>>  	struct drm_crtc_state *crtc_state;
>>>>>>>> -	int ret = 0;
>>>>>>>> +	uint64_t retval;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
>>>>>>>> +	if (!ret && val == retval)
>>>>>>>> +		return 0;
>>>>>>>>  
>>>>>>>>  	state = drm_atomic_state_alloc(crtc->dev);
>>>>>>>>  	if (!state)
>>>>>>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>>>>>>>>  {
>>>>>>>>  	struct drm_atomic_state *state;
>>>>>>>>  	struct drm_plane_state *plane_state;
>>>>>>>> -	int ret = 0;
>>>>>>>> +	uint64_t retval;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
>>>>>>>> +	if (!ret && val == retval)
>>>>>>>> +		return 0;
>>>>>>>>  
>>>>>>>>  	state = drm_atomic_state_alloc(plane->dev);
>>>>>>>>  	if (!state)
>>>>>>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>>>>>>>>  {
>>>>>>>>  	struct drm_atomic_state *state;
>>>>>>>>  	struct drm_connector_state *connector_state;
>>>>>>>> -	int ret = 0;
>>>>>>>> +	uint64_t retval;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
>>>>>>>> +	if (!ret && val == retval)
>>>>>>>> +		return 0;
>>>>>>>>  
>>>>>>>>  	state = drm_atomic_state_alloc(connector->dev);
>>>>>>>>  	if (!state)
>>>>>>> The reason I didn't do this is that a prop change might still result in no
>>>>>>> hw state change (e.g. if you go automitic->explicit setting matching
>>>>>>> automatic one). Hence I think we need to solve this in lower levels
>>>>>>> anyway, i.e. in when computing the config. But it shouldn't cause trouble
>>>>>>> yet.
>>>>>> Is that a ack or nack?
>>>>> I think we shouldn't need this really for i915, and it might cover up
>>>>> bugs. I prefer we just do the evade modeset logic you've implemented once
>>>>> we switch over to atomic props. Since atm we only have atomic props which
>>>>> get updated in pageflips we shouldn't have serious problems here yet (for
>>>>> setting the rotation prop to 0° again when fbdev starts up).
>>>>>
>>>>> Or do I miss something still here?
>>>> Yes, if the hardware mode is incompatible with its calculated sw mode,
>>>> and we set a different mode from fbdev you get 2 modesets instead of 1.
>>> How does that happen? For setting the rotation property we should just
>>> duplicate the current crtc state. Since there's no mode changing (they
>>> should match perfectly no matter how botched the reconstruction is) there
>>> shouldn't be any need to recompute the config completely and discover that
>>> there's a mismatch. Which means we'll just do the plane update (which
>>> might do a few silly mmios but shouldn't block) and that's it.
>>>
>>> At least that's what I'd expect - where does this fall apart?
>> If crtc is active and primary fb visible, and converted to atomic:
>>
>> restore_fbdev_mode() ->
>> 	drm_mode_plane_set_obj_prop() ->
>> 		drm_atomic_helper_plane_set_property() ->
>> 			drm_atomic_get_plane_state() ->
>> 				drm_atomic_get_crtc_state()
>> crtc state is part of the state, intel_modeset_pipe_config performs
>> the initial check if modeset's needed. Lets assume yes:
> "Let's assume yes" -> that's imo a bug, so where does this happen so that
> we can fix it? Disabling a plane or setting a plane prop really shouldn't
> result in a modeset. Well at least if it's not a plane prop that does
> required a modeset (but I don't think we have any of those).
From a driver point of view you wouldn't be able to distinguish it from a real modeset to the same mode. :(
In both cases you have all planes added and the crtc.

Thinking about it more there will be 1 thing saving us from a modeset,
drm_atomic_crtc_check will reject enable without mode_blob for atomic drivers,
so until the first mode is set all atomic updates to the crtc will be rejected.

Unfortunately you will still get WARN_ON's for this, so a better solution's needed.

~Maarten

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

Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Mon, Jul 13, 2015 at 11:23:45AM +0200, Maarten Lankhorst wrote:
> Op 13-07-15 om 11:13 schreef Daniel Vetter:
> > On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
> >> Op 08-07-15 om 22:12 schreef Daniel Vetter:
> >>> On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
> >>>> Op 08-07-15 om 19:52 schreef Daniel Vetter:
> >>>>> On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 08-07-15 om 10:55 schreef Daniel Vetter:
> >>>>>>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
> >>>>>>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
> >>>>>>>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
> >>>>>>>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> >>>>>>>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
> >>>>>>>>>>>>>> which is useful for forcing a recalculation.
> >>>>>>>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
> >>>>>>>>>>>>> way. What exactly do you need this for, what's the implications?
> >>>>>>>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> >>>>>>>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
> >>>>>>>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> >>>>>>>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
> >>>>>>>>>>> the legacy entry points with atomic helpers, and then even plane_disable
> >>>>>>>>>>> will be a full atomic modeset.
> >>>>>>>>>>>
> >>>>>>>>>>> What did fall apart with just touching properties/planes now?
> >>>>>>>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
> >>>>>>>>>> if a modeset is needed after the first atomic call. Right now because
> >>>>>>>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
> >>>>>>>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
> >>>>>>>>>> and if the final mode is different this will introduce a double modeset.
> >>>>>>>>> For expensive properties (i.e. a no-op changes causes something that takes
> >>>>>>>>> time like modeset or vblank wait) we need to make sure we filter them out
> >>>>>>>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
> >>>>>>>>> the existing legacy set_prop functions should all filter out no-op changes
> >>>>>>>>> themselves. If we don't do that for rotation then that's a bug.
> >>>>>>>>>
> >>>>>>>>> Same for disabling planes harder, that shouldn't take time. Especially
> >>>>>>>>> since fbcon only force-disable non-primary plane, and for driver load
> >>>>>>>>> that's the exact thing we already do in the driver anyway.
> >>>>>>>> Something like this?
> >>>>>>>> ---
> >>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>>>> index a1d4e13f3908..2989232f4996 100644
> >>>>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>>>> @@ -30,6 +30,7 @@
> >>>>>>>>  #include <drm/drm_plane_helper.h>
> >>>>>>>>  #include <drm/drm_crtc_helper.h>
> >>>>>>>>  #include <drm/drm_atomic_helper.h>
> >>>>>>>> +#include "drm_crtc_internal.h"
> >>>>>>>>  #include <linux/fence.h>
> >>>>>>>>  
> >>>>>>>>  /**
> >>>>>>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
> >>>>>>>>  {
> >>>>>>>>  	struct drm_atomic_state *state;
> >>>>>>>>  	struct drm_crtc_state *crtc_state;
> >>>>>>>> -	int ret = 0;
> >>>>>>>> +	uint64_t retval;
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
> >>>>>>>> +	if (!ret && val == retval)
> >>>>>>>> +		return 0;
> >>>>>>>>  
> >>>>>>>>  	state = drm_atomic_state_alloc(crtc->dev);
> >>>>>>>>  	if (!state)
> >>>>>>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
> >>>>>>>>  {
> >>>>>>>>  	struct drm_atomic_state *state;
> >>>>>>>>  	struct drm_plane_state *plane_state;
> >>>>>>>> -	int ret = 0;
> >>>>>>>> +	uint64_t retval;
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
> >>>>>>>> +	if (!ret && val == retval)
> >>>>>>>> +		return 0;
> >>>>>>>>  
> >>>>>>>>  	state = drm_atomic_state_alloc(plane->dev);
> >>>>>>>>  	if (!state)
> >>>>>>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
> >>>>>>>>  {
> >>>>>>>>  	struct drm_atomic_state *state;
> >>>>>>>>  	struct drm_connector_state *connector_state;
> >>>>>>>> -	int ret = 0;
> >>>>>>>> +	uint64_t retval;
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
> >>>>>>>> +	if (!ret && val == retval)
> >>>>>>>> +		return 0;
> >>>>>>>>  
> >>>>>>>>  	state = drm_atomic_state_alloc(connector->dev);
> >>>>>>>>  	if (!state)
> >>>>>>> The reason I didn't do this is that a prop change might still result in no
> >>>>>>> hw state change (e.g. if you go automitic->explicit setting matching
> >>>>>>> automatic one). Hence I think we need to solve this in lower levels
> >>>>>>> anyway, i.e. in when computing the config. But it shouldn't cause trouble
> >>>>>>> yet.
> >>>>>> Is that a ack or nack?
> >>>>> I think we shouldn't need this really for i915, and it might cover up
> >>>>> bugs. I prefer we just do the evade modeset logic you've implemented once
> >>>>> we switch over to atomic props. Since atm we only have atomic props which
> >>>>> get updated in pageflips we shouldn't have serious problems here yet (for
> >>>>> setting the rotation prop to 0° again when fbdev starts up).
> >>>>>
> >>>>> Or do I miss something still here?
> >>>> Yes, if the hardware mode is incompatible with its calculated sw mode,
> >>>> and we set a different mode from fbdev you get 2 modesets instead of 1.
> >>> How does that happen? For setting the rotation property we should just
> >>> duplicate the current crtc state. Since there's no mode changing (they
> >>> should match perfectly no matter how botched the reconstruction is) there
> >>> shouldn't be any need to recompute the config completely and discover that
> >>> there's a mismatch. Which means we'll just do the plane update (which
> >>> might do a few silly mmios but shouldn't block) and that's it.
> >>>
> >>> At least that's what I'd expect - where does this fall apart?
> >> If crtc is active and primary fb visible, and converted to atomic:
> >>
> >> restore_fbdev_mode() ->
> >> 	drm_mode_plane_set_obj_prop() ->
> >> 		drm_atomic_helper_plane_set_property() ->
> >> 			drm_atomic_get_plane_state() ->
> >> 				drm_atomic_get_crtc_state()
> >> crtc state is part of the state, intel_modeset_pipe_config performs
> >> the initial check if modeset's needed. Lets assume yes:
> > "Let's assume yes" -> that's imo a bug, so where does this happen so that
> > we can fix it? Disabling a plane or setting a plane prop really shouldn't
> > result in a modeset. Well at least if it's not a plane prop that does
> > required a modeset (but I don't think we have any of those).
> From a driver point of view you wouldn't be able to distinguish it from a real modeset to the same mode. :(
> In both cases you have all planes added and the crtc.
> 
> Thinking about it more there will be 1 thing saving us from a modeset,
> drm_atomic_crtc_check will reject enable without mode_blob for atomic drivers,
> so until the first mode is set all atomic updates to the crtc will be rejected.
> 
> Unfortunately you will still get WARN_ON's for this, so a better solution's needed.

Ok I think I start to grasp what's wrong, the trouble is that we don't
have the mode stuff fully set up yet (which is part of fastboot), which
means we'll get a bogus crtc_state->mode_changed despite that nothing
really changed. Ugly.

Could we insert a dummy mode_blob to avoid the WARNs and the bogus
mode_changed instead? The problem really is that doing this here is just
plugging the one source of troubles you're seeing right now (fbcon), the
initial set_* calls could come from anything really in any order. So we
really better be able to cope.

Even converting fbdev to have an optional atomic patch for DRIVER_ATOMIC
(unsafe i915 options aren't a concern here for me) won't fix this.
-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 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Maarten Lankhorst]

Op 13-07-15 om 11:45 schreef Daniel Vetter:
> On Mon, Jul 13, 2015 at 11:23:45AM +0200, Maarten Lankhorst wrote:
>> Op 13-07-15 om 11:13 schreef Daniel Vetter:
>>> On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
>>>> Op 08-07-15 om 22:12 schreef Daniel Vetter:
>>>>> On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
>>>>>> Op 08-07-15 om 19:52 schreef Daniel Vetter:
>>>>>>> On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
>>>>>>>> Op 08-07-15 om 10:55 schreef Daniel Vetter:
>>>>>>>>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
>>>>>>>>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
>>>>>>>>>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
>>>>>>>>>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
>>>>>>>>>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
>>>>>>>>>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
>>>>>>>>>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
>>>>>>>>>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
>>>>>>>>>>>>>>>> which is useful for forcing a recalculation.
>>>>>>>>>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
>>>>>>>>>>>>>>> way. What exactly do you need this for, what's the implications?
>>>>>>>>>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
>>>>>>>>>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
>>>>>>>>>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
>>>>>>>>>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
>>>>>>>>>>>>> the legacy entry points with atomic helpers, and then even plane_disable
>>>>>>>>>>>>> will be a full atomic modeset.
>>>>>>>>>>>>>
>>>>>>>>>>>>> What did fall apart with just touching properties/planes now?
>>>>>>>>>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
>>>>>>>>>>>> if a modeset is needed after the first atomic call. Right now because
>>>>>>>>>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
>>>>>>>>>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
>>>>>>>>>>>> and if the final mode is different this will introduce a double modeset.
>>>>>>>>>>> For expensive properties (i.e. a no-op changes causes something that takes
>>>>>>>>>>> time like modeset or vblank wait) we need to make sure we filter them out
>>>>>>>>>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
>>>>>>>>>>> the existing legacy set_prop functions should all filter out no-op changes
>>>>>>>>>>> themselves. If we don't do that for rotation then that's a bug.
>>>>>>>>>>>
>>>>>>>>>>> Same for disabling planes harder, that shouldn't take time. Especially
>>>>>>>>>>> since fbcon only force-disable non-primary plane, and for driver load
>>>>>>>>>>> that's the exact thing we already do in the driver anyway.
>>>>>>>>>> Something like this?
>>>>>>>>>> ---
>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>>>>>> index a1d4e13f3908..2989232f4996 100644
>>>>>>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>>>>>> @@ -30,6 +30,7 @@
>>>>>>>>>>  #include <drm/drm_plane_helper.h>
>>>>>>>>>>  #include <drm/drm_crtc_helper.h>
>>>>>>>>>>  #include <drm/drm_atomic_helper.h>
>>>>>>>>>> +#include "drm_crtc_internal.h"
>>>>>>>>>>  #include <linux/fence.h>
>>>>>>>>>>  
>>>>>>>>>>  /**
>>>>>>>>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>>>>>>>>>>  {
>>>>>>>>>>  	struct drm_atomic_state *state;
>>>>>>>>>>  	struct drm_crtc_state *crtc_state;
>>>>>>>>>> -	int ret = 0;
>>>>>>>>>> +	uint64_t retval;
>>>>>>>>>> +	int ret;
>>>>>>>>>> +
>>>>>>>>>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
>>>>>>>>>> +	if (!ret && val == retval)
>>>>>>>>>> +		return 0;
>>>>>>>>>>  
>>>>>>>>>>  	state = drm_atomic_state_alloc(crtc->dev);
>>>>>>>>>>  	if (!state)
>>>>>>>>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>>>>>>>>>>  {
>>>>>>>>>>  	struct drm_atomic_state *state;
>>>>>>>>>>  	struct drm_plane_state *plane_state;
>>>>>>>>>> -	int ret = 0;
>>>>>>>>>> +	uint64_t retval;
>>>>>>>>>> +	int ret;
>>>>>>>>>> +
>>>>>>>>>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
>>>>>>>>>> +	if (!ret && val == retval)
>>>>>>>>>> +		return 0;
>>>>>>>>>>  
>>>>>>>>>>  	state = drm_atomic_state_alloc(plane->dev);
>>>>>>>>>>  	if (!state)
>>>>>>>>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>>>>>>>>>>  {
>>>>>>>>>>  	struct drm_atomic_state *state;
>>>>>>>>>>  	struct drm_connector_state *connector_state;
>>>>>>>>>> -	int ret = 0;
>>>>>>>>>> +	uint64_t retval;
>>>>>>>>>> +	int ret;
>>>>>>>>>> +
>>>>>>>>>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
>>>>>>>>>> +	if (!ret && val == retval)
>>>>>>>>>> +		return 0;
>>>>>>>>>>  
>>>>>>>>>>  	state = drm_atomic_state_alloc(connector->dev);
>>>>>>>>>>  	if (!state)
>>>>>>>>> The reason I didn't do this is that a prop change might still result in no
>>>>>>>>> hw state change (e.g. if you go automitic->explicit setting matching
>>>>>>>>> automatic one). Hence I think we need to solve this in lower levels
>>>>>>>>> anyway, i.e. in when computing the config. But it shouldn't cause trouble
>>>>>>>>> yet.
>>>>>>>> Is that a ack or nack?
>>>>>>> I think we shouldn't need this really for i915, and it might cover up
>>>>>>> bugs. I prefer we just do the evade modeset logic you've implemented once
>>>>>>> we switch over to atomic props. Since atm we only have atomic props which
>>>>>>> get updated in pageflips we shouldn't have serious problems here yet (for
>>>>>>> setting the rotation prop to 0° again when fbdev starts up).
>>>>>>>
>>>>>>> Or do I miss something still here?
>>>>>> Yes, if the hardware mode is incompatible with its calculated sw mode,
>>>>>> and we set a different mode from fbdev you get 2 modesets instead of 1.
>>>>> How does that happen? For setting the rotation property we should just
>>>>> duplicate the current crtc state. Since there's no mode changing (they
>>>>> should match perfectly no matter how botched the reconstruction is) there
>>>>> shouldn't be any need to recompute the config completely and discover that
>>>>> there's a mismatch. Which means we'll just do the plane update (which
>>>>> might do a few silly mmios but shouldn't block) and that's it.
>>>>>
>>>>> At least that's what I'd expect - where does this fall apart?
>>>> If crtc is active and primary fb visible, and converted to atomic:
>>>>
>>>> restore_fbdev_mode() ->
>>>> 	drm_mode_plane_set_obj_prop() ->
>>>> 		drm_atomic_helper_plane_set_property() ->
>>>> 			drm_atomic_get_plane_state() ->
>>>> 				drm_atomic_get_crtc_state()
>>>> crtc state is part of the state, intel_modeset_pipe_config performs
>>>> the initial check if modeset's needed. Lets assume yes:
>>> "Let's assume yes" -> that's imo a bug, so where does this happen so that
>>> we can fix it? Disabling a plane or setting a plane prop really shouldn't
>>> result in a modeset. Well at least if it's not a plane prop that does
>>> required a modeset (but I don't think we have any of those).
>> From a driver point of view you wouldn't be able to distinguish it from a real modeset to the same mode. :(
>> In both cases you have all planes added and the crtc.
>>
>> Thinking about it more there will be 1 thing saving us from a modeset,
>> drm_atomic_crtc_check will reject enable without mode_blob for atomic drivers,
>> so until the first mode is set all atomic updates to the crtc will be rejected.
>>
>> Unfortunately you will still get WARN_ON's for this, so a better solution's needed.
> Ok I think I start to grasp what's wrong, the trouble is that we don't
> have the mode stuff fully set up yet (which is part of fastboot), which
> means we'll get a bogus crtc_state->mode_changed despite that nothing
> really changed. Ugly.
No, mode_changed would be harmless, with proper skip modeset support it can be converted to a noop.
> Could we insert a dummy mode_blob to avoid the WARNs and the bogus
> mode_changed instead? The problem really is that doing this here is just
> plugging the one source of troubles you're seeing right now (fbcon), the
> initial set_* calls could come from anything really in any order. So we
> really better be able to cope.
Doesn't this mean we should set a real mode read out from hw state instead?

> Even converting fbdev to have an optional atomic patch for DRIVER_ATOMIC
> (unsafe i915 options aren't a concern here for me) won't fix this.

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

Re: [Intel-gfx] [PATCH v2 02/20] drm: Don't update plane properties for atomic planes if it stays the same

[Daniel Vetter]

On Mon, Jul 13, 2015 at 11:49:01AM +0200, Maarten Lankhorst wrote:
> Op 13-07-15 om 11:45 schreef Daniel Vetter:
> > On Mon, Jul 13, 2015 at 11:23:45AM +0200, Maarten Lankhorst wrote:
> >> Op 13-07-15 om 11:13 schreef Daniel Vetter:
> >>> On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
> >>>> Op 08-07-15 om 22:12 schreef Daniel Vetter:
> >>>>> On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 08-07-15 om 19:52 schreef Daniel Vetter:
> >>>>>>> On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
> >>>>>>>> Op 08-07-15 om 10:55 schreef Daniel Vetter:
> >>>>>>>>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
> >>>>>>>>>>> On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>>>> Op 07-07-15 om 14:10 schreef Daniel Vetter:
> >>>>>>>>>>>>> On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>>>>>> Op 07-07-15 om 11:18 schreef Daniel Vetter:
> >>>>>>>>>>>>>>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote:
> >>>>>>>>>>>>>>>> This allows the first atomic call during hw init to be a real modeset,
> >>>>>>>>>>>>>>>> which is useful for forcing a recalculation.
> >>>>>>>>>>>>>>> fbcon is optional, you can't rely on anything being done in any specific
> >>>>>>>>>>>>>>> way. What exactly do you need this for, what's the implications?
> >>>>>>>>>>>>>> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout.
> >>>>>>>>>>>>>> I want the first function to be the modeset, so we have a sane base to commit changes on.
> >>>>>>>>>>>>>> Ideally this whole function would have a atomic counterpart which does it in one go. :)
> >>>>>>>>>>>>> Yeah. Otoh as soon as we have atomic modeset working we can replace all
> >>>>>>>>>>>>> the legacy entry points with atomic helpers, and then even plane_disable
> >>>>>>>>>>>>> will be a full atomic modeset.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> What did fall apart with just touching properties/planes now?
> >>>>>>>>>>>> Also when i915 is fully atomic it calculates in intel_modeset_compute_config
> >>>>>>>>>>>> if a modeset is needed after the first atomic call. Right now because
> >>>>>>>>>>>> intel_modeset_compute_config is only called in set_config so this works as expected.
> >>>>>>>>>>>> Otherwise drm_plane_force_disable or rotate_0 will force a modeset,
> >>>>>>>>>>>> and if the final mode is different this will introduce a double modeset.
> >>>>>>>>>>> For expensive properties (i.e. a no-op changes causes something that takes
> >>>>>>>>>>> time like modeset or vblank wait) we need to make sure we filter them out
> >>>>>>>>>>> in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile
> >>>>>>>>>>> the existing legacy set_prop functions should all filter out no-op changes
> >>>>>>>>>>> themselves. If we don't do that for rotation then that's a bug.
> >>>>>>>>>>>
> >>>>>>>>>>> Same for disabling planes harder, that shouldn't take time. Especially
> >>>>>>>>>>> since fbcon only force-disable non-primary plane, and for driver load
> >>>>>>>>>>> that's the exact thing we already do in the driver anyway.
> >>>>>>>>>> Something like this?
> >>>>>>>>>> ---
> >>>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>>>>>> index a1d4e13f3908..2989232f4996 100644
> >>>>>>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>>>>>> @@ -30,6 +30,7 @@
> >>>>>>>>>>  #include <drm/drm_plane_helper.h>
> >>>>>>>>>>  #include <drm/drm_crtc_helper.h>
> >>>>>>>>>>  #include <drm/drm_atomic_helper.h>
> >>>>>>>>>> +#include "drm_crtc_internal.h"
> >>>>>>>>>>  #include <linux/fence.h>
> >>>>>>>>>>  
> >>>>>>>>>>  /**
> >>>>>>>>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
> >>>>>>>>>>  {
> >>>>>>>>>>  	struct drm_atomic_state *state;
> >>>>>>>>>>  	struct drm_crtc_state *crtc_state;
> >>>>>>>>>> -	int ret = 0;
> >>>>>>>>>> +	uint64_t retval;
> >>>>>>>>>> +	int ret;
> >>>>>>>>>> +
> >>>>>>>>>> +	ret = drm_atomic_get_property(&crtc->base, property, &retval);
> >>>>>>>>>> +	if (!ret && val == retval)
> >>>>>>>>>> +		return 0;
> >>>>>>>>>>  
> >>>>>>>>>>  	state = drm_atomic_state_alloc(crtc->dev);
> >>>>>>>>>>  	if (!state)
> >>>>>>>>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct drm_plane *plane,
> >>>>>>>>>>  {
> >>>>>>>>>>  	struct drm_atomic_state *state;
> >>>>>>>>>>  	struct drm_plane_state *plane_state;
> >>>>>>>>>> -	int ret = 0;
> >>>>>>>>>> +	uint64_t retval;
> >>>>>>>>>> +	int ret;
> >>>>>>>>>> +
> >>>>>>>>>> +	ret = drm_atomic_get_property(&plane->base, property, &retval);
> >>>>>>>>>> +	if (!ret && val == retval)
> >>>>>>>>>> +		return 0;
> >>>>>>>>>>  
> >>>>>>>>>>  	state = drm_atomic_state_alloc(plane->dev);
> >>>>>>>>>>  	if (!state)
> >>>>>>>>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
> >>>>>>>>>>  {
> >>>>>>>>>>  	struct drm_atomic_state *state;
> >>>>>>>>>>  	struct drm_connector_state *connector_state;
> >>>>>>>>>> -	int ret = 0;
> >>>>>>>>>> +	uint64_t retval;
> >>>>>>>>>> +	int ret;
> >>>>>>>>>> +
> >>>>>>>>>> +	ret = drm_atomic_get_property(&connector->base, property, &retval);
> >>>>>>>>>> +	if (!ret && val == retval)
> >>>>>>>>>> +		return 0;
> >>>>>>>>>>  
> >>>>>>>>>>  	state = drm_atomic_state_alloc(connector->dev);
> >>>>>>>>>>  	if (!state)
> >>>>>>>>> The reason I didn't do this is that a prop change might still result in no
> >>>>>>>>> hw state change (e.g. if you go automitic->explicit setting matching
> >>>>>>>>> automatic one). Hence I think we need to solve this in lower levels
> >>>>>>>>> anyway, i.e. in when computing the config. But it shouldn't cause trouble
> >>>>>>>>> yet.
> >>>>>>>> Is that a ack or nack?
> >>>>>>> I think we shouldn't need this really for i915, and it might cover up
> >>>>>>> bugs. I prefer we just do the evade modeset logic you've implemented once
> >>>>>>> we switch over to atomic props. Since atm we only have atomic props which
> >>>>>>> get updated in pageflips we shouldn't have serious problems here yet (for
> >>>>>>> setting the rotation prop to 0° again when fbdev starts up).
> >>>>>>>
> >>>>>>> Or do I miss something still here?
> >>>>>> Yes, if the hardware mode is incompatible with its calculated sw mode,
> >>>>>> and we set a different mode from fbdev you get 2 modesets instead of 1.
> >>>>> How does that happen? For setting the rotation property we should just
> >>>>> duplicate the current crtc state. Since there's no mode changing (they
> >>>>> should match perfectly no matter how botched the reconstruction is) there
> >>>>> shouldn't be any need to recompute the config completely and discover that
> >>>>> there's a mismatch. Which means we'll just do the plane update (which
> >>>>> might do a few silly mmios but shouldn't block) and that's it.
> >>>>>
> >>>>> At least that's what I'd expect - where does this fall apart?
> >>>> If crtc is active and primary fb visible, and converted to atomic:
> >>>>
> >>>> restore_fbdev_mode() ->
> >>>> 	drm_mode_plane_set_obj_prop() ->
> >>>> 		drm_atomic_helper_plane_set_property() ->
> >>>> 			drm_atomic_get_plane_state() ->
> >>>> 				drm_atomic_get_crtc_state()
> >>>> crtc state is part of the state, intel_modeset_pipe_config performs
> >>>> the initial check if modeset's needed. Lets assume yes:
> >>> "Let's assume yes" -> that's imo a bug, so where does this happen so that
> >>> we can fix it? Disabling a plane or setting a plane prop really shouldn't
> >>> result in a modeset. Well at least if it's not a plane prop that does
> >>> required a modeset (but I don't think we have any of those).
> >> From a driver point of view you wouldn't be able to distinguish it from a real modeset to the same mode. :(
> >> In both cases you have all planes added and the crtc.
> >>
> >> Thinking about it more there will be 1 thing saving us from a modeset,
> >> drm_atomic_crtc_check will reject enable without mode_blob for atomic drivers,
> >> so until the first mode is set all atomic updates to the crtc will be rejected.
> >>
> >> Unfortunately you will still get WARN_ON's for this, so a better solution's needed.
> > Ok I think I start to grasp what's wrong, the trouble is that we don't
> > have the mode stuff fully set up yet (which is part of fastboot), which
> > means we'll get a bogus crtc_state->mode_changed despite that nothing
> > really changed. Ugly.
> No, mode_changed would be harmless, with proper skip modeset support it can be converted to a noop.
> > Could we insert a dummy mode_blob to avoid the WARNs and the bogus
> > mode_changed instead? The problem really is that doing this here is just
> > plugging the one source of troubles you're seeing right now (fbcon), the
> > initial set_* calls could come from anything really in any order. So we
> > really better be able to cope.
> Doesn't this mean we should set a real mode read out from hw state instead?

Yeah I guess so. We simply need to make sure that we have a mismatch.
Your DRIVER_MODE approach seems like it should work out.
-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