Re: [PATCH 07/24] drm/i915: Introduce intel_atomic_get_plane_state_after_check()

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 10-10-2019 om 14:39 schreef Ville Syrjälä:
> On Thu, Oct 10, 2019 at 01:56:42PM +0200, Maarten Lankhorst wrote:
>> Op 08-10-2019 om 19:03 schreef Ville Syrjälä:
>>> On Fri, Oct 04, 2019 at 01:34:57PM +0200, Maarten Lankhorst wrote:
>>>> Use this in all the places where we try to acquire planes after the planes
>>>> atomic_check().
>>>>
>>>> In case of intel_modeset_all_pipes() this is not yet done after atomic_check,
>>>> but seems like it will be in the future. To add some paranoia, add all planes
>>>> rather than active planes, because of bigjoiner and planar YUV support having
>>>> extra planes outside of the core's view that wouldn't be added otherwise.
>>> If the plane isn't active what good does adding it do?
>>>
>>> Maybe the only real exception I can think of is the watermarks
>>> and the primary vs. gamma/csc_enable on pre-skl, but those are
>>> already handled correctly.
>> Planar YUV Y planes are not enumerated, so it's useful to add. Instead of
>> typing up special case support I thought it was easier to just add all
>> planes. On bigjoiner slave no planes are enabled either, even if they're
>> active.
> I think once we have the state split we should get rid of all the
> special casing of the Y planes. Then they (and the bigjoiner slave
> planes) should just look like an active planes which is not
> logically enabled by uapi.

With the plane split it's doable, even for watermarks, but prefer to do it later on, not change the world too much. :)

There is no issue in the core with adding planes that are disabled, so I don't see the problem?

~Maarten

>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/display/intel_atomic.c   | 41 +++++++++----------
>>>>  .../gpu/drm/i915/display/intel_atomic_plane.c | 19 +++++++++
>>>>  drivers/gpu/drm/i915/display/intel_cdclk.c    | 15 ++++---
>>>>  drivers/gpu/drm/i915/display/intel_color.c    |  7 ++--
>>>>  .../drm/i915/display/intel_display_types.h    |  6 +++
>>>>  drivers/gpu/drm/i915/intel_pm.c               | 14 ++++---
>>>>  6 files changed, 66 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
>>>> index c5a552a69752..e6cb85d41c8d 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
>>>> @@ -313,13 +313,10 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>>>>  			       struct intel_crtc *intel_crtc,
>>>>  			       struct intel_crtc_state *crtc_state)
>>>>  {
>>>> -	struct drm_plane *plane = NULL;
>>>> -	struct intel_plane *intel_plane;
>>>> -	struct intel_plane_state *plane_state = NULL;
>>>>  	struct intel_crtc_scaler_state *scaler_state =
>>>>  		&crtc_state->scaler_state;
>>>>  	struct drm_atomic_state *drm_state = crtc_state->base.state;
>>>> -	struct intel_atomic_state *intel_state = to_intel_atomic_state(drm_state);
>>>> +	struct intel_atomic_state *state = to_intel_atomic_state(drm_state);
>>>>  	int num_scalers_need;
>>>>  	int i;
>>>>  
>>>> @@ -346,6 +343,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>>>>  
>>>>  	/* walkthrough scaler_users bits and start assigning scalers */
>>>>  	for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
>>>> +		struct intel_plane_state *plane_state = NULL;
>>>>  		int *scaler_id;
>>>>  		const char *name;
>>>>  		int idx;
>>>> @@ -361,19 +359,16 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>>>>  			/* panel fitter case: assign as a crtc scaler */
>>>>  			scaler_id = &scaler_state->scaler_id;
>>>>  		} else {
>>>> -			name = "PLANE";
>>>> +			struct intel_plane *plane;
>>>>  
>>>>  			/* plane scaler case: assign as a plane scaler */
>>>>  			/* find the plane that set the bit as scaler_user */
>>>> -			plane = drm_state->planes[i].ptr;
>>>>  
>>>>  			/*
>>>>  			 * to enable/disable hq mode, add planes that are using scaler
>>>>  			 * into this transaction
>>>>  			 */
>>>> -			if (!plane) {
>>>> -				struct drm_plane_state *state;
>>>> -
>>>> +			if (!drm_state->planes[i].ptr) {
>>>>  				/*
>>>>  				 * GLK+ scalers don't have a HQ mode so it
>>>>  				 * isn't necessary to change between HQ and dyn mode
>>>> @@ -382,24 +377,28 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>>>>  				if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>>>>  					continue;
>>>>  
>>>> -				plane = drm_plane_from_index(&dev_priv->drm, i);
>>>> -				state = drm_atomic_get_plane_state(drm_state, plane);
>>>> -				if (IS_ERR(state)) {
>>>> -					DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
>>>> -						plane->base.id);
>>>> -					return PTR_ERR(state);
>>>> +				plane = to_intel_plane(drm_plane_from_index(&dev_priv->drm, i));
>>>> +				plane_state =
>>>> +					intel_atomic_get_plane_state_after_check(state,
>>>> +										 crtc_state,
>>>> +										 plane);
>>>> +				if (IS_ERR(plane_state)) {
>>>> +					DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state: %li\n",
>>>> +						plane->base.base.id, PTR_ERR(plane_state));
>>>> +					return PTR_ERR(plane_state);
>>>>  				}
>>>> +			} else {
>>>> +				plane = to_intel_plane(drm_state->planes[i].ptr);
>>>> +				plane_state = intel_atomic_get_new_plane_state(state,
>>>> +									       plane);
>>>>  			}
>>>>  
>>>> -			intel_plane = to_intel_plane(plane);
>>>> -			idx = plane->base.id;
>>>> -
>>>>  			/* plane on different crtc cannot be a scaler user of this crtc */
>>>> -			if (WARN_ON(intel_plane->pipe != intel_crtc->pipe))
>>>> +			if (WARN_ON(plane->pipe != intel_crtc->pipe))
>>>>  				continue;
>>>>  
>>>> -			plane_state = intel_atomic_get_new_plane_state(intel_state,
>>>> -								       intel_plane);
>>>> +			name = "PLANE";
>>>> +			idx = plane->base.base.id;
>>>>  			scaler_id = &plane_state->scaler_id;
>>>>  		}
>>>>  
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>> index 98b7766eaa7a..4eaab0de98bf 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>> @@ -366,6 +366,25 @@ void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
>>>>  	}
>>>>  }
>>>>  
>>>> +struct intel_plane_state *
>>>> +intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state,
>>>> +					 struct intel_crtc_state *new_crtc_state,
>>>> +					 struct intel_plane *plane)
>>>> +{
>>>> +	struct intel_plane_state *plane_state =
>>>> +		intel_atomic_get_new_plane_state(state, plane);
>>>> +
>>>> +	if (plane_state)
>>>> +		return plane_state;
>>>> +
>>>> +	plane_state = intel_atomic_get_plane_state(state, plane);
>>>> +	if (IS_ERR(plane_state))
>>>> +		return plane_state;
>>>> +
>>>> +	new_crtc_state->update_planes |= BIT(plane->id);
>>> I also don't really like burying this all the way down. Seems
>>> rather non-obvious from the caller. Also seems wrong to not set
>>> the flag if the plane was already part of the state.
>>>
>>> I'd say we just need to hand roll add_affected_planes() for the
>>> cdclk code. And there is should definitely be sufficient to
>>> add just the active planes.
>>>
>>>> +	return plane_state;
>>>> +}
>>>> +
>>>>  const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
>>>>  	.prepare_fb = intel_prepare_plane_fb,
>>>>  	.cleanup_fb = intel_cleanup_plane_fb,
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>>>> index 43564295b864..42bd02638d32 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>>>> @@ -2249,6 +2249,7 @@ static int intel_modeset_all_pipes(struct intel_atomic_state *state)
>>>>  	 */
>>>>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
>>>>  		struct intel_crtc_state *crtc_state;
>>>> +		struct intel_plane *plane;
>>>>  		int ret;
>>>>  
>>>>  		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
>>>> @@ -2266,12 +2267,14 @@ static int intel_modeset_all_pipes(struct intel_atomic_state *state)
>>>>  		if (ret)
>>>>  			return ret;
>>>>  
>>>> -		ret = drm_atomic_add_affected_planes(&state->base,
>>>> -						     &crtc->base);
>>>> -		if (ret)
>>>> -			return ret;
>>>> -
>>>> -		crtc_state->update_planes |= crtc_state->active_planes;
>>>> +		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
>>>> +			struct intel_plane_state *plane_state =
>>>> +				intel_atomic_get_plane_state_after_check(state,
>>>> +									 crtc_state,
>>>> +									 plane);
>>>> +			if (IS_ERR(plane_state))
>>>> +				return PTR_ERR(plane_state);
>>>> +		}
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>>>> index 9ab34902663e..1e3a623eaf82 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>>>> @@ -1077,11 +1077,12 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state)
>>>>  		if (!need_plane_update(plane, new_crtc_state))
>>>>  			continue;
>>>>  
>>>> -		plane_state = intel_atomic_get_plane_state(state, plane);
>>>> +		plane_state =
>>>> +			intel_atomic_get_plane_state_after_check(state,
>>>> +								 new_crtc_state,
>>>> +								 plane);
>>>>  		if (IS_ERR(plane_state))
>>>>  			return PTR_ERR(plane_state);
>>>> -
>>>> -		new_crtc_state->update_planes |= BIT(plane->id);
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>>>> index 976669f01a8c..526423437f63 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>>>> @@ -1541,4 +1541,10 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
>>>>  	return i915_ggtt_offset(state->vma);
>>>>  }
>>>>  
>>>> +/* intel_atomic_plane.c */
>>>> +struct intel_plane_state *
>>>> +intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state,
>>>> +					 struct intel_crtc_state *crtc_state,
>>>> +					 struct intel_plane *plane);
>>>> +
>>>>  #endif /*  __INTEL_DISPLAY_TYPES_H__ */
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index 53358e33df1b..5e6e54cb22fe 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -5242,11 +5242,12 @@ skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
>>>>  					&new_crtc_state->wm.skl.plane_ddb_uv[plane_id]))
>>>>  			continue;
>>>>  
>>>> -		plane_state = intel_atomic_get_plane_state(state, plane);
>>>> +		plane_state =
>>>> +			intel_atomic_get_plane_state_after_check(state,
>>>> +								 new_crtc_state,
>>>> +								 plane);
>>>>  		if (IS_ERR(plane_state))
>>>>  			return PTR_ERR(plane_state);
>>>> -
>>>> -		new_crtc_state->update_planes |= BIT(plane_id);
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> @@ -5534,11 +5535,12 @@ static int skl_wm_add_affected_planes(struct intel_atomic_state *state,
>>>>  					&new_crtc_state->wm.skl.optimal.planes[plane_id]))
>>>>  			continue;
>>>>  
>>>> -		plane_state = intel_atomic_get_plane_state(state, plane);
>>>> +		plane_state =
>>>> +			intel_atomic_get_plane_state_after_check(state,
>>>> +								 new_crtc_state,
>>>> +								 plane);
>>>>  		if (IS_ERR(plane_state))
>>>>  			return PTR_ERR(plane_state);
>>>> -
>>>> -		new_crtc_state->update_planes |= BIT(plane_id);
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> -- 
>>>> 2.23.0
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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